Allow signing in to SP after session timeout by monfresh · Pull Request #1275 · 18F/identity-idp

monfresh · 2017-03-28T03:31:09Z

Why: We rely on the original SP request URL to be available in the
session in order to complete the authentication flow and redirect the
user back to the SP. If a user comes from an SP, but sits on the sign
in page long enough for the session to time out, the request URL will
be deleted from the session, and after they sign back in, they will end
up on the profile page instead of going back to the SP.

How:

Take advantage of the request_id param to restore the SP info
in the session after the user signs in, the same way we do when a user
confirms their email in a different browser
Create a signed_in_path method that only redirects to the profile
path if the user is fully authenticated. Otherwise, it redirects to
user_two_factor_authentication_path. Use this new method in
after_sign_in_path_for. Why? Because any controller that includes a
before_action that gets executed will set the stored location for the
user to that controller. This means that if we redirect to the
profile_path while there is no stored location (which will be the case
if the session times out) and the user isn't yet fully authenticated, it
will call the confirm_two_factor_authenticated before_action, and
the stored location will now be the profile page. So, after the user
enters their OTP, they will be sent to the profile page instead of back
to SamlIdpController#auth to complete the auth flow back to the SP.

pkarman

lgtm % comments

pkarman · 2017-03-28T14:42:40Z

.reek

hallelujah! 🎉

woooooo! Care to comment on why this now?

Because I was getting a bunch of Reek errors locally, and instead of adding a bunch of methods to the exclusion list, I figured we'd turn it off since we'll probably keep getting more of these in the future.

pkarman · 2017-03-28T14:45:49Z

app/controllers/sign_up/recovery_codes_controller.rb

In isolation this change looks odd to me. Isn't the logic more subtle than simply always going to profile_path?

I'm pretty sure that in this scenario, after_sign_in_path will always be profile_path, so might as well go there directly but I will revert this since it's not really related to this PR.

pkarman · 2017-03-28T14:47:15Z

app/services/store_sp_metadata_in_session.rb

having the name of a class be a verb seems like an odd pattern to introduce.

This is not the first introduction of this pattern. See CreateVerifiedAccountEvent, RequestPasswordReset, UpdateUser, and UpdateUserPassword.

Micropurchase does this as well.

I think it's fine to have some classes be verbs and others nouns, depending on the best way to describe them.

I don't mind verb names in service class methods. I do feel like some of the service classes in this app could perhaps be classified as something else...eg RandomPhrase...but that's for a separate PR :)

**Why**: We rely on the original SP request URL to be available in the session in order to complete the authentication flow and redirect the user back to the SP. If a user comes from an SP, but sits on the sign in page long enough for the session to time out, the request URL will be deleted from the session, and after they sign back in, they will end up on the profile page instead of going back to the SP. **How**: - Take advantage of the `request_id` param to restore the SP info in the session after the user signs in, the same way we do when a user confirms their email in a different browser - Create a `signed_in_path` method that only redirects to the profile path if the user is fully authenticated. Otherwise, it redirects to user_two_factor_authentication_path. Use this new method in `after_sign_in_path_for`. Why? Because any controller that includes a `before_action` that gets executed will set the stored location for the user to that controller. This means that if we redirect to the profile_path while there is no stored location (which will be the case if the session times out) and the user isn't yet fully authenticated, it will call the `confirm_two_factor_authenticated` `before_action`, and the stored location will now be the profile page. So, after the user enters their OTP, they will be sent to the profile page instead of back to SamlIdpController#auth to complete the auth flow back to the SP.

monfresh · 2017-03-28T15:44:58Z

These Travis failures are not due to this PR. I see the same failure in Jessie's earlier branch, which doesn't seem related to this failure either: https://travis-ci.org/18F/identity-idp/builds/215722877

monfresh · 2017-03-28T15:46:52Z

I'll restart Travis and open an issue to fix that flickering spec.

monfresh · 2017-03-28T15:53:17Z

This failing test was introduced in #1262.

jessieay

Looks good! Had some smaller questions / comments but overall 🥇

jessieay · 2017-03-28T16:02:09Z

.reek

woooooo! Care to comment on why this now?

jessieay · 2017-03-28T16:02:43Z

app/controllers/application_controller.rb

    @_decorated_session ||= DecoratedSession.new(sp: current_sp, view_context: view_context).call
  end

+  def signed_in_path


put this below after_sign_in_path so that method dependencies point downward?

It needs to be a public method so it can be tested. Alternatively, we can make it a private method and only rely on integration tests.

oooh...hm yeah I tend not to make methods public just for testing. Seems problematic.

I'm fine with removing the application controller spec. Since this method is called in pretty much every feature spec, I think it's got good coverage. Is that what you're proposing?

Done here: 15b624f

Thanks!

jessieay · 2017-03-28T16:03:13Z

app/controllers/sign_up/completions_controller.rb

        service_provider_attributes
      )
-      redirect_to after_sign_in_path_for(current_user)
+      redirect_to sp_session[:request_url]


would someone ever get to this point without being referred by an SP?

No. The show action requires session[:sp] to be present.

jessieay · 2017-03-28T16:03:51Z

app/controllers/users/sessions_controller.rb


+    def process_locked_out_user
+      render 'two_factor_authentication/shared/max_login_attempts_reached'
+      sign_out


do you think we should sign out then render? Not sure if it matters, just wondering if the page might render different content based on signed in / out status.

Not sure. I only refactored this to please Code Climate. I didn't change the behavior.

However, I believe this order is like that for a reason. I recall this fixed a bug at one time.

jessieay · 2017-03-28T16:04:33Z

app/controllers/users/sessions_controller.rb

-      return false unless current_user.present?
-
+      return false unless current_user
      !user_locked_out?(user)


based on the name of this method, I'd expect this to read current_user && !user_locked_out?(user)

This is not new to this PR. I didn't want to make any changes that were not related. I can add the .present? back if that makes things clearer.

that's fine, we can keep as-is

jessieay · 2017-03-28T16:05:16Z

app/controllers/users/sessions_controller.rb

@@ -91,13 +93,21 @@ def cache_active_profile
    end

    def user_signed_in_and_not_locked_out?(user)


do we need the user to get passed in here? We should have access to current_user within this method, right?

I didn't change this behavior. This method existed before this PR. Can we revisit later if necessary?

sure thing!

jessieay · 2017-03-28T16:06:42Z

app/services/store_sp_metadata_in_session.rb

I don't mind verb names in service class methods. I do feel like some of the service classes in this app could perhaps be classified as something else...eg RandomPhrase...but that's for a separate PR :)

jessieay · 2017-03-28T16:11:52Z

app/views/devise/sessions/new.html.slim

                  html: { autocomplete: 'off', role: 'form' }) do |f|
  = f.input :email, required: true, input_html: { class: 'mb4' }
  = f.input :password, required: true
+  = f.input :request_id, as: :hidden, input_html: { value: params[:request_id] }


params[:_request_id] ?

The underscore is only present in the confirmation email.

jessieay · 2017-03-28T16:13:50Z

spec/features/saml/loa1_sso_spec.rb

      expect(current_path).to eq sign_up_completed_path
    end
+
+    it 'after session timeout, signing in takes user back to SP' do


jessieay

😃

monfresh force-pushed the mb-preserve-sp-request-after-timeout branch 2 times, most recently from 27ffd2e to b157186 Compare March 28, 2017 04:08

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

monfresh requested review from jessieay and pkarman March 28, 2017 14:21

monfresh force-pushed the mb-preserve-sp-request-after-timeout branch 2 times, most recently from 4eef502 to e57cc78 Compare March 28, 2017 14:32

pkarman approved these changes Mar 28, 2017

View reviewed changes

monfresh force-pushed the mb-preserve-sp-request-after-timeout branch from e57cc78 to f19d61f Compare March 28, 2017 15:13

jessieay reviewed Mar 28, 2017

View reviewed changes

Remove controller spec for signed_in_path

15b624f

jessieay approved these changes Mar 28, 2017

View reviewed changes

monfresh merged commit 0fa95ab into master Mar 28, 2017

monfresh deleted the mb-preserve-sp-request-after-timeout branch March 28, 2017 17:45

aduth mentioned this pull request Apr 5, 2023

Remove request_id parameters from Sign In view links #8137

Merged

		@@ -91,13 +93,21 @@ def cache_active_profile
		end

		def user_signed_in_and_not_locked_out?(user)

Conversation

monfresh commented Mar 28, 2017

Uh oh!

pkarman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh commented Mar 28, 2017

Uh oh!

monfresh commented Mar 28, 2017

Uh oh!

monfresh commented Mar 28, 2017

Uh oh!

jessieay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessieay left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees