Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions app/controllers/openid_connect/logout_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ class LogoutController < ApplicationController
before_action :apply_secure_headers_override, only: [:index]

def index
@logout_form = OpenidConnectLogoutForm.new(logout_params)
@logout_form = OpenidConnectLogoutForm.new(
params: logout_params,
current_user: current_user,
)

result = @logout_form.submit

Expand All @@ -28,7 +31,18 @@ def index
end

def logout_params
params.permit(:id_token_hint, :post_logout_redirect_uri, :state, :prevent_logout_redirect)
permitted = [
:id_token_hint,
:post_logout_redirect_uri,
:state,
:prevent_logout_redirect,
]

if IdentityConfig.store.accept_client_id_in_oidc_logout ||
IdentityConfig.store.reject_id_token_hint_in_logout
permitted << :client_id
end
params.permit(*permitted)
end
end
end
13 changes: 9 additions & 4 deletions app/forms/openid_connect_authorize_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ def verified_at_requested?
scope.include?('profile:verified_at')
end

def cannot_validate_redirect_uri?
errors.include?(:redirect_uri) || errors.include?(:client_id)
end

def service_provider
return @service_provider if defined?(@service_provider)
@service_provider = ServiceProvider.find_by(issuer: client_id)
Expand All @@ -81,9 +85,10 @@ def link_identity_to_service_provider(current_user, rails_session_id)
end

def success_redirect_uri
uri = redirect_uri unless errors.include?(:redirect_uri)
return if cannot_validate_redirect_uri?
code = identity&.session_uuid

UriService.add_params(uri, code: code, state: state) if code
UriService.add_params(redirect_uri, code: code, state: state) if code
end

def ial_values
Expand Down Expand Up @@ -218,10 +223,10 @@ def result_uri
end

def error_redirect_uri
uri = redirect_uri unless errors.include?(:redirect_uri)
return if cannot_validate_redirect_uri?

UriService.add_params(
uri,
redirect_uri,
error: 'invalid_request',
error_description: errors.full_messages.join(' '),
state: state,
Expand Down
92 changes: 77 additions & 15 deletions app/forms/openid_connect_logout_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ class OpenidConnectLogoutForm
include RedirectUriValidator

ATTRS = %i[
client_id
current_user
id_token_hint
post_logout_redirect_uri
state
Expand All @@ -13,41 +15,72 @@ class OpenidConnectLogoutForm

RANDOM_VALUE_MINIMUM_LENGTH = OpenidConnectAuthorizeForm::RANDOM_VALUE_MINIMUM_LENGTH

validates :id_token_hint, presence: true
validates :client_id,
presence: {
message: I18n.t('openid_connect.logout.errors.client_id_missing'),
},
if: :reject_id_token_hint?
validates :client_id,
absence: true,
unless: -> { accept_client_id? }
validates :id_token_hint,
absence: {
message: I18n.t('openid_connect.logout.errors.id_token_hint_present'),
},
if: :reject_id_token_hint?
validates :post_logout_redirect_uri, presence: true
validates :state, presence: true, length: { minimum: RANDOM_VALUE_MINIMUM_LENGTH }

validate :validate_identity
validate :id_token_hint_or_client_id_present,
if: -> { accept_client_id? && !reject_id_token_hint? }
validate :validate_identity, unless: :reject_id_token_hint?
validate :valid_client_id, if: :accept_client_id?

def initialize(params)
def initialize(params:, current_user:)
ATTRS.each do |key|
instance_variable_set(:"@#{key}", params[key])
end

@current_user = current_user
@identity = load_identity
end

def submit
@success = valid?

identity.deactivate if success
identity&.deactivate if success

FormResponse.new(success: success, errors: errors, extra: extra_analytics_attributes)
end

private

attr_reader :identity,
:success
attr_reader :identity, :success

def load_identity
payload, _headers = JWT.decode(
id_token_hint, AppArtifacts.store.oidc_public_key, true,
algorithm: 'RS256',
leeway: Float::INFINITY
).map(&:with_indifferent_access)
def accept_client_id?
IdentityConfig.store.accept_client_id_in_oidc_logout || reject_id_token_hint?
end

identity_from_payload(payload)
def reject_id_token_hint?
IdentityConfig.store.reject_id_token_hint_in_logout
end

def load_identity
identity_from_client_id = current_user&.
identities&.
find_by(service_provider: client_id)

if reject_id_token_hint?
identity_from_client_id
else
payload, _headers = JWT.decode(
id_token_hint, AppArtifacts.store.oidc_public_key, true,
algorithm: 'RS256',
leeway: Float::INFINITY
).map(&:with_indifferent_access)

identity_from_payload(payload) || identity_from_client_id
end
rescue JWT::DecodeError
nil
end
Expand All @@ -58,7 +91,30 @@ def identity_from_payload(payload)
AgencyIdentityLinker.sp_identity_from_uuid_and_sp(uuid, sp)
end

def id_token_hint_or_client_id_present
return if client_id.present? || id_token_hint.present?

errors.add(
:base,
t('openid_connect.logout.errors.no_client_id_or_id_token_hint'),
type: :client_id_or_id_token_hint_missing,
)
end

def valid_client_id
return unless client_id.present? && id_token_hint.blank?
return if service_provider.present?

errors.add(
:client_id,
t('openid_connect.logout.errors.client_id_invalid'),
type: :client_id_invalid,
)
end

def validate_identity
return if client_id.present? # there won't alwasy be an identity found

unless identity
errors.add(
:id_token_hint, t('openid_connect.logout.errors.id_token_hint'),
Expand All @@ -69,12 +125,18 @@ def validate_identity

# Used by RedirectUriValidator
def service_provider
identity&.service_provider_record
sp_from_client_id = ServiceProvider.find_by(issuer: client_id)

if reject_id_token_hint?
sp_from_client_id
else
identity&.service_provider_record || sp_from_client_id
end
end

def extra_analytics_attributes
{
client_id: identity&.service_provider,
client_id: service_provider&.issuer,
redirect_uri: redirect_uri,
sp_initiated: true,
oidc: true,
Expand Down
3 changes: 2 additions & 1 deletion app/validators/redirect_uri_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ module RedirectUriValidator
private

def allowed_redirect_uri
return unless service_provider.present?
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this since the extra redirect_uri error message when you don't have a valid SP has caused significant confusion in the past.

return if any_registered_sp_redirect_uris_identical_to_the_requested_uri?

errors.add(
Expand All @@ -19,7 +20,7 @@ def allowed_redirect_uri
end

def any_registered_sp_redirect_uris_identical_to_the_requested_uri?
service_provider&.redirect_uris&.any? do |sp_redirect_uri|
service_provider.redirect_uris.any? do |sp_redirect_uri|
parsed_sp_redirect_uri = URI.parse(sp_redirect_uri)

parsed_sp_redirect_uri == parsed_redirect_uri
Expand Down
2 changes: 2 additions & 0 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ aamva_supported_jurisdictions: '["AL","AR","AZ","CO","CT","DC","DE","FL","GA","H
aamva_verification_request_timeout: 5.0
aamva_verification_url: https://example.org:12345/verification/url
all_redirect_uris_cache_duration_minutes: 2
accept_client_id_in_oidc_logout: false
account_reset_token_valid_for_days: 1
account_reset_wait_period_days: 1
acuant_maintenance_window_start:
Expand Down Expand Up @@ -227,6 +228,7 @@ reg_confirmed_email_max_attempts: 20
reg_confirmed_email_window_in_minutes: 60
reg_unconfirmed_email_max_attempts: 20
reg_unconfirmed_email_window_in_minutes: 60
reject_id_token_hint_in_logout: false
remember_device_expiration_hours_aal_1: 720
remember_device_expiration_hours_aal_2: 12
report_timeout: 0
Expand Down
6 changes: 6 additions & 0 deletions config/locales/openid_connect/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,13 @@ en:
unauthorized_scope: Unauthorized scope
logout:
errors:
client_id_invalid: client_id was not recognized
client_id_missing: client_id is missing
id_token_hint: id_token_hint was not recognized
id_token_hint_present: This application is misconfigured and should not be
sending id_token_hint. Please send client_id instead.
no_client_id_or_id_token_hint: This application is misconfigured and must send
either client_id or id_token_hint.
token:
errors:
expired_code: is expired
Expand Down
8 changes: 7 additions & 1 deletion config/locales/openid_connect/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,13 @@ es:
unauthorized_scope: Alcance no autorizado
logout:
errors:
id_token_hint: Id_token_hint no fue reconocido
client_id_invalid: client_id no fue reconocido
client_id_missing: falta client_id
id_token_hint: id_token_hint no fue reconocido
id_token_hint_present: Esta aplicación está mal configurada y no debería enviar
id_token_hint. Por favor, envíe client_id en su lugar.
no_client_id_or_id_token_hint: Esta aplicación está mal configurada y debe
enviar client_id o id_token_hint.
token:
errors:
expired_code: ha expirado
Expand Down
6 changes: 6 additions & 0 deletions config/locales/openid_connect/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,13 @@ fr:
unauthorized_scope: Portée non autorisée
logout:
errors:
client_id_invalid: client_id n’a pas été reconnu
client_id_missing: client_id est manquant
id_token_hint: id_token_hint n’a pas été reconnu
id_token_hint_present: Cette application est mal configurée et ne devrait pas
envoyer id_token_hint. Veuillez envoyer client_id à la place.
no_client_id_or_id_token_hint: Cette application est mal configurée et doit
envoyer client_id ou id_token_hint.
token:
errors:
expired_code: est expiré
Expand Down
2 changes: 2 additions & 0 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ def self.build_store(config_map)
config.add(:aamva_verification_request_timeout, type: :float)
config.add(:aamva_verification_url)
config.add(:all_redirect_uris_cache_duration_minutes, type: :integer)
config.add(:accept_client_id_in_oidc_logout, type: :boolean)
config.add(:account_reset_token_valid_for_days, type: :integer)
config.add(:account_reset_wait_period_days, type: :integer)
config.add(:acuant_maintenance_window_start, type: :timestamp, allow_nil: true)
Expand Down Expand Up @@ -316,6 +317,7 @@ def self.build_store(config_map)
config.add(:reg_confirmed_email_window_in_minutes, type: :integer)
config.add(:reg_unconfirmed_email_max_attempts, type: :integer)
config.add(:reg_unconfirmed_email_window_in_minutes, type: :integer)
config.add(:reject_id_token_hint_in_logout, type: :boolean)
config.add(:remember_device_expiration_hours_aal_1, type: :integer)
config.add(:remember_device_expiration_hours_aal_2, type: :integer)
config.add(:report_timeout, type: :integer)
Expand Down
Loading