- Notifications
You must be signed in to change notification settings - Fork928
fix: make 'NoRefresh' honor unlimited tokens in gitauth#9472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
- improve testing coverage of gitauth
coderd/gitauth/config.go Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@kylecarbs The existing tests for this logic pass no matter what becauserefreshToken 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Neat
coderd/gitauth/config_test.go Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This was one test, I made it two. Testing both an expired and and aIsZero time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM! Only a single note on naming, but this was from theoidctest PR:
WithRefreshHook could just beWithRefresh, because other funcs likeWithDynamicUserInfo drop theHook suffix, but that's a minor.
coderd/gitauth/config.go Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Neat
Uh oh!
There was an error while loading.Please reload this page.
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).