diff --git a/app/controllers/openid_connect/logout_controller.rb b/app/controllers/openid_connect/logout_controller.rb index 53ec869bbd5..3aed416bf98 100644 --- a/app/controllers/openid_connect/logout_controller.rb +++ b/app/controllers/openid_connect/logout_controller.rb @@ -61,6 +61,7 @@ def handle_successful_logout_request(result, redirect_uri) post_logout_redirect_uri: logout_params[:post_logout_redirect_uri], } @params[:state] = logout_params[:state] if !logout_params[:state].nil? + @service_provider_name = @logout_form.service_provider&.friendly_name render :index else analytics.logout_initiated(**result.to_h.except(:redirect_uri)) diff --git a/app/forms/openid_connect_logout_form.rb b/app/forms/openid_connect_logout_form.rb index bb7b309c871..fc71d2ee6d3 100644 --- a/app/forms/openid_connect_logout_form.rb +++ b/app/forms/openid_connect_logout_form.rb @@ -52,6 +52,21 @@ def submit FormResponse.new(success: success, errors: errors, extra: extra_analytics_attributes) end + # Used by RedirectUriValidator + def service_provider + return @service_provider if defined?(@service_provider) + sp_from_client_id = ServiceProvider.find_by(issuer: client_id) + + @service_provider = + if reject_id_token_hint? + sp_from_client_id + else + identity&.service_provider_record || sp_from_client_id + end + + @service_provider + end + private attr_reader :identity, :success @@ -118,17 +133,6 @@ def validate_identity end end - # Used by RedirectUriValidator - def service_provider - 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_parameter_present: client_id.present?, diff --git a/app/views/openid_connect/logout/index.html.erb b/app/views/openid_connect/logout/index.html.erb index e9b8ddf4d11..35a0e313d36 100644 --- a/app/views/openid_connect/logout/index.html.erb +++ b/app/views/openid_connect/logout/index.html.erb @@ -2,7 +2,11 @@
<%= image_tag(asset_url('user-access.svg'), width: '280', height: '91', alt: '') %> - <%= render PageHeadingComponent.new.with_content(t('openid_connect.logout.heading', app_name: APP_NAME)) %> + <% if @service_provider_name.present? %> + <%= render PageHeadingComponent.new.with_content(t('openid_connect.logout.heading_with_sp', app_name: APP_NAME, service_provider_name: @service_provider_name)) %> + <% else %> + <%= render PageHeadingComponent.new.with_content(t('openid_connect.logout.heading', app_name: APP_NAME)) %> + <% end %>
<%= button_to( diff --git a/config/locales/openid_connect/en.yml b/config/locales/openid_connect/en.yml index 7447715d5e8..c1e85e47030 100644 --- a/config/locales/openid_connect/en.yml +++ b/config/locales/openid_connect/en.yml @@ -29,6 +29,8 @@ en: no_client_id_or_id_token_hint: This application is misconfigured and must send either client_id or id_token_hint. heading: Do you want to sign out of %{app_name}? + heading_with_sp: Do you want to sign out of %{app_name} and return to + %{service_provider_name}? token: errors: expired_code: is expired diff --git a/config/locales/openid_connect/es.yml b/config/locales/openid_connect/es.yml index faf39a0ec2b..a973dd12b0f 100644 --- a/config/locales/openid_connect/es.yml +++ b/config/locales/openid_connect/es.yml @@ -29,6 +29,8 @@ es: no_client_id_or_id_token_hint: Esta aplicación está mal configurada y debe enviar client_id o id_token_hint. heading: ¿Quieres cerrar sesión en %{app_name}? + heading_with_sp: ¿Quiere cerrar sesión en %{app_name} y regresar a + %{service_provider_name}? token: errors: expired_code: ha expirado diff --git a/config/locales/openid_connect/fr.yml b/config/locales/openid_connect/fr.yml index 268c19312f9..f502226dac8 100644 --- a/config/locales/openid_connect/fr.yml +++ b/config/locales/openid_connect/fr.yml @@ -29,6 +29,8 @@ fr: no_client_id_or_id_token_hint: Cette application est mal configurée et doit envoyer client_id ou id_token_hint. heading: Voulez-vous vous déconnecter de %{app_name}? + heading_with_sp: Souhaitez-vous vous déconnecter de %{app_name} et revenir à + %{service_provider_name}? token: errors: expired_code: est expiré diff --git a/spec/controllers/openid_connect/logout_controller_spec.rb b/spec/controllers/openid_connect/logout_controller_spec.rb index 11559088c8e..9bc34dc04e4 100644 --- a/spec/controllers/openid_connect/logout_controller_spec.rb +++ b/spec/controllers/openid_connect/logout_controller_spec.rb @@ -3,14 +3,22 @@ RSpec.describe OpenidConnect::LogoutController do let(:state) { SecureRandom.hex } let(:code) { SecureRandom.uuid } + let(:valid_post_logout_redirect_uri) { 'gov.gsa.openidconnect.test://result/signout' } let(:post_logout_redirect_uri) { 'gov.gsa.openidconnect.test://result/signout' } let(:user) { build(:user) } - let(:service_provider) { 'urn:gov:gsa:openidconnect:test' } + + let(:service_provider) do + create( + :service_provider, issuer: 'test', redirect_uris: [ + valid_post_logout_redirect_uri, + ] + ) + end let(:identity) do create( :service_provider_identity, - service_provider: service_provider, + service_provider: service_provider.issuer, user: user, access_token: SecureRandom.hex, session_uuid: SecureRandom.uuid, @@ -67,7 +75,7 @@ 'OIDC Logout Requested', hash_including( success: true, - client_id: service_provider, + client_id: service_provider.issuer, client_id_parameter_present: false, id_token_hint_parameter_present: true, errors: {}, @@ -80,7 +88,7 @@ 'Logout Initiated', hash_including( success: true, - client_id: service_provider, + client_id: service_provider.issuer, client_id_parameter_present: false, id_token_hint_parameter_present: true, errors: {}, @@ -123,7 +131,7 @@ with( 'OIDC Logout Requested', success: false, - client_id: service_provider, + client_id: service_provider.issuer, client_id_parameter_present: false, id_token_hint_parameter_present: true, errors: errors, @@ -177,7 +185,7 @@ subject(:action) do get :index, params: { - client_id: service_provider, + client_id: service_provider.issuer, post_logout_redirect_uri: post_logout_redirect_uri, state: state, } @@ -187,20 +195,16 @@ before { stub_sign_in(user) } context 'with valid params' do - it 'renders logout confirmation page' do - action + render_views - expect(response).to render_template(:index) - end - - it 'tracks events' do + it 'renders logout confirmation page' do stub_analytics expect(@analytics).to receive(:track_event). with( 'OIDC Logout Requested', hash_including( success: true, - client_id: service_provider, + client_id: service_provider.issuer, client_id_parameter_present: true, id_token_hint_parameter_present: false, errors: {}, @@ -213,7 +217,7 @@ 'OIDC Logout Page Visited', hash_including( success: true, - client_id: service_provider, + client_id: service_provider.issuer, client_id_parameter_present: true, id_token_hint_parameter_present: false, errors: {}, @@ -223,6 +227,8 @@ ) action + expect(response).to render_template(:index) + expect(response.body).to include service_provider.friendly_name end end @@ -252,7 +258,7 @@ 'OIDC Logout Requested', hash_including( success: false, - client_id: service_provider, + client_id: service_provider.issuer, client_id_parameter_present: true, id_token_hint_parameter_present: false, errors: errors, @@ -284,7 +290,7 @@ subject(:action) do delete :delete, params: { - client_id: service_provider, + client_id: service_provider.issuer, post_logout_redirect_uri: post_logout_redirect_uri, state: state, } @@ -350,7 +356,7 @@ subject(:action) do get :index, params: { - client_id: service_provider, + client_id: service_provider.issuer, id_token_hint: id_token_hint, post_logout_redirect_uri: post_logout_redirect_uri, state: state, @@ -374,7 +380,7 @@ 'OIDC Logout Requested', hash_including( success: true, - client_id: service_provider, + client_id: service_provider.issuer, client_id_parameter_present: true, id_token_hint_parameter_present: false, errors: {}, @@ -388,7 +394,7 @@ 'OIDC Logout Page Visited', hash_including( success: true, - client_id: service_provider, + client_id: service_provider.issuer, client_id_parameter_present: true, id_token_hint_parameter_present: false, errors: {}, @@ -426,7 +432,7 @@ with( 'OIDC Logout Requested', success: false, - client_id: service_provider, + client_id: service_provider.issuer, client_id_parameter_present: true, id_token_hint_parameter_present: true, errors: errors, @@ -466,7 +472,7 @@ with( 'OIDC Logout Requested', success: false, - client_id: service_provider, + client_id: service_provider.issuer, client_id_parameter_present: true, id_token_hint_parameter_present: false, errors: errors, @@ -496,7 +502,7 @@ subject(:action) do delete :delete, params: { - client_id: service_provider, + client_id: service_provider.issuer, post_logout_redirect_uri: post_logout_redirect_uri, state: state, } @@ -517,7 +523,7 @@ with( 'OIDC Logout Submitted', success: true, - client_id: service_provider, + client_id: service_provider.issuer, client_id_parameter_present: true, id_token_hint_parameter_present: false, errors: {}, @@ -531,7 +537,7 @@ with( 'Logout Initiated', success: true, - client_id: service_provider, + client_id: service_provider.issuer, client_id_parameter_present: true, id_token_hint_parameter_present: false, errors: {}, @@ -594,7 +600,7 @@ def add_sp_session_request_url params = { acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, - client_id: service_provider, + client_id: service_provider.issuer, nonce: SecureRandom.hex, redirect_uri: 'gov.gsa.openidconnect.test://result', response_type: 'code', diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index a3be2d15066..0646a7c2e35 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/spec/features/openid_connect/openid_connect_spec.rb @@ -175,17 +175,23 @@ context 'when sending client_id' do it 'logout destroys the session when confirming logout' do - client_id = 'urn:gov:gsa:openidconnect:test' - sign_in_get_id_token(client_id: client_id) + service_provider = ServiceProvider.find_by(issuer: 'urn:gov:gsa:openidconnect:test') + sign_in_get_id_token(client_id: service_provider.issuer) state = SecureRandom.hex visit openid_connect_logout_path( - client_id: client_id, + client_id: service_provider.issuer, post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout', state: state, ) - expect(page).to have_content(t('openid_connect.logout.heading', app_name: APP_NAME)) + expect(page).to have_content( + t( + 'openid_connect.logout.heading_with_sp', + app_name: APP_NAME, + service_provider_name: service_provider.friendly_name, + ), + ) click_button t('openid_connect.logout.confirm', app_name: APP_NAME) visit account_path @@ -194,14 +200,20 @@ end it 'logout does not require state' do - client_id = 'urn:gov:gsa:openidconnect:test' - sign_in_get_id_token(client_id: client_id) + service_provider = ServiceProvider.find_by(issuer: 'urn:gov:gsa:openidconnect:test') + sign_in_get_id_token(client_id: service_provider.issuer) visit openid_connect_logout_path( - client_id: client_id, + client_id: service_provider.issuer, post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout', ) - expect(page).to have_content(t('openid_connect.logout.heading', app_name: APP_NAME)) + expect(page).to have_content( + t( + 'openid_connect.logout.heading_with_sp', + app_name: APP_NAME, + service_provider_name: service_provider.friendly_name, + ), + ) click_button t('openid_connect.logout.confirm', app_name: APP_NAME) visit account_path @@ -210,17 +222,23 @@ end it 'does not destroy the session and redirects to account page when denying logout' do - client_id = 'urn:gov:gsa:openidconnect:test' - sign_in_get_id_token(client_id: client_id) + service_provider = ServiceProvider.find_by(issuer: 'urn:gov:gsa:openidconnect:test') + sign_in_get_id_token(client_id: service_provider.issuer) state = SecureRandom.hex visit openid_connect_logout_path( - client_id: client_id, + client_id: service_provider.issuer, post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout', state: state, ) - expect(page).to have_content(t('openid_connect.logout.heading', app_name: APP_NAME)) + expect(page).to have_content( + t( + 'openid_connect.logout.heading_with_sp', + app_name: APP_NAME, + service_provider_name: service_provider.friendly_name, + ), + ) click_link t('openid_connect.logout.deny') expect(page).to have_content(t('headings.account.login_info')) @@ -235,17 +253,23 @@ end it 'logout destroys the session when confirming logout' do - client_id = 'urn:gov:gsa:openidconnect:test' - sign_in_get_id_token(client_id: client_id) + service_provider = ServiceProvider.find_by(issuer: 'urn:gov:gsa:openidconnect:test') + sign_in_get_id_token(client_id: service_provider.issuer) state = SecureRandom.hex visit openid_connect_logout_path( - client_id: client_id, + client_id: service_provider.issuer, post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout', state: state, ) - expect(page).to have_content(t('openid_connect.logout.heading', app_name: APP_NAME)) + expect(page).to have_content( + t( + 'openid_connect.logout.heading_with_sp', + app_name: APP_NAME, + service_provider_name: service_provider.friendly_name, + ), + ) click_button t('openid_connect.logout.confirm', app_name: APP_NAME) visit account_path @@ -254,14 +278,20 @@ end it 'logout does not require state' do - client_id = 'urn:gov:gsa:openidconnect:test' - sign_in_get_id_token(client_id: client_id) + service_provider = ServiceProvider.find_by(issuer: 'urn:gov:gsa:openidconnect:test') + sign_in_get_id_token(client_id: service_provider.issuer) visit openid_connect_logout_path( - client_id: client_id, + client_id: service_provider.issuer, post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout', ) - expect(page).to have_content(t('openid_connect.logout.heading', app_name: APP_NAME)) + expect(page).to have_content( + t( + 'openid_connect.logout.heading_with_sp', + app_name: APP_NAME, + service_provider_name: service_provider.friendly_name, + ), + ) click_button t('openid_connect.logout.confirm', app_name: APP_NAME) visit account_path