Skip to content
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.

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

Redirect user to landing page if multiple sessions have different cookies #2307

Merged
merged 10 commits into from Jul 9, 2019

Conversation

@Ronolibert
Copy link
Contributor

commented Jun 20, 2019

Description

Fix issue where user has cookies out of sync in different tabs/sessions causing them to get an internal server error.

Reviewer Notes

  • I'm not sure if this is the correct way to fix this behavior and if this glosses over other problems. The goal was to make it so the user does not hit the Internal server error page and be stuck there. This change seems to redirect the user to their app of use in the loggedIn state

Setup

make server_run
make client_run
Giphy in screenshot section of bug being replicated

  1. Open 2 Sign In tabs
  2. Log in to the "older" of the Sign In tabs
    Expected Behavior: User should be logged in

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • The requirements listed in
    Querying the Database Safely
    have been satisfied.
  • Any new migrations/schema changes:
    • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
    • Have been communicated to #dp3-engineering
    • Secure migrations have been tested using scripts/run-prod-migrations
  • There are no aXe warnings for UI.
  • This works in Supported Browsers and their phone views (Chrome, Firefox, IE, Edge).
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • User facing changes have been reviewed by design.
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

Screenshots

Steps to reproduce bug
https://streamable.com/hf3jh

@chrisgilmerproj
Copy link
Contributor

left a comment

My worry here is that we ought to delete one of their cookies (preferably the expired one of the stale page if that's possible). If we redirect here without really taking any action then there's no benefit to the cookies we set.

@jim
Copy link
Contributor

left a comment

Let's add the error query param for users. I also agree with @chrisgilmerproj: let's clear the lg_state cookie so that the user starts over with a clean slate the next time.

What do we think about making a similar change up on line 251? If a user takes longer than the 30 minute window to complete the login process, they're going to hit that error and it seems like another case we could handle in the same way.

if hash != shaAsString(returnedState) {
h.logger.Error("State returned from Login.gov does not match state value stored in cookie",
zap.String("state", returnedState),
zap.String("cookie", hash),
zap.String("hash", shaAsString(returnedState)))
http.Error(w, http.StatusText(500), http.StatusInternalServerError)
http.Redirect(w, r, landingURL.String(), http.StatusTemporaryRedirect)

This comment has been minimized.

Copy link
@jim

jim Jun 21, 2019

Contributor

It looks like we can include an error param in the query string and have a helpful message appear on the login page. Let's do that to help folks understand what happened (or at least encourage them to try again).

@jim

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

We should also have a unit test around this behavior. I'm happy to pair on getting that going next week if that would be helpful!

@Ronolibert

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Let's add the error query param for users. I also agree with @chrisgilmerproj: let's clear the lg_state cookie so that the user starts over with a clean slate the next time.

What do we think about making a similar change up on line 251? If a user takes longer than the 30 minute window to complete the login process, they're going to hit that error and it seems like another case we could handle in the same way.

Regarding the 30min expiration, I tested it and login.gov goes back to the login view if you stay in the 2FA screen for longer than 15 min and they show an error respectively

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Regarding the 30min expiration, I tested it and login.gov goes back to the login view if you stay in the 2FA screen for longer than 15 min and they show an error respectively

Our session expiration is 15 minutes so it's probably ok to set the timeout of 15 minutes here as well.

@jim jim added the roci label Jun 27, 2019

@jim jim assigned jim and unassigned jim Jun 28, 2019

@codecov

This comment has been minimized.

Copy link

commented Jul 2, 2019

Codecov Report

Merging #2307 into master will increase coverage by 0.16%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2307      +/-   ##
==========================================
+ Coverage   59.73%   59.88%   +0.16%     
==========================================
  Files         260      260              
  Lines       14657    14665       +8     
==========================================
+ Hits         8754     8782      +28     
+ Misses       4888     4864      -24     
- Partials     1015     1019       +4

@donaldthai donaldthai requested review from jim and chrisgilmerproj Jul 2, 2019

@donaldthai donaldthai added the 🦇team label Jul 2, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

I'm not sure I'm satisfied with this just yet. Comments in the code.

@donaldthai donaldthai requested a review from tinyels Jul 3, 2019

@donaldthai donaldthai self-assigned this Jul 3, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

🚀 - Thanks for the hard work on this everyone!

@jim

jim approved these changes Jul 9, 2019

@donaldthai donaldthai merged commit b152d8e into master Jul 9, 2019

17 checks passed

ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details

@donaldthai donaldthai deleted the roc-#166780736-outdated-session-cookie branch Jul 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.