Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a skip_browser argument to make auto-launching of the default browser optional #182

Merged
merged 4 commits into from
Oct 19, 2021

Conversation

dekimsey
Copy link
Contributor

@dekimsey dekimsey commented Oct 14, 2021

Overview

This change allows a user to opt-out of the auto-launching of their
default browser which may already have a logged in OIDC session. Since
SSO can be a bit tricky to logout/switch users, this provides the user
an escape option for more careful handling of the login attempt.

Primary use-case is Firefox Containers or users using separate browser
profiles to manage multiple logins. If one has an existing SSO session in their default browser,
it becomes impossible to get the vault CLI to login to another session.

Design of Change

A command line flag was added to allow users to opt-out.

Related Issues/Pull Requests

None

Contributor Checklist

[x] hashicorp/vault#12833
[ ] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
[x] Backwards compatible

NOTE I was unable to test the change, I could not find any docs on how to test the client side changes here. If I could get some instruction/direction on how to do that, I'd be happy to confirm it works as intended.

This change allows a user to opt-out of the auto-launching of their
default browser which may already have a logged in OIDC session. Since
SSO can be a bit tricky to logout/switch users, this provides the user
an escape option for more careful handling of the login attempt.

Primary use-case is Firefox Containers or users using separate browser
profiles to manage multiple logins.
Copy link

@MasonM MasonM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is a pain point for me too, and I was just about to work on implementing this.

@kalafut
Copy link
Contributor

kalafut commented Oct 15, 2021

I think this looks pretty good and is a useful option. WDYT @austingebauer ?

@austingebauer austingebauer self-requested a review October 15, 2021 15:05
Copy link
Member

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @dekimsey! This is a nice option. Left a couple of comments, but things are looking good.

Also, thanks for opening up the docs PR over in Vault 👍

cli.go Outdated Show resolved Hide resolved
cli.go Outdated Show resolved Hide resolved
Added a comment regarding the implemented interface
Additionally update internal representation and logic of argument to match
Copy link
Member

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dekimsey - One more update to the CLI help text is needed. Otherwise, I tested this out and everything looks good 👍 Thanks again for this contribution.

cli.go Show resolved Hide resolved
@calvn
Copy link
Member

calvn commented Oct 18, 2021

@austingebauer we should also update https://www.vaultproject.io/docs/auth/jwt#oidc-login-cli afterwards

Copy link
Member

@calvn calvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, pending Austin's last comment on the help text!

@austingebauer
Copy link
Member

@calvn - The author has already provided a PR for the docs update: hashicorp/vault#12833. Will review and get that merged too.

minor: Fix whitespacing on the other options to be consistent
@dekimsey dekimsey changed the title Add a -no-launch argument to make auto-launching of the default browser optional Add a skip_browser argument to make auto-launching of the default browser optional Oct 19, 2021
@dekimsey
Copy link
Contributor Author

Should I do the squash/rebase or is that done as part of the merge process?

Copy link
Member

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dekimsey - The squash is handled when merging, so you don't need to do it. I'll take care of merging this and getting it into a Vault release. Thanks again for the contribution!

@austingebauer austingebauer merged commit db876b0 into hashicorp:master Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants