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

Modify the cookie name to be specific to the hostname #1688

Merged
merged 18 commits into from Feb 14, 2019

Conversation

3 participants
@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj commented Feb 1, 2019

Description

This change changes the session cookie name to be distinct across all apps by adding a prefix based on the lower-cased application name that we put in the session. This should help prevent different apps attempting to decode and use the same session data.

In a separate PR I will experiment with explicitly setting the domain on the cookies as we create them but I felt it was too much for this PR.

Reviewer Notes

I don't really see the point of having the DetectorMiddleware be separate from the SessionCookieMiddleware after having done this change and I want some confirmation on that. Since they both use the new method ApplicationName if feels a lot like I'm doing double work that is unnecessary.

Setup

The best test for this is to log into the website with developer tools into each app so you can see the session cookie name. You'll now see these distinct cookie names:

  • my_session_token
  • office_session_token
  • tsp_session_token

Failure to log in probably means the javascript client hasn't reset and picked up the new cookie names.

Now try logging out, you should see only the cookie for your app deleted and not the others.

Also, I used the Application Name as the prefix instead of using the subdomain of the application (which is unreliable). I think that makes this pretty solid overall.

Code Review Verification Steps

  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@chrisgilmerproj chrisgilmerproj self-assigned this Feb 1, 2019

chrisgilmerproj added some commits Feb 1, 2019

Abstract out the code that creates the application name so it can be …
…used in multiple places. Do a similar check in the javascript to get the cookie name.

@chrisgilmerproj chrisgilmerproj changed the title WIP - Modify the cookie name to be specific to the hostname Modify the cookie name to be specific to the hostname Feb 1, 2019

@pjdufour-truss
Copy link
Contributor

pjdufour-truss left a comment

suggestions inline

Show resolved Hide resolved pkg/auth/app_detector.go Outdated
@pjdufour-truss

This comment has been minimized.

Copy link
Contributor

pjdufour-truss commented Feb 1, 2019

I like the idea of collapsing detector and session middleware into 1.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

chrisgilmerproj commented Feb 1, 2019

@pjdufour-truss - I'll merge the middlewares and see where that gets me.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

chrisgilmerproj commented Feb 2, 2019

I collapsed the middlewares in be88020.

@chrisgilmerproj chrisgilmerproj requested review from rdhariwal and reggieriser Feb 6, 2019

@chrisgilmerproj chrisgilmerproj requested a review from stangah Feb 7, 2019

@tinyels
Copy link
Contributor

tinyels left a comment

We are being pretty inconsistent about using "mil" in some places and "my" in others. Given that the app is not longer called mymove, I think we need a better internal name for the service member app. One that doesn't sound like it was written in Visual Basic ;)

Show resolved Hide resolved bin/find-invoices Outdated
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

chrisgilmerproj commented Feb 9, 2019

@tinyels and @pjdufour-truss - I think I'm complete with the updates here. Would love a final review.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

chrisgilmerproj commented Feb 13, 2019

@pjdufour-truss - Deployed to experimental. Verified it worked:

screen shot 2019-02-12 at 4 32 03 pm

I logged into https://my.experimental.move.mil and received session_token. I waited until the deploy was complete, refreshed my page, and was logged out. Logging back in gave me mil_session_token while leaving the other token (as seen in the picture).

The consequence of this deploy means logged-in users will have to re-authenticate with Login.gov.

@pjdufour-truss
Copy link
Contributor

pjdufour-truss left a comment

Looks good. Tested on experimental.

@chrisgilmerproj chrisgilmerproj merged commit 7c86d51 into master Feb 14, 2019

15 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
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_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test 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

@chrisgilmerproj chrisgilmerproj deleted the cg_163496246_change_session_cookie_name branch Feb 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment