-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
api: adding a gRPC call for revoking refresh tokens. #802
Conversation
c786e4b
to
cf6232d
Compare
server/api.go
Outdated
return nil, err | ||
} | ||
|
||
offlineSessions, err := d.s.GetOfflineSessions(id.UserId, id.ConnId) |
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.
Why isn't this done in an Update
?
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.
Changed this to an update operation
server/api.go
Outdated
|
||
// If the user has no more refresh tokens, the OfflineSession object should be cleaned up. | ||
if len(offlineSessions.Refresh) == 0 { | ||
if err := d.s.DeleteOfflineSessions(id.UserId, id.ConnId); err != nil { |
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.
Can we just keep this object around? Why delete it?
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.
Discussed offline and decided to keep the Offline Session object around even if the user does not have active refresh tokens. Might be helpful later on for auditing purposes.
thanks for the test. couple nits |
cf6232d
to
0bc79f1
Compare
// RevokeRefreshResp determines if the refresh token is revoked successfully. | ||
message RevokeRefreshResp { | ||
// Set to true is refresh token was not found and token could not be revoked. | ||
bool not_found = 1; |
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 is never set?
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.
Set this to true when UpdateOfflineSession returns err == storage.ErrNotFound.
0bc79f1
to
1ec19d4
Compare
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
Added a gRPC call that will enable users to revoke client specific refresh tokens.
Also added a test in api_test.go. Since we do not have APIs in place for creating refresh tokens and offline session objects, this has been done using the storage methods. If this test case seems like an overkill, I could get rid of it.