OIDC Client and Journey Client Bug Fixes #511
Conversation
|
📝 WalkthroughWalkthroughRe-exports Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client Code
participant OIDC as oidc-client (renew flow)
participant Store as TokenStorage
participant Revoke as Revoke API
Client->>OIDC: trigger token renewal
OIDC->>Store: read current tokens (priorTokens)
alt priorTokens exist
OIDC->>Revoke: revoke priorTokens.accessToken
Revoke-->>OIDC: revoke result
OIDC->>Store: remove stored entry
else no priorTokens
Note right of OIDC: skip revoke/remove
end
OIDC->>OIDC: obtain newTokens
OIDC->>Store: set newTokens
OIDC-->>Client: return refreshed tokens
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
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. Comment |
| export * from './lib/client.store.js'; | ||
|
|
||
| // Re-export types from internal packages that consumers need | ||
| export { callbackType } from '@forgerock/sdk-types'; |
ryanbas21
left a comment
ryanbas21
left a comment
There was a problem hiding this comment.
cloud should be fixed hopefully today, approving now
|
View your CI Pipeline Execution ↗ for commit 0bc297f
☁️ Nx Cloud last updated this comment at |
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #511 +/- ##
===========================================
+ Coverage 18.79% 72.17% +53.37%
===========================================
Files 140 63 -77
Lines 27640 4830 -22810
Branches 980 476 -504
===========================================
- Hits 5195 3486 -1709
+ Misses 22445 1344 -21101
🚀 New features to boost your workflow:
|
|
Deployed 766a305 to https://ForgeRock.github.io/ping-javascript-sdk/pr-511/766a3057ccdfb1be78055ab121e109224e83ed76 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.4 KB, -100.0%) 📊 Minor Changes📈 @forgerock/oidc-client - 23.5 KB (+0.0 KB) ➖ No Changes➖ @forgerock/device-client - 9.2 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
|
Thanks for the quick review! Moving this to draft as I may find more bugs while I develop the new sample app and already added a new one to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @packages/oidc-client/src/lib/client.store.ts:
- Around line 292-304: The changes to the token renewal flow (Micro.tap callback
updating tokens) are unrelated to the callbackType export fix; revert or remove
the added logic around Micro.tap, the calls to
store.dispatch(oidcApi.endpoints.revoke.initiate(...)), storageClient.remove(),
and storageClient.set(newTokens) from this PR and instead place them in a
separate focused PR or commit that clearly documents the token renewal behavior;
ensure the current PR only contains the callbackType export fix and no
modifications to the Micro.tap/token revocation flow or storageClient usage.
- Around line 292-304: The Micro.tap handler currently calls
store.dispatch(oidcApi.endpoints.revoke.initiate(...)) without checking
wellknown.revocation_endpoint and then calls storageClient.remove() before
storageClient.set(newTokens), which can throw and create a storage gap; update
the Micro.tap callback to first check that wellknown.revocation_endpoint is
present before attempting revocation, wrap the revoke call in a try-catch (or
use Micro.tapError) so any revocation failure is logged/ignored and does not
prevent saving newTokens, and avoid calling storageClient.remove(); instead call
storageClient.set(newTokens) directly to atomically overwrite existing tokens
(keeping store.dispatch and oidcApi.endpoints.revoke.initiate as the revocation
call site).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/oidc-client/src/lib/client.store.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
⏰ 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). (2)
- GitHub Check: Mend Code Security Check
- GitHub Check: pr
| Micro.tap(async (newTokens) => { | ||
| if (tokens && 'accessToken' in tokens) { | ||
| await store.dispatch( | ||
| oidcApi.endpoints.revoke.initiate({ | ||
| accessToken: tokens.accessToken, | ||
| clientId: config.clientId, | ||
| endpoint: wellknown.revocation_endpoint, | ||
| }), | ||
| ); | ||
| await storageClient.remove(); | ||
| } | ||
| await storageClient.set(newTokens); | ||
| }), |
There was a problem hiding this comment.
These changes appear unrelated to the PR objectives.
The PR title indicates this is about fixing callbackType export in journey-client, but this file contains token renewal flow changes in the oidc-client package. Consider whether these changes should be in a separate PR for better traceability and review focus.
🤖 Prompt for AI Agents
In @packages/oidc-client/src/lib/client.store.ts around lines 292 - 304, The
changes to the token renewal flow (Micro.tap callback updating tokens) are
unrelated to the callbackType export fix; revert or remove the added logic
around Micro.tap, the calls to
store.dispatch(oidcApi.endpoints.revoke.initiate(...)), storageClient.remove(),
and storageClient.set(newTokens) from this PR and instead place them in a
separate focused PR or commit that clearly documents the token renewal behavior;
ensure the current PR only contains the callbackType export fix and no
modifications to the Micro.tap/token revocation flow or storageClient usage.
Consider error handling and atomic storage updates.
The token renewal logic has several concerns:
-
Critical: Revocation failure blocks new token storage — If the old token revocation fails (e.g., token already expired/invalid), the tap will throw and prevent storing
newTokens, leaving the user in a broken state. Consider usingMicro.tapErroror try-catch to ensure new tokens are stored even if revocation fails. -
Missing revocation endpoint check — Unlike the
token.revokemethod (line 331), this code doesn't verifywellknown.revocation_endpointexists before using it (line 298). This could cause runtime errors if the endpoint is unavailable. -
Atomic gap in storage updates — Calling
remove()followed byset()creates a brief window where no tokens exist in storage. Concurrent reads during this gap will fail. Consider directly callingstorageClient.set(newTokens)without the remove, sincesetshould overwrite existing values.
🔎 Proposed fix with better error handling
Micro.tap(async (newTokens) => {
if (tokens && 'accessToken' in tokens) {
- await store.dispatch(
- oidcApi.endpoints.revoke.initiate({
- accessToken: tokens.accessToken,
- clientId: config.clientId,
- endpoint: wellknown.revocation_endpoint,
- }),
- );
- await storageClient.remove();
+ if (wellknown.revocation_endpoint) {
+ try {
+ await store.dispatch(
+ oidcApi.endpoints.revoke.initiate({
+ accessToken: tokens.accessToken,
+ clientId: config.clientId,
+ endpoint: wellknown.revocation_endpoint,
+ }),
+ );
+ } catch (error) {
+ log.warn('Failed to revoke old access token during renewal', error);
+ }
+ }
}
await storageClient.set(newTokens);
}),🤖 Prompt for AI Agents
In @packages/oidc-client/src/lib/client.store.ts around lines 292 - 304, The
Micro.tap handler currently calls
store.dispatch(oidcApi.endpoints.revoke.initiate(...)) without checking
wellknown.revocation_endpoint and then calls storageClient.remove() before
storageClient.set(newTokens), which can throw and create a storage gap; update
the Micro.tap callback to first check that wellknown.revocation_endpoint is
present before attempting revocation, wrap the revoke call in a try-catch (or
use Micro.tapError) so any revocation failure is logged/ignored and does not
prevent saving newTokens, and avoid calling storageClient.remove(); instead call
storageClient.set(newTokens) directly to atomically overwrite existing tokens
(keeping store.dispatch and oidcApi.endpoints.revoke.initiate as the revocation
call site).
ancheetah
changed the title
JIRA Ticket
Description
Bug 1: fix callbackType export --> it's not really a type so it needs to be exported this way to be useful
Bug 2: fix token force renew --> calling oidcClient.user.info() after renewing tokens fails because the new token was mistakenly revoked instead of the old token
Summary by CodeRabbit
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.