diff --git a/app/controllers/openid_connect/logout_controller.rb b/app/controllers/openid_connect/logout_controller.rb index b5f9cb27183..9c419a9dbf2 100644 --- a/app/controllers/openid_connect/logout_controller.rb +++ b/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)) diff --git a/app/forms/openid_connect_logout_form.rb b/app/forms/openid_connect_logout_form.rb index 18a062bd213..92f19b1a6e2 100644 --- a/app/forms/openid_connect_logout_form.rb +++ b/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 diff --git a/app/views/openid_connect/logout/index.html.erb b/app/views/openid_connect/logout/index.html.erb index 52486d1b8ed..e9b8ddf4d11 100644 --- a/app/views/openid_connect/logout/index.html.erb +++ b/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', diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index 7afda448bb0..a43c3ed4e6d 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/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 diff --git a/spec/forms/openid_connect_logout_form_spec.rb b/spec/forms/openid_connect_logout_form_spec.rb index 566c1dc02a7..864d8eb1193 100644 --- a/spec/forms/openid_connect_logout_form_spec.rb +++ b/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