From e38a3df595bd1bcf84da69ea3acfb5c44f4dc742 Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Wed, 12 Jul 2017 23:37:40 -0400 Subject: [PATCH] Allow multiple account creation in same session **Why**: We noticed some users were running into an exception while trying to set their password. It turned out they were signing up for multiple accounts in the same session, but using different browsers, which is a common use case for users starting the process in a mobile app, and then confirming their email in a separate app or mobile browser. The bug was that we were only storing the request in the DB if the session didn't already contain the `request_id`, and we also deleted the request after a successful redirect back to the SP. Here's a concrete example, which I wrote a test for: - User starts by entering their email in browser 1, request 1 is stored in the DB and in the session - User confirms their email in browser 2, which looks up request 1 in the DB, and stores it in the session - User continues the account creation process successfully and continues on to the SP. At this point, request 1 is deleted from the DB - User goes back to browser 1 and makes a new request while the original session is still alive. Since the session is still alive and still contains request 1, the app doesn't store this new request, but the same `request_id` is passed on to the email confirmation - User confirms their email in browser 2, which passes because we don't validate the request_id at this point. - User tries to create their password, but when they submit the form, they get an exception because the app is trying to look up the request in the DB that matches the orginal `request_id`, but it was deleted earlier. **How**: Always store a request in the DB every time a new request is made by the SP. Don't reuse requests. The reason we reused requests before was to make it easier to delete requests in the DB that were no longer needed, but I obviously didn't think it through. We'll need to come up with a rake task or some other way to prevent the `ServiceProviderRequests` table from growing too large. --- .../concerns/saml_idp_auth_concern.rb | 12 +---- .../authorization_controller.rb | 12 +---- spec/controllers/saml_idp_controller_spec.rb | 6 +-- .../openid_connect/openid_connect_spec.rb | 52 ++++++++++++++----- spec/features/saml/loa1_sso_spec.rb | 48 +++++++++++++---- spec/support/features/session_helper.rb | 2 +- 6 files changed, 81 insertions(+), 51 deletions(-) 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}"