Add Client Authentication Methods Registry #1772

ThisIsMissEm · 2025-04-20T20:49:47Z

Summary

In #1770, I realised that really we do need the ability for extensions to be able to register additional client authentication mechanisms. This implements a strategy similar to that of the grant flows registry.

Essentially we should be able to support all of the registered methods, either in core or via extensions: https://www.iana.org/assignments/oauth-parameters/oauth-parameters.xhtml#token-endpoint-auth-method

Implementation Notes

This allows for another gem that extends doorkeeper to do something like:

  Doorkeeper::ClientAuthentication.register(
    :private_key_jwt,
    PrivateKeyJwt
  )

And provided PrivateKeyJwt implements what it needs to, then that client authentication mechanism would then be supported by Doorkeeper!

For the user, they'd need to do:

Doorkeeper.configure do
  # ...
  client_authentication [ :none, :client_secret_basic, :client_secret_post, :private_key_jwt ]
  # ...
end

Other Information

This would be a breaking change, unless we managed to convert :from_params and :from_basic across to the new values — however, this does use request.request_parameter instead of request.parameters so only looks at the request body, not the query params as well.

So like, if you had:

Doorkeeper.configure do
  # ...

  # disabling `:from_params` for client_secret_post / none
  client_credentials :from_basic
  # ...
end

You'd now need:

Doorkeeper.configure do
  # ...
  client_authentication [ :client_secret_basic ]
  # ...
end

I'm not yet 100% happy with this code, but it's a start.

I haven't yet integrated this in fully, nor added tests for it all.

This work was in part funded by my wonderful supporters: https://support.thisismissem.social

lib/doorkeeper/client_authentication.rb

lib/doorkeeper/request.rb

ThisIsMissEm · 2025-04-20T20:56:48Z

lib/doorkeeper/server.rb

      context.send :resource_owner_from_credentials
    end

    def credentials


This is used by the client method above, however, we probably need to change that method to:

def client @client ||= OAuth::Client.find(credentials.uid) end

and have:

def client_authenticated? return false unless client # I'm not sure whether this should return true or false, is # a "public client" that doesn't require authentication # "authenticated"? return true if credentials.blank? && !client.confidential? client.secret_matches?(credentials.secret) end

i.e., we're separating the "getting of the client" from the "authenticating of a client", such that for things like token revocation, we can just get the client, authenticate it if necessary, but then assert that the access token or refresh token was issued to that client.

I'm not 100% sure on the exact specifics here though.

Now that I think about it, maybe as it is is better? You only get a client when it's authenticated, even if there were credentials on the request. Maybe it's important to know how the client is authenticated, I'm not sure?

ThisIsMissEm · 2025-04-20T22:13:02Z

spec/support/helpers/url_helper.rb

 # frozen_string_literal: true

 module UrlHelper
+  # FIXME


@nbulaj so thanks to doing this, I've just realized all our tests are technically wrong. We're passing everything through query parameters, when the client_secret should never go via query parameters, as far as I know?

Including the client credentials in the request-body using the two parameters is NOT RECOMMENDED and SHOULD be limited to clients unable to directly utilize the HTTP Basic authentication scheme (or other password-based HTTP authentication schemes). The parameters can only be transmitted in the request-body and MUST NOT be included in the request URI.
— RFC 6749 Section 2.3.1

That last sentence in particular. The client_id may go via the request query parameters, but client_secret should never, as client authentication is either via the request body or via the request http authorization header.

Here in our tests we're passing the parameters via the request URI, not the request body.

/oauth/token is a POST request has client authentication

/oauth/revoke is a POST request has client authentication

/oauth/authorize is a GET request (doesn't include client_secret)

So the URLs that we are generating here are largely incorrect, and that's going to require some pretty serious work to fix. We likely need to change most of these to be like a build_token_request or build_refresh_token_request method which returns the url, body, and headers.

I have a fix in 93b2d41

Well I believe these are legacy helpers and tests which are pass params via the url params. So if we can refactor them - will be great, thanks!

@nbulaj yeah, I've gone ahead and fixed how we do all the requests in 93b2d41

It was a fairly significant change, but we can resolve this conversation point.

Suggested change

# FIXME

This FIXME is resolved.

We may want to go through and find and replace refresh_token_endpoint_url with just token_endpoint_url — though these are just the same value.

ThisIsMissEm · 2025-04-20T23:29:30Z

spec/requests/endpoints/token_spec.rb

  it "accepts client credentials with basic auth header" do
    post token_endpoint_url,
-         params: {
+         params: token_endpoint_params(


This was failing because of missing grant_type parameter. I'm not sure how it ever passed.

(it's clearly using authorization_code though, due to the presence of the code parameter

token_endpoint_url is a spec helper which has this:

grant_type: options[:grant_type] || "authorization_code",

So I believe with token_endpoint_params you just override the params from the URL generated by the helper

Yeah, the issue I'm pointing out here is that token_endpoint_url returned a string like /oauth/token?grant_type=authorization_code but then the params were set just as a hash. I was expecting params to be all the params.

It looks like after some investigation the post method from Rack::Test (which I think is this post method here), is actually parsing the uri and merging it's query parameters with the params from the env/options hash, which was unexpected to me.

ref: https://github.com/rack/rack-test/blob/main/lib/rack/test.rb#L160
ref: https://github.com/rack/rack-test/blob/main/lib/rack/test.rb#L293

lib/doorkeeper/client_authentication/fallback_mechanism.rb

ThisIsMissEm · 2025-04-20T23:37:29Z

So this actually works now, and all the tests pass.

All we need now is:

tests for the classes in doorkeeper/oauth/client_authentication/
any tests we want for the client authentication registry stuff (I haven't looked at GrantFlow to determine what we might need here)

ThisIsMissEm · 2025-04-21T01:46:44Z

spec/lib/oauth/client_authentication/none_spec.rb

+    # 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)


There's gotta be a better way to get a fake request that acts in a given way in ruby, surely?

This comment still applies, I just changed the method signature to use keyword args.

ThisIsMissEm · 2025-04-21T01:47:31Z

spec/lib/oauth/client_authentication/none_spec.rb

+        client_secret: "5678"
+      })
+
+      expect(described_class.matches_request?(request)).to_not be true


matches_request? actually returns true or nil, not true or false, just due to a quirk of boolean logic in ruby.

lib/doorkeeper/config/option.rb

ThisIsMissEm · 2025-04-21T01:53:18Z

lib/doorkeeper/config.rb

    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]


Note: this has to be an array instead of multiple arguments:

Doorkeeper.configure do client_authentication [ :client_secret_basic, :client_secret_post, :none ] end

Instead of

Doorkeeper.configure do client_authentication :client_secret_basic, :client_secret_post, :none end

We should maybe consider adding an ability to declare a option as taking a list of arguments as the value.

ThisIsMissEm · 2025-04-21T01:58:12Z

lib/doorkeeper/oauth/client_authentication/client_secret_basic.rb

+          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?


I'm not 100% sure if you can do client authentication with just the username, e.g.,

Authorization: Basic Base64Encode(pPqlv1hayMY3NXSMnikiqMLw3G2tyibuRex2HHo3aVE:)

My reading of the spec is that without the password this mechanism wouldn't match? Though, previously the code would have accepted that as a value authentication, and it'd return credentials without the client_secret, so maybe just the client_id (username) is necessary?

I've asked about this here: https://mailarchive.ietf.org/arch/msg/oauth/QGJfkCMqN2kVMRMDwvCiF3mZ6r4/

Consensus in the mailing list seems to imply that these methods only support POST requests, and require the client_secret to be present.

ThisIsMissEm · 2025-04-21T16:20:21Z

lib/doorkeeper/oauth/client.rb

      end

+      # TODO: Figure out a way to have this just get the client but not assert
+      # authentication if not secret


we could arguably change the self.authenticate to be an instance method instead, so you find the client first, then authenticate it.

I'm afraid of breaking changes TBH with changing public API behavior. We'll need to push as a major version updated I believe and check for compatibility as least with some known extensions like openid_connect and similar

Perhaps we could do it in a non-breaking way? I'm not sure.

For openid_connect, I'd expect it'd need to write their own client authentication methods, which they currently don't have. (i.e., they haven't been able to implement private_key_jwt or similar for client authentication — they just do stuff with ID tokens and amending responses to use JWTs

…credentials

ThisIsMissEm · 2025-04-21T18:39:52Z

lib/doorkeeper/config.rb

      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
+            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


This is pretty messy, but we basically have to handle all the different permutations this supported.

This also now handles the custom authenticator case, where the client_credentials was a lambda or class.

ThisIsMissEm · 2025-04-21T18:48:46Z

lib/doorkeeper/config.rb

      #
      # @param methods [Array] Define client credentials
+      # @deprecated
      def client_credentials(*methods)


This is pretty messy because of all the different ways client_credentials could be set, and we can only convert known methods across automatically.

We could move the deprecated line to the else at the end, and basically say: "here's what you need instead: client_authentication [...]"

lib/doorkeeper/config.rb

ThisIsMissEm · 2025-04-21T18:50:15Z

lib/doorkeeper/config.rb

+      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


I'm not sure if we could/should do this in def client_credentials above?

I think this would depend on the order in which these were registered — we really more want a hook that's like "validations to run after configuration", which may be the Doorkeeper::Config::Validations class.

ThisIsMissEm · 2025-04-21T18:50:54Z

lib/doorkeeper/config/validations.rb

+      def validate_client_authentication_value
+        return if client_authentication.is_a?(Array)
+
+        ::Rails.logger.warn(


I can't figure out a way to test this, but this is where supporting a list of arguments for option would be a good idea.

Set an option in the config_spec.rb ? I mean what exactly is the problem with testing it? any blockers which don't come to my mind?

So I want to avoid a misconfiguration like:

Doorkeeper.config do client_authentication :client_secret_post end

When the client_authentication method expects an array as the value — I guess we could automatically take a single value and convert it to an array, but the reason it's a misconfiguration is because client_authentication isn't additive to the defaults, it replaces.

So if I used a plugin that implemented :private_key_jwt, then I'd need to add it with:

Doorkeeper.config do client_authentication [:private_key_jwt, :client_secret_basic, :client_secret_post, :none] end

Maybe we should have prepend_client_authentication :method and append_client_authentication :method to be additive? And perhaps disable_client_authentication :method to remove a method?

…ers for client_secret_post

ThisIsMissEm · 2025-04-21T19:00:27Z

spec/lib/oauth/client_authentication/client_secret_post_spec.rb

+      expect(described_class.matches_request?(request)).to_not be true
+    end
+
+    it "doesn't match if the parameters are in the query parameters" do


Note: previously this would have passed, and that was incorrect.

ThisIsMissEm · 2025-04-21T19:03:01Z

.rspec

@@ -1 +1 @@
--colour
+--colour --format documentation


This is temporary, but was necessary to help debug the tests — I'm wondering if we can set this based on environment?

By environment you mean what exactly? I personally also prefer documentation format, but sometimes it creates a noisy output and harder for debugging (in CI for example, a lot to scroll and read)

ThisIsMissEm · 2025-04-21T20:09:35Z

I think I'm now settled on and happy with this code and the test coverage, but let me know if it needs more.

ThisIsMissEm · 2025-04-21T20:15:32Z

lib/doorkeeper/server.rb

+    def client_authentication_method_for_request
+      Request.client_authentication_method(context.request)
+    end
+


Arguably we don't need this, there's no tests for it because it just returns the class, it doesn't instantiate it, because there's nothing the class should need as an instance — all the methods are class methods.

BTW I like such utility methods, in some contexts they can say more then their implementations. I'm OK for having such

This actually needs to be pluralized per the comment about multiple authentication methods being present.

ThisIsMissEm · 2025-04-21T20:16:35Z

lib/doorkeeper/server.rb

-      methods = Doorkeeper.config.client_credentials_methods
-      @credentials ||= OAuth::Client::Credentials.from_request(context.request, *methods)
+      @credentials ||= client_authentication_method_for_request.authenticate(context.request)


We could just do this instead:

method = Request.client_authentication_method(context.request) @credentials ||= method.authenticate(context.request)

ThisIsMissEm · 2025-04-21T20:17:27Z

lib/doorkeeper/request.rb

+      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


Currently we only return the first matching method for a request, not all potentially matching methods. I'm not sure if we should support that?

Yeah, this is actually a bug. We need to collect an array of potential client credentials, and then check if we:

had zero, then return an error when client authentication is required (invalid_client, 401)

had more than one, return an error (invalid_request, 400, https://www.rfc-editor.org/rfc/rfc6749.html#section-2.3 but the error type comes from section 5.2)

I discovered this whilst implementing client authentication mechanisms in Hollo (an activitypub server)

…thentication_methods

ThisIsMissEm · 2025-04-21T20:19:46Z

lib/doorkeeper/oauth/client_authentication/client_secret_post.rb

+          request.method.upcase === "POST" && request.request_parameters[:client_id].present? && request.request_parameters[:client_secret].present?
+        end
+
+        def self.authenticate(request)


I'm not sure if authenticate is really the method name that's correct here.. maybe more credentials or parse or something? Maybe call or handle even?

Could also call it from_request or something? Given how it's called?

Good question. Maybe something related to what it actually returns (oorkeeper::ClientAuthentication::Credentials )? credentials (or smth like build_credentials) sounds better to me as its more clean and meaningful

So I think this actually needs to be changed, since the specification actually requires parsing out all present authentication mechanisms, and then failing if more than one matches, per the last line in https://datatracker.ietf.org/doc/html/rfc6749#section-2.3

I think we actually need these to have a match & extract phase, and then an authenticate phase.

This is also interesting from section 2.3.2:

When using other authentication methods, the authorization server MUST define a mapping between the client identifier (registration record) and authentication scheme.

nbulaj · 2025-04-24T12:29:34Z

Wow so many comments and changes 😃 I'll need some time & 🍺 to check everything. Thanks anyway, I'll get back to it ASAP (sorry have busy days currently)

Copilot

Pull Request Overview

The PR introduces a registry for client authentication methods to allow extensions to register additional strategies for client authentication in Doorkeeper. Key changes include:

Adding the Client Authentication Registry, along with wrapper classes for methods and credentials.
Updating configuration options and deprecating the old client_credentials interface.
Adjusting behaviors in the server and request modules to use the new authentication registry.

Reviewed Changes

Copilot reviewed 36 out of 37 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
spec/lib/client_authentication_spec.rb	Tests for the registration process of new methods.
spec/lib/client_authentication/method_spec.rb	Tests verifying the behavior of the client authentication method wrapper.
spec/lib/client_authentication/fallback_method_spec.rb	Basic tests for fallback method behavior.
spec/lib/client_authentication/credentials_spec.rb	Placeholder tests with some parts commented out needing more granular coverage.
lib/doorkeeper/server.rb	Updated to delegate credentials extraction to the new registry interface.
lib/doorkeeper/request.rb	Introduced client_authentication_method to pick the proper strategy.
lib/doorkeeper/oauth/client_authentication/*.rb	Added implementations of various authentication strategies.
lib/doorkeeper/config/validations.rb & lib/doorkeeper/config.rb	Updated configuration validations and migration from client_credentials to client_authentication.
lib/doorkeeper/client_authentication/*.rb	Added the registry, method wrappers, fallback, and credentials structures required to support the registry.
lib/doorkeeper.rb	Autoload adjustments to integrate the new client authentication layer.

Files not reviewed (1)

.rspec: Language not supported

Comments suppressed due to low confidence (2)

spec/lib/client_authentication/credentials_spec.rb:16

The commented-out tests indicate missing coverage for methods like from_request, from_params, and from_basic. Consider un-commenting and splitting these tests into individual cases to ensure comprehensive testing of Credentials behavior.

# FIXME: Move to individual tests:

lib/doorkeeper/client_authentication/registry.rb:12

[nitpick] Consider renaming the parameter 'method' to a more descriptive name (e.g., 'auth_method') to avoid potential confusion with Ruby's Method class and improve code clarity.

def register(name, method)

Initial prototyping

3fbe2f8

ThisIsMissEm commented Apr 20, 2025

View reviewed changes

lib/doorkeeper/client_authentication.rb Outdated Show resolved Hide resolved

ThisIsMissEm commented Apr 20, 2025

View reviewed changes

lib/doorkeeper/request.rb Show resolved Hide resolved

ThisIsMissEm commented Apr 20, 2025

View reviewed changes

Second pass, because wow my code was rough

7fdad73

ThisIsMissEm force-pushed the feat/client-authentication-mechanisms branch from 8345206 to 7fdad73 Compare April 20, 2025 22:04

ThisIsMissEm commented Apr 20, 2025

View reviewed changes

ThisIsMissEm added 2 commits April 21, 2025 01:27

Fix token introspection for missing credentials

c1e95e9

Fix broken tests due to them using query string instead of form body

93b2d41

ThisIsMissEm commented Apr 20, 2025

View reviewed changes

lib/doorkeeper/client_authentication/fallback_mechanism.rb Outdated Show resolved Hide resolved

Fix token introspection, fallback mechanism should return nil

42a9d10

ThisIsMissEm marked this pull request as ready for review April 20, 2025 23:39

ThisIsMissEm added 2 commits April 21, 2025 03:45

Deprecate old client_credentials option

fafad38

Start adding tests for client authentication methods

1595444

ThisIsMissEm commented Apr 21, 2025

View reviewed changes

lib/doorkeeper/config/option.rb Outdated Show resolved Hide resolved

ThisIsMissEm commented Apr 21, 2025

View reviewed changes

ThisIsMissEm mentioned this pull request Apr 21, 2025

Implement helper method to retrieve token_endpoint_auth_methods #1770

Closed

ThisIsMissEm commented Apr 21, 2025

View reviewed changes

Add tests and migration strategy for client_authentication vs client_…

5ed4bc4

…credentials

ThisIsMissEm force-pushed the feat/client-authentication-mechanisms branch from 62b0634 to 5ed4bc4 Compare April 21, 2025 18:06

ThisIsMissEm added 3 commits April 21, 2025 20:15

Add more tests for client authentication methods

4e37539

More improvements to client authentication, renaming mechanism to method

f2fe3f9

Add more tests for client authentication methods

8f47f71

ThisIsMissEm commented Apr 21, 2025

View reviewed changes

Add spec for client_credentials as a lambda / callable

e63e18c

ThisIsMissEm commented Apr 21, 2025

View reviewed changes

lib/doorkeeper/config.rb Outdated Show resolved Hide resolved

ThisIsMissEm commented Apr 21, 2025

View reviewed changes

Improve client authentication method specs and assert request_paramet…

12b6110

…ers for client_secret_post

ThisIsMissEm commented Apr 21, 2025

View reviewed changes

Change message for client_credentials conversion

3084791

ThisIsMissEm force-pushed the feat/client-authentication-mechanisms branch from 121d65f to 3084791 Compare April 21, 2025 19:09

ThisIsMissEm mentioned this pull request Apr 21, 2025

Feat: Implement oauth authorization server metadata #1737

Open

Improve client authentication registry and add specs

d302bb5

ThisIsMissEm changed the title ~~Add Client Authentication Mechanisms Registry~~ Add Client Authentication Methods Registry Apr 21, 2025

ThisIsMissEm commented Apr 21, 2025

View reviewed changes

Add an additional spec to assert the return value of config.client_au…

26a1d8a

…thentication_methods

ThisIsMissEm commented Apr 21, 2025

View reviewed changes

This was referenced Apr 23, 2025

Support OAuth RFC - JWT for Client Authentication and Authorization Grants #1768

Open

Add support for OAuth Public Clients with Token Expiry mastodon/mastodon#30329

Draft

nbulaj requested a review from Copilot April 24, 2025 12:29

Copilot AI reviewed Apr 24, 2025

View reviewed changes

Uh oh!

Add Client Authentication Methods Registry #1772

Are you sure you want to change the base?

Add Client Authentication Methods Registry #1772

Uh oh!

Conversation

ThisIsMissEm commented Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Implementation Notes

Other Information

Uh oh!

Uh oh!

Uh oh!

ThisIsMissEm Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThisIsMissEm Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThisIsMissEm Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ThisIsMissEm commented Apr 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ThisIsMissEm Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

ThisIsMissEm commented Apr 20, 2025 •

edited

Loading

ThisIsMissEm Apr 20, 2025 •

edited

Loading

ThisIsMissEm Apr 20, 2025 •

edited

Loading

ThisIsMissEm Jul 11, 2025 •

edited

Loading

ThisIsMissEm Apr 21, 2025 •

edited

Loading

ThisIsMissEm Apr 21, 2025 •

edited

Loading