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

test(auth): ensure failed cookie auth attempt instructs user-agent to clear cookie #1336

Merged
merged 7 commits into from
Feb 14, 2023

Conversation

GeorgeMac
Copy link
Contributor

@GeorgeMac GeorgeMac commented Feb 14, 2023

Supports #1334

I believe there might exist a deeper experience issue with the UI's handling of entering an unauthenticated state.
However, as pointed out, the expire token self endpoint is an authenticated route.
This means we can only expire a valid (non-expired) authenticated request.

I believe this is correct and fair behaviour. Because the operation is a mutating and destructive action that should only be performed by an authenticated party.
That said though, I think it might be prudent to instruct the user agent to actually expire the cookie in all cases where a token is presented via a Cookie header, but that leads to an unauthenticated response.

I suspect this might not actually fix the whole end-to-end problem in #1334 but I think it is worth doing.

@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2023

Codecov Report

Merging #1336 (9695460) into main (582a518) will increase coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 9695460 differs from pull request most recent head 9c1023f. Consider uploading reports for the commit 9c1023f to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1336      +/-   ##
==========================================
+ Coverage   80.81%   80.88%   +0.07%     
==========================================
  Files          43       43              
  Lines        3336     3349      +13     
==========================================
+ Hits         2696     2709      +13     
  Misses        512      512              
  Partials      128      128              
Impacted Files Coverage Δ
internal/server/auth/http.go 88.57% <100.00%> (+6.75%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@GeorgeMac
Copy link
Contributor Author

GeorgeMac commented Feb 14, 2023

Example of the test failing earlier when I pushed it up:

PUT http://0.0.0.0:8080/auth/v1/self/expire
 ✔ Set-Cookie: flipt_client_token=.*Max-Age=0
 ✔ status 200

GET http://0.0.0.0:8080/auth/v1/self
 ✔ status 401

GET http://0.0.0.0:8080/auth/v1/self
 ✘ Set-Cookie: flipt_client_token=.*Max-Age=0 (actual: Set-Cookie: _gorilla_csrf=<redacted>; Path=/; Expires=Tue, 14 Feb 2023 22:24:50 GMT; Max-Age=43200; HttpOnly; Secure; SameSite=Lax)
 ✔ status 401

It ensures that an expired token cookie is cleared when presented to an endpoint other than the explicit expire one.
The only cookie being set before this change was the CSRF one.
Now it sets the appropriate token(s) with max age of <=0.

@GeorgeMac GeorgeMac marked this pull request as ready for review February 14, 2023 11:10
@GeorgeMac GeorgeMac requested a review from a team as a code owner February 14, 2023 11:10
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! we should prob also rebase this onto the release/1.18 branch for v1.18.2 today

@GeorgeMac GeorgeMac added the automerge Used by Kodiak bot to automerge PRs label Feb 14, 2023
@kodiakhq kodiakhq bot merged commit b6cef5c into main Feb 14, 2023
@kodiakhq kodiakhq bot deleted the gm/expired-token-clearing branch February 14, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Used by Kodiak bot to automerge PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants