Revert to allowing authorizationUrl & tokenUrl to not match networkDo…

…main (#3085)
coda · Oct 8, 2024 · b43ab5b · b43ab5b
1 parent 00f0f33
commit b43ab5b
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 43 deletions.
diff --git a/dist/testing/upload_validation.js b/dist/testing/upload_validation.js
diff --git 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
@@ -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);
       }