From 3fbe2f8366451577010bbe1b2f3baeb92f915099 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Sun, 20 Apr 2025 22:46:13 +0200 Subject: [PATCH 01/16] Initial prototyping --- lib/doorkeeper/client_authentication.rb | 26 ++++++++++++++ .../client_authentication/credentials.rb | 11 ++++++ .../client_authentication/mechanism.rb | 19 ++++++++++ .../mechanisms/client_secret_basic.rb | 22 ++++++++++++ .../mechanisms/client_secret_post.rb | 25 +++++++++++++ .../client_authentication/mechanisms/none.rb | 19 ++++++++++ .../client_authentication/registry.rb | 35 +++++++++++++++++++ lib/doorkeeper/config.rb | 8 +++++ lib/doorkeeper/oauth/client.rb | 2 ++ lib/doorkeeper/request.rb | 14 ++++++++ lib/doorkeeper/server.rb | 8 +++-- 11 files changed, 187 insertions(+), 2 deletions(-) create mode 100644 lib/doorkeeper/client_authentication.rb create mode 100644 lib/doorkeeper/client_authentication/credentials.rb create mode 100644 lib/doorkeeper/client_authentication/mechanism.rb create mode 100644 lib/doorkeeper/client_authentication/mechanisms/client_secret_basic.rb create mode 100644 lib/doorkeeper/client_authentication/mechanisms/client_secret_post.rb create mode 100644 lib/doorkeeper/client_authentication/mechanisms/none.rb create mode 100644 lib/doorkeeper/client_authentication/registry.rb diff --git a/lib/doorkeeper/client_authentication.rb b/lib/doorkeeper/client_authentication.rb new file mode 100644 index 000000000..88d833496 --- /dev/null +++ b/lib/doorkeeper/client_authentication.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require "doorkeeper/client_authentication/flow" +require "doorkeeper/client_authentication/registry" + +module Doorkeeper + module ClientAuthentication + extend Registry + + register( + :none, + mechanism: Doorkeeper::ClientAuthentication::Mechanisms::None, + authenticates_client: false + ) + + register( + :client_secret_post, + mechanism: Doorkeeper::ClientAuthentication::Mechanisms::ClientSecretPost, + ) + + register( + :client_secret_basic, + mechanism: Doorkeeper::ClientAuthentication::Mechanisms::ClientSecretBasic, + ) + end +end diff --git a/lib/doorkeeper/client_authentication/credentials.rb b/lib/doorkeeper/client_authentication/credentials.rb new file mode 100644 index 000000000..5f92e4a1e --- /dev/null +++ b/lib/doorkeeper/client_authentication/credentials.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Doorkeeper + module ClientAuthentication + Credentials = Struct.new(:uid, :secret) do + # Public clients may have their secret blank, but "credentials" are + # still present + delegate :blank?, to: :uid + end + end +end diff --git a/lib/doorkeeper/client_authentication/mechanism.rb b/lib/doorkeeper/client_authentication/mechanism.rb new file mode 100644 index 000000000..8aad8f945 --- /dev/null +++ b/lib/doorkeeper/client_authentication/mechanism.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Doorkeeper + module ClientAuthentication + class Mechanism + attr_reader :name, :mechanism + + def initialize(name, **options) + @name = name + @mechanism = options[:mechanism] + @authenticates_client = options.key?(:authenticates_client) ? options[:authenticates_client] : true + end + + def authenticates_client? + !!@authenticates_client + end + end + end +end diff --git a/lib/doorkeeper/client_authentication/mechanisms/client_secret_basic.rb b/lib/doorkeeper/client_authentication/mechanisms/client_secret_basic.rb new file mode 100644 index 000000000..7be4b442b --- /dev/null +++ b/lib/doorkeeper/client_authentication/mechanisms/client_secret_basic.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Doorkeeper + module ClientAuthentication + module Mechanisms + class ClientSecretBasic + def self.matches_request?(request) + request.authorization.present? && request.authorization.downcase.start_with?('basic') + end + + def authenticate(request) + value = request.authorization.to_s.split(" ", 2).second + client_id, client_secret = Base64.decode64(value).split(':', 2) + + return unless client_id.present? && client_secret.present? + + Doorkeeper::ClientAuthentication::Credentials.new(client_id, client_secret) + end + end + end + end +end diff --git a/lib/doorkeeper/client_authentication/mechanisms/client_secret_post.rb b/lib/doorkeeper/client_authentication/mechanisms/client_secret_post.rb new file mode 100644 index 000000000..4f72c20b0 --- /dev/null +++ b/lib/doorkeeper/client_authentication/mechanisms/client_secret_post.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Doorkeeper + module ClientAuthentication + module Mechanisms + class ClientSecretPost + def self.matches_request?(request) + request.method.upcase === "POST" + end + + def authenticate(request) + client_id = request.request_parameters[:client_id] + client_secret = request.request_parameters[:client_secret] + + return unless client_id.present? && client_secret.present? + + Doorkeeper::ClientAuthentication::Credentials.new( + client_id, + client_secret + ) + end + end + end + end +end diff --git a/lib/doorkeeper/client_authentication/mechanisms/none.rb b/lib/doorkeeper/client_authentication/mechanisms/none.rb new file mode 100644 index 000000000..beb61069d --- /dev/null +++ b/lib/doorkeeper/client_authentication/mechanisms/none.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Doorkeeper + module ClientAuthentication + module Mechanisms + class None + def self.matches_request?(request) + !request.authorization && request.request_parameters[:client_id] && !request.request_parameters[:client_secret] + end + + def authenticate(request) + Doorkeeper::ClientAuthentication::Credentials.new( + request.request_parameters[:client_id], nil + ) + end + end + end + end +end diff --git a/lib/doorkeeper/client_authentication/registry.rb b/lib/doorkeeper/client_authentication/registry.rb new file mode 100644 index 000000000..7bbea33b9 --- /dev/null +++ b/lib/doorkeeper/client_authentication/registry.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Doorkeeper + module GrantFlow + module Registry + mattr_accessor :mechanisms + self.mechanisms = {} + + # Allows to register custom OAuth client authentication mechanism so that + # Doorkeeper could recognize and process it. + # + def register(name_or_mechanism, **options) + unless name_or_mechanism.is_a?(Doorkeeper::ClientAuthentication::Mechanism) + name_or_mechanism = Doorkeeper::ClientAuthentication::Mechanism.new(name_or_mechanism, **options) + end + + name_key = name_or_mechanism.name.to_sym + + if mechanisms.key?(name_key) + ::Kernel.warn <<~WARNING + [DOORKEEPER] '#{name_key}' client authentication strategy is already registered and will be overridden + in #{caller(1..1).first} + WARNING + end + + mechanisms[name_key] = name_or_mechanism + end + + # [NOTE]: make it to use #fetch after removing fallbacks + def get(name) + mechanisms[name.to_sym] + end + end + end +end diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index c464f1803..ab2afbb72 100644 --- a/lib/doorkeeper/config.rb +++ b/lib/doorkeeper/config.rb @@ -253,6 +253,7 @@ def configure_secrets_for(type, using:, fallback:) option :orm, default: :active_record option :native_redirect_uri, default: "urn:ietf:wg:oauth:2.0:oob", deprecated: true option :grant_flows, default: %w[authorization_code client_credentials] + option :client_authentication, default: %w[client_secret_basic client_secret_post none] option :pkce_code_challenge_methods, default: %w[plain S256] option :handle_auth_errors, default: :render option :token_lookup_batch_size, default: 10_000 @@ -579,10 +580,17 @@ def pkce_code_challenge_methods_supported pkce_code_challenge_methods end + # Deprecated? def client_credentials_methods @client_credentials_methods ||= %i[from_basic from_params] end + def client_authentication_mechanisms + @client_authentication_mechanisms ||= client_authentication.map do |name| + Doorkeeper::ClientAuthentication.get(name) + end.compact + end + def access_token_methods @access_token_methods ||= %i[ from_bearer_authorization diff --git a/lib/doorkeeper/oauth/client.rb b/lib/doorkeeper/oauth/client.rb index d3cfbbeca..edde69f5e 100644 --- a/lib/doorkeeper/oauth/client.rb +++ b/lib/doorkeeper/oauth/client.rb @@ -17,6 +17,8 @@ def self.find(uid, method = Doorkeeper.config.application_model.method(:by_uid)) new(application) end + # TODO: Figure out a way to have this just get the client but not assert + # authentication if not secret def self.authenticate(credentials, method = Doorkeeper.config.application_model.method(:by_uid_and_secret)) return if credentials.blank? return unless (application = method.call(credentials.uid, credentials.secret)) diff --git a/lib/doorkeeper/request.rb b/lib/doorkeeper/request.rb index a2705a91f..57dfc9cf5 100644 --- a/lib/doorkeeper/request.rb +++ b/lib/doorkeeper/request.rb @@ -3,6 +3,16 @@ module Doorkeeper module Request class << self + def client_authentication_mechanism(request) + client_authentication_mechanisms = client_authentication_mechanisms.detect do |mechanism| + mechanism.matches_request?(request) + end + + if client_authentication_mechanisms + client_authentication_mechanisms.mechanism + end + end + def authorization_strategy(response_type) grant_flow = authorization_flows.detect do |flow| flow.matches_response_type?(response_type) @@ -40,6 +50,10 @@ def token_strategy(grant_type) private + def client_authentication_mechanisms + Doorkeeper.configuration.client_authentication_mechanisms + end + def authorization_flows Doorkeeper.configuration.authorization_response_flows end diff --git a/lib/doorkeeper/server.rb b/lib/doorkeeper/server.rb index 4ba0e3665..be768afd0 100644 --- a/lib/doorkeeper/server.rb +++ b/lib/doorkeeper/server.rb @@ -8,6 +8,11 @@ def initialize(context) @context = context end + def client_authentication_request(strategy) + klass = Request.client_authentication_mechanism(context.request) + klass.new(self) + end + def authorization_request(strategy) klass = Request.authorization_strategy(strategy) klass.new(self) @@ -37,8 +42,7 @@ def resource_owner end def credentials - methods = Doorkeeper.config.client_credentials_methods - @credentials ||= OAuth::Client::Credentials.from_request(context.request, *methods) + @credentials ||= client_authentication_request.authenticate(context.request) end end end From 7fdad7399edf4f27c2d3830cbe6059683e004c9a Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 00:03:52 +0200 Subject: [PATCH 02/16] Second pass, because wow my code was rough --- lib/doorkeeper.rb | 14 ++- lib/doorkeeper/client_authentication.rb | 10 +- .../fallback_mechanism.rb | 11 +++ .../client_authentication/mechanism.rb | 4 + .../client_authentication/registry.rb | 2 +- lib/doorkeeper/oauth/client/credentials.rb | 34 ------- .../client_secret_basic.rb | 4 +- .../client_secret_post.rb | 8 +- .../client_authentication}/none.rb | 4 +- lib/doorkeeper/request.rb | 12 ++- lib/doorkeeper/server.rb | 4 +- .../client_authentication/credentials_spec.rb | 92 +++++++++++++++++++ spec/lib/oauth/client/credentials_spec.rb | 90 ------------------ spec/lib/oauth/client_spec.rb | 4 +- .../password_access_token_request_spec.rb | 2 +- spec/lib/oauth/refresh_token_request_spec.rb | 6 +- spec/requests/flows/password_spec.rb | 2 +- spec/requests/flows/refresh_token_spec.rb | 1 + spec/support/helpers/url_helper.rb | 1 + 19 files changed, 149 insertions(+), 156 deletions(-) create mode 100644 lib/doorkeeper/client_authentication/fallback_mechanism.rb delete mode 100644 lib/doorkeeper/oauth/client/credentials.rb rename lib/doorkeeper/{client_authentication/mechanisms => oauth/client_authentication}/client_secret_basic.rb (92%) rename lib/doorkeeper/{client_authentication/mechanisms => oauth/client_authentication}/client_secret_post.rb (70%) rename lib/doorkeeper/{client_authentication/mechanisms => oauth/client_authentication}/none.rb (89%) create mode 100644 spec/lib/client_authentication/credentials_spec.rb delete mode 100644 spec/lib/oauth/client/credentials_spec.rb diff --git a/lib/doorkeeper.rb b/lib/doorkeeper.rb index 97e3f549a..a67ec6d8d 100644 --- a/lib/doorkeeper.rb +++ b/lib/doorkeeper.rb @@ -7,6 +7,7 @@ # module Doorkeeper autoload :Errors, "doorkeeper/errors" + autoload :ClientAuthentication, "doorkeeper/client_authentication" autoload :GrantFlow, "doorkeeper/grant_flow" autoload :OAuth, "doorkeeper/oauth" autoload :Rake, "doorkeeper/rake" @@ -33,7 +34,6 @@ module Request autoload :RefreshToken, "doorkeeper/request/refresh_token" autoload :Token, "doorkeeper/request/token" end - module RevocableTokens autoload :RevocableAccessToken, "doorkeeper/revocable_tokens/revocable_access_token" autoload :RevocableRefreshToken, "doorkeeper/revocable_tokens/revocable_refresh_token" @@ -62,6 +62,12 @@ module OAuth autoload :TokenRequest, "doorkeeper/oauth/token_request" autoload :TokenResponse, "doorkeeper/oauth/token_response" + module ClientAuthentication + autoload :None, "doorkeeper/oauth/client_authentication/none" + autoload :ClientSecretBasic, "doorkeeper/oauth/client_authentication/client_secret_basic" + autoload :ClientSecretPost, "doorkeeper/oauth/client_authentication/client_secret_post" + end + module Authorization autoload :Code, "doorkeeper/oauth/authorization/code" autoload :Context, "doorkeeper/oauth/authorization/context" @@ -69,9 +75,9 @@ module Authorization autoload :URIBuilder, "doorkeeper/oauth/authorization/uri_builder" end - class Client - autoload :Credentials, "doorkeeper/oauth/client/credentials" - end + # class Client + # autoload :Credentials, "doorkeeper/oauth/client/credentials" + # end module ClientCredentials autoload :Validator, "doorkeeper/oauth/client_credentials/validator" diff --git a/lib/doorkeeper/client_authentication.rb b/lib/doorkeeper/client_authentication.rb index 88d833496..539f264e8 100644 --- a/lib/doorkeeper/client_authentication.rb +++ b/lib/doorkeeper/client_authentication.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true -require "doorkeeper/client_authentication/flow" +require "doorkeeper/client_authentication/credentials" +require "doorkeeper/client_authentication/fallback_mechanism" +require "doorkeeper/client_authentication/mechanism" require "doorkeeper/client_authentication/registry" module Doorkeeper @@ -9,18 +11,18 @@ module ClientAuthentication register( :none, - mechanism: Doorkeeper::ClientAuthentication::Mechanisms::None, + mechanism: Doorkeeper::OAuth::ClientAuthentication::None, authenticates_client: false ) register( :client_secret_post, - mechanism: Doorkeeper::ClientAuthentication::Mechanisms::ClientSecretPost, + mechanism: Doorkeeper::OAuth::ClientAuthentication::ClientSecretPost, ) register( :client_secret_basic, - mechanism: Doorkeeper::ClientAuthentication::Mechanisms::ClientSecretBasic, + mechanism: Doorkeeper::OAuth::ClientAuthentication::ClientSecretBasic, ) end end diff --git a/lib/doorkeeper/client_authentication/fallback_mechanism.rb b/lib/doorkeeper/client_authentication/fallback_mechanism.rb new file mode 100644 index 000000000..a116829ab --- /dev/null +++ b/lib/doorkeeper/client_authentication/fallback_mechanism.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Doorkeeper + module ClientAuthentication + class FallbackMechanism + def authenticate(request) + Doorkeeper::ClientAuthentication::Credentials.new(nil, nil) + end + end + end +end diff --git a/lib/doorkeeper/client_authentication/mechanism.rb b/lib/doorkeeper/client_authentication/mechanism.rb index 8aad8f945..980f8c4f3 100644 --- a/lib/doorkeeper/client_authentication/mechanism.rb +++ b/lib/doorkeeper/client_authentication/mechanism.rb @@ -14,6 +14,10 @@ def initialize(name, **options) def authenticates_client? !!@authenticates_client end + + def matches_request?(request) + @mechanism.matches_request?(request) + end end end end diff --git a/lib/doorkeeper/client_authentication/registry.rb b/lib/doorkeeper/client_authentication/registry.rb index 7bbea33b9..be8f83280 100644 --- a/lib/doorkeeper/client_authentication/registry.rb +++ b/lib/doorkeeper/client_authentication/registry.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Doorkeeper - module GrantFlow + module ClientAuthentication module Registry mattr_accessor :mechanisms self.mechanisms = {} diff --git a/lib/doorkeeper/oauth/client/credentials.rb b/lib/doorkeeper/oauth/client/credentials.rb deleted file mode 100644 index 8c96a14ab..000000000 --- a/lib/doorkeeper/oauth/client/credentials.rb +++ /dev/null @@ -1,34 +0,0 @@ -# frozen_string_literal: true - -module Doorkeeper - module OAuth - class Client - Credentials = Struct.new(:uid, :secret) do - class << self - def from_request(request, *credentials_methods) - credentials_methods.inject(nil) do |_, method| - method = self.method(method) if method.is_a?(Symbol) - credentials = Credentials.new(*method.call(request)) - break credentials if credentials.present? - end - end - - def from_params(request) - request.parameters.values_at(:client_id, :client_secret) - end - - def from_basic(request) - authorization = request.authorization - if authorization.present? && authorization =~ /^Basic (.*)/m - Base64.decode64(Regexp.last_match(1)).split(/:/, 2) - end - end - end - - # Public clients may have their secret blank, but "credentials" are - # still present - delegate :blank?, to: :uid - end - end - end -end diff --git a/lib/doorkeeper/client_authentication/mechanisms/client_secret_basic.rb b/lib/doorkeeper/oauth/client_authentication/client_secret_basic.rb similarity index 92% rename from lib/doorkeeper/client_authentication/mechanisms/client_secret_basic.rb rename to lib/doorkeeper/oauth/client_authentication/client_secret_basic.rb index 7be4b442b..a85b4eff0 100644 --- a/lib/doorkeeper/client_authentication/mechanisms/client_secret_basic.rb +++ b/lib/doorkeeper/oauth/client_authentication/client_secret_basic.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true module Doorkeeper - module ClientAuthentication - module Mechanisms + module OAuth + module ClientAuthentication class ClientSecretBasic def self.matches_request?(request) request.authorization.present? && request.authorization.downcase.start_with?('basic') diff --git a/lib/doorkeeper/client_authentication/mechanisms/client_secret_post.rb b/lib/doorkeeper/oauth/client_authentication/client_secret_post.rb similarity index 70% rename from lib/doorkeeper/client_authentication/mechanisms/client_secret_post.rb rename to lib/doorkeeper/oauth/client_authentication/client_secret_post.rb index 4f72c20b0..28cdc577b 100644 --- a/lib/doorkeeper/client_authentication/mechanisms/client_secret_post.rb +++ b/lib/doorkeeper/oauth/client_authentication/client_secret_post.rb @@ -1,19 +1,17 @@ # frozen_string_literal: true module Doorkeeper - module ClientAuthentication - module Mechanisms + module OAuth + module ClientAuthentication class ClientSecretPost def self.matches_request?(request) - request.method.upcase === "POST" + request.method.upcase === "POST" && request.request_parameters[:client_id].present? && request.request_parameters[:client_secret].present? end def authenticate(request) client_id = request.request_parameters[:client_id] client_secret = request.request_parameters[:client_secret] - return unless client_id.present? && client_secret.present? - Doorkeeper::ClientAuthentication::Credentials.new( client_id, client_secret diff --git a/lib/doorkeeper/client_authentication/mechanisms/none.rb b/lib/doorkeeper/oauth/client_authentication/none.rb similarity index 89% rename from lib/doorkeeper/client_authentication/mechanisms/none.rb rename to lib/doorkeeper/oauth/client_authentication/none.rb index beb61069d..16f65059f 100644 --- a/lib/doorkeeper/client_authentication/mechanisms/none.rb +++ b/lib/doorkeeper/oauth/client_authentication/none.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true module Doorkeeper - module ClientAuthentication - module Mechanisms + module OAuth + module ClientAuthentication class None def self.matches_request?(request) !request.authorization && request.request_parameters[:client_id] && !request.request_parameters[:client_secret] diff --git a/lib/doorkeeper/request.rb b/lib/doorkeeper/request.rb index 57dfc9cf5..94f55be6a 100644 --- a/lib/doorkeeper/request.rb +++ b/lib/doorkeeper/request.rb @@ -4,12 +4,14 @@ module Doorkeeper module Request class << self def client_authentication_mechanism(request) - client_authentication_mechanisms = client_authentication_mechanisms.detect do |mechanism| - mechanism.matches_request?(request) + strategy = client_authentication_strategies.detect do |strategy| + strategy.matches_request?(request) end - if client_authentication_mechanisms - client_authentication_mechanisms.mechanism + if strategy + strategy.mechanism + else + Doorkeeper::ClientAuthentication::FallbackMechanism end end @@ -50,7 +52,7 @@ def token_strategy(grant_type) private - def client_authentication_mechanisms + def client_authentication_strategies Doorkeeper.configuration.client_authentication_mechanisms end diff --git a/lib/doorkeeper/server.rb b/lib/doorkeeper/server.rb index be768afd0..f7f355278 100644 --- a/lib/doorkeeper/server.rb +++ b/lib/doorkeeper/server.rb @@ -8,9 +8,9 @@ def initialize(context) @context = context end - def client_authentication_request(strategy) + def client_authentication_request klass = Request.client_authentication_mechanism(context.request) - klass.new(self) + klass.new end def authorization_request(strategy) diff --git a/spec/lib/client_authentication/credentials_spec.rb b/spec/lib/client_authentication/credentials_spec.rb new file mode 100644 index 000000000..7271a5a17 --- /dev/null +++ b/spec/lib/client_authentication/credentials_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Doorkeeper::ClientAuthentication::Credentials do + let(:client_id) { "some-uid" } + let(:client_secret) { "some-secret" } + + it "is blank when the uid in credentials is blank" do + expect(described_class.new(nil, nil)).to be_blank + expect(described_class.new(nil, "something")).to be_blank + expect(described_class.new("something", nil)).to be_present + expect(described_class.new("something", "something")).to be_present + end + + # FIXME: Move to individual tests: + # describe Credentials do + + # describe ".from_request" do + # let(:request) { double.as_null_object } + + # let(:method) do + # ->(_request) { %w[uid secret] } + # end + + # it "accepts anything that responds to #call" do + # expect(method).to receive(:call).with(request) + # described_class.from_request request, method + # end + + # it "delegates methods received as symbols to Credentials class" do + # expect(described_class).to receive(:from_params).with(request) + # described_class.from_request request, :from_params + # end + + # it "stops at the first credentials found" do + # not_called_method = double + # expect(not_called_method).not_to receive(:call) + # described_class.from_request request, ->(_) {}, method, not_called_method + # end + + # it "returns new Credentials" do + # credentials = described_class.from_request request, method + # expect(credentials).to be_a(described_class) + # end + + # it "returns uid and secret from extractor method" do + # credentials = described_class.from_request request, method + # expect(credentials.uid).to eq("uid") + # expect(credentials.secret).to eq("secret") + # end + # end + + # describe ".from_params" do + # it "returns credentials from parameters when Authorization header is not available" do + # request = double parameters: { client_id: client_id, client_secret: client_secret } + # uid, secret = described_class.from_params(request) + + # expect(uid).to eq("some-uid") + # expect(secret).to eq("some-secret") + # end + + # it "is blank when there are no credentials" do + # request = double parameters: {} + # uid, secret = described_class.from_params(request) + + # expect(uid).to be_blank + # expect(secret).to be_blank + # end + # end + + # describe ".from_basic" do + # let(:credentials) { Base64.encode64("#{client_id}:#{client_secret}") } + + # it "decodes the credentials" do + # request = double authorization: "Basic #{credentials}" + # uid, secret = described_class.from_basic(request) + + # expect(uid).to eq("some-uid") + # expect(secret).to eq("some-secret") + # end + + # it "is blank if Authorization is not Basic" do + # request = double authorization: credentials.to_s + # uid, secret = described_class.from_basic(request) + + # expect(uid).to be_blank + # expect(secret).to be_blank + # end + # end + # end +end diff --git a/spec/lib/oauth/client/credentials_spec.rb b/spec/lib/oauth/client/credentials_spec.rb deleted file mode 100644 index 46c3425d6..000000000 --- a/spec/lib/oauth/client/credentials_spec.rb +++ /dev/null @@ -1,90 +0,0 @@ -# frozen_string_literal: true - -require "spec_helper" - -class Doorkeeper::OAuth::Client - describe Credentials do - let(:client_id) { "some-uid" } - let(:client_secret) { "some-secret" } - - it "is blank when the uid in credentials is blank" do - expect(described_class.new(nil, nil)).to be_blank - expect(described_class.new(nil, "something")).to be_blank - expect(described_class.new("something", nil)).to be_present - expect(described_class.new("something", "something")).to be_present - end - - describe ".from_request" do - let(:request) { double.as_null_object } - - let(:method) do - ->(_request) { %w[uid secret] } - end - - it "accepts anything that responds to #call" do - expect(method).to receive(:call).with(request) - described_class.from_request request, method - end - - it "delegates methods received as symbols to Credentials class" do - expect(described_class).to receive(:from_params).with(request) - described_class.from_request request, :from_params - end - - it "stops at the first credentials found" do - not_called_method = double - expect(not_called_method).not_to receive(:call) - described_class.from_request request, ->(_) {}, method, not_called_method - end - - it "returns new Credentials" do - credentials = described_class.from_request request, method - expect(credentials).to be_a(described_class) - end - - it "returns uid and secret from extractor method" do - credentials = described_class.from_request request, method - expect(credentials.uid).to eq("uid") - expect(credentials.secret).to eq("secret") - end - end - - describe ".from_params" do - it "returns credentials from parameters when Authorization header is not available" do - request = double parameters: { client_id: client_id, client_secret: client_secret } - uid, secret = described_class.from_params(request) - - expect(uid).to eq("some-uid") - expect(secret).to eq("some-secret") - end - - it "is blank when there are no credentials" do - request = double parameters: {} - uid, secret = described_class.from_params(request) - - expect(uid).to be_blank - expect(secret).to be_blank - end - end - - describe ".from_basic" do - let(:credentials) { Base64.encode64("#{client_id}:#{client_secret}") } - - it "decodes the credentials" do - request = double authorization: "Basic #{credentials}" - uid, secret = described_class.from_basic(request) - - expect(uid).to eq("some-uid") - expect(secret).to eq("some-secret") - end - - it "is blank if Authorization is not Basic" do - request = double authorization: credentials.to_s - uid, secret = described_class.from_basic(request) - - expect(uid).to be_blank - expect(secret).to be_blank - end - end - end -end diff --git a/spec/lib/oauth/client_spec.rb b/spec/lib/oauth/client_spec.rb index af3dced5b..0a9b92ac5 100644 --- a/spec/lib/oauth/client_spec.rb +++ b/spec/lib/oauth/client_spec.rb @@ -21,7 +21,7 @@ describe ".authenticate" do it "returns the authenticated client via credentials" do - credentials = Doorkeeper::OAuth::Client::Credentials.new("some-uid", "some-secret") + credentials = Doorkeeper::ClientAuthentication::Credentials.new("some-uid", "some-secret") authenticator = double expect(authenticator).to receive(:call).with("some-uid", "some-secret").and_return(double) expect(described_class.authenticate(credentials, authenticator)) @@ -29,7 +29,7 @@ end it "returns nil if client was not authenticated" do - credentials = Doorkeeper::OAuth::Client::Credentials.new("some-uid", "some-secret") + credentials = Doorkeeper::ClientAuthentication::Credentials.new("some-uid", "some-secret") authenticator = double expect(authenticator).to receive(:call).with("some-uid", "some-secret").and_return(nil) expect(described_class.authenticate(credentials, authenticator)).to be_nil diff --git a/spec/lib/oauth/password_access_token_request_spec.rb b/spec/lib/oauth/password_access_token_request_spec.rb index 448e325be..8e00a2a29 100644 --- a/spec/lib/oauth/password_access_token_request_spec.rb +++ b/spec/lib/oauth/password_access_token_request_spec.rb @@ -19,7 +19,7 @@ ) end let(:client) { Doorkeeper::OAuth::Client.new(FactoryBot.create(:application)) } - let(:credentials) { Doorkeeper::OAuth::Client::Credentials.new("uid", "secret") } + let(:credentials) { Doorkeeper::ClientAuthentication::Credentials.new("uid", "secret") } let(:application) { client.application } let(:owner) { FactoryBot.build_stubbed(:resource_owner) } diff --git a/spec/lib/oauth/refresh_token_request_spec.rb b/spec/lib/oauth/refresh_token_request_spec.rb index 79fbd0c3e..47073164d 100644 --- a/spec/lib/oauth/refresh_token_request_spec.rb +++ b/spec/lib/oauth/refresh_token_request_spec.rb @@ -14,7 +14,7 @@ end let(:client) { refresh_token.application } - let(:credentials) { Doorkeeper::OAuth::Client::Credentials.new(client.uid, client.secret) } + let(:credentials) { Doorkeeper::ClientAuthentication::Credentials.new(client.uid, client.secret) } before do allow(Doorkeeper::AccessToken).to receive(:refresh_token_revoked_on_use?).and_return(false) @@ -59,7 +59,7 @@ end it "requires credentials to be valid if provided" do - credentials = Doorkeeper::OAuth::Client::Credentials.new("invalid", "invalid") + credentials = Doorkeeper::ClientAuthentication::Credentials.new("invalid", "invalid") request = described_class.new(server, refresh_token, credentials) request.validate expect(request.error).to eq(Doorkeeper::Errors::InvalidClient) @@ -67,7 +67,7 @@ it "requires the token's client and current client to match" do other_app = FactoryBot.create(:application) - credentials = Doorkeeper::OAuth::Client::Credentials.new(other_app.uid, other_app.secret) + credentials = Doorkeeper::ClientAuthentication::Credentials.new(other_app.uid, other_app.secret) request = described_class.new(server, refresh_token, credentials) request.validate diff --git a/spec/requests/flows/password_spec.rb b/spec/requests/flows/password_spec.rb index 64306a41e..3d65fd6ce 100644 --- a/spec/requests/flows/password_spec.rb +++ b/spec/requests/flows/password_spec.rb @@ -240,7 +240,7 @@ end it "doesn't issue a new token with invalid client credentials in Basic auth" do - invalid_client = Doorkeeper::OAuth::Client::Credentials.new("invalid", "invalid") + invalid_client = Doorkeeper::ClientAuthentication::Credentials.new("invalid", "invalid") expect do post password_token_endpoint_url( diff --git a/spec/requests/flows/refresh_token_spec.rb b/spec/requests/flows/refresh_token_spec.rb index c319bfedf..ccd883c79 100644 --- a/spec/requests/flows/refresh_token_spec.rb +++ b/spec/requests/flows/refresh_token_spec.rb @@ -97,6 +97,7 @@ it "client request a token with expired access token" do @token.update_attribute :expires_in, -100 + # This method is broken: post refresh_token_endpoint_url( client: @client, refresh_token: @token.refresh_token, ) diff --git a/spec/support/helpers/url_helper.rb b/spec/support/helpers/url_helper.rb index 32b6ce683..5c60207c1 100644 --- a/spec/support/helpers/url_helper.rb +++ b/spec/support/helpers/url_helper.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true module UrlHelper + # FIXME def token_endpoint_url(options = {}) parameters = { code: options[:code], From c1e95e9cbbc4d9a7e42ebe2cbb2771c2586be33d Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 01:27:01 +0200 Subject: [PATCH 03/16] Fix token introspection for missing credentials --- lib/doorkeeper/oauth/token_introspection.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/doorkeeper/oauth/token_introspection.rb b/lib/doorkeeper/oauth/token_introspection.rb index 4a88118fc..155563735 100644 --- a/lib/doorkeeper/oauth/token_introspection.rb +++ b/lib/doorkeeper/oauth/token_introspection.rb @@ -57,7 +57,7 @@ def to_json(*) # def authorize! # Requested client authorization - if server.credentials + if server.credentials.present? authorize_using_basic_auth! elsif authorized_token authorize_using_bearer_token! From 93b2d41e77dc1a53608ef8eb7047e073535c6022 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 01:27:32 +0200 Subject: [PATCH 04/16] Fix broken tests due to them using query string instead of form body --- spec/requests/endpoints/token_spec.rb | 16 +++--- .../flows/authorization_code_errors_spec.rb | 6 +-- .../requests/flows/authorization_code_spec.rb | 14 ++--- .../requests/flows/client_credentials_spec.rb | 26 ++++----- spec/requests/flows/password_spec.rb | 54 +++++++++---------- spec/requests/flows/refresh_token_spec.rb | 35 ++++++------ spec/support/helpers/request_spec_helper.rb | 2 +- spec/support/helpers/url_helper.rb | 25 +++++---- 8 files changed, 93 insertions(+), 85 deletions(-) diff --git a/spec/requests/endpoints/token_spec.rb b/spec/requests/endpoints/token_spec.rb index 898af80b1..a5657ec36 100644 --- a/spec/requests/endpoints/token_spec.rb +++ b/spec/requests/endpoints/token_spec.rb @@ -13,7 +13,7 @@ end it "respond with correct headers" do - post token_endpoint_url(code: @authorization.token, client: @client) + post token_endpoint_url, params: token_endpoint_params(code: @authorization.token, client: @client) expect(headers["Cache-Control"]).to be_in(["no-store", "no-cache, no-store", "private, no-store"]) expect(headers["Content-Type"]).to eq("application/json; charset=utf-8") @@ -22,10 +22,10 @@ it "accepts client credentials with basic auth header" do post token_endpoint_url, - params: { + params: token_endpoint_params( code: @authorization.token, redirect_uri: @client.redirect_uri, - }, + ), headers: { "HTTP_AUTHORIZATION" => basic_auth_header_for_client(@client) } expect(json_response).to include("access_token" => Doorkeeper::AccessToken.first.token) @@ -34,14 +34,14 @@ it "returns null for expires_in when a permanent token is set" do config_is_set(:access_token_expires_in, nil) - post token_endpoint_url(code: @authorization.token, client: @client) + post token_endpoint_url, params: token_endpoint_params(code: @authorization.token, client: @client) expect(json_response).to include("access_token" => Doorkeeper::AccessToken.first.token) expect(json_response).not_to include("expires_in") end it "returns unsupported_grant_type for invalid grant_type param" do - post token_endpoint_url(code: @authorization.token, client: @client, grant_type: "nothing") + post token_endpoint_url, params: token_endpoint_params(code: @authorization.token, client: @client, grant_type: "nothing") expect(json_response).to match( "error" => "unsupported_grant_type", @@ -51,7 +51,7 @@ it "returns unsupported_grant_type for disabled grant flows" do config_is_set(:grant_flows, ["implicit"]) - post token_endpoint_url(code: @authorization.token, client: @client, grant_type: "authorization_code") + post token_endpoint_url, params: token_endpoint_params(code: @authorization.token, client: @client, grant_type: "authorization_code") expect(json_response).to match( "error" => "unsupported_grant_type", @@ -60,7 +60,7 @@ end it "returns unsupported_grant_type when refresh_token is not in use" do - post token_endpoint_url(code: @authorization.token, client: @client, grant_type: "refresh_token") + post token_endpoint_url, params: token_endpoint_params(code: @authorization.token, client: @client, grant_type: "refresh_token") expect(json_response).to match( "error" => "unsupported_grant_type", @@ -69,7 +69,7 @@ end it "returns invalid_request if grant_type is missing" do - post token_endpoint_url(code: @authorization.token, client: @client, grant_type: "") + post token_endpoint_url, params: token_endpoint_params(code: @authorization.token, client: @client, grant_type: "") expect(json_response).to match( "error" => "invalid_request", diff --git a/spec/requests/flows/authorization_code_errors_spec.rb b/spec/requests/flows/authorization_code_errors_spec.rb index b192d9f0d..523e2c5ef 100644 --- a/spec/requests/flows/authorization_code_errors_spec.rb +++ b/spec/requests/flows/authorization_code_errors_spec.rb @@ -60,11 +60,11 @@ it "returns :invalid_grant error when posting an already revoked grant code" do # First successful request - post token_endpoint_url(code: @authorization.token, client: @client) + post token_endpoint_url, params: token_endpoint_params(code: @authorization.token, client: @client) # Second attempt with same token expect do - post token_endpoint_url(code: @authorization.token, client: @client) + post token_endpoint_url, params: token_endpoint_params(code: @authorization.token, client: @client) end.not_to(change { Doorkeeper::AccessToken.count }) expect(json_response).to match( @@ -74,7 +74,7 @@ end it "returns :invalid_grant error for invalid grant code" do - post token_endpoint_url(code: "invalid", client: @client) + post token_endpoint_url, params: token_endpoint_params(code: "invalid", client: @client) access_token_should_not_exist diff --git a/spec/requests/flows/authorization_code_spec.rb b/spec/requests/flows/authorization_code_spec.rb index c0bd5495d..9d2bb9a2e 100644 --- a/spec/requests/flows/authorization_code_spec.rb +++ b/spec/requests/flows/authorization_code_spec.rb @@ -155,7 +155,7 @@ def authorize(redirect_url) click_on "Authorize" authorization_code = Doorkeeper::AccessGrant.first.token - page.driver.post token_endpoint_url( + page.driver.post token_endpoint_url, token_endpoint_params( code: authorization_code, client_id: @client.uid, redirect_uri: @client.redirect_uri, @@ -174,7 +174,7 @@ def authorize(redirect_url) click_on "Authorize" authorization_code = Doorkeeper::AccessGrant.first.token - page.driver.post token_endpoint_url( + page.driver.post token_endpoint_url, token_endpoint_params( code: authorization_code, client_secret: @client.secret, redirect_uri: @client.redirect_uri, @@ -334,7 +334,7 @@ def authorize(redirect_url) click_on "Authorize" authorization_code = current_params["code"] - page.driver.post token_endpoint_url( + page.driver.post token_endpoint_url, token_endpoint_params( code: authorization_code, client_id: @client.uid, redirect_uri: @client.redirect_uri, @@ -353,7 +353,7 @@ def authorize(redirect_url) click_on "Authorize" authorization_code = current_params["code"] - page.driver.post token_endpoint_url( + page.driver.post token_endpoint_url, token_endpoint_params( code: authorization_code, client_id: @client.uid, redirect_uri: @client.redirect_uri, @@ -413,7 +413,7 @@ def authorize(redirect_url) click_on "Authorize" authorization_code = current_params["code"] - page.driver.post token_endpoint_url( + page.driver.post token_endpoint_url, token_endpoint_params( code: authorization_code, client: @client, code_verifier: code_challenge, @@ -543,7 +543,7 @@ def authorize(redirect_url) allow_any_instance_of(Doorkeeper::AccessGrant) .to receive(:revoked?).and_return(false, true) - page.driver.post token_endpoint_url(code: authorization_code, client: @client) + page.driver.post token_endpoint_url, token_endpoint_params(code: authorization_code, client: @client) expect(json_response).to match( "error" => "invalid_grant", @@ -573,7 +573,7 @@ def authorize(redirect_url) end it "copies custom attributes from the grant into the token" do - page.driver.post token_endpoint_url(code: grant.token, client: client) + page.driver.post token_endpoint_url, token_endpoint_params(code: grant.token, client: client) access_token = Doorkeeper::AccessToken.find_by(token: json_response["access_token"]) expect(access_token.tenant_name).to eq("Tenant 1") diff --git a/spec/requests/flows/client_credentials_spec.rb b/spec/requests/flows/client_credentials_spec.rb index d30765b51..430bc9247 100644 --- a/spec/requests/flows/client_credentials_spec.rb +++ b/spec/requests/flows/client_credentials_spec.rb @@ -10,7 +10,7 @@ headers = authorization client.uid, client.secret params = { grant_type: "client_credentials" } - post "/oauth/token", params: params, headers: headers + post token_endpoint_url, params: params, headers: headers expect(json_response).to match( "access_token" => Doorkeeper::AccessToken.first.token, @@ -30,7 +30,7 @@ headers = authorization client.uid, client.secret params = { grant_type: "client_credentials", scope: "write" } - post "/oauth/token", params: params, headers: headers + post token_endpoint_url, params: params, headers: headers expect(json_response).to include( "access_token" => Doorkeeper::AccessToken.first.token, @@ -43,7 +43,7 @@ headers = authorization client.uid, client.secret params = { grant_type: "client_credentials", scope: "public" } - post "/oauth/token", params: params, headers: headers + post token_endpoint_url, params: params, headers: headers expect(json_response).to include( "access_token" => Doorkeeper::AccessToken.first.token, @@ -57,7 +57,7 @@ headers = authorization client.uid, client.secret params = { grant_type: "client_credentials", scope: "random" } - post "/oauth/token", params: params, headers: headers + post token_endpoint_url, params: params, headers: headers expect(response.status).to eq(400) expect(json_response).to match( @@ -83,7 +83,7 @@ headers = authorization client.uid, client.secret params = { grant_type: "client_credentials" } - post "/oauth/token", params: params, headers: headers + post token_endpoint_url, params: params, headers: headers expect(json_response).to match( "error" => "unauthorized_client", @@ -97,7 +97,7 @@ headers = authorization client.uid, client.secret params = { grant_type: "client_credentials" } - post "/oauth/token", params: params, headers: headers + post token_endpoint_url, params: params, headers: headers expect(json_response).to match( "access_token" => Doorkeeper::AccessToken.first.token, @@ -120,7 +120,7 @@ params = { grant_type: "client_credentials" } expect do - post "/oauth/token", params: params, headers: headers + post token_endpoint_url, params: params, headers: headers end.to change { Doorkeeper::AccessToken.count }.by(1) token = Doorkeeper::AccessToken.first @@ -139,7 +139,7 @@ params = { grant_type: "client_credentials" } expect do - post "/oauth/token", params: params, headers: headers + post token_endpoint_url, params: params, headers: headers end.to change { Doorkeeper::AccessToken.count }.by(1) token = Doorkeeper::AccessToken.first @@ -157,7 +157,7 @@ headers = authorization client.uid, client.secret params = { grant_type: "client_credentials" } - post "/oauth/token", params: params, headers: headers + post token_endpoint_url, params: params, headers: headers expect(json_response).to match( "error" => "invalid_scope", @@ -171,7 +171,7 @@ headers = {} params = { grant_type: "client_credentials" } - post "/oauth/token", params: params, headers: headers + post token_endpoint_url, params: params, headers: headers expect(response.status).to eq(401) @@ -192,12 +192,12 @@ headers = authorization client.uid, client.secret params = { grant_type: "client_credentials" } - post "/oauth/token", params: params, headers: headers + post token_endpoint_url, params: params, headers: headers expect(json_response).to include("access_token" => Doorkeeper::AccessToken.first.token) token = Doorkeeper::AccessToken.first - post "/oauth/token", params: params, headers: headers + post token_endpoint_url, params: params, headers: headers expect(json_response).to include("access_token" => Doorkeeper::AccessToken.last.token) expect(token.reload).to be_revoked @@ -216,7 +216,7 @@ headers = authorization client.uid, client.secret params = { grant_type: "client_credentials" } - post "/oauth/token", params: params, headers: headers + post token_endpoint_url, params: params, headers: headers expect(json_response).to match( "error" => "invalid_token_reuse", diff --git a/spec/requests/flows/password_spec.rb b/spec/requests/flows/password_spec.rb index 3d65fd6ce..6923d9db8 100644 --- a/spec/requests/flows/password_spec.rb +++ b/spec/requests/flows/password_spec.rb @@ -12,7 +12,7 @@ context "with valid user credentials" do it "does not issue new token" do expect do - post password_token_endpoint_url(client: @client, resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(client: @client, resource_owner: @resource_owner) end.not_to(change { Doorkeeper::AccessToken.count }) end end @@ -32,7 +32,7 @@ context "with confidential client authorized using Basic auth" do it "issues a new token" do expect do - post password_token_endpoint_url( + post token_endpoint_url, params: password_token_endpoint_params( resource_owner: @resource_owner, ), headers: { "HTTP_AUTHORIZATION" => basic_auth_header_for_client(@client) } end.to(change { Doorkeeper::AccessToken.count }) @@ -64,7 +64,7 @@ @client.update(name: "sample app") expect do - post password_token_endpoint_url( + post token_endpoint_url, params: password_token_endpoint_params( client_id: @client.uid, client_secret: "foobar", resource_owner: @resource_owner, @@ -82,7 +82,7 @@ @client.update(name: "admin") expect do - post password_token_endpoint_url(client_id: @client.uid, resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(client_id: @client.uid, resource_owner: @resource_owner) end.to change { Doorkeeper::AccessToken.count }.by(1) token = Doorkeeper::AccessToken.first @@ -95,7 +95,7 @@ context "when client_secret absent" do it "issues a new token" do expect do - post password_token_endpoint_url(client_id: @client.uid, resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(client_id: @client.uid, resource_owner: @resource_owner) end.to change { Doorkeeper::AccessToken.count }.by(1) token = Doorkeeper::AccessToken.first @@ -108,7 +108,7 @@ context "when client_secret present" do it "issues a new token" do expect do - post password_token_endpoint_url(client: @client, resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(client: @client, resource_owner: @resource_owner) end.to change { Doorkeeper::AccessToken.count }.by(1) token = Doorkeeper::AccessToken.first @@ -120,7 +120,7 @@ context "when client_secret incorrect" do it "doesn't issue new token" do expect do - post password_token_endpoint_url( + post token_endpoint_url, params: password_token_endpoint_params( client_id: @client.uid, client_secret: "foobar", resource_owner: @resource_owner, @@ -140,7 +140,7 @@ context "with confidential/private client" do it "issues a new token" do expect do - post password_token_endpoint_url(client: @client, resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(client: @client, resource_owner: @resource_owner) end.to change { Doorkeeper::AccessToken.count }.by(1) token = Doorkeeper::AccessToken.first @@ -152,7 +152,7 @@ context "when client_secret absent" do it "doesn't issue new token" do expect do - post password_token_endpoint_url(client_id: @client.uid, resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(client_id: @client.uid, resource_owner: @resource_owner) end.not_to(change { Doorkeeper::AccessToken.count }) expect(response.status).to eq(401) @@ -167,7 +167,7 @@ it "issues a refresh token if enabled" do config_is_set(:refresh_token_enabled, true) - post password_token_endpoint_url(client: @client, resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(client: @client, resource_owner: @resource_owner) token = Doorkeeper::AccessToken.first expect(json_response).to include("refresh_token" => token.refresh_token) @@ -178,7 +178,7 @@ client_is_authorized(@client, @resource_owner) - post password_token_endpoint_url(client: @client, resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(client: @client, resource_owner: @resource_owner) expect(Doorkeeper::AccessToken.count).to be(1) expect(json_response).to include("access_token" => Doorkeeper::AccessToken.first.token) @@ -191,7 +191,7 @@ it "issues new token" do expect do - post password_token_endpoint_url(client: @client, resource_owner: @resource_owner, scope: "public") + post token_endpoint_url, params: password_token_endpoint_params(client: @client, resource_owner: @resource_owner, scope: "public") end.to change { Doorkeeper::AccessToken.count }.by(1) token = Doorkeeper::AccessToken.first @@ -214,7 +214,7 @@ it "issues a new token without client credentials" do expect do - post password_token_endpoint_url(resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(resource_owner: @resource_owner) end.to(change { Doorkeeper::AccessToken.count }.by(1)) token = Doorkeeper::AccessToken.first @@ -225,7 +225,7 @@ it "doesn't issue a new token with invalid client credentials in query string" do expect do - post password_token_endpoint_url( + post token_endpoint_url, params: password_token_endpoint_params( resource_owner: @resource_owner, client_id: "invalid", client_secret: "invalid", @@ -243,7 +243,7 @@ invalid_client = Doorkeeper::ClientAuthentication::Credentials.new("invalid", "invalid") expect do - post password_token_endpoint_url( + post token_endpoint_url, params: password_token_endpoint_params( resource_owner: @resource_owner, ), headers: { "HTTP_AUTHORIZATION" => basic_auth_header_for_client(invalid_client) } end.not_to(change { Doorkeeper::AccessToken.count }) @@ -264,7 +264,7 @@ it "doesn't issue a new token without client credentials" do expect do - post password_token_endpoint_url(resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(resource_owner: @resource_owner) end.not_to(change { Doorkeeper::AccessToken.count }) expect(response.status).to eq(401) @@ -284,7 +284,7 @@ it "issues new token without any scope" do expect do - post password_token_endpoint_url(client: @client, resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(client: @client, resource_owner: @resource_owner) end.to change { Doorkeeper::AccessToken.count }.by(1) token = Doorkeeper::AccessToken.first @@ -305,7 +305,7 @@ default_scopes_exist :public, :admin expect do - post password_token_endpoint_url(client: @client, resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(client: @client, resource_owner: @resource_owner) end.to change { Doorkeeper::AccessToken.count }.by(1) token = Doorkeeper::AccessToken.first @@ -321,7 +321,7 @@ default_scopes_exist :public, :read, :update expect do - post password_token_endpoint_url(client: @client, resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(client: @client, resource_owner: @resource_owner) end.to change { Doorkeeper::AccessToken.count }.by(1) token = Doorkeeper::AccessToken.first @@ -337,7 +337,7 @@ context "with invalid scopes" do it "doesn't issue new token" do expect do - post password_token_endpoint_url( + post token_endpoint_url, params: password_token_endpoint_params( client: @client, resource_owner: @resource_owner, scope: "random", @@ -346,7 +346,7 @@ end it "returns invalid_scope error" do - post password_token_endpoint_url( + post token_endpoint_url, params: password_token_endpoint_params( client: @client, resource_owner: @resource_owner, scope: "random", @@ -363,7 +363,7 @@ context "with invalid user credentials" do it "doesn't issue new token with bad password" do expect do - post password_token_endpoint_url( + post token_endpoint_url, params: password_token_endpoint_params( client: @client, resource_owner_username: @resource_owner.name, resource_owner_password: "wrongpassword", @@ -373,7 +373,7 @@ it "doesn't issue new token without credentials" do expect do - post password_token_endpoint_url(client: @client) + post token_endpoint_url, params: password_token_endpoint_params(client: @client) end.not_to(change { Doorkeeper::AccessToken.count }) end @@ -381,13 +381,13 @@ config_is_set(:resource_owner_from_credentials) { false } expect do - post password_token_endpoint_url(client: @client) + post token_endpoint_url, params: password_token_endpoint_params(client: @client) end.not_to(change { Doorkeeper::AccessToken.count }) config_is_set(:resource_owner_from_credentials) { nil } expect do - post password_token_endpoint_url(client: @client) + post token_endpoint_url, params: password_token_endpoint_params(client: @client) end.not_to(change { Doorkeeper::AccessToken.count }) end end @@ -395,7 +395,7 @@ context "with invalid confidential client credentials" do it "doesn't issue new token with bad client credentials" do expect do - post password_token_endpoint_url( + post token_endpoint_url, params: password_token_endpoint_params( client_id: @client.uid, client_secret: "bad_secret", resource_owner: @resource_owner, @@ -407,7 +407,7 @@ context "with invalid public client id" do it "doesn't issue new token with bad client id" do expect do - post password_token_endpoint_url(client_id: "bad_id", resource_owner: @resource_owner) + post token_endpoint_url, params: password_token_endpoint_params(client_id: "bad_id", resource_owner: @resource_owner) end.not_to(change { Doorkeeper::AccessToken.count }) end end diff --git a/spec/requests/flows/refresh_token_spec.rb b/spec/requests/flows/refresh_token_spec.rb index ccd883c79..e6903c1dc 100644 --- a/spec/requests/flows/refresh_token_spec.rb +++ b/spec/requests/flows/refresh_token_spec.rb @@ -22,7 +22,7 @@ end it "client gets the refresh token and refreshes it" do - post token_endpoint_url(code: @authorization.token, client: @client) + post token_endpoint_url, params: token_endpoint_params(code: @authorization.token, client: @client) token = Doorkeeper::AccessToken.first @@ -33,7 +33,10 @@ expect(@authorization.reload).to be_revoked - post refresh_token_endpoint_url(client: @client, refresh_token: token.refresh_token) + post refresh_token_endpoint_url, params: refresh_token_endpoint_params( + client: @client, + refresh_token: token.refresh_token + ) new_token = Doorkeeper::AccessToken.last expect(json_response).to include( @@ -59,7 +62,7 @@ context "when refresh_token revoked on use" do it "client requests a token with refresh token" do - post refresh_token_endpoint_url( + post refresh_token_endpoint_url, params: refresh_token_endpoint_params( client: @client, refresh_token: @token.refresh_token, ) expect(json_response).to include( @@ -70,7 +73,7 @@ it "client requests a token with expired access token" do @token.update_attribute :expires_in, -100 - post refresh_token_endpoint_url( + post refresh_token_endpoint_url, params: refresh_token_endpoint_params( client: @client, refresh_token: @token.refresh_token, ) expect(json_response).to include( @@ -86,7 +89,7 @@ end it "client request a token with refresh token" do - post refresh_token_endpoint_url( + post refresh_token_endpoint_url, params: refresh_token_endpoint_params( client: @client, refresh_token: @token.refresh_token, ) expect(json_response).to include( @@ -98,7 +101,7 @@ it "client request a token with expired access token" do @token.update_attribute :expires_in, -100 # This method is broken: - post refresh_token_endpoint_url( + post refresh_token_endpoint_url, params: refresh_token_endpoint_params( client: @client, refresh_token: @token.refresh_token, ) expect(json_response).to include( @@ -137,7 +140,7 @@ end it "issues a new token without client_secret when refresh token was issued to a public client" do - post refresh_token_endpoint_url( + post refresh_token_endpoint_url, params: refresh_token_endpoint_params( client_id: public_client.uid, refresh_token: token_for_public_client.refresh_token, ) @@ -150,13 +153,13 @@ end it "returns an error without credentials" do - post refresh_token_endpoint_url(refresh_token: token_for_private_client.refresh_token) + post refresh_token_endpoint_url, params: refresh_token_endpoint_params(refresh_token: token_for_private_client.refresh_token) expect(json_response).to include("error" => "invalid_grant") end it "returns an error with wrong credentials" do - post refresh_token_endpoint_url( + post refresh_token_endpoint_url, params: refresh_token_endpoint_params( client_id: "1", client_secret: "1", refresh_token: token_for_private_client.refresh_token, @@ -169,7 +172,7 @@ end it "client gets an error for invalid refresh token" do - post refresh_token_endpoint_url(client: @client, refresh_token: "invalid") + post refresh_token_endpoint_url, params: refresh_token_endpoint_params(client: @client, refresh_token: "invalid") expect(json_response).to match( "error" => "invalid_grant", @@ -179,7 +182,7 @@ it "client gets an error for revoked access token" do @token.revoke - post refresh_token_endpoint_url(client: @client, refresh_token: @token.refresh_token) + post refresh_token_endpoint_url, params: refresh_token_endpoint_params(client: @client, refresh_token: @token.refresh_token) expect(json_response).to match( "error" => "invalid_grant", @@ -189,7 +192,7 @@ it "second of simultaneous client requests get an error for revoked access token" do allow_any_instance_of(Doorkeeper::AccessToken).to receive(:revoked?).and_return(false, true) - post refresh_token_endpoint_url(client: @client, refresh_token: @token.refresh_token) + post refresh_token_endpoint_url, params: refresh_token_endpoint_params(client: @client, refresh_token: @token.refresh_token) expect(json_response).to match( "error" => "invalid_grant", @@ -206,7 +209,7 @@ User.authenticate! params[:username], params[:password] end create_resource_owner - _another_token = post password_token_endpoint_url( + _another_token = post token_endpoint_url, params: password_token_endpoint_params( client: @client, resource_owner: resource_owner, ) last_token.update(created_at: 5.seconds.ago) @@ -223,7 +226,7 @@ context "when refresh_token revoked on use" do it "client request a token after creating another token with the same user" do - post refresh_token_endpoint_url( + post refresh_token_endpoint_url, params: refresh_token_endpoint_params( client: @client, refresh_token: @token.refresh_token, ) @@ -238,7 +241,7 @@ end it "client request a token after creating another token with the same user" do - post refresh_token_endpoint_url( + post refresh_token_endpoint_url, params: refresh_token_endpoint_params( client: @client, refresh_token: @token.refresh_token, ) @@ -265,7 +268,7 @@ end it "copies custom attributes from the previous token into the new token" do - post refresh_token_endpoint_url( + post refresh_token_endpoint_url, params: refresh_token_endpoint_params( client: @client, refresh_token: @token.refresh_token, ) diff --git a/spec/support/helpers/request_spec_helper.rb b/spec/support/helpers/request_spec_helper.rb index 0b5769123..ef458b770 100644 --- a/spec/support/helpers/request_spec_helper.rb +++ b/spec/support/helpers/request_spec_helper.rb @@ -63,7 +63,7 @@ def sign_in end def create_access_token(authorization_code, client, code_verifier = nil) - page.driver.post token_endpoint_url(code: authorization_code, client: client, code_verifier: code_verifier) + page.driver.post token_endpoint_url, token_endpoint_params(code: authorization_code, client: client, code_verifier: code_verifier) end def i_should_see_translated_error_message(key) diff --git a/spec/support/helpers/url_helper.rb b/spec/support/helpers/url_helper.rb index 5c60207c1..7a77e7525 100644 --- a/spec/support/helpers/url_helper.rb +++ b/spec/support/helpers/url_helper.rb @@ -2,8 +2,12 @@ module UrlHelper # FIXME - def token_endpoint_url(options = {}) - parameters = { + def token_endpoint_url + "/oauth/token" + end + + def token_endpoint_params(options = {}) + { code: options[:code], client_id: options[:client_id] || options[:client].try(:uid), client_secret: options[:client_secret] || options[:client].try(:secret), @@ -12,11 +16,10 @@ def token_endpoint_url(options = {}) code_verifier: options[:code_verifier], code_challenge_method: options[:code_challenge_method], }.reject { |_, v| v.blank? } - "/oauth/token?#{build_query(parameters)}" end - - def password_token_endpoint_url(options = {}) - parameters = { + + def password_token_endpoint_params(options = {}) + { code: options[:code], client_id: options[:client_id] || options[:client].try(:uid), client_secret: options[:client_secret] || options[:client].try(:secret), @@ -25,7 +28,6 @@ def password_token_endpoint_url(options = {}) scope: options[:scope], grant_type: "password", }.reject { |_, v| v.blank? } - "/oauth/token?#{build_query(parameters)}" end def authorization_endpoint_url(options = {}) @@ -42,14 +44,17 @@ def authorization_endpoint_url(options = {}) "/oauth/authorize?#{build_query(parameters)}" end - def refresh_token_endpoint_url(options = {}) - parameters = { + def refresh_token_endpoint_url + "/oauth/token" + end + + def refresh_token_endpoint_params(options = {}) + { refresh_token: options[:refresh_token], client_id: options[:client_id] || options[:client].try(:uid), client_secret: options[:client_secret] || options[:client].try(:secret), grant_type: options[:grant_type] || "refresh_token", }.reject { |_, v| v.blank? } - "/oauth/token?#{build_query(parameters)}" end def revocation_token_endpoint_url From 42a9d103c3751e7be67b643df279b5ff8dd9b383 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 01:35:50 +0200 Subject: [PATCH 05/16] Fix token introspection, fallback mechanism should return nil --- lib/doorkeeper/client_authentication/fallback_mechanism.rb | 2 +- lib/doorkeeper/oauth/token_introspection.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/doorkeeper/client_authentication/fallback_mechanism.rb b/lib/doorkeeper/client_authentication/fallback_mechanism.rb index a116829ab..af3620f30 100644 --- a/lib/doorkeeper/client_authentication/fallback_mechanism.rb +++ b/lib/doorkeeper/client_authentication/fallback_mechanism.rb @@ -4,7 +4,7 @@ module Doorkeeper module ClientAuthentication class FallbackMechanism def authenticate(request) - Doorkeeper::ClientAuthentication::Credentials.new(nil, nil) + nil end end end diff --git a/lib/doorkeeper/oauth/token_introspection.rb b/lib/doorkeeper/oauth/token_introspection.rb index 155563735..4a88118fc 100644 --- a/lib/doorkeeper/oauth/token_introspection.rb +++ b/lib/doorkeeper/oauth/token_introspection.rb @@ -57,7 +57,7 @@ def to_json(*) # def authorize! # Requested client authorization - if server.credentials.present? + if server.credentials authorize_using_basic_auth! elsif authorized_token authorize_using_bearer_token! From fafad381562e72c6fefbe010c6b5585bf3ae9d0d Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 03:45:16 +0200 Subject: [PATCH 06/16] Deprecate old client_credentials option --- lib/doorkeeper/config.rb | 30 +++++++++++++++--------------- lib/doorkeeper/config/option.rb | 2 +- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index ab2afbb72..3de6b0cd0 100644 --- a/lib/doorkeeper/config.rb +++ b/lib/doorkeeper/config.rb @@ -64,16 +64,6 @@ def scopes_by_grant_type(hash = {}) @config.instance_variable_set(:@scopes_by_grant_type, hash) end - # Change the way client credentials are retrieved from the request object. - # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then - # falls back to the `:client_id` and `:client_secret` params from the - # `params` object. - # - # @param methods [Array] Define client credentials - def client_credentials(*methods) - @config.instance_variable_set(:@client_credentials_methods, methods) - end - # Change the way access token is authenticated from the request object. # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then # falls back to the `:access_token` or `:bearer_token` params from the @@ -257,6 +247,21 @@ def configure_secrets_for(type, using:, fallback:) option :pkce_code_challenge_methods, default: %w[plain S256] option :handle_auth_errors, default: :render option :token_lookup_batch_size, default: 10_000 + + # Change the way client credentials are retrieved from the request object. + # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then + # falls back to the `:client_id` and `:client_secret` params from the + # `params` object. + # + # @param methods [Array] Define client credentials + # @deprecated + option :client_credentials, array: true, + default: %i[from_basic from_params], + as: :client_credentials_methods, + deprecated: { + message: "The client_credentials option has been deprecated in favor of client_authentication, as to not confuse with client_credentials grant flow" + } + # Sets the token_reuse_limit # It will be used only when reuse_access_token option in enabled # By default it will be 100 @@ -580,11 +585,6 @@ def pkce_code_challenge_methods_supported pkce_code_challenge_methods end - # Deprecated? - def client_credentials_methods - @client_credentials_methods ||= %i[from_basic from_params] - end - def client_authentication_mechanisms @client_authentication_mechanisms ||= client_authentication.map do |name| Doorkeeper::ClientAuthentication.get(name) diff --git a/lib/doorkeeper/config/option.rb b/lib/doorkeeper/config/option.rb index c053f9a85..04ed87659 100644 --- a/lib/doorkeeper/config/option.rb +++ b/lib/doorkeeper/config/option.rb @@ -53,7 +53,7 @@ def option(name, options = {}) value = if attribute_builder attribute_builder.new(&block).build else - block || args.first + block || (options[:array] == true ? args : args.first) end @config.instance_variable_set(:"@#{attribute}", value) From 159544452c70a10f616993d40a5cd9a7c7fdfb64 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 03:45:41 +0200 Subject: [PATCH 07/16] Start adding tests for client authentication methods --- .rspec | 2 +- .../client_secret_basic_spec.rb | 6 ++ .../client_secret_post_spec.rb | 6 ++ .../oauth/client_authentication/none_spec.rb | 59 +++++++++++++++++++ 4 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 spec/lib/oauth/client_authentication/client_secret_basic_spec.rb create mode 100644 spec/lib/oauth/client_authentication/client_secret_post_spec.rb create mode 100644 spec/lib/oauth/client_authentication/none_spec.rb diff --git a/.rspec b/.rspec index 53607ea52..660778bdc 100644 --- a/.rspec +++ b/.rspec @@ -1 +1 @@ ---colour +--colour --format documentation diff --git a/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb b/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb new file mode 100644 index 000000000..bc7850d32 --- /dev/null +++ b/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Doorkeeper::OAuth::ClientAuthentication::ClientSecretBasic do +end diff --git a/spec/lib/oauth/client_authentication/client_secret_post_spec.rb b/spec/lib/oauth/client_authentication/client_secret_post_spec.rb new file mode 100644 index 000000000..d981fef1f --- /dev/null +++ b/spec/lib/oauth/client_authentication/client_secret_post_spec.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Doorkeeper::OAuth::ClientAuthentication::ClientSecretPost do +end diff --git a/spec/lib/oauth/client_authentication/none_spec.rb b/spec/lib/oauth/client_authentication/none_spec.rb new file mode 100644 index 000000000..7c6cece6f --- /dev/null +++ b/spec/lib/oauth/client_authentication/none_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Doorkeeper::OAuth::ClientAuthentication::None do + describe 'matches_request?' do + it "matches if the request doesn't have authorization or client_secret" do + request = mock_request({ + client_id: '1234' + }) + + expect(described_class.matches_request?(request)).to be true + end + + it "doesn't match if the request has client_secret" do + request = mock_request({ + client_id: '1234', + client_secret: "5678" + }) + + expect(described_class.matches_request?(request)).to_not be true + end + + it "doesn't match if the request has authorization" do + request = mock_request({ + client_id: '1234' + }, ActionController::HttpAuthentication::Basic.encode_credentials('username', 'password')) + + binding.break + expect(described_class.matches_request?(request)).to_not be true + end + + private + + # I'm not sure if there's a better way to get a mock rack request for + # testing. Here we don't need a full request spec, but we do need enough to + # check that the logic of these classes works. + def mock_request(params, credentials = nil) + request = ActionDispatch::Request.new({ + "REQUEST_METHOD"=>"POST", + "SERVER_NAME"=>"example.org", + "SERVER_PORT"=>"80", + "SERVER_PROTOCOL"=>"HTTP/1.1", + "rack.url_scheme"=>"http", + "HTTP_HOST"=> "example.org", + "ORIGINAL_FULLPATH" => "/test", + "action_dispatch.remote_ip" => "127.0.0.1", + "action_dispatch.request.query_parameters" => {}, + "action_dispatch.request.request_parameters" => params + }) + + unless credentials.nil? + request.env["HTTP_AUTHORIZATION"] = credentials + end + + request + end + end +end From 5ed4bc4647a7ebd4442aff7ae2800570a8423b72 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 20:01:43 +0200 Subject: [PATCH 08/16] Add tests and migration strategy for client_authentication vs client_credentials --- lib/doorkeeper/config.rb | 68 ++++++++++---- lib/doorkeeper/config/option.rb | 2 +- lib/doorkeeper/config/validations.rb | 11 +++ lib/doorkeeper/request.rb | 2 +- spec/lib/config_spec.rb | 94 +++++++++++++++++-- .../oauth/client_authentication/none_spec.rb | 1 - 6 files changed, 152 insertions(+), 26 deletions(-) diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index 3de6b0cd0..0258b1318 100644 --- a/lib/doorkeeper/config.rb +++ b/lib/doorkeeper/config.rb @@ -64,6 +64,34 @@ def scopes_by_grant_type(hash = {}) @config.instance_variable_set(:@scopes_by_grant_type, hash) end + # Change the way client credentials are retrieved from the request object. + # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then + # falls back to the `:client_id` and `:client_secret` params from the + # `params` object. + # + # @param methods [Array] Define client credentials + # @deprecated + def client_credentials(*methods) + deprecated("client_credentials", "Use the client_authentication option instead. Automatically converting to client_authentication") + + client_authentication = methods.map {|method| + case method + when :from_basic + :client_secret_basic + when :from_params + :client_secret_post + else + Kernel.warn("[DOORKEEPER] Unknown client_credentials method detected: #{method}") + end + }.reject(&:nil?) + + if client_authentication.empty? + Kernel.warn("[DOORKEEPER] No known client_credentials method detected, ignoring option") + else + @config.instance_variable_set(:@client_credentials_methods, client_authentication.concat([:none])) + end + end + # Change the way access token is authenticated from the request object. # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then # falls back to the `:access_token` or `:bearer_token` params from the @@ -176,6 +204,13 @@ def hash_application_secrets(using: nil, fallback: nil) private + def deprecated(name, message=nil) + warning = "[DOORKEEPER] #{name} has been deprecated and will soon be removed" + warning = "#{warning}\n#{message}" if message.present? + + Kernel.warn(warning) + end + # Configure the secret storing functionality def configure_secrets_for(type, using:, fallback:) raise ArgumentError, "Invalid type #{type}" if %i[application token].exclude?(type) @@ -243,25 +278,11 @@ def configure_secrets_for(type, using:, fallback:) option :orm, default: :active_record option :native_redirect_uri, default: "urn:ietf:wg:oauth:2.0:oob", deprecated: true option :grant_flows, default: %w[authorization_code client_credentials] - option :client_authentication, default: %w[client_secret_basic client_secret_post none] + option :client_authentication, default: %i[client_secret_basic client_secret_post none] option :pkce_code_challenge_methods, default: %w[plain S256] option :handle_auth_errors, default: :render option :token_lookup_batch_size, default: 10_000 - # Change the way client credentials are retrieved from the request object. - # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then - # falls back to the `:client_id` and `:client_secret` params from the - # `params` object. - # - # @param methods [Array] Define client credentials - # @deprecated - option :client_credentials, array: true, - default: %i[from_basic from_params], - as: :client_credentials_methods, - deprecated: { - message: "The client_credentials option has been deprecated in favor of client_authentication, as to not confuse with client_credentials grant flow" - } - # Sets the token_reuse_limit # It will be used only when reuse_access_token option in enabled # By default it will be 100 @@ -585,8 +606,21 @@ def pkce_code_challenge_methods_supported pkce_code_challenge_methods end - def client_authentication_mechanisms - @client_authentication_mechanisms ||= client_authentication.map do |name| + def client_authentication_methods + return @client_authentication_methods if defined?(@client_authentication_methods) + + methods = if instance_variable_defined?("@client_credentials_methods") + if instance_variable_defined?("@client_authentication") + Kernel.warn("[DOORKEEPER] Both client_credentials and client_authentication are set, using client_authentication") + client_authentication + else + instance_variable_get("@client_credentials_methods") + end + else + client_authentication + end + + @client_authentication_methods = methods.map do |name| Doorkeeper::ClientAuthentication.get(name) end.compact end diff --git a/lib/doorkeeper/config/option.rb b/lib/doorkeeper/config/option.rb index 04ed87659..c053f9a85 100644 --- a/lib/doorkeeper/config/option.rb +++ b/lib/doorkeeper/config/option.rb @@ -53,7 +53,7 @@ def option(name, options = {}) value = if attribute_builder attribute_builder.new(&block).build else - block || (options[:array] == true ? args : args.first) + block || args.first end @config.instance_variable_set(:"@#{attribute}", value) diff --git a/lib/doorkeeper/config/validations.rb b/lib/doorkeeper/config/validations.rb index d7ee68659..2dde66868 100644 --- a/lib/doorkeeper/config/validations.rb +++ b/lib/doorkeeper/config/validations.rb @@ -8,6 +8,7 @@ module Validations # Validates configuration options to be set properly. # def validate! + validate_client_authentication_value validate_reuse_access_token_value validate_token_reuse_limit validate_secret_strategies @@ -16,6 +17,16 @@ def validate! private + def validate_client_authentication_value + return if client_authentication.is_a?(Array) + + ::Rails.logger.warn( + "[DOORKEEPER] You have configured client_authentication as a non-array value. Using default value" + ) + + @client_authentication = [:client_secret_basic, :client_secret_post, :none] + end + # Determine whether +reuse_access_token+ and a non-restorable # +token_secret_strategy+ have both been activated. # diff --git a/lib/doorkeeper/request.rb b/lib/doorkeeper/request.rb index 94f55be6a..bf83fdcde 100644 --- a/lib/doorkeeper/request.rb +++ b/lib/doorkeeper/request.rb @@ -53,7 +53,7 @@ def token_strategy(grant_type) private def client_authentication_strategies - Doorkeeper.configuration.client_authentication_mechanisms + Doorkeeper.configuration.client_authentication_methods end def authorization_flows diff --git a/spec/lib/config_spec.rb b/spec/lib/config_spec.rb index 83970ec89..2ea7322bb 100644 --- a/spec/lib/config_spec.rb +++ b/spec/lib/config_spec.rb @@ -279,19 +279,101 @@ end describe "client_credentials" do - it "has defaults order" do - expect(config.client_credentials_methods) - .to eq(%i[from_basic from_params]) + it "prints a deprecation warning message" do + expect(Kernel).to receive(:warn).with( + /\[DOORKEEPER\] client_credentials has been deprecated and will soon be removed/, + ) + + Doorkeeper.configure do + orm DOORKEEPER_ORM + client_credentials :from_params + end + end + + it "prints a warning message when passed an unknown client_credentials method" do + expect(Kernel).to receive(:warn).with(/\[DOORKEEPER\] client_credentials has been deprecated/).once + expect(Kernel).to receive(:warn).with( + /\[DOORKEEPER\] Unknown client_credentials method detected: from_digest/, + ).once + expect(Kernel).to receive(:warn).with( + /\[DOORKEEPER\] No known client_credentials method detected, ignoring option/ + ).once + + Doorkeeper.configure do + orm DOORKEEPER_ORM + client_credentials :from_digest + end + + expect(config.instance_variable_defined?("@client_credentials_methods")).to be false + end + + it "can change the value, but converts to client_authentication compatible values" do + Doorkeeper.configure do + orm DOORKEEPER_ORM + client_credentials :from_params + end + + expect(config.instance_variable_get("@client_credentials_methods")) + .to contain_exactly(:client_secret_post, :none) + end + end + + describe "client_authentication" do + it "has a default" do + expect(config.client_authentication).to contain_exactly(:client_secret_basic, :client_secret_post, :none) + end + + it "warns if configured to a non-array value & sets to default" do + # Not sure how to capture this expectation? + # + # expect(::Rails.logger).to receive(:error).with( + # /\[DOORKEEPER\] You have configured client_authentication as a non-array value/ + # ) + + Doorkeeper.configure do + orm DOORKEEPER_ORM + # :client_secret_post would get silently ignored + client_authentication :client_secret_basic, :client_secret_post + end + + expect(config.client_authentication).to contain_exactly(:client_secret_basic, :client_secret_post, :none) end it "can change the value" do Doorkeeper.configure do orm DOORKEEPER_ORM - client_credentials :from_digest, :from_params + client_authentication [:client_secret_basic] + end + + expect(config.client_authentication).to contain_exactly(:client_secret_basic) + end + end + + describe "client_authentication_methods" do + it "provides a warning if both client_credentials and client_authentication are set" do + expect(Kernel).to receive(:warn).with(/\[DOORKEEPER\] client_credentials has been deprecated/) + + Doorkeeper.configure do + orm DOORKEEPER_ORM + client_credentials :from_params + client_authentication [:client_secret_basic] + end + + expect(Kernel).to receive(:warn).with( + /\[DOORKEEPER\] Both client_credentials and client_authentication are set, using client_authentication/, + ) + + expect(config.client_authentication_methods.map(&:name)).to contain_exactly(:client_secret_basic) + end + + it "uses client_authentication if both client_credentials and client_authentication are set" do + Doorkeeper.configure do + orm DOORKEEPER_ORM + client_credentials :from_params + client_authentication [:client_secret_basic] end - expect(config.client_credentials_methods) - .to eq(%i[from_digest from_params]) + expect(config.client_authentication_methods.map(&:name)).to contain_exactly(:client_secret_basic) end end diff --git a/spec/lib/oauth/client_authentication/none_spec.rb b/spec/lib/oauth/client_authentication/none_spec.rb index 7c6cece6f..669d0054e 100644 --- a/spec/lib/oauth/client_authentication/none_spec.rb +++ b/spec/lib/oauth/client_authentication/none_spec.rb @@ -26,7 +26,6 @@ client_id: '1234' }, ActionController::HttpAuthentication::Basic.encode_credentials('username', 'password')) - binding.break expect(described_class.matches_request?(request)).to_not be true end From 4e37539a4f5136518bb593564a2e488681b0033c Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 20:15:10 +0200 Subject: [PATCH 09/16] Add more tests for client authentication methods --- .../client_secret_basic_spec.rb | 13 +++++++++ .../client_secret_post_spec.rb | 18 +++++++++++++ .../oauth/client_authentication/none_spec.rb | 26 ------------------ spec/support/helpers/request_mock_helper.rb | 27 +++++++++++++++++++ 4 files changed, 58 insertions(+), 26 deletions(-) create mode 100644 spec/support/helpers/request_mock_helper.rb diff --git a/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb b/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb index bc7850d32..a6fb84467 100644 --- a/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb +++ b/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb @@ -3,4 +3,17 @@ require "spec_helper" RSpec.describe Doorkeeper::OAuth::ClientAuthentication::ClientSecretBasic do + describe 'matches_request?' do + it "matches if the request has basic authorization" do + request = mock_request({}, ActionController::HttpAuthentication::Basic.encode_credentials('username', 'password')) + + expect(described_class.matches_request?(request)).to be true + end + + it "doesn't match if the request has bearer authorization" do + request = mock_request({}, "Bearer foobar") + + expect(described_class.matches_request?(request)).to_not be true + end + end end diff --git a/spec/lib/oauth/client_authentication/client_secret_post_spec.rb b/spec/lib/oauth/client_authentication/client_secret_post_spec.rb index d981fef1f..ac382fa54 100644 --- a/spec/lib/oauth/client_authentication/client_secret_post_spec.rb +++ b/spec/lib/oauth/client_authentication/client_secret_post_spec.rb @@ -3,4 +3,22 @@ require "spec_helper" RSpec.describe Doorkeeper::OAuth::ClientAuthentication::ClientSecretPost do + describe 'matches_request?' do + it "matches if the request doesn't have authorization" do + request = mock_request({ + client_id: '1234', + client_secret: '5678' + }) + + expect(described_class.matches_request?(request)).to be true + end + + it "doesn't match if the request is missing client_secret" do + request = mock_request({ + client_id: '1234' + }) + + expect(described_class.matches_request?(request)).to_not be true + end + end end diff --git a/spec/lib/oauth/client_authentication/none_spec.rb b/spec/lib/oauth/client_authentication/none_spec.rb index 669d0054e..3896daed5 100644 --- a/spec/lib/oauth/client_authentication/none_spec.rb +++ b/spec/lib/oauth/client_authentication/none_spec.rb @@ -28,31 +28,5 @@ expect(described_class.matches_request?(request)).to_not be true end - - private - - # I'm not sure if there's a better way to get a mock rack request for - # testing. Here we don't need a full request spec, but we do need enough to - # check that the logic of these classes works. - def mock_request(params, credentials = nil) - request = ActionDispatch::Request.new({ - "REQUEST_METHOD"=>"POST", - "SERVER_NAME"=>"example.org", - "SERVER_PORT"=>"80", - "SERVER_PROTOCOL"=>"HTTP/1.1", - "rack.url_scheme"=>"http", - "HTTP_HOST"=> "example.org", - "ORIGINAL_FULLPATH" => "/test", - "action_dispatch.remote_ip" => "127.0.0.1", - "action_dispatch.request.query_parameters" => {}, - "action_dispatch.request.request_parameters" => params - }) - - unless credentials.nil? - request.env["HTTP_AUTHORIZATION"] = credentials - end - - request - end end end diff --git a/spec/support/helpers/request_mock_helper.rb b/spec/support/helpers/request_mock_helper.rb new file mode 100644 index 000000000..f55f0cf3d --- /dev/null +++ b/spec/support/helpers/request_mock_helper.rb @@ -0,0 +1,27 @@ +module RequestMockHelper + # I'm not sure if there's a better way to get a mock rack request for + # testing. Here we don't need a full request spec, but we do need enough to + # check that the logic of these classes works. + def mock_request(params, credentials = nil) + request = ActionDispatch::Request.new({ + "REQUEST_METHOD"=>"POST", + "SERVER_NAME"=>"example.org", + "SERVER_PORT"=>"80", + "SERVER_PROTOCOL"=>"HTTP/1.1", + "rack.url_scheme"=>"http", + "HTTP_HOST"=> "example.org", + "ORIGINAL_FULLPATH" => "/test", + "action_dispatch.remote_ip" => "127.0.0.1", + "action_dispatch.request.query_parameters" => {}, + "action_dispatch.request.request_parameters" => params + }) + + unless credentials.nil? + request.env["HTTP_AUTHORIZATION"] = credentials + end + + request + end +end + +RSpec.configuration.send :include, RequestMockHelper From f2fe3f910c5b5e22eb0124ecee3cbf353dcdc675 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 20:26:19 +0200 Subject: [PATCH 10/16] More improvements to client authentication, renaming mechanism to method --- lib/doorkeeper.rb | 4 ---- lib/doorkeeper/client_authentication.rb | 11 ++++----- ...llback_mechanism.rb => fallback_method.rb} | 8 +++++-- .../client_authentication/mechanism.rb | 23 ------------------- .../client_authentication/method.rb | 18 +++++++++++++++ .../client_authentication/registry.rb | 20 ++++++++-------- .../client_secret_basic.rb | 2 +- .../client_secret_post.rb | 2 +- .../oauth/client_authentication/none.rb | 2 +- lib/doorkeeper/request.rb | 13 +++++++---- lib/doorkeeper/server.rb | 7 +++--- 11 files changed, 53 insertions(+), 57 deletions(-) rename lib/doorkeeper/client_authentication/{fallback_mechanism.rb => fallback_method.rb} (50%) delete mode 100644 lib/doorkeeper/client_authentication/mechanism.rb create mode 100644 lib/doorkeeper/client_authentication/method.rb diff --git a/lib/doorkeeper.rb b/lib/doorkeeper.rb index a67ec6d8d..34bae57cc 100644 --- a/lib/doorkeeper.rb +++ b/lib/doorkeeper.rb @@ -75,10 +75,6 @@ module Authorization autoload :URIBuilder, "doorkeeper/oauth/authorization/uri_builder" end - # class Client - # autoload :Credentials, "doorkeeper/oauth/client/credentials" - # end - module ClientCredentials autoload :Validator, "doorkeeper/oauth/client_credentials/validator" autoload :Creator, "doorkeeper/oauth/client_credentials/creator" diff --git a/lib/doorkeeper/client_authentication.rb b/lib/doorkeeper/client_authentication.rb index 539f264e8..30fa4ca88 100644 --- a/lib/doorkeeper/client_authentication.rb +++ b/lib/doorkeeper/client_authentication.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true require "doorkeeper/client_authentication/credentials" -require "doorkeeper/client_authentication/fallback_mechanism" -require "doorkeeper/client_authentication/mechanism" +require "doorkeeper/client_authentication/fallback_method" +require "doorkeeper/client_authentication/method" require "doorkeeper/client_authentication/registry" module Doorkeeper @@ -11,18 +11,17 @@ module ClientAuthentication register( :none, - mechanism: Doorkeeper::OAuth::ClientAuthentication::None, - authenticates_client: false + method: Doorkeeper::OAuth::ClientAuthentication::None, ) register( :client_secret_post, - mechanism: Doorkeeper::OAuth::ClientAuthentication::ClientSecretPost, + method: Doorkeeper::OAuth::ClientAuthentication::ClientSecretPost, ) register( :client_secret_basic, - mechanism: Doorkeeper::OAuth::ClientAuthentication::ClientSecretBasic, + method: Doorkeeper::OAuth::ClientAuthentication::ClientSecretBasic, ) end end diff --git a/lib/doorkeeper/client_authentication/fallback_mechanism.rb b/lib/doorkeeper/client_authentication/fallback_method.rb similarity index 50% rename from lib/doorkeeper/client_authentication/fallback_mechanism.rb rename to lib/doorkeeper/client_authentication/fallback_method.rb index af3620f30..1a29d4049 100644 --- a/lib/doorkeeper/client_authentication/fallback_mechanism.rb +++ b/lib/doorkeeper/client_authentication/fallback_method.rb @@ -2,8 +2,12 @@ module Doorkeeper module ClientAuthentication - class FallbackMechanism - def authenticate(request) + class FallbackMethod + def self.matches_request? + true + end + + def self.authenticate(request) nil end end diff --git a/lib/doorkeeper/client_authentication/mechanism.rb b/lib/doorkeeper/client_authentication/mechanism.rb deleted file mode 100644 index 980f8c4f3..000000000 --- a/lib/doorkeeper/client_authentication/mechanism.rb +++ /dev/null @@ -1,23 +0,0 @@ -# frozen_string_literal: true - -module Doorkeeper - module ClientAuthentication - class Mechanism - attr_reader :name, :mechanism - - def initialize(name, **options) - @name = name - @mechanism = options[:mechanism] - @authenticates_client = options.key?(:authenticates_client) ? options[:authenticates_client] : true - end - - def authenticates_client? - !!@authenticates_client - end - - def matches_request?(request) - @mechanism.matches_request?(request) - end - end - end -end diff --git a/lib/doorkeeper/client_authentication/method.rb b/lib/doorkeeper/client_authentication/method.rb new file mode 100644 index 000000000..9816e11f1 --- /dev/null +++ b/lib/doorkeeper/client_authentication/method.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Doorkeeper + module ClientAuthentication + class Method + attr_reader :name, :method + + def initialize(name, **options) + @name = name + @method = options[:method] + end + + def matches_request?(request) + @method.matches_request?(request) + end + end + end +end diff --git a/lib/doorkeeper/client_authentication/registry.rb b/lib/doorkeeper/client_authentication/registry.rb index be8f83280..03027bc5b 100644 --- a/lib/doorkeeper/client_authentication/registry.rb +++ b/lib/doorkeeper/client_authentication/registry.rb @@ -3,32 +3,32 @@ module Doorkeeper module ClientAuthentication module Registry - mattr_accessor :mechanisms - self.mechanisms = {} + mattr_accessor :methods + self.methods = {} - # Allows to register custom OAuth client authentication mechanism so that + # Allows to register custom OAuth client authentication method so that # Doorkeeper could recognize and process it. # - def register(name_or_mechanism, **options) - unless name_or_mechanism.is_a?(Doorkeeper::ClientAuthentication::Mechanism) - name_or_mechanism = Doorkeeper::ClientAuthentication::Mechanism.new(name_or_mechanism, **options) + def register(name_or_method, **options) + unless name_or_method.is_a?(Doorkeeper::ClientAuthentication::Method) + name_or_method = Doorkeeper::ClientAuthentication::Method.new(name_or_method, **options) end - name_key = name_or_mechanism.name.to_sym + name_key = name_or_method.name.to_sym - if mechanisms.key?(name_key) + if methods.key?(name_key) ::Kernel.warn <<~WARNING [DOORKEEPER] '#{name_key}' client authentication strategy is already registered and will be overridden in #{caller(1..1).first} WARNING end - mechanisms[name_key] = name_or_mechanism + methods[name_key] = name_or_method end # [NOTE]: make it to use #fetch after removing fallbacks def get(name) - mechanisms[name.to_sym] + methods[name.to_sym] end end end diff --git a/lib/doorkeeper/oauth/client_authentication/client_secret_basic.rb b/lib/doorkeeper/oauth/client_authentication/client_secret_basic.rb index a85b4eff0..9c51ba95e 100644 --- a/lib/doorkeeper/oauth/client_authentication/client_secret_basic.rb +++ b/lib/doorkeeper/oauth/client_authentication/client_secret_basic.rb @@ -8,7 +8,7 @@ def self.matches_request?(request) request.authorization.present? && request.authorization.downcase.start_with?('basic') end - def authenticate(request) + def self.authenticate(request) value = request.authorization.to_s.split(" ", 2).second client_id, client_secret = Base64.decode64(value).split(':', 2) diff --git a/lib/doorkeeper/oauth/client_authentication/client_secret_post.rb b/lib/doorkeeper/oauth/client_authentication/client_secret_post.rb index 28cdc577b..5e4302e86 100644 --- a/lib/doorkeeper/oauth/client_authentication/client_secret_post.rb +++ b/lib/doorkeeper/oauth/client_authentication/client_secret_post.rb @@ -8,7 +8,7 @@ def self.matches_request?(request) request.method.upcase === "POST" && request.request_parameters[:client_id].present? && request.request_parameters[:client_secret].present? end - def authenticate(request) + def self.authenticate(request) client_id = request.request_parameters[:client_id] client_secret = request.request_parameters[:client_secret] diff --git a/lib/doorkeeper/oauth/client_authentication/none.rb b/lib/doorkeeper/oauth/client_authentication/none.rb index 16f65059f..97e2d51a9 100644 --- a/lib/doorkeeper/oauth/client_authentication/none.rb +++ b/lib/doorkeeper/oauth/client_authentication/none.rb @@ -8,7 +8,7 @@ def self.matches_request?(request) !request.authorization && request.request_parameters[:client_id] && !request.request_parameters[:client_secret] end - def authenticate(request) + def self.authenticate(request) Doorkeeper::ClientAuthentication::Credentials.new( request.request_parameters[:client_id], nil ) diff --git a/lib/doorkeeper/request.rb b/lib/doorkeeper/request.rb index bf83fdcde..c0265ddc8 100644 --- a/lib/doorkeeper/request.rb +++ b/lib/doorkeeper/request.rb @@ -3,15 +3,18 @@ module Doorkeeper module Request class << self - def client_authentication_mechanism(request) - strategy = client_authentication_strategies.detect do |strategy| + def client_authentication_method(request) + # TODO: Should we support theoretically more than one method matching a + # request and then check each for authentication? Currently we only + # check the first that matches the request + strategy = client_authentication_methods.detect do |strategy| strategy.matches_request?(request) end if strategy - strategy.mechanism + strategy.method else - Doorkeeper::ClientAuthentication::FallbackMechanism + Doorkeeper::ClientAuthentication::FallbackMethod end end @@ -52,7 +55,7 @@ def token_strategy(grant_type) private - def client_authentication_strategies + def client_authentication_methods Doorkeeper.configuration.client_authentication_methods end diff --git a/lib/doorkeeper/server.rb b/lib/doorkeeper/server.rb index f7f355278..76cf16d78 100644 --- a/lib/doorkeeper/server.rb +++ b/lib/doorkeeper/server.rb @@ -8,9 +8,8 @@ def initialize(context) @context = context end - def client_authentication_request - klass = Request.client_authentication_mechanism(context.request) - klass.new + def client_authentication_method_for_request + Request.client_authentication_method(context.request) end def authorization_request(strategy) @@ -42,7 +41,7 @@ def resource_owner end def credentials - @credentials ||= client_authentication_request.authenticate(context.request) + @credentials ||= client_authentication_method_for_request.authenticate(context.request) end end end From 8f47f71e2522b874782a4bf3888682f1b7b65cb5 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 20:38:58 +0200 Subject: [PATCH 11/16] Add more tests for client authentication methods --- .../client_authentication/fallback_method.rb | 2 +- .../client_authentication/method.rb | 7 ++--- .../fallback_method_spec.rb | 17 ++++++++++++ .../client_secret_basic_spec.rb | 20 ++++++++++++++ .../client_secret_post_spec.rb | 15 +++++++++++ .../oauth/client_authentication/none_spec.rb | 27 +++++++++++++++++++ 6 files changed, 82 insertions(+), 6 deletions(-) create mode 100644 spec/lib/client_authentication/fallback_method_spec.rb diff --git a/lib/doorkeeper/client_authentication/fallback_method.rb b/lib/doorkeeper/client_authentication/fallback_method.rb index 1a29d4049..ca6711981 100644 --- a/lib/doorkeeper/client_authentication/fallback_method.rb +++ b/lib/doorkeeper/client_authentication/fallback_method.rb @@ -3,7 +3,7 @@ module Doorkeeper module ClientAuthentication class FallbackMethod - def self.matches_request? + def self.matches_request?(request) true end diff --git a/lib/doorkeeper/client_authentication/method.rb b/lib/doorkeeper/client_authentication/method.rb index 9816e11f1..5ce00e454 100644 --- a/lib/doorkeeper/client_authentication/method.rb +++ b/lib/doorkeeper/client_authentication/method.rb @@ -4,15 +4,12 @@ module Doorkeeper module ClientAuthentication class Method attr_reader :name, :method - + delegate :matches_request?, to: :method + def initialize(name, **options) @name = name @method = options[:method] end - - def matches_request?(request) - @method.matches_request?(request) - end end end end diff --git a/spec/lib/client_authentication/fallback_method_spec.rb b/spec/lib/client_authentication/fallback_method_spec.rb new file mode 100644 index 000000000..321a6b138 --- /dev/null +++ b/spec/lib/client_authentication/fallback_method_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Doorkeeper::ClientAuthentication::FallbackMethod do + describe 'matches_request?' do + it "matches all requests" do + expect(described_class.matches_request?({})).to be true + end + end + + describe 'authenticate' do + it "returns nil" do + expect(described_class.authenticate({})).to be_nil + end + end +end diff --git a/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb b/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb index a6fb84467..5ba55444b 100644 --- a/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb +++ b/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb @@ -16,4 +16,24 @@ expect(described_class.matches_request?(request)).to_not be true end end + + describe 'authenticate' do + it "returns credentials using the values from the authorization header" do + request = mock_request({}, ActionController::HttpAuthentication::Basic.encode_credentials('client_id', 'client_secret')) + + credentials = described_class.authenticate(request) + + expect(credentials).to be_instance_of(Doorkeeper::ClientAuthentication::Credentials) + expect(credentials.uid).to eq("client_id") + expect(credentials.secret).to eq("client_secret") + end + + it "returns nil if the client_secret is missing from the authorization header" do + request = mock_request({}, ActionController::HttpAuthentication::Basic.encode_credentials('client_id', '')) + + credentials = described_class.authenticate(request) + + expect(credentials).to be_nil + end + end end diff --git a/spec/lib/oauth/client_authentication/client_secret_post_spec.rb b/spec/lib/oauth/client_authentication/client_secret_post_spec.rb index ac382fa54..8c3ac8011 100644 --- a/spec/lib/oauth/client_authentication/client_secret_post_spec.rb +++ b/spec/lib/oauth/client_authentication/client_secret_post_spec.rb @@ -21,4 +21,19 @@ expect(described_class.matches_request?(request)).to_not be true end end + + describe 'authenticate' do + it "returns credentials using the values from the request parameters" do + request = mock_request({ + client_id: 'client_id', + client_secret: 'client_secret' + }) + + credentials = described_class.authenticate(request) + + expect(credentials).to be_instance_of(Doorkeeper::ClientAuthentication::Credentials) + expect(credentials.uid).to eq("client_id") + expect(credentials.secret).to eq("client_secret") + end + end end diff --git a/spec/lib/oauth/client_authentication/none_spec.rb b/spec/lib/oauth/client_authentication/none_spec.rb index 3896daed5..7375a7eaa 100644 --- a/spec/lib/oauth/client_authentication/none_spec.rb +++ b/spec/lib/oauth/client_authentication/none_spec.rb @@ -29,4 +29,31 @@ expect(described_class.matches_request?(request)).to_not be true end end + + describe 'authenticate' do + it "returns credentials using the values from the request parameters, without a secret" do + request = mock_request({ + client_id: 'client_id' + }) + + credentials = described_class.authenticate(request) + + expect(credentials).to be_instance_of(Doorkeeper::ClientAuthentication::Credentials) + expect(credentials.uid).to eq("client_id") + expect(credentials.secret).to be_nil + end + + it "ignores the client_secret if set" do + request = mock_request({ + client_id: 'client_id', + client_secret: 'client_secret' + }) + + credentials = described_class.authenticate(request) + + expect(credentials).to be_instance_of(Doorkeeper::ClientAuthentication::Credentials) + expect(credentials.uid).to eq("client_id") + expect(credentials.secret).to be_nil + end + end end From e63e18c4ff41de122153707a4aa4f978d1779ab3 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 20:46:23 +0200 Subject: [PATCH 12/16] Add spec for client_credentials as a lambda / callable --- lib/doorkeeper/config.rb | 6 +++++- spec/lib/config_spec.rb | 17 +++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index 0258b1318..52e4dde6e 100644 --- a/lib/doorkeeper/config.rb +++ b/lib/doorkeeper/config.rb @@ -81,7 +81,11 @@ def client_credentials(*methods) when :from_params :client_secret_post else - Kernel.warn("[DOORKEEPER] Unknown client_credentials method detected: #{method}") + if method.respond_to? :call + Kernel.warn("[DOORKEEPER] Unknown client_credentials method detected, received callable block") + else + Kernel.warn("[DOORKEEPER] Unknown client_credentials method detected: #{method}") + end end }.reject(&:nil?) diff --git a/spec/lib/config_spec.rb b/spec/lib/config_spec.rb index 2ea7322bb..cf077a2b3 100644 --- a/spec/lib/config_spec.rb +++ b/spec/lib/config_spec.rb @@ -307,6 +307,23 @@ expect(config.instance_variable_defined?("@client_credentials_methods")).to be false end + it "prints a warning message when passed a block for client_credentials" do + expect(Kernel).to receive(:warn).with(/\[DOORKEEPER\] client_credentials has been deprecated/).once + expect(Kernel).to receive(:warn).with( + /\[DOORKEEPER\] Unknown client_credentials method detected, received callable block/, + ).once + expect(Kernel).to receive(:warn).with( + /\[DOORKEEPER\] No known client_credentials method detected, ignoring option/ + ).once + + Doorkeeper.configure do + orm DOORKEEPER_ORM + client_credentials lambda { |request| return 'uid', 'secret' } + end + + expect(config.instance_variable_defined?("@client_credentials_methods")).to be false + end + it "can change the value, but converts to client_authentication compatible values" do Doorkeeper.configure do orm DOORKEEPER_ORM From 12b611008658053a93ac4f173ced0f08bb3ff250 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 20:59:35 +0200 Subject: [PATCH 13/16] Improve client authentication method specs and assert request_parameters for client_secret_post --- .../client_secret_basic_spec.rb | 8 +++---- .../client_secret_post_spec.rb | 21 +++++++++++++------ .../oauth/client_authentication/none_spec.rb | 20 +++++++++--------- spec/support/helpers/request_mock_helper.rb | 10 ++++----- 4 files changed, 34 insertions(+), 25 deletions(-) diff --git a/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb b/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb index 5ba55444b..749a29598 100644 --- a/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb +++ b/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb @@ -5,13 +5,13 @@ RSpec.describe Doorkeeper::OAuth::ClientAuthentication::ClientSecretBasic do describe 'matches_request?' do it "matches if the request has basic authorization" do - request = mock_request({}, ActionController::HttpAuthentication::Basic.encode_credentials('username', 'password')) + request = mock_request authorization: ActionController::HttpAuthentication::Basic.encode_credentials('username', 'password') expect(described_class.matches_request?(request)).to be true end it "doesn't match if the request has bearer authorization" do - request = mock_request({}, "Bearer foobar") + request = mock_request authorization: "Bearer foobar" expect(described_class.matches_request?(request)).to_not be true end @@ -19,7 +19,7 @@ describe 'authenticate' do it "returns credentials using the values from the authorization header" do - request = mock_request({}, ActionController::HttpAuthentication::Basic.encode_credentials('client_id', 'client_secret')) + request = mock_request authorization: ActionController::HttpAuthentication::Basic.encode_credentials('client_id', 'client_secret') credentials = described_class.authenticate(request) @@ -29,7 +29,7 @@ end it "returns nil if the client_secret is missing from the authorization header" do - request = mock_request({}, ActionController::HttpAuthentication::Basic.encode_credentials('client_id', '')) + request = mock_request authorization: ActionController::HttpAuthentication::Basic.encode_credentials('client_id', '') credentials = described_class.authenticate(request) diff --git a/spec/lib/oauth/client_authentication/client_secret_post_spec.rb b/spec/lib/oauth/client_authentication/client_secret_post_spec.rb index 8c3ac8011..7baf65b5d 100644 --- a/spec/lib/oauth/client_authentication/client_secret_post_spec.rb +++ b/spec/lib/oauth/client_authentication/client_secret_post_spec.rb @@ -5,18 +5,27 @@ RSpec.describe Doorkeeper::OAuth::ClientAuthentication::ClientSecretPost do describe 'matches_request?' do it "matches if the request doesn't have authorization" do - request = mock_request({ + request = mock_request request_parameters: { client_id: '1234', client_secret: '5678' - }) + } expect(described_class.matches_request?(request)).to be true end it "doesn't match if the request is missing client_secret" do - request = mock_request({ + request = mock_request request_parameters: { client_id: '1234' - }) + } + + expect(described_class.matches_request?(request)).to_not be true + end + + it "doesn't match if the parameters are in the query parameters" do + request = mock_request query_parameters: { + client_id: '1234', + client_secret: '5678' + } expect(described_class.matches_request?(request)).to_not be true end @@ -24,10 +33,10 @@ describe 'authenticate' do it "returns credentials using the values from the request parameters" do - request = mock_request({ + request = mock_request request_parameters: { client_id: 'client_id', client_secret: 'client_secret' - }) + } credentials = described_class.authenticate(request) diff --git a/spec/lib/oauth/client_authentication/none_spec.rb b/spec/lib/oauth/client_authentication/none_spec.rb index 7375a7eaa..402bcada5 100644 --- a/spec/lib/oauth/client_authentication/none_spec.rb +++ b/spec/lib/oauth/client_authentication/none_spec.rb @@ -5,26 +5,26 @@ RSpec.describe Doorkeeper::OAuth::ClientAuthentication::None do describe 'matches_request?' do it "matches if the request doesn't have authorization or client_secret" do - request = mock_request({ + request = mock_request request_parameters: { client_id: '1234' - }) + } expect(described_class.matches_request?(request)).to be true end it "doesn't match if the request has client_secret" do - request = mock_request({ + request = mock_request request_parameters: { client_id: '1234', client_secret: "5678" - }) + } expect(described_class.matches_request?(request)).to_not be true end it "doesn't match if the request has authorization" do - request = mock_request({ + request = mock_request request_parameters: { client_id: '1234' - }, ActionController::HttpAuthentication::Basic.encode_credentials('username', 'password')) + }, authorization: ActionController::HttpAuthentication::Basic.encode_credentials('username', 'password') expect(described_class.matches_request?(request)).to_not be true end @@ -32,9 +32,9 @@ describe 'authenticate' do it "returns credentials using the values from the request parameters, without a secret" do - request = mock_request({ + request = mock_request request_parameters: { client_id: 'client_id' - }) + } credentials = described_class.authenticate(request) @@ -44,10 +44,10 @@ end it "ignores the client_secret if set" do - request = mock_request({ + request = mock_request request_parameters: { client_id: 'client_id', client_secret: 'client_secret' - }) + } credentials = described_class.authenticate(request) diff --git a/spec/support/helpers/request_mock_helper.rb b/spec/support/helpers/request_mock_helper.rb index f55f0cf3d..968d9dcf0 100644 --- a/spec/support/helpers/request_mock_helper.rb +++ b/spec/support/helpers/request_mock_helper.rb @@ -2,7 +2,7 @@ module RequestMockHelper # I'm not sure if there's a better way to get a mock rack request for # testing. Here we don't need a full request spec, but we do need enough to # check that the logic of these classes works. - def mock_request(params, credentials = nil) + def mock_request(request_parameters: {}, query_parameters: {}, authorization: nil) request = ActionDispatch::Request.new({ "REQUEST_METHOD"=>"POST", "SERVER_NAME"=>"example.org", @@ -12,12 +12,12 @@ def mock_request(params, credentials = nil) "HTTP_HOST"=> "example.org", "ORIGINAL_FULLPATH" => "/test", "action_dispatch.remote_ip" => "127.0.0.1", - "action_dispatch.request.query_parameters" => {}, - "action_dispatch.request.request_parameters" => params + "action_dispatch.request.query_parameters" => query_parameters, + "action_dispatch.request.request_parameters" => request_parameters }) - unless credentials.nil? - request.env["HTTP_AUTHORIZATION"] = credentials + unless authorization.nil? + request.env["HTTP_AUTHORIZATION"] = authorization end request From 30847913ed30b390757793e59847f07574db0a47 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 21:07:12 +0200 Subject: [PATCH 14/16] Change message for client_credentials conversion --- lib/doorkeeper/config.rb | 2 +- spec/lib/config_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index 52e4dde6e..6991f0ed4 100644 --- a/lib/doorkeeper/config.rb +++ b/lib/doorkeeper/config.rb @@ -90,7 +90,7 @@ def client_credentials(*methods) }.reject(&:nil?) if client_authentication.empty? - Kernel.warn("[DOORKEEPER] No known client_credentials method detected, ignoring option") + Kernel.warn("[DOORKEEPER] No known client_credentials method detected, cannot automatically convert to client_authentication option") else @config.instance_variable_set(:@client_credentials_methods, client_authentication.concat([:none])) end diff --git a/spec/lib/config_spec.rb b/spec/lib/config_spec.rb index cf077a2b3..92278e7dd 100644 --- a/spec/lib/config_spec.rb +++ b/spec/lib/config_spec.rb @@ -296,7 +296,7 @@ /\[DOORKEEPER\] Unknown client_credentials method detected: from_digest/, ).once expect(Kernel).to receive(:warn).with( - /\[DOORKEEPER\] No known client_credentials method detected, ignoring option/ + /\[DOORKEEPER\] No known client_credentials method detected, cannot automatically convert to client_authentication option/ ).once Doorkeeper.configure do @@ -313,7 +313,7 @@ /\[DOORKEEPER\] Unknown client_credentials method detected, received callable block/, ).once expect(Kernel).to receive(:warn).with( - /\[DOORKEEPER\] No known client_credentials method detected, ignoring option/ + /\[DOORKEEPER\] No known client_credentials method detected, cannot automatically convert to client_authentication option/ ).once Doorkeeper.configure do From d302bb54c5cccef00456a42fd440821fbea7f6d1 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 22:00:50 +0200 Subject: [PATCH 15/16] Improve client authentication registry and add specs --- lib/doorkeeper/client_authentication.rb | 6 +-- .../client_authentication/method.rb | 4 +- .../client_authentication/registry.rb | 10 ++-- spec/lib/client_authentication/method_spec.rb | 24 +++++++++ spec/lib/client_authentication_spec.rb | 51 +++++++++++++++++++ 5 files changed, 83 insertions(+), 12 deletions(-) create mode 100644 spec/lib/client_authentication/method_spec.rb create mode 100644 spec/lib/client_authentication_spec.rb diff --git a/lib/doorkeeper/client_authentication.rb b/lib/doorkeeper/client_authentication.rb index 30fa4ca88..3ad05160b 100644 --- a/lib/doorkeeper/client_authentication.rb +++ b/lib/doorkeeper/client_authentication.rb @@ -11,17 +11,17 @@ module ClientAuthentication register( :none, - method: Doorkeeper::OAuth::ClientAuthentication::None, + Doorkeeper::OAuth::ClientAuthentication::None, ) register( :client_secret_post, - method: Doorkeeper::OAuth::ClientAuthentication::ClientSecretPost, + Doorkeeper::OAuth::ClientAuthentication::ClientSecretPost, ) register( :client_secret_basic, - method: Doorkeeper::OAuth::ClientAuthentication::ClientSecretBasic, + Doorkeeper::OAuth::ClientAuthentication::ClientSecretBasic, ) end end diff --git a/lib/doorkeeper/client_authentication/method.rb b/lib/doorkeeper/client_authentication/method.rb index 5ce00e454..6f9899635 100644 --- a/lib/doorkeeper/client_authentication/method.rb +++ b/lib/doorkeeper/client_authentication/method.rb @@ -6,9 +6,9 @@ class Method attr_reader :name, :method delegate :matches_request?, to: :method - def initialize(name, **options) + def initialize(name, method) @name = name - @method = options[:method] + @method = method end end end diff --git a/lib/doorkeeper/client_authentication/registry.rb b/lib/doorkeeper/client_authentication/registry.rb index 03027bc5b..ab4ff9cb7 100644 --- a/lib/doorkeeper/client_authentication/registry.rb +++ b/lib/doorkeeper/client_authentication/registry.rb @@ -9,12 +9,8 @@ module Registry # Allows to register custom OAuth client authentication method so that # Doorkeeper could recognize and process it. # - def register(name_or_method, **options) - unless name_or_method.is_a?(Doorkeeper::ClientAuthentication::Method) - name_or_method = Doorkeeper::ClientAuthentication::Method.new(name_or_method, **options) - end - - name_key = name_or_method.name.to_sym + def register(name, method) + name_key = name.to_sym if methods.key?(name_key) ::Kernel.warn <<~WARNING @@ -23,7 +19,7 @@ def register(name_or_method, **options) WARNING end - methods[name_key] = name_or_method + methods[name_key] = Doorkeeper::ClientAuthentication::Method.new(name, method) end # [NOTE]: make it to use #fetch after removing fallbacks diff --git a/spec/lib/client_authentication/method_spec.rb b/spec/lib/client_authentication/method_spec.rb new file mode 100644 index 000000000..6de4be128 --- /dev/null +++ b/spec/lib/client_authentication/method_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Doorkeeper::ClientAuthentication::Method do + subject(:method) { described_class.new(name, client_authentication_method) } + + let(:name) { "secret_handshake" } + let(:client_authentication_method) { double } + + it "reflects the given name" do + expect(method.name).to eq name + end + + it "reflects the given method" do + expect(method.method).to eq client_authentication_method + end + + it "delegates matches_request? to the method" do + expect(client_authentication_method).to receive(:matches_request?).with({ example: true }) + + method.matches_request?({ example: true }) + end +end diff --git a/spec/lib/client_authentication_spec.rb b/spec/lib/client_authentication_spec.rb new file mode 100644 index 000000000..c8afbbcf7 --- /dev/null +++ b/spec/lib/client_authentication_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Doorkeeper::ClientAuthentication do + # Avoid global side effects + before do + @original_client_authentication_methods = Doorkeeper::ClientAuthentication::Registry.methods.deep_dup + end + + after do + Doorkeeper::ClientAuthentication::Registry.methods = @original_client_authentication_methods + end + + describe "#register" do + context "with a name and options" do + subject(:the_registered_method) { described_class.get(name) } + + let(:name) { "puzzle_box" } + let(:client_authentication_method) { double } + + before do + described_class.register( + name, + client_authentication_method, + ) + end + + it "creates a new Flow" do + expect(the_registered_method).to be_a(Doorkeeper::ClientAuthentication::Method) + end + + it "passes on the given name" do + expect(the_registered_method.name).to eq name + end + + it "sets the options" do + expect(the_registered_method.method).to eq client_authentication_method + end + + it "shows a warning when trying to register already existing method" do + expect(::Kernel).to receive(:warn).with(/already registered/) + + described_class.register( + name, + client_authentication_method, + ) + end + end + end +end From 26a1d8af20fe45201b4d7bffc563aa601cac999d Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 21 Apr 2025 22:18:20 +0200 Subject: [PATCH 16/16] Add an additional spec to assert the return value of config.client_authentication_methods --- spec/lib/config_spec.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/spec/lib/config_spec.rb b/spec/lib/config_spec.rb index 92278e7dd..b1ce2a38d 100644 --- a/spec/lib/config_spec.rb +++ b/spec/lib/config_spec.rb @@ -392,6 +392,16 @@ expect(config.client_authentication_methods.map(&:name)).to contain_exactly(:client_secret_basic) end + + it "returns an array of Doorkeeper::ClientAuthentication::Method" do + Doorkeeper.configure do + orm DOORKEEPER_ORM + client_authentication [:client_secret_basic, :client_secret_post] + end + + expect(config.client_authentication_methods.size).to be 2 + expect(config.client_authentication_methods).to all(be_a(Doorkeeper::ClientAuthentication::Method)) + end end describe "force_ssl_in_redirect_uri" do