Feat: Implement oauth authorization server metadata #1737

ThisIsMissEm · 2024-10-02T22:00:38Z

Summary

This is early work on supporting #1587, it's currently based off an older commit from #1732, #1735, and #1736 since those allow me to work semi-productively on the doorkeeper codebase.

I'm unsure if we'd really need a custom controller for discovery, as I think the initializer for configuration should probably handle the "adding extra stuff to the .well-known/oauth-authorization-server endpoint, instead of actually overriding the controller.

I don't think it makes sense for the scope option with doorkeeper to affect the .well-known/oauth-authorization-server endpoint, however, nesting that within a scope can work (allowing multiple oauth providers on the one domain, similar to how keycloak works)

I'm still to figure out all the necessary stuff for actually building out the data for response. I think I'm probably inclined to create a Doorkeeper::OAuth::DiscoveryResponse class, and a custom_discovery_response configuration block option, then have the controller just apply that class.

There's also the question of "should this be rename-able by other plugins?", e.g., OIDC that uses .well-known/openid-configuration instead of .well-known/oauth-authorization-server — I'd probably guess maybe no, and instead the OIDC plugin could just remove the .well-known/oauth-authorization-server route and add its own which is calling a subclass of Doorkeeper::OAuth::DiscoveryResponse?

ThisIsMissEm · 2024-10-02T22:12:36Z

@nbulaj @ransombriggs would love some feedback on my implementation proposal in the PR description — I think that's the right way forwards, but haven't yet begun to implement.

ransombriggs · 2024-10-03T14:46:41Z

I would give feedback but I am not familiar with https://www.rfc-editor.org/rfc/rfc8414 and really just a bystander on this project. I got it to do what I needed and then was moved onto other things.

nbulaj · 2024-10-30T10:09:26Z

I have some feeling that the work should be done inside https://github.com/doorkeeper-gem/doorkeeper-openid_connect 🤔

I remember it handles all these stuff around Discovery and related. Have to recheck deeply

ThisIsMissEm · 2024-10-30T10:19:26Z

I have some feeling that the work should be done inside https://github.com/doorkeeper-gem/doorkeeper-openid_connect 🤔

I remember it handles all these stuff around Discovery and related. Have to recheck deeply

RFC 8414 is part of OAuth and is separate from OIDC's Discovery, but is compatible in some ways.

RFC 8414 uses /.well-known/oauth-authorization-server and OIDC Discovery uses /.well-known/openid_configuration (iirc), but the contents of the documents is largely the same. These differences are noted in RFC 8414

ThisIsMissEm · 2025-04-19T21:49:55Z

app/controllers/doorkeeper/discovery_controller.rb

+        root_url,
+        -> (**args) { url_for(**args) }


I'm not particularly keen on this, but including UrlFor or similar in the Doorkeeper::OAuth::DiscoveryResponse class didn't work as I'd expect it to, and I need the url builder in order to generate the absolute URLs for the authorization server metadata.

ThisIsMissEm · 2025-04-19T21:50:41Z

lib/doorkeeper/config.rb

+    # Issuer URL for the OAuth Authorization Server
+    option :issuer,                         default: nil


Technically this is maybe OpenID more, but OAuth 2.0 does have a spec for it, see: #1720

ThisIsMissEm · 2025-04-19T21:51:31Z

lib/doorkeeper/config.rb

+    # Allows setting a hash of custom data on the OAuth 2.0 Authorization
+    # Server Metadata discovery response.
+    option :custom_discovery_data, default: {}


This is intended for applications to use, e.g., Mastodon where we want to set service_documentation and app_registration_url properties in the metadata.

ThisIsMissEm · 2025-04-19T21:54:10Z

lib/doorkeeper/oauth/discovery_response.rb

+
+      def grant_types_supported
+        grant_types_supported = config.grant_flows.dup
+        grant_types_supported << 'refresh_token' if !!config.refresh_token_enabled?


This is technically broken, because use_refresh_token configuration can be a block/proc, which accepts server and context which, well, I don't think we have here, nor would it be possible to have?

Thinking about this more, if we did drastically change how refresh tokens work, as suggested in #1771, then we could have options like:

Doorkeeper.configure do orm :active_record # Enable using them in general use_refresh_tokens true # Set the expiry for refresh tokens: refresh_token_expires_in 30.days # Allow refresh tokens to only be used by specific clients: allow_refresh_tokens_for do |client, access_grant| access_grant.scopes.exists?('offline_access') end end

Or something.

ThisIsMissEm · 2025-04-19T21:55:51Z

spec/requests/endpoints/discovery_spec.rb

+    expect(json_response).to have_key("issuer")
+    expect(json_response).to have_key("authorization_endpoint")
+    expect(json_response).to have_key("token_endpoint")
+    expect(json_response).to have_key("revocation_endpoint")
+    expect(json_response).to have_key("userinfo_endpoint")
+    expect(json_response).to have_key("scopes_supported")
+    expect(json_response).to have_key("response_types_supported")
+    expect(json_response).to have_key("response_modes_supported")
+    expect(json_response).to have_key("grant_types_supported")
+    expect(json_response).to have_key("token_endpoint_auth_methods_supported")
+    expect(json_response).to have_key("code_challenge_methods_supported")
+
+    expect(json_response["issuer"]).to be_a(String)
+
+    expect(json_response["scopes_supported"]).to be_a(Array)
+    expect(json_response["response_types_supported"]).to be_a(Array)
+    expect(json_response["response_modes_supported"]).to be_a(Array)
+    expect(json_response["grant_types_supported"]).to be_a(Array)
+    expect(json_response["token_endpoint_auth_methods_supported"]).to be_a(Array)
+    expect(json_response["code_challenge_methods_supported"]).to be_a(Array)


I'm not sure if I should really test logic of the discovery response here, so instead I'm just asserting properties and types for things that must be arrays.

Very similar to what we did for a one-off in our application:

get oauth_discovery_path expect(response).to have_http_status(200) expect(response.content_type).to eq("application/json; charset=utf-8") object = JSON.parse(response.body) expect(object.dig("authorization_endpoint")).to eq("http://www.example.com/oauth/authorize") expect(object.dig("token_endpoint")).to eq("http://www.example.com/oauth/token") expect(object.dig("revocation_endpoint")).to eq("http://www.example.com/oauth/revoke") ...

Yeah, similar to the Mastodon one as well: https://github.com/mastodon/mastodon/blob/main/spec/requests/well_known/oauth_metadata_spec.rb

I'm just not sure I want to be validating the values or not here, since the paths can be configured with doorkeeper.

ThisIsMissEm · 2025-04-19T21:56:46Z

@nbulaj @ransombriggs I think this is ready for a look, I think it works okay for now? Not sure if we need to do something else or not, there's a few things I've noted in the comments above.

ThisIsMissEm · 2025-04-20T19:20:45Z

spec/requests/endpoints/discovery_spec.rb

+  it "returns json" do
+    get "/.well-known/oauth-authorization-server"
+
+    response_status_should_be(200)


We should add a content-type assertion here as well.

ThisIsMissEm · 2025-04-20T19:22:30Z

lib/doorkeeper/oauth/discovery_response.rb

+      end
+
+      def scopes_supported
+        config.scopes.to_a


Might be better as:

Suggested change

config.scopes.to_a

config.scopes.map(&:to_s)

Might depend on how Scopes implements to_a

ThisIsMissEm · 2025-04-21T19:42:57Z

lib/doorkeeper/oauth/discovery_response.rb

+
+      # FIXME: https://github.com/doorkeeper-gem/doorkeeper/pull/1770
+      def token_endpoint_auth_methods_supported
+        %w(none client_secret_basic client_secret_post)


This will be fixed by #1772

alex-ross · 2025-10-19T22:12:03Z

Is this kind of forgotten or is it someway to enable a .well-known/oauth-authorization-server trough this gem?

ThisIsMissEm · 2025-10-19T23:35:19Z

@alex-ross this isn't forgotten, work is just progressing more slowly because of time constraints

ThisIsMissEm changed the title ~~Feat/implement oauth authorization server metadata~~ Feat: Implement oauth authorization server metadata Oct 2, 2024

ThisIsMissEm mentioned this pull request Apr 9, 2025

Add API authentication using OAuth2 Client Credentials flow manyfold3d/manyfold#3951

Merged

Floppy mentioned this pull request Apr 9, 2025

Implement RFC 8414 for OAuth discovery manyfold3d/manyfold#3965

Open

ThisIsMissEm mentioned this pull request Apr 16, 2025

Implement helper method to retrieve token_endpoint_auth_methods #1770

Closed

ThisIsMissEm added 3 commits April 19, 2025 22:19

Initial .well-known/oauth-authorization-server endpoint

fe5c4fe

Use custom controller for discovery

f55ca15

Finish implementing discovery responses

9266140

ThisIsMissEm force-pushed the feat/implement-oauth-authorization-server-metadata branch from a9b73f9 to 9266140 Compare April 19, 2025 21:48

ThisIsMissEm commented Apr 19, 2025

View reviewed changes

ThisIsMissEm marked this pull request as ready for review April 19, 2025 21:56

ThisIsMissEm commented Apr 20, 2025

View reviewed changes

ThisIsMissEm commented Apr 21, 2025

View reviewed changes

		# Issuer URL for the OAuth Authorization Server
		option :issuer, default: nil

Uh oh!

Feat: Implement oauth authorization server metadata #1737

Are you sure you want to change the base?

Feat: Implement oauth authorization server metadata #1737

Conversation

ThisIsMissEm commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

ThisIsMissEm commented Oct 2, 2024

Uh oh!

ransombriggs commented Oct 3, 2024

Uh oh!

nbulaj commented Oct 30, 2024

Uh oh!

ThisIsMissEm commented Oct 30, 2024

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

ThisIsMissEm commented Apr 19, 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!

alex-ross commented Oct 19, 2025

Uh oh!

ThisIsMissEm commented Oct 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ThisIsMissEm commented Oct 2, 2024 •

edited

Loading

ThisIsMissEm Apr 20, 2025 •

edited

Loading