Allow for expired refresh tokens to be revoked #1744
Conversation
| token_and_type&.[](:type) | ||
| end | ||
|
|
||
| def token_and_type |
There was a problem hiding this comment.
I admit that this implementation is pretty awkward, I went with the simplest / quickest just to show the issue. I think I would rather create an object here that is the pair instead, but not sure if y'all would think that is overkill or would match your patterns, just let me know and I can refactor this however you want it to look.
There was a problem hiding this comment.
Agree, maybe better to have some clear DTO object. Can we refactor it please? 🙏 We have some (well not good actually) examples like OAuth::Client
There was a problem hiding this comment.
I went a slightly different direction and refactored in 5dd6454 into classes that respond to revocable? and revoke. I think it is cleaner than this the original code, but can keep tweaking.
| @@ -113,19 +113,53 @@ def revoke_token | |||
| # The authorization server responds with HTTP status code 200 if the token | |||
| # has been revoked successfully or if the client submitted an invalid | |||
| # token | |||
| token.revoke if token&.accessible? | |||
There was a problem hiding this comment.
We have already checked token.blank? by the time we call this so the &. felt unnecessary, but I can bring it back.
| @@ -183,8 +191,89 @@ | |||
| expect(response.status).to eq 200 | |||
| end | |||
|
|
|||
| it "revokes the access token" do | |||
| post :revoke, params: { client_id: client.uid, token: access_token.token } | |||
| it "does not revoke the access token when token_type_hint == refresh_token" do | |||
There was a problem hiding this comment.
The token method was not fully tested so added some specs before I made my changes.
Summary
This alters the revocation endpoint so that when a refresh token is revoked, we check
revoked?rather thanaccessible?since the extraexpired?check inaccessible?does not apply to refresh tokens since they never expire.#1743