coder · Emyrk · Sep 5, 2023 · Aug 31, 2023 · Aug 31, 2023 · Aug 31, 2023
diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go
@@ -41,7 +41,7 @@ import (
 type FakeIDP struct {
 	issuer   string
 	key      *rsa.PrivateKey
-	provider providerJSON
+	provider ProviderJSON
 	handler  http.Handler
 	cfg      *oauth2.Config
 
@@ -181,16 +181,20 @@ func NewFakeIDP(t testing.TB, opts ...FakeIDPOpt) *FakeIDP {
 	return idp
 }
 
+func (f *FakeIDP) WellknownConfig() ProviderJSON {
+	return f.provider
+}
+
 func (f *FakeIDP) updateIssuerURL(t testing.TB, issuer string) {
 	t.Helper()
 
 	u, err := url.Parse(issuer)
 	require.NoError(t, err, "invalid issuer URL")
 
 	f.issuer = issuer
-	// providerJSON is the JSON representation of the OpenID Connect provider
+	// ProviderJSON is the JSON representation of the OpenID Connect provider
 	// These are all the urls that the IDP will respond to.
-	f.provider = providerJSON{
+	f.provider = ProviderJSON{
 		Issuer:      issuer,
 		AuthURL:     u.ResolveReference(&url.URL{Path: authorizePath}).String(),
 		TokenURL:    u.ResolveReference(&url.URL{Path: tokenPath}).String(),
@@ -220,6 +224,15 @@ func (f *FakeIDP) realServer(t testing.TB) *httptest.Server {
 	return srv
 }
 
+// GenerateAuthenticatedToken skips all oauth2 flows, and just generates a
+// valid token for some given claims.
+func (f *FakeIDP) GenerateAuthenticatedToken(claims jwt.MapClaims) (*oauth2.Token, error) {
+	state := uuid.NewString()
+	f.stateToIDTokenClaims.Store(state, claims)
+	code := f.newCode(state)
+	return f.cfg.Exchange(oidc.ClientContext(context.Background(), f.HTTPClient(nil)), code)
+}
+
 // Login does the full OIDC flow starting at the "LoginButton".
 // The client argument is just to get the URL of the Coder instance.
 //
@@ -333,7 +346,8 @@ func (f *FakeIDP) OIDCCallback(t testing.TB, state string, idTokenClaims jwt.Map
 	return resp, nil
 }
 
-type providerJSON struct {
+// ProviderJSON is the .well-known/configuration JSON
+type ProviderJSON struct {
 	Issuer      string   `json:"issuer"`
 	AuthURL     string   `json:"authorization_endpoint"`
 	TokenURL    string   `json:"token_endpoint"`

diff --git a/coderd/gitauth/config.go b/coderd/gitauth/config.go
@@ -63,13 +63,26 @@ type Config struct {
 func (c *Config) RefreshToken(ctx context.Context, db database.Store, gitAuthLink database.GitAuthLink) (database.GitAuthLink, bool, error) {
 	// If the token is expired and refresh is disabled, we prompt
 	// the user to authenticate again.
-	if c.NoRefresh && gitAuthLink.OAuthExpiry.Before(database.Now()) {
+	if c.NoRefresh &&
+		// If the time is set to 0, then it should never expire.
+		// This is true for github, which has no expiry.
+		!gitAuthLink.OAuthExpiry.IsZero() &&
+		gitAuthLink.OAuthExpiry.Before(database.Now()) {
 		return gitAuthLink, false, nil
 	}
 
+	// This is additional defensive programming. Because TokenSource is an interface,
+	// we cannot be sure that the implementation will treat an 'IsZero' time
+	// as "not-expired". The default implementation does, but a custom implementation
+	// might not. Removing the refreshToken will guarantee a refresh will fail.
+	refreshToken := gitAuthLink.OAuthRefreshToken
+	if c.NoRefresh {
+		refreshToken = ""
+	}
+
 	token, err := c.TokenSource(ctx, &oauth2.Token{
 		AccessToken:  gitAuthLink.OAuthAccessToken,
-		RefreshToken: gitAuthLink.OAuthRefreshToken,
+		RefreshToken: refreshToken,
 		Expiry:       gitAuthLink.OAuthExpiry,
 	}).Token()
 	if err != nil {
@@ -129,8 +142,13 @@ func (c *Config) ValidateToken(ctx context.Context, token string) (bool, *coders
 	if err != nil {
 		return false, nil, err
 	}
+
+	cli := http.DefaultClient
+	if v, ok := ctx.Value(oauth2.HTTPClient).(*http.Client); ok {
+		cli = v
+	}
 	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
-	res, err := http.DefaultClient.Do(req)
+	res, err := cli.Do(req)
 	if err != nil {
 		return false, nil, err
 	}

diff --git a/coderd/gitauth/config_test.go b/coderd/gitauth/config_test.go
@@ -8,6 +8,14 @@ import (
 	"testing"
 	"time"
 
+	"github.com/golang-jwt/jwt/v4"
+
+	"github.com/google/uuid"
+
+	"github.com/coreos/go-oidc/v3/oidc"
+
+	"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
+
 	"github.com/stretchr/testify/require"
 	"golang.org/x/oauth2"
 	"golang.org/x/xerrors"
@@ -22,17 +30,82 @@ import (
 
 func TestRefreshToken(t *testing.T) {
 	t.Parallel()
-	t.Run("FalseIfNoRefresh", func(t *testing.T) {
+	const providerID = "test-idp"
+	expired := time.Now().Add(time.Hour * -1)
+	t.Run("NoRefreshExpired", func(t *testing.T) {
 		t.Parallel()
+
+		fake := oidctest.NewFakeIDP(t,
+			// The IDP should not be contacted since the token is expired. An expired
+			// token with 'NoRefresh' should early abort.
+			oidctest.WithRefreshHook(func(_ string) error {
+				t.Error("refresh on the IDP was called, but NoRefresh was set")
+				return xerrors.New("should not be called")
+			}),
+			oidctest.WithDynamicUserInfo(func(_ string) jwt.MapClaims {
+				t.Error("token was validated, but it was expired and this should never have happened.")
+				return nil
+			}),
+		)
+
+		ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
 		config := &gitauth.Config{
-			NoRefresh: true,
+			ID:           providerID,
+			OAuth2Config: fake.OIDCConfig(t, nil),
+			NoRefresh:    true,
+			ValidateURL:  fake.WellknownConfig().UserInfoURL,
 		}
-		_, refreshed, err := config.RefreshToken(context.Background(), nil, database.GitAuthLink{
-			OAuthExpiry: time.Time{},
+		_, refreshed, err := config.RefreshToken(ctx, nil, database.GitAuthLink{
+			ProviderID:        providerID,
+			UserID:            uuid.New(),
+			OAuthAccessToken:  uuid.NewString(),
+			OAuthRefreshToken: uuid.NewString(),
+			OAuthExpiry:       expired,
 		})
 		require.NoError(t, err)
 		require.False(t, refreshed)
 	})
+	t.Run("NoRefreshNoExpiry", func(t *testing.T) {
+		t.Parallel()
+
+		validated := false
+		fake := oidctest.NewFakeIDP(t,
+			// The IDP should not be contacted since the token is expired. An expired
+			// token with 'NoRefresh' should early abort.
+			oidctest.WithRefreshHook(func(_ string) error {
+				t.Error("refresh on the IDP was called, but NoRefresh was set")
+				return xerrors.New("should not be called")
+			}),
+			oidctest.WithDynamicUserInfo(func(_ string) jwt.MapClaims {
+				validated = true
+				return jwt.MapClaims{}
+			}),
+		)
+
+		ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
+		config := &gitauth.Config{
+			ID:           providerID,
+			OAuth2Config: fake.OIDCConfig(t, nil),
+			NoRefresh:    true,
+			ValidateURL:  fake.WellknownConfig().UserInfoURL,
+		}
+
+		token, err := fake.GenerateAuthenticatedToken(jwt.MapClaims{})
+		require.NoError(t, err)
+
+		_, refreshed, err := config.RefreshToken(ctx, nil, database.GitAuthLink{
+			ProviderID:       providerID,
+			UserID:           uuid.New(),
+			OAuthAccessToken: token.AccessToken,
+			// Pass a refresh token, but this should be ignored in this test!
+			OAuthRefreshToken: token.RefreshToken,
+			// Zero time used
+			OAuthExpiry: time.Time{},
+		})
+		require.NoError(t, err)
+		require.True(t, refreshed, "token without expiry is always valid")
+		require.True(t, validated, "token should have been validated")
+	})
 	t.Run("FalseIfTokenSourceFails", func(t *testing.T) {
 		t.Parallel()
 		config := &gitauth.Config{
@@ -42,7 +115,9 @@ func TestRefreshToken(t *testing.T) {
 				},
 			},
 		}
-		_, refreshed, err := config.RefreshToken(context.Background(), nil, database.GitAuthLink{})
+		_, refreshed, err := config.RefreshToken(context.Background(), nil, database.GitAuthLink{
+			OAuthExpiry: expired,
+		})
 		require.NoError(t, err)
 		require.False(t, refreshed)
 	})
@@ -56,7 +131,9 @@ func TestRefreshToken(t *testing.T) {
 			OAuth2Config: &testutil.OAuth2Config{},
 			ValidateURL:  srv.URL,
 		}
-		_, _, err := config.RefreshToken(context.Background(), nil, database.GitAuthLink{})
+		_, _, err := config.RefreshToken(context.Background(), nil, database.GitAuthLink{
+			OAuthExpiry: expired,
+		})
 		require.ErrorContains(t, err, "Failure")
 	})
 	t.Run("ValidateFailure", func(t *testing.T) {
@@ -69,7 +146,9 @@ func TestRefreshToken(t *testing.T) {
 			OAuth2Config: &testutil.OAuth2Config{},
 			ValidateURL:  srv.URL,
 		}
-		_, refreshed, err := config.RefreshToken(context.Background(), nil, database.GitAuthLink{})
+		_, refreshed, err := config.RefreshToken(context.Background(), nil, database.GitAuthLink{
+			OAuthExpiry: expired,
+		})
 		require.NoError(t, err)
 		require.False(t, refreshed)
 	})
@@ -100,6 +179,7 @@ func TestRefreshToken(t *testing.T) {
 		link := dbgen.GitAuthLink(t, db, database.GitAuthLink{
 			ProviderID:       config.ID,
 			OAuthAccessToken: "initial",
+			OAuthExpiry:      expired,
 		})
 		_, refreshed, err := config.RefreshToken(context.Background(), db, link)
 		require.NoError(t, err)
@@ -124,6 +204,7 @@ func TestRefreshToken(t *testing.T) {
 		}
 		_, valid, err := config.RefreshToken(context.Background(), nil, database.GitAuthLink{
 			OAuthAccessToken: accessToken,
+			OAuthExpiry:      expired,
 		})
 		require.NoError(t, err)
 		require.True(t, valid)
@@ -143,6 +224,7 @@ func TestRefreshToken(t *testing.T) {
 		link := dbgen.GitAuthLink(t, db, database.GitAuthLink{
 			ProviderID:       config.ID,
 			OAuthAccessToken: "initial",
+			OAuthExpiry:      expired,
 		})
 		_, valid, err := config.RefreshToken(context.Background(), db, link)
 		require.NoError(t, err)