refresh the access token in the background #7942
Conversation
BacLuc
force-pushed
the
feature-refresh-jwt-token
branch
from
d3359d2 to
358d897
Compare
⛔ Feature branch deployment currently inactive.If the PR is still open, you can add the |
There was a problem hiding this comment.
This did not work in my tests yet.
- I logged in with username and password, and got 3 cookies
- I waited for more than 30 minutes
- I refreshed the page
- I got to the login page and suddenly had 4 cookies. The fourth one called ph_phc_Tbfg4EiRsr5iefEoth2Y1Hi3sttTeLQ5RV5TLg4hL1W_posthog. What is this new cookie for?
- The terms of service on the landing page also need to be adjusted, right now they state that all cookies are deleted after 12 hours. The refresh token I got was valid for 1 month.
|
@CodeRabbit review this PR |
|
@BacLuc, I'll review the PR changes for implementing JWT refresh token functionality in the background. ✅ Actions performedReview triggered.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces server-side JWT refresh tokens via GesdinetJWTRefreshTokenBundle, configures a /token/refresh endpoint, adds a RefreshToken entity and migration, adjusts JWT TTL to 1800 seconds, updates security, routes, and OpenAPI docs, and implements client-side token auto-refresh with scheduling, integrating it into app initialization and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Frontend(auth.js)
participant API as API /token/refresh
participant Auth as Auth Server
Browser->>Frontend(auth.js): App mounted -> initRefresh()
Frontend(auth.js)->>Frontend(auth.js): Check JWT cookie & expiry
alt Not logged in or expired
Frontend(auth.js)->>API: POST /token/refresh
API->>Auth: Validate refresh token
Auth-->>API: Issue new JWT + refresh token (cookies)
API-->>Frontend(auth.js): 204 (cookies set)
Frontend(auth.js)->>Frontend(auth.js): Schedule next refresh (half-life)
else Valid token
Frontend(auth.js)->>Frontend(auth.js): Schedule refresh at half remaining TTL
end
sequenceDiagram
participant Client
participant Symfony Security
participant Gesdinet Bundle
participant DB as DB (refresh_tokens)
Client->>Symfony Security: POST /token/refresh
Symfony Security->>Gesdinet Bundle: Handle refresh_jwt
Gesdinet Bundle->>DB: Validate single-use refresh token
DB-->>Gesdinet Bundle: Token valid
Gesdinet Bundle-->>Client: 204 with new JWT cookies + new refresh_token cookie
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (2)
frontend/src/plugins/auth.js (1)
7-12: Fix Axios 401 detection to use error.response.statusAxios populates status on error.response, not on error. Current check won’t trigger on 401 responses.
-axios.interceptors.response.use(null, (error) => { - if (error.status === 401) { - logout().then(() => {}) - } - return Promise.reject(error) -}) +axios.interceptors.response.use( + (response) => response, + (error) => { + if (error?.response?.status === 401) { + // Do not await to avoid blocking interceptor chain + logout().catch(() => {}) + } + return Promise.reject(error) + } +)frontend/src/plugins/__tests__/auth.spec.js (1)
520-557: Add refreshToken link to the mocked API root in createState()Helps href resolution find /token/refresh without trying to fetch /api.
login: { href: '/authentication_token', }, + refreshToken: { + href: '/token/refresh', + },
🧹 Nitpick comments (8)
api/config/packages/lexik_jwt_authentication.yaml (1)
10-11: TTL reduction looks good; consider adding clock skew and verify client refresh cadence30-minute access tokens align with the PR goals. To reduce edge 401s around expiry, consider configuring a small clock skew and ensure the frontend schedules refresh slightly before expiry with jitter.
- Optional: add Lexik clock skew to tolerate time drift.
- Verify frontend refresh runs at ~80–90% of TTL with jitter to avoid thundering herd.
Example (optional):
lexik_jwt_authentication: # Tokens are valid for 30 minutes. token_ttl: 1800 + # Allow small time drift between clients and server (seconds) + clock_skew: 60api/src/Serializer/Normalizer/UriTemplateNormalizer.php (1)
54-54: Consider being explicit about templated=false for consistencyOther non-templated links sometimes set 'templated' => false explicitly. Not required, but it improves consistency/readability.
- $result['_links']['refreshToken'] = ['href' => $this->urlGenerator->generate('api_refresh_token')]; + $result['_links']['refreshToken'] = ['href' => $this->urlGenerator->generate('api_refresh_token'), 'templated' => false];api/config/packages/security.yaml (1)
52-52: Restrict access_control to POST for /token/refresh.
Tighten the rule to the actual method to reduce surface area.Apply:
- - { path: ^/token/refresh, roles: PUBLIC_ACCESS } + - { path: ^/token/refresh$, methods: [POST], roles: PUBLIC_ACCESS }api/migrations/schema/Version20250809140557.php (1)
18-21: Optional: silence PHPMD UnusedFormalParameter on $schema.
Standard for Doctrine migrations; avoid noisy static analysis.Add suppression on methods:
public function up(Schema $schema): void { + // @phpmdSuppressWarnings(UnusedFormalParameter) $this->addSql('CREATE TABLE refresh_tokens (refresh_token VARCHAR(128) NOT NULL, username VARCHAR(255) NOT NULL, valid TIMESTAMP(0) WITHOUT TIME ZONE NOT NULL, id INT GENERATED BY DEFAULT AS IDENTITY NOT NULL, PRIMARY KEY (id))'); $this->addSql('CREATE UNIQUE INDEX UNIQ_9BACE7E1C74F2195 ON refresh_tokens (refresh_token)'); } public function down(Schema $schema): void { + // @phpmdSuppressWarnings(UnusedFormalParameter) $this->addSql('DROP TABLE refresh_tokens'); }Also applies to: 23-25
api/tests/Api/SnapshotTests/__snapshots__/ResponseSnapshotTest__testOpenApiSpecMatchesSnapshot__1.yml (1)
34708-34708: Avoid hard-coding example cookie names/domains in the descriptionThe description hard-codes
example_com_jwt_hp,example_com_jwt_s, andexample_com_refresh_token. This can drift from configuration. Prefer a generic description or inject the actual configured cookie names from parameters.Example:
- description: 'Get a refreshed JWT token split across the two cookies example_com_jwt_hp and example_com_jwt_s. Also returns a new refresh token example_com_refresh_token.' + description: 'Sets refreshed JWT cookies (header.payload and signature split) and a new refresh token cookie.'frontend/src/plugins/auth.js (2)
189-193: Null-out timer handle after clearing to avoid stale referencesNot strictly required, but makes the state unambiguous and prevents double-clears.
export async function logout() { if (scheduledRefresh != null) { clearTimeout(scheduledRefresh) + scheduledRefresh = null }
16-37: Capture hash fragment in originalTarget when navigating backMinor UX nit: preserve window.location.hash when restoring the original target after a background refresh.
let originalTarget = `${window.location.pathname}` if (window.location.search) { originalTarget += `?${window.location.search}` } + if (window.location.hash) { + originalTarget += `${window.location.hash}` + }api/src/OpenApi/RefreshTokenDecorator.php (1)
21-25: Tighten the 204 description and align wording with JwtDecoratorConsistency nit: mirror the phrasing used in JwtDecorator and explicitly mention cookie-only mechanism.
- '204' => [ - 'description' => "Get a refreshed JWT token split across the two cookies {$cookiePrefix}jwt_hp and {$cookiePrefix}jwt_s. Also returns a new refresh token {$cookiePrefix}refresh_token.", - ], + '204' => [ + 'description' => "Refreshed JWT split across {$cookiePrefix}jwt_hp and {$cookiePrefix}jwt_s; a new refresh token is returned in {$cookiePrefix}refresh_token (all via Set-Cookie).", + ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api/composer.lockis excluded by!**/*.lockapi/symfony.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
api/composer.json(1 hunks)api/config/bundles.php(2 hunks)api/config/packages/gesdinet_jwt_refresh_token.yaml(1 hunks)api/config/packages/lexik_jwt_authentication.yaml(1 hunks)api/config/packages/security.yaml(2 hunks)api/config/routes.yaml(1 hunks)api/config/services.yaml(1 hunks)api/migrations/schema/Version20250809140557.php(1 hunks)api/src/Entity/RefreshToken.php(1 hunks)api/src/OpenApi/JwtDecorator.php(1 hunks)api/src/OpenApi/RefreshTokenDecorator.php(1 hunks)api/src/Serializer/Normalizer/UriTemplateNormalizer.php(1 hunks)api/tests/Api/SnapshotTests/EndpointPerformanceTest.php(1 hunks)api/tests/Api/SnapshotTests/ResponseSnapshotTest.php(1 hunks)api/tests/Api/SnapshotTests/__snapshots__/ResponseSnapshotTest__testOpenApiSpecMatchesSnapshot__1.yml(1 hunks)api/tests/Api/SnapshotTests/__snapshots__/ResponseSnapshotTest__testRootEndpointMatchesSnapshot__1.json(1 hunks)api/tests/Serializer/Normalizer/UriTemplateNormalizerTest.php(2 hunks)frontend/src/main.js(2 hunks)frontend/src/plugins/__tests__/auth.spec.js(4 hunks)frontend/src/plugins/auth.js(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
frontend/src/main.js (1)
frontend/src/plugins/auth.js (1)
initRefresh(16-37)
frontend/src/plugins/auth.js (1)
frontend/src/plugins/__tests__/auth.spec.js (1)
apiStore(16-16)
api/src/OpenApi/RefreshTokenDecorator.php (1)
api/src/OpenApi/JwtDecorator.php (2)
__construct(13-13)__invoke(15-65)
frontend/src/plugins/__tests__/auth.spec.js (1)
frontend/src/plugins/auth.js (4)
auth(210-225)auth(210-225)isLoggedIn(85-93)isLoggedIn(86-86)
🪛 PHPMD (2.15.0)
api/migrations/schema/Version20250809140557.php
18-18: Avoid unused parameters such as '$schema'. (Unused Code Rules)
(UnusedFormalParameter)
23-23: Avoid unused parameters such as '$schema'. (Unused Code Rules)
(UnusedFormalParameter)
🪛 GitHub Actions: CI (mandatory checks)
frontend/src/plugins/__tests__/auth.spec.js
[error] 1-1: Unhandled Rejection: AxiosError: Network Error during API calls in tests. Tests attempted to fetch '/api' (baseURL '/api', URL '/'), causing 19 unhandled errors across tests (e.g., 'schedules refresh when refreshing', 'schedules refresh after login', 'clears refresh timer on logout').
🪛 YAMLlint (1.37.1)
api/config/services.yaml
[warning] 107-107: wrong indentation: expected 8 but found 6
(indentation)
[warning] 109-109: wrong indentation: expected 10 but found 8
(indentation)
🔇 Additional comments (11)
api/tests/Api/SnapshotTests/ResponseSnapshotTest.php (1)
120-120: Exclude /token/refresh from collection tests — LGTMThe refresh endpoint is not a standard GET collection; exclusion avoids false positives.
api/tests/Serializer/Normalizer/UriTemplateNormalizerTest.php (1)
48-50: Test updates for refresh link — LGTMRoute mapping and expected link for refreshToken are correct and aligned with the new route.
Also applies to: 97-99
api/tests/Api/SnapshotTests/EndpointPerformanceTest.php (1)
247-248: Exclude /token/refresh from performance tests — LGTMAppropriate exclusion for a non-GET auth endpoint.
api/src/OpenApi/JwtDecorator.php (1)
41-41: OpenAPI description updated to include refresh cookie — LGTMAccurately documents the additional refresh_token cookie returned at login.
frontend/src/main.js (1)
26-26: Import looks correct and aligns with new background refresh flow.api/tests/Api/SnapshotTests/__snapshots__/ResponseSnapshotTest__testRootEndpointMatchesSnapshot__1.json (1)
114-116: Snapshot update for refreshToken link is correct.
The root document exposing /token/refresh via HAL link matches the new endpoint.api/config/bundles.php (1)
9-9: Bundle registration is correct.
Registering GesdinetJWTRefreshTokenBundle in all environments is appropriate.Also applies to: 48-48
api/config/packages/security.yaml (1)
31-31: JWT entry point and refresh_jwt configuration look good.
Configuration matches Gesdinet + Lexik integration.Also applies to: 39-41
api/src/Entity/RefreshToken.php (1)
1-10: Entity mapping is minimal and correct.
Extending the bundle’s entity and mapping to refresh_tokens table is sufficient.api/config/packages/gesdinet_jwt_refresh_token.yaml (1)
1-11: Secure cookie-based refresh token config looks solid.
http_only + same_site=strict + secure via env and single_use are good choices. The prefixed token_parameter_name avoids collisions.api/tests/Api/SnapshotTests/__snapshots__/ResponseSnapshotTest__testOpenApiSpecMatchesSnapshot__1.yml (1)
34695-34711: Cookie Attributes Verified and DocumentedI’ve confirmed that your JWT and refresh‐token cookies are configured with the recommended security attributes:
• api/config/packages/lexik_jwt_authentication.yaml
–%env(COOKIE_PREFIX)%jwt_hp: samesite=strict, path=/, secure (via COOKIE_SECURE), HttpOnly=false
–%env(COOKIE_PREFIX)%jwt_s: samesite=strict, path=/, secure, HttpOnly=true• api/config/packages/gesdinet_jwt_refresh_token.yaml
– refresh_token cookie: enabled, same_site=strict, path=/, secure, HttpOnly=trueNo explicit max-age/lifetime is set (cookies default to session scope). If you’d rather tie cookie expiry to your 1800 s JWT TTL, you can add a
lifetime: 1800(ormax_age: 1800) to the access-token cookies.Next steps:
- Update your OpenAPI spec to document these cookie attributes.
- Consider adding origin/referrer checks or CSRF protection on your POST endpoints (
/authentication_token,/token/refresh).Let me know if you’d like assistance updating the OpenAPI description.
.../Api/SnapshotTests/__snapshots__/ResponseSnapshotTest__testOpenApiSpecMatchesSnapshot__1.yml
Outdated
Show resolved
Hide resolved
Core Meeting DecisionNice work! |
|
Minor: when you sign out, delete local storage and then refresh the login page you get this error 
|
BacLuc
force-pushed
the
feature-refresh-jwt-token
branch
from
43d08d3 to
90c96a9
Compare
|
Github somehow cannot show the checks that are running. |
Made the TTL configurable up to the helm chart. |
There was a problem hiding this comment.
It works a little better now, but I still have some issues:
- I logged in with username and password, and got 3 cookies
- I noticed that the session cookies had an expiry time for 12 hours. I didn't want to wait that long, so I deleted the jwt_hp and jwt_s cookies manually (to simulate them expiring)
- When refreshing the page in this state, I get to the login page for a split second and then back to where I was before, with 3 cookies. This works correctly 👍
- When doing any interaction other than refreshing the page instead (in this state with expired session cookies), I just get sent to the login page. This includes navigating any link in the app, or trying to change some input field.
Since the normal interactions with eCamp v3 are not to just refresh the page, I think this is not really helping yet.
Also, we still need an adjustment PR to the terms of service, as mentioned in my previous comment.
Could you please test the same i did? |
BacLuc
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
|
Sorry, the update did not inject the correct lifetime of the cookie. I will fix that. |
BacLuc
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
BacLuc
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
Now the short ttl is active, fixed it |
BacLuc
force-pushed
the
feature-refresh-jwt-token
branch
from
87ce3f6 to
fcf2b8d
Compare
BacLuc
had a problem deploying
to
feature-branch
GitHub Actions
Error
Use gesdinet/jwt-refresh-token-bundle to achieve this. The access token lifetime was reduced to 30 mins according to https://cloud.google.com/apigee/docs/api-platform/antipatterns/oauth-long-expiration as a starting point. Do not log out if the refresh in the background fails, the user might want to backup the content he is currently editing. One proposed pattern is to intercept 401 responses and refresh the token when the requests fail. Because we often have multiple requests running, this did not work well. Now we have a timeout in the background which refreshes the access token periodically. closes ecamp#4898
Don't add it to the workflows yet because ecamp#8025 will make this a lot easier. Has merge conflict with ecamp#8025 The ttl will be made shorter ones all users have refresh tokens.
That you also refresh the token when you reactivate the tab. Using the vuex store to get this value was not reliable when the tab was reactivated. -> get it directly from localStorage Issue: ecamp#4898
BacLuc
force-pushed
the
feature-refresh-jwt-token
branch
from
fcf2b8d to
c483357
Compare
BacLuc
had a problem deploying
to
feature-branch
GitHub Actions
Failure
BacLuc
had a problem deploying
to
feature-branch
GitHub Actions
Failure
BacLuc
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
4 participants
Use gesdinet/jwt-refresh-token-bundle to achieve this. The access token lifetime was reduced to 30 mins
according to https://cloud.google.com/apigee/docs/api-platform/antipatterns/oauth-long-expiration as a starting point.
Do not log out if the refresh in the background fails, the user might want to backup the content he is currently editing.
One proposed pattern is to intercept 401 responses and refresh the token when the requests fail. Because we often have multiple requests running, this did not work well. Now we have a timeout in the background which refreshes the access token periodically.
closes #4898