Merge pull request #1353 from doorkeeper-gem/revokation-client-creden…

…tials-fix

Make revocation of access tokens optional

CHANGELOG.md

@@ -13,6 +13,13 @@ User-visible changes worth mentioning.
 - [#1345] Allow to set custom classes for Doorkeeper models, extract reusable AR mixins.
 - [#1346] Refactor `Doorkeeper::Application#to_json` into convenient `#as_json` (fix #1344).
 - [#1349] Fix `Doorkeeper::Application` AR associations using an incorrect foreign key name when using a custom class.
+- [#1318] Make existing token revocation for client credentials optional and disable it by default.
+
+  **[IMPORTANT]** This is a change compared to the behaviour of version 5.2.
+  If you were relying on access tokens being revoked once the same client
+  requested a new access token, reenable it with `revoke_previous_client_credentials_token`.
+
+- [#1318] 
 
 ## 5.2.3
 
@@ -54,6 +61,11 @@ User-visible changes worth mentioning.
 
 - [#1270] Find matching tokens in batches for `reuse_access_token` option (fix #1193).
 - [#1271] Reintroduce existing token revocation for client credentials.
+
+  **[IMPORTANT]** If you rely on being able to fetch multiple access tokens from the same
+  client using client credentials flow, you should skip to version 5.3, where this behaviour
+  is deactivated by default.
+
 - [#1269] Update initializer template documentation.
 - [#1266] Use strong parameters within pre-authorization.
 - [#1264] Add :before_successful_authorization and :after_successful_authorization hooks in TokensController

lib/doorkeeper.rb

@@ -38,10 +38,9 @@
 require "doorkeeper/oauth/refresh_token_request"
 require "doorkeeper/oauth/password_access_token_request"
 
-require "doorkeeper/oauth/client_credentials/validation"
+require "doorkeeper/oauth/client_credentials/validator"
 require "doorkeeper/oauth/client_credentials/creator"
 require "doorkeeper/oauth/client_credentials/issuer"
-require "doorkeeper/oauth/client_credentials/validation"
 require "doorkeeper/oauth/client/credentials"
 
 require "doorkeeper/oauth/client_credentials_request"

lib/doorkeeper/config.rb

@@ -144,6 +144,14 @@ def token_reuse_limit(percentage)
         @config.instance_variable_set(:@token_reuse_limit, percentage)
       end
 
+      # TODO: maybe make it more generic for other flows too?
+      # Only allow one valid access token obtained via client credentials
+      # 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_client_credentials_token
+        @config.instance_variable_set(:@revoke_previous_client_credentials_token, true)
+      end
+
       # Use an API mode for applications generated with --api argument
       # It will skip applications controller, disable forgery protection
       def api_only
@@ -440,6 +448,10 @@ def token_reuse_limit
       @token_reuse_limit ||= 100
     end
 
+    def revoke_previous_client_credentials_token
+      @revoke_previous_client_credentials_token || false
+    end
+
     def resolve_controller(name)
       config_option = public_send(:"#{name}_controller")
       controller_name = if config_option.respond_to?(:call)

lib/doorkeeper/oauth/authorization_code_request.rb

@@ -94,7 +94,7 @@ def validate_code_verifier
       end
 
       def generate_code_challenge(code_verifier)
-        Doorkeeper.config.access_grant_model.generate_code_challenge(code_verifier)
+        server_config.access_grant_model.generate_code_challenge(code_verifier)
       end
     end
   end

lib/doorkeeper/oauth/base_request.rb

@@ -36,7 +36,7 @@ def valid?
 
       def find_or_create_access_token(client, resource_owner_id, scopes, server)
         context = Authorization::Token.build_context(client, grant_type, scopes)
-        @access_token = Doorkeeper.config.access_token_model.find_or_create_for(
+        @access_token = server_config.access_token_model.find_or_create_for(
           client,
           resource_owner_id,
           scopes,
@@ -46,11 +46,15 @@ def find_or_create_access_token(client, resource_owner_id, scopes, server)
       end
 
       def before_successful_response
-        Doorkeeper.config.before_successful_strategy_response.call(self)
+        server_config.before_successful_strategy_response.call(self)
       end
 
       def after_successful_response
-        Doorkeeper.config.after_successful_strategy_response.call(self, @response)
+        server_config.after_successful_strategy_response.call(self, @response)
+      end
+
+      def server_config
+        Doorkeeper.config
       end
 
       private

lib/doorkeeper/oauth/client_credentials/creator.rb

@@ -5,22 +5,31 @@ module OAuth
     class ClientCredentialsRequest < BaseRequest
       class Creator
         def call(client, scopes, attributes = {})
-          existing_token = existing_token_for(client, scopes)
+          if lookup_existing_token?
+            existing_token = find_existing_token_for(client, scopes)
+            return existing_token if server_config.reuse_access_token && existing_token&.reusable?
 
-          return existing_token if Doorkeeper.config.reuse_access_token && existing_token&.reusable?
+            existing_token&.revoke if server_config.revoke_previous_client_credentials_token
+          end
 
-          existing_token&.revoke
-
-          Doorkeeper.config.access_token_model.find_or_create_for(
+          server_config.access_token_model.find_or_create_for(
             client, nil, scopes, attributes[:expires_in],
             attributes[:use_refresh_token],
           )
         end
 
         private
 
-        def existing_token_for(client, scopes)
-          Doorkeeper.config.access_token_model.matching_token_for(client, nil, scopes)
+        def lookup_existing_token?
+          server_config.reuse_access_token || server_config.revoke_previous_client_credentials_token
+        end
+
+        def find_existing_token_for(client, scopes)
+          server_config.access_token_model.matching_token_for(client, nil, scopes)
+        end
+
+        def server_config
+          Doorkeeper.config
         end
       end
     end

lib/doorkeeper/oauth/client_credentials/issuer.rb

@@ -4,20 +4,20 @@ module Doorkeeper
   module OAuth
     class ClientCredentialsRequest < BaseRequest
       class Issuer
-        attr_accessor :token, :validation, :error
+        attr_accessor :token, :validator, :error
 
-        def initialize(server, validation)
+        def initialize(server, validator)
           @server = server
-          @validation = validation
+          @validator = validator
         end
 
         def create(client, scopes, creator = Creator.new)
-          if validation.valid?
+          if validator.valid?
             @token = create_token(client, scopes, creator)
             @error = :server_error unless @token
           else
             @token = false
-            @error = validation.error
+            @error = validator.error
           end
           @token
         end

lib/doorkeeper/oauth/client_credentials/validation.rb → lib/doorkeeper/oauth/client_credentials/validator.rb

@@ -3,7 +3,7 @@
 module Doorkeeper
   module OAuth
     class ClientCredentialsRequest < BaseRequest
-      class Validation
+      class Validator
         include Validations
         include OAuth::Helpers
 

lib/doorkeeper/oauth/client_credentials_request.rb

@@ -12,7 +12,7 @@ class ClientCredentialsRequest < BaseRequest
       delegate :error, to: :issuer
 
       def issuer
-        @issuer ||= Issuer.new(server, Validation.new(server, self))
+        @issuer ||= Issuer.new(server, Validator.new(server, self))
       end
 
       def initialize(server, client, parameters = {})

lib/doorkeeper/oauth/password_access_token_request.rb

@@ -50,7 +50,7 @@ def validate_client
       end
 
       def validate_client_supports_grant_flow
-        Doorkeeper.config.allow_grant_flow_for_client?(grant_type, client)
+        server_config.allow_grant_flow_for_client?(grant_type, client)
       end
     end
   end

lib/doorkeeper/oauth/refresh_token_request.rb

@@ -27,7 +27,7 @@ def initialize(server, refresh_token, credentials, parameters = {})
       private
 
       def load_client(credentials)
-        Doorkeeper.config.application_model.by_uid_and_secret(credentials.uid, credentials.secret)
+        server_config.application_model.by_uid_and_secret(credentials.uid, credentials.secret)
       end
 
       def before_successful_response
@@ -42,15 +42,15 @@ def before_successful_response
       end
 
       def refresh_token_revoked_on_use?
-        Doorkeeper.config.access_token_model.refresh_token_revoked_on_use?
+        server_config.access_token_model.refresh_token_revoked_on_use?
       end
 
       def default_scopes
         refresh_token.scopes
       end
 
       def create_access_token
-        @access_token = Doorkeeper.config.access_token_model.create!(access_token_attributes)
+        @access_token = server_config.access_token_model.create!(access_token_attributes)
       end
 
       def access_token_attributes

lib/generators/doorkeeper/templates/initializer.rb

@@ -127,6 +127,16 @@
   #
   # token_reuse_limit 100
 
+  # Only allow one valid access token obtained via client credentials
+  # per client. If a new access token is obtained before the old one
+  # expired, the old one gets revoked (disabled by default)
+  #
+  # When enabling this option, make sure that you do not expect multiple processes
+  # using the same credentials at the same time (e.g. web servers spanning
+  # multiple machines and/or processes).
+  #
+  # revoke_previous_client_credentials_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/client_credentials/creator_spec.rb

@@ -31,17 +31,31 @@ class Doorkeeper::OAuth::ClientCredentialsRequest
       end
 
       context "when existing token has not crossed token_reuse_limit" do
-        it "returns the existing valid token" do
+        let!(:existing_token) { subject.call(client, scopes, expires_in: 1000) }
+
+        before do
           allow(Doorkeeper.configuration).to receive(:reuse_access_token).and_return(true)
           allow(Doorkeeper.configuration).to receive(:token_reuse_limit).and_return(50)
-          existing_token = subject.call(client, scopes, expires_in: 1000)
-
           allow_any_instance_of(Doorkeeper::AccessToken).to receive(:expires_in_seconds).and_return(600)
+        end
+
+        it "returns the existing valid token" do
           result = subject.call(client, scopes, expires_in: 1000)
 
           expect(Doorkeeper::AccessToken.count).to eq(1)
           expect(result).to eq(existing_token)
         end
+
+        context "and when revoke_previous_client_credentials_token is true" do
+          before do
+            allow(Doorkeeper.configuration).to receive(:revoke_previous_client_credentials_token).and_return(false)
+          end
+
+          it "does not revoke the existing valid token" do
+            subject.call(client, scopes, expires_in: 1000)
+            expect(existing_token.reload).not_to be_revoked
+          end
+        end
       end
 
       context "when existing token has crossed token_reuse_limit" do
@@ -55,7 +69,6 @@ class Doorkeeper::OAuth::ClientCredentialsRequest
 
           expect(Doorkeeper::AccessToken.count).to eq(2)
           expect(result).not_to eq(existing_token)
-          expect(existing_token.reload).to be_revoked
         end
       end
 
@@ -70,7 +83,6 @@ class Doorkeeper::OAuth::ClientCredentialsRequest
 
           expect(Doorkeeper::AccessToken.count).to eq(2)
           expect(result).not_to eq(existing_token)
-          expect(existing_token.reload).to be_revoked
         end
       end
     end
@@ -84,10 +96,35 @@ class Doorkeeper::OAuth::ClientCredentialsRequest
 
         expect(Doorkeeper::AccessToken.count).to eq(2)
         expect(result).not_to eq(existing_token)
+      end
+    end
+
+    context "when revoke_previous_client_credentials_token is true" do
+      let!(:existing_token) { subject.call(client, scopes, expires_in: 1000) }
+
+      before do
+        allow(Doorkeeper.configuration).to receive(:revoke_previous_client_credentials_token).and_return(true)
+      end
+
+      it "revokes the existing token" do
+        subject.call(client, scopes, expires_in: 1000)
         expect(existing_token.reload).to be_revoked
       end
     end
 
+    context "when revoke_previous_client_credentials_token is false" do
+      let!(:existing_token) { subject.call(client, scopes, expires_in: 1000) }
+
+      before do
+        allow(Doorkeeper.configuration).to receive(:revoke_previous_client_credentials_token).and_return(false)
+      end
+
+      it "does not revoke the existing token" do
+        subject.call(client, scopes, expires_in: 1000)
+        expect(existing_token.reload).not_to be_revoked
+      end
+    end
+
     it "returns false if creation fails" do
       expect(Doorkeeper::AccessToken).to receive(:find_or_create_for).and_return(false)
       created = subject.call(client, scopes)

spec/lib/oauth/client_credentials/issuer_spec.rb

@@ -11,13 +11,13 @@ class Doorkeeper::OAuth::ClientCredentialsRequest
         access_token_expires_in: 100,
       )
     end
-    let(:validation) { double :validation, valid?: true }
+    let(:validator) { double :validator, valid?: true }
 
     before do
       allow(server).to receive(:option_defined?).with(:custom_access_token_expires_in).and_return(false)
     end
 
-    subject { Issuer.new(server, validation) }
+    subject { Issuer.new(server, validator) }
 
     describe :create do
       let(:client) { double :client, id: "some-id" }
@@ -48,14 +48,14 @@ class Doorkeeper::OAuth::ClientCredentialsRequest
         expect(subject.error).to eq(:server_error)
       end
 
-      context "when validation fails" do
+      context "when validator fails" do
         before do
-          allow(validation).to receive(:valid?).and_return(false)
-          allow(validation).to receive(:error).and_return(:validation_error)
+          allow(validator).to receive(:valid?).and_return(false)
+          allow(validator).to receive(:error).and_return(:validation_error)
           expect(creator).not_to receive(:create)
         end
 
-        it "has error set from validation" do
+        it "has error set from validator" do
           subject.create client, scopes, creator
           expect(subject.error).to eq(:validation_error)
         end
@@ -65,7 +65,7 @@ class Doorkeeper::OAuth::ClientCredentialsRequest
         end
       end
 
-      context "with custom expirations" do
+      context "with custom expiration" do
         let(:custom_ttl_grant) { 1234 }
         let(:custom_ttl_scope) { 1235 }
         let(:custom_scope) { "special" }

spec/lib/oauth/client_credentials/validation_spec.rb

@@ -3,13 +3,13 @@
 require "spec_helper"
 
 class Doorkeeper::OAuth::ClientCredentialsRequest
-  describe Validation do
+  describe Validator do
     let(:server)      { double :server, scopes: nil }
     let(:application) { double scopes: nil }
     let(:client)      { double application: application }
     let(:request)     { double :request, client: client, scopes: nil }
 
-    subject { Validation.new(server, request) }
+    subject { described_class.new(server, request) }
 
     it "is valid with valid request" do
       expect(subject).to be_valid