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. (cherry picked from commit e38a3df #1542)
18F · Jul 13, 2017 · db022fd · db022fd
1 parent 77f8b0b
commit db022fd
Show file tree

Hide file tree

Showing 6 changed files with 81 additions and 51 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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 = '[email protected]'
+      second_email = '[email protected]'
+
+      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
@@ -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 = '[email protected]'
+      second_email = '[email protected]'
+      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
@@ -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}"