From 913b4a24324cecb5895018cb1bdc6d6df788e29d Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Wed, 19 Jul 2017 18:50:23 -0400 Subject: [PATCH] Don't delete SP info from session after redirect **Why**: I don't think there's a valid reason to do that. Digging into the git history, it looks like the deletion of the SP info from the session was introduced in the same PR that implemented the branded experience. I believe the reasoning was that unless the branded experience was deleted, returning to the IdP after signing out from the SP would keep the branded experience around, which is not the case since the entire session is deleted upon logout. Deleting the SP info from the session is resulting in an exception in the following scenario: - A user launches a mobile app that integrates with login.gov - The user completes the account creation process on their mobile device - Once they arrive on the completions page and click "Continue", they are prompted to launch the mobile app. Whether the user chooses to launch the app or click Cancel, the IdP web page stays where it is. - If the user comes back to the IdP page at a later time and tries to hit the Continue button again, they will get an error because `CompletionsController#update` depends on `sp_session[:request_url]` being present. **How**: - Don't delete the SP info from the session - To allow the user to visit the IdP after being redirected back to the SP, update `SessionsController#check_user_needs_redirect` to redirect to `signed_in_path` instead of `after_sign_in_path_for(current_user)` because the latter will redirect back to `sp_session[:request_url]` now that we are no longer deleting `sp_session`. --- .../concerns/fully_authenticatable.rb | 1 - app/controllers/users/sessions_controller.rb | 2 +- spec/controllers/saml_idp_controller_spec.rb | 4 ++-- .../users/sessions_controller_spec.rb | 17 +++++++++++++++++ .../openid_connect/openid_connect_spec.rb | 10 +++++----- spec/features/saml/loa1_sso_spec.rb | 13 ++++++++----- 6 files changed, 33 insertions(+), 14 deletions(-) diff --git a/app/controllers/concerns/fully_authenticatable.rb b/app/controllers/concerns/fully_authenticatable.rb index f5e7eb0665c..a0326a0ff11 100644 --- a/app/controllers/concerns/fully_authenticatable.rb +++ b/app/controllers/concerns/fully_authenticatable.rb @@ -9,7 +9,6 @@ def confirm_two_factor_authenticated(id = nil) def delete_branded_experience ServiceProviderRequest.from_uuid(sp_session[:request_id]).delete - session.delete(:sp) end def request_id diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index ae84123fdb3..078c3739a06 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -44,7 +44,7 @@ def timeout def check_user_needs_redirect if user_fully_authenticated? - redirect_to after_sign_in_path_for(current_user) + redirect_to signed_in_path elsif current_user sign_out end diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index f6fe4d29e6d..025cf9ed02a 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -306,8 +306,8 @@ saml_get_auth(saml_settings) end - it 'deletes SP metadata from session' do - expect(session.key?(:sp)).to eq(false) + it 'does not delete SP metadata from session' do + expect(session.key?(:sp)).to eq(true) end end end diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb index 9be166701c8..b981880208e 100644 --- a/spec/controllers/users/sessions_controller_spec.rb +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -323,5 +323,22 @@ get :new end end + + context 'with fully authenticated user who has a pending profile' do + it 'redirects to the verify profile page' do + profile = create( + :profile, + deactivation_reason: :verification_pending, + phone_confirmed: false, + pii: { otp: 'abc123', ssn: '6666', dob: '1920-01-01' } + ) + user = profile.user + + stub_sign_in(user) + get :new + + expect(response).to redirect_to verify_account_path + end + end end end diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index 9c1d90cc899..ab2516c478b 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/spec/features/openid_connect/openid_connect_spec.rb @@ -111,7 +111,7 @@ click_submit_default expect(current_url).to start_with('http://localhost:7654/auth/result') - expect(page.get_rack_session.keys).to_not include('sp') + expect(page.get_rack_session.keys).to include('sp') end it 'auto-allows and sets the correct CSP headers after an incorrect OTP' do @@ -146,7 +146,7 @@ click_submit_default expect(current_url).to start_with('http://localhost:7654/auth/result') - expect(page.get_rack_session.keys).to_not include('sp') + expect(page.get_rack_session.keys).to include('sp') end end @@ -238,7 +238,7 @@ click_button t('forms.buttons.continue') redirect_uri = URI(current_url) expect(redirect_uri.to_s).to start_with('gov.gsa.openidconnect.test://result') - expect(page.get_rack_session.keys).to_not include('sp') + expect(page.get_rack_session.keys).to include('sp') end end end @@ -501,7 +501,7 @@ redirect_uri = URI(current_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') - expect(page.get_rack_session.keys).to_not include('sp') + expect(page.get_rack_session.keys).to include('sp') end perform_in_browser(:one) do @@ -515,7 +515,7 @@ redirect_uri = URI(current_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') - expect(page.get_rack_session.keys).to_not include('sp') + expect(page.get_rack_session.keys).to include('sp') end end end diff --git a/spec/features/saml/loa1_sso_spec.rb b/spec/features/saml/loa1_sso_spec.rb index dc974e046c7..669bb18703a 100644 --- a/spec/features/saml/loa1_sso_spec.rb +++ b/spec/features/saml/loa1_sso_spec.rb @@ -30,19 +30,22 @@ click_on t('forms.buttons.continue') expect(current_url).to eq authn_request - expect(page.get_rack_session.keys).to_not include('sp') + expect(page.get_rack_session.keys).to include('sp') end end it 'takes user to the service provider, allows user to visit IDP' do + allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) + user = create(:user, :signed_up) saml_authn_request = auth_request.create(saml_settings) visit saml_authn_request - sign_in_live_with_2fa(user) + click_link t('links.sign_in') + fill_in_credentials_and_submit(user.email, user.password) + click_submit_default expect(current_url).to eq saml_authn_request - expect(page.get_rack_session.keys).to_not include('sp') visit root_path expect(current_path).to eq account_path @@ -177,7 +180,7 @@ click_button t('forms.buttons.continue') expect(current_url).to eq authn_request - expect(page.get_rack_session.keys).to_not include('sp') + expect(page.get_rack_session.keys).to include('sp') end perform_in_browser(:one) do @@ -190,7 +193,7 @@ click_button t('forms.buttons.continue') expect(current_url).to eq authn_request - expect(page.get_rack_session.keys).to_not include('sp') + expect(page.get_rack_session.keys).to include('sp') end end end