Add UserAccountConnect login option (OAuth authorization code flow) - #4634
Add UserAccountConnect login option (OAuth authorization code flow)#4634jricher wants to merge 29 commits into
Conversation
|
This PR has gone 30 days without any activity and meets the project’s definition of "stale". This will be auto-closed if there is no new activity over the next 30 days. If the issue is still relevant and active, you can simply comment with a "bump" to keep it open, or add the label "not_stale". Thanks for keeping our repository healthy! |
|
@jricher Might it be possible for to you break this down into a stack of self-contained changes? The size of this diff might slow down (manual) reviews. |
|
@saisundar I'm honestly not sure what smaller set of changes would be meaningful here, as this is an entirely new authentication and api connection path being added, and that cascades throughout the code. What functions do you think would be separable? |
saisundar
left a comment
saisundar
left a comment
There was a problem hiding this comment.
Please add a note on the failing checks and why they are a no-op when fixed.
| return nil, errors.New("discovery response missing metadata") | ||
| } | ||
|
|
||
| func (opts *UserDelegationFlow) Run(ctx context.Context) error { |
There was a problem hiding this comment.
This part, a good chunk of the PKCE and persistence piece, seems to be not be covered by tests. Could we please add some to gain confidence especially on some of the side effects?
| msg := "Connected to MongoDB Atlas" | ||
|
|
||
| if opts.tokenExpiry != "" { | ||
| if t, err := time.Parse(time.RFC3339, opts.tokenExpiry); err == nil { |
There was a problem hiding this comment.
Should these have the same else fallbacks as the empty token expiry?
There was a problem hiding this comment.
No, as they don't have the same auto-refresh capabilities wired in to the transport layer that this path does.
| authTypeOptions = []string{userAccountAuth, prompt.UserDelegationAuth, prompt.ServiceAccountAuth, prompt.APIKeysAuth} | ||
| authTypeDescription = map[string]string{ | ||
| userAccountAuth: "(best for getting started)", | ||
| userAccountAuth: "(legacy account connection)", |
There was a problem hiding this comment.
I realize this is what we want to call out when things have stabilized, and verified as working cleanly - does this wording change (legacy vs “best for user accounts”) need to ship as part of this release?
There was a problem hiding this comment.
This label needs feedback from the CLI team (as does the rest of the user-facing components, like the callback page and paste-helper page) before release.
There was a problem hiding this comment.
This major change would ideally be accompanied by a
- rollback strategy and a rollout strategy. Could you link one ( can't find it in the linked tickets or TD/Scope) ?
There is the technical capability, and the product phrasing.
Phrasing this yet-to-be-validated-by-prod-traffic path as
" prompt.UserDelegationAuth: "(best for user accounts)",
during its v0 commit is heavy-handed.
a) the refresher_opts.go file can be independent. Figuring out the DAG and then shipping the innermost independent PRs ( kinda like reverse toplogical sort) is usually the way to go. you can always do b) c) a) too - but this reverse topological sorting helps make reviewing faster. |
Wires up the connect command as a skeleton for the new dedicated OAuth AS path. Sets auth_type=user_delegation on the profile but does not yet implement the actual OAuth flow. Includes hidden root-level shortcut (atlas connect) matching the pattern of existing auth commands. Assisted-by: Claude Code (claude-opus-4-6)
The connect command fetches and caches OAuth AS metadata via RFC 8414 discovery on first run. Subsequent runs use cached metadata until expiry. Metadata is stored as-is from the server with a separate cache expiry key. Assisted-by: Claude Code (claude-opus-4-6)
Orchestrates the full OAuth authorization code flow with PKCE: discovers AS metadata (with issuer-based cache validation), generates and validates a state parameter for CSRF protection, starts a loopback callback server, opens the browser to the authorization endpoint, waits for the code, exchanges it for tokens, and stores the result as a user_delegation session. Assisted-by: Claude Code (claude-opus-4-6)
…mand Cache is invalidated when the issuer in cached metadata does not match the configured auth server URL. ConnectConfig interface extended with ClientID and AuthServerURL for profile-based overrides. Assisted-by: Claude Code (claude-opus-4-6)
Connect command now persists the token expiry from the token response. Organizations create validates UserDelegation the same as UserAccount since both are OAuth-based user sessions. Assisted-by: Claude Code (claude-opus-4-6)
Shows connection status with token expiry and refresh availability instead of user identity, since the access token is not parsed for UserDelegation sessions. Legacy auth types retain existing behavior. Assisted-by: Claude Code (claude-opus-4-6)
Clears the cached metadata and saves before proceeding, so a failed discovery leaves an empty cache rather than a stale one. Assisted-by: Claude Code (claude-opus-4-6)
Revokes the refresh token at the discovered revocation endpoint via RevokeAuthServerToken. Reads metadata from the profile to find the endpoint. ConfigDeleter extended with Service, AuthServerURL, and AuthServerMetadata for FlowForAuthIssuer access. Assisted-by: Claude Code (claude-opus-4-6)
Groups with APIKeys and ServiceAccount since the authServerTransport handles token refresh transparently during API calls. Assisted-by: Claude Code (claude-opus-4-6)
When --noBrowser is set, prints the authorization URL for the user to visit manually. After approving, the user pastes the redirect URL back into the CLI. Browser path no longer displays the URL unless the browser fails to open. Assisted-by: Claude Code (claude-opus-4-6)
The file was committed without running gofmt over it, so the field column was off by a tab. No code change. Assisted-by: Claude Code (claude-opus-4-6)
Earlier commits on this branch added Service, AuthServerURL, and AuthServerMetadata to ConfigDeleter without regenerating the mock, breaking go vet on the package. Assisted-by: Claude Code (claude-opus-4-6)
The comments before GeneratePKCE/GenerateState and ExchangeCode restated the following function names. The Browser-flow branch comment now names the mode from the user's perspective, matching the Manual-flow branch. Assisted-by: Claude Code (claude-opus-4-7)
ParseCodeFromRedirectURL now takes an io.Reader; the connect command passes os.Stdin so the user can paste the redirect URL after authorizing in their browser. Assisted-by: Claude Code (claude-opus-4-7)
Adds a go:generate directive for the ConnectConfig interface and checks in the generated mock so the connect command can be unit tested with the same pattern used by logout and login. Assisted-by: Claude Code (claude-opus-4-7)
revokeAuthServerToken is now a package-level function plus a func(ctx) error field on logoutOpts populated in initFlow, mirroring revokeServiceAccountToken. Tests can swap in their own implementation the same way they do for the service-account path. Assisted-by: Claude Code (claude-opus-4-7)
RunUserDelegation now reads tokenExpiry and refreshToken from opts fields, following the same pattern as authSubject and authType for the other auth types. Globals are read once in RunE. Assisted-by: Claude Code (claude-opus-4-7)
connect_test covers metadataExpired and discoverOrLoadMetadata including the cache-hit, stale-cache, issuer-mismatch, no-cache, and Discover-flag branches. logout_test adds a UserDelegation case to the Run series using the injectable revokeAuthServerToken field. whoami_test covers the four output paths of RunUserDelegation through the new opts fields. Assisted-by: Claude Code (claude-opus-4-7)
The whoami command previously split routing between RunE and the methods on whoOpts: RunE checked AuthType and chose between Run() and RunUserDelegation(), so the UserDelegation arm in authTypeAndSubject was never reached. Run now switches on opts.authType internally, RunE always calls authTypeAndSubject and populates the same opts fields, and the helper is unexported. Routing lives in one place. Assisted-by: Claude Code (claude-opus-4-7)
…inery with new labels and entry point
The UserAccountConnect menu work left hand-aligned columns in the authTypeDescription map and the prompt auth-type const block that gofmt rewrites. Reformat so the tree passes gofmt before further work lands on top. Assisted-by: Claude Code (claude-opus-4-8)
A pure rename, no behavior change. internal/cli/auth/connect.go and its test/mock become user_delegation.go, ConnectConfig becomes UserDelegationConfig, and ConnectOpts becomes UserDelegationFlow, so the type names describe the authorization code flow for login to Atlas rather than a standalone connect command. ConnectBuilder keeps its name and registrations so atlas auth connect still works exactly as before; the go:generate header and regenerated mock track the new filenames. Assisted-by: Claude Code (claude-opus-4-8)
RefreshAccessToken drives the device flow against cloud.mongodb.com, so it only makes sense for UserAccount profiles. UserDelegation profiles refresh in the transport layer against a different server, and sending their refresh token through this path targets the wrong endpoint; API key and Service Account profiles carry no token to refresh. Returning early for non-UserAccount profiles fixes the login, register, and clusters watch retry paths, which all reach this primitive. Assisted-by: Claude Code (claude-opus-4-8)
Selecting UserAccountConnect in atlas login now runs the authorization code flow. LoginRun sets the UserDelegation auth type and setUpCredentials dispatches to a UserDelegationFlow built from the default profile and the login command's flags. A new --discover flag forces re-discovery of authorization server metadata. The flow is held behind an injectable interface so LoginRun can be tested without performing the real browser exchange. Assisted-by: Claude Code (claude-opus-4-8)
Logout only initialized the revoke flow for UserAccount and ServiceAccount, leaving revokeAuthServerToken nil for UserDelegation profiles. Run then called it and panicked before reaching the token cleanup, so the profile was never cleared. PreRunE now provisions each auth type's revoke dependency on its own: UserAccount builds the device-flow client, while ServiceAccount and UserDelegation get their respective revoke closures. initFlow is narrowed to building the device-flow client it actually owns. Assisted-by: Claude Code (claude-opus-4-8)
NoAccessToken appended the access token subject in parentheses, but UserDelegation tokens yield no readable subject, producing an awkward "already authenticated with an account ()". Only read a subject for UserAccount and omit the parenthetical when there is none. Assisted-by: Claude Code (claude-opus-4-8)
The authorization code flow is now reached through the atlas login menu, so the standalone connect command and its hidden top-level shortcut are gone from the auth and root builders. UserDelegationFlow stores only the tokens and expiry; LoginRun sets the auth type when it dispatches the menu selection. Assisted-by: Claude Code (claude-opus-4-8)
|
I disagree with the proposed split. The changes to |
13d2595 to
ca5fcd7
Compare
2 participants
Proposed changes
Adds authorization-code-based login to Atlas as a new
UserAccountConnectoption in theatlas loginmenu. Authorization server metadata is discovered automatically (RFC 8414)and cached; a new
--discoverflag forces re-discovery when needed.Behavior changes worth noting beyond the diff:
UserAccountprofiles, which also fixes theclusters watchretry path for non-device-flow profiles.PreRunE, fixing a nil-func panic thatpreviously aborted logout's cleanup.
NoAccessTokenno longer prints an empty()for tokens without a readable subject.Jira ticket: CLOUDP-418563
Depends on: mongodb/atlas-cli-core#70, mongodb/go-client-mongodb-atlas#562
Checklist
--discover)make fmtand formatted my code