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

Logout endpoint should use POST not GET #1830

Merged
merged 34 commits into from Mar 12, 2019

Conversation

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj commented Mar 6, 2019

Description

We should not use GET for logout. This changes the endpoints, uses forms, and changes the redirect from a 307 to a 303.

Reviewer Notes

In this PR I modify the middleware to only create one public CSRF Token in the middleware. The reason for this is that we only need it to change on session updates (like user login/logout) and for debugging you can't track multiple public tokens (even though they ought to all unmask as the same value). It's a little less logic to have to parse when looking into CSRF issues.

Similarly, I no longer call csrf.Token(r) in all the places. Instead I rely on the middleware to properly set the cookie and then we only grab it from the cookie. This changes how the devlocal login page works but has no real effect, except again, to eliminate confusion about what tokens are being generated for debugging.

Setup

Login and then logout!

Code Review Verification Steps

  • There are no aXe warnings for UI.
  • This works in IE
  • This works in MSEdge
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

A number of past PRs that I looked at while prepping this PR:

@chrisgilmerproj chrisgilmerproj self-assigned this Mar 6, 2019

@stangah

This comment has been minimized.

Copy link
Contributor

stangah commented Mar 6, 2019

Code looks fine, and it tested well for me on IE11

I think the new logout button needs some formatting though:
New: image

Old: image

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #1830 into master will increase coverage by 0.14%.
The diff coverage is 31.03%.

@@            Coverage Diff             @@
##           master    #1830      +/-   ##
==========================================
+ Coverage   49.38%   49.52%   +0.14%     
==========================================
  Files         427      428       +1     
  Lines       18341    18466     +125     
  Branches     1636     1636              
==========================================
+ Hits         9058     9146      +88     
- Misses       8493     8518      +25     
- Partials      790      802      +12
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

chrisgilmerproj commented Mar 7, 2019

I can't seem to get this working in Internet Explorer. The best I can get is that CSRF middleware is blocking me, but I can't figure why. The cookie is certainly getting sent along with fetch() and its being rejected. I'll keep digging here.

console.log('============');
console.log(token);
console.log(document.cookie);
console.log('============');

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 7, 2019

Author Contributor

I need to remove this, but only after I figure out if the cookie is incorrect.

@@ -85,6 +85,7 @@ func LogRequestMiddleware(gitBranch string, gitCommit string) func(inner http.Ha
zap.String("x-forwarded-for", r.Header.Get("x-forwarded-for")),
zap.String("x-forwarded-host", r.Header.Get("x-forwarded-host")),
zap.String("x-forwarded-proto", r.Header.Get("x-forwarded-proto")),
zap.String("x-csrf-token", r.Header.Get("x-csrf-token")),

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 8, 2019

Author Contributor

Make sure to remove this before merging. It's for testing that the token is being sent properly from IE (which it is!).

chrisgilmerproj added some commits Mar 8, 2019

Set CSRF token only once, on login. Prevents CSRF token from changing…
… via an async operation while sending another request

@chrisgilmerproj chrisgilmerproj requested a review from donaldthai Mar 8, 2019

@rdhariwal

This comment has been minimized.

Copy link
Contributor

rdhariwal commented Mar 8, 2019

I can't seem to get this working in Internet Explorer. The best I can get is that CSRF middleware is blocking me, but I can't figure why. The cookie is certainly getting sent along with fetch() and its being rejected. I'll keep digging here.

I can debug go in goland if you want to look at the guts of the request. Happy to pair on it or stand in as 🦆 to bounce ideas

@chrisgilmerproj chrisgilmerproj requested a review from tinyels Mar 8, 2019

}
}

// WriteResponse updates the session cookie before writing out the details of the response
func (cur *CookieUpdateResponder) WriteResponse(rw http.ResponseWriter, p runtime.Producer) {
auth.WriteSessionCookie(rw, cur.session, cur.cookieSecret, cur.noSessionTimeout, cur.logger)
auth.WriteMaskedCSRFCookie(rw, csrf.Token(cur.request), cur.noSessionTimeout, cur.logger)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 8, 2019

Author Contributor

I think what this does is ensure even API requests get the cookie set. I'm not clear on if it's helping.

})
.catch(err => {
console.log(err);
});

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 8, 2019

Author Contributor

What should I do with error responses?

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 8, 2019

Author Contributor

Maybe send a signal that the browser is idle.

chrisgilmerproj added some commits Mar 8, 2019

Show resolved Hide resolved src/shared/User/api.js
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

chrisgilmerproj commented Mar 11, 2019

Latest change works in MSEdge! Thanks @donaldthai .

Now to get the integration tests working.

{{.Email}}
({{if .DpsUserID}}dps{{else if .TspUserID}}tsp{{else if .OfficeUserID}}office{{else}}milmove{{end}})
<button name="id" value="{{.ID}}" data-hook="existing-user-login">Login</button>
<input type="hidden" name="id" value="{{.ID}}" />
<button type="submit" value="{{.ID}}" data-hook="existing-user-login">Login</button>

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 11, 2019

Author Contributor

This fixes how IE11 does login, which previously wasn't working. Thanks to @donaldthai for the fix!

@rdhariwal
Copy link
Contributor

rdhariwal left a comment

:shipit:

@donaldthai
Copy link
Contributor

donaldthai left a comment

LGTM :shipit:

@chrisgilmerproj chrisgilmerproj merged commit 4e61612 into master Mar 12, 2019

18 of 19 checks passed

codecov/patch 31.03% of diff hit (target 49.38%)
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
codecov/project 49.52% (+0.14%) compared to 3528d55
Details

@chrisgilmerproj chrisgilmerproj deleted the cg_164014224_logout_should_use_post branch Mar 12, 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.