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.
|
@ransombriggs can we please squash all the commits into a single one for the clear history? 🙏 Thanks! |
ransombriggs
force-pushed
the
allow-refresh-tokens-to-be-revoked-after-expire
branch
from
1503e70 to
eab3e3b
Compare
| end | ||
|
|
||
| def revocable_token | ||
| return @revocable_token if defined? @revocable_token |
There was a problem hiding this comment.
maybe this? instead of 124...126?
| return @revocable_token if defined? @revocable_token | |
| @revocable_token ||= begin ... end |
There was a problem hiding this comment.
I went with the defined? pattern so that nil is memozied properly in the case where the token cannot be found.
|
Thanks a lot @ransombriggs ! 🙇 |
ransombriggs
deleted the
allow-refresh-tokens-to-be-revoked-after-expire
branch
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