From 58f68b4f8a053b3bff581b2f8bb10a5517e8c93b Mon Sep 17 00:00:00 2001 From: JeremyC-za Date: Wed, 3 May 2023 14:46:25 +0200 Subject: [PATCH] Custom access token attributes are now considered when matching tokens (fixes #1665), and introduces `revoke_previous_client_credentials_token` config option. --- CHANGELOG.md | 2 + .../doorkeeper/authorizations_controller.rb | 10 ++- lib/doorkeeper/config.rb | 11 ++++ lib/doorkeeper/models/access_token_mixin.rb | 66 +++++++++++++++++-- .../oauth/authorization_code_request.rb | 8 +++ .../oauth/client_credentials/creator.rb | 9 ++- lib/doorkeeper/oauth/pre_authorization.rb | 2 +- .../doorkeeper/templates/initializer.rb | 6 ++ .../oauth/authorization_code_request_spec.rb | 36 ++++++++++ spec/models/doorkeeper/access_token_spec.rb | 30 +++++++++ 10 files changed, 169 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 288c50359..d07c108e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ User-visible changes worth mentioning. - [#1696] Add missing `#issued_token` method to `OAuth::TokenResponse` - [#1697] Allow a TokenResponse body to be customized. - [#1702] Fix bugs for error response in the form_post and error view +- [#1660] Custom access token attributes are now considered when finding matching tokens (fixes #1665). + Introduce `revoke_previous_client_credentials_token` configuration option. ## 5.6.9 diff --git a/app/controllers/doorkeeper/authorizations_controller.rb b/app/controllers/doorkeeper/authorizations_controller.rb index 9403db433..9452167f7 100644 --- a/app/controllers/doorkeeper/authorizations_controller.rb +++ b/app/controllers/doorkeeper/authorizations_controller.rb @@ -31,7 +31,7 @@ def destroy private def render_success - if skip_authorization? || (matching_token? && pre_auth.client.application.confidential?) + if skip_authorization? || can_authorize_response? redirect_or_render(authorize_response) elsif Doorkeeper.configuration.api_only render json: pre_auth @@ -52,10 +52,16 @@ def render_error end end + def can_authorize_response? + Doorkeeper.config.custom_access_token_attributes.empty? && pre_auth.client.application.confidential? && matching_token? + end + # Active access token issued for the same client and resource owner with # the same set of the scopes exists? def matching_token? - Doorkeeper.config.access_token_model.matching_token_for( + # We don't match tokens on the custom attributes here - we're in the pre-auth here, + # so they haven't been supplied yet (there are no custom attributes to match on yet) + @matching_token ||= Doorkeeper.config.access_token_model.matching_token_for( pre_auth.client, current_resource_owner, pre_auth.scopes, diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index 2391cabd6..09fe87d21 100644 --- a/lib/doorkeeper/config.rb +++ b/lib/doorkeeper/config.rb @@ -106,6 +106,13 @@ def revoke_previous_client_credentials_token @config.instance_variable_set(:@revoke_previous_client_credentials_token, true) end + # Only allow one valid access token obtained via authorization code + # per client. If a new access token is obtained before the old one + # expired, the old one gets revoked (disabled by default) + def revoke_previous_authorization_code_token + @config.instance_variable_set(:@revoke_previous_authorization_code_token, true) + end + # Use an API mode for applications generated with --api argument # It will skip applications controller, disable forgery protection def api_only @@ -481,6 +488,10 @@ def revoke_previous_client_credentials_token? option_set? :revoke_previous_client_credentials_token end + def revoke_previous_authorization_code_token? + option_set? :revoke_previous_authorization_code_token + end + def enforce_configured_scopes? option_set? :enforce_configured_scopes end diff --git a/lib/doorkeeper/models/access_token_mixin.rb b/lib/doorkeeper/models/access_token_mixin.rb index 4b23c69f8..ec6e55224 100644 --- a/lib/doorkeeper/models/access_token_mixin.rb +++ b/lib/doorkeeper/models/access_token_mixin.rb @@ -83,14 +83,18 @@ def revoke_all_for(application_id, resource_owner, clock = Time) # Resource Owner model instance or it's ID # @param scopes [String, Doorkeeper::OAuth::Scopes] # set of scopes + # @param custom_attributes [Nilable Hash] + # A nil value, or hash with keys corresponding to the custom attributes + # configured with the `custom_access_token_attributes` config option. + # A nil value will ignore custom attributes. # # @return [Doorkeeper::AccessToken, nil] Access Token instance or # nil if matching record was not found # - def matching_token_for(application, resource_owner, scopes, include_expired: true) + def matching_token_for(application, resource_owner, scopes, custom_attributes: nil, include_expired: true) tokens = authorized_tokens_for(application&.id, resource_owner) tokens = tokens.not_expired unless include_expired - find_matching_token(tokens, application, scopes) + find_matching_token(tokens, application, custom_attributes, scopes) end # Interface to enumerate access token records in batches in order not @@ -114,11 +118,15 @@ def find_access_token_in_batches(relation, **args, &block) # Application instance # @param scopes [String, Doorkeeper::OAuth::Scopes] # set of scopes + # @param custom_attributes [Nilable Hash] + # A nil value, or hash with keys corresponding to the custom attributes + # configured with the `custom_access_token_attributes` config option. + # A nil value will ignore custom attributes. # # @return [Doorkeeper::AccessToken, nil] Access Token instance or # nil if matching record was not found # - def find_matching_token(relation, application, scopes) + def find_matching_token(relation, application, custom_attributes, scopes) return nil unless relation matching_tokens = [] @@ -126,7 +134,8 @@ def find_matching_token(relation, application, scopes) find_access_token_in_batches(relation, batch_size: batch_size) do |batch| tokens = batch.select do |token| - scopes_match?(token.scopes, scopes, application&.scopes) + scopes_match?(token.scopes, scopes, application&.scopes) && + custom_attributes_match?(token, custom_attributes) end matching_tokens.concat(tokens) @@ -160,6 +169,31 @@ def scopes_match?(token_scopes, param_scopes, app_scopes) ) end + # Checks whether the token custom attribute values match the custom + # attributes from the parameters. + # + # @param token [Doorkeeper::AccessToken] + # The access token whose custom attributes are being compared + # to the custom_attributes. + # + # @param custom_attributes [Hash] + # A hash of the attributes for which we want to determine whether + # the token's custom attributes match. + # + # @return [Boolean] true if the token's custom attribute values + # match those in the custom_attributes, or if both are empty/blank. + # False otherwise. + def custom_attributes_match?(token, custom_attributes) + return true if custom_attributes.nil? + + token_attribs = token.custom_attributes + return true if token_attribs.blank? && custom_attributes.blank? + + Doorkeeper.config.custom_access_token_attributes.all? do |attribute| + token_attribs[attribute] == custom_attributes[attribute] + end + end + # Looking for not expired AccessToken record with a matching set of # scopes that belongs to specific Application and Resource Owner. # If it doesn't exists - then creates it. @@ -181,7 +215,9 @@ def scopes_match?(token_scopes, param_scopes, app_scopes) # def find_or_create_for(application:, resource_owner:, scopes:, **token_attributes) if Doorkeeper.config.reuse_access_token - access_token = matching_token_for(application, resource_owner, scopes, include_expired: false) + custom_attributes = extract_custom_attributes(token_attributes).presence + access_token = matching_token_for( + application, resource_owner, scopes, custom_attributes: custom_attributes, include_expired: false) return access_token if access_token&.reusable? end @@ -276,6 +312,18 @@ def secret_strategy def fallback_secret_strategy ::Doorkeeper.config.token_secret_fallback_strategy end + + # Extracts the token's custom attributes (defined by the + # custom_access_token_attributes config option) from the token's attributes. + # + # @param attributes [Hash] + # A hash of the access token's attributes. + # @return [Hash] + # A hash containing only the custom access token attributes. + def extract_custom_attributes(attributes) + attributes.with_indifferent_access.slice( + *Doorkeeper.configuration.custom_access_token_attributes) + end end # Access Token type: Bearer. @@ -308,6 +356,14 @@ def as_json(_options = {}) end end + # The token's custom attributes, as defined by + # the custom_access_token_attributes config option. + # + # @return [Hash] hash of custom access token attributes. + def custom_attributes + self.class.extract_custom_attributes(attributes) + end + # Indicates whether the token instance have the same credential # as the other Access Token. # diff --git a/lib/doorkeeper/oauth/authorization_code_request.rb b/lib/doorkeeper/oauth/authorization_code_request.rb index b3f3a7c23..be9ba0dbb 100644 --- a/lib/doorkeeper/oauth/authorization_code_request.rb +++ b/lib/doorkeeper/oauth/authorization_code_request.rb @@ -29,6 +29,10 @@ def before_successful_response grant.lock! raise Errors::InvalidGrantReuse if grant.revoked? + if Doorkeeper.config.revoke_previous_authorization_code_token? + revoke_previous_tokens(grant.application, resource_owner) + end + grant.revoke find_or_create_access_token( @@ -109,6 +113,10 @@ def custom_token_attributes_with_data .slice(*Doorkeeper.config.custom_access_token_attributes) .symbolize_keys end + + def revoke_previous_tokens(application, resource_owner) + Doorkeeper.config.access_token_model.revoke_all_for(application.id, resource_owner) + end end end end diff --git a/lib/doorkeeper/oauth/client_credentials/creator.rb b/lib/doorkeeper/oauth/client_credentials/creator.rb index 50f7ec8ae..6a0be4f6b 100644 --- a/lib/doorkeeper/oauth/client_credentials/creator.rb +++ b/lib/doorkeeper/oauth/client_credentials/creator.rb @@ -8,7 +8,7 @@ def call(client, scopes, attributes = {}) existing_token = nil if lookup_existing_token? - existing_token = find_active_existing_token_for(client, scopes) + existing_token = find_active_existing_token_for(client, scopes, attributes) return existing_token if Doorkeeper.config.reuse_access_token && existing_token&.reusable? end @@ -44,8 +44,11 @@ def lookup_existing_token? Doorkeeper.config.revoke_previous_client_credentials_token? end - def find_active_existing_token_for(client, scopes) - Doorkeeper.config.access_token_model.matching_token_for(client, nil, scopes, include_expired: false) + def find_active_existing_token_for(client, scopes, attributes) + custom_attributes = Doorkeeper.config.access_token_model. + extract_custom_attributes(attributes).presence + Doorkeeper.config.access_token_model.matching_token_for( + client, nil, scopes, custom_attributes: custom_attributes, include_expired: false) end end end diff --git a/lib/doorkeeper/oauth/pre_authorization.rb b/lib/doorkeeper/oauth/pre_authorization.rb index 7961a36e9..1c9fe8c0f 100644 --- a/lib/doorkeeper/oauth/pre_authorization.rb +++ b/lib/doorkeeper/oauth/pre_authorization.rb @@ -31,7 +31,7 @@ def initialize(server, parameters = {}, resource_owner = nil) @code_challenge = parameters[:code_challenge] @code_challenge_method = parameters[:code_challenge_method] @resource_owner = resource_owner - @custom_access_token_attributes = parameters.slice(*Doorkeeper.config.custom_access_token_attributes) + @custom_access_token_attributes = parameters.slice(*Doorkeeper.config.custom_access_token_attributes).to_h end def authorizable? diff --git a/lib/generators/doorkeeper/templates/initializer.rb b/lib/generators/doorkeeper/templates/initializer.rb index 080edf9d9..16696cf1e 100644 --- a/lib/generators/doorkeeper/templates/initializer.rb +++ b/lib/generators/doorkeeper/templates/initializer.rb @@ -167,6 +167,12 @@ # # revoke_previous_client_credentials_token + # Only allow one valid access token obtained via authorization code + # per client. If a new access token is obtained before the old one + # expired, the old one gets revoked (disabled by default) + # + # revoke_previous_authorization_code_token + # Hash access and refresh tokens before persisting them. # This will disable the possibility to use +reuse_access_token+ # since plain values can no longer be retrieved. diff --git a/spec/lib/oauth/authorization_code_request_spec.rb b/spec/lib/oauth/authorization_code_request_spec.rb index 580a43bc3..48a14487b 100644 --- a/spec/lib/oauth/authorization_code_request_spec.rb +++ b/spec/lib/oauth/authorization_code_request_spec.rb @@ -220,4 +220,40 @@ end end end + + context "when revoke_previous_authorization_code_token is false" do + before do + allow(Doorkeeper.config).to receive(:revoke_previous_authorization_code_token?).and_return(false) + end + + it "does not revoke the previous token" do + previous_token = FactoryBot.create( + :access_token, + application_id: client.id, + resource_owner_id: grant.resource_owner_id, + resource_owner_type: grant.resource_owner_type, + scopes: grant.scopes.to_s, + ) + + expect { request.authorize }.not_to(change { previous_token.reload.revoked_at }) + end + end + + context "when revoke_previous_authorization_code_token is true" do + before do + allow(Doorkeeper.config).to receive(:revoke_previous_authorization_code_token?).and_return(true) + end + + it "revokes the previous token" do + previous_token = FactoryBot.create( + :access_token, + application_id: client.id, + resource_owner_id: grant.resource_owner_id, + resource_owner_type: grant.resource_owner_type, + scopes: grant.scopes.to_s, + ) + + expect { request.authorize }.to(change { previous_token.reload.revoked_at }) + end + end end diff --git a/spec/models/doorkeeper/access_token_spec.rb b/spec/models/doorkeeper/access_token_spec.rb index 9714933f9..ab24571c7 100644 --- a/spec/models/doorkeeper/access_token_spec.rb +++ b/spec/models/doorkeeper/access_token_spec.rb @@ -634,6 +634,36 @@ def self.generate(_opts = {}) last_token = described_class.matching_token_for(application, resource_owner_id, scopes) expect(last_token).to eq(matching_token) end + + context "when custom access token attributes are used" do + before do + Doorkeeper.configure do + orm DOORKEEPER_ORM + custom_access_token_attributes [:tenant_name] + end + default_scopes_exist(*scopes.all) + end + let(:custom_attributes) { { tenant_name: "Me" } } + + it "returns a token when attributes match" do + token = FactoryBot.create :access_token, default_attributes.merge(custom_attributes) + last_token = described_class.matching_token_for( + application, resource_owner, scopes, custom_attributes: custom_attributes) + expect(last_token).to eq(token) + end + + it "does not return a token if attributes don't match" do + token = FactoryBot.create :access_token, default_attributes.merge(custom_attributes) + last_token = described_class.matching_token_for(application, resource_owner, scopes, custom_attributes: { tenant_id: 'different' }) + expect(last_token).to eq(nil) + end + + it "ignores custom attributes if a nil value is passed" do + token = FactoryBot.create :access_token, default_attributes.merge(custom_attributes) + last_token = described_class.matching_token_for(application, resource_owner, scopes, custom_attributes: nil) + expect(last_token).to eq(token) + end + end end describe "#as_json" do