Skip to content

fix: GoogleCalendar - Ensure that Delegation Credentials are able to reuse access_token #21389

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zomars merged 3 commits into from
May 26, 2025
Merged

fix: GoogleCalendar - Ensure that Delegation Credentials are able to reuse access_token #21389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d778249
persist-access-token-dwd
hariombalhara May 19, 2025
ef45c63
Add missing appId
hariombalhara May 26, 2025
0c13e1b
Merge branch 'main' into 05-19-persist-access-token-dwd
Udit-takkar May 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 83 additions & 5 deletions packages/app-store/_utils/oauth/OAuthManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,49 @@ describe("Credential Sync Disabled", () => {
expiry_date: 0,
});
});

test("`fetchNewTokenObject` is called if token is about to expire. Also, `updateTokenObject` is called with currentTokenObject and newTokenObject merged", async () => {
const userId = 1;
const invalidateTokenObject = vi.fn();
const expireAccessToken = vi.fn();
const updateTokenObject = vi.fn();
const currentTokenObject = getDummyTokenObject({
refresh_token: "REFRESH_TOKEN",
expiry_date: Date.now() - 2 * 1000,
});
const newTokenObjectInResponse = getDummyTokenObject();
const fetchNewTokenObject = vi
.fn()
.mockResolvedValue(generateJsonResponse({ json: newTokenObjectInResponse }));

const auth1 = new OAuthManager({
credentialSyncVariables: useCredentialSyncVariables,
resourceOwner: {
type: "user",
id: userId,
},
appSlug: "demo-app",
currentTokenObject: currentTokenObject,
fetchNewTokenObject,
isTokenObjectUnusable: async () => {
return null;
},
isAccessTokenUnusable: async () => {
return null;
},
invalidateTokenObject: invalidateTokenObject,
updateTokenObject: updateTokenObject,
expireAccessToken: expireAccessToken,
});
await auth1.getTokenObjectOrFetch();
expect(fetchNewTokenObject).toHaveBeenCalledWith({ refreshToken: "REFRESH_TOKEN" });
expect(updateTokenObject).toHaveBeenCalledWith({
...currentTokenObject,
...newTokenObjectInResponse,
// Consider the token as expired as newTokenObjectInResponse didn't have expiry
expiry_date: 0,
});
});
});

describe("checking using expires_in", () => {
Expand Down Expand Up @@ -285,7 +328,7 @@ describe("Credential Sync Disabled", () => {
appSlug: "demo-app",
currentTokenObject: getDummyTokenObject({
refresh_token: "REFRESH_TOKEN",
expires_in: Date.now() / 1000 + 5,
expires_in: Date.now() / 1000 + 10,
}),
fetchNewTokenObject,
isTokenObjectUnusable: async () => {
Expand Down Expand Up @@ -388,7 +431,7 @@ describe("Credential Sync Disabled", () => {
appSlug: "demo-app",
currentTokenObject: getDummyTokenObject(),
fetchNewTokenObject: async () => {
throw new Error("testError");
throw new Error("fetchNewTokenObject error");
},
isTokenObjectUnusable: async () => {
return null;
Expand All @@ -403,7 +446,7 @@ describe("Credential Sync Disabled", () => {

expect(async () => {
return auth.getTokenObjectOrFetch();
}).rejects.toThrowError("Invalid token response");
}).rejects.toThrowError("fetchNewTokenObject error");
Copy link
Member Author

@hariombalhara hariombalhara May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we report the actual error

});

test("if fetchNewTokenObject throws error that's handled by isTokenObjectUnusable then auth.getTokenObjectOrFetch would still throw error but a different one as access_token won't be available", async () => {
Expand All @@ -421,7 +464,7 @@ describe("Credential Sync Disabled", () => {
appSlug: "demo-app",
currentTokenObject: getDummyTokenObject(),
fetchNewTokenObject: async () => {
throw new Error("testError");
throw new Error("fetchNewTokenObject error");
},
isTokenObjectUnusable: async () => {
return {
Expand All @@ -438,7 +481,42 @@ describe("Credential Sync Disabled", () => {

expect(async () => {
return auth.getTokenObjectOrFetch();
}).rejects.toThrowError("Invalid token response");
}).rejects.toThrowError("fetchNewTokenObject error");
});

test("when currentTokenObject is not set, but getCurrentTokenObject is set", async () => {
const userId = 1;
const getCurrentTokenObject = vi.fn().mockResolvedValue(getDummyTokenObject());
const auth = new OAuthManager({
credentialSyncVariables: useCredentialSyncVariables,
resourceOwner: {
type: "user",
id: userId,
},
appSlug: "demo-app",
isTokenExpiring: async () => {
return false;
},
isAccessTokenUnusable: async () => {
return null;
},
getCurrentTokenObject: getCurrentTokenObject,
fetchNewTokenObject: async () => {
return generateJsonResponse({ json: getDummyTokenObject() });
},
updateTokenObject: vi.fn(),
invalidateTokenObject: vi.fn(),
expireAccessToken: vi.fn(),
isTokenObjectUnusable: async () => {
return null;
},
});
const tokenObject = await auth.getTokenObjectOrFetch();
expect(getCurrentTokenObject).toHaveBeenCalled();
expect(tokenObject).toEqual({
token: expect.objectContaining(getDummyTokenObject()),
isUpdated: false,
});
});
});

Expand Down
94 changes: 73 additions & 21 deletions packages/app-store/_utils/oauth/OAuthManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,27 +58,32 @@ type CredentialSyncVariables = {

APP_CREDENTIAL_SHARING_ENABLED: boolean;
};

type CurrentTokenObject = z.infer<typeof OAuth2UniversalSchema>;
type GetCurrentTokenObject = () => Promise<CurrentTokenObject>;
/**
* Manages OAuth2.0 tokens for an app and resourceOwner
* If expiry_date or expires_in isn't provided in token then it is considered expired immediately(if credential sync is not enabled)
* If credential sync is enabled, the token is considered expired after a year. It is expected to be refreshed by the API request from the credential source(as it knows when the token is expired)
*/
export class OAuthManager {
private currentTokenObject: z.infer<typeof OAuth2UniversalSchema>;
protected currentTokenObject: CurrentTokenObject | null;
private getCurrentTokenObject: GetCurrentTokenObject | null;
private resourceOwner: ResourceOwner;
private appSlug: string;
private fetchNewTokenObject: FetchNewTokenObject;
private updateTokenObject: UpdateTokenObject;
private isTokenObjectUnusable: isTokenObjectUnusable;
private isAccessTokenUnusable: isAccessTokenUnusable;
private isTokenExpired: IsTokenExpired;
private isTokenExpiring: IsTokenExpired;
private invalidateTokenObject: InvalidateTokenObject;
private expireAccessToken: ExpireAccessToken;
private credentialSyncVariables: CredentialSyncVariables;
private useCredentialSync: boolean;
private autoCheckTokenExpiryOnRequest: boolean;

constructor({
getCurrentTokenObject,
resourceOwner,
appSlug,
currentTokenObject,
Expand All @@ -90,14 +95,22 @@ export class OAuthManager {
expireAccessToken,
credentialSyncVariables,
autoCheckTokenExpiryOnRequest = true,
isTokenExpired = (token: z.infer<typeof OAuth2TokenResponseInDbWhenExistsSchema>) => {
isTokenExpiring = (token: z.infer<typeof OAuth2TokenResponseInDbWhenExistsSchema>) => {
// TODO: Make it configurable later
// 5 seconds before expiry so that we can refresh the token before any request is made with the expired token
const expireThreshold = 5000;
const isGoingToExpire = getExpiryDate() - expireThreshold <= Date.now();
log.debug(
"isTokenExpired called",
safeStringify({ expiry_date: token.expiry_date, currentTime: Date.now() })
"isTokenExpiring",
safeStringify({
isGoingToExpire,
expiry_date: token.expiry_date,
expires_in: token.expires_in,
currentTime: Date.now(),
expireThreshold,
})
);

return getExpiryDate() <= Date.now();

return isGoingToExpire;
function isRelativeToEpoch(relativeTimeInSeconds: number) {
return relativeTimeInSeconds > 1000000000; // If it is more than 2001-09-09 it can be considered relative to epoch. Also, that is more than 30 years in future which couldn't possibly be relative to current time
}
Expand Down Expand Up @@ -137,11 +150,14 @@ export class OAuthManager {
/**
* The current token object.
*/
currentTokenObject: z.infer<typeof OAuth2UniversalSchema>;
currentTokenObject?: CurrentTokenObject;

getCurrentTokenObject?: GetCurrentTokenObject;
/**
* The unique identifier of the app that the token is for. It is required to do credential syncing in self-hosting
*/
appSlug: string;

/**
*
* It could be null in case refresh_token isn't available. This is possible when credential sync happens from a third party who doesn't want to share refresh_token and credential syncing has been disabled after the sync has happened.
Expand Down Expand Up @@ -172,15 +188,19 @@ export class OAuthManager {
/**
* If there is a different way to check if the token is expired(and not the standard way of checking expiry_date)
*/
isTokenExpired?: IsTokenExpired;
isTokenExpiring?: IsTokenExpired;
}) {
if (!getCurrentTokenObject && !currentTokenObject) {
throw new Error("One of getCurrentTokenObject or currentTokenObject is required");
}
this.resourceOwner = resourceOwner;
this.currentTokenObject = currentTokenObject;
this.currentTokenObject = currentTokenObject ?? null;
this.getCurrentTokenObject = getCurrentTokenObject ?? null;
this.appSlug = appSlug;
this.fetchNewTokenObject = fetchNewTokenObject;
this.isTokenObjectUnusable = isTokenObjectUnusable;
this.isAccessTokenUnusable = isAccessTokenUnusable;
this.isTokenExpired = isTokenExpired;
this.isTokenExpiring = isTokenExpiring;
this.invalidateTokenObject = invalidateTokenObject;
this.expireAccessToken = expireAccessToken;
this.credentialSyncVariables = credentialSyncVariables;
Expand All @@ -205,29 +225,45 @@ export class OAuthManager {
return !response.ok || response.status < 200 || response.status >= 300;
}

/**
* Gets the current token object as is if not expired.
* If expired, it refreshes the token and returns the new token object.
* It also calls the `updateTokenObject` to update the token object in the database if it is changed.
*/
public async getTokenObjectOrFetch() {
const myLog = log.getSubLogger({
prefix: [`getTokenObjectOrFetch:appSlug=${this.appSlug}`],
});
const isExpired = await this.isTokenExpired(this.currentTokenObject);
let currentTokenObject;
if (this.currentTokenObject) {
currentTokenObject = this.currentTokenObject;
} else if (this.getCurrentTokenObject) {
this.currentTokenObject = currentTokenObject = await this.getCurrentTokenObject();
} else {
throw new Error("Neither currentTokenObject nor getCurrentTokenObject is set");
}
const isExpired = await this.isTokenExpiring(currentTokenObject);
myLog.debug(
"getTokenObjectOrFetch called",
safeStringify({
currentTokenObjectHasAccessToken: !!currentTokenObject.access_token,
isExpired,
resourceOwner: this.resourceOwner,
})
);

if (!isExpired) {
myLog.debug("Token is not expired. Returning the current token object");
return { token: this.normalizeNewlyReceivedToken(this.currentTokenObject), isUpdated: false };
return { token: this.normalizeNewlyReceivedToken(currentTokenObject), isUpdated: false };
} else {
const token = {
// Keep the old token object as it is, as some integrations don't send back all the props e.g. refresh_token isn't sent again by Google Calendar
// It also allows any other properties set to be retained.
// Let's not use normalizedCurrentTokenObject here as `normalizeToken` could possible be not idempotent
...this.currentTokenObject,
...this.normalizeNewlyReceivedToken(await this.refreshOAuthToken()),
...currentTokenObject,
...this.normalizeNewlyReceivedToken(
await this.refreshOAuthToken({ refreshToken: currentTokenObject.refresh_token ?? null })
),
};
myLog.debug("Token is expired. So, returning new token object");
this.currentTokenObject = token;
Expand Down Expand Up @@ -268,7 +304,6 @@ export class OAuthManager {
) {
let response;
const myLog = log.getSubLogger({ prefix: ["request"] });

if (this.autoCheckTokenExpiryOnRequest) {
await this.getTokenObjectOrFetch();
}
Expand All @@ -284,6 +319,7 @@ export class OAuthManager {
response = handleFetchError(e);
}
} else {
this.assertCurrentTokenObjectIsSet();
const { url, options } = customFetchOrUrlAndOptions;
const headers = {
Authorization: `Bearer ${this.currentTokenObject.access_token}`,
Expand Down Expand Up @@ -330,6 +366,16 @@ export class OAuthManager {
return { tokenStatus: tokenStatus, json };
}

/**
* currentTokenObject is set through getTokenObjectOrFetch call
*/
private assertCurrentTokenObjectIsSet(): asserts this is this & {
currentTokenObject: CurrentTokenObject;
} {
if (!this.currentTokenObject) {
throw new Error("currentTokenObject is not set");
}
}
/**
* It doesn't automatically detect the response for tokenObject and accessToken becoming invalid
* Could be used when you expect a possible non JSON response as well.
Expand All @@ -340,6 +386,9 @@ export class OAuthManager {
if (this.autoCheckTokenExpiryOnRequest) {
await this.getTokenObjectOrFetch();
}
// Either `getTokenObjectOrFetch` has been called through autoCheckTokenExpiryOnRequest or through a direct call to it outside OAuthManager
// In both cases, `currentTokenObject` is set
this.assertCurrentTokenObjectIsSet();
const headers = {
Authorization: `Bearer ${this.currentTokenObject.access_token}`,
"Content-Type": "application/json",
Expand Down Expand Up @@ -409,10 +458,9 @@ export class OAuthManager {
}

// TODO: On regenerating access_token successfully, we should call makeTokenObjectValid(to counter invalidateTokenObject). This should fix stale banner in UI to reconnect when the connection is working
private async refreshOAuthToken() {
private async refreshOAuthToken({ refreshToken }: { refreshToken: string | null }) {
Copy link
Member Author

@hariombalhara hariombalhara May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass refresh token directly from the place we are sure currentTokenObject is available. Keeps TS happy

const myLog = log.getSubLogger({ prefix: ["refreshOAuthToken"] });
let response;
const refreshToken = this.currentTokenObject.refresh_token ?? null;
if (this.resourceOwner.id && this.useCredentialSync) {
if (
!this.credentialSyncVariables.CREDENTIAL_SYNC_SECRET ||
Expand Down Expand Up @@ -468,9 +516,8 @@ export class OAuthManager {

const clonedResponse = response.clone();
myLog.info(
"Response from refreshOAuthToken",
"Response status from refreshOAuthToken",
safeStringify({
text: await clonedResponse.text(),
Copy link
Member Author

@hariombalhara hariombalhara May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't log entire response unnecessarily

ok: clonedResponse.ok,
status: clonedResponse.status,
statusText: clonedResponse.statusText,
Expand All @@ -485,6 +532,11 @@ export class OAuthManager {
} else if (tokenStatus === TokenStatus.UNUSABLE_ACCESS_TOKEN) {
await this.expireAccessToken();
}

if (json && json.myFetchError) {
// Throw error back as it isn't a valid token response and we can't process it further
throw new Error(json.myFetchError);
}
Comment on lines +536 to +539
Copy link
Member Author

@hariombalhara hariombalhara May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We report the actual error now instead of going forward with this json and then being unable to parse the obviously invalid token and then throwing 'Invalid token response' error.

This was hiding the actual reason behind the error.

const parsedToken = OAuth2UniversalSchemaWithCalcomBackwardCompatibility.safeParse(json);
if (!parsedToken.success) {
myLog.error(
Expand Down
Loading
Loading