Use sets for OpenAPI security settings

kstich · kstich · commit dc9eb9fa89b3 · 2021-01-12T08:54:42.000-08:00
This commit updates the OpenAPI document and Operation object builders
to use sets for storing their security settings instead of lists. Each
entry in these collections can be safely made unique, as duplicates would
represent an "A or A" posture. Having multiple entries can cause issues
with consuming the output document.
diff --git a/smithy-openapi/src/main/java/software/amazon/smithy/openapi/model/OpenApi.java b/smithy-openapi/src/main/java/software/amazon/smithy/openapi/model/OpenApi.java
@@ -18,9 +18,11 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Comparator;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.TreeMap;
 import software.amazon.smithy.model.node.ArrayNode;
 import software.amazon.smithy.model.node.Node;
@@ -143,7 +145,9 @@ public static final class Builder extends Component.Builder<Builder, OpenApi> {
         private final List<ServerObject> servers = new ArrayList<>();
         private Map<String, PathItem> paths = new TreeMap<>();
         private ComponentsObject components;
-        private final List<Map<String, List<String>>> security = new ArrayList<>();
+        // Use a set for security as duplicate entries are unnecessary (effectively
+        // represent an "A or A" security posture) and can cause downstream issues.
+        private final Set<Map<String, List<String>>> security = new LinkedHashSet<>();
         private final List<TagObject> tags = new ArrayList<>();
         private ExternalDocumentation externalDocs;
 
diff --git a/smithy-openapi/src/main/java/software/amazon/smithy/openapi/model/OperationObject.java b/smithy-openapi/src/main/java/software/amazon/smithy/openapi/model/OperationObject.java
@@ -19,9 +19,11 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.TreeMap;
 import software.amazon.smithy.model.node.ArrayNode;
 import software.amazon.smithy.model.node.Node;
@@ -189,7 +191,9 @@ public static final class Builder extends Component.Builder<Builder, OperationOb
         private final List<ParameterObject> parameters = new ArrayList<>();
         private final Map<String, ResponseObject> responses = new TreeMap<>();
         private final Map<String, CallbackObject> callbacks = new TreeMap<>();
-        private List<Map<String, List<String>>> security;
+        // Use a set for security as duplicate entries are unnecessary (effectively
+        // represent an "A or A" security posture) and can cause downstream issues.
+        private Set<Map<String, List<String>>> security;
         private final List<ServerObject> servers = new ArrayList<>();
         private String summary;
         private String description;
@@ -280,14 +284,14 @@ public Builder deprecated(boolean deprecated) {
         }
 
         public Builder security(Collection<Map<String, List<String>>> security) {
-            this.security = new ArrayList<>();
+            this.security = new LinkedHashSet<>();
             this.security.addAll(security);
             return this;
         }
 
         public Builder addSecurity(Map<String, List<String>> security) {
             if (this.security == null) {
-                this.security = new ArrayList<>();
+                this.security = new LinkedHashSet<>();
             }
             this.security.add(security);
             return this;
diff --git a/smithy-openapi/src/test/java/software/amazon/smithy/openapi/fromsmithy/OpenApiConverterTest.java b/smithy-openapi/src/test/java/software/amazon/smithy/openapi/fromsmithy/OpenApiConverterTest.java
@@ -21,6 +21,7 @@
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.not;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 
 import java.util.Collections;
 import java.util.List;
@@ -254,14 +255,14 @@ public void protocolsCanOmitOperations() {
                 .convert(model);
 
         for (PathItem pathItem : result.getPaths().values()) {
-            Assertions.assertFalse(pathItem.getGet().isPresent());
-            Assertions.assertFalse(pathItem.getHead().isPresent());
-            Assertions.assertFalse(pathItem.getDelete().isPresent());
-            Assertions.assertFalse(pathItem.getPatch().isPresent());
-            Assertions.assertFalse(pathItem.getPost().isPresent());
-            Assertions.assertFalse(pathItem.getPut().isPresent());
-            Assertions.assertFalse(pathItem.getTrace().isPresent());
-            Assertions.assertFalse(pathItem.getOptions().isPresent());
+            assertFalse(pathItem.getGet().isPresent());
+            assertFalse(pathItem.getHead().isPresent());
+            assertFalse(pathItem.getDelete().isPresent());
+            assertFalse(pathItem.getPatch().isPresent());
+            assertFalse(pathItem.getPost().isPresent());
+            assertFalse(pathItem.getPut().isPresent());
+            assertFalse(pathItem.getTrace().isPresent());
+            assertFalse(pathItem.getOptions().isPresent());
         }
     }
 
@@ -353,6 +354,34 @@ public void canChangeSecurityRequirementName() {
         assertThat(result.getPaths().get("/2").getGet().get().getSecurity().get().get(0).keySet(), contains("foo_baz"));
     }
 
+    @Test
+    public void consolidatesSameSecurityRequirements() {
+        // This service model has multiple auth types throughout it for both
+        // the top level and operation level security setting. Validate that,
+        // after they're set to use the same name, they're consolidated for
+        // being the same.
+        Model model = Model.assembler()
+                .addImport(getClass().getResource("consolidates-security-service.json"))
+                .discoverModels()
+                .assemble()
+                .unwrap();
+        OpenApiConfig config = new OpenApiConfig();
+        config.setService(ShapeId.from("smithy.example#Service"));
+        OpenApi result = OpenApiConverter.create()
+                .addOpenApiMapper(new ConstantSecurity())
+                .config(config)
+                .convert(model);
+
+        assertThat(result.getSecurity().size(), equalTo(1));
+        assertThat(result.getSecurity().get(0).keySet(), contains("foo_baz"));
+        // This security matches the service, so isn't applied.
+        assertFalse(result.getPaths().get("/1").getGet().get().getSecurity().isPresent());
+        assertThat(result.getPaths().get("/2").getGet().get().getSecurity().get().size(), equalTo(1));
+        assertThat(result.getPaths().get("/2").getGet().get().getSecurity().get().get(0).keySet(), contains("foo_baz"));
+        assertThat(result.getPaths().get("/3").getGet().get().getSecurity().get().size(), equalTo(1));
+        assertThat(result.getPaths().get("/3").getGet().get().getSecurity().get().get(0).keySet(), contains("foo_baz"));
+    }
+
     @Test
     public void mergesInSchemaDocumentExtensions() {
         OpenApiConfig config = new OpenApiConfig();
diff --git a/smithy-openapi/src/test/resources/software/amazon/smithy/openapi/fromsmithy/consolidates-security-service.json b/smithy-openapi/src/test/resources/software/amazon/smithy/openapi/fromsmithy/consolidates-security-service.json
@@ -0,0 +1,79 @@
+{
+    "smithy": "1.0",
+    "shapes": {
+        "smithy.example#Service": {
+            "type": "service",
+            "version": "2006-03-01",
+            "operations": [
+                {
+                    "target": "smithy.example#Operation1"
+                },
+                {
+                    "target": "smithy.example#Operation2"
+                },
+                {
+                    "target": "smithy.example#Operation3"
+                },
+                {
+                    "target": "smithy.example#UnauthenticatedOperation"
+                }
+            ],
+            "traits": {
+                "aws.protocols#restJson1": {},
+                "aws.auth#sigv4": {
+                    "name": "example"
+                },
+                "smithy.api#httpBasicAuth": {},
+                "smithy.api#httpDigestAuth": {},
+                "smithy.api#auth": [
+                    "aws.auth#sigv4",
+                    "smithy.api#httpDigestAuth"
+                ]
+            }
+        },
+        "smithy.example#Operation1": {
+            "type": "operation",
+            "traits": {
+                "smithy.api#http": {
+                    "uri": "/1",
+                    "method": "GET"
+                }
+            }
+        },
+        "smithy.example#Operation2": {
+            "type": "operation",
+            "traits": {
+                "smithy.api#http": {
+                    "uri": "/2",
+                    "method": "GET"
+                },
+                "smithy.api#auth": [
+                    "smithy.api#httpBasicAuth"
+                ]
+            }
+        },
+        "smithy.example#Operation3": {
+            "type": "operation",
+            "traits": {
+                "smithy.api#http": {
+                    "uri": "/3",
+                    "method": "GET"
+                },
+                "smithy.api#auth": [
+                    "smithy.api#httpBasicAuth",
+                    "aws.auth#sigv4"
+                ]
+            }
+        },
+        "smithy.example#UnauthenticatedOperation": {
+            "type": "operation",
+            "traits": {
+                "smithy.api#http": {
+                    "uri": "/4",
+                    "method": "GET"
+                },
+                "smithy.api#auth": []
+            }
+        }
+    }
+}
