diff --git a/app/controllers/openid_connect/logout_controller.rb b/app/controllers/openid_connect/logout_controller.rb index c614725adbc..c774e9785d1 100644 --- a/app/controllers/openid_connect/logout_controller.rb +++ b/app/controllers/openid_connect/logout_controller.rb @@ -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 @@ -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 diff --git a/app/forms/openid_connect_authorize_form.rb b/app/forms/openid_connect_authorize_form.rb index cbfaba7eb34..6b540df4fd6 100644 --- a/app/forms/openid_connect_authorize_form.rb +++ b/app/forms/openid_connect_authorize_form.rb @@ -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) @@ -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 @@ -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, diff --git a/app/forms/openid_connect_logout_form.rb b/app/forms/openid_connect_logout_form.rb index 71cf1335e82..3e136780880 100644 --- a/app/forms/openid_connect_logout_form.rb +++ b/app/forms/openid_connect_logout_form.rb @@ -4,6 +4,8 @@ class OpenidConnectLogoutForm include RedirectUriValidator ATTRS = %i[ + client_id + current_user id_token_hint post_logout_redirect_uri state @@ -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 @@ -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'), @@ -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, diff --git a/app/validators/redirect_uri_validator.rb b/app/validators/redirect_uri_validator.rb index 072191502dc..b88281168a0 100644 --- a/app/validators/redirect_uri_validator.rb +++ b/app/validators/redirect_uri_validator.rb @@ -10,6 +10,7 @@ module RedirectUriValidator private def allowed_redirect_uri + return unless service_provider.present? return if any_registered_sp_redirect_uris_identical_to_the_requested_uri? errors.add( @@ -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 diff --git a/config/application.yml.default b/config/application.yml.default index 9db997484f9..493aae383d6 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -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: @@ -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 diff --git a/config/locales/openid_connect/en.yml b/config/locales/openid_connect/en.yml index 4b64a5e9439..4fdb945aff2 100644 --- a/config/locales/openid_connect/en.yml +++ b/config/locales/openid_connect/en.yml @@ -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 diff --git a/config/locales/openid_connect/es.yml b/config/locales/openid_connect/es.yml index caf9b6527d2..9426b95ea90 100644 --- a/config/locales/openid_connect/es.yml +++ b/config/locales/openid_connect/es.yml @@ -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 diff --git a/config/locales/openid_connect/fr.yml b/config/locales/openid_connect/fr.yml index 84b20776555..d50e176be40 100644 --- a/config/locales/openid_connect/fr.yml +++ b/config/locales/openid_connect/fr.yml @@ -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é diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 5aa5aade35d..3ded35f953e 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -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) @@ -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) diff --git a/spec/controllers/openid_connect/logout_controller_spec.rb b/spec/controllers/openid_connect/logout_controller_spec.rb index 6a90665bbd5..98a3c06f8bb 100644 --- a/spec/controllers/openid_connect/logout_controller_spec.rb +++ b/spec/controllers/openid_connect/logout_controller_spec.rb @@ -17,7 +17,7 @@ ) end - let(:id_token_hint) do + let(:valid_id_token_hint) do IdTokenBuilder.new( identity: identity, code: code, @@ -25,132 +25,558 @@ ).id_token end - describe '#index' do - subject(:action) do - get :index, - params: { - id_token_hint: id_token_hint, - post_logout_redirect_uri: post_logout_redirect_uri, - state: state, - } + context 'when accepting id_token_hint and not client_id' do + before do + allow(IdentityConfig.store).to receive(:reject_id_token_hint_in_logout). + and_return(false) + allow(IdentityConfig.store).to receive(:accept_client_id_in_oidc_logout). + and_return(false) end - context 'user is signed in' do - before { sign_in user } + describe '#index' do + let(:id_token_hint) { valid_id_token_hint } + subject(:action) do + get :index, + params: { + id_token_hint: id_token_hint, + post_logout_redirect_uri: post_logout_redirect_uri, + state: state, + } + end - context 'with valid params' do - it 'destroys the session' do - expect(controller).to receive(:sign_out).and_call_original + context 'user is signed in' do + before { sign_in user } - action + context 'with valid params' do + it 'destroys the session' do + expect(controller).to receive(:sign_out).and_call_original + + action + end + + it 'redirects back to the client' do + action + + expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) + end + + it 'tracks events' do + stub_analytics + expect(@analytics).to receive(:track_event). + with( + 'Logout Initiated', + hash_including( + success: true, + client_id: service_provider, + errors: {}, + sp_initiated: true, + oidc: true, + ), + ) + + stub_attempts_tracker + expect(@irs_attempts_api_tracker).to receive(:logout_initiated). + with( + success: true, + ) + action + end end - it 'redirects back to the client' do - action + context 'when sending client_id' do + let(:action) do + get :index, + params: { + client_id: service_provider, + post_logout_redirect_uri: post_logout_redirect_uri, + state: state, + } + end - expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) + it 'renders an error page' do + action + + expect(response).to render_template(:error) + end end - it 'tracks events' do - stub_analytics - expect(@analytics).to receive(:track_event). - with( - 'Logout Initiated', - hash_including( - success: true, + context 'with a bad redirect URI' do + let(:post_logout_redirect_uri) { 'https://example.com' } + + it 'renders an error page' do + action + + expect(response).to render_template(:error) + end + + it 'does not destroy the session' do + expect(controller).to_not receive(:sign_out) + + action + end + + it 'tracks events' do + stub_analytics + + errors = { + redirect_uri: [t('openid_connect.authorization.errors.redirect_uri_no_match')], + } + expect(@analytics).to receive(:track_event). + with( + 'Logout Initiated', + success: false, client_id: service_provider, - errors: {}, + errors: errors, + error_details: hash_including(*errors.keys), sp_initiated: true, oidc: true, - ), - ) - - stub_attempts_tracker - expect(@irs_attempts_api_tracker).to receive(:logout_initiated). - with( - success: true, - ) - action + method: nil, + saml_request_valid: nil, + ) + stub_attempts_tracker + expect(@irs_attempts_api_tracker).to receive(:logout_initiated). + with( + success: false, + ) + + action + end end - end - context 'with a bad redirect URI' do - let(:post_logout_redirect_uri) { 'https://example.com' } + context 'with a bad id_token_hint' do + let(:id_token_hint) { 'abc123' } + it 'tracks events' do + stub_analytics + errors_keys = [:id_token_hint] - it 'renders an error page' do + expect(@analytics).to receive(:track_event). + with( + 'Logout Initiated', + success: false, + client_id: nil, + errors: hash_including(*errors_keys), + error_details: hash_including(*errors_keys), + sp_initiated: true, + oidc: true, + method: nil, + saml_request_valid: nil, + ) + stub_attempts_tracker + expect(@irs_attempts_api_tracker).to receive(:logout_initiated). + with( + success: false, + ) + + action + end + end + end + + context 'user is not signed in' do + it 'redirects back with an error' do action - expect(response).to render_template(:error) + expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) end + end + end + end + + context 'when accepting id_token_hint and client_id' do + before do + allow(IdentityConfig.store).to receive(:reject_id_token_hint_in_logout). + and_return(false) + allow(IdentityConfig.store).to receive(:accept_client_id_in_oidc_logout). + and_return(true) + end - it 'does not destroy the session' do - expect(controller).to_not receive(:sign_out) + describe '#index' do + let(:id_token_hint) { valid_id_token_hint } - action + context 'when sending id_token_hint' do + subject(:action) do + get :index, + params: { + id_token_hint: id_token_hint, + post_logout_redirect_uri: post_logout_redirect_uri, + state: state, + } end - it 'tracks events' do - stub_analytics + context 'user is signed in' do + before { sign_in user } - errors = { - redirect_uri: [t('openid_connect.authorization.errors.redirect_uri_no_match')], - } - expect(@analytics).to receive(:track_event). - with( - 'Logout Initiated', - success: false, - client_id: service_provider, - errors: errors, - error_details: hash_including(*errors.keys), - sp_initiated: true, - oidc: true, - method: nil, - saml_request_valid: nil, - ) - stub_attempts_tracker - expect(@irs_attempts_api_tracker).to receive(:logout_initiated). - with( - success: false, - ) + context 'with valid params' do + it 'destroys the session' do + expect(controller).to receive(:sign_out).and_call_original - action + action + end + + it 'redirects back to the client' do + action + + expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) + end + + it 'tracks events' do + stub_analytics + expect(@analytics).to receive(:track_event). + with( + 'Logout Initiated', + hash_including( + success: true, + client_id: service_provider, + errors: {}, + sp_initiated: true, + oidc: true, + ), + ) + + stub_attempts_tracker + expect(@irs_attempts_api_tracker).to receive(:logout_initiated). + with( + success: true, + ) + action + end + end + + context 'with a bad redirect URI' do + let(:post_logout_redirect_uri) { 'https://example.com' } + + it 'renders an error page' do + action + + expect(response).to render_template(:error) + end + + it 'does not destroy the session' do + expect(controller).to_not receive(:sign_out) + + action + end + + it 'tracks events' do + stub_analytics + + errors = { + redirect_uri: [t('openid_connect.authorization.errors.redirect_uri_no_match')], + } + expect(@analytics).to receive(:track_event). + with( + 'Logout Initiated', + success: false, + client_id: service_provider, + errors: errors, + error_details: hash_including(*errors.keys), + sp_initiated: true, + oidc: true, + method: nil, + saml_request_valid: nil, + ) + stub_attempts_tracker + expect(@irs_attempts_api_tracker).to receive(:logout_initiated). + with( + success: false, + ) + + action + end + end + + context 'with a bad id_token_hint' do + let(:id_token_hint) { 'abc123' } + it 'tracks events' do + stub_analytics + errors_keys = [:id_token_hint] + + expect(@analytics).to receive(:track_event). + with( + 'Logout Initiated', + success: false, + client_id: nil, + errors: hash_including(*errors_keys), + error_details: hash_including(*errors_keys), + sp_initiated: true, + oidc: true, + method: nil, + saml_request_valid: nil, + ) + stub_attempts_tracker + expect(@irs_attempts_api_tracker).to receive(:logout_initiated). + with( + success: false, + ) + + action + end + end + end + + context 'user is not signed in' do + it 'redirects back with an error' do + action + + expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) + end end end - context 'with a bad id_token_hint' do - let(:id_token_hint) { { id_token_hint: 'abc123' } } - it 'tracks events' do - stub_analytics - errors_keys = [:id_token_hint, :redirect_uri] - - expect(@analytics).to receive(:track_event). - with( - 'Logout Initiated', - success: false, - client_id: nil, - errors: hash_including(*errors_keys), - error_details: hash_including(*errors_keys), - sp_initiated: true, - oidc: true, - method: nil, - saml_request_valid: nil, - ) - stub_attempts_tracker - expect(@irs_attempts_api_tracker).to receive(:logout_initiated). - with( - success: false, - ) + context 'when sending client_id' do + subject(:action) do + get :index, + params: { + client_id: service_provider, + post_logout_redirect_uri: post_logout_redirect_uri, + state: state, + } + end - action + context 'user is signed in' do + before { sign_in user } + + context 'with valid params' do + it 'destroys the session' do + expect(controller).to receive(:sign_out).and_call_original + + action + end + + it 'redirects back to the client' do + action + + expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) + end + + it 'tracks events' do + stub_analytics + expect(@analytics).to receive(:track_event). + with( + 'Logout Initiated', + hash_including( + success: true, + client_id: service_provider, + errors: {}, + sp_initiated: true, + oidc: true, + ), + ) + + stub_attempts_tracker + expect(@irs_attempts_api_tracker).to receive(:logout_initiated). + with( + success: true, + ) + action + end + end + + context 'with a bad redirect URI' do + let(:post_logout_redirect_uri) { 'https://example.com' } + + it 'renders an error page' do + action + + expect(response).to render_template(:error) + end + + it 'does not destroy the session' do + expect(controller).to_not receive(:sign_out) + + action + end + + it 'tracks events' do + stub_analytics + + errors = { + redirect_uri: [t('openid_connect.authorization.errors.redirect_uri_no_match')], + } + expect(@analytics).to receive(:track_event). + with( + 'Logout Initiated', + success: false, + client_id: service_provider, + errors: errors, + error_details: hash_including(*errors.keys), + sp_initiated: true, + oidc: true, + method: nil, + saml_request_valid: nil, + ) + stub_attempts_tracker + expect(@irs_attempts_api_tracker).to receive(:logout_initiated). + with( + success: false, + ) + + action + end + end + end + + context 'user is not signed in' do + it 'redirects back with an error' do + action + + expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) + end end end end + end + + context 'when rejecting id_token_hint' do + before do + allow(IdentityConfig.store).to receive(:reject_id_token_hint_in_logout). + and_return(true) + end + + describe '#index' do + let(:id_token_hint) { nil } + subject(:action) do + get :index, + params: { + client_id: service_provider, + id_token_hint: id_token_hint, + post_logout_redirect_uri: post_logout_redirect_uri, + state: state, + } + end + + context 'user is signed in' do + before { sign_in user } + + context 'with valid params' do + it 'destroys the session' do + expect(controller).to receive(:sign_out).and_call_original + + action + end + + it 'redirects back to the client' do + action + + expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) + end + + it 'tracks events' do + stub_analytics + expect(@analytics).to receive(:track_event). + with( + 'Logout Initiated', + hash_including( + success: true, + client_id: service_provider, + errors: {}, + sp_initiated: true, + oidc: true, + ), + ) + + stub_attempts_tracker + expect(@irs_attempts_api_tracker).to receive(:logout_initiated). + with( + success: true, + ) + action + end + end + + context 'with an id_token_hint' do + let(:id_token_hint) { valid_id_token_hint } + + it 'renders an error page' do + action + + expect(response).to render_template(:error) + end + + it 'does not destroy the session' do + expect(controller).to_not receive(:sign_out) + + action + end + + it 'tracks events' do + stub_analytics + + errors = { + id_token_hint: [t('openid_connect.logout.errors.id_token_hint_present')], + } + expect(@analytics).to receive(:track_event). + with( + 'Logout Initiated', + success: false, + client_id: service_provider, + errors: errors, + error_details: hash_including(*errors.keys), + sp_initiated: true, + oidc: true, + method: nil, + saml_request_valid: nil, + ) + stub_attempts_tracker + expect(@irs_attempts_api_tracker).to receive(:logout_initiated). + with( + success: false, + ) + + action + end + end + + context 'with a bad redirect URI' do + let(:post_logout_redirect_uri) { 'https://example.com' } + + it 'renders an error page' do + action - context 'user is not signed in' do - it 'redirects back with an error' do - action + expect(response).to render_template(:error) + end - expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) + it 'does not destroy the session' do + expect(controller).to_not receive(:sign_out) + + action + end + + it 'tracks events' do + stub_analytics + + errors = { + redirect_uri: [t('openid_connect.authorization.errors.redirect_uri_no_match')], + } + expect(@analytics).to receive(:track_event). + with( + 'Logout Initiated', + success: false, + client_id: service_provider, + errors: errors, + error_details: hash_including(*errors.keys), + sp_initiated: true, + oidc: true, + method: nil, + saml_request_valid: nil, + ) + stub_attempts_tracker + expect(@irs_attempts_api_tracker).to receive(:logout_initiated). + with( + success: false, + ) + + action + end + end + end + + context 'user is not signed in' do + it 'redirects back with an error' do + action + + expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) + end end end end diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index 462cee82f5c..603f2a6d4bd 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/spec/features/openid_connect/openid_connect_spec.rb @@ -136,30 +136,157 @@ end end - it 'logout includes redirect_uris in CSP headers and destroys the session' do - id_token = sign_in_get_id_token + context 'when accepting id_token_hint in logout' do + before do + allow(IdentityConfig.store).to receive(:reject_id_token_hint_in_logout). + and_return(false) + end - state = SecureRandom.hex + context 'when sending id_token_hint' do + it 'logout includes redirect_uris in CSP headers and destroys the session' do + id_token = sign_in_get_id_token - visit openid_connect_logout_path( - post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout', - state: state, - id_token_hint: id_token, - prevent_logout_redirect: true, - ) + state = SecureRandom.hex - current_url_no_port = URI(current_url).tap { |uri| uri.port = nil }.to_s - expect(current_url_no_port).to include( - "http://www.example.com/openid_connect/logout?id_token_hint=#{id_token}", - ) + visit openid_connect_logout_path( + post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout', + state: state, + id_token_hint: id_token, + prevent_logout_redirect: true, + ) - expect(page.response_headers['Content-Security-Policy']).to include( - 'form-action \'self\' gov.gsa.openidconnect.test:', - ) + current_url_no_port = URI(current_url).tap { |uri| uri.port = nil }.to_s + expect(current_url_no_port).to include( + "http://www.example.com/openid_connect/logout?id_token_hint=#{id_token}", + ) + + expect(page.response_headers['Content-Security-Policy']).to include( + 'form-action \'self\' gov.gsa.openidconnect.test:', + ) + + visit account_path + expect(page).to_not have_content(t('headings.account.login_info')) + expect(page).to have_content(t('headings.sign_in_without_sp')) + end + end + + context 'when permitting client_id' do + before do + allow(IdentityConfig.store).to receive(:accept_client_id_in_oidc_logout). + and_return(true) + end + + context 'when sending client_id' do + it 'logout includes redirect_uris in CSP headers and destroys the session' do + client_id = 'urn:gov:gsa:openidconnect:test' + sign_in_get_id_token(client_id: client_id) + + state = SecureRandom.hex + + visit openid_connect_logout_path( + client_id: client_id, + post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout', + state: state, + prevent_logout_redirect: true, + ) + + current_url_no_port = URI(current_url).tap { |uri| uri.port = nil }.to_s + expect(current_url_no_port).to include( + "http://www.example.com/openid_connect/logout?client_id=#{URI.encode_www_form_component(client_id)}", + ) + + expect(page.response_headers['Content-Security-Policy']).to include( + 'form-action \'self\' gov.gsa.openidconnect.test:', + ) + + visit account_path + expect(page).to_not have_content(t('headings.account.login_info')) + expect(page).to have_content(t('headings.sign_in_without_sp')) + end + end + end + + context 'when not permitting client_id' do + before do + allow(IdentityConfig.store).to receive(:accept_client_id_in_oidc_logout). + and_return(false) + end + + context 'when sending client_id' do + it 'renders an error' do + client_id = 'urn:gov:gsa:openidconnect:test' + sign_in_get_id_token(client_id: client_id) + + state = SecureRandom.hex + + visit openid_connect_logout_path( + client_id: client_id, + post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout', + state: state, + prevent_logout_redirect: true, + ) + + current_url_no_port = URI(current_url).tap { |uri| uri.port = nil }.to_s + expect(current_url_no_port).to include( + "http://www.example.com/openid_connect/logout?client_id=#{URI.encode_www_form_component(client_id)}", + ) + expect(page).to have_content(t('openid_connect.logout.errors.id_token_hint')) + end + end + end + end + + context 'when rejecting id_token_hint in logout' do + before do + allow(IdentityConfig.store).to receive(:reject_id_token_hint_in_logout). + and_return(true) + end + + it 'logout includes redirect_uris in CSP headers and destroys the session' do + client_id = 'urn:gov:gsa:openidconnect:test' + sign_in_get_id_token(client_id: client_id) + + state = SecureRandom.hex + + visit openid_connect_logout_path( + client_id: client_id, + post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout', + state: state, + prevent_logout_redirect: true, + ) + + current_url_no_port = URI(current_url).tap { |uri| uri.port = nil }.to_s + expect(current_url_no_port).to include( + "http://www.example.com/openid_connect/logout?client_id=#{URI.encode_www_form_component(client_id)}", + ) + + expect(page.response_headers['Content-Security-Policy']).to include( + 'form-action \'self\' gov.gsa.openidconnect.test:', + ) + + visit account_path + expect(page).to_not have_content(t('headings.account.login_info')) + expect(page).to have_content(t('headings.sign_in_without_sp')) + end + + it 'logout rejects requests that include id_token_hint' do + id_token = sign_in_get_id_token - visit account_path - expect(page).to_not have_content(t('headings.account.login_info')) - expect(page).to have_content(t('headings.sign_in_without_sp')) + state = SecureRandom.hex + + visit openid_connect_logout_path( + id_token_hint: id_token, + post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout', + state: state, + prevent_logout_redirect: true, + ) + + current_url_no_port = URI(current_url).tap { |uri| uri.port = nil }.to_s + expect(current_url_no_port).to include( + "http://www.example.com/openid_connect/logout?id_token_hint=#{id_token}", + ) + expect(page).to have_content(t('openid_connect.logout.errors.id_token_hint_present')) + end end it 'returns verified_at in an ial1 session if requested', driver: :mobile_rack_test do diff --git a/spec/forms/openid_connect_logout_form_spec.rb b/spec/forms/openid_connect_logout_form_spec.rb index 53e1a12b639..566c1dc02a7 100644 --- a/spec/forms/openid_connect_logout_form_spec.rb +++ b/spec/forms/openid_connect_logout_form_spec.rb @@ -6,17 +6,20 @@ let(:post_logout_redirect_uri) { 'gov.gsa.openidconnect.test://result/signout' } let(:service_provider) { 'urn:gov:gsa:openidconnect:test' } + let(:user) { create(:user) } let(:identity) do create( :service_provider_identity, service_provider: service_provider, - user: build(:user), + user: user, access_token: SecureRandom.hex, session_uuid: SecureRandom.uuid, ) end - let(:id_token_hint) do + let(:client_id) { service_provider } + + let(:valid_id_token_hint) do IdTokenBuilder.new( identity: identity, code: code, @@ -26,130 +29,315 @@ subject(:form) do OpenidConnectLogoutForm.new( - id_token_hint: id_token_hint, - post_logout_redirect_uri: post_logout_redirect_uri, - state: state, + current_user: current_user, + params: { + client_id: client_id, + id_token_hint: id_token_hint, + post_logout_redirect_uri: post_logout_redirect_uri, + state: state, + }, ) end - describe '#submit' do - subject(:result) { form.submit } + context 'when we accept id_token_hint' do + let(:id_token_hint) { valid_id_token_hint } + let(:client_id) { nil } + let(:current_user) { nil } - context 'with a valid form' do - it 'deactivates the identity' do - expect { result }.to change { identity.reload.session_uuid }.to(nil) - end + before do + allow(IdentityConfig.store).to receive(:reject_id_token_hint_in_logout). + and_return(false) + end - it 'has a redirect URI without errors' do - expect(UriService.params(result.extra[:redirect_uri])).to_not have_key(:error) + describe '#submit' do + subject(:result) { form.submit } + + context 'with a valid form' do + it 'deactivates the identity' do + expect { result }.to change { identity.reload.session_uuid }.to(nil) + end + + it 'has a redirect URI without errors' do + expect(UriService.params(result.extra[:redirect_uri])).to_not have_key(:error) + end + + it 'has a successful response' do + expect(result).to be_success + end end - it 'has a successful response' do - expect(result).to be_success + context 'with an invalid form' do + let(:state) { nil } + + it 'is not successful' do + expect(result).to_not be_success + end + + it 'has an error code in the redirect URI' do + expect(UriService.params(result.extra[:redirect_uri])[:error]).to eq('invalid_request') + end end end - context 'with an invalid form' do - let(:state) { nil } + describe '#valid?' do + subject(:valid?) { form.valid? } - it 'is not successful' do - expect(result).to_not be_success - end + context 'validating state' do + context 'when state is missing' do + let(:state) { nil } + + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:state]).to be_present + end + end - it 'has an error code in the redirect URI' do - expect(UriService.params(result.extra[:redirect_uri])[:error]).to eq('invalid_request') + context 'when state is shorter than the minimum length' do + let(:state) { 'a' } + + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:state]).to be_present + end + end end - end - end - describe '#valid?' do - subject(:valid?) { form.valid? } + context 'validating id_token_hint' do + context 'without an id_token_hint' do + let(:id_token_hint) { nil } - context 'validating state' do - context 'when state is missing' do - let(:state) { nil } + context 'when accepting client_id' do + before do + allow(IdentityConfig.store).to receive(:accept_client_id_in_oidc_logout). + and_return(true) + end + + context 'without client_id' do + let(:client_id) { nil } + + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:base]).to be_present + end + end + + context 'with a valid client_id' do + let(:client_id) { service_provider } + + it 'is valid' do + expect(valid?).to eq(true) + end + end + end + + context 'when not accepting client_id' do + before do + allow(IdentityConfig.store).to receive(:accept_client_id_in_oidc_logout). + and_return(false) + end + + context 'without client_id' do + let(:client_id) { nil } + + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:id_token_hint]).to be_present + end + end + + context 'with a valid client_id' do + let(:client_id) { service_provider } + + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:client_id]).to be_present + end + end + end + end + + context 'with an id_token_hint that is not a JWT' do + let(:id_token_hint) { 'asdasd' } - it 'is not valid' do - expect(valid?).to eq(false) - expect(form.errors[:state]).to be_present + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:id_token_hint]). + to include(t('openid_connect.logout.errors.id_token_hint')) + end + end + + context 'with a payload that does not correspond to an identity' do + let(:id_token_hint) do + JWT.encode({ sub: '123', aud: '456' }, AppArtifacts.store.oidc_private_key, 'RS256') + end + + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:id_token_hint]). + to include(t('openid_connect.logout.errors.id_token_hint')) + end + end + + context 'with an expired, but otherwise valid id_token_hint' do + let(:id_token_hint) do + IdTokenBuilder.new( + identity: identity, + code: code, + custom_expiration: 5.days.ago.to_i, + ).id_token + end + + it 'is valid' do + expect(valid?).to eq(true) + expect(form.errors[:id_token_hint]).to be_blank + end end end - context 'when state is shorter than the minimum length' do - let(:state) { 'a' } + context 'post_logout_redirect_uri' do + context 'without a post_logout_redirect_uri' do + let(:post_logout_redirect_uri) { nil } + + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:redirect_uri]).to be_present + end + end + + context 'with URI that does not match what is registered' do + let(:post_logout_redirect_uri) { 'https://example.com' } - it 'is not valid' do - expect(valid?).to eq(false) - expect(form.errors[:state]).to be_present + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:redirect_uri]). + to include(t('openid_connect.authorization.errors.redirect_uri_no_match')) + end end end end + end + + context 'when we reject id_token_hint' do + let(:id_token_hint) { nil } + let(:current_user) { nil } + + before do + allow(IdentityConfig.store).to receive(:reject_id_token_hint_in_logout). + and_return(true) + end - context 'validating id_token_hint' do - context 'without an id_token_hint' do - let(:id_token_hint) { nil } + describe '#submit' do + subject(:result) { form.submit } - it 'is not valid' do - expect(valid?).to eq(false) - expect(form.errors[:id_token_hint]).to be_present + context 'with a valid form' do + context 'with a current user' do + let(:current_user) { user } + + it 'deactivates the identity' do + expect { result }.to change { identity.reload.session_uuid }.to(nil) + end end - end - context 'with an id_token_hint that is not a JWT' do - let(:id_token_hint) { 'asdasd' } + it 'has a redirect URI without errors' do + expect(UriService.params(result.extra[:redirect_uri])).to_not have_key(:error) + end - it 'is not valid' do - expect(valid?).to eq(false) - expect(form.errors[:id_token_hint]). - to include(t('openid_connect.logout.errors.id_token_hint')) + it 'has a successful response' do + expect(result).to be_success end end - context 'with a payload that does not correspond to an identity' do - let(:id_token_hint) do - JWT.encode({ sub: '123', aud: '456' }, AppArtifacts.store.oidc_private_key, 'RS256') + context 'with an invalid form' do + let(:state) { nil } + + it 'is not successful' do + expect(result).to_not be_success end - it 'is not valid' do - expect(valid?).to eq(false) - expect(form.errors[:id_token_hint]). - to include(t('openid_connect.logout.errors.id_token_hint')) + it 'has an error code in the redirect URI' do + expect(UriService.params(result.extra[:redirect_uri])[:error]).to eq('invalid_request') end end + end + + describe '#valid?' do + subject(:valid?) { form.valid? } + + context 'validating state' do + context 'when state is missing' do + let(:state) { nil } - context 'with an expired, but otherwise valid id_token_hint' do - let(:id_token_hint) do - IdTokenBuilder.new( - identity: identity, - code: code, - custom_expiration: 5.days.ago.to_i, - ).id_token + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:state]).to be_present + end end - it 'is valid' do - expect(valid?).to eq(true) - expect(form.errors[:id_token_hint]).to be_blank + context 'when state is shorter than the minimum length' do + let(:state) { 'a' } + + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:state]).to be_present + end end end - end - context 'post_logout_redirect_uri' do - context 'without a post_logout_redirect_uri' do - let(:post_logout_redirect_uri) { nil } + context 'validating id_token_hint' do + context 'without an id_token_hint' do + let(:id_token_hint) { nil } + + it 'is valid' do + expect(valid?).to eq(true) + expect(form.errors[:id_token_hint]).not_to be_present + end + end + + context 'with an id_token_hint' do + let(:id_token_hint) { valid_id_token_hint } - it 'is not valid' do - expect(valid?).to eq(false) - expect(form.errors[:redirect_uri]).to be_present + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:id_token_hint]). + to include(t('openid_connect.logout.errors.id_token_hint_present')) + end end end - context 'with URI that does not match what is registered' do - let(:post_logout_redirect_uri) { 'https://example.com' } + context 'post_logout_redirect_uri' do + context 'without a post_logout_redirect_uri' do + let(:post_logout_redirect_uri) { nil } + + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:redirect_uri]).to be_present + end + end + + context 'with URI that does not match what is registered' do + let(:post_logout_redirect_uri) { 'https://example.com' } + + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:redirect_uri]). + to include(t('openid_connect.authorization.errors.redirect_uri_no_match')) + end + end + + context 'when no client_id passed' do + let(:client_id) { nil } + + it 'does not include error about redirect_uri' do + expect(valid?).to eq(false) + expect(form.errors[:redirect_uri]). + not_to include(t('openid_connect.authorization.errors.redirect_uri_no_match')) + end - it 'is not valid' do - expect(valid?).to eq(false) - expect(form.errors[:redirect_uri]). - to include(t('openid_connect.authorization.errors.redirect_uri_no_match')) + it 'is not valid' do + expect(valid?).to eq(false) + expect(form.errors[:client_id]). + to include(t('openid_connect.logout.errors.client_id_missing')) + end end end end