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

Revert "Revert "Logout endpoint should use POST not GET"" #1861

Merged
merged 8 commits into from Mar 19, 2019

Conversation

2 participants
@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj commented Mar 13, 2019

Description

Original PR #1830
Reverts #1858

Two things fixed in this PR:

  1. "Sign Out" link now acts like a real link for mouse rollover
  2. Login.gov redirect is moved from the server to the client

More detail

The redirect was a bit complicated because of the use of fetch(). Unfortunately, while fetch() will follow redirects from the server the redirect will use pre-flight checks against CORS redirects that can't be turned off. This is despite the fact that a 303 should allow a POST request to be redirected to a GET request. If fetch() allowed you to intercept the redirect to manually walk the redirect chain then we would do that, but the redirect: 'manual' doesn't do this, it instead returns an opaque response with no actionable information.

The only reasonable thing to do then is to process the post request with a 200 response and pass back the logout URL for Login.gov. The client then has to do the redirect itself by using window.location.href which acts like the GET request we'd want in the first place. This does perform as expected and is probably the simplest way forward here. The consequence of not handling this URL is that the user remains logged into Login.gov and on a subsequent visit to /auth/login would re-authenticate and get a new session token.

The alternative is to not let the client have any say and use a <form> instead of an <a onClick=...>, but that opens up a world where we'd need the form to be styled as though it was a link to not change the behavior requirements from design. However, using a form would allow the server to do all of the work of logout AND redirect.

Question for reviewers: Is this introducing a security flaw? Or are we confident that letting the client log itself out is sufficient for our application?

Testing

This needs to be tested in three ways:

  • Dev environment devlocal login/logout works
  • Dev environment login.gov login/logout works
  • Experimental login.gov login/logout works

@chrisgilmerproj chrisgilmerproj self-assigned this Mar 13, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 13, 2019

Codecov Report

Merging #1861 into master will decrease coverage by 0.24%.
The diff coverage is 29.03%.

@@            Coverage Diff             @@
##           master    #1861      +/-   ##
==========================================
- Coverage   49.58%   49.33%   -0.25%     
==========================================
  Files         430      430              
  Lines       18499    18376     -123     
  Branches     1632     1632              
==========================================
- Hits         9172     9066     -106     
+ Misses       8523     8521       -2     
+ Partials      804      789      -15

chrisgilmerproj added some commits Mar 13, 2019

Instead of the server handling redirect pass that responsibility to t…
…he client by writing out the redirect uri and having it change the window location.
try {
// Successful logout should return a redirect url
let resp = await Swagger.http(req);
window.location.href = resp.text;

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 13, 2019

Author Contributor

After reading this article I think this is the best choice: https://appendto.com/2016/04/javascript-redirect-how-to-redirect-a-web-page-with-javascript/

This comment has been minimized.

@donaldthai

donaldthai Mar 18, 2019

Contributor

I have an alternative solution to client side redirection. If we can follow the redirection via form post, then we can just set the request header 'Content-Type': 'application/x-www-form-urlencoded;charset=UTF-8' to mimic a form post without the html form tag!

More info here:

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 18, 2019

Author Contributor

Ok, this seemed very promising so I tried it. I run into the same CORS problem as I did before, so perhaps I was wrong that a POST from a form would make this work. Here's the diff:

diff --git a/pkg/auth/authentication/auth.go b/pkg/auth/authentication/auth.go
index b5dbeb575..ab7c4762a 100644
--- a/pkg/auth/authentication/auth.go
+++ b/pkg/auth/authentication/auth.go
@@ -121,10 +121,10 @@ func (h LogoutHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
                        session.IDToken = ""
                        session.UserID = uuid.Nil
                        auth.WriteSessionCookie(w, session, h.clientAuthSecretKey, h.noSessionTimeout, h.logger, h.useSecureCookie)
-                       fmt.Fprint(w, logoutURL)
+                       http.Redirect(w, r, logoutURL, http.StatusSeeOther)
                } else {
                        // Can't log out of login.gov without a token, redirect and let them re-auth
-                       fmt.Fprint(w, redirectURL)
+                       http.Redirect(w, r, redirectURL, http.StatusSeeOther)
                }
        }
 }
diff --git a/src/shared/User/api.js b/src/shared/User/api.js
index 83c1d0957..9f469b7d0 100644
--- a/src/shared/User/api.js
+++ b/src/shared/User/api.js
@@ -11,18 +11,22 @@ export async function GetLoggedInUser() {

 export async function LogoutUser() {
   const logoutEndpoint = '/auth/logout';
+  const headers = {
+    'Content-Type': 'application/x-www-form-urlencoded;charset=UTF-8',
+  };
   const req = {
     url: logoutEndpoint,
     method: 'POST',
     credentials: 'same-origin', // Passes through CSRF cookies
+    headers,
     requestInterceptor,
   };
   try {
     // Successful logout should return a redirect url
     let resp = await Swagger.http(req);
-    window.location.href = resp.text;
+    // window.location.href = resp.text;
   } catch (err) {
     // Failure to logout should return user to homepage
-    window.location.href = '/';
+    // window.location.href = '/';
   }
 }

This comment has been minimized.

@donaldthai

donaldthai Mar 19, 2019

Contributor

That's too bad. It was worth a shot. I wonder if it's because of the custom headers (eg csrf token) we use that is triggering the CORS pre-flight?

} else {
// Can't log out of login.gov without a token, redirect and let them re-auth
http.Redirect(w, r, redirectURL, http.StatusTemporaryRedirect)
fmt.Fprint(w, redirectURL)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 14, 2019

Author Contributor

This is where we pass back the Logout URL in the response.

Should this be formatted as a JSON object?

This comment has been minimized.

@donaldthai

donaldthai Mar 19, 2019

Contributor

Seems fine to me like this.

Revert "Deploy to experimental"
This reverts commit 93cc5e7.

@chrisgilmerproj chrisgilmerproj marked this pull request as ready for review Mar 14, 2019

@donaldthai
Copy link
Contributor

donaldthai left a comment

LGTM :shipit:

Too bad doing a form post didn't bypass the CORS pre-flight issue.

@chrisgilmerproj chrisgilmerproj merged commit 827d18a into master Mar 19, 2019

17 of 19 checks passed

codecov/patch 29.03% of diff hit (target 49.58%)
Details
codecov/project 49.33% (-0.25%) compared to 379b766
Details
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: client_test_coverage 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

@chrisgilmerproj chrisgilmerproj deleted the revert-1858-revert-1830-cg_164014224_logout_should_use_post branch Mar 19, 2019

@chrisgilmerproj chrisgilmerproj referenced this pull request Mar 22, 2019

Open

WIP - Try out load testing framework locust.io #1597

0 of 2 tasks complete
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.