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

feat: auth expire self / logout #1279

Merged
merged 9 commits into from
Jan 20, 2023
Merged

feat: auth expire self / logout #1279

merged 9 commits into from
Jan 20, 2023

Conversation

markphelps
Copy link
Collaborator

@markphelps markphelps commented Jan 19, 2023

Fixes: FLI-172

  • Adds new grpc method auth.ExpireAuthenticationSelf that takes optional expires_at param to update the current authentication expiration date, defaults to now if not set
  • Adds HTTP middleware wrapper to support clearing the flipt_client_token and flipt_client_state cookies to support logout functionality in the UI

I made it a PUT because that seemed the most appropriate, even though it isn't technically idempotent since continuos calling (without the optional expiresAt field) will result in expiresAt getting set to NOW() each time). Open to suggestion though

@markphelps markphelps marked this pull request as ready for review January 19, 2023 20:43
@markphelps markphelps requested a review from a team as a code owner January 19, 2023 20:43
@markphelps markphelps changed the title chore: auth expire self / logout feat: auth expire self / logout Jan 19, 2023
@GeorgeMac
Copy link
Member

On the PUT vs GET this is definitely something to consider. PUT could be better in some ways.

I couldn't really make the callback a PUT or POST because the callback is a target that an external provider redirects the browser to.
Using GET means you can navigate a browser to the path and get the behaviour.
Is this something we desire for Flipt? As in, the ability to navigate to this expire route and logout.

It is not really an abusable thing given out setup. The flipt_client_token cookie is set with SameSite: strict.
So, an attacker couldn't force someone to logout. Because they would navigate from another domain and the browser would omit the cookie.
TL;DR GET wouldn't be abusable. But it is just the experience we want.

Copy link
Member

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

Nice. I think this looks great.

My feedback is mostly at our discretion to take it or leave it. So I have approved.

@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2023

Codecov Report

Merging #1279 (88b6a24) into main (4902107) will decrease coverage by 0.12%.
The diff coverage is 82.75%.

@@            Coverage Diff             @@
##             main    #1279      +/-   ##
==========================================
- Coverage   79.82%   79.70%   -0.12%     
==========================================
  Files          42       43       +1     
  Lines        3217     3267      +50     
==========================================
+ Hits         2568     2604      +36     
- Misses        521      531      +10     
- Partials      128      132       +4     
Impacted Files Coverage Δ
internal/storage/auth/auth.go 19.23% <ø> (ø)
internal/storage/auth/memory/store.go 77.09% <66.66%> (-0.77%) ⬇️
internal/server/auth/method/oidc/http.go 83.50% <75.00%> (ø)
internal/server/auth/http.go 81.81% <81.81%> (ø)
internal/server/auth/server.go 82.00% <88.88%> (+1.51%) ⬆️
internal/storage/auth/sql/store.go 88.60% <100.00%> (+0.50%) ⬆️
internal/storage/oplock/sql/sql.go 90.82% <0.00%> (-5.51%) ⬇️

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

@markphelps markphelps enabled auto-merge (squash) January 20, 2023 18:35
@markphelps markphelps merged commit f6d5e53 into main Jan 20, 2023
@markphelps markphelps deleted the auth-expire-self branch January 20, 2023 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants