fix: make 'NoRefresh' honor unlimited tokens in gitauth #9472
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- improve testing coverage of gitauth
Emyrk
commented
Aug 31, 2023
coderd/gitauth/config.go
Outdated
Comment on lines
66
to
81
| 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 = "" | ||
| } |
There was a problem hiding this comment.
@kylecarbs The existing tests for this logic pass no matter what because refreshToken was always the empty string. I am refactoring all the tests with the new fake oidc IDP.
Just want to get your eyes since all this logic isn't very well solidified.
Emyrk
commented
Aug 31, 2023
coderd/gitauth/config_test.go
Outdated
Comment on lines
32
to
92
| t.Run("NoRefreshExpired", func(t *testing.T) { | ||
| t.Parallel() | ||
| config := &gitauth.Config{ | ||
| NoRefresh: true, | ||
| } | ||
| _, refreshed, err := config.RefreshToken(context.Background(), nil, database.GitAuthLink{ | ||
| OAuthExpiry: time.Time{}, | ||
| fake, config, link := setupOauth2Test(t, testConfig{ | ||
| FakeIDPOpts: []oidctest.FakeIDPOpt{ | ||
| oidctest.WithRefreshHook(func(_ string) error { | ||
| t.Error("refresh on the IDP was called, but NoRefresh was set") | ||
| return xerrors.New("should not be called") | ||
| }), | ||
| // The IDP should not be contacted since the token is expired. An expired | ||
| // token with 'NoRefresh' should early abort. | ||
| oidctest.WithDynamicUserInfo(func(_ string) (jwt.MapClaims, error) { | ||
| t.Error("token was validated, but it was expired and this should never have happened.") | ||
| return nil, xerrors.New("should not be called") | ||
| }), | ||
| }, | ||
| GitConfigOpt: func(cfg *gitauth.Config) { | ||
| cfg.NoRefresh = true | ||
| }, | ||
| }) | ||
|
|
||
| ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil)) | ||
| // Expire the link | ||
| link.OAuthExpiry = expired | ||
|
|
||
| _, refreshed, err := config.RefreshToken(ctx, nil, link) | ||
| require.NoError(t, err) | ||
| require.False(t, refreshed) | ||
| }) | ||
|
|
||
| // NoRefreshNoExpiry tests that an oauth token without an expiry is always valid. | ||
| // The "validate url" should be hit, but the refresh endpoint should not. | ||
| t.Run("NoRefreshNoExpiry", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| validated := false | ||
| fake, config, link := setupOauth2Test(t, testConfig{ | ||
| FakeIDPOpts: []oidctest.FakeIDPOpt{ | ||
| 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, error) { | ||
| validated = true | ||
| return jwt.MapClaims{}, nil | ||
| }), | ||
| }, | ||
| GitConfigOpt: func(cfg *gitauth.Config) { | ||
| cfg.NoRefresh = true | ||
| }, | ||
| }) | ||
|
|
||
| ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil)) | ||
|
|
||
| // Zero time used | ||
| link.OAuthExpiry = time.Time{} | ||
| _, refreshed, err := config.RefreshToken(ctx, nil, link) | ||
| require.NoError(t, err) | ||
| require.True(t, refreshed, "token without expiry is always valid") | ||
| require.True(t, validated, "token should have been validated") | ||
| }) | ||
|
|
There was a problem hiding this comment.
This was one test, I made it two. Testing both an expired and and a IsZero time.
kylecarbs
approved these changes
Sep 5, 2023
coderd/gitauth/config.go
Outdated
Comment on lines
66
to
81
| 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 = "" | ||
| } |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this does
This makes NoRefresh honor GitHub's unlimited lifetime tokens. Github oauth tokens have no expiry time set.
This PR mainly just refactors all the existing tests
Testing
The existing tests were not actually testing the right things. For one, the refresh tokens were always
"", so no refreshing would ever happen. TheNoRefreshconfig value would therefore always be honored no matter what in those cases, even if there was a bug.The new tests return fully valid oauth tokens and communicate with the fake IDP all in memory. It can check the right IDP endpoints are called (validate & refresh).