Allow confirming email in a different browser (#1265)

* Allow confirming email in a different browser

**Why**: Some people might visit the site via a Service Provider (SP)
in one browser, but open the email confirmation link in a different
browser. This means that anything that was previously stored in the
session in the first browser won't be available in the second browser.
The consequence is that the user won't be redirected back to the SP
after they finish creating their account.

**How**:
- Add a new ServiceProviderRequest DB table

When a request is made from an SP, whether it's via SAML or OpenID
Connect, we create a new ServiceProviderRequest with the following
attributes: the full request URL, a random unique UUID (because it's
not guaranteed that separate requests will have a unique identifier
attached to them), the issuer, and LOA. We also store these same
attributes in the session to support the sign in scenario (as opposed
to account creation).

Then, we check to see if the user is fully authenticated, and if not,
we redirect them to the `/sign_up/start` page, and include the
request_id in the params, as well as in the links on that page that
point to the account creation and sign in pages. Note that I moved the
logic that determines whether or not the landing page should appear,
from the sessions controller to the SAML and OpenID Connect controllers,
since they are the ones that know where the user should go. This adheres
to the Tell, Don't Ask principle, and allows us to remove the
`show_start_page` key from the session.

When the user chooses to create an account, the form contains the
request_id in a hidden field so it can pass it on to the registrations
controller. To include the request_id in the confirmation email, I had
to create a new method based on the one Devise uses, but with the
ability to pass in the request_id. Since Devise automatically sends the
confirmation email when the user is created (via a callback), I had to
override the original method to do nothing.

By having the request_id in the email, that means that if the user
visits the confirmation URL in a different browser, we can go back to
storing the SP info in the session by looking up the
ServiceProviderRequest whose uuid matches the `_request_id` parameter
from the URL in the email. By going back to using the session from then
on, it frees us from having to keep passing in the request_id in URLs
and in forms. This is done in
`EmailConfirmationsController#process_valid_confirmation_token`.

Note that the request_id is prefixed with an underscore in the email to
make sure that the URL ends with the confirmation token. By default,
Rails sorts params in `link_to` helpers alphabetically, but we don't
want `request_id` to come last because if the user chooses to copy the
link instead of clicking it, and if they don't copy it correctly, the
app won't find the ServiceProviderRequest, whereas an invalid
confirmation token will display an error to the user.

Once the user finishes creating their account, when they click the
button to continue to the SP, the app makes the original SP request
again, calling the `before_action`s in the SAML and OpenId Connect
controllers a second time. However, we don't want to create a new
ServiceProviderRequest because we want to be able to delete the
original one after the user goes back to the SP. We don't want the
ServiceProviderRequests table to grow indefinitely. The only way
to know for sure whether or not a ServiceProviderRequest that matches
the request URL already exists is to query on its `url` field. The
problem is, given that we want to index that field since it will be
queried on every SP request, the url field value is too big for the
Postgres btree index. There are solutions to this problem (that I
haven't implemented before), but I didn't feel it was worth the
trouble when we can rely on the presence of `sp_session[:request_id]`
to determine whether or not a new ServiceProviderRequest should be
created or not. We also use the `sp_session[:request_id]` to determine
which ServiceProviderRequest to delete by find a matching uuid. Then,
we can safely delete the `:sp` Hash from the session.

By keeping track of the SP via the request_id, we can also remove the
session timeout code and flash message that dealt with the scenario
where a user makes a request from an SP, but then remains on the
sign in page for longer than 8 minutes. Note that all this does is
preserve the branded experience on the sign in page. Allowing the
SP info to be restored after sign in will be implemented in a
separate PR.

Note that this doesn't include the same fix for the scenario where a
user opens the password reset link in a different browser. That will be
in a separate PR.

.reek

@@ -16,6 +16,9 @@ FeatureEnvy:
     - EncryptedSidekiqRedis#zrem
     - UserDecorator#should_acknowledge_recovery_code?
     - Pii::Attributes#[]=
+InstanceVariableAssumption:
+  exclude:
+    - User
 IrresponsibleModule:
   enabled: false
 ManualDispatch:
@@ -58,6 +61,7 @@ TooManyMethods:
     - ApplicationController
     - OpenidConnectAuthorizeForm
     - Idv::Session
+    - User
 UncommunicativeMethodName:
   exclude:
     - PhoneConfirmationFlow

app/controllers/application_controller.rb

@@ -57,21 +57,22 @@ def disable_caching
   def redirect_on_timeout
     return unless params[:timeout]
 
-    flash[:timeout] = decorated_session.timeout_flash_text
+    flash[:timeout] = t('notices.session_cleared', minutes: Figaro.env.session_timeout_in_minutes)
     redirect_to url_for(params.except(:timeout))
   end
 
   def current_sp
-    @current_sp ||= sp_from_sp_session || sp_from_params
+    @current_sp ||= sp_from_sp_session || sp_from_request_id
   end
 
   def sp_from_sp_session
     sp = ServiceProvider.from_issuer(sp_session[:issuer])
     sp if sp.is_a? ServiceProvider
   end
 
-  def sp_from_params
-    sp = ServiceProvider.from_issuer(params[:issuer])
+  def sp_from_request_id
+    issuer = ServiceProviderRequest.from_uuid(params[:request_id]).issuer
+    sp = ServiceProvider.from_issuer(issuer)
     sp if sp.is_a? ServiceProvider
   end
 

app/controllers/concerns/fully_authenticatable.rb

@@ -0,0 +1,14 @@
+module FullyAuthenticatable
+  def confirm_two_factor_authenticated(id = nil)
+    return redirect_to sign_up_start_url(request_id: id) unless user_signed_in?
+
+    return prompt_to_set_up_2fa unless current_user.two_factor_enabled?
+
+    prompt_to_enter_otp
+  end
+
+  def delete_branded_experience
+    ServiceProviderRequest.from_uuid(sp_session[:request_id]).delete
+    session.delete(:sp)
+  end
+end

app/controllers/concerns/saml_idp_auth_concern.rb

@@ -4,8 +4,8 @@ module SamlIdpAuthConcern
   included do
     before_action :validate_saml_request, only: :auth
     before_action :validate_service_provider_and_authn_context, only: :auth
+    before_action :store_saml_request, only: :auth
     before_action :add_sp_metadata_to_session, only: :auth
-    before_action :confirm_two_factor_authenticated, only: :auth
   end
 
   private
@@ -22,11 +22,26 @@ def validate_service_provider_and_authn_context
     render nothing: true, status: :unauthorized
   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
+      sp_request.loa = requested_authn_context
+      sp_request.url = request.original_url
+    end
+  end
+
   def add_sp_metadata_to_session
-    session[:sp] = { loa3: loa3_requested?,
-                     issuer: current_issuer,
-                     request_url: request.original_url,
-                     show_start_page: true }
+    return if sp_session[:request_id]
+
+    session[:sp] = {
+      issuer: current_issuer,
+      loa3: loa3_requested?,
+      request_id: @request_id,
+      request_url: request.original_url,
+    }
   end
 
   def requested_authn_context

app/controllers/concerns/unconfirmed_user_concern.rb

@@ -1,11 +1,11 @@
 module UnconfirmedUserConcern
   def with_unconfirmed_user
-    token = params[:confirmation_token]
-    @user = User.find_or_initialize_with_error_by(:confirmation_token, token)
-    @user = User.confirm_by_token(token) if @user.confirmed?
+    @confirmation_token = params[:confirmation_token]
+    @user = User.find_or_initialize_with_error_by(:confirmation_token, @confirmation_token)
+    @user = User.confirm_by_token(@confirmation_token) if @user.confirmed?
     @password_form = PasswordForm.new(@user)
 
-    yield
+    yield if block_given?
   end
 
   def after_confirmation_path_for(user)

app/controllers/openid_connect/authorization_controller.rb

@@ -1,35 +1,33 @@
 module OpenidConnect
   class AuthorizationController < ApplicationController
+    include FullyAuthenticatable
+
     before_action :build_authorize_form_from_params, only: [:index]
+    before_action :store_request, only: [:index]
     before_action :add_sp_metadata_to_session, only: [:index]
-    before_action :confirm_two_factor_authenticated
-    before_action :load_authorize_form_from_session, only: [:create, :destroy]
     before_action :apply_secure_headers_override, only: [:index]
+    before_action :confirm_fully_authenticated, only: [:create, :destroy]
+    before_action :load_authorize_form_from_session, only: [:create, :destroy]
 
     def index
+      return confirm_two_factor_authenticated(@request_id) unless user_fully_authenticated?
       return redirect_to verify_url if identity_needs_verification?
 
-      success = @authorize_form.valid?
-      analytics.track_event(Analytics::OPENID_CONNECT_REQUEST_AUTHORIZATION,
-                            success: success,
-                            client_id: @authorize_form.client_id,
-                            errors: @authorize_form.errors.messages)
+      track_index_action_analytics
 
       return create if already_allowed?
 
-      render(success ? :index : :error)
+      render(@success ? :index : :error)
     end
 
     def create
       result = @authorize_form.submit(current_user, session.id)
-      analytics_attributes = result.to_h.except(:redirect_uri)
 
-      analytics.track_event(
-        Analytics::OPENID_CONNECT_ALLOW, analytics_attributes
-      )
+      track_create_action_analytics(result)
 
       if (redirect_uri = result.extra[:redirect_uri])
         redirect_to redirect_uri
+        delete_branded_experience
       else
         render :error
       end
@@ -47,6 +45,23 @@ def destroy
 
     private
 
+    def track_index_action_analytics
+      @success = @authorize_form.valid?
+
+      analytics.track_event(Analytics::OPENID_CONNECT_REQUEST_AUTHORIZATION,
+                            success: @success,
+                            client_id: @authorize_form.client_id,
+                            errors: @authorize_form.errors.messages)
+    end
+
+    def track_create_action_analytics(result)
+      analytics_attributes = result.to_h.except(:redirect_uri)
+
+      analytics.track_event(
+        Analytics::OPENID_CONNECT_ALLOW, analytics_attributes
+      )
+    end
+
     def already_allowed?
       IdentityLinker.new(current_user, @authorize_form.client_id).already_linked?
     end
@@ -58,6 +73,12 @@ def apply_secure_headers_override
       )
     end
 
+    def confirm_fully_authenticated
+      return if user_fully_authenticated?
+
+      redirect_to root_url
+    end
+
     def identity_needs_verification?
       @authorize_form.loa3_requested? && decorated_user.identity_not_verified?
     end
@@ -84,10 +105,26 @@ def session_params
       user_session.delete(:openid_auth_request) || {}
     end
 
+    def store_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 = @authorize_form.client_id
+        sp_request.loa = @authorize_form.acr_values.sort.max
+        sp_request.url = request.original_url
+      end
+    end
+
     def add_sp_metadata_to_session
-      session[:sp] = { loa3: @authorize_form.loa3_requested?,
-                       issuer: @authorize_form.client_id,
-                       show_start_page: true }
+      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,
+      }
     end
   end
 end

app/controllers/saml_idp_controller.rb

@@ -6,18 +6,16 @@ class SamlIdpController < ApplicationController
   include SamlIdp::Controller
   include SamlIdpAuthConcern
   include SamlIdpLogoutConcern
+  include FullyAuthenticatable
 
   skip_before_action :verify_authenticity_token
   skip_before_action :handle_two_factor_authentication, only: :logout
 
   def auth
-    link_identity_from_session_data
-
-    needs_idv = identity_needs_verification?
-    analytics.track_event(Analytics::SAML_AUTH, @result.to_h.merge(idv: needs_idv))
-
-    return redirect_to verify_url if needs_idv
-
+    return confirm_two_factor_authenticated(@request_id) unless user_fully_authenticated?
+    process_fully_authenticated_user do |needs_idv|
+      return store_location_and_redirect_to_verify_url if needs_idv
+    end
     delete_branded_experience
     render_template_for(saml_response, saml_request.response_url, 'SAMLResponse')
   end
@@ -38,6 +36,20 @@ def logout
 
   private
 
+  def process_fully_authenticated_user
+    link_identity_from_session_data
+
+    needs_idv = identity_needs_verification?
+    analytics.track_event(Analytics::SAML_AUTH, @result.to_h.merge(idv: needs_idv))
+
+    yield needs_idv
+  end
+
+  def store_location_and_redirect_to_verify_url
+    store_location_for(:user, request.original_url)
+    redirect_to verify_url
+  end
+
   def render_template_for(message, action_url, type)
     domain = SecureHeadersWhitelister.extract_domain(action_url)
     override_content_security_policy_directives(form_action: ["'self'", domain])
@@ -48,9 +60,4 @@ def render_template_for(message, action_url, type)
       layout: false
     )
   end
-
-  def delete_branded_experience
-    session.delete(:sp)
-    session.delete('user_return_to')
-  end
 end

app/controllers/sign_up/email_confirmations_controller.rb

@@ -28,9 +28,30 @@ def process_successful_confirmation
 
     def process_valid_confirmation_token
       @confirmation_token = params[:confirmation_token]
-      flash.now[:notice] = t('devise.confirmations.confirmed_but_must_set_password')
+      flash[:notice] = t('devise.confirmations.confirmed_but_must_set_password')
       session[:user_confirmation_token] = @confirmation_token
-      render '/sign_up/passwords/new'
+      request_id = params.fetch(:_request_id, '')
+      add_sp_details_to_session unless request_id.empty?
+      redirect_to sign_up_enter_password_url(
+        request_id: request_id, confirmation_token: @confirmation_token
+      )
+    end
+
+    def add_sp_details_to_session
+      session[:sp] = {
+        issuer: sp_request.issuer,
+        loa3: loa3_requested?,
+        request_url: sp_request.url,
+        request_id: sp_request.uuid,
+      }
+    end
+
+    def sp_request
+      @_sp_request ||= ServiceProviderRequest.from_uuid(params[:_request_id])
+    end
+
+    def loa3_requested?
+      sp_request.loa == Saml::Idp::Constants::LOA3_AUTHN_CONTEXT_CLASSREF
     end
 
     def process_confirmed_user
@@ -46,7 +67,7 @@ def process_unsuccessful_confirmation
 
       @confirmation_token = params[:confirmation_token]
       flash[:error] = unsuccessful_confirmation_error
-      redirect_to sign_up_email_resend_path
+      redirect_to sign_up_email_resend_url(request_id: params[:_request_id])
     end
 
     def process_already_confirmed_user

app/controllers/sign_up/email_resend_controller.rb

@@ -8,7 +8,7 @@ def new
     end
 
     def create
-      @resend_email_confirmation_form = ResendEmailConfirmationForm.new(email_from_params)
+      @resend_email_confirmation_form = ResendEmailConfirmationForm.new(permitted_params)
       result = @resend_email_confirmation_form.submit
 
       analytics.track_event(Analytics::EMAIL_CONFIRMATION_RESEND, result.to_h)
@@ -22,16 +22,19 @@ def create
 
     private
 
-    def email_from_params
-      params[:resend_email_confirmation_form][:email]
+    def permitted_params
+      params.require(:resend_email_confirmation_form).permit(:email, :request_id)
     end
 
     def handle_valid_email
-      User.send_confirmation_instructions(email: form_email)
       session[:email] = form_email
       redirect_to sign_up_verify_email_path
     end
 
+    def request_id
+      permitted_params[:request_id]
+    end
+
     def form_email
       @resend_email_confirmation_form.email
     end

app/controllers/sign_up/passwords_controller.rb

@@ -2,6 +2,10 @@ module SignUp
   class PasswordsController < ApplicationController
     include UnconfirmedUserConcern
 
+    def new
+      with_unconfirmed_user
+    end
+
     def create
       with_unconfirmed_user do
         result = @password_form.submit(permitted_params)