Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new expiry time parameter to the RotateRefreshToken method to enhance refresh token rotation functionality. The change modifies the storage interface signature to pass expiry information during token rotation.
- Added
time.Timeparameter for next token expiry toRotateRefreshTokenmethod signature - Updated interface implementations to accept the new expiry parameter
- Switched from using
go.uber.org/mock/gomocktogithub.com/golang/mock/gomockin test files
Reviewed Changes
Copilot reviewed 73 out of 74 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| storage/memory.go | Updated RotateRefreshToken method signature to accept expiry parameter |
| handler/oauth2/storage.go | Modified interface to include expiry parameter |
| handler/oauth2/flow_refresh.go | Enhanced rotation logic to calculate and pass expiry time |
| Multiple test files | Updated gomock import path from uber.org to golang |
| Multiple internal mock files | Updated gomock imports and parameter signatures |
Comments suppressed due to low confidence (2)
storage/memory.go:514
- The new
expiresAtparameter is added to the method signature but is not used in the implementation. This parameter should either be utilized for token expiry management or test coverage should verify its proper handling.
func (s *MemoryStore) RotateRefreshToken(ctx context.Context, requestID string, refreshTokenSignature string, expiresAt time.Time) (err error) {
handler/oauth2/flow_refresh.go:143
- [nitpick] The variable name
farthestExpiryis ambiguous. Consider renaming tonextTokenExpiryorrotatedTokenExpiryto better reflect its purpose as the expiry time for the next refresh token.
farthestExpiry := requester.GetSession().GetExpiresAt(fosite.RefreshToken)
BREAKING CHANGE:
```patch
type RefreshTokenStorage interface {
- RotateRefreshToken(ctx context.Context, requestID string, refreshTokenSignature string) (err error)
+ RotateRefreshToken(ctx context.Context, requestID string, refreshTokenSignature string, nextExpiry time.Time) (err error)
}
```
|
Why exactly do the gomock change? |
|
Ignore me, it's the opposite of what I thought. Think this is because of an old local mockgen? |
Canonical introduced uber's go mock, and broke the tooling in the process (generating new mocks), so I reverted the change and since this PR needs updated mocks, I did it in this PR |
|
Ah that makes sense (didn't see the second commit). Maybe this was missed? Line 24 in b252c87 |
3 participants
No description provided.