From 3df99cae3455d2f3774fdd17b9a929fe0d6eb1bd Mon Sep 17 00:00:00 2001 From: Ransom Briggs Date: Wed, 30 Oct 2024 16:26:39 -0500 Subject: [PATCH 1/4] Allow for expired refresh tokens to be revoked --- .../doorkeeper/tokens_controller.rb | 44 ++++++++- spec/controllers/tokens_controller_spec.rb | 95 ++++++++++++++++++- 2 files changed, 131 insertions(+), 8 deletions(-) diff --git a/app/controllers/doorkeeper/tokens_controller.rb b/app/controllers/doorkeeper/tokens_controller.rb index 824b36123..7f674142f 100644 --- a/app/controllers/doorkeeper/tokens_controller.rb +++ b/app/controllers/doorkeeper/tokens_controller.rb @@ -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? + case token_type + when :access_token + token.revoke if token.accessible? + when :refresh_token + token.revoke unless token.revoked? + end end def token - @token ||= + token_and_type&.[](:token) + end + + def token_type + token_and_type&.[](:type) + end + + def token_and_type + return @token_and_type if defined? @token_and_type + + @token_and_type = if params[:token_type_hint] == "refresh_token" - Doorkeeper.config.access_token_model.by_refresh_token(params["token"]) + refresh_token else - Doorkeeper.config.access_token_model.by_token(params["token"]) || - Doorkeeper.config.access_token_model.by_refresh_token(params["token"]) + access_token || refresh_token end end + def refresh_token + token = Doorkeeper.config.access_token_model.by_refresh_token(params["token"]) + return unless token + + { + token: token, + type: :refresh_token, + } + end + + def access_token + token = Doorkeeper.config.access_token_model.by_token(params["token"]) + return unless token + + { + token: token, + type: :access_token, + } + end + def strategy @strategy ||= server.token_request(params[:grant_type]) end diff --git a/spec/controllers/tokens_controller_spec.rb b/spec/controllers/tokens_controller_spec.rb index 08de1c189..de0088b68 100644 --- a/spec/controllers/tokens_controller_spec.rb +++ b/spec/controllers/tokens_controller_spec.rb @@ -172,7 +172,15 @@ # https://datatracker.ietf.org/doc/html/rfc7009#section-2.2 describe "POST #revoke" do let(:client) { FactoryBot.create(:application) } - let(:access_token) { FactoryBot.create(:access_token, application: client) } + let(:revoked_at) { nil } + let(:access_token) do + FactoryBot.create( + :access_token, + application: client, + revoked_at: revoked_at, + use_refresh_token: true + ) + end context "when associated app is public" do let(:client) { FactoryBot.create(:application, confidential: false) } @@ -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 + post :revoke, params: { + client_id: client.uid, + token: access_token.token, + token_type_hint: "refresh_token", + } + + expect(response.status).to eq 200 + + expect(access_token.reload).to have_attributes(revoked?: false) + end + + it "revokes the refresh token when token_type_hint == refresh_token" do + post :revoke, params: { + client_id: client.uid, + token: access_token.refresh_token, + token_type_hint: "refresh_token", + } + + expect(response.status).to eq 200 + + expect(access_token.reload).to have_attributes(revoked?: true) + end + + it "revokes the refresh token when token_type_hint not passed" do + post :revoke, params: { + client_id: client.uid, + token: access_token.refresh_token, + } + + expect(response.status).to eq 200 + + expect(access_token.reload).to have_attributes(revoked?: true) + end + + context "when access_token has already been revoked" do + let(:revoked_at) { 1.day.ago.floor } + + it "does not update the revoked_at when the access token has already been revoked" do + post :revoke, params: { + client_id: client.uid, + token: access_token.token, + } + + expect(response.status).to eq 200 + + expect(access_token.reload).to have_attributes(revoked_at: revoked_at) + end + + it "does not update the revoked_at when the refresh token has already been revoked" do + post :revoke, params: { + client_id: client.uid, + token: access_token.refresh_token, + } + + expect(response.status).to eq 200 + + expect(access_token.reload).to have_attributes(revoked_at: revoked_at) + end + end + + it "does not revoke when the access token has expired" do + access_token.update!(created_at: access_token.created_at - access_token.expires_in - 1) + + post :revoke, params: { + client_id: client.uid, + token: access_token.token, + } + + expect(response.status).to eq 200 + + expect(access_token.reload).to have_attributes(revoked?: false) + end + + it "revokes the refresh token after the access token has expired" do + access_token.update!(created_at: access_token.created_at - access_token.expires_in - 1) + + post :revoke, params: { + client_id: client.uid, + token: access_token.refresh_token, + } + + expect(response.status).to eq 200 expect(access_token.reload).to have_attributes(revoked?: true) end From 07861c648b093cd9a7c82ad0a467e9022120ce17 Mon Sep 17 00:00:00 2001 From: Ransom Briggs Date: Wed, 30 Oct 2024 16:33:25 -0500 Subject: [PATCH 2/4] Updating CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5595b7693..b6148218f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ User-visible changes worth mentioning. Add your entry here. +- [#1744] Allow for expired refresh tokens to be revoked - [#1739] Add support for dynamic scopes - [#1715] Fix token introspection invalid request reason - [#1714] Fix `Doorkeeper::AccessToken.find_or_create_for` with empty scopes which raises NoMethodError From a009e8801a0f6116810745528e8a50c20b44ee6d Mon Sep 17 00:00:00 2001 From: Ransom Briggs Date: Thu, 31 Oct 2024 13:58:24 -0500 Subject: [PATCH 3/4] Try to fix revoke_token cognitive complexity --- app/controllers/doorkeeper/tokens_controller.rb | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/app/controllers/doorkeeper/tokens_controller.rb b/app/controllers/doorkeeper/tokens_controller.rb index 7f674142f..d21310b90 100644 --- a/app/controllers/doorkeeper/tokens_controller.rb +++ b/app/controllers/doorkeeper/tokens_controller.rb @@ -113,12 +113,14 @@ 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 - case token_type - when :access_token - token.revoke if token.accessible? - when :refresh_token - token.revoke unless token.revoked? - end + can_be_revoked = case token_type + when :access_token + token.accessible? + when :refresh_token + !token.revoked? + end + + token.revoke if can_be_revoked end def token From 5dd6454dc940aa0d50b505e4308e39d37e8ec8ba Mon Sep 17 00:00:00 2001 From: Ransom Briggs Date: Fri, 8 Nov 2024 09:35:18 -0600 Subject: [PATCH 4/4] Move revoke logic out into classes --- .../doorkeeper/tokens_controller.rb | 31 +++++-------------- lib/doorkeeper.rb | 5 +++ .../revocable_access_token.rb | 21 +++++++++++++ .../revocable_refresh_token.rb | 21 +++++++++++++ 4 files changed, 54 insertions(+), 24 deletions(-) create mode 100644 lib/doorkeeper/revocable_tokens/revocable_access_token.rb create mode 100644 lib/doorkeeper/revocable_tokens/revocable_refresh_token.rb diff --git a/app/controllers/doorkeeper/tokens_controller.rb b/app/controllers/doorkeeper/tokens_controller.rb index d21310b90..8bcaafb96 100644 --- a/app/controllers/doorkeeper/tokens_controller.rb +++ b/app/controllers/doorkeeper/tokens_controller.rb @@ -113,28 +113,17 @@ 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 - can_be_revoked = case token_type - when :access_token - token.accessible? - when :refresh_token - !token.revoked? - end - - token.revoke if can_be_revoked + revocable_token.revoke if revocable_token.revocable? end def token - token_and_type&.[](:token) - end - - def token_type - token_and_type&.[](:type) + revocable_token&.token end - def token_and_type - return @token_and_type if defined? @token_and_type + def revocable_token + return @revocable_token if defined? @revocable_token - @token_and_type = + @revocable_token = if params[:token_type_hint] == "refresh_token" refresh_token else @@ -146,20 +135,14 @@ def refresh_token token = Doorkeeper.config.access_token_model.by_refresh_token(params["token"]) return unless token - { - token: token, - type: :refresh_token, - } + RevocableTokens::RevocableRefreshToken.new(token) end def access_token token = Doorkeeper.config.access_token_model.by_token(params["token"]) return unless token - { - token: token, - type: :access_token, - } + RevocableTokens::RevocableAccessToken.new(token) end def strategy diff --git a/lib/doorkeeper.rb b/lib/doorkeeper.rb index 2400bc9c4..97e3f549a 100644 --- a/lib/doorkeeper.rb +++ b/lib/doorkeeper.rb @@ -34,6 +34,11 @@ module Request autoload :Token, "doorkeeper/request/token" end + module RevocableTokens + autoload :RevocableAccessToken, "doorkeeper/revocable_tokens/revocable_access_token" + autoload :RevocableRefreshToken, "doorkeeper/revocable_tokens/revocable_refresh_token" + end + module OAuth autoload :BaseRequest, "doorkeeper/oauth/base_request" autoload :AuthorizationCodeRequest, "doorkeeper/oauth/authorization_code_request" diff --git a/lib/doorkeeper/revocable_tokens/revocable_access_token.rb b/lib/doorkeeper/revocable_tokens/revocable_access_token.rb new file mode 100644 index 000000000..623114cb0 --- /dev/null +++ b/lib/doorkeeper/revocable_tokens/revocable_access_token.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Doorkeeper + module RevocableTokens + class RevocableAccessToken + attr_reader :token + + def initialize(token) + @token = token + end + + def revocable? + token.accessible? + end + + def revoke + token.revoke + end + end + end +end diff --git a/lib/doorkeeper/revocable_tokens/revocable_refresh_token.rb b/lib/doorkeeper/revocable_tokens/revocable_refresh_token.rb new file mode 100644 index 000000000..66678c1de --- /dev/null +++ b/lib/doorkeeper/revocable_tokens/revocable_refresh_token.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Doorkeeper + module RevocableTokens + class RevocableRefreshToken + attr_reader :token + + def initialize(token) + @token = token + end + + def revocable? + !token.revoked? + end + + def revoke + token.revoke + end + end + end +end