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 index 07bb8c5af41..ddd643a13f2 100644 --- 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 { private final List servers = new ArrayList<>(); private Map paths = new TreeMap<>(); private ComponentsObject components; - private final List>> 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>> security = new LinkedHashSet<>(); private final List 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 index 140a4a4b51e..1f35456edb1 100644 --- 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 parameters = new ArrayList<>(); private final Map responses = new TreeMap<>(); private final Map callbacks = new TreeMap<>(); - private List>> 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>> security; private final List servers = new ArrayList<>(); private String summary; private String description; @@ -280,14 +284,14 @@ public Builder deprecated(boolean deprecated) { } public Builder security(Collection>> security) { - this.security = new ArrayList<>(); + this.security = new LinkedHashSet<>(); this.security.addAll(security); return this; } public Builder addSecurity(Map> 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 index 9075a3a7a0f..668f3b9c3df 100644 --- 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 new file mode 100644 index 00000000000..0cf36c9577e --- /dev/null +++ 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": [] + } + } + } +}