Merge pull request #1660 from JeremyC-za/custom_attributes_multitenan…

…cy_patch

Stop auto-creation of AccessGrant when a matching token is found if custom access token attributes are used

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
 

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,

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

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,19 +118,24 @@ 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 = []
         batch_size = Doorkeeper.configuration.token_lookup_batch_size
 
         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.
     #

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

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

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?

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.

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

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