From 6331624ab159164d4a064210604722c860b91d8d Mon Sep 17 00:00:00 2001 From: Sattvik Chakravarthy Date: Tue, 28 May 2024 11:41:08 +0530 Subject: [PATCH] fix: thirdparty recursion and config reference issues (#846) --- CHANGELOG.md | 5 + .../thirdparty/providers/configUtils.js | 73 +++++---- .../recipe/thirdparty/providers/utils.d.ts | 4 +- .../recipe/thirdparty/providers/utils.js | 1 - .../thirdparty/providers/configUtils.ts | 76 +++++---- lib/ts/recipe/thirdparty/providers/utils.ts | 4 +- test/thirdparty/provider.config.test.js | 153 ++++++++++++++++++ 7 files changed, 250 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3cf72a72..c5bd65564 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - If you were using `ThirdPartyPasswordless`, you should now init `ThirdParty` and `Passwordless` recipes separately. The config for the individual recipes are mostly the same, except the syntax may be different. Check our recipe guides for [ThirdParty](https://supertokens.com/docs/thirdparty/introduction) and [Passwordless](https://supertokens.com/docs/passwordless/introduction) for more information. +### Fixes + +- Fixes override recursion build-up in built-in providers due to the modification of the `input.override` object in the ThirdParty providers list. +- Fixes issue with reference to `config` object in `TypeProvider` in the provider override. The issue was the `originalImplementation.config` object did not have the updated `config` values that was being used in the provider implementation. + ## [17.1.2] - 2024-05-21 ### Fixes diff --git a/lib/build/recipe/thirdparty/providers/configUtils.js b/lib/build/recipe/thirdparty/providers/configUtils.js index 14044f753..8fd13fb86 100644 --- a/lib/build/recipe/thirdparty/providers/configUtils.js +++ b/lib/build/recipe/thirdparty/providers/configUtils.js @@ -15,38 +15,53 @@ function getProviderConfigForClient(providerConfig, clientConfig) { exports.getProviderConfigForClient = getProviderConfigForClient; async function fetchAndSetConfig(provider, clientType, userContext) { let config = await provider.getConfigForClientType({ clientType, userContext }); - config = await utils_1.discoverOIDCEndpoints(config); - provider.config = config; + await utils_1.discoverOIDCEndpoints(config); + // We are doing Object.assign here to retain the reference to the provider.config which originally came from + // the impl object in the `custom.ts`. We are doing this because if we replace the provider.config with the + // new config object, then when the user overrides a provider function, and access the `originalImplementaion.config`, + // it will not receive the updated config, since the user's override would have returned a new provider object and + // the config is being set on to it. By doing Object.assign, we retain the original reference and the config is + // consistently available in the override as well as the implementation. + Object.assign(provider.config, config); } function createProvider(input) { - if (input.config.thirdPartyId.startsWith("active-directory")) { - return _1.ActiveDirectory(input); - } else if (input.config.thirdPartyId.startsWith("apple")) { - return _1.Apple(input); - } else if (input.config.thirdPartyId.startsWith("bitbucket")) { - return _1.Bitbucket(input); - } else if (input.config.thirdPartyId.startsWith("discord")) { - return _1.Discord(input); - } else if (input.config.thirdPartyId.startsWith("facebook")) { - return _1.Facebook(input); - } else if (input.config.thirdPartyId.startsWith("github")) { - return _1.Github(input); - } else if (input.config.thirdPartyId.startsWith("gitlab")) { - return _1.Gitlab(input); - } else if (input.config.thirdPartyId.startsWith("google-workspaces")) { - return _1.GoogleWorkspaces(input); - } else if (input.config.thirdPartyId.startsWith("google")) { - return _1.Google(input); - } else if (input.config.thirdPartyId.startsWith("okta")) { - return _1.Okta(input); - } else if (input.config.thirdPartyId.startsWith("linkedin")) { - return _1.Linkedin(input); - } else if (input.config.thirdPartyId.startsWith("twitter")) { - return _1.Twitter(input); - } else if (input.config.thirdPartyId.startsWith("boxy-saml")) { - return _1.BoxySAML(input); + // We are cloning the input object because the built-in providers modify the `override` in the input + // object, and we don't want to modify the original input object. If we do not clone the input object, + // we add a new function for override and then call the override from the input object, and this way, + // the override call stack keeps increasing over time. After a certain number of calls, the call stack + // will overflow and the thirdparty provider will start failing. + // We are only doing a shallow clone because the `override` is in the top level of the input object, + // and other properties are just used for reading. The input object is also not exposed by any of the + // interface for modification. + const clonedInput = Object.assign({}, input); + if (clonedInput.config.thirdPartyId.startsWith("active-directory")) { + return _1.ActiveDirectory(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("apple")) { + return _1.Apple(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("bitbucket")) { + return _1.Bitbucket(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("discord")) { + return _1.Discord(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("facebook")) { + return _1.Facebook(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("github")) { + return _1.Github(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("gitlab")) { + return _1.Gitlab(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("google-workspaces")) { + return _1.GoogleWorkspaces(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("google")) { + return _1.Google(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("okta")) { + return _1.Okta(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("linkedin")) { + return _1.Linkedin(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("twitter")) { + return _1.Twitter(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("boxy-saml")) { + return _1.BoxySAML(clonedInput); } - return custom_1.default(input); + return custom_1.default(clonedInput); } async function findAndCreateProviderInstance(providers, thirdPartyId, clientType, userContext) { for (const providerInput of providers) { diff --git a/lib/build/recipe/thirdparty/providers/utils.d.ts b/lib/build/recipe/thirdparty/providers/utils.d.ts index 34ac7d565..c43eb396d 100644 --- a/lib/build/recipe/thirdparty/providers/utils.d.ts +++ b/lib/build/recipe/thirdparty/providers/utils.d.ts @@ -32,6 +32,4 @@ export declare function verifyIdTokenFromJWKSEndpointAndGetPayload( jwks: jose.JWTVerifyGetKey, otherOptions: jose.JWTVerifyOptions ): Promise; -export declare function discoverOIDCEndpoints( - config: ProviderConfigForClientType -): Promise; +export declare function discoverOIDCEndpoints(config: ProviderConfigForClientType): Promise; diff --git a/lib/build/recipe/thirdparty/providers/utils.js b/lib/build/recipe/thirdparty/providers/utils.js index 1d6c81d8d..0fc79f65a 100644 --- a/lib/build/recipe/thirdparty/providers/utils.js +++ b/lib/build/recipe/thirdparty/providers/utils.js @@ -143,6 +143,5 @@ async function discoverOIDCEndpoints(config) { config.jwksURI = oidcInfo.jwks_uri; } } - return config; } exports.discoverOIDCEndpoints = discoverOIDCEndpoints; diff --git a/lib/ts/recipe/thirdparty/providers/configUtils.ts b/lib/ts/recipe/thirdparty/providers/configUtils.ts index 01eadcd34..547923e8a 100644 --- a/lib/ts/recipe/thirdparty/providers/configUtils.ts +++ b/lib/ts/recipe/thirdparty/providers/configUtils.ts @@ -37,41 +37,57 @@ export function getProviderConfigForClient( async function fetchAndSetConfig(provider: TypeProvider, clientType: string | undefined, userContext: UserContext) { let config = await provider.getConfigForClientType({ clientType, userContext }); - config = await discoverOIDCEndpoints(config); - - provider.config = config; + await discoverOIDCEndpoints(config); + + // We are doing Object.assign here to retain the reference to the provider.config which originally came from + // the impl object in the `custom.ts`. We are doing this because if we replace the provider.config with the + // new config object, then when the user overrides a provider function, and access the `originalImplementaion.config`, + // it will not receive the updated config, since the user's override would have returned a new provider object and + // the config is being set on to it. By doing Object.assign, we retain the original reference and the config is + // consistently available in the override as well as the implementation. + Object.assign(provider.config, config); } function createProvider(input: ProviderInput): TypeProvider { - if (input.config.thirdPartyId.startsWith("active-directory")) { - return ActiveDirectory(input); - } else if (input.config.thirdPartyId.startsWith("apple")) { - return Apple(input); - } else if (input.config.thirdPartyId.startsWith("bitbucket")) { - return Bitbucket(input); - } else if (input.config.thirdPartyId.startsWith("discord")) { - return Discord(input); - } else if (input.config.thirdPartyId.startsWith("facebook")) { - return Facebook(input); - } else if (input.config.thirdPartyId.startsWith("github")) { - return Github(input); - } else if (input.config.thirdPartyId.startsWith("gitlab")) { - return Gitlab(input); - } else if (input.config.thirdPartyId.startsWith("google-workspaces")) { - return GoogleWorkspaces(input); - } else if (input.config.thirdPartyId.startsWith("google")) { - return Google(input); - } else if (input.config.thirdPartyId.startsWith("okta")) { - return Okta(input); - } else if (input.config.thirdPartyId.startsWith("linkedin")) { - return Linkedin(input); - } else if (input.config.thirdPartyId.startsWith("twitter")) { - return Twitter(input); - } else if (input.config.thirdPartyId.startsWith("boxy-saml")) { - return BoxySAML(input); + // We are cloning the input object because the built-in providers modify the `override` in the input + // object, and we don't want to modify the original input object. If we do not clone the input object, + // we add a new function for override and then call the override from the input object, and this way, + // the override call stack keeps increasing over time. After a certain number of calls, the call stack + // will overflow and the thirdparty provider will start failing. + // We are only doing a shallow clone because the `override` is in the top level of the input object, + // and other properties are just used for reading. The input object is also not exposed by any of the + // interface for modification. + const clonedInput = { ...input }; + + if (clonedInput.config.thirdPartyId.startsWith("active-directory")) { + return ActiveDirectory(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("apple")) { + return Apple(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("bitbucket")) { + return Bitbucket(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("discord")) { + return Discord(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("facebook")) { + return Facebook(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("github")) { + return Github(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("gitlab")) { + return Gitlab(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("google-workspaces")) { + return GoogleWorkspaces(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("google")) { + return Google(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("okta")) { + return Okta(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("linkedin")) { + return Linkedin(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("twitter")) { + return Twitter(clonedInput); + } else if (clonedInput.config.thirdPartyId.startsWith("boxy-saml")) { + return BoxySAML(clonedInput); } - return NewProvider(input); + return NewProvider(clonedInput); } export async function findAndCreateProviderInstance( diff --git a/lib/ts/recipe/thirdparty/providers/utils.ts b/lib/ts/recipe/thirdparty/providers/utils.ts index e1ed8a685..a4e4a7d7b 100644 --- a/lib/ts/recipe/thirdparty/providers/utils.ts +++ b/lib/ts/recipe/thirdparty/providers/utils.ts @@ -123,7 +123,7 @@ async function getOIDCDiscoveryInfo(issuer: string): Promise { return oidcInfo.jsonResponse!; } -export async function discoverOIDCEndpoints(config: ProviderConfigForClientType): Promise { +export async function discoverOIDCEndpoints(config: ProviderConfigForClientType): Promise { if (config.oidcDiscoveryEndpoint !== undefined) { const oidcInfo = await getOIDCDiscoveryInfo(config.oidcDiscoveryEndpoint); @@ -143,6 +143,4 @@ export async function discoverOIDCEndpoints(config: ProviderConfigForClientType) config.jwksURI = oidcInfo.jwks_uri; } } - - return config; } diff --git a/test/thirdparty/provider.config.test.js b/test/thirdparty/provider.config.test.js index 80b197e3a..1ae9a0a50 100644 --- a/test/thirdparty/provider.config.test.js +++ b/test/thirdparty/provider.config.test.js @@ -39,6 +39,159 @@ describe(`providerConfigTest: ${printPath("[test/thirdparty/provider.config.test await cleanST(); }); + it("test the provider override is not increasing call stack", async function () { + const connectionURI = await startSTWithMultitenancy(); + + let stackCount = []; + + STExpress.init({ + supertokens: { + connectionURI, + }, + appInfo: { + apiDomain: "api.supertokens.io", + appName: "SuperTokens", + websiteDomain: "supertokens.io", + }, + recipeList: [ + ThirdPartyRecipe.init({ + signInAndUpFeature: { + providers: [ + { + config: { + thirdPartyId: "google", + clients: [{ clientId: "test", clientSecret: "secret" }], + }, + override: (oI) => { + const err = new Error(); + stackCount.push(err.stack.split("\n").length); + return { + ...oI, + getAuthorisationRedirectURL: async (input) => { + return await oI.getAuthorisationRedirectURL(input); + }, + }; + }, + }, + ], + }, + }), + ], + }); + + for (let i = 0; i < 10; i++) { + await (await ThirdParty.getProvider("public", "google")).getAuthorisationRedirectURL({ + redirectURIOnProviderDashboard: "http://localhost:8080", + userContext: {}, + }); + } + + for (let i = 1; i < stackCount.length; i++) { + assert.equal(stackCount[i], stackCount[i - 1]); // stack should be same and not increasing + } + }); + + it("test the config is same within override and provider instance", async function () { + const connectionURI = await startSTWithMultitenancy(); + + let oIConfig; + STExpress.init({ + supertokens: { + connectionURI, + }, + appInfo: { + apiDomain: "api.supertokens.io", + appName: "SuperTokens", + websiteDomain: "supertokens.io", + }, + recipeList: [ + ThirdPartyRecipe.init({ + signInAndUpFeature: { + providers: [ + { + config: { + thirdPartyId: "google", + clients: [{ clientId: "test", clientSecret: "secret" }], + }, + override: (oI) => { + return { + ...oI, + getAuthorisationRedirectURL: async (input) => { + oIConfig = oI.config; + return await oI.getAuthorisationRedirectURL(input); + }, + }; + }, + }, + ], + }, + }), + ], + }); + + const provider = await ThirdParty.getProvider("public", "google"); + await provider.getAuthorisationRedirectURL({ + redirectURIOnProviderDashboard: "http://localhost:8080", + userContext: {}, + }); + + assert.strictEqual(provider.config, oIConfig); + }); + + it("test the config is same within override and provider instance when overriding getConfigForClientType", async function () { + const connectionURI = await startSTWithMultitenancy(); + + let oIConfig; + STExpress.init({ + supertokens: { + connectionURI, + }, + appInfo: { + apiDomain: "api.supertokens.io", + appName: "SuperTokens", + websiteDomain: "supertokens.io", + }, + recipeList: [ + ThirdPartyRecipe.init({ + signInAndUpFeature: { + providers: [ + { + config: { + thirdPartyId: "google", + clients: [{ clientId: "test", clientSecret: "secret" }], + }, + override: (oI) => { + return { + ...oI, + getConfigForClientType: async (input) => { + return { + id: "google", + clientId: "hello", + authorizationEndpoint: "http://localhost:8080/authorize", + }; + }, + getAuthorisationRedirectURL: async (input) => { + oIConfig = oI.config; + return await oI.getAuthorisationRedirectURL(input); + }, + }; + }, + }, + ], + }, + }), + ], + }); + + const provider = await ThirdParty.getProvider("public", "google"); + await provider.getAuthorisationRedirectURL({ + redirectURIOnProviderDashboard: "http://localhost:8080", + userContext: {}, + }); + + assert.strictEqual(provider.config, oIConfig); + }); + it("test built-in provider computed config from static config", async function () { const connectionURI = await startSTWithMultitenancy();