Allow confirming email in a different browser #1265

monfresh · 2017-03-24T13:56:05Z

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_actions 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.

zachmargolis · 2017-03-24T14:15:36Z

app/controllers/openid_connect/authorization_controller.rb

+                            errors: @authorize_form.errors.messages)
+    end
+
+    def track_create_action_analytics(result)


is this method extraction to reduce the ABC size of the #create method?

zachmargolis · 2017-03-24T14:16:48Z

app/controllers/saml_idp_controller.rb

+
+    yield true if needs_idv
+
+    yield false


this will yield twice when true, is that intentional?

That was not intentional, but it doesn't seem to cause a problem. However, I'll fix it so that it reads yield needs_idv. That's what I get for coding late at night.

zachmargolis · 2017-03-24T14:19:24Z

app/models/user.rb

+  end
+
+  def send_custom_confirmation_instructions(id = nil)
+    generate_confirmation_token! unless @raw_confirmation_token


Where does @ raw_confirmation_token get set? Where does generate_confirmation_token! come from? Devise?

Yes, I copied Devise's send_confirmation_instructions with the only modification being the ability to pass in the request_id to the mailer. I'll add a comment.

zachmargolis · 2017-03-24T14:21:13Z

app/models/service_provider_request.rb

@@ -0,0 +1,5 @@
+class ServiceProviderRequest < ActiveRecord::Base
+  def self.find_by(*args)


I think overriding find_by can lead to confusing behavior especially since will never return nil.

What about making a new method?

def self.find_by_or_null(*args) find_by(*args) || NullServiceProviderRequest.new end

zachmargolis · 2017-03-24T14:22:30Z

spec/controllers/openid_connect/authorization_controller_spec.rb

@@ -99,16 +99,21 @@

    context 'user is not signed in' do
      it 'redirects to login' do


update the comment?

it 'redirects to login with the request_id' or something?

Yes, will do.

zachmargolis · 2017-03-24T14:25:01Z

spec/support/features/session_helper.rb

+
+      enter_2fa_code
+
+      # expect(page).to have_css('img[src*=sp-logos]')


is this commented out on purpose?

Yes. I noticed that we currently do not preserve the branded experience on the recovery code page. I opened an issue asking if that was intentional or not but haven't heard back yet. I would assume that we would show the branded nav bar throughout the whole creation process.

huh! create a ticket?

Yep. In my previous comment I said that I opened an issue already. https://github.com/18F/identity-private/issues/1692

zachmargolis · 2017-03-24T14:32:56Z

app/controllers/concerns/fully_authenticatable.rb

+
+    return prompt_to_set_up_2fa unless current_user.two_factor_enabled?
+
+    prompt_to_enter_otp


this line is red from codeclimate, do we not have test coverage?

Probably not for this scenario, which is where a user makes a request from an SP while they are already signed in with email and password only and haven't entered their OTP yet. I'll add a test.

link to commit with spec?

darn looks like that was squashed?

Here you go: d1ba3b9
Scroll down to loa1_sso_spec

monfresh · 2017-03-24T17:22:12Z

Thanks for the feedback. I addressed it in second commit. PTAL. I'll also fix conflicts with master and push again.

zachmargolis

LGTM!

jessieay

Left a round of feedback. Could probably comb through more but figured I'd submit my comments so you have time to respond :)

jessieay · 2017-03-24T17:20:48Z

spec/views/devise/sessions/new.html.slim_spec.rb

@@ -26,7 +26,7 @@

    expect(rendered).
      to have_link(
-        t('links.create_account'), href: sign_up_email_path
+        t('links.create_account'), href: sign_up_email_url(request_id: nil)


should an href be a _path?

I believe it needs to be _url to include params. Plus, I think we're moving to using _url exclusively. I would assume this would fail if it had to be _path.

We are moving to _url for redirects since that is in line with the HTTP spec but we are not moving to _url for everything. There are some benefits to _path :)

AFAIK, you don't need _url to include params/

my memory is that _url came up only because the HTTP spec requires it for Redirect responses. That's all. I agree that _path is fine for href values etc, since those are all client-side anyway.

I just tried changing to _path and it failed. _url is required if you want to check for params as well.

jessieay · 2017-03-24T17:32:40Z

spec/support/features/session_helper.rb

+
+      enter_2fa_code
+
+      # expect(page).to have_css('img[src*=sp-logos]')


huh! create a ticket?

jessieay · 2017-03-24T17:34:08Z

spec/support/features/session_helper.rb

+      open_email(email)
+      visit_in_email(t('mailer.confirmation_instructions.link_text'))
+
+      expect(page).to have_css('img[src*=sp-logos]')


I wouldn't necessarily expect to see expectations in a setup step like this.

jessieay · 2017-03-24T17:34:29Z

spec/support/features/session_helper.rb

@@ -213,5 +213,91 @@ def session_store
      config = Rails.application.config
      config.session_store.new({}, config.session_options)
    end
+
+    def sign_up_and_2fa_loa1_user_who_came_from_sp


this method is AWESOME and LONG - any way to break it up into smaller named chunks for easier reading?

Yep, I knew you would bring this up, but thought I'd get this out for review and then clean it up.

you know me soooo well 🥇

jessieay · 2017-03-24T17:35:22Z

spec/models/user_spec.rb

@@ -11,10 +11,10 @@
    it { is_expected.to have_many(:events) }
  end

-  it 'should only send one email during creation' do
+  it 'does not send an email when #create is called' do


did we change this behavior intentionally? If so, seems potentially unrelated to moving SP info from the session into the DB

It is very related, and it was intentional. As mentioned in my commit message, Devise automatically calls send_confirmation_instructions when the user is created (via a model callback). Unfortunately, Devise does not make it possible to send it additional parameters that can be included in the email. Therefore, I had to create a new send_custom_confirmation_instructions method that accepts an argument, and at the same time override Devise's send_confirmation_instructions to do nothing. Otherwise, the user would receive 2 emails.

jessieay · 2017-03-24T17:53:33Z

spec/features/saml/loa1_sso_spec.rb


      expect(current_path).to eq sign_up_completed_path

      click_on t('forms.buttons.continue_to', sp: 'Your friendly Government Agency')

      expect(current_url).to eq saml_authn_request
+      expect(ServiceProviderRequest.from_uuid(sp_request_id)).


I wouldn't expect to see DB queries in a feature spec. My rule of thumb is that the expectations should be looking for things that the end user can see or experience, like page content or a redirect.

Any way to test this in a controller or other type of spec instead?

Not that I can think of. I'm open to ideas if you have them. I want to be able to isolate the creation of the ServiceProviderRequest and then make sure it gets deleted. In a controller spec, the entire action will run in one step. AFAIK, I can't use a controller spec to verify that a particular ServiceProviderRequest was created since it will get deleted after calling the action. This controller is special in that it gets called twice, but we only want some of what it does to be called once. I think this is best tested with a feature spec.

maybe a request spec? I usually think of feature specs as "end user" focused, so no database-related expectations. Request specs are more backend-y so perhaps this would work there?

It might, but I'm not sure it's worth writing a whole new spec that in essence duplicates what the feature spec does, just to test a single expectation.

I would expect to see a feature test using perform_in_browser to show the full end-to-end multiple browser interplay, from the user's perspective. Am I missing seeing that?

Ah, yes, thanks for reminding me. I meant to add a step that deletes the session. Doing that now.

Fixed in c630146
PTAL

jessieay · 2017-03-24T17:55:52Z

db/migrate/20170321170516_create_service_provider_requests.rb

+
+      t.timestamps null: false
+
+      t.index :uuid


unique: true ?

jessieay · 2017-03-24T17:56:54Z

config/routes.rb

@@ -92,13 +92,13 @@
  get '/sign_up/email/confirm' => 'sign_up/email_confirmations#create',
      as: :sign_up_create_email_confirmation
  get '/sign_up/enter_email' => 'sign_up/registrations#new', as: :sign_up_email
+  post '/sign_up/enter_email' => 'sign_up/registrations#create', as: :sign_up_register


jessieay · 2017-03-24T17:57:30Z

app/views/sign_up/registrations/show.html.slim

@@ -40,8 +41,5 @@
          p.mb2 = t('devise.registrations.start.bullet_5_html')
          p.mb3 = link_to \
            t('devise.registrations.start.learn_more'), MarketingSite.help_url, target: '_blank'
-  .center
-    - ab_test(:demo) do |variant, _|


nice! glad we are removing this (assuming it's unused). Thoughts about removing in a separate PR for clarity?

This is not removing the A/B testing entirely. It is removing a duplicate div that was causing the LOA3 spec to fail because it found 2 elements matching the "Get started" button. I suppose I could edit the spec to use the first match and then remove this in a separate PR. What do you think?

separate might be nice since this PR is quite large as-is, but up to you!

jessieay · 2017-03-24T18:48:41Z

app/controllers/openid_connect/authorization_controller.rb

-                       show_start_page: true }
+      return if sp_session[:request_id]
+
+      session[:sp] = {


now that we have the sp data in the database, why do we need to store anything other than the UUI Din the session?

Good question. For several reasons.

We need to store the request URL to make sure that existing users who sign in are redirected back to the SP. It's easier and faster to query the session rather than the DB, and easier than making sure the request_id is available on all forms and URLs throughout the auth process.

The branded nav bar depends on the presence of either the issuer in the session or the request_id in the params. In the scenario where a user is signing in, we don't include the request_id params throughout the sign in process because that's only necessary for scenarios where users are switching browsers. Therefore, we need the issuer in the session.

We have several places that check for sp_session[:loa3] in order to do stuff. In order to replace that, we'd need to pass in the request_id on every form and URL, and it would also mean a lot more DB lookups.

I think @monfresh has taken the right approach. Populating the session once from the db is cleaner, since there is already a single link between the session and the browser (the session cookie).

jessieay · 2017-03-24T21:48:22Z

Hey @monfresh I just pulled this down locally to test.

CHROME:

visit http://localhost:3000/test/saml
entered my email

Firefox:

confirmed email
set password
confirmed phone
confirmed recovery code

after this flow, I was redirected to my profile page rather than the SP, which makes me think that the SP-related info was not being saved properly in Firefox. Am I doing something wrong?

monfresh · 2017-03-24T22:26:55Z

@jessieay You need to come from the sp-rails app. The test/saml endpoint doesn't call SamlIdpController#auth, which is where all the magic happens.

monfresh · 2017-03-24T22:34:37Z

Here are the scenarios I could think of and wrote tests for. Please see if I missed any.

User comes from SP, clicks "Get started", submits form with invalid email => page displays inline error as before, and URL still contains request_id param, and the form still contains the correct request_id in a hidden field.
User comes from SP, clicks "Get started", submits form with valid email => user is redirected to /sign_up/verify_email and receives an email with a link containing the correct request id in a param named _request_id (note the underscore prefix) and the order of the params should be _request_id, then confirmation_token.
- User clicks "Resend" on the /sign_up/verify_email page. => User receives another confirmation email with the same correct params in the link
- User clicks "try a different email" on the /sign_up/verify_email page, then enters a different email => User receives an email with the correct link
- User clicks the link in the email => User is confirmed and is redirected to /sign_up/enter_password with the request_id in the params
- User sits idle until session times out => SP branding should remain and continuing the account creation should redirect back to SP (I just thought of this new scenario and the second part is failing)
User comes from SP, clicks "Get started", submits form with valid email, opens email link in a different browser, disables JS in the browser, then enters an invalid password => page displays inline error as before, and URL still contains request_id param, and the form still contains the correct request_id in a hidden field.
- Enter correct password and complete the account creation => you should be redirected to SP
User comes from SP, clicks "Get started", then stays idle until session times out => branded nav should remain visible, and completing the account creation should redirect back to SP

monfresh · 2017-03-27T15:56:33Z

@jessieay I believe I addressed your feedback. We need to get this in to the RC so it can be deployed and tested. If you find anything else, let me know and I'll address it in a separate PR. Thanks!

pkarman · 2017-03-27T17:14:15Z

spec/support/features/session_helper.rb

@@ -257,6 +257,8 @@ def sign_up_and_2fa_loa1_user_who_came_from_sp
      expect(current_url).to eq sign_up_verify_email_url(request_id: sp_request_id, resend: true)
      expect(page).to have_css('img[src*=sp-logos]')

+      delete_sp_info_from_session_to_simulate_user_switching_browsers


why not actually switch browsers? perform_in_browser lets you do that.

monfresh · 2017-03-27T17:50:51Z

Used Perform in browser here: 4e61357

**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.

jessieay · 2017-03-27T17:18:15Z

app/controllers/saml_idp_controller.rb

+    needs_idv = identity_needs_verification?
+    analytics.track_event(Analytics::SAML_AUTH, @result.to_h.merge(idv: needs_idv))
+
+    yield needs_idv


why are we calling yield here instead of just returning needs_idv? It looks like that is a boolean value?

Because this method is meant to be called with a block so that the auth method can return if the user needs_idv.

jessieay · 2017-03-27T17:19:22Z

app/views/devise/mailer/confirmation_instructions.html.slim

@@ -11,13 +11,16 @@ table.button.expanded.large.radius
              td
                center
                  = link_to t('mailer.confirmation_instructions.link_text'), \
-                    sign_up_create_email_confirmation_url(confirmation_token: @token), \
+                    sign_up_create_email_confirmation_url(_request_id: \


why are we naming this param _request_id instead of request_id?

Please read the initial commit message 😄

to recap for others:

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.

I find this logic sensible, although I am wondering how we can communicate the reasoning for this underscore to future people / our future selves, since it does feel a bit mysterious when browsing the code.

Good point. I typically use git log -S or I look up the file in question in GitHub and look at the history to find out which commit introduced something and then I look up the PR, assuming the commit message explains the change since we've been pretty good about doing that.

Yeah I think that is ideal, but that cannot always be depended on. I will noodle on ways we can make this more obvious via a future PR, perhaps through some specs

jessieay · 2017-03-27T17:22:50Z

app/controllers/sign_up/email_confirmations_controller.rb

+    end
+
+    def current_sp
+      @_current_sp ||= ServiceProviderRequest.from_uuid(params[:_request_id])


wondering if sp_request_id would be a clearer param name?

Yes, current_sp_request. Good catch.

Fixed here: cdf1da9
PTAL

jessieay · 2017-03-27T17:25:42Z

app/controllers/sign_up/registrations_controller.rb

@@ -18,6 +18,7 @@ def new
      @register_user_email_form = RegisterUserEmailForm.new
      session[:sign_up_init] = true
      analytics.track_event(Analytics::USER_REGISTRATION_ENTER_EMAIL_VISIT)
+      render :new, locals: { request_id: nil }


I know that nil.to_s is empty string, but perhaps it would be clearer to just use '' here?

Where does nil.to_s come from? I want to pass in a nil value to the form. When the form renders, the request_id hidden input field won't have a value at all.

anything in the view is going to inherently have to_s called on it. So this will render in the hidden input field as '', I believe

That doesn't seem to be the case for nil:

<input class="block col-12 field hidden" type="hidden" name="user[request_id]" id="user_request_id">

It works either way, but I think nil conveys the intent better, which is that by default, we don't want any value at all, not even an empty string, because it can imply presence. For example, if we had a conditional in the view that asked if request_id, then it would pass with an empty string, but not if it was nil.

jessieay · 2017-03-27T17:26:37Z

app/forms/resend_email_confirmation_form.rb

+  end
+
+  def user
+    @_user ||= (email.presence && User.find_with_email(email)) || NonexistentUser.new


I always thought Nonexistent was two words - TIL!

jessieay · 2017-03-27T17:28:44Z

app/models/user.rb

+    # no-op
+  end
+
+  # This is basically Devise's send_confirmation_instructions method copied


This comment is very clear / accurate...what worries me about this comment is that what if we change the method and don't update the comment? I might just leave the comment as "Overrride send_confirmation_instructions from Devise" or no comment at all ;)

thoughts on my comment about this comment, @monfresh ?

Yes, just remembered: 03a4683

looks good!

monfresh · 2017-03-27T19:56:12Z

Thanks for the thorough review, @jessieay! Is this good to go now?

jessieay · 2017-03-27T20:01:06Z

spec/support/features/session_helper.rb

@@ -213,5 +213,143 @@ def session_store
      config = Rails.application.config
      config.session_store.new({}, config.session_options)
    end
+
+    def sign_up_user_from_sp_without_confirming_email(email)


so is the plan to break this up in a future PR? I commented about doing that before and I remember that you replied but the thread is gone now...

Oh I see you did break it up...WOW still so long.

jessieay

💯 this is awesme - so glad we are going to fix this before release number 1!

* 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.

**Why**: When we first introduced the `ServiceProvideRequests` table (#1265), it was to allow users to go through the account creation process across multiple browsers and still end up back at the SP. This initial implementation did not result in any stray entries in the `ServiceProvideRequests` table in the typical sign in scenario. However, we noticed a bug with this implementation that prevented users from creating multiple accounts during the same session, so we removed the guard clause in the SAML and OIDC controllers (#1542), resulting in stray entries being created, which we acknowledged in the PR commit message and proposed using a rake task to clean them up, but never followed up. Fast forward about a year later, and the table now was millions of entries and is causing DB errors and slowdowns. Upon revisiting the history of this table, and thinking about the problem some more, I found a solution that allows us to support all scenarios while also preventing stray entries. This solution also reduces the total amount of DB transactions and cuts writes in half. **Before:** 1. User (who is not signed in) makes request A from SP 2. Request A is stored in the DB and in the session by the controller (this requires a DB read) 3. User signs in + 2FA 4. ApplicationController looks up Request A from the session to redirect the user back to the entry point controller 5. The controller stores Request A again as a new entry in the DB, which we'll call Request B. The info is stored in the session again as well, resulting in another DB read. 6. User is redirected back to SP, and Request B is deleted from the DB, but Request A remains in the DB. Total DB transactions: 2 writes, 2 reads, 1 delete **After:** 1. User (who is not signed in) makes request A from SP 2. Request A is stored in the DB and in the session by the controller (this requires a DB read) 3. User signs in + 2FA 4. ApplicationController looks up Request A from the session to redirect the user back to the entry point controller 5. The controller compares the request stored in the session (DB read) with the one that was just received by the controller and sees that they are both from the same SP, and does create a new entry in the DB 6. User is redirected back to SP, and Request A is deleted from the DB. Total DB transactions: 1 write, 2 reads, 1 delete **How**: Instead of using the presence of the request_id in the session as the guard clause, compare the SP that made the last request with the one stored in the session. If they're the same, don't store a new entry in the DB, otherwise, delete the entry that matches the one in the session, and then create a new entry.

* LG-667 Don't create stray ServiceProvideRequests **Why**: When we first introduced the `ServiceProvideRequests` table (#1265), it was to allow users to go through the account creation process across multiple browsers and still end up back at the SP. This initial implementation did not result in any stray entries in the `ServiceProvideRequests` table in the typical sign in scenario. However, we noticed a bug with this implementation that prevented users from creating multiple accounts during the same session, so we removed the guard clause in the SAML and OIDC controllers (#1542), resulting in stray entries being created, which we acknowledged in the PR commit message and proposed using a rake task to clean them up, but never followed up. Fast forward about a year later, and the table now was millions of entries and is causing DB errors and slowdowns. Upon revisiting the history of this table, and thinking about the problem some more, I found a solution that allows us to support all scenarios while also preventing stray entries. This solution also reduces the total amount of DB transactions and cuts writes in half. **Before:** 1. User (who is not signed in) makes request A from SP 2. Request A is stored in the DB and in the session by the controller (this requires a DB read) 3. User signs in + 2FA 4. ApplicationController looks up Request A from the session to redirect the user back to the entry point controller 5. The controller stores Request A again as a new entry in the DB, which we'll call Request B. The info is stored in the session again as well, resulting in another DB read. 6. User is redirected back to SP, and Request B is deleted from the DB, but Request A remains in the DB. Total DB transactions: 2 writes, 2 reads, 1 delete **After:** 1. User (who is not signed in) makes request A from SP 2. Request A is stored in the DB and in the session by the controller (this requires a DB read) 3. User signs in + 2FA 4. ApplicationController looks up Request A from the session to redirect the user back to the entry point controller 5. The controller compares the request stored in the session (DB read) with the one that was just received by the controller and sees that they are both from the same SP, and does create a new entry in the DB 6. User is redirected back to SP, and Request A is deleted from the DB. Total DB transactions: 1 write, 2 reads, 1 delete **How**: Instead of using the presence of the request_id in the session as the guard clause, compare the SP that made the last request with the one stored in the session. If they're the same, don't store a new entry in the DB, otherwise, delete the entry that matches the one in the session, and then create a new entry.

monfresh added the status - ready for review label Mar 24, 2017

monfresh force-pushed the mb-dont-store-sp-in-session branch from e907f8a to a75c49a Compare March 24, 2017 14:19

zachmargolis reviewed Mar 24, 2017

View reviewed changes

monfresh force-pushed the mb-dont-store-sp-in-session branch from 2f2231c to b077dda Compare March 24, 2017 17:24

zachmargolis approved these changes Mar 24, 2017

View reviewed changes

monfresh force-pushed the mb-dont-store-sp-in-session branch from b077dda to 6e1ff63 Compare March 24, 2017 18:41

jessieay reviewed Mar 24, 2017

View reviewed changes

monfresh force-pushed the mb-dont-store-sp-in-session branch from 0191a25 to c458433 Compare March 27, 2017 16:02

pkarman reviewed Mar 27, 2017

View reviewed changes

monfresh added 7 commits March 27, 2017 13:58

Address PR feedback

d1ba3b9

Make uuid index unique

06dec67

Refactor helper method

b8aea03

Delete sp_session before clicking confirmation

99066a9

Use perform_in_browser

5b99b87

Fix perform_in_browser for openidconnect

cc60390

monfresh force-pushed the mb-dont-store-sp-in-session branch from 4e61357 to cc60390 Compare March 27, 2017 18:15

jessieay reviewed Mar 27, 2017

View reviewed changes

Clarify sp_request method names

cdf1da9

jessieay reviewed Mar 27, 2017

View reviewed changes

jessieay approved these changes Mar 27, 2017

View reviewed changes

Update Devise comment

03a4683

monfresh merged commit a79af7e into master Mar 27, 2017

monfresh deleted the mb-dont-store-sp-in-session branch March 27, 2017 21:17

monfresh mentioned this pull request Mar 30, 2017

Set SameSite=Lax for cookies #1300

Merged

jessieay mentioned this pull request Mar 31, 2017

Revert #1288 for RC branch #1302

Merged

monfresh mentioned this pull request Sep 27, 2018

LG-667 Don't create stray ServiceProvideRequests #2563

Merged

12 tasks

		@@ -0,0 +1,5 @@
		class ServiceProviderRequest < ActiveRecord::Base
		def self.find_by(*args)

		@@ -99,16 +99,21 @@

		context 'user is not signed in' do
		it 'redirects to login' do


		enter_2fa_code

		# expect(page).to have_css('img[src*=sp-logos]')


		return prompt_to_set_up_2fa unless current_user.two_factor_enabled?

		prompt_to_enter_otp

Allow confirming email in a different browser #1265

Allow confirming email in a different browser #1265

Conversation

monfresh commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfresh commented Mar 24, 2017

zachmargolis left a comment

Choose a reason for hiding this comment

jessieay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessieay Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfresh Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessieay Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessieay commented Mar 24, 2017

monfresh commented Mar 24, 2017

monfresh commented Mar 24, 2017

monfresh commented Mar 27, 2017

Choose a reason for hiding this comment

monfresh commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessieay Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessieay Mar 24, 2017 •

edited

Loading

monfresh Mar 27, 2017 •

edited

Loading

jessieay Mar 24, 2017 •

edited

Loading

jessieay Mar 27, 2017 •

edited

Loading