Keep a forced device code flow on browserless devices without a PKCE fallback

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)

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 {
+func preferDeviceFlow(hasGraphicalSession bool) bool {
-	return force || (runtime.GOOS == "linux" || runtime.GOOS == "freebsd") && !hasGraphicalSession
+	return (runtime.GOOS == "linux" || runtime.GOOS == "freebsd") && !hasGraphicalSession
 }
 
-// flowOrder returns both flows in the order they should be attempted.
+// flowOrder returns the flows to attempt, in order.
-func flowOrder(preferDevice bool) []oauthFlowInit {
+//
+// 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
+// so a server that only offers one of them still works. forceDeviceCodeFlow restricts the client
-// code flow regardless of platform (e.g. for Android TV).
+// 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)
 }
 

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)
+	graphical := flowOrder(false, true)
-	assert.Equal(t, "device code flow", flowOrder(true)[0].name)
+	require.Len(t, graphical, 2, "both flows must be attempted when the device has a browser")
-	assert.Len(t, flowOrder(false), 2, "both flows must always be attempted")
+	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), "headless unix hosts prefer the device flow")
-	assert.Equal(t, isUnix, preferDeviceFlow(false, false), "headless unix hosts prefer the device flow")
+	assert.False(t, preferDeviceFlow(true), "clients with a graphical session prefer PKCE")
-	assert.False(t, preferDeviceFlow(false, true), "desktop clients prefer PKCE")
 }