[client] Keep the account email backing the SSO login hint correct - #6986
Open
pappz wants to merge 5 commits into
Open
[client] Keep the account email backing the SSO login hint correct#6986pappz wants to merge 5 commits into
pappz wants to merge 5 commits into
Conversation
The daemon returns the authenticated user's email from WaitSSOLogin but cannot persist it: it runs as root while the per-profile state file is user-owned. The CLI's handleSSOLogin writes it after its own WaitSSOLogin; the GUI path read the value and dropped it. The profile was therefore left with no email, so Profiles.List showed no account for it, and later logins and session extends went out with no login_hint — leaving the IdP to pick an account instead of reusing the one the profile belongs to. Mirror the CLI and store it, next to the Logout path that already clears the same file for the same reason.
Contributor
📝 WalkthroughWalkthroughProfile state persistence is centralized for targeted and active profiles. SSO login now carries the resolved profile ID so authenticated email is stored in the correct profile, while profile removal performs best-effort state-file cleanup. ChangesProfile state management
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ConnectionLogin
participant WaitSSOLogin
participant ProfileManager
ConnectionLogin->>WaitSSOLogin: pass resolved ProfileID
WaitSSOLogin->>ProfileManager: persist authenticated email
ProfileManager-->>WaitSSOLogin: persistence result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/ui/services/connection.go (1)
252-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the new persistence contract.
Please test that a non-empty email is stored, an empty email skips persistence, and a state-write failure still returns the daemon email without failing login.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/ui/services/connection.go` around lines 252 - 258, Extend the login response flow around resp.GetEmail() with regression tests covering three cases: persist a non-empty email, skip SetActiveProfileState for an empty email, and return the daemon email successfully when SetActiveProfileState fails. Use mocks or fixtures for profilemanager.NewProfileManager and verify the persistence call and returned login result without changing the existing non-fatal error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@client/ui/services/connection.go`:
- Around line 252-258: Extend the login response flow around resp.GetEmail()
with regression tests covering three cases: persist a non-empty email, skip
SetActiveProfileState for an empty email, and return the daemon email
successfully when SetActiveProfileState fails. Use mocks or fixtures for
profilemanager.NewProfileManager and verify the persistence call and returned
login result without changing the existing non-fatal error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 63806dfe-c27f-47f8-a40e-a319500e9a85
📒 Files selected for processing (1)
client/ui/services/connection.go
SetActiveProfileState resolves the target itself, so it writes to whichever profile is active when it is called. A GUI SSO login spans seconds of user interaction in the browser, and the tray stays clickable throughout: switching profiles in that window left the email filed under the profile that happened to be active when the flow returned. The wrong profile then advertised an account it does not own, and offered it as the login_hint next time. Add SetProfileState(id, state), the write-side counterpart of the existing GetProfileState(id), and keep SetActiveProfileState as a wrapper for callers with no particular profile in mind. Login now reports the profile it resolved so the frontend can hand it back with the SSO wait, which closes the window.
Removing a profile left its state file behind: the daemon deletes what it owns, but the file holding the account email is user-owned and out of reach for a root daemon, which is why Connection.Logout already clears it from the UI side. Beyond the stray file, legacy profiles are keyed by name rather than by a generated ID, so recreating a profile under a removed one's name inherited its email — shown as the account in the profile list and sent as the login_hint on the next login.
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
pappz
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/ui/services/connection.go`:
- Line 133: Use canonical daemon-resolved profile IDs for SSO state persistence
and cleanup: in client/ui/services/connection.go lines 133 and 228, use
LoginResponse.Id or resolve the profile’s canonical ID before passing it to
ProfileState, SetProfileState, or RemoveProfileState; in
client/ui/services/profile.go lines 163-173, resolve the removed profile by
username and pass its canonical ID to RemoveProfileState, supporting removals by
username only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0519fd65-1bf9-4244-bd27-2b8fb41bb208
⛔ Files ignored due to path filters (1)
client/ui/frontend/src/lib/connection.tsis excluded by!**/*.ts
📒 Files selected for processing (3)
client/internal/profilemanager/state.goclient/ui/services/connection.goclient/ui/services/profile.go
8 tasks
RemoveProfile takes a handle — a display name or an ID prefix resolve just as well as a full ID — but the state file holding the account email is named after the ID. Passing the request handle straight through therefore named a different file, or none, leaving the email behind for a recreated profile to inherit. The daemon already echoes back the ID it resolved for exactly this purpose; use it.
LoginResult.ProfileID was filled from the request's ProfileName, which is a handle: a display name or an ID prefix resolve just as well. waitSSOLogin names the state file after it, so a handle would have written the account email to a file no reader looks for — the email silently lost, plus a stray file. Fill it only on the branch where the daemon supplied the ID, and leave it empty otherwise; waitSSOLogin then falls back to the active profile, as it did before the field existed.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/ui/services/connection.go (1)
260-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the profile-switch window.
Capture
LoginResult.ProfileID, switch the active profile beforeWaitSSOLogin, and verify the email is written to the captured profile rather than the new active profile. Also cover the empty-ID fallback path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/ui/services/connection.go` around lines 260 - 284, 添加回归测试覆盖 SSO 等待期间切换 profile 的场景：捕获 LoginResult.ProfileID，在调用 WaitSSOLogin 前切换活动 profile，并验证邮箱通过该 captured profile ID 写入原 profile 而非新活动 profile。另增加 ProfileID 为空时的测试，确认邮箱写入当前活动 profile 的 fallback 路径。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@client/ui/services/connection.go`:
- Around line 260-284: 添加回归测试覆盖 SSO 等待期间切换 profile 的场景：捕获
LoginResult.ProfileID，在调用 WaitSSOLogin 前切换活动 profile，并验证邮箱通过该 captured profile
ID 写入原 profile 而非新活动 profile。另增加 ProfileID 为空时的测试，确认邮箱写入当前活动 profile 的 fallback
路径。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a58bee2f-d820-4546-abd4-b43002a1b2ed
📒 Files selected for processing (2)
client/ui/services/connection.goclient/ui/services/profile.go
lixmal
approved these changes
Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Three fixes to how the desktop GUI keeps the account email that backs the SSO
login_hint. Each is independent and reviewable on its own.1. Store the email after a GUI SSO login
The daemon returns the authenticated user's email from
WaitSSOLoginbut cannot persist it: it runs as root while the per-profile state file is user-owned. The CLI'shandleSSOLoginwrites it after its ownWaitSSOLogin; the GUI path read the value and dropped it.The profile was therefore left with no email, so
Profiles.Listshowed no account for it, and later logins and session extends went out with nologin_hint— leaving the IdP to pick an account instead of reusing the one the profile belongs to. Mirror the CLI and store it, next to theLogoutpath that already clears the same file for the same reason.2. File the email against the profile the login ran for
SetActiveProfileStateresolves the target itself, so it writes to whichever profile is active when it is called. A GUI SSO login spans seconds of user interaction in the browser, and the tray stays clickable throughout: switching profiles in that window left the email filed under the profile that happened to be active when the flow returned. The wrong profile then advertised an account it does not own, and offered it as thelogin_hintnext time.Adds
SetProfileState(id, state), the write-side counterpart of the existingGetProfileState(id), and keepsSetActiveProfileStateas a wrapper for callers with no particular profile in mind.Loginnow reports the profile it resolved so the frontend can hand it back with the SSO wait, which closes the window.3. Delete the email when a profile is removed
Removing a profile left its state file behind: the daemon deletes what it owns, but the email file is user-owned and out of reach for a root daemon — the same split that already puts the
Logoutcleanup on the UI side.Beyond the stray file, legacy profiles are keyed by name rather than by a generated ID, so recreating a profile under a removed one's name inherited its email — shown as the account in the profile list and sent as the
login_hinton the next login.Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Bug Fixes