[client] Fall back to an available OAuth flow instead of hard-coding device code - #7186
[client] Fall back to an available OAuth flow instead of hard-coding device code#7186lixmal wants to merge 1 commit into
Conversation
|
📝 WalkthroughWalkthroughThe change adds graphical-session detection and propagates it through authentication requests. OAuth initialization now supports ordered PKCE and device-flow fallback. Unavailable SSO errors receive setup-key guidance. ChangesOAuth session flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Daemon
participant OAuthFlow
participant IdentityProvider
Client->>Daemon: Request authentication with hasGraphicalSession
Daemon->>OAuthFlow: Create OAuth flow
OAuthFlow->>IdentityProvider: Request authorization information
IdentityProvider-->>OAuthFlow: Flow available or unavailable
OAuthFlow-->>Daemon: Selected flow or fallback error
Daemon-->>Client: Authentication result
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 (7)
client/internal/auth/auth.go (1)
154-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
IsSSOUnavailablefor the classification.
IsSSOUnavailableinclient/internal/auth/oauth.goperforms the sameerrors.Ascheck. Call it here to keep one classification point.♻️ Proposed fix
- var ssoUnavailable *ssoUnavailableError - if errors.As(err, &ssoUnavailable) { + if IsSSOUnavailable(err) { return backoff.Permanent(err) }🤖 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/internal/auth/auth.go` around lines 154 - 157, Update the error classification in the retry flow to call IsSSOUnavailable(err) instead of performing a local errors.As check against ssoUnavailableError, while preserving the existing backoff.Permanent(err) behavior.client/internal/auth/oauth.go (2)
214-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the ignored
authClient.Close()error.Line 218 discards the error from
Close(). ThenewAuthcleanup on Lines 226-230 logs the same failure at debug level. Use the same handling in both places.As per coding guidelines: "Do not ignore write, network-send, or critical-cleanup errors; log ignored errors at debug or trace level".
♻️ Proposed fix
- defer authClient.Close() + defer func() { + if err := authClient.Close(); err != nil { + log.Debugf("close auth client: %v", err) + } + }()🤖 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/internal/auth/oauth.go` around lines 214 - 218, The deferred cleanup in the auth-client creation flow should not discard errors from authClient.Close(). Update the defer associated with NewAuth to capture any Close error and log it at debug level, matching the existing newAuth cleanup handling and preserving the current return behavior.Source: Coding guidelines
287-299: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard
flowInitErroragainst an empty error list.If
errsis empty,allMatchreturns false anderrors.Join()returns nil. The finalfmt.Errorf("initialize authorization flow: %w", nil)then produces the text%!w(<nil>). The current callers always pass at least one error, so this is a latent trap only. Return an explicit error for the empty case.🤖 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/internal/auth/oauth.go` around lines 287 - 299, Update flowInitError to handle an empty errs slice before the allMatch checks and return an explicit initialization error without calling errors.Join on nil; preserve the existing SSO-specific and joined-error behavior when errs contains errors.client/internal/auth/device_flow.go (1)
222-227: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider narrowing the OAuth error codes that trigger a flow change.
unsupported_grant_typeandunauthorized_clientindicate a disabled device grant.invalid_clientusually indicates wrong client credentials, which affects the PKCE flow in the same way. For that case the client changes flow and then fails again, and the user sees the PKCE error instead of the credential error. The original error is preserved when no other flow is configured, so the impact is limited to the message the user reads.🤖 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/internal/auth/device_flow.go` around lines 222 - 227, Update the OAuth error handling switch around oauthErr.Error to treat only unsupported_grant_type and unauthorized_client as signals to change flows; remove invalid_client from that set so credential errors do not trigger a misleading PKCE fallback, while preserving the existing default false behavior.client/cmd/login.go (2)
413-417: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated setup-key guidance text. Both call sites append the same sentence and documentation URL after
auth.IsSSOUnavailable. The two copies can drift, and the URL must then be updated twice.
client/cmd/login.go#L413-L417: call a shared helper that adds the enrollment guidance to an SSO-unavailable error.client/server/server.go#L685-L689: call the same helper here.🤖 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/cmd/login.go` around lines 413 - 417, The setup-key guidance appended to SSO-unavailable errors is duplicated. Add a shared helper for constructing this enriched error, then update client/cmd/login.go lines 413-417 and client/server/server.go lines 685-689 to call it instead of formatting the message locally, keeping the guidance text and URL centralized.
123-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm that the
IsUnixDesktopClientproto field now carries graphical-session state.The CLI now sends
util.HasGraphicalSession()inIsUnixDesktopClient. The daemon forwards this field into thehasGraphicalSessionparameter ofauth.NewOAuthFlow(seeclient/server/server.goLine 682). The names no longer match the meaning, which makes the flow-selection contract hard to follow. Renaming the wire field would break compatibility, so add a comment inclient/proto/daemon.protothat documents the current meaning, or introduce a new field and deprecate this one.🤖 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/cmd/login.go` at line 123, Document the current meaning of the IsUnixDesktopClient proto field in client/proto/daemon.proto, stating that it carries graphical-session state used by auth.NewOAuthFlow rather than strictly identifying a Unix desktop client. Preserve the existing field and wire compatibility; do not rename it.client/internal/auth/oauth_test.go (1)
160-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for a failing
authFactory.
fallbackFlow.initNextreturns early whennewAuthfails, andRequestAuthInfothen returns the original error.stubAuthFactoryalways succeeds, so this branch stays untested. A subtest with a factory that returns an error would cover it.🤖 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/internal/auth/oauth_test.go` around lines 160 - 207, Add a subtest in TestFallbackFlowRequestAuthInfo that uses an authFactory returning an error, exercises the fallback path through RequestAuthInfo, and verifies the factory error is returned while the original flow remains active. Reuse the existing flow setup and assertions where applicable, targeting fallbackFlow.initNext behavior rather than changing production code.
🤖 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/internal/auth/oauth.go`:
- Around line 144-153: The fallback path in initNext must not read the shared
Auth.client field without synchronization. In
client/internal/auth/oauth.go:144-153, obtain the management client through the
mutex-protected accessor or pass a captured client into initFirstAvailableFlow;
in client/internal/auth/auth.go:145-148, return the client already captured
under RLock by withRetry so the shared field is not reread. Run go test -race
./client/internal/auth/... after the change.
In `@client/server/server.go`:
- Around line 685-689: Update the auth.IsSSOUnavailable(err) branch to return a
gRPC status error using a terminal code handled by the login CLI, while
preserving the existing setup-key guidance and underlying error context.
In `@util/common.go`:
- Around line 19-24: Define private constants for all browser graphical-session
environment-variable names in util/common.go, then build browserSessionEnvVars
and the session-type lookup at util/common.go lines 19-24 and 44-44 from those
constants. Update util/session_test.go lines 18-18 and 30-38 to use the same
shared constants when clearing inherited state and populating table env values,
keeping names centralized and eliminating duplicate literals.
---
Nitpick comments:
In `@client/cmd/login.go`:
- Around line 413-417: The setup-key guidance appended to SSO-unavailable errors
is duplicated. Add a shared helper for constructing this enriched error, then
update client/cmd/login.go lines 413-417 and client/server/server.go lines
685-689 to call it instead of formatting the message locally, keeping the
guidance text and URL centralized.
- Line 123: Document the current meaning of the IsUnixDesktopClient proto field
in client/proto/daemon.proto, stating that it carries graphical-session state
used by auth.NewOAuthFlow rather than strictly identifying a Unix desktop
client. Preserve the existing field and wire compatibility; do not rename it.
In `@client/internal/auth/auth.go`:
- Around line 154-157: Update the error classification in the retry flow to call
IsSSOUnavailable(err) instead of performing a local errors.As check against
ssoUnavailableError, while preserving the existing backoff.Permanent(err)
behavior.
In `@client/internal/auth/device_flow.go`:
- Around line 222-227: Update the OAuth error handling switch around
oauthErr.Error to treat only unsupported_grant_type and unauthorized_client as
signals to change flows; remove invalid_client from that set so credential
errors do not trigger a misleading PKCE fallback, while preserving the existing
default false behavior.
In `@client/internal/auth/oauth_test.go`:
- Around line 160-207: Add a subtest in TestFallbackFlowRequestAuthInfo that
uses an authFactory returning an error, exercises the fallback path through
RequestAuthInfo, and verifies the factory error is returned while the original
flow remains active. Reuse the existing flow setup and assertions where
applicable, targeting fallbackFlow.initNext behavior rather than changing
production code.
In `@client/internal/auth/oauth.go`:
- Around line 214-218: The deferred cleanup in the auth-client creation flow
should not discard errors from authClient.Close(). Update the defer associated
with NewAuth to capture any Close error and log it at debug level, matching the
existing newAuth cleanup handling and preserving the current return behavior.
- Around line 287-299: Update flowInitError to handle an empty errs slice before
the allMatch checks and return an explicit initialization error without calling
errors.Join on nil; preserve the existing SSO-specific and joined-error behavior
when errs contains errors.
🪄 Autofix
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: 88bcee06-e97e-4ba7-bf44-fc6ee32183bd
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
client/cmd/login.goclient/cmd/up.goclient/internal/auth/auth.goclient/internal/auth/device_flow.goclient/internal/auth/oauth.goclient/internal/auth/oauth_test.goclient/internal/auth/pkce_flow.goclient/proto/daemon.protoclient/server/server.goclient/ssh/common.goclient/ui/authsession/service.goclient/ui/services/connection.goutil/common.goutil/session_test.go
| a, cleanup, err := f.newAuth(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer cleanup() | ||
|
|
||
| flow, remaining, err := initFirstAvailableFlow(a, a.client, f.remaining, f.hint) | ||
| if err != nil { | ||
| log.Debugf("failed to initialize pkce authentication with error: %v\n", err) | ||
| log.Debug("falling back to device code flow") | ||
| return authenticateWithDeviceCodeFlow(ctx, config, hint) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unsynchronized read of Auth.client in the fallback path. initNext reads a.client directly, while Auth.reconnect can reassign that field and Auth.Close can set the connection unusable, both under Auth.mutex. In the GetOAuthFlow path the factory returns the same long-lived Auth, and withRetry reconnects on connection errors, so a fallback can read the field while another goroutine replaces it.
client/internal/auth/oauth.go#L144-L153: read the management client through an accessor that takesAuth.mutex(asAuth.CloseandwithRetrydo) instead of readinga.clientdirectly, or pass the client in from the caller.client/internal/auth/auth.go#L145-L148: this factory hands out the shareda, which makes the unsynchronized read reachable; return the client thatwithRetryalready captured underRLockso the fallback does not re-read the field.
Run go test -race ./client/internal/auth/... after the change.
As per coding guidelines: "Protect shared mutable state with a mutex, atomic, or channel as appropriate; perform a two-pass race analysis and run go test -race on touched packages after concurrency changes" and "Do not read a mutable struct field inside a goroutine when another goroutine can nil or reassign it; capture it locally or pass it as a parameter".
📍 Affects 2 files
client/internal/auth/oauth.go#L144-L153(this comment)client/internal/auth/auth.go#L145-L148
🤖 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/internal/auth/oauth.go` around lines 144 - 153, The fallback path in
initNext must not read the shared Auth.client field without synchronization. In
client/internal/auth/oauth.go:144-153, obtain the management client through the
mutex-protected accessor or pass a captured client into initFirstAvailableFlow;
in client/internal/auth/auth.go:145-148, return the client already captured
under RLock by withRetry so the shared field is not reread. Run go test -race
./client/internal/auth/... after the change.
Source: Coding guidelines
| // enrolling a device is the one flow a setup key can replace | ||
| if auth.IsSSOUnavailable(err) { | ||
| return nil, fmt.Errorf("%w. Set this device up with a setup key instead: "+ | ||
| "https://docs.netbird.io/how-to/register-machines-using-setup-keys", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return a terminal gRPC status code for the SSO-unavailable case.
This branch reports a permanent condition: the management server offers no usable SSO flow. fmt.Errorf produces codes.Unknown at the RPC boundary. In client/cmd/login.go Lines 148-156, the CLI stops its backoff loop only for InvalidArgument, PermissionDenied, NotFound, and Unimplemented. With codes.Unknown the CLI retries the login until the backoff window expires, so the user waits instead of seeing the setup-key guidance.
Wrap the message in a status with a code that the CLI treats as terminal.
🔧 Proposed fix
// enrolling a device is the one flow a setup key can replace
if auth.IsSSOUnavailable(err) {
- return nil, fmt.Errorf("%w. Set this device up with a setup key instead: "+
- "https://docs.netbird.io/how-to/register-machines-using-setup-keys", err)
+ return nil, gstatus.Errorf(codes.NotFound,
+ "%v. Set this device up with a setup key instead: "+
+ "https://docs.netbird.io/how-to/register-machines-using-setup-keys", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // enrolling a device is the one flow a setup key can replace | |
| if auth.IsSSOUnavailable(err) { | |
| return nil, fmt.Errorf("%w. Set this device up with a setup key instead: "+ | |
| "https://docs.netbird.io/how-to/register-machines-using-setup-keys", err) | |
| } | |
| // enrolling a device is the one flow a setup key can replace | |
| if auth.IsSSOUnavailable(err) { | |
| return nil, gstatus.Errorf(codes.NotFound, | |
| "%v. Set this device up with a setup key instead: "+ | |
| "https://docs.netbird.io/how-to/register-machines-using-setup-keys", err) | |
| } |
🤖 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/server/server.go` around lines 685 - 689, Update the
auth.IsSSOUnavailable(err) branch to return a gRPC status error using a terminal
code handled by the login CLI, while preserving the existing setup-key guidance
and underlying error context.
| // browserSessionEnvVars returns the variables that decide whether OpenBrowser can open a URL: | ||
| // BROWSER is the explicit override it honors first, DESKTOP_SESSION and XDG_CURRENT_DESKTOP are | ||
| // what xdg-open uses to pick a handler, and DISPLAY / WAYLAND_DISPLAY are what any graphical | ||
| // browser it launches needs. | ||
| func browserSessionEnvVars() []string { | ||
| return []string{"BROWSER", "DESKTOP_SESSION", "XDG_CURRENT_DESKTOP", "DISPLAY", "WAYLAND_DISPLAY"} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Define the graphical-session environment-variable names once.
The production helper and its tests use separate string literals for the same environment-variable names. Define private constants in util/common.go and use them at every site.
util/common.go#L19-L24: buildbrowserSessionEnvVarsfrom private environment-variable constants.util/common.go#L44-L44: use the XDG session-type constant for the lookup.util/session_test.go#L18-L18: use the XDG session-type constant when clearing inherited state.util/session_test.go#L30-L38: use the shared constants as tableenvvalues.
As per coding guidelines: “Declare environment-variable names as constants.”
📍 Affects 2 files
util/common.go#L19-L24(this comment)util/common.go#L44-L44util/session_test.go#L18-L18util/session_test.go#L30-L38
🤖 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 `@util/common.go` around lines 19 - 24, Define private constants for all
browser graphical-session environment-variable names in util/common.go, then
build browserSessionEnvVars and the session-type lookup at util/common.go lines
19-24 and 44-44 from those constants. Update util/session_test.go lines 18-18
and 30-38 to use the same shared constants when clearing inherited state and
populating table env values, keeping names centralized and eliminating duplicate
literals.
Source: Coding guidelines
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. |
1 participant
Describe your changes
Clients on Linux hosts pick the device code flow whenever no desktop environment is detected, and stay on it even when the server or the identity provider does not offer that flow, which fails the login instead of using PKCE.
BROWSER,DESKTOP_SESSION,XDG_CURRENT_DESKTOP,DISPLAY,WAYLAND_DISPLAY,XDG_SESSION_TYPE), and let the desktop UI report a graphical session outright instead of inferring onenetbird login --extendand SSH authentication always choosing the device code flow on Linux: both run in the daemon, which does not inherit the user's session environment, so they now take the answer from the caller, and their errors no longer suggest setup keys, which cannot extend a session or authenticate SSHIssue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
The flow selection is internal to the client; no configuration or user-facing option changes.
Summary by CodeRabbit
New Features
Bug Fixes