-
Notifications
You must be signed in to change notification settings - Fork 89
CLOUDP-330675: Reworks atlas auth login
flow
#4038
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
CLOUDP-330675: Reworks atlas auth login
flow
#4038
Conversation
deea26c
to
6cbf561
Compare
6cbf561
to
eeb7bc4
Compare
APIx Bot |
internal/cli/auth/login.go
Outdated
@@ -316,8 +421,10 @@ func LoginBuilder() *cobra.Command { | |||
} | |||
|
|||
cmd.Flags().BoolVar(&opts.IsGov, "gov", false, "Log in to Atlas for Government.") | |||
cmd.Flags().BoolVar(&opts.NoBrowser, "noBrowser", false, "Don't try to open a browser session.") | |||
cmd.Flags().BoolVar(&opts.NoBrowser, "noBrowser", false, "Don't try to open a browser session to authenticate your User Account.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is true, will the CLI use only config files / env variables for auth? Or perhaps a session that is already authenticated? Wondering if we need to be clear about that behavior here.
I think we can remove the word "try" as it isn't clear to me if this is true if it just doesn't open a browser window or if it opens it if specific conditions are met or not met.
cmd.Flags().BoolVar(&opts.NoBrowser, "noBrowser", false, "Don't try to open a browser session to authenticate your User Account.") | |
cmd.Flags().BoolVar(&opts.NoBrowser, "noBrowser", false, "Don't open a browser session to authenticate your User Account.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When --noBrowser=false
, a browser session is opened for users to authenticate through.
When it is true
, the behaviour is that a browser session will not be opened automatically and users will have to access the link manually.
Would this make that behaviour more clear?
cmd.Flags().BoolVar(&opts.NoBrowser, "noBrowser", false, "Don't try to open a browser session to authenticate your User Account.") | |
cmd.Flags().BoolVar(&opts.NoBrowser, "noBrowser", false, "Don't automatically open a browser session.") |
internal/cli/auth/login.go
Outdated
@@ -238,6 +344,9 @@ func (opts *LoginOpts) oauthFlow(ctx context.Context) error { | |||
|
|||
opts.printAuthInstructions(code) | |||
if !askedToOpenBrowser { | |||
fmt.Printf("\nBrowser will be automatically opened in %d seconds...", browserWait) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure to run by Jonny, he designed these changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, will check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified this logic to more closely follow what was outlined in Scope (prompt user to press a key to open browser) and checked w Jonny, he gives his thumbs up on new implementation
@@ -49,20 +51,119 @@ type LoginConfig interface { | |||
ProjectID() string | |||
} | |||
|
|||
type TrackAsker interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while I think this is fine, why have you added this? don't we just disable telemetry on our tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried and was unable to get this to work for testing runApiKeysLogin.
I believe the limitation is that, even if telemetry is disabled, the prompts are still asked and wait for input, resulting in the test failing with err EOF.
This structure also allows us to mock prompt responses (as seen for Test_shouldRetryAuthenticate here) which would allow us to cover more specific cases in future if needed.
Proposed changes
APIKeys logic is taken directly from
atlas config init
. This duplicated code will be removed as a part of CLOUDP-329797--force
. When used, browser flow (User Account) is taken and user will not be prompted on authentication method.Jira ticket: CLOUDP-330675
Checklist
make fmt
and formatted my code