feat: improve authenticateClient by avoiding unnecessary token creation
#2443
Conversation
wilsonrivera
requested review from
JivusAyrus,
StarpTech and
thisisnithin
as code owners
WalkthroughThe Keycloak authentication service is enhanced to check JWT token expiration and conditionally refresh tokens. When an access token exists, it's decoded to verify expiration; if valid, re-authentication is skipped. If expired and a refresh token is available, a refresh token grant is used to obtain a new token. The password grant remains the fallback mechanism. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @controlplane/src/core/services/Keycloak.ts:
- Around line 41-48: The current check in the Keycloak token validation treats
tokens missing an exp claim as valid indefinitely; update the logic in the
Keycloak method that inspects this.client.accessToken (where decodeJwt is used)
so that absence of exp does NOT cause an early return—only return early when exp
exists and exp*1000 > Date.now(); if exp is missing or expired, fall through to
re-authenticate. Ensure you only change the conditional that now reads something
like `if (!exp || exp * 1000 > Date.now()) return;` to require exp to be present
and valid (e.g., `if (exp && exp * 1000 > Date.now()) return;`), so tokens
without expiration force re-authentication.
🧹 Nitpick comments (2)
controlplane/src/core/services/Keycloak.ts (2)
50-58: Token refresh logic looks correct.The refresh token flow properly uses the
refresh_tokengrant type and maintains consistency with the password flow by using the sameadmin-cliclient ID.Consider adding logging when token refresh succeeds to aid in debugging and monitoring:
🔎 Suggested logging enhancement
if (this.client.refreshToken) { await this.client.auth({ grantType: 'refresh_token', refreshToken: this.client.refreshToken, clientId: 'admin-cli', }); - + this.logger.debug('Successfully refreshed Keycloak admin access token'); return; }
59-61: Add logging in the error handler for better observability.The silent catch block makes it difficult to diagnose token refresh failures. Adding debug-level logging would help identify whether the issue is token decode failure, refresh token expiration, or network problems.
🔎 Proposed enhancement
- } catch { - // ignore + } catch (error) { + this.logger.debug(error, 'Token validation or refresh failed, falling back to password grant'); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/core/services/Keycloak.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: wilsonrivera
Repo: wundergraph/cosmo PR: 2100
File: docker/keycloak/realm.json:1768-1774
Timestamp: 2025-08-04T17:50:30.553Z
Learning: The sso-cookie-authenticator in Keycloak authentication flows must use "REQUIRED" as the requirement setting, not "ALTERNATIVE". This is due to implementation constraints of the custom SSOCookieAuthenticator class.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
controlplane/src/core/services/Keycloak.ts (2)
44-48: Consider the security implications of not verifying JWT signatures.
decodeJwtonly decodes the token without cryptographic signature verification. While this may be acceptable for the admin client's self-managed token (since Keycloak will validate it on actual API calls), it does mean that a tampered or forged token could bypass the expiration check.Please confirm that skipping signature verification aligns with your security requirements. If this token could be exposed or if there's a risk of tampering, consider using
jwtVerifyfrom jose instead, which validates signatures against a JWKS endpoint.
6-6: No action needed. Thejosedependency is properly declared incontrolplane/package.jsonas"jose": "^5.2.4". The import statement at line 6 is valid and will not cause runtime failures.Likely an incorrect or invalid review comment.
| if (this.client.accessToken) { | ||
| // We already have an access token, determine whether the token still valid before trying to authenticate again | ||
| try { | ||
| const { exp } = decodeJwt(this.client.accessToken); | ||
| if (!exp || exp * 1000 > Date.now()) { | ||
| // Either the access token never expires or it hasn't expired | ||
| return; | ||
| } |
There was a problem hiding this comment.
Reconsider accepting tokens without an expiration claim.
Line 45 returns early when !exp is true, meaning tokens without an expiration claim are treated as valid indefinitely. This poses a security risk because:
- Compromised tokens would remain valid forever
- There's no mechanism to force re-authentication
- It violates JWT security best practices
Consider changing the logic to require expiration:
🔎 Proposed fix to require expiration claim
- const { exp } = decodeJwt(this.client.accessToken);
- if (!exp || exp * 1000 > Date.now()) {
- // Either the access token never expires or it hasn't expired
+ const { exp } = decodeJwt(this.client.accessToken);
+ if (exp && exp * 1000 > Date.now()) {
+ // Access token is valid and hasn't expired
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (this.client.accessToken) { | |
| // We already have an access token, determine whether the token still valid before trying to authenticate again | |
| try { | |
| const { exp } = decodeJwt(this.client.accessToken); | |
| if (!exp || exp * 1000 > Date.now()) { | |
| // Either the access token never expires or it hasn't expired | |
| return; | |
| } | |
| if (this.client.accessToken) { | |
| // We already have an access token, determine whether the token still valid before trying to authenticate again | |
| try { | |
| const { exp } = decodeJwt(this.client.accessToken); | |
| if (exp && exp * 1000 > Date.now()) { | |
| // Access token is valid and hasn't expired | |
| return; | |
| } |
🤖 Prompt for AI Agents
In @controlplane/src/core/services/Keycloak.ts around lines 41 - 48, The current
check in the Keycloak token validation treats tokens missing an exp claim as
valid indefinitely; update the logic in the Keycloak method that inspects
this.client.accessToken (where decodeJwt is used) so that absence of exp does
NOT cause an early return—only return early when exp exists and exp*1000 >
Date.now(); if exp is missing or expired, fall through to re-authenticate.
Ensure you only change the conditional that now reads something like `if (!exp
|| exp * 1000 > Date.now()) return;` to require exp to be present and valid
(e.g., `if (exp && exp * 1000 > Date.now()) return;`), so tokens without
expiration force re-authentication.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (47.05%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2443 +/- ##
==========================================
- Coverage 62.74% 62.74% -0.01%
==========================================
Files 296 296
Lines 41353 41370 +17
Branches 4247 4251 +4
==========================================
+ Hits 25949 25957 +8
- Misses 15383 15392 +9
Partials 21 21
🚀 New features to boost your workflow:
|
The goal of this PR is to avoid creating Keycloak access tokens every time we call
authenticateClient.To achieve this we are taking a look at the existing token (if any) and, if that token still valid, we don't do anything, however, if there is a refresh token, we try to refresh the current access token with it and if everything fails or is unavailable, we fallback to the original flow or creating a new access token.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist