fix(coderd/externalauth): preserve scopes on entra v1 token refresh (#24851)

Without this, Entra silently narrows scopes to the default set.
2026-09-24 15:04:27 +08:00 · 2026-07-09 11:49:19 -08:00
parent f84801eecf
commit 37558fcdc9
2 changed files with 205 additions and 11 deletions
@@ -216,9 +216,10 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
 		Expiry:       externalAuthLink.OAuthExpiry,
 	}

-	// Note: The TokenSource(...) method will make no remote HTTP requests if the
-	// token is expired and no refresh token is set. This is important to prevent
-	// spamming the API, consuming rate limits, when the token is known to fail.
+	// NOTE: TokenSource(...).Token() will short-circuit if the token:
+	// - is not expired (returns original token)
+	// - is expired and has no refresh token (returns error)
+	// This means we will avoid making useless HTTP requests.
 	//
 	// External providers (GitHub in particular) intermittently fail token
 	// refreshes with transient errors such as 5xx responses, network timeouts,
@@ -229,9 +230,9 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
 	// will never succeed and retrying wastes the refresh quota.
 	token, err := c.refreshTokenWithRetry(ctx, existingToken)
 	if err != nil {
-		// TokenSource can fail for numerous reasons. If it fails because of
-		// a bad refresh token, then the refresh token is invalid, and we should
-		// get rid of it. Keeping it around will cause additional refresh
+		// A refresh attempt can fail for numerous reasons. If it fails because
+		// of a bad refresh token, then the refresh token is invalid, and we
+		// should get rid of it. Keeping it around will cause additional refresh
 		// attempts that will fail and cost us api rate limits.
 		//
 		// The error message is saved for debugging purposes.
@@ -305,8 +306,8 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
 			return externalAuthLink, InvalidTokenError("token expired, refreshing is either disabled or refreshing failed and will not be retried")
 		}

-		// TokenSource(...).Token() will always return the current token if the token is not expired.
-		// So this error is only returned if a refresh of the token failed.
+		// Non-expired tokens are short-circuited as noted above; reaching here
+		// means refresh failed.
 		return externalAuthLink, InvalidTokenError(fmt.Sprintf("refresh token: %s", err.Error()))
 	}

@@ -1343,8 +1344,16 @@ func (c *jwtConfig) Exchange(ctx context.Context, code string, opts ...oauth2.Au
 	)
 }

-// When authenticating via Entra ID ADO only supports v1 tokens that requires the 'resource' rather than scopes
-// When ADO gets support for V2 Entra ID tokens this struct and functions can be removed
+// The Entra wrapper accounts for two things:
+//
+//  1. When authenticating via Entra ID ADO only supports v1 tokens which
+//     require 'resource'.
+//
+//  2. When refreshing, Entra ID requires the original scopes or it will switch
+//     to using the default scopes.
+//
+//     This struct and its functions might be removable once ADO gets support for
+//     Entra ID V2.
 type entraV1Oauth struct {
 	*oauth2.Config
 }
@@ -1363,6 +1372,47 @@ func (c *entraV1Oauth) Exchange(ctx context.Context, code string, opts ...oauth2
 	)
 }

+func (c *entraV1Oauth) TokenSource(ctx context.Context, token *oauth2.Token) oauth2.TokenSource {
+	return oauth2.ReuseTokenSource(token, &entraV1TokenSource{
+		ctx:   ctx,
+		cfg:   c,
+		token: token,
+	})
+}
+
+type entraV1TokenSource struct {
+	ctx   context.Context
+	cfg   *entraV1Oauth
+	token *oauth2.Token
+}
+
+func (s *entraV1TokenSource) Token() (*oauth2.Token, error) {
+	var refreshToken string
+	if s.token != nil {
+		refreshToken = s.token.RefreshToken
+	}
+	if refreshToken == "" {
+		return s.cfg.Config.TokenSource(s.ctx, s.token).Token()
+	}
+
+	refreshOpts := []oauth2.AuthCodeOption{
+		oauth2.SetAuthURLParam("grant_type", "refresh_token"),
+		oauth2.SetAuthURLParam("refresh_token", refreshToken),
+	}
+	if len(s.cfg.Config.Scopes) > 0 {
+		refreshOpts = append(refreshOpts, oauth2.SetAuthURLParam("scope", strings.Join(s.cfg.Config.Scopes, " ")))
+	}
+
+	token, err := s.cfg.Exchange(s.ctx, "", refreshOpts...)
+	if err != nil {
+		return nil, err
+	}
+	if token.RefreshToken == "" {
+		token.RefreshToken = refreshToken
+	}
+	return token, nil
+}
+
 // exchangeWithClientSecret wraps an OAuth config and adds the client secret
 // to the Exchange request as a Bearer header. This is used by JFrog Artifactory.
 type exchangeWithClientSecret struct {
@@ -1427,7 +1477,7 @@ func isRateLimited(resp *http.Response) bool {
 	return false
 }

-// isFailedRefresh returns true if the error returned by the TokenSource.Token()
+// isFailedRefresh returns true if the error returned by the refresh attempt
 // is due to a failed refresh. The failure being the refresh token itself.
 // If this returns true, no amount of retries will fix the issue.
 //
@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -850,6 +851,149 @@ func TestRefreshToken(t *testing.T) {
 	})
 }

+// TestRefreshTokenWithScopes verifies the refresh path echoes Config.Scopes on
+// the token-endpoint request and preserves the prior refresh_token when the
+// authorization server omits a new one (RFC 6749 §6).
+func TestRefreshTokenWithScopes(t *testing.T) {
+	t.Parallel()
+
+	// fakeAS returns an http.Client + a pointer the test can read after
+	// RefreshToken returns. The roundTripper captures the form body of every
+	// outbound request and replies with tokenJSON to refresh requests.
+	fakeAS := func(t *testing.T, tokenJSON []byte) (*http.Client, *url.Values) {
+		t.Helper()
+		captured := &url.Values{}
+		client := &http.Client{Transport: roundTripper(func(req *http.Request) (*http.Response, error) {
+			body, err := io.ReadAll(req.Body)
+			require.NoError(t, err)
+			values, err := url.ParseQuery(string(body))
+			require.NoError(t, err)
+			if values.Get("grant_type") == "refresh_token" {
+				*captured = values
+			}
+			rec := httptest.NewRecorder()
+			rec.Header().Set("Content-Type", "application/json")
+			rec.WriteHeader(http.StatusOK)
+			_, err = rec.Write(tokenJSON)
+			return rec.Result(), err
+		})}
+		return client, captured
+	}
+
+	newConfig := func(t *testing.T, scopes []string) *externalauth.Config {
+		t.Helper()
+		instrument := promoauth.NewFactory(prometheus.NewRegistry())
+		configs, err := externalauth.ConvertConfig(instrument, []codersdk.ExternalAuthConfig{{
+			ID:           "test",
+			Type:         codersdk.EnhancedExternalAuthProviderAzureDevopsEntra.String(),
+			ClientID:     "id",
+			ClientSecret: "secret",
+			AuthURL:      "https://login.microsoftonline.com/tenant/oauth2/authorize",
+			TokenURL:     "https://login.microsoftonline.com/tenant/oauth2/token",
+			Scopes:       scopes,
+		}}, &url.URL{Scheme: "https", Host: "coder.example.com"})
+		require.NoError(t, err)
+		return configs[0]
+	}
+
+	expired := dbtime.Now().Add(-time.Hour)
+
+	// mockDBPassthrough returns a mock store that echoes the
+	// UpdateExternalAuthLink params back as a populated ExternalAuthLink,
+	// letting the test read what RefreshToken decided to persist.
+	mockDBPassthrough := func(t *testing.T) database.Store {
+		t.Helper()
+		ctrl := gomock.NewController(t)
+		mDB := dbmock.NewMockStore(ctrl)
+		mDB.EXPECT().UpdateExternalAuthLink(gomock.Any(), gomock.Any()).
+			DoAndReturn(func(_ context.Context, p database.UpdateExternalAuthLinkParams) (database.ExternalAuthLink, error) {
+				return database.ExternalAuthLink{
+					ProviderID:        p.ProviderID,
+					UserID:            p.UserID,
+					OAuthAccessToken:  p.OAuthAccessToken,
+					OAuthRefreshToken: p.OAuthRefreshToken,
+					OAuthExpiry:       p.OAuthExpiry,
+				}, nil
+			}).AnyTimes()
+		return mDB
+	}
+
+	t.Run("EchoesConfiguredScopesOnRefresh", func(t *testing.T) {
+		t.Parallel()
+		client, captured := fakeAS(t,
+			[]byte(`{"access_token":"new","refresh_token":"new-r","token_type":"bearer","expires_in":3600}`))
+		cfg := newConfig(t, []string{"openid", "offline_access", "api://app/session:role-any"})
+
+		ctx := context.WithValue(context.Background(), oauth2.HTTPClient, client)
+		_, err := cfg.RefreshToken(ctx, mockDBPassthrough(t), database.ExternalAuthLink{
+			OAuthAccessToken:  "old",
+			OAuthRefreshToken: "old-r",
+			OAuthExpiry:       expired,
+		})
+		require.NoError(t, err)
+
+		require.Equal(t, "refresh_token", captured.Get("grant_type"))
+		require.Equal(t, "old-r", captured.Get("refresh_token"))
+		require.Equal(t, "openid offline_access api://app/session:role-any", captured.Get("scope"),
+			"refresh request must echo configured scopes joined by space")
+	})
+
+	t.Run("OmitsScopeParamWhenScopesEmpty", func(t *testing.T) {
+		t.Parallel()
+		client, captured := fakeAS(t,
+			[]byte(`{"access_token":"new","refresh_token":"new-r","token_type":"bearer","expires_in":3600}`))
+		cfg := newConfig(t, nil)
+
+		ctx := context.WithValue(context.Background(), oauth2.HTTPClient, client)
+		_, err := cfg.RefreshToken(ctx, mockDBPassthrough(t), database.ExternalAuthLink{
+			OAuthAccessToken:  "old",
+			OAuthRefreshToken: "old-r",
+			OAuthExpiry:       expired,
+		})
+		require.NoError(t, err)
+
+		require.Equal(t, "refresh_token", captured.Get("grant_type"))
+		require.Equal(t, "old-r", captured.Get("refresh_token"))
+		require.Empty(t, captured.Get("scope"),
+			"refresh request must not send a scope param when Config.Scopes is empty")
+	})
+
+	t.Run("PreservesPriorRefreshTokenWhenASOmitsNewOne", func(t *testing.T) {
+		t.Parallel()
+		// Token response intentionally omits refresh_token.
+		client, _ := fakeAS(t,
+			[]byte(`{"access_token":"new","token_type":"bearer","expires_in":3600}`))
+		cfg := newConfig(t, nil)
+
+		ctx := context.WithValue(context.Background(), oauth2.HTTPClient, client)
+		link, err := cfg.RefreshToken(ctx, mockDBPassthrough(t), database.ExternalAuthLink{
+			OAuthAccessToken:  "old",
+			OAuthRefreshToken: "prior-r",
+			OAuthExpiry:       expired,
+		})
+		require.NoError(t, err)
+		require.Equal(t, "prior-r", link.OAuthRefreshToken,
+			"prior refresh_token must be preserved when AS omits a new one (RFC 6749 §6)")
+	})
+
+	t.Run("AcceptsRotatedRefreshTokenWhenASReturnsOne", func(t *testing.T) {
+		t.Parallel()
+		client, _ := fakeAS(t,
+			[]byte(`{"access_token":"new","refresh_token":"rotated-r","token_type":"bearer","expires_in":3600}`))
+		cfg := newConfig(t, nil)
+
+		ctx := context.WithValue(context.Background(), oauth2.HTTPClient, client)
+		link, err := cfg.RefreshToken(ctx, mockDBPassthrough(t), database.ExternalAuthLink{
+			OAuthAccessToken:  "old",
+			OAuthRefreshToken: "prior-r",
+			OAuthExpiry:       expired,
+		})
+		require.NoError(t, err)
+		require.Equal(t, "rotated-r", link.OAuthRefreshToken,
+			"rotated refresh_token from AS must be persisted")
+	})
+}
+
 func TestValidateToken(t *testing.T) {
 	t.Parallel()