diff --git a/app/controllers/concerns/saml_idp_auth_concern.rb b/app/controllers/concerns/saml_idp_auth_concern.rb index 6b7616d1e82..c2b5e8f9e91 100644 --- a/app/controllers/concerns/saml_idp_auth_concern.rb +++ b/app/controllers/concerns/saml_idp_auth_concern.rb @@ -23,8 +23,6 @@ def validate_service_provider_and_authn_context end def store_saml_request - return if sp_session[:request_id] - @request_id = SecureRandom.uuid ServiceProviderRequest.find_or_create_by(uuid: @request_id) do |sp_request| sp_request.issuer = current_issuer @@ -35,15 +33,7 @@ def store_saml_request end def add_sp_metadata_to_session - return if sp_session[:request_id] - - session[:sp] = { - issuer: current_issuer, - loa3: loa3_requested?, - request_id: @request_id, - request_url: request.original_url, - requested_attributes: requested_attributes, - } + StoreSpMetadataInSession.new(session: session, request_id: @request_id).call end def requested_authn_context diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index e903364b2c1..662f84c4525 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -76,8 +76,6 @@ def validate_authorize_form end def store_request - return if sp_session[:request_id] - client_id = @authorize_form.client_id @request_id = SecureRandom.uuid @@ -90,15 +88,7 @@ def store_request end def add_sp_metadata_to_session - return if sp_session[:request_id] - - session[:sp] = { - issuer: @authorize_form.client_id, - loa3: @authorize_form.loa3_requested?, - request_id: @request_id, - request_url: request.original_url, - requested_attributes: requested_attributes, - } + StoreSpMetadataInSession.new(session: session, request_id: @request_id).call end def requested_attributes diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index a08b2c16d98..f6fe4d29e6d 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -292,11 +292,11 @@ sp_request_id = ServiceProviderRequest.last.uuid expect(session[:sp]).to eq( - loa3: false, issuer: saml_settings.issuer, - request_id: sp_request_id, + loa3: false, request_url: @saml_request.request.original_url, - requested_attributes: [:email] + request_id: sp_request_id, + requested_attributes: ['email'] ) end diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index 6dad66fde81..c0367043c44 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/spec/features/openid_connect/openid_connect_spec.rb @@ -102,7 +102,6 @@ prompt: 'select_account' ) - sp_request_id = ServiceProviderRequest.last.uuid allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) sign_in_user(user) @@ -112,8 +111,6 @@ click_submit_default expect(current_url).to start_with('http://localhost:7654/auth/result') - expect(ServiceProviderRequest.from_uuid(sp_request_id)). - to be_a NullServiceProviderRequest expect(page.get_rack_session.keys).to_not include('sp') end @@ -134,7 +131,6 @@ prompt: 'select_account' ) - sp_request_id = ServiceProviderRequest.last.uuid allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) sign_in_user(user) @@ -150,8 +146,6 @@ click_submit_default expect(current_url).to start_with('http://localhost:7654/auth/result') - expect(ServiceProviderRequest.from_uuid(sp_request_id)). - to be_a NullServiceProviderRequest expect(page.get_rack_session.keys).to_not include('sp') end end @@ -238,16 +232,12 @@ sign_up_user_from_sp_without_confirming_email(email) end - sp_request_id = ServiceProviderRequest.last.uuid - perform_in_browser(:two) do confirm_email_in_a_different_browser(email) 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(ServiceProviderRequest.from_uuid(sp_request_id)). - to be_a NullServiceProviderRequest expect(page.get_rack_session.keys).to_not include('sp') end end @@ -423,15 +413,14 @@ end context 'visiting IdP via SP, then going back to SP and visiting IdP again' do - it 'maintains the request_id in the params' do + it 'displays the branded page' do visit_idp_from_sp_with_loa1 - sp_request_id = ServiceProviderRequest.last.uuid - expect(current_url).to eq sign_up_start_url(request_id: sp_request_id) + expect(current_url).to match(%r{http://www.example.com/sign_up/start\?request_id=.+}) visit_idp_from_sp_with_loa1 - expect(current_url).to eq sign_up_start_url(request_id: sp_request_id) + expect(current_url).to match(%r{http://www.example.com/sign_up/start\?request_id=.+}) end end @@ -496,6 +485,41 @@ end end + context 'creating two accounts during the same session' do + it 'allows the second account creation process to complete fully', email: true do + first_email = 'test1@test.com' + second_email = 'test2@test.com' + + perform_in_browser(:one) do + visit_idp_from_sp_with_loa1 + sign_up_user_from_sp_without_confirming_email(first_email) + end + + perform_in_browser(:two) do + confirm_email_in_a_different_browser(first_email) + click_button t('forms.buttons.continue') + 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') + end + + perform_in_browser(:one) do + visit_idp_from_sp_with_loa1 + sign_up_user_from_sp_without_confirming_email(second_email) + end + + perform_in_browser(:two) do + confirm_email_in_a_different_browser(second_email) + click_button t('forms.buttons.continue') + 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') + end + end + end + def visit_idp_from_sp_with_loa1(state: SecureRandom.hex) client_id = 'urn:gov:gsa:openidconnect:sp:server' nonce = SecureRandom.hex diff --git a/spec/features/saml/loa1_sso_spec.rb b/spec/features/saml/loa1_sso_spec.rb index b8e1e9bae7a..dc974e046c7 100644 --- a/spec/features/saml/loa1_sso_spec.rb +++ b/spec/features/saml/loa1_sso_spec.rb @@ -13,8 +13,6 @@ sign_up_user_from_sp_without_confirming_email(email) end - sp_request_id = ServiceProviderRequest.last.uuid - perform_in_browser(:two) do confirm_email_in_a_different_browser(email) @@ -32,8 +30,6 @@ click_on t('forms.buttons.continue') expect(current_url).to eq authn_request - expect(ServiceProviderRequest.from_uuid(sp_request_id)). - to be_a NullServiceProviderRequest expect(page.get_rack_session.keys).to_not include('sp') end end @@ -43,12 +39,9 @@ saml_authn_request = auth_request.create(saml_settings) visit saml_authn_request - sp_request_id = ServiceProviderRequest.last.uuid sign_in_live_with_2fa(user) expect(current_url).to eq saml_authn_request - expect(ServiceProviderRequest.from_uuid(sp_request_id)). - to be_a NullServiceProviderRequest expect(page.get_rack_session.keys).to_not include('sp') visit root_path @@ -139,16 +132,15 @@ end context 'visiting IdP via SP, then going back to SP and visiting IdP again' do - it 'maintains the request_id in the params' do + it 'displays the branded page' do authn_request = auth_request.create(saml_settings) visit authn_request - sp_request_id = ServiceProviderRequest.last.uuid - expect(current_url).to eq sign_up_start_url(request_id: sp_request_id) + expect(current_url).to match(%r{http://www.example.com/sign_up/start\?request_id=.+}) visit authn_request - expect(current_url).to eq sign_up_start_url(request_id: sp_request_id) + expect(current_url).to match(%r{http://www.example.com/sign_up/start\?request_id=.+}) end end @@ -169,6 +161,40 @@ end end + context 'creating two accounts during the same session' do + it 'allows the second account creation process to complete fully', email: true do + first_email = 'test1@test.com' + second_email = 'test2@test.com' + authn_request = auth_request.create(saml_settings) + + perform_in_browser(:one) do + visit authn_request + sign_up_user_from_sp_without_confirming_email(first_email) + end + + perform_in_browser(:two) do + confirm_email_in_a_different_browser(first_email) + click_button t('forms.buttons.continue') + + expect(current_url).to eq authn_request + expect(page.get_rack_session.keys).to_not include('sp') + end + + perform_in_browser(:one) do + visit authn_request + sign_up_user_from_sp_without_confirming_email(second_email) + end + + perform_in_browser(:two) do + confirm_email_in_a_different_browser(second_email) + click_button t('forms.buttons.continue') + + expect(current_url).to eq authn_request + expect(page.get_rack_session.keys).to_not include('sp') + end + end + end + def sign_in_and_require_viewing_personal_key(user) login_as(user, scope: :user, run_callbacks: false) Warden.on_next_request do |proxy| diff --git a/spec/support/features/session_helper.rb b/spec/support/features/session_helper.rb index 80a6bec46bc..f5831cf230b 100644 --- a/spec/support/features/session_helper.rb +++ b/spec/support/features/session_helper.rb @@ -243,7 +243,7 @@ def sign_up_user_from_sp_without_confirming_email(email) expect(current_url).to eq sign_up_email_url(request_id: sp_request_id) expect(page).to have_css('img[src*=sp-logos]') - submit_form_with_valid_email + submit_form_with_valid_email(email) expect(current_url).to eq sign_up_verify_email_url(request_id: sp_request_id) expect(last_email.html_part.body).to have_content "?_request_id=#{sp_request_id}"