Feat/add support for forcing device auth flow on ios#4944
Conversation
- Remove verbose debug logging from WaitToken in device_flow.go - Improve TUN FD=0 fallback comments and warning messages - Document why config save after login differs from Android
WalkthroughAdds tvOS-friendly non-atomic config read/write and in-memory preloaded config, refactors iOS/tvOS login to listener-driven flows and a changed URLOpener interface, and hardens iOS TUN creation by treating tunFd == 0 as an explicit error and guarding fd-based TUN construction. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App/UI
participant Client as Client
participant Auth as Authenticator
participant URLOpener as URLOpener
participant Profile as ProfileManager
participant TUN as TunDevice
Note over Client,Profile: Run() / config selection
App->>Client: Run()
alt preloadedConfig present
Client->>Client: use preloadedConfig
else
Client->>Profile: DirectUpdateOrCreateConfig()
end
Note over Client,Auth: Login orchestration
Client->>Auth: LoginSync() or Login(resultListener,...)
Auth->>URLOpener: Open(url, userCode)
URLOpener-->>App: show URL + userCode
URLOpener->>Auth: OnLoginSuccess()
Auth->>Auth: foregroundGetTokenInfo (poll / wait)
Auth->>Profile: DirectWriteOutConfig(cfg)
Note over Client,TUN: TUN creation
Client->>TUN: TunDevice.Create(tunFd)
alt tunFd == 0
TUN->>TUN: create fallback TUN (CreateTUN)
else tunFd != 0
TUN->>TUN: dup fd -> set O_NONBLOCK -> create from fd
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
client/ios/NetBirdSDK/client.go (1)
268-272: Consider logging config load errors for debugging.Similar to
IsLoginRequired, the error fromDirectUpdateOrCreateConfigis ignored. While subsequent operations would fail ifcfgis nil, logging the error would help with debugging tvOS issues.// Use DirectUpdateOrCreateConfig to avoid atomic file operations (temp file + rename) // which are blocked by the tvOS sandbox in App Group containers - cfg, _ := profilemanager.DirectUpdateOrCreateConfig(profilemanager.ConfigInput{ + cfg, err := profilemanager.DirectUpdateOrCreateConfig(profilemanager.ConfigInput{ ConfigPath: c.cfgFile, }) + if err != nil { + log.Errorf("LoginForMobile: failed to load config: %v", err) + return err.Error() + }client/ios/NetBirdSDK/login.go (1)
273-276: Inconsistent context usage withcontext.TODO().
RequestAuthInfousescontext.TODO()while the rest of the method usesa.ctx. This inconsistency means cancellation ofa.ctxwon't propagate toRequestAuthInfo, which could lead to resource leaks or unexpected behavior.- flowInfo, err := oAuthFlow.RequestAuthInfo(context.TODO()) + flowInfo, err := oAuthFlow.RequestAuthInfo(a.ctx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/iface/device/device_ios.go(1 hunks)client/internal/profilemanager/config.go(2 hunks)client/ios/NetBirdSDK/client.go(5 hunks)client/ios/NetBirdSDK/login.go(8 hunks)client/ios/NetBirdSDK/preferences.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
client/ios/NetBirdSDK/preferences.go (1)
client/internal/profilemanager/config.go (1)
DirectUpdateOrCreateConfig(825-840)
client/ios/NetBirdSDK/client.go (1)
client/internal/profilemanager/config.go (4)
Config(90-161)ConfigFromJSON(876-883)DirectUpdateOrCreateConfig(825-840)ConfigInput(47-87)
client/internal/profilemanager/config.go (1)
util/file.go (2)
DirectWriteJson(59-95)ReadJson(183-202)
client/ios/NetBirdSDK/login.go (6)
client/android/login.go (4)
Auth(40-44)SSOListener(21-24)ErrListener(27-30)URLOpener(34-37)client/internal/pkce_auth.go (1)
GetPKCEAuthorizationFlowInfo(52-118)client/internal/profilemanager/config.go (3)
DirectWriteOutConfig(819-821)ConfigToJSON(866-872)ConfigFromJSON(876-883)client/internal/login.go (2)
Login(51-84)IsLoginRequired(21-48)client/system/info.go (1)
DeviceNameCtxKey(16-16)client/internal/auth/oauth.go (1)
TokenInfo(45-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (386)
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: JS / Lint
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
🔇 Additional comments (8)
client/internal/profilemanager/config.go (1)
863-883: LGTM!The JSON serialization helpers are clean and follow the existing patterns in the codebase. They provide the necessary functionality for tvOS alternative storage mechanisms.
client/ios/NetBirdSDK/preferences.go (1)
112-116: LGTM!The switch to
DirectUpdateOrCreateConfigis appropriate for tvOS sandbox compatibility, and the comment clearly explains the rationale.client/ios/NetBirdSDK/client.go (1)
94-106: LGTM!The
SetConfigFromJSONmethod provides a clean way to load preloaded config for tvOS with proper error handling and logging.client/ios/NetBirdSDK/login.go (5)
34-37: LGTM!The updated
URLOpenerinterface withuserCodeparameter andOnLoginSuccesscallback aligns with the Android SDK and enables proper device auth flow support for tvOS.
75-119: LGTM!The async refactoring of
SaveConfigIfSSOSupportedfollows a clean pattern with proper error propagation to the listener. The fallback from PKCE to Device Authorization flow handles the tvOS use case correctly.
154-185: LGTM!
LoginSynccorrectly handles the background VPN scenario where the user should already be authenticated. Returning an error whenneedsLoginis true is the right behavior for this non-interactive context.
187-192: LGTM!Good use of the deprecation pattern - the old
Loginmethod delegates to the newLoginWithDeviceNamewith sensible defaults, maintaining backward compatibility while encouraging migration.
300-319: LGTM!The JSON config methods provide clean integration with alternative storage mechanisms. The nil check in
GetConfigJSONis appropriate defensive coding.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/ios/NetBirdSDK/login.go (2)
20-37: Document/guarantee callback threading forSSOListener/ErrListener/URLOpener(currently background goroutines).
All callbacks are invoked from goroutines (not the caller thread). If the iOS/tvOS UI layer expects main-thread calls, this needs to be explicitly documented (or dispatched). Also consider nil-guardinglistener/urlOpenerto avoid SDK panics on misuse.
122-152: SamecfgPathhazard inloginWithSetupKeyAndSaveConfig+ consider passingctxWithValuesintowithBackOff.
Ifa.cfgPathis empty this will fail at the end; that’s fine, but make it explicit/early to avoid “login succeeded but save failed” surprises. Also,withBackOff(a.ctx, ...)could takectxWithValuesinstead to keep one context for cancellation/propagation.func (a *Auth) loginWithSetupKeyAndSaveConfig(setupKey string, deviceName string) error { @@ - err := a.withBackOff(a.ctx, func() error { + err := a.withBackOff(ctxWithValues, func() error { backoffErr := internal.Login(ctxWithValues, a.config, setupKey, "") @@ } + if a.cfgPath == "" { + return fmt.Errorf("cfgPath is empty") + } + // Use DirectWriteOutConfig to avoid atomic file operations (temp file + rename) // which are blocked by the tvOS sandbox in App Group containers return profilemanager.DirectWriteOutConfig(a.cfgPath, a.config) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/ios/NetBirdSDK/login.go(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Android / Build
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
🔇 Additional comments (3)
client/ios/NetBirdSDK/login.go (3)
154-185:LoginSync()behavior looks consistent for “background connect only” usage.
Returningnot authenticatedwhenIsLoginRequiredis true is a clear contract for callers.
301-320:GetConfigJSON/SetConfigFromJSONLGTM—please treat returned JSON as sensitive.
Since this likely contains private key material/tokens, callers should avoid logging it and ensure storage is protected (Keychain-encrypted storage / appropriate tvOS protections).
75-119: GuardcfgPath == ""before callingDirectWriteOutConfig(currently can fail unexpectedly).
saveConfigIfSSOSupported()always writes toa.cfgPath; but ifNewAuthWithConfigdoesn't setcfgPath, this can error even when SSO probing succeeds. Either (a) return a clear error like "cfgPath is empty", or (b) skip writing when empty and rely onGetConfigJSON()persistence. Verify the intended behavior when PKCE is unavailable but device flow is available.func (a *Auth) saveConfigIfSSOSupported() (bool, error) { if err != nil { return false, fmt.Errorf("backoff cycle failed: %v", err) } + if a.cfgPath == "" { + return false, fmt.Errorf("cfgPath is empty") + } + // Use DirectWriteOutConfig to avoid atomic file operations (temp file + rename) // which are blocked by the tvOS sandbox in App Group containers err = profilemanager.DirectWriteOutConfig(a.cfgPath, a.config) return true, err }
- Remove goroutine from OnLoginSuccess callback, invoke synchronously - Stop treating PermissionDenied as success, propagate as permanent error - Replace context.TODO() with bounded timeout context (30s) in RequestAuthInfo - Handle DirectUpdateOrCreateConfig errors in IsLoginRequired and LoginForMobile - Add permission enforcement to DirectUpdateOrCreateConfig for existing configs - Fix variable shadowing in device_ios.go where err was masked by := in else block
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/ios/NetBirdSDK/client.go (2)
75-106: GuardpreloadedConfigwith a mutex (current code is race-prone).
SetConfigFromJSON()writesc.preloadedConfig, whileRun()/IsLoginRequired()read it without any synchronization.type Client struct { @@ connectClient *internal.ConnectClient // preloadedConfig holds config loaded from JSON (used on tvOS where file writes are blocked) preloadedConfig *profilemanager.Config + preloadedConfigMu sync.RWMutex } @@ func (c *Client) SetConfigFromJSON(jsonStr string) error { @@ - c.preloadedConfig = cfg + c.preloadedConfigMu.Lock() + c.preloadedConfig = cfg + c.preloadedConfigMu.Unlock() @@ } @@ - if c.preloadedConfig != nil { + c.preloadedConfigMu.RLock() + preloaded := c.preloadedConfig + c.preloadedConfigMu.RUnlock() + if preloaded != nil { log.Infof("Run: using preloaded config from memory") - cfg = c.preloadedConfig + cfg = preloaded } else { @@ - if c.preloadedConfig != nil { + c.preloadedConfigMu.RLock() + preloaded := c.preloadedConfig + c.preloadedConfigMu.RUnlock() + if preloaded != nil { log.Infof("IsLoginRequired: using preloaded config from memory") - cfg = c.preloadedConfig + cfg = preloaded } else {Also applies to: 112-131, 236-270
311-330:loginCompleteis also written/read across goroutines without synchronization.
LoginForMobile()setsc.loginComplete = truein a goroutine, whileIsLoginComplete()/ClearLoginComplete()access it without locks/atomics. Considersync/atomic(or a small mutex) for this flag.Also applies to: 332-338
♻️ Duplicate comments (3)
client/iface/device/device_ios.go (1)
51-89: Good fix for the priorerrshadowing; fallback path is clear and separated.The
dupTunFd, err = unix.Dup(...)pattern avoids masking errors from the fd-based path (matches the earlier concern).client/internal/profilemanager/config.go (1)
823-846: Good: permission enforcement parity withUpdateOrCreateConfig.client/ios/NetBirdSDK/login.go (1)
208-246: Use the samectx(with deviceName + cancellation) throughout the OAuth device-flow.
login()buildsctx, butforegroundGetTokenInfo()usesa.ctx, so timeouts/cancellation/value propagation diverge.- tokenInfo, err := a.foregroundGetTokenInfo(urlOpener, forceDeviceAuth) + tokenInfo, err := a.foregroundGetTokenInfo(ctx, urlOpener, forceDeviceAuth) @@ -func (a *Auth) foregroundGetTokenInfo(urlOpener URLOpener, forceDeviceAuth bool) (*auth.TokenInfo, error) { - oAuthFlow, err := auth.NewOAuthFlow(a.ctx, a.config, false, forceDeviceAuth, "") +func (a *Auth) foregroundGetTokenInfo(ctx context.Context, urlOpener URLOpener, forceDeviceAuth bool) (*auth.TokenInfo, error) { + oAuthFlow, err := auth.NewOAuthFlow(ctx, a.config, false, forceDeviceAuth, "") @@ - authInfoCtx, authInfoCancel := context.WithTimeout(a.ctx, authInfoRequestTimeout) + authInfoCtx, authInfoCancel := context.WithTimeout(ctx, authInfoRequestTimeout) @@ - waitCTX, cancel := context.WithTimeout(a.ctx, waitTimeout) + waitCTX, cancel := context.WithTimeout(ctx, waitTimeout)Also applies to: 268-294
🧹 Nitpick comments (1)
client/internal/profilemanager/config.go (1)
817-867: Direct (non-atomic) writes are fine for tvOS constraints, but document/mitigate partial-write risk.Since
DirectWriteJsoncan leave a truncated file if the process is killed mid-write, consider at least (a) a comment warning callers that recovery may require re-auth, and/or (b) adding a cheap “write then fsync” insideutil.DirectWriteJsonif feasible for the target container.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/iface/device/device_ios.go(1 hunks)client/internal/profilemanager/config.go(2 hunks)client/ios/NetBirdSDK/client.go(6 hunks)client/ios/NetBirdSDK/login.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/ios/NetBirdSDK/client.go (2)
client/internal/profilemanager/config.go (4)
Config(90-161)ConfigFromJSON(882-889)DirectUpdateOrCreateConfig(825-846)ConfigInput(47-87)client/internal/login.go (2)
IsLoginRequired(21-48)Login(51-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
🔇 Additional comments (2)
client/ios/NetBirdSDK/client.go (1)
236-270: Nice hardening ofIsLoginRequiredaround config load failures / nil config.client/ios/NetBirdSDK/login.go (1)
154-186:LoginSync()behavior and error messaging look coherent for “no-UI” background flows.
- Make tunFd == 0 a hard error with exported ErrInvalidTunnelFD (remove dead fallback code) - Apply defaults in ConfigFromJSON to prevent partially-initialized configs - Add nil guards for listener/urlOpener interfaces in public SDK entry points - Reorder config save before OnLoginSuccess to prevent teardown race - Add explanatory comment for urlOpener.Open goroutine
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/iface/device/device_ios.go (1)
71-90: Fixos.NewFile(...)+unix.Close(dupTunFd)double-close hazard.The code creates an
*os.FilewrappingdupTunFdand passes it toCreateTUNFromFile(line 85). According to wireguard-go documentation,CreateTUNFromFiletakes ownership of the file descriptor — the Device will close it whenDevice.Close()is called.If
CreateTUNFromFilefails (line 86–90), the code callsunix.Close(dupTunFd)directly. However, the*os.Filecreated at line 85 still exists and has a finalizer that will close the same fd when garbage collected. This creates a double-close hazard.Extract the file to a named variable and call
f.Close()on error instead:- tunDevice, err = tun.CreateTUNFromFile(os.NewFile(uintptr(dupTunFd), "/dev/tun"), 0) + f := os.NewFile(uintptr(dupTunFd), "/dev/tun") + tunDevice, err = tun.CreateTUNFromFile(f, 0) if err != nil { log.Errorf("Unable to create new tun device from fd: %v", err) - _ = unix.Close(dupTunFd) + _ = f.Close() return nil, err }
♻️ Duplicate comments (1)
client/ios/NetBirdSDK/login.go (1)
284-316: Don’t launchurlOpener.Open(...)in a goroutine (ordering + UX races).
This was flagged in earlier review and it’s still present:WaitTokenpolling can begin before the UI is actually presented, and Swift gets less deterministic ordering guarantees. Prefer callingOpensynchronously; if Swift needs main-thread work, dispatch there in Swift.- go urlOpener.Open(flowInfo.VerificationURIComplete, flowInfo.UserCode) + urlOpener.Open(flowInfo.VerificationURIComplete, flowInfo.UserCode)Also recommended: pass the
ctxcreated inlogin()down intoforegroundGetTokenInfoso cancellation/values stay consistent (and you don’t silently dropdeviceNamecontext values).
🧹 Nitpick comments (3)
client/ios/NetBirdSDK/login.go (3)
20-37: URLOpener + listener interfaces look consistent with device-flow UX needs.
One request: ensure the SDK docs clearly state which callbacks can be invoked on background goroutines (most of these do).
90-123: SSO probing via PKCE then Device Flow + DirectWriteOutConfig is sensible for tvOS constraints.
Minor: consider whether returning(true, err)on save failure is what callers expect, vs(false, err)(since you couldn’t persist the “SSO-supported config”).
162-194:LoginSyncsemantics are clearer now (fails fast when interactive auth is required).
Optional: consider exporting a sentinel error (like you did withErrInvalidTunnelFD) so Swift can branch on “needs interactive auth” without string matching.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/iface/device/device_ios.go(3 hunks)client/internal/profilemanager/config.go(2 hunks)client/ios/NetBirdSDK/login.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/internal/profilemanager/config.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/iface/device/device_ios.go (2)
shared/management/status/error.go (1)
Errorf(70-75)relay/testec2/tun/tun.go (1)
Device(13-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: JS / Lint
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
🔇 Additional comments (6)
client/iface/device/device_ios.go (1)
49-69: Hard-error ontunFd == 0is the right move (clearer failure mode for iOS/tvOS NE fd wiring).
This also resolves the earlier “fallback TUN” ambiguity and should make tvOS failures much more diagnosable.client/ios/NetBirdSDK/login.go (5)
75-88: Nil-guard + async wrapper forSaveConfigIfSSOSupportedis good defensive SDK hygiene.
126-160: AsyncLoginWithSetupKeyAndSaveConfigwithbackoff.Permanenton PermissionDenied is a good UX optimization.
203-223:LoginWithDeviceNamenil-guards and async result delivery look solid for Swift bindings.
225-282: Good: persist config beforeOnLoginSuccess()and call success synchronously.
This directly avoids the teardown-order race noted in earlier reviews.
327-346: Config JSON helpers are a good escape hatch for tvOS/UserDefaults persistence.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/ios/NetBirdSDK/client.go (2)
64-109: GuardpreloadedConfigwith a mutex (current Set/Read is racy).
Line 78–80 + Line 100–108 + Line 121–135:SetConfigFromJSON()can run concurrently withRun()/IsLoginRequired()and races onc.preloadedConfig.diff --git a/client/ios/NetBirdSDK/client.go b/client/ios/NetBirdSDK/client.go @@ import ( "context" "fmt" "net/netip" "os" "sort" "strings" "sync" + "sync/atomic" "time" @@ type Client struct { @@ connectClient *internal.ConnectClient // preloadedConfig holds config loaded from JSON (used on tvOS where file writes are blocked) preloadedConfig *profilemanager.Config + preloadedConfigMu sync.RWMutex + loginComplete uint32 // accessed concurrently (goroutine in LoginForMobile) } @@ func (c *Client) SetConfigFromJSON(jsonStr string) error { cfg, err := profilemanager.ConfigFromJSON(jsonStr) if err != nil { log.Errorf("SetConfigFromJSON: failed to parse config JSON: %v", err) return err } - c.preloadedConfig = cfg + c.preloadedConfigMu.Lock() + c.preloadedConfig = cfg + c.preloadedConfigMu.Unlock() log.Infof("SetConfigFromJSON: config loaded successfully from JSON") return nil } @@ // Use preloaded config if available (tvOS where file writes are blocked) - if c.preloadedConfig != nil { + c.preloadedConfigMu.RLock() + preloaded := c.preloadedConfig + c.preloadedConfigMu.RUnlock() + if preloaded != nil { log.Infof("Run: using preloaded config from memory") - cfg = c.preloadedConfig + cfg = preloaded } else {Also applies to: 120-135
228-274: Don’t holdctxCancelLockacross network calls; it blocksStop()from canceling.
Line 236–273 and Line 287–321:defer c.ctxCancelLock.Unlock()keeps the lock held for the full login-required check / auth-info request, soStop()can’t acquire the lock to callc.ctxCancel().diff --git a/client/ios/NetBirdSDK/client.go b/client/ios/NetBirdSDK/client.go @@ func (c *Client) IsLoginRequired() bool { @@ - c.ctxCancelLock.Lock() - defer c.ctxCancelLock.Unlock() - ctx, c.ctxCancel = context.WithCancel(ctxWithValues) + c.ctxCancelLock.Lock() + ctx, cancel := context.WithCancel(ctxWithValues) + c.ctxCancel = cancel + c.ctxCancelLock.Unlock() + defer cancel() @@ - if c.preloadedConfig != nil { + c.preloadedConfigMu.RLock() + preloaded := c.preloadedConfig + c.preloadedConfigMu.RUnlock() + if preloaded != nil { log.Infof("IsLoginRequired: using preloaded config from memory") - cfg = c.preloadedConfig + cfg = preloaded } else { @@ func (c *Client) LoginForMobile() string { @@ - c.ctxCancelLock.Lock() - defer c.ctxCancelLock.Unlock() - ctx, c.ctxCancel = context.WithCancel(ctxWithValues) + c.ctxCancelLock.Lock() + ctx, cancel := context.WithCancel(ctxWithValues) + c.ctxCancel = cancel + c.ctxCancelLock.Unlock() + // NOTE: don't defer cancel here; the goroutine below uses ctx. Stop() should cancel it. @@ go func() { + defer cancel() waitTimeout := time.Duration(flowInfo.ExpiresIn) * time.Second waitCTX, cancel := context.WithTimeout(ctx, waitTimeout) defer cancel() @@ - c.loginComplete = true + atomic.StoreUint32(&c.loginComplete, 1) }() @@ func (c *Client) IsLoginComplete() bool { - return c.loginComplete + return atomic.LoadUint32(&c.loginComplete) == 1 } @@ func (c *Client) ClearLoginComplete() { - c.loginComplete = false + atomic.StoreUint32(&c.loginComplete, 0) }Also applies to: 279-334
client/ios/NetBirdSDK/login.go (1)
92-125: Fail fast (clear error) whencfgPathis empty before callingDirectWriteOutConfig.
Line 121–124 and Line 159–162: ifa.cfgPath == "", the write will fail with a relatively opaque filesystem error; returning a clearer error helps SDK consumers.diff --git a/client/ios/NetBirdSDK/login.go b/client/ios/NetBirdSDK/login.go @@ func (a *Auth) saveConfigIfSSOSupported() (bool, error) { @@ - err = profilemanager.DirectWriteOutConfig(a.cfgPath, a.config) + if a.cfgPath == "" { + return false, fmt.Errorf("cfgPath is empty; cannot persist config") + } + err = profilemanager.DirectWriteOutConfig(a.cfgPath, a.config) return true, err } @@ func (a *Auth) loginWithSetupKeyAndSaveConfig(setupKey string, deviceName string) error { @@ - return profilemanager.DirectWriteOutConfig(a.cfgPath, a.config) + if a.cfgPath == "" { + return fmt.Errorf("cfgPath is empty; cannot persist config") + } + return profilemanager.DirectWriteOutConfig(a.cfgPath, a.config) }Also applies to: 143-162
🧹 Nitpick comments (4)
client/ios/NetBirdSDK/preferences.go (1)
114-118: Switch to non-atomic config update path makes sense; tweak comment to match build-tag/platform reality.
Line 115–117: UsingDirectUpdateOrCreateConfigaligns with the App Group atomic-rename constraint. Only nit: consider rewording the comment to “iOS/tvOS App Group containers” (or whatever theiosbuild tag actually covers in this repo) to avoid confusion.client/ios/NetBirdSDK/client.go (1)
291-299: Consider using the preloaded-config path inLoginForMobile()for tvOS parity.
Line 291–299:LoginForMobile()always hitsDirectUpdateOrCreateConfigeven ifpreloadedConfigis set, which differs fromRun()/IsLoginRequired(). If tvOS uses this path, it’ll likely break.client/ios/NetBirdSDK/login.go (2)
227-314: Use the samectx(deviceName + cancellation) forforegroundGetTokenInfo, nota.ctx.
Line 231–262 vs Line 289–307:login()correctly switches to a deviceName-enrichedctx, butforegroundGetTokenInfo()usesa.ctx, so the interactive flow may ignore cancellation and context values intended for this login attempt.diff --git a/client/ios/NetBirdSDK/login.go b/client/ios/NetBirdSDK/login.go @@ - tokenInfo, err := a.foregroundGetTokenInfo(urlOpener, forceDeviceAuth) + tokenInfo, err := a.foregroundGetTokenInfo(ctx, urlOpener, forceDeviceAuth) @@ -func (a *Auth) foregroundGetTokenInfo(urlOpener URLOpener, forceDeviceAuth bool) (*auth.TokenInfo, error) { - oAuthFlow, err := auth.NewOAuthFlow(a.ctx, a.config, false, forceDeviceAuth, "") +func (a *Auth) foregroundGetTokenInfo(ctx context.Context, urlOpener URLOpener, forceDeviceAuth bool) (*auth.TokenInfo, error) { + if ctx == nil { + ctx = context.Background() + } + oAuthFlow, err := auth.NewOAuthFlow(ctx, a.config, false, forceDeviceAuth, "") if err != nil { return nil, err } @@ - authInfoCtx, authInfoCancel := context.WithTimeout(a.ctx, authInfoRequestTimeout) + authInfoCtx, authInfoCancel := context.WithTimeout(ctx, authInfoRequestTimeout) @@ - waitCTX, cancel := context.WithTimeout(a.ctx, waitTimeout) + waitCTX, cancel := context.WithTimeout(ctx, waitTimeout)
325-344: Config JSON API is fine; ensure callers treat it as sensitive.
Line 325–343:GetConfigJSON()will include private key material and other secrets fromprofilemanager.Config; worth a short doc note that it must be stored securely (Keychain/UserDefaults protections/etc.).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/ios/NetBirdSDK/client.go(6 hunks)client/ios/NetBirdSDK/login.go(7 hunks)client/ios/NetBirdSDK/preferences.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
client/ios/NetBirdSDK/preferences.go (1)
client/internal/profilemanager/config.go (1)
DirectUpdateOrCreateConfig(825-846)
client/ios/NetBirdSDK/client.go (2)
client/internal/profilemanager/config.go (4)
Config(90-161)ConfigFromJSON(883-897)DirectUpdateOrCreateConfig(825-846)ConfigInput(47-87)client/internal/login.go (2)
IsLoginRequired(21-48)Login(51-84)
client/ios/NetBirdSDK/login.go (5)
client/android/login.go (3)
Auth(40-44)SSOListener(21-24)URLOpener(34-37)client/internal/profilemanager/config.go (3)
DirectWriteOutConfig(819-821)ConfigToJSON(872-878)ConfigFromJSON(883-897)client/internal/login.go (2)
Login(51-84)IsLoginRequired(21-48)client/internal/auth/oauth.go (2)
TokenInfo(45-53)NewOAuthFlow(74-86)management/server/http/testing/testing_tools/tools.go (1)
ExpiresIn(43-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: JS / Lint
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
shuuri-labs
deleted the
feat/add-support-for-forcing-device-auth-flow-on-ios
branch
3 participants
Describe your changes
This PR adds support for forcing device authentication flow on iOS/tvOS, enabling Apple TV (tvOS) support similar to what was done for Android TV.
Background
tvOS (Apple TV) cannot use the standard PKCE authentication flow because:
This implementation uses the device code flow where users scan a QR code or enter a code on another device to authenticate.
SDK Changes (
client/ios/NetBirdSDK/)login.goclient.godevice_ios.goconfig.go(profilemanager)Breaking API Changes
These are intentional breaking changes for the iOS SDK to support tvOS:
Open(string)→Open(url, userCode)+OnLoginSuccess()LoginSync(), newLogin()has different signatureThe iOS client app has been updated to use these new APIs. Existing iOS functionality is preserved.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Summary by CodeRabbit
New Features
New Behavior
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.