fix: Salesforce tokens add token_lifetime#27264
Conversation
- Add unit tests for getSalesforceTokenLifetime function - Add integration tests for token lifecycle management in CrmService - Fix type error in CrmService.ts by extracting refreshToken variable Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
| * @param forceIntrospection - If true, always introspect to recalibrate token_lifetime (e.g., after unexpected expiry) | ||
| * @param existingTokenLifetime - The current token_lifetime to reuse if not forcing introspection | ||
| */ | ||
| private refreshAccessToken = async ({ |
There was a problem hiding this comment.
Extract the call to refresh the access token
|
|
||
| // Introspect if forced or if we don't have a token_lifetime yet | ||
| let tokenLifetime = existingTokenLifetime; | ||
| if (forceIntrospection || !tokenLifetime) { |
There was a problem hiding this comment.
We'll query the introspection endpoint if there is no token_lifetime property on the credential or if we get a token error if the credential was marked as not expired but was. This is to handle the unlikely case that an org changes their token life spans.
| }; | ||
| }; | ||
|
|
||
| private getClient = async (credential: CredentialPayload) => { |
There was a problem hiding this comment.
Same method as before to get the jsforce client
|
|
||
| // Check if token is still valid | ||
| // issued_at is in milliseconds (string), token_lifetime is in seconds | ||
| const BUFFER_MS = 5 * 60 * 1000; // 5 minutes buffer before expiry |
There was a problem hiding this comment.
Added new check to see if the token is still valid to avoid making a token call on every request. All access tokens are still valid until they are expired. Issuing a new token does not invalidate any existing tokens.
| instanceUrl: credentialKey.instance_url, | ||
| accessToken: credentialKey.access_token, | ||
| refreshToken: credentialKey.refresh_token, | ||
| refreshFn: async (conn, callback) => { |
There was a problem hiding this comment.
New addition to the jsforce.connection if we get an token error then retry to refetch the token only once
| this.hasAttemptedRefresh = true; | ||
|
|
||
| try { | ||
| // Force introspection to recalibrate token_lifetime after unexpected expiry |
There was a problem hiding this comment.
While we handle this case. It will be very unlikely.
| }): Promise<number> { | ||
| const { consumer_key, consumer_secret } = await getSalesforceAppKeys(); | ||
|
|
||
| const response = await fetch(`${instanceUrl}/services/oauth2/introspect`, { |
There was a problem hiding this comment.
We need to hit this introspection endpoint to calculate if a token is still valid after it was issued.
E2E results are ready! |
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/app-store/salesforce/lib/CrmService.ts">
<violation number="1" location="packages/app-store/salesforce/lib/CrmService.ts:262">
P1: `hasAttemptedRefresh` is set the first time `refreshFn` runs and never cleared, so every later token expiry immediately fails with “Token refresh already attempted.” Reset the flag after the refresh attempt so the connection can refresh tokens again while still preventing concurrent refresh loops.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Devin AI is addressing Cubic AI's review feedbackA Devin session has been created to address the issues identified by Cubic AI. |
|
I reviewed the Cubic AI feedback regarding the Issue summary: The Why this is being skipped: Per the review guidelines, only issues with confidence scores of 9/10 or higher should be automatically fixed. This issue has a confidence score of 8/10. Recommendation: A human reviewer should evaluate whether this is a valid concern. If the |
Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
emrysal
left a comment
There was a problem hiding this comment.
Looks good to me, happy to merge & release.
What does this PR do?
Currently, Salesforce OAuth tokens do not include an expiry date. This means we refresh Salesforce tokens on every request, which is inefficient and can cause rate limiting issues.
This PR adds token lifetime tracking by:
token_lifetimein the credential alongside the access tokenissued_at + token_lifetimewith a 5-minute buffer)refreshFnto the jsforce connection that handles unexpected expiry by re-introspecting to recalibrate the lifetimeVisual Demo (For contributors especially)
N/A - Backend-only change with no UI impact.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
token_lifetimevalueissued_atto an old value) and verify refresh works correctlyHuman Review Checklist
issued_atis in milliseconds (string),token_lifetimeis in seconds${instanceUrl}/services/oauth2/introspect) is correcthasAttemptedRefreshflag doesn't cause issues if the service instance is reused across multiple requestsCredentialRepository.updateWhereIdexists and works as expectedChecklist
Updates since last revision
Added comprehensive test coverage:
getSalesforceTokenLifetime.test.ts: Unit tests for the new introspection function (5 tests covering lifetime calculation, endpoint parameters, Basic auth, error handling, and different lifetimes)CrmService.integration.test.ts: Added "Token lifecycle management" test suite (4 tests covering valid token skip, expired token refresh, legacy credentials without token_lifetime, and 5-minute buffer behavior)CrmService.tsby extractingrefreshTokenvariable after the null check to ensure type safety in therefreshFncallbackLink to Devin run: https://app.devin.ai/sessions/649172013c17485889563f15ca2c83e6
Requested by: joe@cal.com (@joeauyeung)