Add token refresh logic if the user's access token is expired#528
Conversation
|
Warning Review limit reached
More reviews will be available in 13 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughisSignedIn() now returns true if already signed in, waits if a token refresh is underway, or attempts refreshAccessToken() and returns true on success or false on error; separately, the PR builder workflow changes the lint job's ChangesToken refresh handling
CI workflow matrix
Sequence DiagramsequenceDiagram
participant Caller
participant AuthClient
participant TokenRefresher
Caller->>AuthClient: isSignedIn()
alt AuthClient signed in
AuthClient-->>Caller: true
else Token refresh in progress
Caller->>TokenRefresher: await _isTokenRefreshing
TokenRefresher-->>AuthClient: refresh completes
AuthClient-->>Caller: isSignedIn() result
else Not signed in and not refreshing
Caller->>TokenRefresher: refreshAccessToken()
alt refresh succeeds
TokenRefresher-->>Caller: success -> return true
else refresh throws
TokenRefresher-->>Caller: error -> return false
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/browser/src/__legacy__/helpers/authentication-helper.ts`:
- Around line 708-719: The isSignedIn() method can trigger concurrent
refreshAccessToken() calls; modify isSignedIn() to reuse the same single-flight
coordination used in httpRequest() by consulting the _isTokenRefreshing
indicator (or the shared refresh promise) before starting a new refresh: if
_isTokenRefreshing is truthy await the existing refresh promise, otherwise
initiate refreshAccessToken() and set/clear the shared _isTokenRefreshing
promise/flag around it so parallel callers wait on the same promise and avoid
duplicate refreshes; keep existing behavior of returning true on successful
refresh and false on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26480d08-62c9-4604-b86a-ee8a37628682
📒 Files selected for processing (1)
packages/browser/src/__legacy__/helpers/authentication-helper.ts
ShanChathusanda93
force-pushed
the
refresh-call-issignedin-branch
branch
from
1c5dc9b to
3ed6e68
Compare
| Config, | ||
| } from '@asgardeo/javascript'; | ||
| import {SPAHelper} from './spa-helper'; | ||
| import { SPAHelper } from './spa-helper'; |
There was a problem hiding this comment.
hope these changes are not intentional
There was a problem hiding this comment.
seeems lint failure is due to these unintentional formatting changes. let's revert these
There was a problem hiding this comment.
Install the Prettier VSCode extension. And format the document.
| RP_IFRAME, | ||
| } from '../constants'; | ||
| import {HttpClient} from '@asgardeo/javascript'; | ||
| import { HttpClient } from '@asgardeo/javascript'; |
| import { SPACustomGrantConfig } from '../models/request-custom-grant'; | ||
| import { BrowserStorage } from '../models/storage'; | ||
| import { SPAUtils } from '../utils'; |
| if (await this._authenticationClient.isSignedIn()) { | ||
| return true; | ||
| } | ||
|
|
||
| // A refresh is already in progress — wait for it to finish then re-check. | ||
| if (this._isTokenRefreshing) { | ||
| await SPAUtils.until(() => !this._isTokenRefreshing); | ||
|
|
||
| return this._authenticationClient.isSignedIn(); | ||
| } | ||
|
|
||
| // Token may be expired — attempt a silent refresh before giving up. | ||
| try { | ||
| this._isTokenRefreshing = true; | ||
| await this.refreshAccessToken(); | ||
| this._isTokenRefreshing = false; | ||
|
|
||
| return true; | ||
| } catch { | ||
| this._isTokenRefreshing = false; | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
maybe we could use something like the following version to keep this._authenticationClient.isSignedIn method as the single source for determining the signed in status, instead of explicitly returning boolean status. (haven't tested the following myself though)
public async isSignedIn(): Promise<boolean> {
if (await this._authenticationClient.isSignedIn()) {
return this._authenticationClient.isSignedIn();
}
if (this._isTokenRefreshing) {
await SPAUtils.until(() => !this._isTokenRefreshing);
return this._authenticationClient.isSignedIn();
}
try {
this._isTokenRefreshing = true;
await this.refreshAccessToken();
} catch {
// Ignore refresh failures. Signed-in state is determined below.
} finally {
this._isTokenRefreshing = false;
}
return this._authenticationClient.isSignedIn();
}There was a problem hiding this comment.
Better to do this IMO. But +1 to merge the already tested path immediately and later bring this in.
ShanChathusanda93
force-pushed
the
refresh-call-issignedin-branch
branch
3 times, most recently
from
5c73f0a to
e1b2b25
Compare
ShanChathusanda93
force-pushed
the
refresh-call-issignedin-branch
branch
3 times, most recently
from
56a446c to
424b67a
Compare
ShanChathusanda93
force-pushed
the
refresh-call-issignedin-branch
branch
from
424b67a to
76ca694
Compare
🦋 Changeset detectedThe changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. |
…efresh fix) Restores silent token refresh on access-token expiry (upstream asgardeo/javascript#528). Before this, isSignedIn() returned false the moment the access token expired, bouncing idle engineers to login; AuthGuard gates the whole app on isSignedIn, so this degraded session stability. No application code changes — the consumed SDK surface (AsgardeoProvider props, useAsgardeo: isSignedIn/getAccessToken/getDecodedIdToken/signOut) is unchanged across the jump; tsc build passes. Peer @asgardeo/react-router@2.0.0 (needs >=0.15.0) still satisfied. Fixes #1885.
4 participants
Purpose
Previously,
isSignedIninAuthenticationHelperreturned false immediately when the access token was expired, causing theSPA-AUTH_CLIENT-VM-IV02error to be thrown for any method that required authentication (e.g., after a long idle period where the SPAHelper timer was no longer active).This fix attempts a silent token refresh via the browser's
refreshAccessTokenbefore returningfalse. If the refresh succeeds,isSignedInreturnstrueand the SPAHelper timer is re-armed. If the refresh fails, it returnsfalseas before.The fix is intentionally scoped to the browser layer (AuthenticationHelper) so it uses the browser-specific refresh flow without introducing network side effects into the base
@asgardeo/javascriptclient'sisSignedIn, which is a shared pure-check method.Related Issues
Related PRs
Checklist
Security checks
Summary by CodeRabbit
Bug Fixes
Refactor
Chores