Check for sensitive trait in Shape members (#181)

Author: trivikr

smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/StructuredMemberWriter.java

@@ -73,7 +73,7 @@ void writeMembers(TypeScriptWriter writer, Shape shape) {
     void writeFilterSensitiveLog(TypeScriptWriter writer, String objectParam) {
         writer.write("...$L,", objectParam);
         for (MemberShape member : members) {
-            if (isMemberOverwriteRequired(member)) {
+            if (isMemberOverwriteRequired(member, new HashSet<String>())) {
                 Shape memberTarget = model.expectShape(member.getTarget());
                 String memberName = getSanitizedMemberName(member);
                 String memberParam = String.format("%s.%s", objectParam, memberName);
@@ -139,7 +139,7 @@ private void writeCollectionFilterSensitiveLog(
                 MemberShape mapMember = ((MapShape) collectionMemberTarget).getValue();
                 writeMapFilterSensitiveLog(writer, mapMember, itemParam);
             } else {
-                // This path should not reach because of recursive isIterationRequired.
+                // This path should not reach because of recursive isMemberOverwriteRequired.
                 throw new CodegenException(String.format(
                     "CollectionFilterSensitiveLog attempted for %s while it was not required",
                     collectionMemberTarget.getType()
@@ -180,7 +180,7 @@ private void writeMapFilterSensitiveLog(TypeScriptWriter writer, MemberShape map
                         MemberShape nestedMapMember = ((MapShape) mapMemberTarget).getValue();
                         writeMapFilterSensitiveLog(writer, nestedMapMember, valueParam);
                     } else {
-                        // This path should not reach because of recursive isIterationRequired.
+                        // This path should not reach because of recursive isMemberOverwriteRequired.
                         throw new CodegenException(String.format(
                             "MapFilterSensitiveLog attempted for %s while it was not required",
                             mapMemberTarget.getType()
@@ -196,37 +196,41 @@ private void writeMapFilterSensitiveLog(TypeScriptWriter writer, MemberShape map
     }
 
     /**
-     * Identifies if iteration is required on member.
+     * Identifies if member needs to be overwritten in filterSensitiveLog.
      *
-     * @param member a {@link MemberShape} to check for iteration required.
-     * @return Returns true if the iteration is required on member.
+     * @param member a {@link MemberShape} to check if overwrite is required.
+     * @param parents a set of membernames which are parents of existing member to avoid unending recursion.
+     * @return Returns true if the overwrite is required on member.
      */
-    private boolean isIterationRequired(MemberShape member) {
+    private boolean isMemberOverwriteRequired(MemberShape member, Set<String> parents) {
+        if (member.getMemberTrait(model, SensitiveTrait.class).isPresent()) {
+            return true;
+        }
+
         Shape memberTarget = model.expectShape(member.getTarget());
+        parents.add(symbolProvider.toMemberName(member));
         if (memberTarget instanceof StructureShape) {
-            return true;
+            Collection<MemberShape> structureMemberList = ((StructureShape) memberTarget).getAllMembers().values();
+            for (MemberShape structureMember: structureMemberList) {
+                if (!parents.contains(symbolProvider.toMemberName(structureMember))
+                        && isMemberOverwriteRequired(structureMember, parents)) {
+                    return true;
+                }
+            }
         } else if (memberTarget instanceof CollectionShape) {
             MemberShape collectionMember = ((CollectionShape) memberTarget).getMember();
-            return isIterationRequired(collectionMember);
+            if (!parents.contains(symbolProvider.toMemberName(collectionMember))) {
+                return isMemberOverwriteRequired(collectionMember, parents);
+            }
         } else if (memberTarget instanceof MapShape) {
             MemberShape mapMember = ((MapShape) memberTarget).getValue();
-            return isIterationRequired(mapMember);
+            if (!parents.contains(symbolProvider.toMemberName(mapMember))) {
+                return isMemberOverwriteRequired(mapMember, parents);
+            }
         }
         return false;
     }
 
-    /**
-     * Identifies if member needs to be overwritten in filterSensitiveLog.
-     *
-     * @param member a {@link MemberShape} to check if overwrite is required.
-     * @return Returns true if the overwrite is required on member.
-     */
-    private boolean isMemberOverwriteRequired(MemberShape member) {
-        return (
-            member.getMemberTrait(model, SensitiveTrait.class).isPresent() || isIterationRequired(member)
-        );
-    }
-
     /**
      * Returns the member name to be used in generation.
      *

smithy-typescript-codegen/src/test/java/software/amazon/smithy/typescript/codegen/StructureGeneratorTest.java

@@ -54,6 +54,14 @@ public void filtersSensitiveSimpleShape() {
                                 + "  })\n");
     }
 
+    @Test
+    public void skipsFilterForInsensitiveSimpleShape() {
+        testStructureCodegen("test-insensitive-simple-shape.smithy",
+                                "  export const filterSensitiveLog = (obj: GetFooInput): any => ({\n"
+                                + "    ...obj,\n"
+                                + "  })\n");
+    }
+
     @Test
     public void callsFilterForStructureWithSensitiveData() {
         testStructureCodegen("test-structure-with-sensitive-data.smithy",
@@ -87,6 +95,14 @@ public void filtersSensitiveStructure() {
                                 + "  })\n");
     }
 
+    @Test
+    public void skipsFilterForInsensitiveStructure() {
+        testStructureCodegen("test-insensitive-structure.smithy",
+                                "  export const filterSensitiveLog = (obj: GetFooInput): any => ({\n"
+                                + "    ...obj,\n"
+                                + "  })\n");
+    }
+
     @Test
     public void filtersSensitiveMemberPointingToStructure() {
         testStructureCodegen("test-sensitive-member-pointing-to-structure.smithy",
@@ -123,6 +139,17 @@ public void callsFilterInListWithSensitiveData() {
                                 + "  })\n");
     }
 
+    @Test
+    public void callsFilterForListWithSensitiveMember() {
+        testStructureCodegen("test-list-with-sensitive-member.smithy",
+                                "  export const filterSensitiveLog = (obj: GetFooInput): any => ({\n"
+                                + "    ...obj,\n"
+                                + "    ...(obj.foo && { foo:\n"
+                                + "      SENSITIVE_STRING\n"
+                                + "    }),\n"
+                                + "  })\n");
+    }
+
     @Test
     public void filtersSensitiveList() {
         testStructureCodegen("test-sensitive-list.smithy",
@@ -134,6 +161,14 @@ public void filtersSensitiveList() {
                                 + "  })\n");
     }
 
+    @Test
+    public void skipsFilterForInsensitiveList() {
+        testStructureCodegen("test-insensitive-list.smithy",
+                                "  export const filterSensitiveLog = (obj: GetFooInput): any => ({\n"
+                                + "    ...obj,\n"
+                                + "  })\n");
+    }
+
     @Test
     public void filtersSensitiveMemberPointingToList() {
         testStructureCodegen("test-sensitive-member-pointing-to-list.smithy",
@@ -172,6 +207,17 @@ public void callsFilterInMapWithSensitiveData() {
                             + "  })\n");
     }
 
+    @Test
+    public void callsFilterForMapWithSensitiveMember() {
+        testStructureCodegen("test-map-with-sensitive-member.smithy",
+                                "  export const filterSensitiveLog = (obj: GetFooInput): any => ({\n"
+                                + "    ...obj,\n"
+                                + "    ...(obj.foo && { foo:\n"
+                                + "      SENSITIVE_STRING\n"
+                                + "    }),\n"
+                                + "  })\n");
+    }
+
     @Test
     public void filtersSensitiveMap() {
         testStructureCodegen("test-sensitive-map.smithy",
@@ -194,6 +240,22 @@ public void filtersSensitiveMemberPointingToMap() {
                             + "  })\n");
     }
 
+    @Test
+    public void skipsFilterForInsensitiveMap() {
+        testStructureCodegen("test-insensitive-map.smithy",
+                                "  export const filterSensitiveLog = (obj: GetFooInput): any => ({\n"
+                                + "    ...obj,\n"
+                                + "  })\n");
+    }
+
+    @Test
+    public void skipsFilterOnEncounteringRecursiveShapes() {
+        testStructureCodegen("test-recursive-shapes.smithy",
+                                "  export const filterSensitiveLog = (obj: GetFooInput): any => ({\n"
+                                + "    ...obj,\n"
+                                + "  })\n");
+    }
+
     private String testStructureCodegen(String file, String expectedType) {
         Model model = Model.assembler()
                 .addImport(getClass().getResource(file))

smithy-typescript-codegen/src/test/resources/software/amazon/smithy/typescript/codegen/test-insensitive-list.smithy

@@ -0,0 +1,17 @@
+namespace smithy.example
+
+@protocols([{name: "aws.rest-json-1.1"}])
+service Example {
+    version: "1.0.0",
+    operations: [GetFoo]
+}
+
+operation GetFoo(GetFooInput)
+
+structure GetFooInput {
+    foo: NamesList
+}
+
+list NamesList {
+    member: String
+}

smithy-typescript-codegen/src/test/resources/software/amazon/smithy/typescript/codegen/test-insensitive-map.smithy

@@ -0,0 +1,18 @@
+namespace smithy.example
+
+@protocols([{name: "aws.rest-json-1.1"}])
+service Example {
+    version: "1.0.0",
+    operations: [GetFoo]
+}
+
+operation GetFoo(GetFooInput)
+
+structure GetFooInput {
+    foo: NamesMap
+}
+
+map NamesMap {
+    key: String,
+    value: String
+}

smithy-typescript-codegen/src/test/resources/software/amazon/smithy/typescript/codegen/test-insensitive-simple-shape.smithy

@@ -0,0 +1,14 @@
+namespace smithy.example
+
+@protocols([{name: "aws.rest-json-1.1"}])
+service Example {
+    version: "1.0.0",
+    operations: [GetFoo]
+}
+
+operation GetFoo(GetFooInput)
+
+structure GetFooInput {
+    firstname: String,
+    lastname: String
+}

smithy-typescript-codegen/src/test/resources/software/amazon/smithy/typescript/codegen/test-insensitive-structure.smithy

@@ -0,0 +1,18 @@
+namespace smithy.example
+
+@protocols([{name: "aws.rest-json-1.1"}])
+service Example {
+    version: "1.0.0",
+    operations: [GetFoo]
+}
+
+operation GetFoo(GetFooInput)
+
+structure GetFooInput {
+    foo: User
+}
+
+structure User {
+    firstname: String,
+    lastname: String
+}

smithy-typescript-codegen/src/test/resources/software/amazon/smithy/typescript/codegen/test-list-with-sensitive-member.smithy

@@ -0,0 +1,18 @@
+namespace smithy.example
+
+@protocols([{name: "aws.rest-json-1.1"}])
+service Example {
+    version: "1.0.0",
+    operations: [GetFoo]
+}
+
+operation GetFoo(GetFooInput)
+
+structure GetFooInput {
+    foo: PhoneNumbersList
+}
+
+list PhoneNumbersList {
+    @sensitive
+    member: String
+}

smithy-typescript-codegen/src/test/resources/software/amazon/smithy/typescript/codegen/test-map-with-sensitive-member.smithy

@@ -0,0 +1,20 @@
+namespace smithy.example
+
+@protocols([{name: "aws.rest-json-1.1"}])
+service Example {
+    version: "1.0.0",
+    operations: [GetFoo]
+}
+
+operation GetFoo(GetFooInput)
+
+structure GetFooInput {
+    foo: PhoneNumbersMap
+}
+
+map PhoneNumbersMap {
+    key: String,
+    
+    @sensitive
+    value: String
+}

smithy-typescript-codegen/src/test/resources/software/amazon/smithy/typescript/codegen/test-recursive-shapes.smithy

@@ -0,0 +1,28 @@
+namespace smithy.example
+
+@protocols([{name: "aws.rest-json-1.1"}])
+service Example {
+    version: "1.0.0",
+    operations: [GetFoo]
+}
+
+operation GetFoo(GetFooInput)
+
+structure GetFooInput {
+    foo: User
+}
+
+structure User {
+    recursiveUser: User,
+    recursiveList: UsersList,
+    recursiveMap: UsersMap
+}
+
+list UsersList {
+    member: User
+}
+
+map UsersMap {
+    key: String,
+    value: User
+}