-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add Client Authentication Methods Registry #1772
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 1 commit
3fbe2f8
7fdad73
c1e95e9
93b2d41
42a9d10
fafad38
1595444
5ed4bc4
4e37539
f2fe3f9
8f47f71
e63e18c
12b6110
3084791
d302bb5
26a1d8a
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,26 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "doorkeeper/client_authentication/flow" | ||
| require "doorkeeper/client_authentication/registry" | ||
|
|
||
| module Doorkeeper | ||
| module ClientAuthentication | ||
| extend Registry | ||
|
|
||
| register( | ||
| :none, | ||
| mechanism: Doorkeeper::ClientAuthentication::Mechanisms::None, | ||
| authenticates_client: false | ||
| ) | ||
|
|
||
| register( | ||
| :client_secret_post, | ||
| mechanism: Doorkeeper::ClientAuthentication::Mechanisms::ClientSecretPost, | ||
| ) | ||
|
|
||
| register( | ||
| :client_secret_basic, | ||
| mechanism: Doorkeeper::ClientAuthentication::Mechanisms::ClientSecretBasic, | ||
| ) | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Doorkeeper | ||
| module ClientAuthentication | ||
| class Mechanism | ||
| attr_reader :name, :mechanism | ||
|
|
||
| def initialize(name, **options) | ||
| @name = name | ||
| @mechanism = options[:mechanism] | ||
| @authenticates_client = options.key?(:authenticates_client) ? options[:authenticates_client] : true | ||
| end | ||
|
|
||
| def authenticates_client? | ||
| !!@authenticates_client | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Doorkeeper | ||
| module ClientAuthentication | ||
| module Mechanisms | ||
| class ClientSecretBasic | ||
| def self.matches_request?(request) | ||
| request.authorization.present? && request.authorization.downcase.start_with?('basic') | ||
| end | ||
|
|
||
| def authenticate(request) | ||
| value = request.authorization.to_s.split(" ", 2).second | ||
| client_id, client_secret = Base64.decode64(value).split(':', 2) | ||
|
|
||
| return unless client_id.present? && client_secret.present? | ||
|
|
||
| Doorkeeper::ClientAuthentication::Credentials.new(client_id, client_secret) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Doorkeeper | ||
| module ClientAuthentication | ||
| module Mechanisms | ||
| class ClientSecretPost | ||
| def self.matches_request?(request) | ||
| request.method.upcase === "POST" | ||
| end | ||
|
|
||
| def authenticate(request) | ||
| client_id = request.request_parameters[:client_id] | ||
| client_secret = request.request_parameters[:client_secret] | ||
|
|
||
| return unless client_id.present? && client_secret.present? | ||
|
|
||
| Doorkeeper::ClientAuthentication::Credentials.new( | ||
| client_id, | ||
| client_secret | ||
| ) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Doorkeeper | ||
| module ClientAuthentication | ||
| module Mechanisms | ||
| class None | ||
| def self.matches_request?(request) | ||
| !request.authorization && request.request_parameters[:client_id] && !request.request_parameters[:client_secret] | ||
| end | ||
|
|
||
| def authenticate(request) | ||
| Doorkeeper::ClientAuthentication::Credentials.new( | ||
| request.request_parameters[:client_id], nil | ||
| ) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Doorkeeper | ||
| module GrantFlow | ||
| module Registry | ||
| mattr_accessor :mechanisms | ||
| self.mechanisms = {} | ||
|
|
||
| # Allows to register custom OAuth client authentication mechanism so that | ||
| # Doorkeeper could recognize and process it. | ||
| # | ||
| def register(name_or_mechanism, **options) | ||
| unless name_or_mechanism.is_a?(Doorkeeper::ClientAuthentication::Mechanism) | ||
| name_or_mechanism = Doorkeeper::ClientAuthentication::Mechanism.new(name_or_mechanism, **options) | ||
| end | ||
|
|
||
| name_key = name_or_mechanism.name.to_sym | ||
|
|
||
| if mechanisms.key?(name_key) | ||
| ::Kernel.warn <<~WARNING | ||
| [DOORKEEPER] '#{name_key}' client authentication strategy is already registered and will be overridden | ||
| in #{caller(1..1).first} | ||
| WARNING | ||
| end | ||
|
|
||
| mechanisms[name_key] = name_or_mechanism | ||
| end | ||
|
|
||
| # [NOTE]: make it to use #fetch after removing fallbacks | ||
| def get(name) | ||
| mechanisms[name.to_sym] | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -253,6 +253,7 @@ def configure_secrets_for(type, using:, fallback:) | |
| option :orm, default: :active_record | ||
| option :native_redirect_uri, default: "urn:ietf:wg:oauth:2.0:oob", deprecated: true | ||
| option :grant_flows, default: %w[authorization_code client_credentials] | ||
| option :client_authentication, default: %w[client_secret_basic client_secret_post none] | ||
|
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. Note: this has to be an array instead of multiple arguments: Doorkeeper.configure do
client_authentication [ :client_secret_basic, :client_secret_post, :none ]
endInstead of Doorkeeper.configure do
client_authentication :client_secret_basic, :client_secret_post, :none
endWe should maybe consider adding an ability to declare a
This comment was marked as outdated.
Sorry, something went wrong. |
||
| option :pkce_code_challenge_methods, default: %w[plain S256] | ||
| option :handle_auth_errors, default: :render | ||
| option :token_lookup_batch_size, default: 10_000 | ||
|
|
@@ -579,10 +580,17 @@ def pkce_code_challenge_methods_supported | |
| pkce_code_challenge_methods | ||
| end | ||
|
|
||
| # Deprecated? | ||
| def client_credentials_methods | ||
| @client_credentials_methods ||= %i[from_basic from_params] | ||
| end | ||
|
|
||
| def client_authentication_mechanisms | ||
| @client_authentication_mechanisms ||= client_authentication.map do |name| | ||
| Doorkeeper::ClientAuthentication.get(name) | ||
| end.compact | ||
| end | ||
|
|
||
| def access_token_methods | ||
| @access_token_methods ||= %i[ | ||
| from_bearer_authorization | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
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 could arguably change the
Member
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 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
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. 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 |
||
| 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)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,11 @@ def initialize(context) | |
| @context = context | ||
| end | ||
|
|
||
| def client_authentication_request(strategy) | ||
| klass = Request.client_authentication_mechanism(context.request) | ||
| klass.new(self) | ||
| end | ||
|
|
||
|
Comment on lines
+11
to
+14
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. 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.
Member
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. BTW I like such utility methods, in some contexts they can say more then their implementations. I'm OK for having such
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 actually needs to be pluralized per the comment about multiple authentication methods being present. |
||
| def authorization_request(strategy) | ||
| klass = Request.authorization_strategy(strategy) | ||
| klass.new(self) | ||
|
|
@@ -37,8 +42,7 @@ def resource_owner | |
| end | ||
|
|
||
| def credentials | ||
|
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 used by the def client
@client ||= OAuth::Client.find(credentials.uid)
endand 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)
endi.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.
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. Now that I think about it, maybe as it is is better? You only get a |
||
| methods = Doorkeeper.config.client_credentials_methods | ||
| @credentials ||= OAuth::Client::Credentials.from_request(context.request, *methods) | ||
| @credentials ||= client_authentication_request.authenticate(context.request) | ||
| end | ||
| end | ||
| end | ||
Uh oh!
There was an error while loading. Please reload this page.