From 9230ebf85d6d8a0d1a2638132829703cf74f3484 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Wed, 12 Aug 2026 18:56:47 +0200 Subject: [PATCH] Fall back to an available OAuth flow when the preferred one is not configured --- client/cmd/login.go | 2 +- client/internal/auth/auth.go | 36 ++-- client/internal/auth/device_flow.go | 47 +++- client/internal/auth/oauth.go | 321 +++++++++++++++++++++++----- client/internal/auth/oauth_test.go | 237 ++++++++++++++++++++ client/internal/auth/pkce_flow.go | 11 +- client/server/server.go | 5 + 7 files changed, 588 insertions(+), 71 deletions(-) create mode 100644 client/internal/auth/oauth_test.go diff --git a/client/cmd/login.go b/client/cmd/login.go index 6aa019896..43e7cdda3 100644 --- a/client/cmd/login.go +++ b/client/cmd/login.go @@ -410,7 +410,7 @@ func foregroundGetTokenInfo(ctx context.Context, cmd *cobra.Command, config *pro oAuthFlow, err := auth.NewOAuthFlow(ctx, config, util.HasGraphicalSession(), false, hint) if err != nil { - return nil, err + return nil, auth.WithSetupKeyAdvice(err) } flowInfo, err := oAuthFlow.RequestAuthInfo(context.TODO()) diff --git a/client/internal/auth/auth.go b/client/internal/auth/auth.go index 153727a6c..8cb930d4b 100644 --- a/client/internal/auth/auth.go +++ b/client/internal/auth/auth.go @@ -83,6 +83,15 @@ func NewAuth(ctx context.Context, privateKey string, mgmURL *url.URL, config *pr }, nil } +// grpcClient returns the current management connection. Callers must go through it rather than +// reading a.client: reconnect replaces that field while other goroutines are using it. +func (a *Auth) grpcClient() *mgm.GrpcClient { + a.mutex.RLock() + defer a.mutex.RUnlock() + + return a.client +} + // Close closes the management client connection func (a *Auth) Close() error { a.mutex.Lock() @@ -140,25 +149,20 @@ func (a *Auth) IsSSOSupported(ctx context.Context) (bool, error) { // This avoids creating a new connection to the management server func (a *Auth) GetOAuthFlow(ctx context.Context, forceDeviceAuth bool) (OAuthFlow, error) { var flow OAuthFlow - var err error - err = a.withRetry(ctx, func(client *mgm.GrpcClient) error { - if forceDeviceAuth { - flow, err = a.getDeviceFlow(client) - return err - } + // the connection is owned by a and outlives this call, so a later fallback reuses it + newAuth := func(context.Context) (*Auth, func(), error) { + return a, func() {}, nil + } - // Try PKCE flow first - flow, err = a.getPKCEFlow(client) - if err != nil { - // If PKCE not supported, try Device flow - if s, ok := status.FromError(err); ok && (s.Code() == codes.NotFound || s.Code() == codes.Unimplemented) { - flow, err = a.getDeviceFlow(client) - return err - } - return err + err := a.withRetry(ctx, func(client *mgm.GrpcClient) error { + var err error + flow, err = oauthFlowWithFallback(a, client, flowOrder(forceDeviceAuth), "", newAuth) + + if IsSSOUnavailable(err) { + return backoff.Permanent(err) } - return nil + return err }) return flow, err diff --git a/client/internal/auth/device_flow.go b/client/internal/auth/device_flow.go index 9dec7cf53..b0739322f 100644 --- a/client/internal/auth/device_flow.go +++ b/client/internal/auth/device_flow.go @@ -48,8 +48,17 @@ type DeviceAuthProviderConfig struct { LoginHint string } -// validateDeviceAuthConfig validates device authorization provider configuration +// validateDeviceAuthConfig validates device authorization provider configuration. A missing +// value means management does not have this flow configured, so the error wraps +// errFlowNotConfigured and the caller can fall back to the other flow. func validateDeviceAuthConfig(config *DeviceAuthProviderConfig) error { + if err := checkDeviceAuthConfig(config); err != nil { + return fmt.Errorf("%w: %w", errFlowNotConfigured, err) + } + return nil +} + +func checkDeviceAuthConfig(config *DeviceAuthProviderConfig) error { errorMsgFormat := "invalid provider configuration received from management: %s value is empty. Contact your NetBird administrator" if config.Audience == "" { @@ -161,8 +170,12 @@ func (d *DeviceAuthorizationFlow) RequestAuthInfo(ctx context.Context) (AuthFlow return AuthFlowInfo{}, fmt.Errorf("reading body failed with error: %v", err) } - if res.StatusCode != 200 { - return AuthFlowInfo{}, fmt.Errorf("request device code returned status %d error: %s", res.StatusCode, string(body)) + if res.StatusCode != http.StatusOK { + reqErr := fmt.Errorf("request device code returned status %d error: %s", res.StatusCode, string(body)) + if deviceGrantUnsupported(res.StatusCode, body) { + return AuthFlowInfo{}, fmt.Errorf("%w: %w", errFlowNotConfigured, reqErr) + } + return AuthFlowInfo{}, reqErr } deviceCode := AuthFlowInfo{} @@ -186,6 +199,34 @@ func (d *DeviceAuthorizationFlow) RequestAuthInfo(ctx context.Context) (AuthFlow return deviceCode, err } +// deviceGrantUnsupported reports whether the IdP's answer to a device code request means it does +// not serve the device authorization grant at all, rather than a transient or request-specific +// failure. An IdP that does not route the endpoint answers 404/405/501; one that knows the +// endpoint but has the grant disabled for this client answers with an OAuth 2.0 error code. +func deviceGrantUnsupported(statusCode int, body []byte) bool { + switch statusCode { + case http.StatusNotFound, http.StatusMethodNotAllowed, http.StatusNotImplemented: + return true + case http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden: + default: + return false + } + + var oauthErr struct { + Error string `json:"error"` + } + if err := json.Unmarshal(body, &oauthErr); err != nil { + return false + } + + switch oauthErr.Error { + case "unsupported_grant_type", "unauthorized_client": + return true + default: + return false + } +} + func appendLoginHint(uri, loginHint string) string { if uri == "" || loginHint == "" { return uri diff --git a/client/internal/auth/oauth.go b/client/internal/auth/oauth.go index a50a2ce6f..977398058 100644 --- a/client/internal/auth/oauth.go +++ b/client/internal/auth/oauth.go @@ -2,15 +2,19 @@ package auth import ( "context" + "errors" "fmt" "net/http" + "net/url" "runtime" + "sync" log "github.com/sirupsen/logrus" "google.golang.org/grpc/codes" gstatus "google.golang.org/grpc/status" "github.com/netbirdio/netbird/client/internal/profilemanager" + mgm "github.com/netbirdio/netbird/shared/management/client" ) // OAuthFlow represents an interface for authorization using different OAuth 2.0 flows @@ -59,77 +63,294 @@ func (t TokenInfo) GetTokenToUse() string { return t.AccessToken } -func shouldUseDeviceFlow(force bool, isUnixDesktopClient bool) bool { - return force || (runtime.GOOS == "linux" || runtime.GOOS == "freebsd") && !isUnixDesktopClient +// errFlowNotConfigured marks a flow this deployment does not offer: management returned no +// configuration for it, the configuration it returned is incomplete, or the IdP refuses to serve +// the grant. It is the only condition that makes the client try the other flow, so that a +// transient failure keeps failing on the flow the user actually wants. +var errFlowNotConfigured = errors.New("authorization flow is not configured") + +// ssoUnavailableError reports that the management server offers no usable SSO flow at all. +// Retrying cannot help, so callers should surface it to the user instead of backing off. +type ssoUnavailableError struct { + msg string } -// NewOAuthFlow initializes and returns the appropriate OAuth flow based on the management configuration -// -// It starts by initializing the PKCE.If this process fails, it resorts to the Device Code Flow, -// and if that also fails, the authentication process is deemed unsuccessful -// -// On Linux distros without desktop environment support, it only tries to initialize the Device Code Flow -// forceDeviceCodeFlow can be used to skip PKCE and go directly to Device Code Flow (e.g., for Android TV) -func NewOAuthFlow(ctx context.Context, config *profilemanager.Config, isUnixDesktopClient bool, forceDeviceCodeFlow bool, hint string) (OAuthFlow, error) { - if shouldUseDeviceFlow(forceDeviceCodeFlow, isUnixDesktopClient) { - return authenticateWithDeviceCodeFlow(ctx, config, hint) - } - - pkceFlow, err := authenticateWithPKCEFlow(ctx, config, 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 pkceFlow, nil +func (e *ssoUnavailableError) Error() string { + return e.msg } -// authenticateWithPKCEFlow initializes the Proof Key for Code Exchange flow auth flow -func authenticateWithPKCEFlow(ctx context.Context, config *profilemanager.Config, hint string) (OAuthFlow, error) { - authClient, err := NewAuth(ctx, config.PrivateKey, config.ManagementURL, config) - if err != nil { - return nil, fmt.Errorf("failed to create auth client: %v", err) - } - defer authClient.Close() +// oauthFlowInit names one of the OAuth flows and builds it from the management configuration. +type oauthFlowInit struct { + name string + init func(a *Auth, client *mgm.GrpcClient, hint string) (OAuthFlow, error) +} - pkceFlowInfo, err := authClient.getPKCEFlow(authClient.client) +// authFactory hands out a management connection to build a flow with, plus the cleanup that +// releases it. Callers that own a long-lived connection return it with a no-op cleanup. +type authFactory func(ctx context.Context) (*Auth, func(), error) + +// fallbackFlow wraps the flow that was picked at initialization time with the flows that were +// not tried. Whether the IdP actually serves a flow only shows up when the flow is run: an IdP +// with the device grant disabled answers the device code request with 404 even though +// management handed out a device flow configuration. When that happens the wrapper swaps in the +// next flow instead of failing the login. +type fallbackFlow struct { + mu sync.Mutex + active OAuthFlow + remaining []oauthFlowInit + hint string + newAuth authFactory +} + +func (f *fallbackFlow) RequestAuthInfo(ctx context.Context) (AuthFlowInfo, error) { + info, err := f.current().RequestAuthInfo(ctx) + if err == nil || !isFlowUnavailable(err) { + return info, err + } + + next, nextErr := f.initNext(ctx) + if nextErr != nil { + log.Debugf("failed to fall back to another authorization flow: %v", nextErr) + return AuthFlowInfo{}, err + } + + return next.RequestAuthInfo(ctx) +} + +func (f *fallbackFlow) WaitToken(ctx context.Context, info AuthFlowInfo) (TokenInfo, error) { + return f.current().WaitToken(ctx, info) +} + +func (f *fallbackFlow) GetClientID(ctx context.Context) string { + return f.current().GetClientID(ctx) +} + +func (f *fallbackFlow) current() OAuthFlow { + f.mu.Lock() + defer f.mu.Unlock() + + return f.active +} + +// initNext initializes the next flow this deployment offers and makes it the active one. +func (f *fallbackFlow) initNext(ctx context.Context) (OAuthFlow, error) { + f.mu.Lock() + defer f.mu.Unlock() + + if len(f.remaining) == 0 { + return nil, errors.New("no authorization flow left to try") + } + + a, cleanup, err := f.newAuth(ctx) if err != nil { - return nil, fmt.Errorf("getting pkce authorization flow info failed with error: %v", err) + return nil, err + } + defer cleanup() + + flow, remaining, err := initFirstAvailableFlow(a, a.grpcClient(), f.remaining, f.hint) + if err != nil { + return nil, err + } + + log.Infof("the identity provider does not serve the selected authorization flow, continuing with the next one") + f.active = flow + f.remaining = remaining + + return flow, nil +} + +// preferDeviceFlow reports whether the device code flow should be tried before PKCE. PKCE needs +// a browser on this host and a loopback listener to receive the redirect, neither of which +// exists on a Unix host without a graphical session. The GOOS guard keeps a caller that reports +// no graphical session on a platform that always has one from changing the preference. +func preferDeviceFlow(force bool, hasGraphicalSession bool) bool { + return force || (runtime.GOOS == "linux" || runtime.GOOS == "freebsd") && !hasGraphicalSession +} + +// flowOrder returns both flows in the order they should be attempted. +func flowOrder(preferDevice bool) []oauthFlowInit { + pkce := oauthFlowInit{name: "pkce authorization flow", init: initPKCEFlow} + device := oauthFlowInit{name: "device code flow", init: initDeviceFlow} + + if preferDevice { + return []oauthFlowInit{device, pkce} + } + return []oauthFlowInit{pkce, device} +} + +func initPKCEFlow(a *Auth, client *mgm.GrpcClient, hint string) (OAuthFlow, error) { + flow, err := a.getPKCEFlow(client) + if err != nil { + return nil, err } if hint != "" { - pkceFlowInfo.SetLoginHint(hint) + flow.SetLoginHint(hint) } - return pkceFlowInfo, nil + return flow, nil } -// authenticateWithDeviceCodeFlow initializes the Device Code auth Flow -func authenticateWithDeviceCodeFlow(ctx context.Context, config *profilemanager.Config, hint string) (OAuthFlow, error) { +func initDeviceFlow(a *Auth, client *mgm.GrpcClient, hint string) (OAuthFlow, error) { + flow, err := a.getDeviceFlow(client) + if err != nil { + return nil, err + } + + if hint != "" { + flow.SetLoginHint(hint) + } + + return flow, nil +} + +// NewOAuthFlow initializes and returns an OAuth flow based on the management configuration. +// +// Both flows are optional server side: management answers NotFound for a flow it has no +// configuration for. The preferred flow is tried first and the other one is used as a fallback, +// so a server that only offers one of them still works. forceDeviceCodeFlow prefers the device +// code flow regardless of platform (e.g. for Android TV). +func NewOAuthFlow(ctx context.Context, config *profilemanager.Config, hasGraphicalSession bool, forceDeviceCodeFlow bool, hint string) (OAuthFlow, error) { authClient, err := NewAuth(ctx, config.PrivateKey, config.ManagementURL, config) if err != nil { - return nil, fmt.Errorf("failed to create auth client: %v", err) + return nil, fmt.Errorf("create auth client: %w", err) } - defer authClient.Close() + defer func() { + if err := authClient.Close(); err != nil { + log.Debugf("failed to close auth client: %v", err) + } + }() - deviceFlowInfo, err := authClient.getDeviceFlow(authClient.client) + // the connection above is closed on return, so a later fallback opens its own + newAuth := func(ctx context.Context) (*Auth, func(), error) { + a, err := NewAuth(ctx, config.PrivateKey, config.ManagementURL, config) + if err != nil { + return nil, nil, fmt.Errorf("create auth client: %w", err) + } + return a, func() { + if err := a.Close(); err != nil { + log.Debugf("failed to close auth client: %v", err) + } + }, nil + } + + flows := flowOrder(preferDeviceFlow(forceDeviceCodeFlow, hasGraphicalSession)) + return oauthFlowWithFallback(authClient, authClient.grpcClient(), flows, hint, newAuth) +} + +// oauthFlowWithFallback initializes the first flow this deployment offers, moving on to the next +// one when a flow is not configured here. It only fails once every flow has been tried, and any +// flow left untried is handed to the returned flow so it can still fall back if the IdP rejects +// the flow that was picked. +func oauthFlowWithFallback(a *Auth, client *mgm.GrpcClient, flows []oauthFlowInit, hint string, newAuth authFactory) (OAuthFlow, error) { + flow, remaining, err := initFirstAvailableFlow(a, client, flows, hint) if err != nil { - switch s, ok := gstatus.FromError(err); { - case ok && s.Code() == codes.NotFound: - return nil, fmt.Errorf("no SSO provider returned from management. " + - "Please proceed with setting up this device using setup keys " + - "https://docs.netbird.io/how-to/register-machines-using-setup-keys") - case ok && s.Code() == codes.Unimplemented: - return nil, fmt.Errorf("the management server, %s, does not support SSO providers, "+ - "please update your server or use Setup Keys to login", config.ManagementURL) - default: - return nil, fmt.Errorf("getting device authorization flow info failed with error: %v", err) + return nil, err + } + + if len(remaining) == 0 { + return flow, nil + } + + return &fallbackFlow{ + active: flow, + remaining: remaining, + hint: hint, + newAuth: newAuth, + }, nil +} + +// initFirstAvailableFlow returns the first flow that could be initialized along with the flows +// after it, which are still untried. +func initFirstAvailableFlow(a *Auth, client *mgm.GrpcClient, flows []oauthFlowInit, hint string) (OAuthFlow, []oauthFlowInit, error) { + var errs []error + for i, f := range flows { + flow, err := f.init(a, client, hint) + if err == nil { + return flow, flows[i+1:], nil + } + + errs = append(errs, fmt.Errorf("%s: %w", f.name, err)) + + // only a flow this deployment does not offer is worth replacing with another one + if !isFlowUnavailable(err) { + break + } + if i < len(flows)-1 { + log.Infof("%s is not configured (%v), falling back to %s", f.name, err, flows[i+1].name) } } - if hint != "" { - deviceFlowInfo.SetLoginHint(hint) + return nil, nil, flowInitError(a.mgmURL, errs) +} + +// flowInitError turns the per-flow initialization errors into a single actionable error. The +// message stays neutral about what to do instead: SSO is also how a peer extends its session and +// authenticates SSH, where a setup key is no alternative. Callers that are enrolling a device add +// that advice themselves, see IsSSOUnavailable. +func flowInitError(mgmURL *url.URL, errs []error) error { + if allMatch(errs, isFlowUnimplemented) { + return &ssoUnavailableError{msg: fmt.Sprintf("the management server, %s, does not support SSO providers, "+ + "please update your server", mgmURL)} } - return deviceFlowInfo, nil + if allMatch(errs, isFlowUnavailable) { + return &ssoUnavailableError{msg: "the management server has no SSO provider configured: " + + "neither the pkce authorization flow nor the device code flow is available"} + } + + return fmt.Errorf("initialize authorization flow: %w", errors.Join(errs...)) +} + +// IsSSOUnavailable reports whether err means the management server offers no usable SSO flow, so +// no retry and no other flow can help. Enrollment paths use it to point the user at setup keys. +func IsSSOUnavailable(err error) bool { + var ssoUnavailable *ssoUnavailableError + return errors.As(err, &ssoUnavailable) +} + +// WithSetupKeyAdvice appends enrollment guidance to an SSO-unavailable error and returns any +// other error unchanged. Only enrollment can fall back to a setup key: extending a session and +// authenticating SSH cannot, so those paths must not call this. +func WithSetupKeyAdvice(err error) error { + if !IsSSOUnavailable(err) { + return err + } + + return fmt.Errorf("%w. Set this device up with a setup key instead: "+ + "https://docs.netbird.io/how-to/register-machines-using-setup-keys", err) +} + +func allMatch(errs []error, match func(error) bool) bool { + if len(errs) == 0 { + return false + } + + for _, err := range errs { + if !match(err) { + return false + } + } + return true +} + +// isFlowUnavailable reports whether the flow is not on offer here: management has no +// configuration for it (NotFound), predates the RPC entirely (Unimplemented), returned an +// incomplete configuration, or the IdP does not serve the grant. +func isFlowUnavailable(err error) bool { + return errors.Is(err, errFlowNotConfigured) || + hasStatusCode(err, codes.NotFound) || + hasStatusCode(err, codes.Unimplemented) +} + +func isFlowUnimplemented(err error) bool { + return hasStatusCode(err, codes.Unimplemented) +} + +func hasStatusCode(err error, code codes.Code) bool { + s, ok := gstatus.FromError(err) + if !ok { + return false + } + return s.Code() == code } diff --git a/client/internal/auth/oauth_test.go b/client/internal/auth/oauth_test.go new file mode 100644 index 000000000..21363cdcb --- /dev/null +++ b/client/internal/auth/oauth_test.go @@ -0,0 +1,237 @@ +package auth + +import ( + "context" + "errors" + "fmt" + "net/url" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + mgm "github.com/netbirdio/netbird/shared/management/client" +) + +// stubFlow is a minimal OAuthFlow returned by the fake initializers below. requestErr, when set, +// is what its RequestAuthInfo returns, standing in for an IdP that rejects the flow. +type stubFlow struct { + name string + hint string + requestErr error +} + +func (s *stubFlow) RequestAuthInfo(context.Context) (AuthFlowInfo, error) { + if s.requestErr != nil { + return AuthFlowInfo{}, s.requestErr + } + return AuthFlowInfo{UserCode: s.name}, nil +} + +func (s *stubFlow) WaitToken(context.Context, AuthFlowInfo) (TokenInfo, error) { + return TokenInfo{}, nil +} + +func (s *stubFlow) GetClientID(context.Context) string { + return "" +} + +// stubInit returns a flow initializer that yields a named stub flow, or err when err is non-nil. +func stubInit(name string, err error) oauthFlowInit { + return stubInitFlow(name, err, nil) +} + +// stubInitFlow is stubInit with control over what the resulting flow's RequestAuthInfo returns. +func stubInitFlow(name string, initErr, requestErr error) oauthFlowInit { + return oauthFlowInit{ + name: name, + init: func(_ *Auth, _ *mgm.GrpcClient, hint string) (OAuthFlow, error) { + if initErr != nil { + return nil, initErr + } + return &stubFlow{name: name, hint: hint, requestErr: requestErr}, nil + }, + } +} + +// stubAuthFactory hands out an Auth without a management connection, which the stub +// initializers above never touch. +func stubAuthFactory(a *Auth) authFactory { + return func(context.Context) (*Auth, func(), error) { + return a, func() {}, nil + } +} + +func TestOAuthFlowWithFallback(t *testing.T) { + notFound := status.Error(codes.NotFound, "no device authorization flow information available") + unimplemented := status.Error(codes.Unimplemented, "unknown method") + incompleteConfig := fmt.Errorf("%w: Client ID value is empty", errFlowNotConfigured) + unreachable := status.Error(codes.Unavailable, "connection refused") + + tests := []struct { + name string + flows []oauthFlowInit + expectedFlow string + expectedErr string + expectedNoSSO bool + }{ + { + name: "preferred flow is used", + flows: []oauthFlowInit{stubInit("device", nil), stubInit("pkce", nil)}, + expectedFlow: "device", + }, + { + // the RedHat case: device code flow disabled on management, PKCE configured + name: "falls back when preferred flow is not configured", + flows: []oauthFlowInit{stubInit("device", notFound), stubInit("pkce", nil)}, + expectedFlow: "pkce", + }, + { + name: "falls back on an incomplete flow configuration", + flows: []oauthFlowInit{stubInit("pkce", incompleteConfig), stubInit("device", nil)}, + expectedFlow: "device", + }, + { + name: "does not fall back when the preferred flow fails for another reason", + flows: []oauthFlowInit{stubInit("pkce", unreachable), stubInit("device", nil)}, + expectedErr: "connection refused", + }, + { + // stays neutral about the remedy: --extend and SSH auth cannot use a setup key + name: "neither flow configured reports no SSO provider", + flows: []oauthFlowInit{stubInit("device", notFound), stubInit("pkce", notFound)}, + expectedErr: "no SSO provider configured", + expectedNoSSO: true, + }, + { + name: "old server without the flow RPCs asks for an update", + flows: []oauthFlowInit{stubInit("device", unimplemented), stubInit("pkce", unimplemented)}, + expectedErr: "does not support SSO providers", + expectedNoSSO: true, + }, + } + + mgmURL, err := url.Parse("https://api.netbird.io:443") + require.NoError(t, err) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := &Auth{mgmURL: mgmURL} + flow, err := oauthFlowWithFallback(a, nil, tt.flows, "user@example.com", stubAuthFactory(a)) + + if tt.expectedErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.expectedErr) + + var ssoUnavailable *ssoUnavailableError + assert.Equal(t, tt.expectedNoSSO, errors.As(err, &ssoUnavailable), + "terminal SSO-unavailable classification mismatch for %v", err) + return + } + + require.NoError(t, err) + stub := activeStub(t, flow) + assert.Equal(t, tt.expectedFlow, stub.name) + assert.Equal(t, "user@example.com", stub.hint, "login hint must be passed to the flow") + }) + } +} + +// activeStub unwraps the flow currently in use, which is behind a fallbackFlow whenever an +// untried flow is left. +func activeStub(t *testing.T, flow OAuthFlow) *stubFlow { + t.Helper() + + if fallback, ok := flow.(*fallbackFlow); ok { + flow = fallback.current() + } + + stub, ok := flow.(*stubFlow) + require.True(t, ok, "unexpected flow type %T", flow) + return stub +} + +// TestFallbackFlowRequestAuthInfo covers the failure the RedHat report hit: management hands out +// a device flow configuration, but the IdP does not serve the grant and only says so when the +// device code is requested. +func TestFallbackFlowRequestAuthInfo(t *testing.T) { + mgmURL, err := url.Parse("https://api.netbird.io:443") + require.NoError(t, err) + a := &Auth{mgmURL: mgmURL} + + idpRejects := fmt.Errorf("%w: request device code returned status 404", errFlowNotConfigured) + + t.Run("swaps in the untried flow", func(t *testing.T) { + flows := []oauthFlowInit{stubInitFlow("device", nil, idpRejects), stubInit("pkce", nil)} + + flow, err := oauthFlowWithFallback(a, nil, flows, "", stubAuthFactory(a)) + require.NoError(t, err) + require.Equal(t, "device", activeStub(t, flow).name) + + info, err := flow.RequestAuthInfo(context.Background()) + require.NoError(t, err) + assert.Equal(t, "pkce", info.UserCode, "the request must be served by the fallback flow") + assert.Equal(t, "pkce", activeStub(t, flow).name, "the fallback flow must stay active for WaitToken") + }) + + t.Run("keeps the original error when nothing else is configured", func(t *testing.T) { + flows := []oauthFlowInit{ + stubInitFlow("device", nil, idpRejects), + stubInit("pkce", status.Error(codes.NotFound, "no pkce authorization flow information available")), + } + + flow, err := oauthFlowWithFallback(a, nil, flows, "", stubAuthFactory(a)) + require.NoError(t, err) + + _, err = flow.RequestAuthInfo(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "status 404") + }) + + t.Run("keeps the original error when the fallback cannot reach management", func(t *testing.T) { + flows := []oauthFlowInit{stubInitFlow("device", nil, idpRejects), stubInit("pkce", nil)} + + unreachable := func(context.Context) (*Auth, func(), error) { + return nil, nil, errors.New("connect to management: connection refused") + } + + flow, err := oauthFlowWithFallback(a, nil, flows, "", unreachable) + require.NoError(t, err) + + _, err = flow.RequestAuthInfo(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "status 404", "the IdP error must survive a failed fallback") + assert.Equal(t, "device", activeStub(t, flow).name, "a failed fallback must not swap the flow") + }) + + t.Run("does not swap flows on an unrelated failure", func(t *testing.T) { + flows := []oauthFlowInit{ + stubInitFlow("device", nil, errors.New("timeout talking to the IdP")), + stubInit("pkce", nil), + } + + flow, err := oauthFlowWithFallback(a, nil, flows, "", stubAuthFactory(a)) + require.NoError(t, err) + + _, err = flow.RequestAuthInfo(context.Background()) + require.Error(t, err) + assert.Equal(t, "device", activeStub(t, flow).name, "the preferred flow must stay active") + }) +} + +func TestFlowOrder(t *testing.T) { + assert.Equal(t, "pkce authorization flow", flowOrder(false)[0].name) + assert.Equal(t, "device code flow", flowOrder(true)[0].name) + assert.Len(t, flowOrder(false), 2, "both flows must always be attempted") +} + +func TestPreferDeviceFlow(t *testing.T) { + isUnix := runtime.GOOS == "linux" || runtime.GOOS == "freebsd" + + assert.True(t, preferDeviceFlow(true, true), "forced device flow wins over a desktop session") + assert.Equal(t, isUnix, preferDeviceFlow(false, false), "headless unix hosts prefer the device flow") + assert.False(t, preferDeviceFlow(false, true), "desktop clients prefer PKCE") +} diff --git a/client/internal/auth/pkce_flow.go b/client/internal/auth/pkce_flow.go index be64cc6a8..71e39a8a5 100644 --- a/client/internal/auth/pkce_flow.go +++ b/client/internal/auth/pkce_flow.go @@ -62,8 +62,17 @@ type PKCEAuthProviderConfig struct { LoginHint string } -// validatePKCEConfig validates PKCE provider configuration +// validatePKCEConfig validates PKCE provider configuration. A missing value means management +// does not have this flow configured, so the error wraps errFlowNotConfigured and the caller can +// fall back to the other flow. func validatePKCEConfig(config *PKCEAuthProviderConfig) error { + if err := checkPKCEConfig(config); err != nil { + return fmt.Errorf("%w: %w", errFlowNotConfigured, err) + } + return nil +} + +func checkPKCEConfig(config *PKCEAuthProviderConfig) error { errorMsgFormat := "invalid provider configuration received from management: %s value is empty. Contact your NetBird administrator" if config.ClientID == "" { diff --git a/client/server/server.go b/client/server/server.go index f33e19075..173f2200f 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -682,6 +682,11 @@ func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*pro oAuthFlow, err := auth.NewOAuthFlow(ctx, config, msg.IsUnixDesktopClient, false, hint) if err != nil { state.Set(internal.StatusLoginFailed) + // enrolling a device is the one flow a setup key can replace. NotFound so the CLI + // stops its backoff loop and shows this instead of retrying a permanent condition. + if auth.IsSSOUnavailable(err) { + return nil, gstatus.Error(codes.NotFound, auth.WithSetupKeyAdvice(err).Error()) + } return nil, err }