diff --git a/app/controllers/openid_connect/logout_controller.rb b/app/controllers/openid_connect/logout_controller.rb index 2db0f22cf3b..5a6be466add 100644 --- a/app/controllers/openid_connect/logout_controller.rb +++ b/app/controllers/openid_connect/logout_controller.rb @@ -90,12 +90,6 @@ def apply_logout_secure_headers_override(redirect_uri, service_provider) override_form_action_csp(uris) end - def require_logout_confirmation? - (logout_params[:id_token_hint].nil? || IdentityConfig.store.reject_id_token_hint_in_logout) && - logout_params[:client_id] && - current_user - end - # @return [OpenidConnectLogoutForm] def build_logout_form OpenidConnectLogoutForm.new( @@ -108,12 +102,13 @@ def build_logout_form # @param redirect_uri [String] The URL to redirect the user to after logout def handle_successful_logout_request(result, redirect_uri) apply_logout_secure_headers_override(redirect_uri, @logout_form.service_provider) - if require_logout_confirmation? + if current_user.present? analytics.oidc_logout_visited(**to_event(result)) @params = { client_id: logout_params[:client_id], post_logout_redirect_uri: logout_params[:post_logout_redirect_uri], + id_token_hint: logout_params[:id_token_hint], } @params[:state] = logout_params[:state] if !logout_params[:state].nil? @service_provider_name = @logout_form.service_provider&.friendly_name diff --git a/spec/controllers/openid_connect/logout_controller_spec.rb b/spec/controllers/openid_connect/logout_controller_spec.rb index 86fa260d6c1..68838801606 100644 --- a/spec/controllers/openid_connect/logout_controller_spec.rb +++ b/spec/controllers/openid_connect/logout_controller_spec.rb @@ -59,30 +59,9 @@ before { stub_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 if server-side redirect is enabled' do - allow(IdentityConfig.store).to receive(:openid_connect_redirect) - .and_return('server_side') - action - - expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) - end - - it 'renders JS client-side redirect if client-side JS redirect is enabled' do - allow(IdentityConfig.store).to receive(:openid_connect_redirect) - .and_return('client_side_js') - action - - expect(controller).to render_template('openid_connect/shared/redirect_js') - expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) - end + render_views - it 'tracks events' do + it 'renders logout confirmation page' do stub_analytics action @@ -99,7 +78,7 @@ ), ) expect(@analytics).to have_logged_event( - 'Logout Initiated', + 'OIDC Logout Page Visited', success: true, client_id: service_provider.issuer, client_id_parameter_present: false, @@ -107,10 +86,11 @@ sp_initiated: true, oidc: true, ) - expect(@analytics).to_not have_logged_event( :sp_integration_errors_present, ) + expect(response).to render_template(:confirm_logout) + expect(response.body).to include service_provider.friendly_name end end diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index f743a6227a8..d20abd1a87b 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/spec/features/openid_connect/openid_connect_spec.rb @@ -312,8 +312,9 @@ end context 'when sending id_token_hint' do - it 'logout destroys the session' do - id_token = sign_in_get_id_token + it 'logout destroys the session when confirming logout' do + service_provider = ServiceProvider.find_by(issuer: 'urn:gov:gsa:openidconnect:test') + id_token = sign_in_get_id_token(client_id: service_provider.issuer) state = SecureRandom.hex @@ -323,19 +324,38 @@ id_token_hint: id_token, ) + 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 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 does not require state' do - id_token = sign_in_get_id_token + service_provider = ServiceProvider.find_by(issuer: 'urn:gov:gsa:openidconnect:test') + id_token = sign_in_get_id_token(client_id: service_provider.issuer) visit openid_connect_logout_path( post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout', id_token_hint: id_token, ) + 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 expect(page).to_not have_content(t('headings.account.login_info')) expect(page).to have_content(t('headings.sign_in_without_sp')) @@ -956,7 +976,8 @@ context 'signing out' do it 'redirects back to the client app and destroys the session' do - id_token = sign_in_get_id_token + service_provider = ServiceProvider.find_by(issuer: 'urn:gov:gsa:openidconnect:test') + id_token = sign_in_get_id_token(client_id: service_provider.issuer) state = SecureRandom.hex @@ -966,6 +987,15 @@ id_token_hint: id_token, ) + 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) + expect(oidc_redirect_url).to eq( UriService.add_params('gov.gsa.openidconnect.test://result/signout', state:), )