Adding conditionKeyValue and conditionKeysResolvedByService traits by 0xjjoyy · Pull Request #1677 · smithy-lang/smithy

0xjjoyy · 2023-03-14T03:35:52Z

Issue #, if available:

Description of changes:
Adding conditionKeyValue and conditionKeysResolvedByService traits

conditionKeyValue
Uses the associated member’s value as this condition key’s value. Needed when the member name doesn't match the condition key name.

conditionKeysResolvedByService
Specifies the list of IAM condition keys which must be resolved by the service, as opposed to being pulled from the request.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

kstich · 2023-03-29T04:45:37Z

docs/source-2.0/aws/aws-iam.rst

+Value type
+    ``string``
+
+Specifies the list of IAM condition keys which must be resolved by the


Redundant with the Summary, can remove.

removed redundant text

kstich · 2023-03-29T04:47:42Z

docs/source-2.0/aws/aws-iam.rst

+Specifies the list of IAM condition keys which must be resolved by the
+service, as opposed to being pulled from the request.
+
+The following example defines two operations:


How does this interact with derived condition keys?

added
Derived resource condition keys MUST NOT be included
with the conditionKeysResolvedByService trait.

kstich · 2023-03-29T04:48:14Z

docs/source-2.0/aws/aws-iam.rst

+Trait selector
+    ``service``
+Value type
+    ``string``


This is a list<string>

updated to be a list

kstich · 2023-03-29T04:49:41Z

docs/source-2.0/aws/aws-iam.rst

+Value type
+    ``string``
+
+Uses the associated member’s value as this condition key’s value. Needed when


Should mimic the language for actionName.

updated, also added that MUST also be defined via the :ref:aws.iam#defineConditionKeys-trait trait.

kstich · 2023-03-29T04:49:56Z

docs/source-2.0/aws/aws-iam.rst

+
+The following example defines two operations:
+
+``myservice:ActionContextKey1`` is an service-specific IAM action


s/is an/is a

updated text

...s-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeysValidator.java

kstich · 2023-03-29T05:05:18Z

...s-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeysValidator.java

+                                String conditionKey = trait.getValue();
+                                if (!knownKeys.contains(conditionKey)) {
+                                    results.add(error(operation, String.format(
+                                            "This operation scoped within the `%s` service refers to an undefined "


This error message doesn't correctly map to the trait being validated.

Should also pass the trait in as the second parameter to error() for source location clarity.

added operation id and member id to the error message details

ah cool, didn't realize there was sourcelocation on the trait. Added.

added errorfiles test case

kstich · 2023-03-29T05:07:35Z

...s-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/ConditionKeysIndexTest.java

A lot of content changed in this file that wasn't actually the specific test additions.

updated this file and applied only the new tests.

0xjjoyy · 2023-04-07T12:58:58Z

I've updated the pull request based on feedback.

kstich · 2023-04-18T18:18:30Z

...m-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/condition-key-value.smithy

+}
+
+
+@aws.iam#actionName("overridingActionName")


minor: the Echo operation isn't needed to express the test here, can remove

Shapes and traits cleaned up.

kstich · 2023-04-18T18:20:47Z

...m-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/condition-key-value.smithy

+    "smithy:ActionContextKey3": { type: "String" },
+    "smithy:requesterId": { type: "String" }
+)
+@aws.iam#conditionKeysResolvedByService(["smithy:requesterId"])


minor: the conditionKeysResolvedByService trait isn't needed to express the test here, can remove

kstich · 2023-04-18T18:21:00Z

...m-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/condition-key-value.smithy

+$version: "2.0"
+namespace smithy.example
+
+use aws.iam#conditionKeyValue


The trait is applied with the namespace, meaning this use statement is unused

kstich · 2023-04-18T18:23:09Z

...esources/software/amazon/smithy/aws/iam/traits/errorfiles/invalid-condition-key-value.errors

Should have newline at EOF

kstich · 2023-04-18T18:23:13Z

...tware/amazon/smithy/aws/iam/traits/errorfiles/invalid-condition-keys-service-resolved.errors

Should have newline at EOF

kstich · 2023-04-18T18:38:44Z

...s-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeysValidator.java

+                                ConditionKeyValueTrait trait = memberShape.expectTrait(ConditionKeyValueTrait.class);
+                                String conditionKey = trait.getValue();
+                                if (!knownKeys.contains(conditionKey)) {
+                                    results.add(error(memberShape, trait.getSourceLocation(), String.format(


trait doesn't need the getSourceLocation() call applied, it's handled by error.

kstich · 2023-04-18T18:39:20Z

...s-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeysValidator.java

+                            }
+                        }
+                        if (!invalidNames.isEmpty()) {
+                            results.add(error(service, trait.getSourceLocation(), String.format(


trait doesn't need the getSourceLocation() call applied, it's handled by error.

...s-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeysValidator.java

kstich · 2023-04-18T19:36:16Z

docs/source-2.0/aws/aws-iam.rst

+.. _aws.iam#conditionKeysResolvedByService-trait:
+
+------------------------------------------------
+``aws.iam#conditionKeysResolvedByService`` trait


What about serviceResolvedConditionKeys for the name of this trait? It's a clearer read to me and aligns more with the other IAM traits. Would need updating in several places (docs, code (ID, class names, etc.), test files.)

0xjjoyy

addressed feedback
adding validation to check condition key value doesn't intersect with service resolved keys

kstich

A lot/all of the test case files could also use empty lines at EOF.

docs/source-2.0/aws/aws-iam.rst

...src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeysResolvedByServiceTrait.java

...test/java/software/amazon/smithy/aws/iam/traits/ConditionKeysResolvedByServiceTraitTest.java

kstich · 2023-07-18T21:52:33Z

...e/amazon/smithy/aws/iam/traits/errorfiles/invalid-condition-key-also-service-resolved.smithy

+@aws.iam#defineConditionKeys(
+    "smithy:ServiceResolveContextKey": { type: "String" }
+)
+@aws.iam#conditionKeysResolvedByService(["smithy:ServiceResolveContextKey"])


Will need to be updated with the trait rename.

thanks, updated

kstich · 2023-07-18T21:52:56Z

...e/amazon/smithy/aws/iam/traits/errorfiles/invalid-condition-key-also-service-resolved.smithy

@@ -0,0 +1,23 @@
+$version: "2.0"
+namespace smithy.example


nit: should have a new line between $version and `namespace

done, updated

...s-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeysValidator.java

docs/source-2.0/aws/aws-iam.rst

0xjjoyy · 2023-07-26T18:51:04Z

Updated based on feedback. Updated trait and class names to match new name. Added EOF to *.smithy and *.errors

kstich · 2023-08-08T17:38:56Z

docs/source-2.0/aws/aws-iam.rst

+        "myservice:ActionContextKey1": { type: "String" },
+        "myservice:ActionContextKey2": { type: "String" }
+    )
+    @conditionKeyResolvers(["myservice:ActionContextKey1"])


Suggested change

@conditionKeyResolvers(["myservice:ActionContextKey1"])

@serviceResolvedConditionKeys(["myservice:ActionContextKey1"])

Thanks, updated

kstich · 2023-08-17T04:23:29Z

Appears to need a rebase before merging, but content looks good.

) * Adding conditionKeyValue and serviceResolvedConditionKeys traits * Update trait constructors to be public

0xjjoyy requested a review from a team as a code owner March 14, 2023 03:35

cmoher assigned kstich Mar 20, 2023

kstich requested changes Mar 29, 2023

View reviewed changes

kstich requested changes Apr 18, 2023

View reviewed changes

0xjjoyy commented Jun 5, 2023

View reviewed changes

0xjjoyy requested a review from kstich June 6, 2023 03:50

kstich requested changes Jul 18, 2023

View reviewed changes

0xjjoyy requested a review from a team as a code owner July 26, 2023 04:45

0xjjoyy requested a review from kstich July 26, 2023 18:54

kstich reviewed Aug 8, 2023

View reviewed changes

0xjjoyy requested a review from kstich August 9, 2023 03:54

kstich approved these changes Aug 17, 2023

View reviewed changes

0xjjoyy force-pushed the conditionkeys branch from b7d6b57 to 4b5c6aa Compare August 17, 2023 07:02

Adding conditionKeyValue and serviceResolvedConditionKeys traits

99705e8

0xjjoyy force-pushed the conditionkeys branch from 4b5c6aa to 99705e8 Compare August 17, 2023 07:04

Trait constructors to be public

c09a286

0xjjoyy requested a review from kstich August 18, 2023 17:32

kstich approved these changes Aug 18, 2023

View reviewed changes

kstich merged commit b5b708a into smithy-lang:main Aug 18, 2023

alextwoods pushed a commit to alextwoods/smithy that referenced this pull request Sep 15, 2023

Add conditionKeyValue and serviceResolvedConditionKeys (smithy-lang#1677

9894b2c

) * Adding conditionKeyValue and serviceResolvedConditionKeys traits * Update trait constructors to be public


		The following example defines two operations:

		``myservice:ActionContextKey1`` is an service-specific IAM action

	@conditionKeyResolvers(["myservice:ActionContextKey1"])
	@serviceResolvedConditionKeys(["myservice:ActionContextKey1"])

		}


		@aws.iam#actionName("overridingActionName")

Conversation

0xjjoyy commented Mar 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xjjoyy commented Apr 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xjjoyy left a comment

Choose a reason for hiding this comment

Uh oh!

kstich left a comment

Choose a reason for hiding this comment

Uh oh!