Skip to content

Clicking site logo during 2fa signs user out #897

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
monfresh merged 1 commit into from
Jan 3, 2017
Merged

Clicking site logo during 2fa signs user out #897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions app/controllers/users/sessions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ class SessionsController < Devise::SessionsController
include ::ActionView::Helpers::DateHelper

skip_before_action :session_expires_at, only: [:active]
skip_before_action :require_no_authentication, only: [:new]
before_action :confirm_two_factor_authenticated, only: [:update]
before_action :check_user_needs_redirect, only: [:new]

def new
analytics.track_event(Analytics::SIGN_IN_PAGE_VISIT)
Expand Down Expand Up @@ -41,6 +43,14 @@ def timeout

private

def check_user_needs_redirect
if user_fully_authenticated?
redirect_to after_sign_in_path_for(current_user)
elsif current_user
sign_out
end
end

def now
@_now ||= Time.zone.now
end
Expand Down
37 changes: 33 additions & 4 deletions spec/controllers/users/sessions_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,12 +271,41 @@
end

describe '#new' do
it 'tracks page visit' do
stub_analytics
context 'with fully authenticated user' do
it 'redirects to the profile page' do
stub_sign_in
subject.session[:logged_in] = true
get :new

expect(response).to redirect_to profile_path
expect(subject.session[:logged_in]).to be true
end
end

expect(@analytics).to receive(:track_event).with(Analytics::SIGN_IN_PAGE_VISIT)
context 'with current user' do
it 'logs the user out' do
stub_sign_in_before_2fa
subject.session[:logged_in] = true
get :new

get :new
expect(request.path).to eq root_path
expect(subject.session[:logged_in]).to be_nil
end
end

context 'with a new user' do
it 'renders the new template' do
get :new
expect(response).to render_template(:new)
end

it 'tracks page visit' do
stub_analytics

expect(@analytics).to receive(:track_event).with(Analytics::SIGN_IN_PAGE_VISIT)

get :new
end
end
end
end
9 changes: 9 additions & 0 deletions spec/features/two_factor_authentication/sign_in_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,4 +235,13 @@
expect(current_path).to eq profile_path
end
end

describe 'clicking the logo image during 2fa process' do
it 'returns them to the home page' do
user = build_stubbed(:user, :signed_up)
sign_in_user(user)
find("img[alt='login.gov']").click
expect(current_path).to eq root_path
Copy link
Contributor

@jessieay jessieay Dec 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@el-mapache el-mapache Dec 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@el-mapache el-mapache Dec 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@monfresh monfresh Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@monfresh monfresh Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@monfresh monfresh Dec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. A partially signed in user can access the login page
  2. 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.

Copy link
Contributor Author

@el-mapache el-mapache Dec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@monfresh monfresh Dec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@el-mapache el-mapache Dec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@monfresh monfresh Dec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
Copy link
Contributor

@monfresh monfresh Dec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@el-mapache el-mapache Dec 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
end
1 change: 1 addition & 0 deletions spec/support/controller_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def stub_sign_in(user = build(:user, password: VALID_PASSWORD))
allow(controller).to receive(:user_session).and_return(authn_at: Time.zone.now)
allow(controller).to receive(:current_user).and_return(user)
allow(controller).to receive(:confirm_two_factor_authenticated).and_return(true)
allow(controller).to receive(:user_fully_authenticated?).and_return(true)
user
end

Expand Down