Conversation
aeneasr
force-pushed
the
improve-refresh
branch
from
838a43d to
4cc28f4
Compare
Previously, the refresh token handler was using a combination of delete/update storage primitives. This made optimizing and implementing the refresh token handling difficult. Going forward, the RefreshTokenStorage must implement `RotateRefreshToken`. Token creation continues to be separated.
BREAKING CHANGES:
Method `RevokeRefreshTokenMaybeGracePeriod` was removed from `handler/fosite/TokenRevocationStorage`.
Interface `handler/fosite/RefreshTokenStorage` has changed:
- `CreateRefreshToken` now takes an additional argument `accessSignature` to keep track of refresh/access token pairs:
- A new method `RotateRefreshToken` was added, which revokes old refresh tokens and associated access tokens:
```patch
// handler/fosite/storage.go
type RefreshTokenStorage interface {
- CreateRefreshTokenSession(ctx context.Context, signature string, request fosite.Requester) (err error)
+ CreateRefreshTokenSession(ctx context.Context, signature string, accessSignature string, request fosite.Requester) (err error)
GetRefreshTokenSession(ctx context.Context, signature string, session fosite.Session) (request fosite.Requester, err error)
DeleteRefreshTokenSession(ctx context.Context, signature string) (err error)
+ RotateRefreshToken(ctx context.Context, requestID string, refreshTokenSignature string) (err error)
}
```
aeneasr
force-pushed
the
improve-refresh
branch
from
9255ff3 to
9a18e70
Compare
aeneasr
force-pushed
the
improve-refresh
branch
from
9a18e70 to
117effb
Compare
aeneasr
force-pushed
the
improve-refresh
branch
from
117effb to
b57570a
Compare
aeneasr
force-pushed
the
improve-refresh
branch
from
6c4e8fd to
57cf545
Compare
|
|
||
| if err := c.TokenRevocationStorage.RevokeRefreshTokenMaybeGracePeriod(ctx, ts.GetID(), signature); err != nil { | ||
| return err | ||
| if err = c.TokenRevocationStorage.RotateRefreshToken(ctx, requester.GetID(), signature); err != nil { |
There was a problem hiding this comment.
@aeneasr I think this introduced a bug. After we upgraded our systems to the version with this change, no refresh tokens in our tests (where we use memory storage) got invalidated. The reason I think is that requester here has a completely new and random ID, which of course has nothing with the existing refresh token.
Previous code had it right, it retrieved refresh token requester with GetRefreshTokenSession and used ts.GetID() here. I think this should be fixed and correct requester ID should be passed here.
There was a problem hiding this comment.
My current workaround is to fix this at store level:
func (s *MemoryStore) RotateRefreshToken(ctx context.Context, _ string, refreshTokenSignature string) error {
ts, err := s.GetRefreshTokenSession(ctx, refreshTokenSignature, nil)
if err != nil {
return err
}
if err := s.RevokeRefreshToken(ctx, ts.GetID()); err != nil {
return err
}
return s.RevokeAccessToken(ctx, ts.GetID())
}
But this is just a workaround, I think, it is completely useless to pass new requester ID into this function. Or is this something you use and need in Hydra?
There was a problem hiding this comment.
The request ID is supposed to remain stable for the lifetime duration of a token chain, so this is correct! You can check the implementation in Hydra for reference
There was a problem hiding this comment.
You are right. It works. I had in our code a part where I was unconditionally setting request ID to a new value. The reason is that I want a bit shorter ID representation than what default uuid.New().String() provides. I will have to find another way to do that. I could not find an easy way to patch GetID() to use a different ID constructor. :-(
Thanks.
There was a problem hiding this comment.
Fosite’s not the easiest library to work with
4 participants
Related Issue or Design Document
Checklist
If this pull request addresses a security vulnerability,
I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
Further comments