* [client] Force interactive login when extending the auth session
A session extend must be answered from the account the peer is registered
under. With a silent PKCE flow (DisablePromptLogin or max_age=0) the IdP
answers from whatever session it already holds, which need not be the
peer's account when several are signed in; the token then fails the
user match in ExtendAuthSession with no way to pick another account.
Mark the PKCE flow request as a session extend so the management server
can force prompt=login for it, overriding the configured silent flow.
* [client] Reduce cognitive complexity of Server.Login
Login sat at cognitive complexity 27, over the 25 the linter allows.
Extract the interactive SSO branch into startSSOLogin, and split the
nested in-flight-flow reuse check out of it into reuseOAuthFlow, which
flattens the original if/else into early returns: it returns the cached
auth info when the previous flow targets the same client and still has
more than 90s left, otherwise cancels the stale wait and returns nil so
the caller requests a fresh flow.
The helpers take the contextState through a small statusSetter
interface, since internal.contextState is unexported and re-deriving it
with CtxGetState inside the helper would resolve against callerCtx
rather than rootCtx.
No behavior change: same ordering of state transitions, same mutex scope
around the oauthAuthFlow write, same error paths. Login is now at 21.
* [client] Respect DisablePromptLogin when extending the auth session
Forcing prompt=login on a session extend overrode DisablePromptLogin, which
is set for IdPs that break on it: Authentik triggers a double authentication
and social logins fail outright. Overriding it there trades a recoverable
extend for a login that cannot complete at all.
Keep the LoginFlag override, which only replaces max_age=0 or none with
prompt=login so the IdP honours login_hint, and leave DisablePromptLogin as
configured. Those deployments keep the silent flow, and with several accounts
signed in an extend answered from the wrong one still fails the user match.
* [client] Guard the shared OAuth flow state with the server mutex
reuseOAuthFlow read flow, expiresAt, waitCancel and info without holding
s.mutex, while startSSOLogin and WaitSSOLogin write them under it. Reading the
fields one at a time could also answer with auth info from a flow that was
already replaced, or cancel a wait that no longer belongs to the flow just
judged stale. Take one snapshot under the lock and decide from it.
WaitSSOLogin read oauthAuthFlow.flow twice outside the lock; both now use a
value snapshotted in the critical section that already installs actCancel.
Its stale waitCancel was read and called in a separate section from the one
installing the new one, so two racing calls could read the same predecessor and
leave one wait uncancelled. Swap the two in a single critical section. Both
cancels run after unlocking: the displaced wait takes s.mutex as it unwinds.
* [client] Verify the SSO login came back for the hinted account
login_hint is a suggestion the IdP may ignore: with a silent flow configured
(DisablePromptLogin or max_age=0) and a live IdP session for another account,
the login completes with that account's token. On a registered peer the
management server rejects it as a user mismatch, but on a fresh profile the
peer silently registers under the wrong account and the profile is then bound
to it — every later login follows the stored hint straight back.
After the token exchange, compare the ID token's email against the hint the
flow was sent with. On a mismatch, do not log in to management with the token;
run one more round asking the IdP to re-decide the account (prompt=login, via
ForceAccountPrompt — DisablePromptLogin still wins there). If the prompted
round also comes back different, proceed with a warning: the address may
legitimately have changed, and refusing forever would lock the user out of the
profile while the management server still rejects a token that does not own
the peer. A token or profile with no email to compare is not judged.
The retry differs per platform because of who opens the browser:
- CLI (netbird login foreground) and Android run the whole flow in one
process, so the mismatch retries automatically: the browser reopens with
the account prompt within the same login attempt.
- On desktop the login is split between the daemon and the GUI: Login hands
the authorize URL to the GUI, WaitSSOLogin blocks for the token, and only
the GUI can open a browser. A new URL cannot be handed out from inside
WaitSSOLogin (its response has no field for one, kept that way to avoid a
proto change), so the daemon arms forceAccountPrompt, fails the round with
"connect again to choose the account", and builds the next Login's flow
with the prompt — the user's next connect is the retry.
The flag and the flow annotations live in daemon memory only; SwitchProfile
drops them so the previous profile's hint cannot judge the next profile's
token. The device code flow has no prompt parameter (RFC 8628), so a prompted
round there runs as-is and a repeated mismatch is let through with the
warning rather than looping.
* [client] Address review comments on PKCE session extend flow
Fail the PKCE authorization flow test on request error instead of
continuing into a nil dereference, and make the godoc comments on the
touched exported symbols identifier-leading full sentences.
* [client] Match accounts only on the email claim of the ID token
The name-claim fallback in the ID token parsing is kept for the login
hint and display, but account matching now only considers a value that
came from the email claim, so a token without one no longer produces a
false account mismatch.
* [client] Drop the pending session extend on a profile switch
The profile-switch cleanup dropped the pending login flow and the
account-prompt flag, but left extendAuthSessionFlow untouched. Its device
code was issued by the previous profile's IdP client, so a
WaitExtendAuthSession still parked on the browser leg would submit the
resulting token against the new profile's engine.
* [client] Judge the SSO account against the flow that produced the token
WaitSSOLogin snapshotted the flow on entry but re-read the info, hint and
accountPrompted from the live s.oauthAuthFlow afterwards, in separate
critical sections. WaitToken blocks for the whole browser leg, so a
concurrent Login or RequestJWTAuth could replace the flow meanwhile and
the mismatch check would compare this wait's token against another flow's
account: either arming the prompt spuriously or letting a wrong-account
token through against an unrelated profile's hint. Take all of it in the
entry snapshot.
* [client] Keep the forced account prompt from being lost to flow reuse
startSSOLogin consumed forceAccountPrompt and applied the prompt to the
freshly built flow, but reuseOAuthFlow could then answer from a cached
flow for the same client — one built without prompt=login, e.g. by
RequestJWTAuth. The user got the same silent authorization URL that
produced the mismatch, with the flag already spent, so no later round
asked either. Rule reuse out when the prompt is forced, while still
cancelling the predecessor's wait.
RequestJWTAuth also wrote the flow fields one by one, leaving the previous
login's hint and accountPrompted behind for WaitSSOLogin to judge a later
token against. Both sites now replace the whole record.
* [client] Consume the forced account prompt after the retry
forceAccountPrompt was never cleared, so a flow that outlived the retry it
was armed for kept sending prompt=login on every later authorization
request and re-authenticated the user each time. RequestAuthInfo now takes
the flag as it builds the request.
* [client] Cancel the caller context in the SSO login tests
WaitSSOLogin parks a goroutine on the caller's context for the whole
browser leg. The tests passed context.Background(), which never cancels,
so each left one goroutine behind for the lifetime of the test binary.
* [client] Cancel the wait displaced by an OAuth flow replacement
Replacing the shared record with a whole struct value dropped the previous
flow's waitCancel, so an SSO browser wait still parked on it lost its
cancel: nothing could preempt it, and it could go on to run attemptLogin
or mutate the record behind the new flow. Both replacement sites now take
the displaced cancel over in the same critical section, via a shared
replaceOAuthFlow, and invoke it after the unlock.
* [client] Guard OAuth flow mutations by the flow that owns the wait
* [client] Arm the account prompt only from the wait that owns the flow
* [client] Adopt the three-value parseEmailFromIDToken in the device flow
The main merge brought in the device flow's email extraction from #7193,
which still used the two-value signature this branch replaced when account
matching was narrowed to the email claim. Git merged the files without a
textual conflict, so the branch stopped compiling.
Take the fromEmailClaim result and fill EmailClaim from it, the same way the
PKCE path does, so device-flow clients get the same account matching.
* [client] Populate the pending extend flow in the test server helper
SwitchProfile cancels and clears the pending session extend flow
unconditionally, the same way it clears the SSH JWT cache. New always
populates the field, but the hand-assembled test server did not, so
TestSwitchProfile_ClearsJWTCache panicked on a nil PendingFlow.
* [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 <noreply@anthropic.com>
* [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 <noreply@anthropic.com>
* [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 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
## Describe your changes
`isValidAccessToken` only decodes the JWT payload and checks the
audience
claim, but its name suggested full token validation. Rename it to
`validateTokenAudience` and document what it does: a client-side
audience/shape
check on a token just obtained from the IdP over TLS. Token authenticity
is
enforced server-side by the management server, which verifies the
signature
against the IdP's JWKS (`shared/auth/jwt/validator.go`) on every
request.
Also harden the parser: a non-empty token lacking the three-part JWT
structure
caused an index-out-of-range panic (`strings.Split(token, ".")[1]`); the
shape
is now validated first. `parseEmailFromIDToken` is documented as
best-effort UX
data (login hint/display), never used for authorization. Added tests for
audience matching, malformed tokens, and the panic regression.
Changes:
- Rename `isValidAccessToken` → `validateTokenAudience`; document that
it does
not verify the signature and that authenticity is enforced server-side.
- Fix an index-out-of-range panic on a non-empty token lacking JWT
structure
(`strings.Split(token, ".")[1]`) by validating the three-part shape
first.
- Document `parseEmailFromIDToken` as best-effort/unverified, used only
for the
login-hint/display UX, never for an authorization decision.
- Add `util_test.go` covering audience matching (string and array),
missing
audience, malformed payloads, and the panic regression.
## Issue ticket number and link
Internal cleanup: rename a misleadingly-named client-side helper and
harden JWT
parsing against malformed input (`client/internal/auth/util.go`).
## Stack
- `0.74.7-branch` - ⚠️ No PR associated with branch <!--
branch-stack -->
- \#6806 :point\_left:
### Checklist
- [x] Is it a bug fix
- [ ] Is a typo/documentation fix
- [ ] Is a feature enhancement
- [ ] It is a refactor
- [x] Created tests that fail without the change (if possible)
> By submitting this pull request, you confirm that you have read and
agree to the terms of the [Contributor License
Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md).
## Documentation
Select exactly one:
- [ ] I added/updated documentation for this change
- [x] Documentation is **not needed** for this change (explain why)
Internal client-side helper rename plus a panic hardening fix. No public
API,
gRPC, CLI/service flag, or configuration change; token authenticity
enforcement
(server-side JWKS verification) is unchanged.
### Docs PR URL (required if "docs added" is checked)
Paste the PR link from <https://github.com/netbirdio/docs> here:
N/A
<!-- codesmith:footer -->
***
<a
href="https://app.blacksmith.sh/netbirdio/codesmith/netbird/pr/6806"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img
alt="View with Codesmith"
src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a>
<a
href="https://backend.blacksmith.sh/track/enable-autofix?expires=1786811124&installation_id=146802194&pr_number=6806&repository=netbirdio%2Fnetbird&return_to=https%3A%2F%2Fgithub.com%2Fnetbirdio%2Fnetbird%2Fpull%2F6806&signature=fe45ce9f6df46a609594f84037aaab2a21893621f29d2be48d7f8b347c3c8776"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img
alt="Autofix with Codesmith"
src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a>
<sup>Need help on this PR? Tag <code>/codesmith</code> with what you
need. Autofix is disabled.</sup>
<!-- codesmith:autofix:disabled -->
<!-- /codesmith:footer -->
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Bug Fixes**
- Improved access-token audience validation during device and PKCE
authentication flows.
- Malformed tokens now return clear validation errors instead of risking
runtime failures.
- Added support for validating both string and array audience claims.
- **Tests**
- Added coverage for malformed tokens, invalid claims, missing
audiences, and panic prevention.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Users reported long delays between finishing browser authentication and
the client connecting. Logs could not attribute the time: the PKCE and
device flows were silent between issuing the auth URL and returning the
token, and nothing recorded when the GUI issued the Up request after
WaitSSOLogin completed.
Add log lines covering the full chain: PKCE callback arrival and token
exchange duration, device-flow polling and approval timing, GUI-side
brackets around WaitSSOLogin and Up, daemon-side Up arrival and
WaitSSOLogin return, and a frontend stall detector that reports when
webview timers were suspended (macOS App Nap / hidden-window
throttling), which delays the WaitSSOLogin-to-Up handoff.
Remove client secret from gRPC auth flow. The secret was originally included to support providers like Google Workspace that don't offer a proper PKCE flow, but this is no longer necessary with the embedded IdP. Deployments using such providers should migrate to the embedded IdP instead.
* Consolidate authentication logic
- Moving auth functions from client/internal to client/internal/auth package
- Creating unified auth.Auth client with NewAuth() constructor
- Replacing direct auth function calls with auth client methods
- Refactoring device flow and PKCE flow implementations
- Updating iOS/Android/server code to use new auth client API
* Refactor PKCE auth and login methods
- Remove unnecessary internal package reference in PKCE flow test
- Adjust context assignment placement in iOS and Android login methods
* Add gocritic linter
`gocritic` provides diagnostics that check for bugs, performance, and style issues
We disable the following checks:
- commentFormatting
- captLocal
- deprecatedComment
This PR contains many `//nolint:gocritic` to disable `appendAssign`.
Enhance the user experience by enabling authentication to Netbird using Single Sign-On (SSO) with any Identity Provider (IDP) provider. Current client offers this capability through the Device Authorization Flow, however, is not widely supported by many IDPs, and even some that do support it do not provide a complete verification URL.
To address these challenges, this pull request enable Authorization Code Flow with Proof Key for Code Exchange (PKCE) for client logins, which is a more widely adopted and secure approach to facilitate SSO with various IDP providers.
Zoltan Papp
