From b43ab5be84b6d081936b7fc96a99ab37041a44cf Mon Sep 17 00:00:00 2001 From: patrick-codaio <84040450+patrick-codaio@users.noreply.github.com> Date: Tue, 8 Oct 2024 15:37:51 -0700 Subject: [PATCH] Revert to allowing authorizationUrl & tokenUrl to not match networkDomain (#3085) --- dist/testing/upload_validation.js | 24 +++++++++++-------- test/upload_validation_test.ts | 40 +++++++++++++------------------ testing/upload_validation.ts | 27 +++++++++++++-------- 3 files changed, 48 insertions(+), 43 deletions(-) diff --git a/dist/testing/upload_validation.js b/dist/testing/upload_validation.js index 0b89d6cda..ee94013d7 100644 --- a/dist/testing/upload_validation.js +++ b/dist/testing/upload_validation.js @@ -1572,7 +1572,7 @@ ${endpointKey ? 'endpointKey is set' : `requiresEndpointUrl is ${requiresEndpoin var _a; const metadata = untypedMetadata; for (const authInfo of getAuthentications(metadata)) { - const { name, authentication: authentication } = authInfo; + const { name, authentication } = authInfo; if (authentication.type !== types_1.AuthenticationType.CodaApiHeaderBearerToken) { return; } @@ -1846,7 +1846,7 @@ ${endpointKey ? 'endpointKey is set' : `requiresEndpointUrl is ${requiresEndpoin var _a; const data = untypedData; for (const authInfo of getAuthentications(data)) { - const { name, authentication: authentication } = authInfo; + const { name, authentication } = authInfo; const authNetworkDomains = getDeclaredAuthNetworkDomains(authentication); if (!(0, object_utils_1.isDefined)(authNetworkDomains)) { // This is a Various or None auth pack. @@ -1885,7 +1885,7 @@ ${endpointKey ? 'endpointKey is set' : `requiresEndpointUrl is ${requiresEndpoin return; } for (const authInfo of getAuthentications(data)) { - const { name, authentication: authentication } = authInfo; + const { name, authentication } = authInfo; const readableAuthTitle = name === types_5.ReservedAuthenticationNames.Default ? 'setUserAuthentication()' : `authentication ${name}`; const usedNetworkDomains = getUsedAuthNetworkDomains(authentication); if (usedNetworkDomains) { @@ -1905,7 +1905,7 @@ ${endpointKey ? 'endpointKey is set' : `requiresEndpointUrl is ${requiresEndpoin .superRefine((untypedData, context) => { const data = untypedData; for (const authInfo of getAuthentications(data)) { - const { name, authentication: authentication } = authInfo; + const { name, authentication } = authInfo; const authNetworkDomains = getDeclaredAuthNetworkDomains(authentication); if (!(0, object_utils_1.isDefined)(authNetworkDomains)) { // This is a Various or None auth pack. @@ -1969,8 +1969,12 @@ function getAuthentications(data) { * Return all the domain names that should be validated against declared network domains. * This function just ignores any relative or un-parse-able URLs, trusting that other validations * have already caught such issues. + * + * We may eventually decide that includeOAuthTokenUrls should default true, but that would be + * backwards incompatible for packs that use services like Google, where their authorizationUrl & + * tokenUrl use a different domain than the API domain that is set on networkDomains. */ -function getUsedAuthNetworkDomains(authentication) { +function getUsedAuthNetworkDomains(authentication, includeOAuthTokenUrls = false) { if (authentication.type === types_1.AuthenticationType.None || authentication.type === types_1.AuthenticationType.Various) { return undefined; } @@ -1979,12 +1983,12 @@ function getUsedAuthNetworkDomains(authentication) { if (endpointDomain) { domains.push(endpointDomain); } + if (!includeOAuthTokenUrls) { + return domains; + } switch (type) { case types_1.AuthenticationType.OAuth2: { - const { authorizationUrl, tokenUrl, endpointDomain } = authentication; - if (endpointDomain) { - domains.push(endpointDomain); - } + const { authorizationUrl, tokenUrl } = authentication; const parsedAuthUrl = (0, url_parse_1.default)(authorizationUrl); if (parsedAuthUrl.hostname) { domains.push(parsedAuthUrl.hostname); @@ -1996,7 +2000,7 @@ function getUsedAuthNetworkDomains(authentication) { return domains; } case types_1.AuthenticationType.OAuth2ClientCredentials: { - const { tokenUrl, endpointDomain } = authentication; + const { tokenUrl } = authentication; if (endpointDomain) { domains.push(endpointDomain); } diff --git a/test/upload_validation_test.ts b/test/upload_validation_test.ts index cb2e3dc57..185ce92b9 100644 --- a/test/upload_validation_test.ts +++ b/test/upload_validation_test.ts @@ -4168,6 +4168,19 @@ describe('Pack metadata Validation', async () => { ]); }); + it('allows google packs to use multiple domains', async () => { + const metadata = createFakePackVersionMetadata({ + defaultAuthentication: { + type: AuthenticationType.OAuth2, + authorizationUrl: 'https://accounts.google.com/o/oauth2/auth', + tokenUrl: 'https://accounts.google.com/o/oauth2/token', + scopes: [], + }, + networkDomains: ['googleapis.com'], + }); + await validateJson(metadata); + }); + it('missing networkDomains when specifying authentication', async () => { const metadata = createFakePackVersionMetadata({ networkDomains: undefined, @@ -4482,23 +4495,6 @@ describe('Pack metadata Validation', async () => { ]); }); - it('validates oauth URL domains against network domain', async () => { - const metadata = createFakePackVersionMetadata({ - defaultAuthentication: { - type: AuthenticationType.OAuth2, - authorizationUrl: 'https://wrongdomain.com/oauth/authorize', - tokenUrl: 'https://wrongdomain.com/oauth/token', - }, - }); - const err = await validateJsonAndAssertFails(metadata, '1.0.0'); - assert.deepEqual(err.validationErrors, [ - { - message: `Domain wrongdomain.com is used in setUserAuthentication() but not declared in the pack's "networkDomains".`, - path: 'authentication.defaultUserAuthentication', - }, - ]); - }); - it('validates auth URL must be parse-able', async () => { const metadata = createFakePackVersionMetadata({ defaultAuthentication: { @@ -4544,9 +4540,8 @@ describe('Pack metadata Validation', async () => { adminAuthentications: [ { authentication: { - type: AuthenticationType.OAuth2, - authorizationUrl: 'https://wrongdomain.com/oauth/authorize', - tokenUrl: 'https://wrongdomain.com/oauth/token', + type: AuthenticationType.HeaderBearerToken, + endpointDomain: 'wrongdomain.com', canSyncPermissions: true, }, name: 'goodName', @@ -4592,9 +4587,8 @@ describe('Pack metadata Validation', async () => { adminAuthentications: [ { authentication: { - type: AuthenticationType.OAuth2, - authorizationUrl: 'https://b.com/auth', - tokenUrl: 'https://b.com/token', + type: AuthenticationType.HeaderBearerToken, + endpointDomain: 'b.com', canSyncPermissions: true, }, name: 'foo', diff --git a/testing/upload_validation.ts b/testing/upload_validation.ts index fc9134ac1..205f72548 100644 --- a/testing/upload_validation.ts +++ b/testing/upload_validation.ts @@ -1981,7 +1981,7 @@ ${endpointKey ? 'endpointKey is set' : `requiresEndpointUrl is ${requiresEndpoin const metadata = untypedMetadata as PackVersionMetadata; for (const authInfo of getAuthentications(metadata)) { - const {name, authentication: authentication} = authInfo; + const {name, authentication} = authInfo; if (authentication.type !== AuthenticationType.CodaApiHeaderBearerToken) { return; } @@ -2274,7 +2274,7 @@ ${endpointKey ? 'endpointKey is set' : `requiresEndpointUrl is ${requiresEndpoin const data = untypedData as PackVersionMetadata; for (const authInfo of getAuthentications(data)) { - const {name, authentication: authentication} = authInfo; + const {name, authentication} = authInfo; const authNetworkDomains = getDeclaredAuthNetworkDomains(authentication); if (!isDefined(authNetworkDomains)) { @@ -2319,7 +2319,7 @@ ${endpointKey ? 'endpointKey is set' : `requiresEndpointUrl is ${requiresEndpoin } for (const authInfo of getAuthentications(data)) { - const {name, authentication: authentication} = authInfo; + const {name, authentication} = authInfo; const readableAuthTitle = name === ReservedAuthenticationNames.Default ? 'setUserAuthentication()' : `authentication ${name}`; @@ -2346,7 +2346,7 @@ ${endpointKey ? 'endpointKey is set' : `requiresEndpointUrl is ${requiresEndpoin const data = untypedData as PackVersionMetadata; for (const authInfo of getAuthentications(data)) { - const {name, authentication: authentication} = authInfo; + const {name, authentication} = authInfo; const authNetworkDomains = getDeclaredAuthNetworkDomains(authentication); if (!isDefined(authNetworkDomains)) { @@ -2424,8 +2424,15 @@ function getAuthentications(data: PackVersionMetadata): Array<{name: string; aut * Return all the domain names that should be validated against declared network domains. * This function just ignores any relative or un-parse-able URLs, trusting that other validations * have already caught such issues. + * + * We may eventually decide that includeOAuthTokenUrls should default true, but that would be + * backwards incompatible for packs that use services like Google, where their authorizationUrl & + * tokenUrl use a different domain than the API domain that is set on networkDomains. */ -function getUsedAuthNetworkDomains(authentication: AuthenticationMetadata): string[] | undefined { +function getUsedAuthNetworkDomains( + authentication: AuthenticationMetadata, + includeOAuthTokenUrls: boolean = false, +): string[] | undefined { if (authentication.type === AuthenticationType.None || authentication.type === AuthenticationType.Various) { return undefined; } @@ -2434,12 +2441,12 @@ function getUsedAuthNetworkDomains(authentication: AuthenticationMetadata): stri if (endpointDomain) { domains.push(endpointDomain); } + if (!includeOAuthTokenUrls) { + return domains; + } switch (type) { case AuthenticationType.OAuth2: { - const {authorizationUrl, tokenUrl, endpointDomain} = authentication; - if (endpointDomain) { - domains.push(endpointDomain); - } + const {authorizationUrl, tokenUrl} = authentication; const parsedAuthUrl = URLParse(authorizationUrl); if (parsedAuthUrl.hostname) { domains.push(parsedAuthUrl.hostname); @@ -2451,7 +2458,7 @@ function getUsedAuthNetworkDomains(authentication: AuthenticationMetadata): stri return domains; } case AuthenticationType.OAuth2ClientCredentials: { - const {tokenUrl, endpointDomain} = authentication; + const {tokenUrl} = authentication; if (endpointDomain) { domains.push(endpointDomain); }