[LG-7684] Make state optional for OIDC Logout

app/controllers/openid_connect/logout_controller.rb

@@ -62,9 +62,11 @@ def build_logout_form
     def handle_successful_logout_request(result, redirect_uri)
       if require_logout_confirmation?
         analytics.oidc_logout_visited(**result.to_h.except(:redirect_uri))
-        @client_id = logout_params[:client_id]
-        @state = logout_params[:state]
-        @post_logout_redirect_uri = logout_params[:post_logout_redirect_uri]
+        @params = {
+          client_id: logout_params[:client_id],
+          post_logout_redirect_uri: logout_params[:post_logout_redirect_uri],
+        }
+        @params[:state] = logout_params[:state] if !logout_params[:state].nil?
         render :index
       else
         analytics.logout_initiated(**result.to_h.except(:redirect_uri))

app/forms/openid_connect_logout_form.rb

@@ -29,7 +29,9 @@ class OpenidConnectLogoutForm
             },
             if: :reject_id_token_hint?
   validates :post_logout_redirect_uri, presence: true
-  validates :state, presence: true, length: { minimum: RANDOM_VALUE_MINIMUM_LENGTH }
+  validates :state,
+            length: { minimum: RANDOM_VALUE_MINIMUM_LENGTH },
+            if: -> { !state.nil? }
 
   validate :id_token_hint_or_client_id_present,
            if: -> { accept_client_id? && !reject_id_token_hint? }
@@ -150,9 +152,10 @@ def redirect_uri
   end
 
   def logout_redirect_uri
-    uri = post_logout_redirect_uri unless errors.include?(:redirect_uri)
+    return nil if errors.include?(:redirect_uri)
+    return post_logout_redirect_uri unless state.present?
 
-    UriService.add_params(uri, state: state)
+    UriService.add_params(post_logout_redirect_uri, state: state)
   end
 
   def error_redirect_uri

app/views/openid_connect/logout/index.html.erb

@@ -7,11 +7,7 @@
 
 <%= button_to(
       openid_connect_logout_path,
-      params: {
-        client_id: @client_id,
-        state: @state,
-        post_logout_redirect_uri: @post_logout_redirect_uri,
-      },
+      params: @params,
       method: :delete,
       form_class: 'margin-top-5 margin-bottom-2',
       class: 'usa-button usa-button--big usa-button usa-button--full-width',

spec/features/openid_connect/openid_connect_spec.rb

@@ -158,6 +158,19 @@
         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
+
+        visit openid_connect_logout_path(
+          post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout',
+          id_token_hint: 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'))
+      end
     end
 
     context 'when permitting client_id' do
@@ -186,6 +199,22 @@
           expect(page).to have_content(t('headings.sign_in_without_sp'))
         end
 
+        it 'logout does not require state' do
+          client_id = 'urn:gov:gsa:openidconnect:test'
+          sign_in_get_id_token(client_id: client_id)
+
+          visit openid_connect_logout_path(
+            client_id: client_id,
+            post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout',
+          )
+          expect(page).to have_content(t('openid_connect.logout.heading', app_name: APP_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 '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)
@@ -259,6 +288,22 @@
       expect(page).to have_content(t('headings.sign_in_without_sp'))
     end
 
+    it 'logout does not require state' do
+      client_id = 'urn:gov:gsa:openidconnect:test'
+      sign_in_get_id_token(client_id: client_id)
+
+      visit openid_connect_logout_path(
+        client_id: client_id,
+        post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout',
+      )
+      expect(page).to have_content(t('openid_connect.logout.heading', app_name: APP_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 rejects requests that include id_token_hint' do
       id_token = sign_in_get_id_token
 

spec/forms/openid_connect_logout_form_spec.rb

@@ -64,10 +64,26 @@
         it 'has a successful response' do
           expect(result).to be_success
         end
+
+        context 'with missing state' do
+          let(:state) { nil }
+
+          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
       end
 
       context 'with an invalid form' do
-        let(:state) { nil }
+        let(:state) { 'ab' }
 
         it 'is not successful' do
           expect(result).to_not be_success
@@ -86,9 +102,8 @@
         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
+          it 'is valid' do
+            expect(valid?).to eq(true)
           end
         end
 
@@ -245,10 +260,22 @@
         it 'has a successful response' do
           expect(result).to be_success
         end
+
+        context 'without state' do
+          let(:state) { nil }
+
+          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
       end
 
       context 'with an invalid form' do
-        let(:state) { nil }
+        let(:state) { 'ab' }
 
         it 'is not successful' do
           expect(result).to_not be_success
@@ -267,9 +294,8 @@
         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
+          it 'is valid' do
+            expect(valid?).to eq(true)
           end
         end