-
Notifications
You must be signed in to change notification settings - Fork 5
Support RefreshToken and Include user claims in id token #24
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: master
Are you sure you want to change the base?
Changes from 41 commits
891e2f0
9d84b44
ca29195
dd2f1d8
3a04d75
b293c69
4b8cf65
a2cb86b
b6c7e59
f14be0a
b81e359
9013f59
643bd96
0f5a903
ef1a4dc
78aa6d8
530a3db
de69c33
cd80ae9
ee79665
3bc0374
60ce5d8
64f2012
14d8618
6ba932a
7ae9bb4
f94b515
6e6fef8
097b1b8
d690e21
6636c14
ab8375c
069092f
0bbd2c8
38b6836
bf01e51
7574368
811ddac
174fe38
59ac78a
7232735
a12ccb0
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 |
|---|---|---|
|
|
@@ -13,3 +13,5 @@ gemspec | |
|
|
||
| # To use a debugger | ||
| # gem 'byebug', group: [:development, :test] | ||
|
|
||
| gem "openid_connect" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,8 @@ module OIDCProvider | |
| class AuthorizationsController < ApplicationController | ||
| include Concerns::ConnectEndpoint | ||
|
|
||
| skip_before_action :verify_authenticity_token | ||
|
|
||
| before_action :require_oauth_request | ||
| before_action :require_response_type_code | ||
| before_action :require_client | ||
|
|
@@ -15,13 +17,11 @@ def create | |
|
|
||
| authorization = build_authorization_with(requested_scopes) | ||
|
|
||
| oauth_response.code = authorization.code | ||
| oauth_response.code = authorization.code if @requested_type==:code or @requested_type == :hybrid | ||
| oauth_response.id_token = authorization.id_token.to_jwt if @requested_type==:id_token or @requested_type == :hybrid | ||
| oauth_response.redirect_uri = @redirect_uri | ||
| oauth_response.approve! | ||
| redirect_to oauth_response.location, allow_other_host: true | ||
|
|
||
| # If we ever need to support denied authorizations that is done by: | ||
| # oauth_request.access_denied! | ||
| redirect_to oauth_response.location,allow_other_host: true | ||
| end | ||
|
|
||
| private | ||
|
|
@@ -31,7 +31,8 @@ def build_authorization_with(scopes) | |
| client_id: @client.identifier, | ||
| nonce: oauth_request.nonce, | ||
| scopes: scopes, | ||
| account: oidc_current_account | ||
| account: oidc_current_account, | ||
| issuer: request.base_url | ||
|
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'd prefer to keep this as the configured issuer rather that being request specific
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. Other IDPs such as |
||
| ) | ||
| end | ||
|
|
||
|
|
@@ -46,9 +47,22 @@ def requested_scopes | |
| helper_method :requested_scopes | ||
|
|
||
| def require_response_type_code | ||
| return if oauth_request.response_type == :code | ||
| 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 | ||
|
Comment on lines
+50
to
+63
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. Could we turn this whole method into a single case statement? |
||
|
|
||
|
|
||
| oauth_request.unsupported_response_type! | ||
| end | ||
|
|
||
| def reset_login_if_necessary | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,7 +3,8 @@ class UserInfosController < ApplicationController | |||||
| before_action :require_access_token | ||||||
|
|
||||||
| 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) | ||||||
|
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. We don't need to return the nonce in the user info request
Suggested change
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. When integrating with ABM (apple business manage), nonce is required
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 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 |
||||||
| end | ||||||
| end | ||||||
| end | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,11 +7,37 @@ class AccessToken < ApplicationRecord | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| attribute :token, :string, default: -> { SecureRandom.hex 32 } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| attribute :expires_at, :datetime, default: -> { 1.hours.from_now } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def to_bearer_token | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Rack::OAuth2::AccessToken::Bearer.new( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| access_token: token, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expires_in: (expires_at - Time.now).to_i | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def to_bearer_token(with_refresh_token) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. Keyword argument for clarity please
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if with_refresh_token | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Rack::OAuth2::AccessToken::Bearer.new( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| access_token: token, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expires_in: (expires_at - Time.now).to_i, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| refresh_token: (get_refresh_token(authorization.client_id,authorization.scopes)||generate_refresh_token(authorization.client_id,authorization.scopes)).token, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| scope: authorization.scopes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Rack::OAuth2::AccessToken::Bearer.new( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| access_token: token, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expires_in: (expires_at - Time.now).to_i, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| scope: authorization.scopes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+11
to
+24
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.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def get_refresh_token(client_id,scopes) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| RefreshToken | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .valid | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .where(client_id: client_id, revoked_at: nil,scopes:scopes) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .first | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+29
to
+32
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. This is not scoped to a specific authorization |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def generate_refresh_token(client_id,scopes) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| RefreshToken.create!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| client_id: client_id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| scopes: scopes, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| authorization: authorization | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,14 @@ | ||
| module OIDCProvider | ||
| class Authorization < ApplicationRecord | ||
| belongs_to :account, class_name: OIDCProvider.account_class | ||
| has_one :access_token | ||
| has_one :access_token, dependent: :destroy | ||
| has_one :id_token | ||
|
|
||
| scope :valid, -> { where(arel_table[:expires_at].gteq(Time.now.utc)) } | ||
|
|
||
| attribute :code, :string, default: -> { SecureRandom.hex 32 } | ||
| attribute :expires_at, :datetime, default: -> { 5.minutes.from_now } | ||
| attribute :issuer, :string | ||
|
|
||
| serialize :scopes, coder: JSON | ||
|
|
||
|
|
@@ -20,6 +21,11 @@ def access_token | |
| super || expire! && generate_access_token! | ||
| end | ||
|
|
||
| def refresh! | ||
| access_token = create_access_token! | ||
| access_token.save! | ||
|
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. The access token will already be saved |
||
| end | ||
|
|
||
| def id_token | ||
| super || generate_id_token! | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| # frozen_string_literal: true | ||
| require 'openid_connect/response_object/id_token/id_token_with_user_info' | ||
|
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. Let's not require files here, let's do it in the engine |
||
|
|
||
| module OIDCProvider | ||
| class IdToken < ApplicationRecord | ||
|
|
@@ -11,14 +12,29 @@ class IdToken < ApplicationRecord | |
| delegate :account, to: :authorization | ||
|
|
||
| def to_response_object | ||
| OpenIDConnect::ResponseObject::IdToken.new( | ||
| iss: OIDCProvider.issuer, | ||
| base_claims ={ | ||
| iss: authorization.issuer, | ||
| sub: account.send(OIDCProvider.account_identifier), | ||
| aud: authorization.client_id, | ||
| nonce: nonce, | ||
| exp: expires_at.to_i, | ||
| iat: created_at.to_i | ||
| ) | ||
| iat: created_at.to_i, | ||
| auth_time:created_at.to_i, | ||
| amr: ['pwd'] | ||
| } | ||
| if OIDCProvider.include_user_claims_in_id_token | ||
| 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), | ||
| } | ||
|
Comment on lines
+26
to
+31
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. This hardcodes these specific claims, but we could have introduced other claims through scopes
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'll see how to implement this feature using scopes |
||
| OpenIDConnect::ResponseObject::IdTokenWithUserInfo.new(**extra_claims, **base_claims) | ||
| else | ||
| OpenIDConnect::ResponseObject::IdToken.new( | ||
| **base_claims | ||
| ) | ||
| end | ||
| end | ||
|
|
||
| def to_jwt | ||
|
|
@@ -30,7 +46,6 @@ def to_jwt | |
| class << self | ||
| def config | ||
| { | ||
| issuer: OIDCProvider.issuer, | ||
| jwk_set: JSON::JWK::Set.new(public_jwk) | ||
| } | ||
| end | ||
|
|
@@ -48,7 +63,7 @@ def private_jwk | |
| end | ||
|
|
||
| def public_jwk | ||
| JSON::JWK.new key_pair.public_key | ||
| JSON::JWK.new key_pair.public_key, {use: 'sig',alg: 'RS256'} | ||
| end | ||
| end | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module OIDCProvider | ||
| class RefreshToken < ApplicationRecord | ||
| scope :valid, -> { where(arel_table[:expires_at].gteq(Time.now.utc)) } | ||
| belongs_to :authorization | ||
| attribute :token, :string, default: -> { SecureRandom.hex 32 } | ||
| attribute :expires_at, :datetime, default: -> { 1.month.from_now } | ||
| attribute :revoked_at, :datetime | ||
| serialize :scopes, coder: JSON | ||
| end | ||
| end |
|
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. Views are out of scope here
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 file is not excluded. Previously, an authorization page was added here to handle the issue of POST requests not redirecting
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. The next commit will delete this file |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| <!DOCTYPE html> | ||
| <html> | ||
| <head> | ||
| <title>ConnectOp</title> | ||
| <%= csrf_meta_tags %> | ||
| <%= action_cable_meta_tag %> | ||
|
|
||
| <%= stylesheet_link_tag :app, "data-turbo-track": "reload" %> | ||
| <%= javascript_importmap_tags %> | ||
| </head> | ||
|
|
||
| <body> | ||
| <%= yield %> | ||
| </body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| <article> | ||
| <h2>Authorization Request by <%= @client.name %></h2> | ||
| <%= form_tag authorizations_path do %> | ||
| <ul> | ||
| <% requested_scopes.each do |scope| %> | ||
| <li><%= scope %></li> | ||
| <% end %> | ||
| </ul> | ||
| <% [:client_id, :response_type, :redirect_uri, :scope, :state, :nonce].each do |key| %> | ||
| <%= hidden_field_tag key, oauth_request.send(key) %> | ||
| <% end %> | ||
| <p><%= submit_tag :deny %></p> | ||
| <p><%= submit_tag :approve %></p> | ||
| <% end %> | ||
| </article> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class CreateOIDCProviderRefreshTokens < ActiveRecord::Migration[5.1] | ||
| def change | ||
| create_table :oidc_provider_refresh_tokens do |t| | ||
| t.string :client_id, null: false | ||
| t.belongs_to :authorization, null: false | ||
|
|
||
| t.string :token, null: false | ||
| t.datetime :expires_at, null: false | ||
| t.datetime :revoked_at | ||
| t.text :scopes, null: false | ||
|
|
||
| t.timestamps | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| class AddIssuerToOIDCProviderAuthorizations < ActiveRecord::Migration[5.1] | ||
| def change | ||
| add_column :oidc_provider_authorizations, :issuer, :string | ||
| end | ||
|
|
||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,8 @@ module Scopes | |
| Profile = "profile" | ||
| Email = "email" | ||
| Address = "address" | ||
| Phone = "phone" | ||
| OfflineAccess = "offline_access" | ||
| end | ||
|
|
||
| autoload :TokenEndpoint, 'oidc_provider/token_endpoint' | ||
|
|
@@ -42,6 +44,19 @@ module Scopes | |
| mattr_accessor :account_identifier | ||
| @@account_identifier = :id | ||
|
|
||
| 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 | ||
|
Comment on lines
+47
to
+58
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. We should use scopes to define this info |
||
|
|
||
| mattr_accessor :after_sign_out_path | ||
|
|
||
| def self.add_client(&block) | ||
|
|
||
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.
This is specified by the gemspec