diff --git a/client/internal/auth/auth.go b/client/internal/auth/auth.go index 8cb930d4b..ff4875de2 100644 --- a/client/internal/auth/auth.go +++ b/client/internal/auth/auth.go @@ -157,7 +157,7 @@ func (a *Auth) GetOAuthFlow(ctx context.Context, forceDeviceAuth bool) (OAuthFlo err := a.withRetry(ctx, func(client *mgm.GrpcClient) error { var err error - flow, err = oauthFlowWithFallback(a, client, flowOrder(forceDeviceAuth), "", newAuth) + flow, err = oauthFlowWithFallback(a, client, flowOrder(forceDeviceAuth, true), "", newAuth) if IsSSOUnavailable(err) { return backoff.Permanent(err) diff --git a/client/internal/auth/oauth.go b/client/internal/auth/oauth.go index d573798ec..13add6ada 100644 --- a/client/internal/auth/oauth.go +++ b/client/internal/auth/oauth.go @@ -183,19 +183,28 @@ func (f *fallbackFlow) initNext(ctx context.Context) (OAuthFlow, error) { // 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 +func preferDeviceFlow(hasGraphicalSession bool) bool { + return (runtime.GOOS == "linux" || runtime.GOOS == "freebsd") && !hasGraphicalSession } -// flowOrder returns both flows in the order they should be attempted. -func flowOrder(preferDevice bool) []oauthFlowInit { +// flowOrder returns the flows to attempt, in order. +// +// force leaves the device code flow on its own rather than first: it marks a device with no +// browser at all, such as Android TV or tvOS. PKCE cannot work there even from another device, +// because the redirect has to arrive on the loopback listener of the device being enrolled, so +// offering it as a fallback would only replace a clear error with a login that cannot complete. +func flowOrder(force bool, hasGraphicalSession bool) []oauthFlowInit { pkce := oauthFlowInit{name: "pkce authorization flow", init: initPKCEFlow} device := oauthFlowInit{name: "device code flow", init: initDeviceFlow} - if preferDevice { + switch { + case force: + return []oauthFlowInit{device} + case preferDeviceFlow(hasGraphicalSession): return []oauthFlowInit{device, pkce} + default: + return []oauthFlowInit{pkce, device} } - return []oauthFlowInit{pkce, device} } func initPKCEFlow(a *Auth, client *mgm.GrpcClient, hint string) (OAuthFlow, error) { @@ -228,8 +237,8 @@ func initDeviceFlow(a *Auth, client *mgm.GrpcClient, hint string) (OAuthFlow, er // // 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). +// so a server that only offers one of them still works. forceDeviceCodeFlow restricts the client +// to the device code flow with no fallback, for a device that has no browser at all. 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 { @@ -254,7 +263,7 @@ func NewOAuthFlow(ctx context.Context, config *profilemanager.Config, hasGraphic }, nil } - flows := flowOrder(preferDeviceFlow(forceDeviceCodeFlow, hasGraphicalSession)) + flows := flowOrder(forceDeviceCodeFlow, hasGraphicalSession) return oauthFlowWithFallback(authClient, authClient.grpcClient(), flows, hint, newAuth) } diff --git a/client/internal/auth/oauth_test.go b/client/internal/auth/oauth_test.go index dc9f4b8ab..daafb6064 100644 --- a/client/internal/auth/oauth_test.go +++ b/client/internal/auth/oauth_test.go @@ -226,6 +226,35 @@ func TestFallbackFlowRequestAuthInfo(t *testing.T) { }) } +// TestForcedDeviceFlowHasNoFallback covers Android TV and tvOS: a browserless device must get the +// device code error rather than a PKCE flow it can never complete. +func TestForcedDeviceFlowHasNoFallback(t *testing.T) { + mgmURL, err := url.Parse("https://api.netbird.io:443") + require.NoError(t, err) + a := &Auth{mgmURL: mgmURL} + + notFound := status.Error(codes.NotFound, "no device authorization flow information available") + + t.Run("no wrapper when the device flow works", func(t *testing.T) { + // flowOrder(force) yields this single-entry list, see TestFlowOrder + forced := []oauthFlowInit{stubInit("device", nil)} + + flow, err := oauthFlowWithFallback(a, nil, forced, "", stubAuthFactory(a)) + require.NoError(t, err) + + _, wrapped := flow.(*fallbackFlow) + assert.False(t, wrapped, "nothing may swap the flow later on a browserless device") + }) + + t.Run("reports the device flow error instead of falling back", func(t *testing.T) { + forced := []oauthFlowInit{stubInit("device", notFound)} + + _, err := oauthFlowWithFallback(a, nil, forced, "", stubAuthFactory(a)) + require.Error(t, err) + assert.True(t, IsSSOUnavailable(err), "the caller must see that SSO is unavailable here") + }) +} + // TestFallbackFlowSetLoginHint covers the Android SDK's pattern: it sets the login hint after the // flow is built, through a type assertion that the wrapper must satisfy. func TestFallbackFlowSetLoginHint(t *testing.T) { @@ -265,15 +294,27 @@ func TestWithSetupKeyAdvice(t *testing.T) { } 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") + graphical := flowOrder(false, true) + require.Len(t, graphical, 2, "both flows must be attempted when the device has a browser") + assert.Equal(t, "pkce authorization flow", graphical[0].name) + + headless := flowOrder(false, false) + require.Len(t, headless, 2) + if runtime.GOOS == "linux" || runtime.GOOS == "freebsd" { + assert.Equal(t, "device code flow", headless[0].name, "a headless unix host prefers the device flow") + } + + // Android TV and tvOS have no browser, so PKCE cannot complete there even from another + // device: the redirect must reach the loopback listener of the device being enrolled. + forced := flowOrder(true, false) + require.Len(t, forced, 1, "a forced device code flow must not fall back to PKCE") + assert.Equal(t, "device code flow", forced[0].name) + assert.Len(t, flowOrder(true, true), 1, "force wins over a reported graphical session") } 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") + assert.Equal(t, isUnix, preferDeviceFlow(false), "headless unix hosts prefer the device flow") + assert.False(t, preferDeviceFlow(true), "clients with a graphical session prefer PKCE") }