From 652d5f3c15698635690d9e029a1a05e303957a9c Mon Sep 17 00:00:00 2001 From: evgeniyChepelev <68751844+evgeniyChepelev@users.noreply.github.com> Date: Tue, 1 Sep 2026 13:29:26 +0200 Subject: [PATCH] [client] Reuse the profile's account for iOS SSO logins (#7193) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [client] Reuse the profile's account for iOS SSO logins Android reads the profile's stored account and passes it as the OIDC login_hint, and records it again after a successful login. iOS did neither: it called GetOAuthFlow with an empty hint, so a re-login was resolved by whatever session the browser's cookie jar held rather than by the account the profile belongs to. With a non-ephemeral browser session that is the wrong account as soon as more than one is signed in. Mirror client/android/login.go: hint from mobile.ReadProfileEmail before the flow, mobile.WriteProfileEmail after Login succeeds. Storing after Login and not before keeps a rejected token from leaving a hint that points at an account which cannot be used. Co-Authored-By: Claude Opus 5 * [client] Persist the account email on tvOS and on the device flow Two paths left a profile with no account bound, so every later login went out without a login_hint — the case this change exists to remove. WriteProfileEmail went through util.WriteJsonWithRestrictedPermission, which writes a temp file and renames it over the target. The tvOS App Group sandbox blocks exactly that, which is why the config sitting next to this file is written with DirectWriteOutConfig. On tvOS the email write therefore failed and was dropped with a warning. Use DirectWriteJson: the file is rewritten whole from a single key, so the only thing atomicity buys here is surviving a crash mid-write, and a torn file reads back as "no email" and is replaced by the next login. The device authorization flow never populated TokenInfo.Email, unlike the PKCE flow, so a client driven through it — Android TV and tvOS — bound no account at all. Parse the ID token there too. Co-Authored-By: Claude Opus 5 * [client] Report a failed close from DirectWriteJson The deferred close assigned its error to err, but the return value was not named, so the assignment went nowhere: a close that failed was logged and the function still returned nil. The write is only durable once the file closes cleanly, so every caller — the management config, the profile configs and the profile account email — could be told the data landed when it had not. Name the return so the assignment does what its shape always intended, and report the failure once. When the body succeeded the close error is returned and the caller logs it. When the body already failed, that error is the one that explains the failure and is what the caller gets, which leaves the deferred log as the only place the close failure can surface — at debug, per the logging rules for close errors on writes. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- client/internal/auth/device_flow.go | 10 ++++++++++ client/ios/NetBirdSDK/login.go | 27 ++++++++++++++++++++++++++- client/mobile/profile_state.go | 8 +++++++- util/file.go | 23 ++++++++++++++++++----- 4 files changed, 61 insertions(+), 7 deletions(-) diff --git a/client/internal/auth/device_flow.go b/client/internal/auth/device_flow.go index 9dec7cf53..3592e589d 100644 --- a/client/internal/auth/device_flow.go +++ b/client/internal/auth/device_flow.go @@ -304,6 +304,16 @@ func (d *DeviceAuthorizationFlow) WaitToken(ctx context.Context, info AuthFlowIn return TokenInfo{}, fmt.Errorf("validate access token failed with error: %v", err) } + // Same as the PKCE flow: the account the token belongs to is what + // callers store to send back as the login_hint. Without it a client + // driven through the device flow — Android TV and tvOS — never binds + // an account to its profile and every later login goes out blind. + if email, err := parseEmailFromIDToken(tokenInfo.IDToken); err != nil { + log.Warnf("failed to parse email from ID token: %v", err) + } else { + tokenInfo.Email = email + } + log.Infof("device flow: user authorization confirmed after %d polls in %s", polls, time.Since(start).Round(time.Second)) return tokenInfo, err } diff --git a/client/ios/NetBirdSDK/login.go b/client/ios/NetBirdSDK/login.go index 42a575359..cf7aa6730 100644 --- a/client/ios/NetBirdSDK/login.go +++ b/client/ios/NetBirdSDK/login.go @@ -11,6 +11,7 @@ import ( "github.com/netbirdio/netbird/client/internal/auth" "github.com/netbirdio/netbird/client/internal/profilemanager" + "github.com/netbirdio/netbird/client/mobile" "github.com/netbirdio/netbird/client/system" ) @@ -284,12 +285,14 @@ func (a *Auth) login(urlOpener URLOpener, forceDeviceAuth bool, deviceName strin } jwtToken := "" + email := "" if needsLogin { tokenInfo, err := a.foregroundGetTokenInfo(authClient, urlOpener, forceDeviceAuth) if err != nil { return fmt.Errorf("interactive sso login failed: %v", err) } jwtToken = tokenInfo.GetTokenToUse() + email = tokenInfo.Email } err, isAuthError := authClient.Login(ctx, "", jwtToken) @@ -301,6 +304,14 @@ func (a *Auth) login(urlOpener URLOpener, forceDeviceAuth bool, deviceName strin return fmt.Errorf("login failed: %v", err) } + // Stored after Login, not before: a rejected token must not leave a hint + // pointing at an account that cannot be used. + if email != "" && a.cfgPath != "" { + if err := mobile.WriteProfileEmail(a.cfgPath, email); err != nil { + log.Warnf("failed to store profile account email: %v", err) + } + } + // Save the config before notifying success to ensure persistence completes // before the callback potentially triggers teardown on the Swift side. // Note: This differs from Android which doesn't save config after login. @@ -320,10 +331,24 @@ func (a *Auth) login(urlOpener URLOpener, forceDeviceAuth bool, deviceName strin return nil } +// profileLoginHint returns the stored account email for the profile at cfgPath, +// so a re-login targets the account the profile already belongs to instead of +// whatever session the shared browser cookie jar happens to hold. +// +// An empty hint is deliberate, not a fallback: a fresh profile leaves the +// choice to the IdP. Switching accounts is done by switching or removing +// profiles, not by logging out — logout keeps the email. +func profileLoginHint(cfgPath string) string { + if cfgPath == "" { + return "" + } + return mobile.ReadProfileEmail(cfgPath) +} + const authInfoRequestTimeout = 30 * time.Second func (a *Auth) foregroundGetTokenInfo(authClient *auth.Auth, urlOpener URLOpener, forceDeviceAuth bool) (*auth.TokenInfo, error) { - oAuthFlow, err := authClient.GetOAuthFlow(a.ctx, forceDeviceAuth, "") + oAuthFlow, err := authClient.GetOAuthFlow(a.ctx, forceDeviceAuth, profileLoginHint(a.cfgPath)) if err != nil { return nil, fmt.Errorf("failed to get OAuth flow: %v", err) } diff --git a/client/mobile/profile_state.go b/client/mobile/profile_state.go index bb983ec1d..ad05801f8 100644 --- a/client/mobile/profile_state.go +++ b/client/mobile/profile_state.go @@ -78,8 +78,14 @@ func WriteProfileEmail(configPath string, email string) error { return fmt.Errorf("resolve profile account path: %w", err) } + // DirectWriteJson, not the atomic writers: those create a temp file and + // rename it over the target, which the tvOS App Group sandbox blocks. It is + // the same reason the config next to this file goes through + // DirectWriteOutConfig. The file is rewritten whole from one key, so losing + // atomicity costs nothing beyond a torn write on a crash mid-write, which + // reads back as "no email" and is recovered by the next login. state := profilemanager.ProfileState{Email: email} - if err := util.WriteJsonWithRestrictedPermission(context.Background(), accountPath, state); err != nil { + if err := util.DirectWriteJson(context.Background(), accountPath, state); err != nil { return fmt.Errorf("write profile account: %w", err) } diff --git a/util/file.go b/util/file.go index 926904f9f..52eb91c0f 100644 --- a/util/file.go +++ b/util/file.go @@ -56,9 +56,9 @@ func WriteJson(ctx context.Context, file string, obj interface{}) error { } // DirectWriteJson writes JSON config object to a file creating parent directories if required without creating a temporary file -func DirectWriteJson(ctx context.Context, file string, obj interface{}) error { +func DirectWriteJson(ctx context.Context, file string, obj interface{}) (err error) { - _, _, err := prepareConfigFileDir(file) + _, _, err = prepareConfigFileDir(file) if err != nil { return err } @@ -68,11 +68,24 @@ func DirectWriteJson(ctx context.Context, file string, obj interface{}) error { return err } + // Named return so a failed Close is reported rather than logged and + // swallowed: the write is only durable once the file closes cleanly, and a + // caller told "written" would carry on with data that never landed. defer func() { - err = targetFile.Close() - if err != nil { - log.Errorf("failed to close file %s: %v", file, err) + cerr := targetFile.Close() + if cerr == nil { + return } + if err == nil { + // Returned, not logged: the caller reports it once. + err = cerr + return + } + // The body already failed and that error is the one the caller gets, so + // it is the one that explains the failure. This is then the only place + // the close failure can surface — at debug, per the logging rules for + // close errors on writes. + log.Debugf("failed to close file %s after %v: %v", file, err, cerr) }() // make it pretty