From 2487cfcf24209220b08327d3655fdff7596a3bea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Thu, 27 Aug 2026 10:32:57 +0200 Subject: [PATCH] [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/internal/auth/auth.go | 4 ++-- client/internal/auth/oauth.go | 8 ++++---- shared/management/client/client.go | 4 ++++ shared/management/client/client_test.go | 4 +--- shared/management/client/grpc.go | 2 +- shared/management/client/mock.go | 1 + 6 files changed, 13 insertions(+), 10 deletions(-) diff --git a/client/internal/auth/auth.go b/client/internal/auth/auth.go index cc389b3f2..940c43e8b 100644 --- a/client/internal/auth/auth.go +++ b/client/internal/auth/auth.go @@ -136,8 +136,8 @@ func (a *Auth) IsSSOSupported(ctx context.Context) (bool, error) { return supportsSSO, err } -// GetOAuthFlow returns an OAuth flow (PKCE or Device) using the existing management connection -// This avoids creating a new connection to the management server +// GetOAuthFlow returns an OAuth flow (PKCE or Device) using the existing management connection. +// This avoids creating a new connection to the management server. // // sessionExtend marks the flow as renewing an existing peer's session rather than // logging one in; the server needs it to rule out a silent authorization that the diff --git a/client/internal/auth/oauth.go b/client/internal/auth/oauth.go index 7b0aff0db..1a4f1e94d 100644 --- a/client/internal/auth/oauth.go +++ b/client/internal/auth/oauth.go @@ -88,16 +88,16 @@ func shouldUseDeviceFlow(force bool, isUnixDesktopClient bool) bool { return force || (runtime.GOOS == "linux" || runtime.GOOS == "freebsd") && !isUnixDesktopClient } -// NewOAuthFlow initializes and returns the appropriate OAuth flow based on the management configuration +// 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) +// 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). // // sessionExtend marks the flow as renewing an existing peer's session rather than -// logging one in; see PKCEAuthorizationFlowRequest for what the server makes of it. +// logging one in. See PKCEAuthorizationFlowRequest for what the server makes of it. func NewOAuthFlow(ctx context.Context, config *profilemanager.Config, isUnixDesktopClient bool, forceDeviceCodeFlow bool, hint string, sessionExtend bool) (OAuthFlow, error) { if shouldUseDeviceFlow(forceDeviceCodeFlow, isUnixDesktopClient) { return authenticateWithDeviceCodeFlow(ctx, config, hint) diff --git a/shared/management/client/client.go b/shared/management/client/client.go index 8c88f800c..03f02763c 100644 --- a/shared/management/client/client.go +++ b/shared/management/client/client.go @@ -21,6 +21,10 @@ type Client interface { // is not eligible for session extension. ExtendAuthSession(sysInfo *system.Info, jwtToken string) (*proto.ExtendAuthSessionResponse, error) GetDeviceAuthorizationFlow() (*proto.DeviceAuthorizationFlow, error) + // GetPKCEAuthorizationFlow returns the PKCE authorization flow information. + // + // sessionExtend marks the flow as renewing an existing peer's session rather + // than logging one in. See PKCEAuthorizationFlowRequest. GetPKCEAuthorizationFlow(sessionExtend bool) (*proto.PKCEAuthorizationFlow, error) GetServerURL() string // IsHealthy returns the current connection status without blocking. diff --git a/shared/management/client/client_test.go b/shared/management/client/client_test.go index 75a4978b3..5998fa60f 100644 --- a/shared/management/client/client_test.go +++ b/shared/management/client/client_test.go @@ -614,9 +614,7 @@ func Test_GetPKCEAuthorizationFlow(t *testing.T) { } flowInfo, err := client.GetPKCEAuthorizationFlow(true) - if err != nil { - t.Error("error while retrieving pkce auth flow information") - } + require.NoError(t, err) assert.True(t, gotRequest.GetSessionExtend(), "session extend should reach the server") diff --git a/shared/management/client/grpc.go b/shared/management/client/grpc.go index 8422f7354..a4e02e9cc 100644 --- a/shared/management/client/grpc.go +++ b/shared/management/client/grpc.go @@ -738,7 +738,7 @@ func (c *GrpcClient) GetDeviceAuthorizationFlow() (*proto.DeviceAuthorizationFlo return flowInfoResp, nil } -// GetPKCEAuthorizationFlow returns a pkce authorization flow information. +// GetPKCEAuthorizationFlow returns the PKCE authorization flow information. // It also takes care of encrypting and decrypting messages. // // sessionExtend tells the server the flow will renew an existing peer's session diff --git a/shared/management/client/mock.go b/shared/management/client/mock.go index 1516451b2..d5c29bee0 100644 --- a/shared/management/client/mock.go +++ b/shared/management/client/mock.go @@ -80,6 +80,7 @@ func (m *MockClient) GetDeviceAuthorizationFlow() (*proto.DeviceAuthorizationFlo return m.GetDeviceAuthorizationFlowFunc() } +// GetPKCEAuthorizationFlow returns the PKCE authorization flow information. func (m *MockClient) GetPKCEAuthorizationFlow(sessionExtend bool) (*proto.PKCEAuthorizationFlow, error) { if m.GetPKCEAuthorizationFlowFunc == nil { return nil, nil