diff --git a/.rspec b/.rspec index 53607ea52..660778bdc 100644 --- a/.rspec +++ b/.rspec @@ -1 +1 @@ ---colour +--colour --format documentation diff --git a/lib/doorkeeper.rb b/lib/doorkeeper.rb index 97e3f549a..34bae57cc 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,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 new file mode 100644 index 000000000..3ad05160b --- /dev/null +++ b/lib/doorkeeper/client_authentication.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require "doorkeeper/client_authentication/credentials" +require "doorkeeper/client_authentication/fallback_method" +require "doorkeeper/client_authentication/method" +require "doorkeeper/client_authentication/registry" + +module Doorkeeper + module ClientAuthentication + extend Registry + + register( + :none, + Doorkeeper::OAuth::ClientAuthentication::None, + ) + + register( + :client_secret_post, + Doorkeeper::OAuth::ClientAuthentication::ClientSecretPost, + ) + + register( + :client_secret_basic, + Doorkeeper::OAuth::ClientAuthentication::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/fallback_method.rb b/lib/doorkeeper/client_authentication/fallback_method.rb new file mode 100644 index 000000000..ca6711981 --- /dev/null +++ b/lib/doorkeeper/client_authentication/fallback_method.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Doorkeeper + module ClientAuthentication + class FallbackMethod + def self.matches_request?(request) + true + end + + def self.authenticate(request) + nil + 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..6f9899635 --- /dev/null +++ b/lib/doorkeeper/client_authentication/method.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Doorkeeper + module ClientAuthentication + class Method + attr_reader :name, :method + delegate :matches_request?, to: :method + + def initialize(name, method) + @name = name + @method = method + 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..ab4ff9cb7 --- /dev/null +++ b/lib/doorkeeper/client_authentication/registry.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Doorkeeper + module ClientAuthentication + module Registry + mattr_accessor :methods + self.methods = {} + + # Allows to register custom OAuth client authentication method so that + # Doorkeeper could recognize and process it. + # + def register(name, method) + name_key = name.to_sym + + 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 + + methods[name_key] = Doorkeeper::ClientAuthentication::Method.new(name, method) + end + + # [NOTE]: make it to use #fetch after removing fallbacks + def get(name) + methods[name.to_sym] + end + end + end +end diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index c464f1803..6991f0ed4 100644 --- a/lib/doorkeeper/config.rb +++ b/lib/doorkeeper/config.rb @@ -70,8 +70,30 @@ def scopes_by_grant_type(hash = {}) # `params` object. # # @param methods [Array] Define client credentials + # @deprecated def client_credentials(*methods) - @config.instance_variable_set(:@client_credentials_methods, 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 + 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?) + + if client_authentication.empty? + 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 end # Change the way access token is authenticated from the request object. @@ -186,6 +208,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) @@ -253,9 +282,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: %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 + # Sets the token_reuse_limit # It will be used only when reuse_access_token option in enabled # By default it will be 100 @@ -579,8 +610,23 @@ def pkce_code_challenge_methods_supported pkce_code_challenge_methods end - def client_credentials_methods - @client_credentials_methods ||= %i[from_basic from_params] + 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 def access_token_methods 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/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/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/oauth/client_authentication/client_secret_basic.rb b/lib/doorkeeper/oauth/client_authentication/client_secret_basic.rb new file mode 100644 index 000000000..9c51ba95e --- /dev/null +++ b/lib/doorkeeper/oauth/client_authentication/client_secret_basic.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Doorkeeper + module OAuth + module ClientAuthentication + class ClientSecretBasic + def self.matches_request?(request) + request.authorization.present? && request.authorization.downcase.start_with?('basic') + end + + def self.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/oauth/client_authentication/client_secret_post.rb b/lib/doorkeeper/oauth/client_authentication/client_secret_post.rb new file mode 100644 index 000000000..5e4302e86 --- /dev/null +++ b/lib/doorkeeper/oauth/client_authentication/client_secret_post.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Doorkeeper + module OAuth + module ClientAuthentication + class ClientSecretPost + def self.matches_request?(request) + request.method.upcase === "POST" && request.request_parameters[:client_id].present? && request.request_parameters[:client_secret].present? + end + + def self.authenticate(request) + client_id = request.request_parameters[:client_id] + client_secret = request.request_parameters[:client_secret] + + Doorkeeper::ClientAuthentication::Credentials.new( + client_id, + client_secret + ) + end + end + end + end +end diff --git a/lib/doorkeeper/oauth/client_authentication/none.rb b/lib/doorkeeper/oauth/client_authentication/none.rb new file mode 100644 index 000000000..97e2d51a9 --- /dev/null +++ b/lib/doorkeeper/oauth/client_authentication/none.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Doorkeeper + module OAuth + module ClientAuthentication + class None + def self.matches_request?(request) + !request.authorization && request.request_parameters[:client_id] && !request.request_parameters[:client_secret] + end + + def self.authenticate(request) + Doorkeeper::ClientAuthentication::Credentials.new( + request.request_parameters[:client_id], nil + ) + end + end + end + end +end diff --git a/lib/doorkeeper/request.rb b/lib/doorkeeper/request.rb index a2705a91f..c0265ddc8 100644 --- a/lib/doorkeeper/request.rb +++ b/lib/doorkeeper/request.rb @@ -3,6 +3,21 @@ module Doorkeeper module Request class << self + 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.method + else + Doorkeeper::ClientAuthentication::FallbackMethod + end + end + def authorization_strategy(response_type) grant_flow = authorization_flows.detect do |flow| flow.matches_response_type?(response_type) @@ -40,6 +55,10 @@ def token_strategy(grant_type) private + def client_authentication_methods + Doorkeeper.configuration.client_authentication_methods + end + def authorization_flows Doorkeeper.configuration.authorization_response_flows end diff --git a/lib/doorkeeper/server.rb b/lib/doorkeeper/server.rb index 4ba0e3665..76cf16d78 100644 --- a/lib/doorkeeper/server.rb +++ b/lib/doorkeeper/server.rb @@ -8,6 +8,10 @@ def initialize(context) @context = context end + def client_authentication_method_for_request + Request.client_authentication_method(context.request) + end + def authorization_request(strategy) klass = Request.authorization_strategy(strategy) klass.new(self) @@ -37,8 +41,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_method_for_request.authenticate(context.request) end end end 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/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/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 diff --git a/spec/lib/config_spec.rb b/spec/lib/config_spec.rb index 83970ec89..b1ce2a38d 100644 --- a/spec/lib/config_spec.rb +++ b/spec/lib/config_spec.rb @@ -279,19 +279,128 @@ 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, cannot automatically convert to client_authentication 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 "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, cannot automatically convert to client_authentication 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 + 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_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_credentials_methods) - .to eq(%i[from_digest from_params]) + expect(config.client_authentication_methods.size).to be 2 + expect(config.client_authentication_methods).to all(be_a(Doorkeeper::ClientAuthentication::Method)) 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_authentication/client_secret_basic_spec.rb b/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb new file mode 100644 index 000000000..749a29598 --- /dev/null +++ b/spec/lib/oauth/client_authentication/client_secret_basic_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +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 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 authorization: "Bearer foobar" + + 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 authorization: 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 authorization: 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 new file mode 100644 index 000000000..7baf65b5d --- /dev/null +++ b/spec/lib/oauth/client_authentication/client_secret_post_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +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 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_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 + end + + describe 'authenticate' do + it "returns credentials using the values from the request parameters" do + request = mock_request request_parameters: { + 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 new file mode 100644 index 000000000..402bcada5 --- /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 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_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_parameters: { + client_id: '1234' + }, authorization: ActionController::HttpAuthentication::Basic.encode_credentials('username', 'password') + + 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 request_parameters: { + 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 request_parameters: { + 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 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/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 64306a41e..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", @@ -240,10 +240,10 @@ 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( + 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 c319bfedf..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( @@ -97,7 +100,8 @@ it "client request a token with expired access token" do @token.update_attribute :expires_in, -100 - post refresh_token_endpoint_url( + # This method is broken: + post refresh_token_endpoint_url, params: refresh_token_endpoint_params( client: @client, refresh_token: @token.refresh_token, ) expect(json_response).to include( @@ -136,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, ) @@ -149,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, @@ -168,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", @@ -178,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", @@ -188,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", @@ -205,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) @@ -222,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, ) @@ -237,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, ) @@ -264,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_mock_helper.rb b/spec/support/helpers/request_mock_helper.rb new file mode 100644 index 000000000..968d9dcf0 --- /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(request_parameters: {}, query_parameters: {}, authorization: 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" => query_parameters, + "action_dispatch.request.request_parameters" => request_parameters + }) + + unless authorization.nil? + request.env["HTTP_AUTHORIZATION"] = authorization + end + + request + end +end + +RSpec.configuration.send :include, RequestMockHelper 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 32b6ce683..7a77e7525 100644 --- a/spec/support/helpers/url_helper.rb +++ b/spec/support/helpers/url_helper.rb @@ -1,8 +1,13 @@ # frozen_string_literal: true module UrlHelper - def token_endpoint_url(options = {}) - parameters = { + # FIXME + 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), @@ -11,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), @@ -24,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 = {}) @@ -41,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