feat(iam): add arbitrary conditions to existing principals #7015

zakwalters · 2020-03-25T21:03:40Z

Commit Message

Adds a PrincipalWithConditions wrapper that allows conditions to be added to
any principal.

Behaviour is consistent with the way that PolicyStatement.addCondition
and .addConditions currently work - most notably in that adding an operator
that is already present will merge their objects, but adding a condition to
an operator/key combination that is already set will overwrite the existing
value (rather than merge the values into an array).

BREAKING CHANGES: every place an IAM Condition was expected it is now typed as {[key: string]: any}, instead of plain any. You were always supposed to pass a map/dictionary in these locations, but the type system didn't enforce it. It now does. This will not impact correct programs, but may cause compiler errors in programs that were incorrect.

End Commit Message

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

- Test that conditions are merged with those from `PolicyDocument.addCondition`. - Test that the original principal is unmodified after using `principal.withConditions` (this test is more for invariance - the behaviour may be changed to add conditions to the underlying principal).

In the case of both conflicting keys and conflicting operators, the behaviour is inherited from the way that `PolicyStatement.addCondition` already works. When the same operator is specified in two different conditions, the objects will be merged. When the same key is specified under the same operator in two different conditions, the one added later will overwrite the existing one.

Moves the Conditions type into policy-statement.ts as it makes more sense there than under principals.ts. Changes the other functions that work with conditions to use this type rather than the more permissive `{ [key: string]: any }` which could cause failures in CloudFormation. We cannot model the Conditions type more strongly (e.g. with an object that includes all valid operators as optional keys) because of errors thrown by JSII.

`addCondition` and `addConditions` are added to the `PrincipalWithConditions` class to make its interface more consistent with what users are used to from the `PolicyStatement` class.

mergify · 2020-03-25T21:04:13Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

aws-cdk-automation · 2020-03-25T21:27:57Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: ea625aa
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-03-25T21:46:23Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 05a5997
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-03-25T21:46:47Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 9a32cd6
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-03-26T12:54:08Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 748f275
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr

Thanks for this! Would prefer some slight changes but I like the direction this is going!

rix0rrr · 2020-03-30T14:09:21Z

packages/@aws-cdk/aws-iam/lib/policy-statement.ts

@@ -200,15 +200,15 @@ export class PolicyStatement {
  /**
   * Add a condition to the Policy
   */
-  public addCondition(key: string, value: any) {
+  public addCondition(key: string, value: { [key: string]: string | string[] }) {


Unfortunately we can't retype this this accurately, because we can't support unions, so retype string | string[] as any.

We should probably have the following type aliases, no?

export type Condition = Record<string, any>; export type Conditions = Record<string, Condition>;

Then this whole argument could be typed as Condition.

Sounds good, thanks for the pointer.

rix0rrr · 2020-03-30T14:09:59Z

packages/@aws-cdk/aws-iam/lib/policy-statement.ts

+ * See https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html
+ */
+export interface Conditions {
+  [operator: string]: { [key: string]: string | string[] };


No union type as mentioned.

rix0rrr · 2020-03-30T14:10:38Z

packages/@aws-cdk/aws-iam/lib/principals.ts

+  public readonly assumeRoleAction: string = 'sts:AssumeRole';
+
+  constructor(
+    public readonly principal: PrincipalType,


I would be fine not exposing this.

Sure, can't say I have a strong argument for it so happy to make it private

rix0rrr · 2020-03-30T14:10:57Z

packages/@aws-cdk/aws-iam/lib/principals.ts

+export class PrincipalWithConditions<PrincipalType extends PrincipalBase> implements IPrincipal {
+  public readonly conditions: Conditions;
+  public readonly grantPrincipal: IPrincipal = this;
+  public readonly assumeRoleAction: string = 'sts:AssumeRole';


I think this needs to be this.principal.assumeRoleAction?

Totally right, thanks

rix0rrr · 2020-03-30T14:12:37Z

packages/@aws-cdk/aws-iam/lib/principals.ts

+    conditions: Conditions,
+  ) {
+    // Copy the intiial conditions from the principal
+    this.conditions = JSON.parse(JSON.stringify(this.principal.policyFragment.conditions));


This feels dangerous, I'm not sure tokens will survive this properly. What if we just keep conditions not as a mutable copy of the original, but as a list of mutations? So it starts out empty, and we deep-merge it upon rendering?

I was umm-ing and ahh-ing a bit over the approach here. I went with the copy largely because it was simpler than having a deep merge happen on principal.policyFragment and principal.conditions (so that the conditions getter returns the same thing the final render will include), but I don't have a good sense of how tokens work. If it's likely to break tokens, I'm happy to make the switch.

Can you please try that? It will make me feel better.

rix0rrr · 2020-03-30T14:13:48Z

packages/@aws-cdk/aws-iam/package.json

@@ -149,6 +149,9 @@
      "docs-public-apis:@aws-cdk/aws-iam.PrincipalBase.toJSON",
      "docs-public-apis:@aws-cdk/aws-iam.PrincipalPolicyFragment.conditions",
      "docs-public-apis:@aws-cdk/aws-iam.PrincipalPolicyFragment.principalJson",
+      "docs-public-apis:@aws-cdk/aws-iam.PrincipalWithConditions.conditions",


Please document the offending members instead of silencing the errors.

Will do.

I took the silencing of errors for the same/similar fields on PrincipalBase and PrincipalPolicyFragment to indicate there had been a decision that there was nothing worth documenting about these fields. If that's not the case, I'll add docs.

In fact the decision was "we don't want to regress docs going forward but we're not going to fix all instances before introducing the check".

So all existing members should be documented but we haven't gotten around to it yet.

- Change the conditions type to be `Record<string, Record<string, any>>` so as not to cause breaking changes once JSII transpiles. The change from an interface to a type alias is necessary because an interface in TS will convert to a named type in languages with nominal rather than structural typing. The change from `string | string[]` to `any` is needed because union types can't be supported. - Rather than copy the conditions from the wrapped principal when the `PrincipalWithConditions` is constructed, just merge the conditions in the getter for `conditions` to avoid any problems with tokens. - Add documentation for all public members. - Make wrapped principal private. - Inherit `assumeRoleAction` from the wrapped principal rather than always using "sts:AssumeRole".

Pull request has been modified.

aws-cdk-automation · 2020-04-02T21:53:31Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b452592
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-04-02T22:20:19Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 410e004
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Using `Record<string, Record<string, any>>` still caused breaking changes so change back to using `Record<string, any>`. At least the docs for it are improved.

aws-cdk-automation · 2020-04-02T23:10:28Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: ffc2bdc
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-04-02T23:11:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: f9119f3
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr

Thanks for the changes!

I still have some questions about the types.

I think the additional typing on the Conditions/Condition arguments are valuable. If jsii-diff was complaining on you, don't worry about it. Please put that back, and I will silence the linter.
Unfortunately no generics. Could you take those out?

rix0rrr · 2020-04-03T08:27:06Z

packages/@aws-cdk/aws-iam/lib/policy-statement.ts

+ * For more information, including which operators are supported, see [the IAM
+ * documentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html).
+ */
+export type Conditions = Record<string, any>;


Wait I believe we said two-level types here, right?

type Conditions = Record<string, Condition>; type Condition = Record<string, any>;

Is the compatibility checker giving you guff if you change this?

Yeah, I tried it and it still flagged as causing breaking changes.

From the failed build logs:

API elements with incompatible changes: err - INITIALIZER @aws-cdk/aws-iam.FederatedPrincipal.<initializer>: argument conditions, takes Map<string => Map<string => any>> (formerly Map<string => any>): Map<string => any> is not assignable to Map<string => Map<string => any>>, any is not a map type [incompatible-argument:@aws-cdk/aws-iam.FederatedPrincipal.<initializer>] err - METHOD @aws-cdk/aws-iam.PolicyStatement.addCondition: argument value, takes Map<string => string | Array<string>> (formerly any): any is not a map type [incompatible-argument:@aws-cdk/aws-iam.PolicyStatement.addCondition] err - METHOD @aws-cdk/aws-iam.PolicyStatement.addConditions: argument conditions, takes Map<string => Map<string => any>> (formerly Map<string => any>): Map<string => any> is not assignable to Map<string => Map<string => any>>, any is not a map type [incompatible-argument:@aws-cdk/aws-iam.PolicyStatement.addConditions] err - METHOD @aws-cdk/aws-iam.PolicyStatement.addFederatedPrincipal: argument conditions, takes Map<string => Map<string => any>> (formerly Map<string => any>): Map<string => any> is not assignable to Map<string => Map<string => any>>, any is not a map type [incompatible-argument:@aws-cdk/aws-iam.PolicyStatement.addFederatedPrincipal] err - INITIALIZER @aws-cdk/aws-iam.PrincipalPolicyFragment.<initializer>: argument conditions, takes Optional<Map<string => Map<string => any>>> (formerly Optional<Map<string => any>>): Map<string => any> is not assignable to Map<string => Map<string => any>>, any is not a map type [incompatible-argument:@aws-cdk/aws-iam.PrincipalPolicyFragment.<initializer>]

For reference, here's the commit where I changed it back (and caught the union type I'd somehow missed removing before).

rix0rrr · 2020-04-03T08:27:58Z

packages/@aws-cdk/aws-iam/lib/principals.ts

+ * For more information about conditions, see:
+ * https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html
+ */
+export class PrincipalWithConditions<PrincipalType extends PrincipalBase> implements IPrincipal {


Oh oops, was this in here beforehand? We can't have generics in this code base because jsii doesn't support them (because Go doesn't support them).

Since the inner principal is private you don't really need the generics.

Yeah, good point about it being obsolete now. Will get rid of it.

rix0rrr · 2020-04-03T08:31:06Z

packages/@aws-cdk/aws-iam/lib/principals.ts

+      mergedConditions[operator] = condition;
+    });
+    Object.entries(additionalConditions).forEach(([operator, condition]) => {
+      const existingCondition = mergedConditions[operator];


Since it's legal to can splat undefined in an object context (oh Javascript...), you could avoid the if and just go:

mergedConditions[operator] = {...mergedConditions[operator], ...condition};

This reverts commit ffc2bdc. Also adds the required ignores for these "breaking" changes to be accepted, and fixes the one remaining union type.

Pull request has been modified.

aws-cdk-automation · 2020-04-06T08:26:11Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 9fad94c
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-04-06T08:30:18Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 6b20828
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Pull request has been modified.

aws-cdk-automation · 2020-04-06T12:42:23Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: d955b9c
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-04-06T13:26:30Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: f18db86
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-04-06T14:25:55Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-04-06T14:57:50Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b88bc98
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-04-06T14:58:13Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Closes aws#5855 Adds a `PrincipalWithConditions` wrapper that allows conditions to be added to any principal. Behaviour is consistent with the way that `PolicyStatement.addCondition` and `.addConditions` currently work - most notably in that adding an operator that is already present will merge their objects, but adding a condition to an operator/key combination that is already set will overwrite the existing value (rather than merge the values into an array). BREAKING CHANGES: every place an IAM Condition was expected it is now typed as `{[key: string]: any}`, instead of plain `any`. You were always supposed to pass a map/dictionary in these locations, but the type system didn't enforce it. It now does. This will not impact correct programs, but may cause compiler errors in programs that were incorrect.

zakwalters added 7 commits March 21, 2020 20:36

Passing test

30552b4

Fix quote style

db8dc69

Add docs, plus addCondition/addConditions methods

b038bb1

`addCondition` and `addConditions` are added to the `PrincipalWithConditions` class to make its interface more consistent with what users are used to from the `PolicyStatement` class.

Remove obsolete test and other cleanup

ea625aa

zakwalters changed the title ~~feat(iam) add a wrapper for principals with conditions~~ feat(iam): add a wrapper for principals with conditions Mar 25, 2020

zakwalters and others added 2 commits March 25, 2020 21:16

Add README entry (required)

9a32cd6

Merge branch 'master' into zakwalters/issue-5855

05a5997

Merge branch 'master' into zakwalters/issue-5855

748f275

SomayaB assigned rix0rrr Mar 28, 2020

rix0rrr previously requested changes Mar 30, 2020

View reviewed changes

Merge branch 'master' into zakwalters/issue-5855

410e004

zakwalters and others added 2 commits April 2, 2020 23:39

Revert type changes

ffc2bdc

Using `Record<string, Record<string, any>>` still caused breaking changes so change back to using `Record<string, any>`. At least the docs for it are improved.

Merge branch 'master' into zakwalters/issue-5855

f9119f3

rix0rrr previously requested changes Apr 3, 2020

View reviewed changes

zakwalters added 2 commits April 4, 2020 21:47

Remove generics, simplify merging conditions

e200f8d

Reintroduce the Condition type

9fad94c

This reverts commit ffc2bdc. Also adds the required ignores for these "breaking" changes to be accepted, and fixes the one remaining union type.

Fix docs typo

6b20828

Merge remote-tracking branch 'origin/master' into pr/zakwalters/7015

d955b9c

rix0rrr previously approved these changes Apr 6, 2020

View reviewed changes

rix0rrr changed the title ~~feat(iam): add a wrapper for principals with conditions~~ feat(iam): add arbitrary conditions to existing principals Apr 6, 2020

Merge branch 'master' into zakwalters/issue-5855

f18db86

rix0rrr approved these changes Apr 6, 2020

View reviewed changes

Merge branch 'master' into zakwalters/issue-5855

b88bc98

mergify bot merged commit 64bad91 into aws:master Apr 6, 2020

feat(iam): add arbitrary conditions to existing principals #7015

feat(iam): add arbitrary conditions to existing principals #7015

Conversation

zakwalters commented Mar 25, 2020 • edited by rix0rrr Loading

Commit Message

End Commit Message

mergify bot commented Mar 25, 2020

aws-cdk-automation commented Mar 25, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 25, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 25, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 26, 2020

AWS CodeBuild CI Report

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 2, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 2, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 2, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 2, 2020

AWS CodeBuild CI Report

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 6, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 6, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 6, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 6, 2020

AWS CodeBuild CI Report

mergify bot commented Apr 6, 2020

aws-cdk-automation commented Apr 6, 2020

AWS CodeBuild CI Report

mergify bot commented Apr 6, 2020

zakwalters commented Mar 25, 2020 •

edited by rix0rrr

Loading