-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Feat: Implement oauth authorization server metadata #1737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Doorkeeper | ||
| class DiscoveryController < Doorkeeper::ApplicationMetalController | ||
| def show | ||
| headers.merge!(discovery_response.headers) | ||
| render json: discovery_response.body, | ||
| status: discovery_response.status | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def discovery_response | ||
| @discovery_response ||= Doorkeeper::OAuth::DiscoveryResponse.new( | ||
| root_url, | ||
| -> (**args) { url_for(**args) } | ||
| ) | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -321,6 +321,9 @@ def configure_secrets_for(type, using:, fallback:) | |
| # | ||
| option :realm, default: "Doorkeeper" | ||
|
|
||
| # Issuer URL for the OAuth Authorization Server | ||
| option :issuer, default: nil | ||
|
Comment on lines
+324
to
+325
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically this is maybe OpenID more, but OAuth 2.0 does have a spec for it, see: #1720 |
||
|
|
||
| # Forces the usage of the HTTPS protocol in non-native redirect uris | ||
| # (enabled by default in non-development environments). OAuth2 | ||
| # delegates security in communication to the HTTPS protocol so it is | ||
|
|
@@ -397,6 +400,10 @@ def configure_secrets_for(type, using:, fallback:) | |
| option :application_class, | ||
| default: "Doorkeeper::Application" | ||
|
|
||
| # Allows setting a hash of custom data on the OAuth 2.0 Authorization | ||
| # Server Metadata discovery response. | ||
| option :custom_discovery_data, default: {} | ||
|
Comment on lines
+403
to
+405
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is intended for applications to use, e.g., Mastodon where we want to set |
||
|
|
||
| # Allows to set blank redirect URIs for Applications in case | ||
| # server configured to use URI-less grant flows. | ||
| # | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,115 @@ | ||||||
| # frozen_string_literal: true | ||||||
|
|
||||||
| module Doorkeeper | ||||||
| module OAuth | ||||||
| class DiscoveryResponse < BaseResponse | ||||||
| def initialize(root_url, url_builder) | ||||||
| @root_url = root_url | ||||||
| @url_builder = url_builder | ||||||
| end | ||||||
|
|
||||||
| def body | ||||||
| @body ||= { | ||||||
| issuer: issuer || @root_url, | ||||||
| authorization_endpoint: authorization_endpoint, | ||||||
| token_endpoint: token_endpoint, | ||||||
| revocation_endpoint: revocation_endpoint, | ||||||
| userinfo_endpoint: userinfo_endpoint, | ||||||
| scopes_supported: scopes_supported, | ||||||
| response_types_supported: response_types_supported, | ||||||
| response_modes_supported: response_modes_supported, | ||||||
| grant_types_supported: grant_types_supported, | ||||||
| token_endpoint_auth_methods_supported: token_endpoint_auth_methods_supported, | ||||||
| code_challenge_methods_supported: code_challenge_methods_supported, | ||||||
| }.merge(custom_discovery_data) | ||||||
| end | ||||||
|
|
||||||
| def status | ||||||
| :ok | ||||||
| end | ||||||
|
|
||||||
| def headers | ||||||
| { | ||||||
| "Cache-Control" => "public", | ||||||
| "Content-Type" => "application/json; charset=utf-8", | ||||||
| } | ||||||
| end | ||||||
|
|
||||||
| private | ||||||
|
|
||||||
| def config | ||||||
| @config ||= Doorkeeper.configuration | ||||||
| end | ||||||
|
|
||||||
| def url_for(**args) | ||||||
| @url_builder.call(**args) | ||||||
| end | ||||||
|
|
||||||
| def custom_discovery_data | ||||||
| config.custom_discovery_data.symbolize_keys | ||||||
| end | ||||||
|
|
||||||
| def issuer | ||||||
| config.issuer | ||||||
| end | ||||||
|
|
||||||
| def authorization_endpoint | ||||||
| mapping = Doorkeeper::Rails::Routes.mapping[:authorizations] || {} | ||||||
|
|
||||||
| url_for( | ||||||
| controller: mapping[:controllers] || "doorkeeper/authorizations", | ||||||
| action: 'new' | ||||||
| ) | ||||||
| end | ||||||
|
|
||||||
| def token_endpoint | ||||||
| mapping = Doorkeeper::Rails::Routes.mapping[:tokens] || {} | ||||||
|
|
||||||
| url_for( | ||||||
| controller: mapping[:controllers] || "doorkeeper/tokens", | ||||||
| action: 'create' | ||||||
| ) | ||||||
| end | ||||||
|
|
||||||
| def userinfo_endpoint | ||||||
| nil | ||||||
| end | ||||||
|
|
||||||
| def revocation_endpoint | ||||||
| mapping = Doorkeeper::Rails::Routes.mapping[:tokens] || {} | ||||||
|
|
||||||
| url_for( | ||||||
| controller: mapping[:controllers] || "doorkeeper/tokens", | ||||||
| action: 'revoke' | ||||||
| ) | ||||||
| end | ||||||
|
|
||||||
| def scopes_supported | ||||||
| config.scopes.to_a | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be better as:
Suggested change
Might depend on how |
||||||
| end | ||||||
|
|
||||||
| def response_types_supported | ||||||
| config.authorization_response_types | ||||||
| end | ||||||
|
|
||||||
| def response_modes_supported | ||||||
| config.authorization_response_flows.flat_map(&:response_mode_matches).uniq | ||||||
| end | ||||||
|
|
||||||
| def grant_types_supported | ||||||
| grant_types_supported = config.grant_flows.dup | ||||||
| grant_types_supported << 'refresh_token' if !!config.refresh_token_enabled? | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is technically broken, because
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
endOr something. |
||||||
| grant_types_supported | ||||||
| end | ||||||
|
|
||||||
| # FIXME: https://github.com/doorkeeper-gem/doorkeeper/pull/1770 | ||||||
| def token_endpoint_auth_methods_supported | ||||||
| %w(none client_secret_basic client_secret_post) | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be fixed by #1772 |
||||||
| end | ||||||
|
|
||||||
| def code_challenge_methods_supported | ||||||
| config.pkce_code_challenge_methods_supported | ||||||
| end | ||||||
| end | ||||||
| end | ||||||
| end | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class CustomDiscoveryController < ::ApplicationController | ||
| def show | ||
| render nothing: true | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "spec_helper" | ||
|
|
||
| RSpec.describe "Discovery endpoint" do | ||
| before do | ||
| default_scopes_exist :read | ||
| optional_scopes_exist :write, :publish | ||
| end | ||
|
|
||
| it "returns json" do | ||
| get "/.well-known/oauth-authorization-server" | ||
|
|
||
| response_status_should_be(200) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add a content-type assertion here as well. |
||
|
|
||
| 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) | ||
|
Comment on lines
+16
to
+35
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very similar to what we did for a one-off in our application:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| end | ||
|
|
||
| context 'With custom issuer' do | ||
| before do | ||
| config_is_set(:issuer, "http://example.test/") | ||
| end | ||
|
|
||
| it "returns json" do | ||
| get "/.well-known/oauth-authorization-server" | ||
|
|
||
| response_status_should_be(200) | ||
| expect(json_response["issuer"]).to eq "http://example.test/" | ||
| end | ||
| end | ||
|
|
||
| context 'With code challenge methods' do | ||
| before do | ||
| config_is_set(:pkce_code_challenge_methods, ["S256"]) | ||
| end | ||
|
|
||
| it "returns json" do | ||
| get "/.well-known/oauth-authorization-server" | ||
|
|
||
| response_status_should_be(200) | ||
| expect(json_response["code_challenge_methods_supported"]).to eq(["S256"]) | ||
| end | ||
| end | ||
|
|
||
| context 'With custom discovery attributes' do | ||
| before do | ||
| config_is_set(:custom_discovery_data, { | ||
| userinfo_endpoint: '/userinfo', | ||
| app_registration_url: 'app_registration_url.example' | ||
| }) | ||
| end | ||
|
|
||
| it "returns json" do | ||
| get "/.well-known/oauth-authorization-server" | ||
|
|
||
| response_status_should_be(200) | ||
| expect(json_response["userinfo_endpoint"]).to eq("/userinfo") | ||
| expect(json_response["app_registration_url"]).to eq("app_registration_url.example") | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not particularly keen on this, but including UrlFor or similar in the
Doorkeeper::OAuth::DiscoveryResponseclass 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.