This repository was archived by the owner on Sep 30, 2024. It is now read-only.
Allow users to sign out when there is only one oauth provider configured#44803
Merged
Conversation
… the sso login or not
Contributor
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff ca2c501...830a72f.
|
mrnugget
suggested changes
Nov 25, 2022
Contributor
Author
|
@mrnugget , thanks for the review! I've addressed your suggestions, so it's ready for another look! |
mrnugget
approved these changes
Nov 28, 2022
pjlast
reviewed
Nov 28, 2022
Comment on lines
-85
to
-94
| if got, want := resp.Header.Get("Location"), "/.auth/github/login?"; !strings.Contains(got, want) { | ||
| t.Errorf("got redirect URL %v, want contains %v", got, want) | ||
| } | ||
| redirectURL, err := url.Parse(resp.Header.Get("Location")) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if got, want := redirectURL.Query().Get("redirect"), "/page"; got != want { | ||
| t.Errorf("got return-to URL %v, want %v", got, want) | ||
| } |
Contributor
There was a problem hiding this comment.
Why did these redirect checks get removed?
Contributor
Author
There was a problem hiding this comment.
The redirects were there to cover the previous flow where:
- user clicks signout
- user is redirected to the sso signin flow (a 302 was expected)
now, the behavior is different:
- user clicks signout
- there's a get request to the SG sign in page (a 200 is expected) instead of a redirect to the sso flow as before
Contributor
There was a problem hiding this comment.
But that's not the flow that this test tests. Reading the test it says:
- User is not signed in
- User tries to navigate to "http://example.com/page" sub-page
- User gets redirected to GitHub OAuth sign-in flow
This is still valid and desired behaviour. Similar goes for the test above it.
Contributor
Author
There was a problem hiding this comment.
yes, you are correct!! thank you!!!
Contributor
Author
|
I was waiting until the build finish to ping you, @pjlast , but you were faster :D |
unknwon
reviewed
Dec 2, 2022
|
|
||
| func RemoveSignOutCookieIfSet(r *http.Request, w http.ResponseWriter) { | ||
| if HasSignOutCookie(r) { | ||
| http.SetCookie(w, &http.Cookie{Name: SignoutCookie, Value: "", MaxAge: -1}) |
Contributor
There was a problem hiding this comment.
FYI: It is always safe to clear a cookie regardless of its existence, browser handles it. The opposite version of "upsert".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue: #31192
This PR fixes a signout bug that happens when there's only one auth provider configured.
Bug:
With this PR:
Test plan
Before vs after demos:
Before: Signout flow + Session Expiration for 1 auth provider
https://user-images.githubusercontent.com/1552863/203876630-55ed1efd-3cf9-40c7-972a-91978221bff1.mp4
Now: Signout flow + Session Expiration for 1 auth provider
https://user-images.githubusercontent.com/1552863/203876673-912df8de-8424-4935-8239-03aac77dbde7.mp4