fix: Custom type permissions issues from both authenticated and groups (#589)

Co-authored-by: James Jarvis <jjarvisp@amazon.com>

Author: stocaaro

.changeset/tidy-worlds-sort.md

@@ -0,0 +1,5 @@
+---
+'@aws-amplify/data-schema': patch
+---
+
+Fix custom type permissions issues from both authenticated and group authorization

packages/data-schema/__tests__/CustomOperations.test.ts

@@ -1021,6 +1021,45 @@ describe('CustomOperation transform', () => {
           });
         });
 
+        test('defineFunction for an async operation that has authorization rules for both group and authenticated', () => {
+          const fn1 = defineFunctionStub({});
+          const s = a.schema({
+            getPostDetails: a
+              .query()
+              .arguments({})
+              .handler([
+                a.handler.function(fn1).async(),
+              ])
+              .authorization((allow) => [allow.authenticated(), allow.group('TestGroup')]),
+          });
+
+          const { schema, lambdaFunctions } = s.transform();
+          expect(schema).toMatchSnapshot();
+        });
+
+        test('defineFunction for two async operations that have authorization rules for either group or authenticated', () => {
+          const fn1 = defineFunctionStub({});
+          const s = a.schema({
+            getPostDetailsA: a
+              .query()
+              .arguments({})
+              .handler([
+                a.handler.function(fn1).async(),
+              ])
+              .authorization((allow) => allow.group('TestGroup')),
+            getPostDetailsB: a
+              .query()
+              .arguments({})
+              .handler([
+                a.handler.function(fn1).async(),
+              ])
+              .authorization((allow) => allow.authenticated()),
+          });
+
+          const { schema, lambdaFunctions } = s.transform();
+          expect(schema).toMatchSnapshot();
+        });
+
         test('invalid', () => {
           const invalidFnDef = {};
 
@@ -1841,7 +1880,7 @@ describe('custom operations + custom type auth inheritance', () => {
     expect(result).toMatchSnapshot();
     expect(result).toEqual(
       expect.stringContaining(
-        'type QueryReturn @aws_api_key @aws_cognito_user_pools @aws_iam @aws_cognito_user_pools(cognito_groups: ["admin", "superAdmin"])',
+        'type QueryReturn @aws_api_key @aws_cognito_user_pools @aws_iam',
       ),
     );
   });

packages/data-schema/__tests__/CustomType.test.ts

@@ -1,5 +1,6 @@
 import { expectTypeTestsToPassAsync } from 'jest-tsd';
 import { a } from '../src/index';
+import { defineFunctionStub } from './utils';
 
 // evaluates type defs in corresponding test-d.ts file
 it('should not produce static type errors', async () => {
@@ -258,4 +259,48 @@ describe('CustomType transform', () => {
 
     expect(result).toMatchSnapshot();
   });
+
+  test('Implicit CustomType authorized by both group and authorized though its custom operation', () => {
+    const fn1 = defineFunctionStub({});
+    const s = a
+      .schema({
+        getPostDetailsA: a
+          .query()
+          .arguments({content: a.customType({content: a.string()})})
+          .returns(a.customType({}))
+          .handler([
+            a.handler.function(fn1),
+          ]).authorization((allow) => [allow.authenticated(), allow.group("TestGroup")]),
+      });
+
+    const result = s.transform().schema;
+
+    expect(result).toMatchSnapshot();
+  });
+
+  test('Explicit CustomType authorized by both group and authorized though different custom operations', () => {
+    const fn1 = defineFunctionStub({});
+    const s = a
+      .schema({
+        testCustomType: a.customType({ content: a.string() }),
+        getPostDetailsA: a
+          .query()
+          .arguments({})
+          .returns(a.ref("testCustomType"))
+          .handler([
+            a.handler.function(fn1),
+          ]).authorization((allow) => [allow.authenticated()]),
+        getPostDetailsB: a
+          .query()
+          .arguments({})
+          .returns(a.ref("testCustomType"))
+          .handler([
+            a.handler.function(fn1),
+          ]).authorization((allow) => [allow.group("TestGroup")])
+      });
+
+    const result = s.transform().schema;
+
+    expect(result).toMatchSnapshot();
+  });
 });

packages/data-schema/__tests__/ModelType.test.ts

@@ -277,7 +277,7 @@ describe('model auth rules', () => {
 
     expect(graphql).toEqual(
       expect.stringContaining(
-        'type CustomType @aws_api_key @aws_cognito_user_pools @aws_iam @aws_cognito_user_pools(cognito_groups: ["Admin", "User"])',
+        'type CustomType @aws_api_key @aws_cognito_user_pools @aws_iam',
       ),
     );
   });

packages/data-schema/__tests__/__snapshots__/CustomOperations.test.ts.snap

@@ -754,7 +754,7 @@ exports[`CustomOperation transform dynamo schema handlers a.handler.custom a.han
 }
 
 type Query {
-  customQuery: String @aws_cognito_user_pools @aws_cognito_user_pools(cognito_groups: ["groupA", "groupB"])
+  customQuery: String @aws_cognito_user_pools
 }"
 `;
 
@@ -802,6 +802,30 @@ type Query {
 }"
 `;
 
+exports[`CustomOperation transform dynamo schema handlers a.handler.function defineFunction for an async operation that has authorization rules for both group and authenticated 1`] = `
+"type EventInvocationResponse @aws_cognito_user_pools
+{
+  success: Boolean!
+}
+
+type Query {
+  getPostDetails: EventInvocationResponse @function(name: "FnGetPostDetails", invocationType: Event) @auth(rules: [{allow: private},
+  {allow: groups, groups: ["TestGroup"]}])
+}"
+`;
+
+exports[`CustomOperation transform dynamo schema handlers a.handler.function defineFunction for two async operations that have authorization rules for either group or authenticated 1`] = `
+"type EventInvocationResponse @aws_cognito_user_pools
+{
+  success: Boolean!
+}
+
+type Query {
+  getPostDetailsA: EventInvocationResponse @function(name: "FnGetPostDetailsA", invocationType: Event) @auth(rules: [{allow: groups, groups: ["TestGroup"]}])
+  getPostDetailsB: EventInvocationResponse @function(name: "FnGetPostDetailsB", invocationType: Event) @auth(rules: [{allow: private}])
+}"
+`;
+
 exports[`CustomOperation transform dynamo schema handlers a.handler.function defineFunction sync - async 1`] = `
 "type EventInvocationResponse @aws_cognito_user_pools
 {
@@ -970,7 +994,7 @@ type Query {
 `;
 
 exports[`custom operations + custom type auth inheritance op returns top-level custom type with all supported auth modes 1`] = `
-"type QueryReturn @aws_api_key @aws_cognito_user_pools @aws_iam @aws_cognito_user_pools(cognito_groups: ["admin", "superAdmin"])
+"type QueryReturn @aws_api_key @aws_cognito_user_pools @aws_iam
 {
   fieldA: String
   fieldB: Int

packages/data-schema/__tests__/__snapshots__/CustomType.test.ts.snap

@@ -39,6 +39,18 @@ type Location
 }"
 `;
 
+exports[`CustomType transform Explicit CustomType authorized by both group and authorized though different custom operations 1`] = `
+"type testCustomType @aws_cognito_user_pools
+{
+  content: String
+}
+
+type Query {
+  getPostDetailsA: testCustomType @function(name: "FnGetPostDetailsA") @auth(rules: [{allow: private}])
+  getPostDetailsB: testCustomType @function(name: "FnGetPostDetailsB") @auth(rules: [{allow: groups, groups: ["TestGroup"]}])
+}"
+`;
+
 exports[`CustomType transform Explicit CustomType nests explicit CustomType 1`] = `
 "type Post @model @auth(rules: [{allow: public, provider: apiKey}])
 {
@@ -134,6 +146,22 @@ type PostLocation
 }"
 `;
 
+exports[`CustomType transform Implicit CustomType authorized by both group and authorized though its custom operation 1`] = `
+"type GetPostDetailsAReturnType @aws_cognito_user_pools
+{
+  
+}
+
+input GetPostDetailsAContentInput {
+  content: String
+}
+
+type Query {
+  getPostDetailsA(content: GetPostDetailsAContentInput): GetPostDetailsAReturnType @function(name: "FnGetPostDetailsA") @auth(rules: [{allow: private},
+  {allow: groups, groups: ["TestGroup"]}])
+}"
+`;
+
 exports[`CustomType transform Implicit CustomType nests explicit CustomType 1`] = `
 "type Post @model @auth(rules: [{allow: public, provider: apiKey}])
 {

packages/data-schema/__tests__/__snapshots__/ModelType.test.ts.snap

@@ -137,7 +137,7 @@ exports[`model auth rules can create a static Admins group rule 1`] = `
 `;
 
 exports[`model auth rules can deduplicate authorization prevent errors from auth defined by multiple custom operations 1`] = `
-"type CustomType @aws_api_key @aws_cognito_user_pools @aws_iam @aws_cognito_user_pools(cognito_groups: ["Admin", "User"])
+"type CustomType @aws_api_key @aws_cognito_user_pools @aws_iam
 {
   id: String!
   name: String!

packages/data-schema/src/SchemaProcessor.ts

@@ -897,6 +897,7 @@ function mapToNativeAppSyncAuthDirectives(
   const rules = new Set<string>();
 
   const groupProvider: Map<string, Set<string>> = new Map();
+  const generalProviderUsed = new Set<string>();
 
   for (const entry of authorization) {
     const rule = accessData(entry);
@@ -911,17 +912,20 @@ function mapToNativeAppSyncAuthDirectives(
       };
       rule.groups.forEach((group) => groupProvider.get(provider)?.add(group));
     } else {
+      generalProviderUsed.add(provider);
       rules.add(provider);
     }
   }
 
   groupProvider.forEach((groups, provider) => {
-    rules.add(
-      `${provider}(cognito_groups: [${Array.from(groups)
-        .map((group) => `"${group}"`)
-        .join(', ')}])`,
-    );
-    // example: (cognito_groups: ["Bloggers", "Readers"])
+    if(!generalProviderUsed.has(provider)) {
+      rules.add(
+        `${provider}(cognito_groups: [${Array.from(groups).reduce((acc, group) => 
+          acc == "" ? `"${group}"` : `${acc}, "${group}"`
+        , "")}])`
+      );
+      // example: (cognito_groups: ["Bloggers", "Readers"])
+    }
   })
 
   const authString = [...rules].join(' ');