Clicking site logo during 2fa signs user out by el-mapache · Pull Request #897 · 18F/identity-idp

el-mapache · 2016-12-22T22:25:36Z

Why: Currently, clicking the logo during the 2fa process redirect
the user back to the same page, a behavior that confusing. This doesn't
change the behavior of clicking the logo once the user has authenticated
fully

jessieay

Couple of questions via comments. Also, is there a GH issue for this? Want to make sure I understand the context but don't see anything linked...

jessieay · 2016-12-22T22:39:42Z

spec/features/two_factor_authentication/sign_in_spec.rb

+      user = build_stubbed(:user, :signed_up)
+      sign_in_user(user)
+      find("img[alt='login.gov']").click
+      expect(current_path).to eq root_path


Perhaps here we should check for something else that is more directly related to being logged out? (I assume that logged in users can visit root_path ...)

When a logged in user visits the root_path, they are redirected to the profile page. I don't think the user ever lands on the / route.

Having said that, I don't know if the user goes to the home page under the hood before getting redirected. I believe a 302 prevents the initial requested page from being visited, but I could definitely be mistaken about that.

The way to tell is by removing the new SessionsController code and running the test. It should fail. You always want to start with a failing test to make sure your test is written properly. Once your test fails, then you can add the code to make sure the test passes.

However, to Jessie's point, if you also wanted to make sure that the user was signed out, I would use a controller spec because you can't really test that in a feature spec, afaik. Even if all we wanted to test was that partially signed in users could access the root url, I would still prefer a controller spec since it's faster than a feature spec.

The problem with only testing that the current path is the root path is that it is not a reliable way to determine whether the user has been signed out. For example, we could have code that redirects a signed-in user to the profile page, then signs them out via sign_out. In that case, the current path will still be the profile page, but the user will be signed out.

Or, we might want to allow partially signed in users to access the root url without signing them out, in which case we also want to test that they are not signed out.

The way I understood the feature as captured in the last few comments of issue 1202 was that a user would be able to access the root URL while only partially signed in, but that they wouldn't necessarily also be signed out. In other words, there are 2 distinct things that can happen:

A partially signed in user can access the login page

Optionally, if they visit the login page while partially signed in, we can also sign them out, or we can leave them partially signed in. I'm not sure what the best UX is.

What I said about GitHub not showing the home page was not very clear. I should have specified that it was for fully authenticated users, which isn't relevant to this PR.

Got it, thanks for clarifying! Leaving them partially signed in doesn't appear to produce any side effects, although it does cause the call to the active endpoint to fail, meaning they won't ever be automatically signed out once the session expires. Looking into that now.

Good catch. If it's not an easy fix, perhaps we should sign the user out if that's easier?

spoke too soon, there is a side effect (which is probably a bug we want to address)

If you enter your credentials, then click the logo to return to the home page, you can enter any credentials back into the form and continue the 2fa process for the original user.

In that case, I would vote for signing the user out.

jessieay · 2016-12-22T22:41:02Z

app/controllers/users/sessions_controller.rb


+    protected
+
+    def require_no_authentication


maybe instead of calling super we should rename this and add it as a before_action for new ? I think that might be more readable. I am also a tad confused about why we are adding this and skipping it for certain actions. Is it called by default in Devise::SessionsController, perhaps?

It does get called by default in the Devise::SessionsController. I think we need to override this method because the after_sign_in_path_for method that we've overridden in the ApplicationController tries to redirect the user to /profile, which in turn redirects the user back to whatever 2fa screen they are currently on.

Not sure if that actually answers your question... 😬

The way I would do this is by skipping the before_action only for the :new action, since that's the only one we want to override, and then add our behavior inside the new method:

skip_before_action :require_no_authentication, only: [:new] def new analytics.track_event(Analytics::SIGN_IN_PAGE_VISIT) if user_fully_authenticated? redirect_to after_sign_in_path_for(current_user) return else super end end

I'm not sure that we need to sign the user out if they're not fully authenticated. They should be able to go back to the previous screen without having to sign back in. GitHub doesn't sign the user out, as an example.

Sorry, that analytics call should only be in the else statement:

def new if user_fully_authenticated? redirect_to after_sign_in_path_for(current_user) return else analytics.track_event(Analytics::SIGN_IN_PAGE_VISIT) super end end

monfresh · 2016-12-28T23:26:09Z

app/controllers/users/sessions_controller.rb

Looks like you added this unless statement by mistake. We're already checking if the user is fully authenticated in the if statement. Removing the unless statement will allow the tests to pass.

Ah the mistake was not deleting the if statement. Code Climate complains without the unless!

monfresh · 2016-12-28T23:42:03Z

spec/features/two_factor_authentication/sign_in_spec.rb

What do you think about converting this to a controller spec, so that we can easily check if the user is still signed in or not, depending on whether or not we want to sign them out? In a controller spec, you can do something like: expect(subject.current_user).to_not be_present.

What I have in mind are 2 new contexts in the describe '#new' section at the bottom of /spec/controllers/users/sessions_controller_spec.rb: one where the user is not fully authenticated (stub_sign_in_before_2fa), and one where the user is fully authenticated (stub_sign_in). In the former case, we would expect(response).to render_template(:new), and in the latter, expect(response).to redirect_to profile_path.

To make this work, we have to add allow(controller).to receive(:user_fully_authenticated?).and_return(true) on line 36 of spec/support/controller_helper.rb.

Definitely in favor of having the controller specs! I do think this still has value as an acceptance test.

monfresh · 2016-12-29T22:43:49Z

spec/controllers/users/sessions_controller_spec.rb

Would it be worth it to also make sure that the user does not get signed out? For example, if you add sign_out after redirect_to after_sign_in_path_for(current_user) in the Sessions Controller, this test will still pass.

Yep, good point!

monfresh

Looks good. Feel free to squash and merge.

**Why**: Currently, clicking the logo during the 2fa process redirect the user back to the same page, a behavior that confusing. This doesn't change the behavior of clicking the logo once the user has authenticated fully Add better test coverage for logo sign out behavior **Why**: Previous test wasnt comprehensive enough to cover each of the different redirect/session state scenarios

**Why**: Currently, clicking the logo during the 2fa process redirects the user back to the same page, a behavior that's confusing. **How**: If the user has not yet completed 2FA, clicking the logo will sign them out and take them to the sign in page. This doesn't change the behavior of clicking the logo once the user has fully authenticated.

el-mapache added the status - ready for review label Dec 22, 2016

el-mapache self-assigned this Dec 22, 2016

jessieay reviewed Dec 22, 2016

View reviewed changes

el-mapache force-pushed the ab-logo-cancel-2fa branch 2 times, most recently from 7775ed3 to da7d259 Compare December 27, 2016 18:55

monfresh reviewed Dec 28, 2016

View reviewed changes

el-mapache force-pushed the ab-logo-cancel-2fa branch 2 times, most recently from 8a980d8 to 69c01ee Compare December 29, 2016 18:18

monfresh reviewed Dec 29, 2016

View reviewed changes

el-mapache force-pushed the ab-logo-cancel-2fa branch from 69c01ee to 80f0329 Compare December 30, 2016 13:37

monfresh approved these changes Dec 30, 2016

View reviewed changes

el-mapache force-pushed the ab-logo-cancel-2fa branch from 80f0329 to 21b40dc Compare December 30, 2016 18:19

monfresh merged commit acc66b2 into master Jan 3, 2017

monfresh deleted the ab-logo-cancel-2fa branch January 3, 2017 17:10

This was referenced Jul 24, 2023

Log when user is logged out after partially authenticating #8838

Merged

Do not log user out when accessing root url while partially authenticated #8895

Merged

Conversation

el-mapache commented Dec 22, 2016

Uh oh!

jessieay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

el-mapache Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

el-mapache Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

el-mapache Dec 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

el-mapache Dec 22, 2016 •

edited

Loading

el-mapache Dec 22, 2016 •

edited

Loading

el-mapache Dec 30, 2016 •

edited

Loading