Always require confirmation of user logout in OIDC Logout

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

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,18 +78,19 @@
               ),
             )
             expect(@analytics).to have_logged_event(
-              'Logout Initiated',
+              'OIDC Logout Page Visited',
               success: true,
               client_id: service_provider.issuer,
               client_id_parameter_present: false,
               id_token_hint_parameter_present: true,
               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
 

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:),
       )