Repository: smithy-lang/smithy
Pull request #2288 (2024-05-15 07:03:21)

Add required for service resolved condition keys
#### Background
* What do these changes do? 
    1.  Adds a `required` property to the definitions provided in `@aws.iam#defineConditionKeys`.
    2. Updates the related documentation.

* Why are they important?
    * They define whether a condition key MUST be filled by the service prior to performing authorization, simplifying authorization code.

#### Testing
* How did you test these changes?
    * Added new test cases.

Based on #2183 

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.



Patch:
diff --git a/docs/source-2.0/additional-specs/protocols/smithy-rpc-v2.rst b/docs/source-2.0/additional-specs/protocols/smithy-rpc-v2.rst
index 46b54ffa2..f4abe4669 100644
--- a/docs/source-2.0/additional-specs/protocols/smithy-rpc-v2.rst
+++ b/docs/source-2.0/additional-specs/protocols/smithy-rpc-v2.rst
@@ -214,10 +214,10 @@ Buffered RPC requests for the ``rpcv2Cbor`` protocol SHOULD include a
 ``Content-Length`` header. Event streaming requests MUST NOT specify a content
 length (instead using ``Transfer-Encoding: chunked`` on HTTP/1.1).
 
-Event streaming requests for the ``rpcv2Cbor`` protocol MUST include an
-``Accept`` header set to the value ``application/vnd.amazon.eventstream``.
-Other forms of content streaming MAY be added in the future, utilizing
-different values for ``Accept``.
+Requests with event stream responses for the ``rpcv2Cbor`` protocol MUST
+include an ``Accept`` header set to the value
+``application/vnd.amazon.eventstream``. Other forms of content streaming MAY be
+added in the future, utilizing different values for ``Accept``.
 
 In summary, the ``rpcv2Cbor`` protocol defines behavior for the following
 headers for requests:
@@ -242,7 +242,8 @@ headers for requests:
         For event streaming requests, this MUST NOT be set.
     * - ``Accept``
       - Conditional
-      - For event streaming requests, to the value ``application/vnd.amazon.eventstream``.
+      - For requests with event streaming responses, this is
+        ``application/vnd.amazon.eventstream``.
 
 
 ~~~~~~~~~
diff --git a/docs/source-2.0/aws/aws-iam.rst b/docs/source-2.0/aws/aws-iam.rst
index 570036b42..ad0c2a8cb 100644
--- a/docs/source-2.0/aws/aws-iam.rst
+++ b/docs/source-2.0/aws/aws-iam.rst
@@ -432,6 +432,11 @@ Each condition key structure supports the following members:
       - ``string``
       - A relative URL path that defines more information about the condition key
         within a set of IAM-related documentation.
+    * - required
+      - ``boolean``
+      - Defines whether a service resolved condition key is required. Not 
+        applicable to request resolved condition keys, as the native 
+        :ref:`required-trait` trait MUST be used on the member directly.
 
 .. code-block:: smithy
 
@@ -448,7 +453,14 @@ Each condition key structure supports the following members:
             type: "String"
             documentation: "The Bar string"
             externalDocumentation: "http://example.com"
-        })
+        },
+        "myservice:Baz": {
+            type: "String"
+            documentation: "The Baz string"
+            externalDocumentation: "http://baz.com"
+            required: true
+        }
+    )
     service MyService {
         version: "2017-02-11"
         resources: [MyResource]
diff --git a/smithy-aws-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeyDefinition.java b/smithy-aws-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeyDefinition.java
index da0b38d61..aa112491a 100644
--- a/smithy-aws-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeyDefinition.java
+++ b/smithy-aws-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeyDefinition.java
@@ -17,6 +17,7 @@ package software.amazon.smithy.aws.iam.traits;
 
 import java.util.Objects;
 import java.util.Optional;
+import software.amazon.smithy.model.node.BooleanNode;
 import software.amazon.smithy.model.node.Node;
 import software.amazon.smithy.model.node.ObjectNode;
 import software.amazon.smithy.model.node.StringNode;
@@ -29,17 +30,20 @@ public final class ConditionKeyDefinition implements ToNode, ToSmithyBuilder<Con
     private static final String DOCUMENTATION = "documentation";
     private static final String EXTERNAL_DOCUMENTATION = "externalDocumentation";
     private static final String RELATIVE_DOCUMENTATION = "relativeDocumentation";
+    private static final String REQUIRED = "required";
 
     private final String type;
     private final String documentation;
     private final String externalDocumentation;
     private final String relativeDocumentation;
+    private final boolean required;
 
     private ConditionKeyDefinition(Builder builder) {
         type = SmithyBuilder.requiredState(TYPE, builder.type);
         documentation = builder.documentation;
         externalDocumentation = builder.externalDocumentation;
         relativeDocumentation = builder.relativeDocumentation;
+        required = builder.required;
     }
 
     public static Builder builder() {
@@ -56,6 +60,8 @@ public final class ConditionKeyDefinition implements ToNode, ToSmithyBuilder<Con
                 .ifPresent(builder::externalDocumentation);
         objectNode.getStringMember(RELATIVE_DOCUMENTATION).map(StringNode::getValue)
                 .ifPresent(builder::relativeDocumentation);
+        objectNode.getBooleanMember(REQUIRED).map(BooleanNode::getValue)
+                .ifPresent(builder::required);
 
         return builder.build();
     }
@@ -91,13 +97,24 @@ public final class ConditionKeyDefinition implements ToNode, ToSmithyBuilder<Con
         return Optional.ofNullable(relativeDocumentation);
     }
 
+    /**
+     * Whether a service resolved condition key is required. Not applicable to request resolved condition keys,
+     * as the native @required trait must be used.
+     *
+     * @return If the service resolved condition key is required.
+     **/
+    public boolean isRequired() {
+        return required;
+    }
+
     @Override
     public SmithyBuilder<ConditionKeyDefinition> toBuilder() {
         return builder()
                 .documentation(documentation)
                 .externalDocumentation(externalDocumentation)
                 .relativeDocumentation(relativeDocumentation)
-                .type(type);
+                .type(type)
+                .required(required);
     }
 
     @Override
@@ -107,6 +124,7 @@ public final class ConditionKeyDefinition implements ToNode, ToSmithyBuilder<Con
                 .withOptionalMember(DOCUMENTATION, getDocumentation().map(Node::from))
                 .withOptionalMember(EXTERNAL_DOCUMENTATION, getExternalDocumentation().map(Node::from))
                 .withOptionalMember(RELATIVE_DOCUMENTATION, getRelativeDocumentation().map(Node::from))
+                .withMember(REQUIRED, isRequired())
                 .build();
     }
 
@@ -122,7 +140,8 @@ public final class ConditionKeyDefinition implements ToNode, ToSmithyBuilder<Con
         return Objects.equals(type, that.type)
                && Objects.equals(documentation, that.documentation)
                && Objects.equals(externalDocumentation, that.externalDocumentation)
-               && Objects.equals(relativeDocumentation, that.relativeDocumentation);
+               && Objects.equals(relativeDocumentation, that.relativeDocumentation)
+               && Objects.equals(required, that.required);
     }
 
     @Override
@@ -135,6 +154,7 @@ public final class ConditionKeyDefinition implements ToNode, ToSmithyBuilder<Con
         private String documentation;
         private String externalDocumentation;
         private String relativeDocumentation;
+        private boolean required;
 
         @Override
         public ConditionKeyDefinition build() {
@@ -160,5 +180,10 @@ public final class ConditionKeyDefinition implements ToNode, ToSmithyBuilder<Con
             this.relativeDocumentation = relativeDocumentation;
             return this;
         }
+
+        public Builder required(boolean required) {
+            this.required = required;
+            return this;
+        }
     }
 }
diff --git a/smithy-aws-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/DefineConditionKeysTraitValidator.java b/smithy-aws-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/DefineConditionKeysTraitValidator.java
new file mode 100644
index 000000000..5d5baa25a
--- /dev/null
+++ b/smithy-aws-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/DefineConditionKeysTraitValidator.java
@@ -0,0 +1,57 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package software.amazon.smithy.aws.iam.traits;
+
+import static java.lang.String.format;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import software.amazon.smithy.model.FromSourceLocation;
+import software.amazon.smithy.model.Model;
+import software.amazon.smithy.model.shapes.ServiceShape;
+import software.amazon.smithy.model.traits.StringListTrait;
+import software.amazon.smithy.model.traits.Trait;
+import software.amazon.smithy.model.validation.AbstractValidator;
+import software.amazon.smithy.model.validation.ValidationEvent;
+import software.amazon.smithy.utils.ListUtils;
+
+/**
+ * Checks properties of the defineConditionKeys IAM trait
+ * 1. @required only for service resolved condition keys. ERROR if used for request resolved condition keys.
+ */
+public final class DefineConditionKeysTraitValidator extends AbstractValidator {
+    @Override
+    public List<ValidationEvent> validate(Model model) {
+        List<ValidationEvent> events = new ArrayList<>();
+        ConditionKeysIndex conditionKeysIndex = ConditionKeysIndex.of(model);
+        for (ServiceShape serviceShape : model.getServiceShapes()) {
+            for (Map.Entry<String, ConditionKeyDefinition> conditionKeyEntry
+                    : conditionKeysIndex.getDefinedConditionKeys(serviceShape).entrySet()
+            ) {
+                // Get all the service resolved condition keys.
+                List<String> serviceResolvedKeys = serviceShape.getTrait(ServiceResolvedConditionKeysTrait.class)
+                        .map(StringListTrait::getValues)
+                        .orElse(ListUtils.of());
+                // We want to emit an event for definitions that state being required...
+                if (conditionKeyEntry.getValue().isRequired()
+                        // and are not configured to be service resolved.
+                        && !serviceResolvedKeys.contains(conditionKeyEntry.getKey())
+                ) {
+                    FromSourceLocation sourceLocation = serviceShape.getTrait(DefineConditionKeysTrait.class)
+                            .map(Trait::getSourceLocation)
+                            .orElse(serviceShape.getSourceLocation());
+                    events.add(error(serviceShape, sourceLocation, format("The `%s` condition key is defined as "
+                            + "required but is resolved from the request. This property is only valid for condition "
+                            + "keys resolved by the service, it MUST also be specified in the "
+                            + "`@serviceResolvedConditionKeys` trait or use the `@required` trait instead.",
+                            conditionKeyEntry.getKey())));
+                }
+            }
+        }
+        return events;
+    }
+}
diff --git a/smithy-aws-iam-traits/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator b/smithy-aws-iam-traits/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator
index 120767a57..702fcbb3f 100644
--- a/smithy-aws-iam-traits/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator
+++ b/smithy-aws-iam-traits/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator
@@ -1,4 +1,5 @@
 software.amazon.smithy.aws.iam.traits.ConditionKeysValidator
 software.amazon.smithy.aws.iam.traits.IamActionValidator
 software.amazon.smithy.aws.iam.traits.IamResourceTraitValidator
+software.amazon.smithy.aws.iam.traits.DefineConditionKeysTraitValidator
 software.amazon.smithy.aws.iam.traits.IamResourceTraitConflictingNameValidator
diff --git a/smithy-aws-iam-traits/src/main/resources/META-INF/smithy/aws.iam.smithy b/smithy-aws-iam-traits/src/main/resources/META-INF/smithy/aws.iam.smithy
index 157028602..144ad78e4 100644
--- a/smithy-aws-iam-traits/src/main/resources/META-INF/smithy/aws.iam.smithy
+++ b/smithy-aws-iam-traits/src/main/resources/META-INF/smithy/aws.iam.smithy
@@ -119,6 +119,10 @@ structure ConditionKeyDefinition {
     /// A relative URL path that defines more information about the condition key
     /// within a set of IAM-related documentation.
     relativeDocumentation: String
+
+    /// Whether a service resolved condition key is required.
+    /// Request resolved condition keys MUST use the @required trait.
+    required: Boolean
 }
 
 /// Contains information about a resource an IAM action can be authorized against.


Test patch:
diff --git a/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/DefineConditionKeysTraitTest.java b/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/DefineConditionKeysTraitTest.java
new file mode 100644
index 000000000..5ab1dab9c
--- /dev/null
+++ b/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/DefineConditionKeysTraitTest.java
@@ -0,0 +1,38 @@
+package software.amazon.smithy.aws.iam.traits;
+
+import org.junit.jupiter.api.Test;
+import software.amazon.smithy.model.Model;
+import software.amazon.smithy.model.shapes.Shape;
+import software.amazon.smithy.model.shapes.ShapeId;
+
+import java.util.Collections;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+
+public class DefineConditionKeysTraitTest {
+    @Test
+    public void loadsFromModel() {
+        Model result = Model.assembler()
+                .discoverModels(getClass().getClassLoader())
+                .addImport(getClass().getResource("define-condition-keys.smithy"))
+                .assemble()
+                .unwrap();
+
+        Shape shape = result.expectShape(ShapeId.from("smithy.example#MyService"));
+        DefineConditionKeysTrait trait = shape.expectTrait(DefineConditionKeysTrait.class);
+        assertEquals(3,trait.getConditionKeys().size());
+        assertFalse(trait.getConditionKey("myservice:Bar").get().isRequired());
+        assertFalse(trait.getConditionKey("myservice:Foo").get().isRequired());
+        assertTrue(trait.getConditionKey("myservice:Baz").get().isRequired());
+    }
+}
diff --git a/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/define-condition-keys.smithy b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/define-condition-keys.smithy
new file mode 100644
index 000000000..e26626550
--- /dev/null
+++ b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/define-condition-keys.smithy
@@ -0,0 +1,32 @@
+$version: "2"
+
+namespace smithy.example
+
+use aws.api#service
+use aws.iam#defineConditionKeys
+use aws.iam#serviceResolvedConditionKeys
+
+@service(sdkId: "My Value", arnNamespace: "myservice")
+@defineConditionKeys(
+    "myservice:Bar": {
+        type: "String"
+        documentation: "The Bar string"
+        externalDocumentation: "http://example.com"
+    },
+    "myservice:Baz": {
+        type: "String"
+        documentation: "The Baz string"
+        externalDocumentation: "http://baz.com"
+        required: true
+    }
+    "myservice:Foo": {
+        type: "String"
+        documentation: "The Foo string"
+        externalDocumentation: "http://foo.com"
+        required: false
+    }
+)
+@serviceResolvedConditionKeys(["myservice:Baz"])
+service MyService {
+    version: "2017-02-11"
+}
diff --git a/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-norequired.errors b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-norequired.errors
new file mode 100644
index 000000000..e69de29bb
diff --git a/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-norequired.smithy b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-norequired.smithy
new file mode 100644
index 000000000..44d0c8497
--- /dev/null
+++ b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-norequired.smithy
@@ -0,0 +1,18 @@
+$version: "2"
+
+namespace smithy.example
+
+use aws.api#service
+use aws.iam#defineConditionKeys
+
+@service(sdkId: "My Value", arnNamespace: "myservice")
+@defineConditionKeys(
+    "myservice:Bar": {
+        type: "String"
+        documentation: "The Bar string"
+        externalDocumentation: "http://example.com"
+    })
+service MyService {
+    version: "2017-02-11"
+}
+
diff --git a/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-requestresolved-required-invalid.errors b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-requestresolved-required-invalid.errors
new file mode 100644
index 000000000..0efc3e1c2
--- /dev/null
+++ b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-requestresolved-required-invalid.errors
@@ -0,0 +1,1 @@
+[ERROR] smithy.example#MyService: The `myservice:Bar` condition key is defined as required but is resolved from the request. This property is only valid for condition keys resolved by the service, it MUST also be specified in the `@serviceResolvedConditionKeys` trait or use the `@required` trait instead. | DefineConditionKeysTrait
diff --git a/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-requestresolved-required-invalid.smithy b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-requestresolved-required-invalid.smithy
new file mode 100644
index 000000000..004c04a16
--- /dev/null
+++ b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-requestresolved-required-invalid.smithy
@@ -0,0 +1,19 @@
+$version: "2"
+
+namespace smithy.example
+
+use aws.api#service
+use aws.iam#defineConditionKeys
+
+@service(sdkId: "My Value", arnNamespace: "myservice")
+@defineConditionKeys(
+    "myservice:Bar": {
+        type: "String"
+        documentation: "The Bar string"
+        externalDocumentation: "http://example.com"
+        required: true
+    })
+service MyService {
+    version: "2017-02-11"
+}
+
diff --git a/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-required.errors b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-required.errors
new file mode 100644
index 000000000..e69de29bb
diff --git a/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-required.smithy b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-required.smithy
new file mode 100644
index 000000000..a7ba99d00
--- /dev/null
+++ b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-required.smithy
@@ -0,0 +1,21 @@
+$version: "2"
+
+namespace smithy.example
+
+use aws.api#service
+use aws.iam#defineConditionKeys
+use aws.iam#serviceResolvedConditionKeys
+
+@service(sdkId: "My Value", arnNamespace: "myservice")
+@defineConditionKeys(
+    "myservice:Bar": {
+        type: "String"
+        documentation: "The Bar string"
+        externalDocumentation: "http://example.com"
+        required: true
+    })
+@serviceResolvedConditionKeys(["myservice:Bar"])
+service MyService {
+    version: "2017-02-11"
+}
+