From 89523a7ac34d9878411cb1b197c61f4640b38269 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Tue, 2 Feb 2021 15:14:39 -0600 Subject: [PATCH 1/6] Delete sp from session after successful handoff --- app/controllers/concerns/fully_authenticatable.rb | 1 + spec/features/openid_connect/openid_connect_spec.rb | 3 --- spec/features/saml/ial1_sso_spec.rb | 1 - spec/support/shared_examples/account_creation.rb | 2 -- 4 files changed, 1 insertion(+), 6 deletions(-) diff --git a/app/controllers/concerns/fully_authenticatable.rb b/app/controllers/concerns/fully_authenticatable.rb index bd0ce49dbcb..3b746f8b833 100644 --- a/app/controllers/concerns/fully_authenticatable.rb +++ b/app/controllers/concerns/fully_authenticatable.rb @@ -1,6 +1,7 @@ module FullyAuthenticatable def delete_branded_experience ServiceProviderRequestProxy.delete(request_id) + session.delete(:sp) end def request_id diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index c1cd2ca74e5..eb72db042e1 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/spec/features/openid_connect/openid_connect_spec.rb @@ -89,7 +89,6 @@ click_submit_default expect(current_url).to start_with('http://localhost:7654/auth/result') - expect(page.get_rack_session.keys).to include('sp') end it 'auto-allows and includes redirect_uris in CSP headers after an incorrect OTP' do @@ -116,7 +115,6 @@ click_submit_default expect(current_url).to start_with('http://localhost:7654/auth/result') - expect(page.get_rack_session.keys).to include('sp') end end @@ -488,7 +486,6 @@ continue_as(email) 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 include('sp') end end end diff --git a/spec/features/saml/ial1_sso_spec.rb b/spec/features/saml/ial1_sso_spec.rb index e72d5b186df..e9b3e3bd32a 100644 --- a/spec/features/saml/ial1_sso_spec.rb +++ b/spec/features/saml/ial1_sso_spec.rb @@ -32,7 +32,6 @@ continue_as(email) expect(current_url).to eq authn_request - expect(page.get_rack_session.keys).to include('sp') end end diff --git a/spec/support/shared_examples/account_creation.rb b/spec/support/shared_examples/account_creation.rb index dff1d2258a6..556cddcfabd 100644 --- a/spec/support/shared_examples/account_creation.rb +++ b/spec/support/shared_examples/account_creation.rb @@ -150,7 +150,6 @@ expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end - expect(page.get_rack_session.keys).to include('sp') end perform_in_browser(:one) do @@ -171,7 +170,6 @@ expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end - expect(page.get_rack_session.keys).to include('sp') end end end From fb22c321c087b287e74170972454341f7dcf9bc5 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Mon, 8 Feb 2021 10:30:47 -0600 Subject: [PATCH 2/6] Use session instead of sp_session for sp handoff bounce --- app/controllers/openid_connect/authorization_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 34cad9a6011..9a6ce6a0a9a 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -32,7 +32,7 @@ def check_sp_active end def check_sp_handoff_bounced - return unless SpHandoffBounce::IsBounced.call(sp_session) + return unless SpHandoffBounce::IsBounced.call(session) analytics.track_event(Analytics::SP_HANDOFF_BOUNCED_DETECTED) redirect_to bounced_url true @@ -56,7 +56,7 @@ def link_identity_to_service_provider def handle_successful_handoff track_events - SpHandoffBounce::AddHandoffTimeToSession.call(sp_session) + SpHandoffBounce::AddHandoffTimeToSession.call(session) redirect_to @authorize_form.success_redirect_uri delete_branded_experience end From 8065dbdf5006027a6f766cbcf3d9362342e648b7 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Mon, 8 Feb 2021 13:43:38 -0600 Subject: [PATCH 3/6] remove specs --- .../multiple_tabs_spec.rb | 9 -------- spec/support/shared_examples/sign_in.rb | 21 ------------------- 2 files changed, 30 deletions(-) delete mode 100644 spec/features/two_factor_authentication/multiple_tabs_spec.rb diff --git a/spec/features/two_factor_authentication/multiple_tabs_spec.rb b/spec/features/two_factor_authentication/multiple_tabs_spec.rb deleted file mode 100644 index 0593d891557..00000000000 --- a/spec/features/two_factor_authentication/multiple_tabs_spec.rb +++ /dev/null @@ -1,9 +0,0 @@ -require 'rails_helper' - -feature 'user interacts with 2FA across multiple browser tabs' do - include SpAuthHelper - include SamlAuthHelper - - it_behaves_like 'visiting 2fa when fully authenticated', :oidc - it_behaves_like 'visiting 2fa when fully authenticated', :saml -end diff --git a/spec/support/shared_examples/sign_in.rb b/spec/support/shared_examples/sign_in.rb index 7f1d5cf76b4..57f4f966254 100644 --- a/spec/support/shared_examples/sign_in.rb +++ b/spec/support/shared_examples/sign_in.rb @@ -41,27 +41,6 @@ end end -shared_examples 'visiting 2fa when fully authenticated' do |sp| - before { Timecop.freeze Time.zone.now } - after { Timecop.return } - - it 'redirects to SP after visiting a 2fa screen when fully authenticated', email: true do - ial1_sign_in_with_personal_key_goes_to_sp(sp) - - visit login_two_factor_options_path - - click_continue - continue_as - expect(current_url).to eq @saml_authn_request if sp == :saml - - if sp == :oidc - redirect_uri = URI(current_url) - - expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') - end - end -end - shared_examples 'signing in as IAL2 with personal key' do |sp| before { Timecop.freeze Time.zone.now } after { Timecop.return } From 0e2179d474c63b27c6d8efd498cdc307889a6f4e Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Mon, 8 Feb 2021 15:25:53 -0600 Subject: [PATCH 4/6] remove CSP check from OIDC logout --- app/controllers/openid_connect/logout_controller.rb | 3 --- spec/features/openid_connect/openid_connect_spec.rb | 5 ----- 2 files changed, 8 deletions(-) diff --git a/app/controllers/openid_connect/logout_controller.rb b/app/controllers/openid_connect/logout_controller.rb index fadd49041bc..f4cdc83e21e 100644 --- a/app/controllers/openid_connect/logout_controller.rb +++ b/app/controllers/openid_connect/logout_controller.rb @@ -1,8 +1,5 @@ module OpenidConnect class LogoutController < ApplicationController - include SecureHeadersConcern - - before_action :apply_secure_headers_override, only: [:index] def index @logout_form = OpenidConnectLogoutForm.new(params) diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index eb72db042e1..5f655101d65 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/spec/features/openid_connect/openid_connect_spec.rb @@ -135,11 +135,6 @@ "http://www.example.com/openid_connect/logout?id_token_hint=#{id_token}", ) - expect(page.response_headers['Content-Security-Policy']).to include( - 'form-action \'self\' gov.gsa.openidconnect.test://result '\ - 'gov.gsa.openidconnect.test://result/signout', - ) - 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')) From 5badb5072a7274d99f654b1ef69e99773f87b1d7 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Tue, 9 Feb 2021 09:56:19 -0600 Subject: [PATCH 5/6] add failing spec --- spec/features/users/sign_in_spec.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index c99a4416974..ca4f724757a 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -479,6 +479,25 @@ end end + context 'adds phone number after IAL1 sign in' do + it 'redirects to account page and not the SP' do + user = create(:user, :signed_up) + visit_idp_from_oidc_sp_with_loa1_prompt_login + fill_in_credentials_and_submit(user.email, user.password) + fill_in_code_with_last_phone_otp + click_submit_default + click_agree_and_continue + + visit account_path + click_on "+ #{t('account.index.phone_add')}" + fill_in :new_phone_form_phone, with: '415-555-0199' + click_continue + fill_in_code_with_last_phone_otp + click_submit_default + expect(current_path).to eq account_path + end + end + context 'invalid request_id' do it 'allows the user to sign in and does not try to redirect to any SP' do user = create(:user, :signed_up) From d32180f8bfebfe152328fef6f114e2630544e945 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Tue, 9 Feb 2021 10:07:55 -0600 Subject: [PATCH 6/6] refactor SpHandoffBounce into one class --- .../openid_connect/authorization_controller.rb | 4 ++-- app/services/sp_handoff_bounce.rb | 13 +++++++++++++ .../add_handoff_time_to_session.rb | 7 ------- app/services/sp_handoff_bounce/is_bounced.rb | 11 ----------- 4 files changed, 15 insertions(+), 20 deletions(-) create mode 100644 app/services/sp_handoff_bounce.rb delete mode 100644 app/services/sp_handoff_bounce/add_handoff_time_to_session.rb delete mode 100644 app/services/sp_handoff_bounce/is_bounced.rb diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 9a6ce6a0a9a..e978908321d 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -32,7 +32,7 @@ def check_sp_active end def check_sp_handoff_bounced - return unless SpHandoffBounce::IsBounced.call(session) + return unless SpHandoffBounce.is_bounced?(session) analytics.track_event(Analytics::SP_HANDOFF_BOUNCED_DETECTED) redirect_to bounced_url true @@ -56,7 +56,7 @@ def link_identity_to_service_provider def handle_successful_handoff track_events - SpHandoffBounce::AddHandoffTimeToSession.call(session) + SpHandoffBounce.add_handoff_time_to_session(session) redirect_to @authorize_form.success_redirect_uri delete_branded_experience end diff --git a/app/services/sp_handoff_bounce.rb b/app/services/sp_handoff_bounce.rb new file mode 100644 index 00000000000..ffcf919bab0 --- /dev/null +++ b/app/services/sp_handoff_bounce.rb @@ -0,0 +1,13 @@ +class SpHandoffBounce + def self.is_bounced?(session) + start_time = session[:sp_handoff_start_time] + return if start_time.blank? + tz = Time.zone + start_time = tz.parse(start_time) if start_time.class == String + tz.now <= (start_time + AppConfig.env.sp_handoff_bounce_max_seconds.to_i.seconds) + end + + def self.add_handoff_time_to_session(session) + session[:sp_handoff_start_time] = Time.zone.now + end +end diff --git a/app/services/sp_handoff_bounce/add_handoff_time_to_session.rb b/app/services/sp_handoff_bounce/add_handoff_time_to_session.rb deleted file mode 100644 index feaa37377eb..00000000000 --- a/app/services/sp_handoff_bounce/add_handoff_time_to_session.rb +++ /dev/null @@ -1,7 +0,0 @@ -module SpHandoffBounce - class AddHandoffTimeToSession - def self.call(session) - session[:sp_handoff_start_time] = Time.zone.now - end - end -end diff --git a/app/services/sp_handoff_bounce/is_bounced.rb b/app/services/sp_handoff_bounce/is_bounced.rb deleted file mode 100644 index a0673c48804..00000000000 --- a/app/services/sp_handoff_bounce/is_bounced.rb +++ /dev/null @@ -1,11 +0,0 @@ -module SpHandoffBounce - class IsBounced - def self.call(session) - start_time = session[:sp_handoff_start_time] - return if start_time.blank? - tz = Time.zone - start_time = tz.parse(start_time) if start_time.class == String - tz.now <= (start_time + AppConfig.env.sp_handoff_bounce_max_seconds.to_i.seconds) - end - end -end