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

fix: update JWT cookie at every request #18994

Merged
merged 4 commits into from
Mar 25, 2024

Conversation

mcollovati
Copy link
Collaborator

@mcollovati mcollovati commented Mar 19, 2024

Description

Fixes stateless authentication, by updating the JWT cookies at every request, preventing the browser to remove the cookie after initial max-age time is expired.

Fixes #18895

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

Fixes stateless authentication, by updating the JWT cookies at every request,
preventing the browser to remove the cookie after initial max-age time is expired.

Fixes #18880
@mcollovati
Copy link
Collaborator Author

Tested the change also with Hilla integration test

Copy link

github-actions bot commented Mar 19, 2024

Test Results

1 091 files  + 1  1 091 suites  +1   1h 24m 50s ⏱️ + 2m 47s
6 930 tests + 6  6 881 ✅ + 6  49 💤 ±0  0 ❌ ±0 
7 290 runs  +41  7 229 ✅ +41  61 💤 ±0  0 ❌ ±0 

Results for commit 52de1f8. ± Comparison against base commit 94b2560.

This pull request removes 1 and adds 7 tests. Note that renamed tests count towards both.
com.vaadin.flow.spring.security.stateless.SerializedJwtSplitCookieRepositoryTest ‑ saveSerializedJwt_resets_cookiePair
com.vaadin.flow.spring.security.stateless.JwtStatelessAuthenticationTest ‑ authenticated_jwtCookieUpdatedAtEveryRequest
com.vaadin.flow.spring.security.stateless.JwtStatelessAuthenticationTest ‑ logout_jwtCookieExpired
com.vaadin.flow.spring.security.stateless.JwtStatelessAuthenticationTest ‑ publicResource_authenticated_jwtCookieSet
com.vaadin.flow.spring.security.stateless.JwtStatelessAuthenticationTest ‑ publicResource_notAuthenticated_jwtCookieNotSet
com.vaadin.flow.spring.security.stateless.JwtStatelessAuthenticationTest ‑ successfulAuthentication_jwtCookieSet
com.vaadin.flow.spring.security.stateless.SerializedJwtSplitCookieRepositoryTest ‑ saveSerializedJwt_authenticatedRequest_resets_cookiePair
com.vaadin.flow.spring.security.stateless.SerializedJwtSplitCookieRepositoryTest ‑ saveSerializedJwt_unauthenticatedRequest_doNotSet_cookiePair

♻️ This comment has been updated with latest results.

@mcollovati
Copy link
Collaborator Author

Removed workaround for spring-projects/spring-security#12579 since it has been fixed.

@mshabarov mshabarov self-requested a review March 25, 2024 06:45
@@ -84,18 +97,32 @@ public final class VaadinStatelessSecurityConfigurer<H extends HttpSecurityBuild

private SecretKeyConfigurer secretKeyConfigurer;

public void setSharedObjects(HttpSecurity http) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is public in a public class. Maybe it's better to deprecate it, rather than remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make sense. I'll change it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -84,18 +97,32 @@ public final class VaadinStatelessSecurityConfigurer<H extends HttpSecurityBuild

private SecretKeyConfigurer secretKeyConfigurer;

public void setSharedObjects(HttpSecurity http) {
public static void apply(HttpSecurity http,
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs javadocs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

@mshabarov
Copy link
Contributor

Tested the hilla-auth example project with this patch and the expiration date of cookie and saved JWT token are changed from request to request as expected.

Copy link

Quality Gate Passed Quality Gate passed

Issues
13 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@mshabarov mshabarov merged commit ce9c3d2 into main Mar 25, 2024
26 checks passed
@mshabarov mshabarov deleted the spikes/18880_flow_stateless_security branch March 25, 2024 15:12
vaadin-bot pushed a commit that referenced this pull request Mar 25, 2024
* fix: update JWT cookie at every request

Fixes stateless authentication, by updating the JWT cookies at every request,
preventing the browser to remove the cookie after initial max-age time is expired.

Fixes #18880

* apply review suggestions

---------

Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
vaadin-bot added a commit that referenced this pull request Mar 25, 2024
* fix: update JWT cookie at every request

Fixes stateless authentication, by updating the JWT cookies at every request,
preventing the browser to remove the cookie after initial max-age time is expired.

Fixes #18880

* apply review suggestions

---------

Co-authored-by: Marco Collovati <marco@vaadin.com>
Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Stateless authentication cookie and JWT expiration is not refreshed at every request
3 participants