Support RefreshToken and Include user claims in id token#24
Conversation
willtcarey
left a comment
willtcarey
left a comment
There was a problem hiding this comment.
Quite a few changes to make here. Looks like this is fairly hardcoded for a specific use case. We want to make sure we're adhering to the spec here where possible (even if not all of the current parts do!)
| def show | ||
| render json: AccountToUserInfo.new.(current_token.authorization.account, current_token.authorization.scopes) | ||
|
|
||
| render json: AccountToUserInfo.new.(current_token.authorization.account, current_token.authorization.scopes, current_token.authorization.nonce) |
There was a problem hiding this comment.
We don't need to return the nonce in the user info request
| render json: AccountToUserInfo.new.(current_token.authorization.account, current_token.authorization.scopes, current_token.authorization.nonce) | |
| render json: AccountToUserInfo.new.(current_token.authorization.account, current_token.authorization.scopes) |
There was a problem hiding this comment.
When integrating with ABM (apple business manage), nonce is required
There was a problem hiding this comment.
I can't find that in the spec. I'd prefer to remain spec compliant, but I would be willing to introduce some quirks provided they are documented
| scopes: scopes, | ||
| account: oidc_current_account | ||
| account: oidc_current_account, | ||
| issuer: request.base_url |
There was a problem hiding this comment.
I'd prefer to keep this as the configured issuer rather that being request specific
There was a problem hiding this comment.
Other IDPs such as Authentik implement it this way as well, for adapting to multi-domain scenarios
There was a problem hiding this comment.
This file is not excluded. Previously, an authorization page was added here to handle the issue of POST requests not redirecting
There was a problem hiding this comment.
The next commit will delete this file
| type = oauth_request.response_type | ||
| type.nil? && oauth_request.unsupported_response_type! | ||
| case type | ||
| when :code | ||
| @requested_type=:code | ||
| when :id_token | ||
| @requested_type=:id_token | ||
| when ->(ary) do ary.include?(:code) && ary.include?(:id_token) end | ||
| @requested_type=:hybrid | ||
| # when type.include?("token") | ||
| # @response_type=:token | ||
| else | ||
| oauth_request.unsupported_response_type! | ||
| end |
There was a problem hiding this comment.
Could we turn this whole method into a single case statement?
| Rack::OAuth2::AccessToken::Bearer.new( | ||
| access_token: token, | ||
| expires_in: (expires_at - Time.now).to_i | ||
| def to_bearer_token(with_refresh_token) |
There was a problem hiding this comment.
Keyword argument for clarity please
| def to_bearer_token(with_refresh_token) | |
| def to_bearer_token(with_refresh_token:) |
| @@ -1,4 +1,5 @@ | |||
| # frozen_string_literal: true | |||
| require 'openid_connect/response_object/id_token/id_token_with_user_info' | |||
There was a problem hiding this comment.
Let's not require files here, let's do it in the engine
| extra_claims = { | ||
| email: account.send(OIDCProvider.account_email), | ||
| email_verified: true, | ||
| given_name: account.send(OIDCProvider.account_given_name), | ||
| family_name: account.send(OIDCProvider.account_family_name), | ||
| } |
There was a problem hiding this comment.
This hardcodes these specific claims, but we could have introduced other claims through scopes
There was a problem hiding this comment.
I'll see how to implement this feature using scopes
| with_refresh_token = authorization.scopes.include?('offline_access') | ||
| res.access_token = authorization.access_token.to_bearer_token(with_refresh_token) | ||
| res.id_token = authorization.id_token.to_jwt if authorization.scopes.include?('openid') | ||
| when :refresh_token |
There was a problem hiding this comment.
Refresh token requests need to have their client verified in the same way as authorization code requests
| mattr_accessor :account_email | ||
| @@account_identifier = :email | ||
|
|
||
| mattr_accessor :account_given_name | ||
| @@account_identifier = :given_name | ||
|
|
||
| mattr_accessor :account_family_name | ||
| @@account_identifier = :family_name | ||
|
|
||
| # Include User claims from scopes in the id_token, for applications that don't access the userinfo endpoint. | ||
| mattr_accessor :include_user_claims_in_id_token | ||
| @@include_user_claims_in_id_token = false |
There was a problem hiding this comment.
We should use scopes to define this info
| # To use a debugger | ||
| # gem 'byebug', group: [:development, :test] | ||
|
|
||
| gem "openid_connect" No newline at end of file |
There was a problem hiding this comment.
This is specified by the gemspec
2 participants
Add feature :