From 30552b433b389bb7c2c50a55b1885308eb039114 Mon Sep 17 00:00:00 2001
From: tOOtl <24k@gmx.com>
Date: Sat, 21 Mar 2020 20:36:49 +0000
Subject: [PATCH 01/13] Passing test

---
 .../@aws-cdk/aws-iam/lib/policy-statement.ts  |  3 +-
 packages/@aws-cdk/aws-iam/lib/principals.ts   | 77 +++++++++++++++++--
 .../@aws-cdk/aws-iam/test/principals.test.ts  | 61 ++++++++++++++-
 3 files changed, 131 insertions(+), 10 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index 01d0c3934909b..e459e86571db7 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -135,6 +135,7 @@ export class PolicyStatement {
     this.addPrincipals(new ArnPrincipal(arn));
   }
 
+  // TODO: these helpers should add PrincipalWithConditions if `conditions` is passed
   /**
    * Adds a service principal to this policy statement.
    *
@@ -398,7 +399,7 @@ class JsonPrincipal extends PrincipalBase {
 
     this.policyFragment = {
       principalJson: json,
-      conditions: []
+      conditions: {}
     };
   }
 }
diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts
index 2049e910c4bb0..b85d1406cc818 100644
--- a/packages/@aws-cdk/aws-iam/lib/principals.ts
+++ b/packages/@aws-cdk/aws-iam/lib/principals.ts
@@ -3,6 +3,25 @@ import { Default, RegionInfo } from '@aws-cdk/region-info';
 import { PolicyStatement } from './policy-statement';
 import { mergePrincipal } from './util';
 
+// TODO: add all types. Probably belongs in its own file. Export? Could just be one flat object too.
+// Might be able to better than `any` as the value
+// TODO: check this matches the IAM service. Nija says that you can only specify each condition once,
+// so seems right, but should check
+// export interface Conditions {
+//   readonly "StringEquals"?: any;
+//   readonly StringNotEquals?: any;
+//   readonly StringEqualsIgnoreCase?: any;
+//   readonly StringNotEqualsIgnoreCase?: any;
+//   readonly StringLike?: any;
+//   readonly StringNotLike?: any;
+// };
+/**
+ * TODO: docs
+ */
+export interface Conditions {
+  [key: string]: any;
+}
+
 /**
  * Any object that has an associated principal that a permission can be granted to
  */
@@ -17,7 +36,7 @@ export interface IGrantable {
  * Represents a logical IAM principal.
  *
  * An IPrincipal describes a logical entity that can perform AWS API calls
- * against sets of resources, optionally under certain conditions.
+ * against sets of resources, optionally under certain conditions. // TODO: may need to update these docs
  *
  * Examples of simple principals are IAM objects that you create, such
  * as Users or Roles.
@@ -82,6 +101,50 @@ export abstract class PrincipalBase implements IPrincipal {
     // Have to implement toJSON() because the default will lead to infinite recursion.
     return this.policyFragment.principalJson;
   }
+
+  // TODO: should this be part of IPrincipal too?
+  /**
+   * TODO: docs
+   */
+  public withConditions(conditions: Conditions): IPrincipal {
+    return new PrincipalWithConditions(this, conditions);
+  }
+}
+
+/**
+ * A principal with conditions TODO: improve docs; should this extend BasePrincipal instead?
+ */
+export class PrincipalWithConditions implements IPrincipal {
+  public readonly grantPrincipal: IPrincipal = this;
+
+  /**
+   * When this Principal is used in an AssumeRole policy, the action to use.
+   */
+  public readonly assumeRoleAction: string = 'sts:AssumeRole';
+
+  constructor(
+    /**
+     * The principal to which conditions are being added.
+     * TODO: review docs, esp in context of where they show up
+     */
+    public readonly principal: IPrincipal,
+    // TODO: fix any; review access; ServicePrincipal uses `opts` object, consider that
+    /**
+     * The conditions to add to the principal.
+     * TODO: review docs - can steal from below
+     */
+    public readonly conditions: Conditions,
+  ) {
+  } // TODO: check style for empty constructor
+
+  public get policyFragment(): PrincipalPolicyFragment {
+    // TODO: merge this.conditions with the ones already defined on this.principal
+    return new PrincipalPolicyFragment(this.principal.policyFragment.principalJson, this.conditions);
+  }
+
+  public addToPolicy(statement: PolicyStatement): boolean {
+    return this.principal.addToPolicy(statement);
+  }
 }
 
 /**
@@ -93,7 +156,7 @@ export abstract class PrincipalBase implements IPrincipal {
 export class PrincipalPolicyFragment {
   constructor(
     public readonly principalJson: { [key: string]: string[] },
-    public readonly conditions: { [key: string]: any } = { }) {
+    public readonly conditions: Conditions = {}) {
   }
 }
 
@@ -103,7 +166,7 @@ export class ArnPrincipal extends PrincipalBase {
   }
 
   public get policyFragment(): PrincipalPolicyFragment {
-    return new PrincipalPolicyFragment({ AWS: [ this.arn ] });
+    return new PrincipalPolicyFragment({ AWS: [this.arn] });
   }
 
   public toString() {
@@ -200,7 +263,7 @@ export class CanonicalUserPrincipal extends PrincipalBase {
   }
 
   public get policyFragment(): PrincipalPolicyFragment {
-    return new PrincipalPolicyFragment({ CanonicalUser: [ this.canonicalUserId ] });
+    return new PrincipalPolicyFragment({ CanonicalUser: [this.canonicalUserId] });
   }
 
   public toString() {
@@ -213,7 +276,7 @@ export class FederatedPrincipal extends PrincipalBase {
 
   constructor(
     public readonly federated: string,
-    public readonly conditions: {[key: string]: any},
+    public readonly conditions: Conditions,
     assumeRoleAction: string = 'sts:AssumeRole') {
     super();
 
@@ -221,7 +284,7 @@ export class FederatedPrincipal extends PrincipalBase {
   }
 
   public get policyFragment(): PrincipalPolicyFragment {
-    return new PrincipalPolicyFragment({ Federated: [ this.federated ] }, this.conditions);
+    return new PrincipalPolicyFragment({ Federated: [this.federated] }, this.conditions);
   }
 
   public toString() {
@@ -293,7 +356,7 @@ export class CompositePrincipal extends PrincipalBase {
   }
 
   public get policyFragment(): PrincipalPolicyFragment {
-    const principalJson: { [key: string]: string[] } = { };
+    const principalJson: { [key: string]: string[] } = {};
 
     for (const p of this.principals) {
       mergePrincipal(principalJson, p.policyFragment.principalJson);
diff --git a/packages/@aws-cdk/aws-iam/test/principals.test.ts b/packages/@aws-cdk/aws-iam/test/principals.test.ts
index 02dc795f0e19d..b147c975c05ef 100644
--- a/packages/@aws-cdk/aws-iam/test/principals.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/principals.test.ts
@@ -46,6 +46,63 @@ test('use of cross-stack role reference does not lead to URLSuffix being exporte
         }
       }
     }
-  }
-  );
+  });
 });
+
+// TODO: may well belong in policy-document.test.ts
+test('can create principal with conditions', () => {
+  // GIVEN
+  const accountId = '012345678910';
+  const app = new App();
+  const stack = new Stack(app, 'Stack');
+
+  // WHEN
+  const conditions = {
+    StringEquals: {
+      "s3:x-amz-acl": [
+        "public-read"
+      ]
+    }
+  };
+  const principal = new iam.AccountPrincipal(accountId).withConditions(conditions);
+  new iam.Role(stack, 'Role', {
+    assumedBy: principal,
+  });
+
+  // THEN
+  app.synth();
+
+  expect(stack).toMatchTemplate({
+    Resources: {
+      Role1ABCC5F0: { // how is this name set? Seems to be used a lot of places so I guess it's the one that'll be generated...
+        Type: "AWS::IAM::Role",
+        Properties: {
+          AssumeRolePolicyDocument: {
+            Statement: [
+              {
+                Action: "sts:AssumeRole",
+                Condition: {
+                  StringEquals: {
+                    "s3:x-amz-acl": [
+                      "public-read"
+                    ]
+                  }
+                },
+                Effect: "Allow",
+                Principal: {
+                  AWS: {
+                    'Fn::Join': [
+                      '',
+                      [ 'arn:', { Ref: 'AWS::Partition' }, `:iam::${accountId}:root` ]
+                    ]
+                  }
+                },
+              }
+            ],
+            Version: "2012-10-17"
+          }
+        }
+      }
+    }
+  });
+});
\ No newline at end of file

From c48c6bc318a25590a5e9fa537ede8864ce34266b Mon Sep 17 00:00:00 2001
From: tOOtl <24k@gmx.com>
Date: Sun, 22 Mar 2020 14:41:01 +0000
Subject: [PATCH 02/13] Add extra tests

- 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).
---
 packages/@aws-cdk/aws-iam/lib/principals.ts   | 46 ++++++++-------
 .../aws-iam/test/policy-document.test.ts      | 56 +++++++++++++++++--
 .../aws-iam/test/policy-statement.test.ts     |  2 +-
 3 files changed, 75 insertions(+), 29 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts
index b85d1406cc818..d4a69710628ee 100644
--- a/packages/@aws-cdk/aws-iam/lib/principals.ts
+++ b/packages/@aws-cdk/aws-iam/lib/principals.ts
@@ -3,23 +3,17 @@ import { Default, RegionInfo } from '@aws-cdk/region-info';
 import { PolicyStatement } from './policy-statement';
 import { mergePrincipal } from './util';
 
-// TODO: add all types. Probably belongs in its own file. Export? Could just be one flat object too.
-// Might be able to better than `any` as the value
-// TODO: check this matches the IAM service. Nija says that you can only specify each condition once,
-// so seems right, but should check
-// export interface Conditions {
-//   readonly "StringEquals"?: any;
-//   readonly StringNotEquals?: any;
-//   readonly StringEqualsIgnoreCase?: any;
-//   readonly StringNotEqualsIgnoreCase?: any;
-//   readonly StringLike?: any;
-//   readonly StringNotLike?: any;
-// };
 /**
  * TODO: docs
+ * https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html
+ * https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_condition-keys.html
+ *
+ * TODO: note for commit - JSII blocks this from being more strongly modelled
+ * TODO: check this type is used in all places (e.g. statement.addCondition)...
+ * technically a breaking change to narrow the type but seeing as Cfn enforces it anyway it should be acceptable
  */
 export interface Conditions {
-  [key: string]: any;
+  [key: string]: { [conditionKey: string]: string | string[] };
 }
 
 /**
@@ -114,7 +108,11 @@ export abstract class PrincipalBase implements IPrincipal {
 /**
  * A principal with conditions TODO: improve docs; should this extend BasePrincipal instead?
  */
-export class PrincipalWithConditions implements IPrincipal {
+export class PrincipalWithConditions<PrincipalType extends PrincipalBase> implements IPrincipal {
+  /**
+   * TODO: docs (or exclude in package.json)
+   */
+  public readonly conditions: Conditions;
   public readonly grantPrincipal: IPrincipal = this;
 
   /**
@@ -124,18 +122,14 @@ export class PrincipalWithConditions implements IPrincipal {
 
   constructor(
     /**
-     * The principal to which conditions are being added.
-     * TODO: review docs, esp in context of where they show up
-     */
-    public readonly principal: IPrincipal,
-    // TODO: fix any; review access; ServicePrincipal uses `opts` object, consider that
-    /**
-     * The conditions to add to the principal.
-     * TODO: review docs - can steal from below
+     * TODO: docs (or exclude in package.json)
      */
-    public readonly conditions: Conditions,
+    public readonly principal: PrincipalType,
+    conditions: Conditions,
   ) {
-  } // TODO: check style for empty constructor
+      // TODO: check for clashes?
+      this.conditions = { ...principal.policyFragment.conditions, ...conditions };
+  }
 
   public get policyFragment(): PrincipalPolicyFragment {
     // TODO: merge this.conditions with the ones already defined on this.principal
@@ -145,6 +139,10 @@ export class PrincipalWithConditions implements IPrincipal {
   public addToPolicy(statement: PolicyStatement): boolean {
     return this.principal.addToPolicy(statement);
   }
+
+  public toString() {
+    return this.principal.toString();
+  }
 }
 
 /**
diff --git a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
index a1cb97fed9821..2402f6992ac16 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
@@ -1,8 +1,8 @@
 import '@aws-cdk/assert/jest';
 import { Lazy, Stack, Token } from '@aws-cdk/core';
 import {
-  Anyone, AnyPrincipal, ArnPrincipal, CanonicalUserPrincipal, CompositePrincipal, Effect, FederatedPrincipal,
-  IPrincipal, PolicyDocument, PolicyStatement, PrincipalPolicyFragment, ServicePrincipal
+  AccountPrincipal, Anyone, AnyPrincipal, ArnPrincipal, CanonicalUserPrincipal, CompositePrincipal, Effect,
+  FederatedPrincipal, IPrincipal, PolicyDocument, PolicyStatement, PrincipalPolicyFragment, ServicePrincipal
 } from '../lib';
 
 describe('IAM polocy document', () => {
@@ -427,7 +427,7 @@ describe('IAM polocy document', () => {
 
     test('conditions are not allowed on individual principals of a composite', () => {
       const p = new CompositePrincipal(new ArnPrincipal('i:am'));
-      expect(() => p.addPrincipals(new FederatedPrincipal('federated', { condition: 1 })))
+      expect(() => p.addPrincipals(new FederatedPrincipal('federated', { StringEquals: { 'aws:some-key': 'some-value' } })))
         .toThrow(/Components of a CompositePrincipal must not have conditions/);
     });
 
@@ -469,7 +469,55 @@ describe('IAM polocy document', () => {
 
       // THEN
       expect(() => p.addPrincipals(new FederatedPrincipal('fed', {}, 'sts:Boom')))
-       .toThrow(/Cannot add multiple principals with different "assumeRoleAction". Expecting "sts:AssumeRole", got "sts:Boom"/);
+        .toThrow(/Cannot add multiple principals with different "assumeRoleAction". Expecting "sts:AssumeRole", got "sts:Boom"/);
+    });
+  });
+
+  describe('PrincipalWithConditions can be used to add a principal with conditions', () => {
+    test('includes conditions from both the wrapped principal and the wrapper', () => {
+      // TODO: quote style
+      const stack = new Stack();
+      const principalOpts = {
+        conditions: {
+          BinaryEquals: {
+            "principal-key": "SGV5LCBmcmllbmQh",
+          },
+        },
+      };
+      const p = new ServicePrincipal('s3.amazonaws.com', principalOpts)
+        .withConditions({ StringEquals: { "wrapper-key": ["val-1", "val-2"] } });
+      const statement = new PolicyStatement();
+      statement.addPrincipals(p);
+      expect(stack.resolve(statement.toStatementJson())).toEqual({
+        Condition: {
+          BinaryEquals: { "principal-key": "SGV5LCBmcmllbmQh" },
+          StringEquals: { "wrapper-key": ["val-1", "val-2"] },
+        },
+        Effect: 'Allow',
+        Principal: {
+          Service: 's3.amazonaws.com',
+        },
+      });
+    });
+
+    test('conditions from addCondition are merged with those from the principal', () => {
+      const stack = new Stack();
+      const p = new AccountPrincipal('012345678900').withConditions({ StringEquals: { key: "val" } });
+      const statement = new PolicyStatement();
+      statement.addPrincipals(p);
+      statement.addCondition("Null", { "banned-key": "true" });
+      expect(stack.resolve(statement.toStatementJson())).toEqual({
+        Effect: 'Allow',
+        Principal: { AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::012345678900:root']] } },
+        Condition: { StringEquals: { key: "val" }, Null: { "banned-key": "true" } },
+      });
+    });
+
+    test('adding conditions via `withConditions` does not affect the original principal', () => {
+      const originalPrincipal = new ArnPrincipal('iam:an:arn');
+      const principalWithConditions = originalPrincipal.withConditions({ StringEquals: { key: "val" } });
+      expect(originalPrincipal.policyFragment.conditions).toEqual({});
+      expect(principalWithConditions.policyFragment.conditions).toEqual({ StringEquals: { key: "val" } });
     });
   });
 
diff --git a/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts b/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
index 11207b7ed541b..663520f53ec21 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
@@ -173,7 +173,7 @@ describe('IAM policy statement', () => {
       }).toThrow(/Fields must be either a string or an array of strings/);
     });
 
-    test('throws error with field data being object', () => {
+    test('throws error with field data being array of non-strings', () => {
       expect(() => {
         PolicyStatement.fromJson({
           Action: [{}]

From 81a73b94130c7164e0435fc84dfba87069b24c32 Mon Sep 17 00:00:00 2001
From: tOOtl <24k@gmx.com>
Date: Tue, 24 Mar 2020 00:26:42 +0000
Subject: [PATCH 03/13] Handle conflicting operators and keys

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.
---
 packages/@aws-cdk/aws-iam/lib/principals.ts   |  9 +++-
 .../aws-iam/test/policy-document.test.ts      | 41 +++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts
index d4a69710628ee..9fd5b358f875a 100644
--- a/packages/@aws-cdk/aws-iam/lib/principals.ts
+++ b/packages/@aws-cdk/aws-iam/lib/principals.ts
@@ -127,8 +127,13 @@ export class PrincipalWithConditions<PrincipalType extends PrincipalBase> implem
     public readonly principal: PrincipalType,
     conditions: Conditions,
   ) {
-      // TODO: check for clashes?
-      this.conditions = { ...principal.policyFragment.conditions, ...conditions };
+      this.conditions = conditions;
+      Object.entries(principal.policyFragment.conditions).forEach(([key, valueFromPrincipal]) => {
+        const valueFromConditions = this.conditions[key];
+        this.conditions[key] = valueFromConditions
+          ? { ...valueFromPrincipal, ...valueFromConditions }
+          : valueFromPrincipal;
+      });
   }
 
   public get policyFragment(): PrincipalPolicyFragment {
diff --git a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
index 2402f6992ac16..c42fbc1f1d379 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
@@ -519,6 +519,47 @@ describe('IAM polocy document', () => {
       expect(originalPrincipal.policyFragment.conditions).toEqual({});
       expect(principalWithConditions.policyFragment.conditions).toEqual({ StringEquals: { key: "val" } });
     });
+
+    test('conditions are merged when operators conflict', () => {
+      const p = new FederatedPrincipal('fed', {
+        OperatorOne: { "fed-key": "fed-val" },
+        OperatorTwo: { "fed-key": "fed-val" },
+        OperatorThree: { "fed-key": "fed-val" },
+      }).withConditions({
+        OperatorTwo: { "with-key": "with-val" },
+        OperatorThree: { "with-key": "with-val" },
+      });
+      const statement = new PolicyStatement();
+      statement.addCondition("OperatorThree", { "add-key": "add-val" });
+      statement.addPrincipals(p);
+      expect(statement.toStatementJson()).toEqual({
+        Effect: 'Allow',
+        Principal: { Federated: 'fed' },
+        Condition: {
+          OperatorOne: { "fed-key": "fed-val" },
+          OperatorTwo: { "fed-key": "fed-val", "with-key": "with-val" },
+          OperatorThree: { "fed-key": "fed-val", "with-key": "with-val", "add-key": "add-val" },
+        }
+      });
+    });
+
+    test('values passed to `withConditions` overwrite values from the wrapped principal ' +
+      'when keys conflict within an operator', () => {
+      const p = new FederatedPrincipal('fed', {
+        Operator: { key: "p-val" },
+      }).withConditions({
+        Operator: { key: "with-val" },
+      });
+      const statement = new PolicyStatement();
+      statement.addPrincipals(p);
+      expect(statement.toStatementJson()).toEqual({
+        Effect: "Allow",
+        Principal: { Federated: 'fed' },
+        Condition: {
+          Operator: { key: "with-val" },
+        },
+      });
+    });
   });
 
   describe('duplicate statements', () => {

From db8dc69e311df122e67f62751c4e44e806ee4f97 Mon Sep 17 00:00:00 2001
From: tOOtl <24k@gmx.com>
Date: Tue, 24 Mar 2020 00:40:58 +0000
Subject: [PATCH 04/13] Fix quote style

---
 .../aws-iam/test/policy-document.test.ts      | 44 +++++++++----------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
index c42fbc1f1d379..4ab4366446376 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
@@ -480,18 +480,18 @@ describe('IAM polocy document', () => {
       const principalOpts = {
         conditions: {
           BinaryEquals: {
-            "principal-key": "SGV5LCBmcmllbmQh",
+            'principal-key': 'SGV5LCBmcmllbmQh',
           },
         },
       };
       const p = new ServicePrincipal('s3.amazonaws.com', principalOpts)
-        .withConditions({ StringEquals: { "wrapper-key": ["val-1", "val-2"] } });
+        .withConditions({ StringEquals: { 'wrapper-key': ['val-1', 'val-2'] } });
       const statement = new PolicyStatement();
       statement.addPrincipals(p);
       expect(stack.resolve(statement.toStatementJson())).toEqual({
         Condition: {
-          BinaryEquals: { "principal-key": "SGV5LCBmcmllbmQh" },
-          StringEquals: { "wrapper-key": ["val-1", "val-2"] },
+          BinaryEquals: { 'principal-key': 'SGV5LCBmcmllbmQh' },
+          StringEquals: { 'wrapper-key': ['val-1', 'val-2'] },
         },
         Effect: 'Allow',
         Principal: {
@@ -502,43 +502,43 @@ describe('IAM polocy document', () => {
 
     test('conditions from addCondition are merged with those from the principal', () => {
       const stack = new Stack();
-      const p = new AccountPrincipal('012345678900').withConditions({ StringEquals: { key: "val" } });
+      const p = new AccountPrincipal('012345678900').withConditions({ StringEquals: { key: 'val' } });
       const statement = new PolicyStatement();
       statement.addPrincipals(p);
-      statement.addCondition("Null", { "banned-key": "true" });
+      statement.addCondition('Null', { 'banned-key': 'true' });
       expect(stack.resolve(statement.toStatementJson())).toEqual({
         Effect: 'Allow',
         Principal: { AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::012345678900:root']] } },
-        Condition: { StringEquals: { key: "val" }, Null: { "banned-key": "true" } },
+        Condition: { StringEquals: { key: 'val' }, Null: { 'banned-key': 'true' } },
       });
     });
 
     test('adding conditions via `withConditions` does not affect the original principal', () => {
       const originalPrincipal = new ArnPrincipal('iam:an:arn');
-      const principalWithConditions = originalPrincipal.withConditions({ StringEquals: { key: "val" } });
+      const principalWithConditions = originalPrincipal.withConditions({ StringEquals: { key: 'val' } });
       expect(originalPrincipal.policyFragment.conditions).toEqual({});
-      expect(principalWithConditions.policyFragment.conditions).toEqual({ StringEquals: { key: "val" } });
+      expect(principalWithConditions.policyFragment.conditions).toEqual({ StringEquals: { key: 'val' } });
     });
 
     test('conditions are merged when operators conflict', () => {
       const p = new FederatedPrincipal('fed', {
-        OperatorOne: { "fed-key": "fed-val" },
-        OperatorTwo: { "fed-key": "fed-val" },
-        OperatorThree: { "fed-key": "fed-val" },
+        OperatorOne: { 'fed-key': 'fed-val' },
+        OperatorTwo: { 'fed-key': 'fed-val' },
+        OperatorThree: { 'fed-key': 'fed-val' },
       }).withConditions({
-        OperatorTwo: { "with-key": "with-val" },
-        OperatorThree: { "with-key": "with-val" },
+        OperatorTwo: { 'with-key': 'with-val' },
+        OperatorThree: { 'with-key': 'with-val' },
       });
       const statement = new PolicyStatement();
-      statement.addCondition("OperatorThree", { "add-key": "add-val" });
+      statement.addCondition('OperatorThree', { 'add-key': 'add-val' });
       statement.addPrincipals(p);
       expect(statement.toStatementJson()).toEqual({
         Effect: 'Allow',
         Principal: { Federated: 'fed' },
         Condition: {
-          OperatorOne: { "fed-key": "fed-val" },
-          OperatorTwo: { "fed-key": "fed-val", "with-key": "with-val" },
-          OperatorThree: { "fed-key": "fed-val", "with-key": "with-val", "add-key": "add-val" },
+          OperatorOne: { 'fed-key': 'fed-val' },
+          OperatorTwo: { 'fed-key': 'fed-val', 'with-key': 'with-val' },
+          OperatorThree: { 'fed-key': 'fed-val', 'with-key': 'with-val', 'add-key': 'add-val' },
         }
       });
     });
@@ -546,17 +546,17 @@ describe('IAM polocy document', () => {
     test('values passed to `withConditions` overwrite values from the wrapped principal ' +
       'when keys conflict within an operator', () => {
       const p = new FederatedPrincipal('fed', {
-        Operator: { key: "p-val" },
+        Operator: { key: 'p-val' },
       }).withConditions({
-        Operator: { key: "with-val" },
+        Operator: { key: 'with-val' },
       });
       const statement = new PolicyStatement();
       statement.addPrincipals(p);
       expect(statement.toStatementJson()).toEqual({
-        Effect: "Allow",
+        Effect: 'Allow',
         Principal: { Federated: 'fed' },
         Condition: {
-          Operator: { key: "with-val" },
+          Operator: { key: 'with-val' },
         },
       });
     });

From e3b4e498e3f0bdf271521fdf15a895f3ee472bf1 Mon Sep 17 00:00:00 2001
From: tOOtl <24k@gmx.com>
Date: Tue, 24 Mar 2020 19:33:09 +0000
Subject: [PATCH 05/13] Use Conditions type in PolicyStatement

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.
---
 .../@aws-cdk/aws-iam/lib/policy-statement.ts    | 17 +++++++++++++++--
 packages/@aws-cdk/aws-iam/lib/principals.ts     | 16 +---------------
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index e459e86571db7..f9def728e5c4e 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -146,7 +146,7 @@ export class PolicyStatement {
     this.addPrincipals(new ServicePrincipal(service, opts));
   }
 
-  public addFederatedPrincipal(federated: any, conditions: {[key: string]: any}) {
+  public addFederatedPrincipal(federated: any, conditions: Conditions) {
     this.addPrincipals(new FederatedPrincipal(federated, conditions));
   }
 
@@ -209,7 +209,7 @@ export class PolicyStatement {
   /**
    * Add multiple conditions to the Policy
    */
-  public addConditions(conditions: {[key: string]: any}) {
+  public addConditions(conditions: Conditions) {
     Object.keys(conditions).map(key => {
       this.addCondition(key, conditions[key]);
     });
@@ -304,6 +304,19 @@ export enum Effect {
   DENY = 'Deny',
 }
 
+/**
+ * TODO: docs
+ * https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html
+ * https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_condition-keys.html
+ *
+ * TODO: note for commit - JSII blocks this from being more strongly modelled
+ * TODO: check this type is used in all places (e.g. statement.addCondition)...
+ * technically a breaking change to narrow the type but seeing as Cfn enforces it anyway it should be acceptable
+ */
+export interface Conditions {
+  [operator: string]: { [conditionKey: string]: string | string[] };
+}
+
 /**
  * Interface for creating a policy statement
  */
diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts
index 9fd5b358f875a..6981e223c596c 100644
--- a/packages/@aws-cdk/aws-iam/lib/principals.ts
+++ b/packages/@aws-cdk/aws-iam/lib/principals.ts
@@ -1,21 +1,8 @@
 import * as cdk from '@aws-cdk/core';
 import { Default, RegionInfo } from '@aws-cdk/region-info';
-import { PolicyStatement } from './policy-statement';
+import { Conditions, PolicyStatement } from './policy-statement';
 import { mergePrincipal } from './util';
 
-/**
- * TODO: docs
- * https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html
- * https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_condition-keys.html
- *
- * TODO: note for commit - JSII blocks this from being more strongly modelled
- * TODO: check this type is used in all places (e.g. statement.addCondition)...
- * technically a breaking change to narrow the type but seeing as Cfn enforces it anyway it should be acceptable
- */
-export interface Conditions {
-  [key: string]: { [conditionKey: string]: string | string[] };
-}
-
 /**
  * Any object that has an associated principal that a permission can be granted to
  */
@@ -137,7 +124,6 @@ export class PrincipalWithConditions<PrincipalType extends PrincipalBase> implem
   }
 
   public get policyFragment(): PrincipalPolicyFragment {
-    // TODO: merge this.conditions with the ones already defined on this.principal
     return new PrincipalPolicyFragment(this.principal.policyFragment.principalJson, this.conditions);
   }
 

From b038bb184d658eca8e9c162e806a72afa3265037 Mon Sep 17 00:00:00 2001
From: tOOtl <24k@gmx.com>
Date: Wed, 25 Mar 2020 19:59:07 +0000
Subject: [PATCH 06/13] Add docs, plus addCondition/addConditions methods

`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.
---
 .../@aws-cdk/aws-iam/lib/policy-statement.ts  | 13 ++--
 packages/@aws-cdk/aws-iam/lib/principals.ts   | 61 ++++++++++++-------
 packages/@aws-cdk/aws-iam/package.json        |  3 +
 .../aws-iam/test/policy-document.test.ts      |  4 +-
 4 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index f9def728e5c4e..b152c7f262333 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -135,7 +135,6 @@ export class PolicyStatement {
     this.addPrincipals(new ArnPrincipal(arn));
   }
 
-  // TODO: these helpers should add PrincipalWithConditions if `conditions` is passed
   /**
    * Adds a service principal to this policy statement.
    *
@@ -201,7 +200,7 @@ export class PolicyStatement {
   /**
    * Add a condition to the Policy
    */
-  public addCondition(key: string, value: any) {
+  public addCondition(key: string, value: { [key: string]: string | string[] }) {
     const existingValue = this.condition[key];
     this.condition[key] = existingValue ? { ...existingValue, ...value } : value;
   }
@@ -305,16 +304,12 @@ export enum Effect {
 }
 
 /**
- * TODO: docs
- * https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html
- * https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_condition-keys.html
+ * Conditions for when an IAM Policy is in effect.
  *
- * TODO: note for commit - JSII blocks this from being more strongly modelled
- * TODO: check this type is used in all places (e.g. statement.addCondition)...
- * technically a breaking change to narrow the type but seeing as Cfn enforces it anyway it should be acceptable
+ * See https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html
  */
 export interface Conditions {
-  [operator: string]: { [conditionKey: string]: string | string[] };
+  [operator: string]: { [key: string]: string | string[] };
 }
 
 /**
diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts
index 6981e223c596c..5364d4310ee6b 100644
--- a/packages/@aws-cdk/aws-iam/lib/principals.ts
+++ b/packages/@aws-cdk/aws-iam/lib/principals.ts
@@ -17,7 +17,7 @@ export interface IGrantable {
  * Represents a logical IAM principal.
  *
  * An IPrincipal describes a logical entity that can perform AWS API calls
- * against sets of resources, optionally under certain conditions. // TODO: may need to update these docs
+ * against sets of resources, optionally under certain conditions.
  *
  * Examples of simple principals are IAM objects that you create, such
  * as Users or Roles.
@@ -83,9 +83,14 @@ export abstract class PrincipalBase implements IPrincipal {
     return this.policyFragment.principalJson;
   }
 
-  // TODO: should this be part of IPrincipal too?
   /**
-   * TODO: docs
+   * Returns a new PrincipalWithConditions using this principal as the base, with the
+   * passed conditions added.
+   *
+   * When there is a value for the same operator and key in both the principal and the
+   * conditions parameter, the value from the conditions parameter will be used.
+   *
+   * @returns a new PrincipalWithConditions object.
    */
   public withConditions(conditions: Conditions): IPrincipal {
     return new PrincipalWithConditions(this, conditions);
@@ -93,34 +98,43 @@ export abstract class PrincipalBase implements IPrincipal {
 }
 
 /**
- * A principal with conditions TODO: improve docs; should this extend BasePrincipal instead?
+ * An IAM principal with additional conditions specifying when the policy is in effect.
+ *
+ * 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 {
-  /**
-   * TODO: docs (or exclude in package.json)
-   */
   public readonly conditions: Conditions;
   public readonly grantPrincipal: IPrincipal = this;
-
-  /**
-   * When this Principal is used in an AssumeRole policy, the action to use.
-   */
   public readonly assumeRoleAction: string = 'sts:AssumeRole';
 
   constructor(
-    /**
-     * TODO: docs (or exclude in package.json)
-     */
     public readonly principal: PrincipalType,
     conditions: Conditions,
   ) {
-      this.conditions = conditions;
-      Object.entries(principal.policyFragment.conditions).forEach(([key, valueFromPrincipal]) => {
-        const valueFromConditions = this.conditions[key];
-        this.conditions[key] = valueFromConditions
-          ? { ...valueFromPrincipal, ...valueFromConditions }
-          : valueFromPrincipal;
-      });
+    // Copy the intiial conditions from the principal
+    this.conditions = JSON.parse(JSON.stringify(this.principal.policyFragment.conditions));
+    this.addConditions(conditions);
+  }
+
+  /**
+   * Add a condition to the principal
+   */
+  public addCondition(key: string, value: { [key: string]: string | string[] }) {
+    const existingValue = this.conditions[key];
+    this.conditions[key] = existingValue ? { ...existingValue, ...value } : value;
+  }
+
+  /**
+   * Adds multiple conditions to the principal
+   *
+   * Values from the conditions parameter will overwrite existing values with the same operator
+   * and key.
+   */
+  public addConditions(conditions: Conditions) {
+    Object.entries(conditions).forEach(([key, value]) => {
+      this.addCondition(key, value);
+    });
   }
 
   public get policyFragment(): PrincipalPolicyFragment {
@@ -134,6 +148,11 @@ export class PrincipalWithConditions<PrincipalType extends PrincipalBase> implem
   public toString() {
     return this.principal.toString();
   }
+
+  public toJSON() {
+    // Have to implement toJSON() because the default will lead to infinite recursion.
+    return this.policyFragment.principalJson;
+  }
 }
 
 /**
diff --git a/packages/@aws-cdk/aws-iam/package.json b/packages/@aws-cdk/aws-iam/package.json
index c835261df8618..93c626b4101be 100644
--- a/packages/@aws-cdk/aws-iam/package.json
+++ b/packages/@aws-cdk/aws-iam/package.json
@@ -147,6 +147,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",
+      "docs-public-apis:@aws-cdk/aws-iam.PrincipalWithConditions.principal",
+      "docs-public-apis:@aws-cdk/aws-iam.PrincipalWithConditions.toJSON",
       "docs-public-apis:@aws-cdk/aws-iam.ServicePrincipal.service",
       "props-default-doc:@aws-cdk/aws-iam.GrantOnPrincipalOptions.scope",
       "docs-public-apis:@aws-cdk/aws-iam.GroupProps",
diff --git a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
index 4ab4366446376..ae8c56724a488 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
@@ -449,11 +449,11 @@ describe('IAM polocy document', () => {
 
       // add via policy statement
       statement.addArnPrincipal('aws-principal-3');
-      statement.addCondition('cond2', { boom: 123 });
+      statement.addCondition('cond2', { boom: "123" });
 
       expect(stack.resolve(statement.toStatementJson())).toEqual({
         Condition: {
-          cond2: { boom: 123 }
+          cond2: { boom: "123" }
         },
         Effect: 'Allow',
         Principal: {

From ea625aa175104e6d8500ecaa4f874bdd07f84924 Mon Sep 17 00:00:00 2001
From: Zak Walters <24k@gmx.com>
Date: Wed, 25 Mar 2020 20:47:49 +0000
Subject: [PATCH 07/13] Remove obsolete test and other cleanup

---
 .../aws-iam/test/policy-document.test.ts      |  5 +-
 .../@aws-cdk/aws-iam/test/principals.test.ts  | 61 +------------------
 2 files changed, 4 insertions(+), 62 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
index ae8c56724a488..b0e8332527007 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
@@ -449,11 +449,11 @@ describe('IAM polocy document', () => {
 
       // add via policy statement
       statement.addArnPrincipal('aws-principal-3');
-      statement.addCondition('cond2', { boom: "123" });
+      statement.addCondition('cond2', { boom: '123' });
 
       expect(stack.resolve(statement.toStatementJson())).toEqual({
         Condition: {
-          cond2: { boom: "123" }
+          cond2: { boom: '123' }
         },
         Effect: 'Allow',
         Principal: {
@@ -475,7 +475,6 @@ describe('IAM polocy document', () => {
 
   describe('PrincipalWithConditions can be used to add a principal with conditions', () => {
     test('includes conditions from both the wrapped principal and the wrapper', () => {
-      // TODO: quote style
       const stack = new Stack();
       const principalOpts = {
         conditions: {
diff --git a/packages/@aws-cdk/aws-iam/test/principals.test.ts b/packages/@aws-cdk/aws-iam/test/principals.test.ts
index b147c975c05ef..02dc795f0e19d 100644
--- a/packages/@aws-cdk/aws-iam/test/principals.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/principals.test.ts
@@ -46,63 +46,6 @@ test('use of cross-stack role reference does not lead to URLSuffix being exporte
         }
       }
     }
-  });
+  }
+  );
 });
-
-// TODO: may well belong in policy-document.test.ts
-test('can create principal with conditions', () => {
-  // GIVEN
-  const accountId = '012345678910';
-  const app = new App();
-  const stack = new Stack(app, 'Stack');
-
-  // WHEN
-  const conditions = {
-    StringEquals: {
-      "s3:x-amz-acl": [
-        "public-read"
-      ]
-    }
-  };
-  const principal = new iam.AccountPrincipal(accountId).withConditions(conditions);
-  new iam.Role(stack, 'Role', {
-    assumedBy: principal,
-  });
-
-  // THEN
-  app.synth();
-
-  expect(stack).toMatchTemplate({
-    Resources: {
-      Role1ABCC5F0: { // how is this name set? Seems to be used a lot of places so I guess it's the one that'll be generated...
-        Type: "AWS::IAM::Role",
-        Properties: {
-          AssumeRolePolicyDocument: {
-            Statement: [
-              {
-                Action: "sts:AssumeRole",
-                Condition: {
-                  StringEquals: {
-                    "s3:x-amz-acl": [
-                      "public-read"
-                    ]
-                  }
-                },
-                Effect: "Allow",
-                Principal: {
-                  AWS: {
-                    'Fn::Join': [
-                      '',
-                      [ 'arn:', { Ref: 'AWS::Partition' }, `:iam::${accountId}:root` ]
-                    ]
-                  }
-                },
-              }
-            ],
-            Version: "2012-10-17"
-          }
-        }
-      }
-    }
-  });
-});
\ No newline at end of file

From 9a32cd69a77abbffff10b269756a6f9593339bc0 Mon Sep 17 00:00:00 2001
From: Zak Walters <24k@gmx.com>
Date: Wed, 25 Mar 2020 21:16:54 +0000
Subject: [PATCH 08/13] Add README entry (required)

---
 packages/@aws-cdk/aws-iam/README.md | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/packages/@aws-cdk/aws-iam/README.md b/packages/@aws-cdk/aws-iam/README.md
index 59f0db61645c6..346971f5eb1d1 100644
--- a/packages/@aws-cdk/aws-iam/README.md
+++ b/packages/@aws-cdk/aws-iam/README.md
@@ -188,6 +188,16 @@ const role = new iam.Role(this, 'MyRole', {
 });
 ```
 
+The `PrincipalWithConditions` class can be used to add conditions to a
+principal, especially those that don't take a `conditions` parameter in their
+constructor. The `principal.withConditions()` method can be used to create a
+`PrincipalWithConditions` from an existing principal, for example:
+
+```ts
+const principal = new iam.AccountPrincipal('123456789000')
+  .withConditions({ StringEquals: { foo: "baz" } });
+```
+
 ### Parsing JSON Policy Documents
 
 The `PolicyDocument.fromJson` and `PolicyStatement.fromJson` static methods can be used to parse JSON objects. For example:

From b4525929fcf19d0075712400885bdc7158df2f08 Mon Sep 17 00:00:00 2001
From: Zak Walters <24k@gmx.com>
Date: Thu, 2 Apr 2020 22:05:37 +0100
Subject: [PATCH 09/13] Address PR comments

- 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".
---
 .../@aws-cdk/aws-iam/lib/policy-statement.ts  | 19 ++++--
 packages/@aws-cdk/aws-iam/lib/principals.ts   | 66 ++++++++++++++++---
 packages/@aws-cdk/aws-iam/package.json        |  6 --
 3 files changed, 72 insertions(+), 19 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index b152c7f262333..5747e82071c06 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -304,13 +304,22 @@ export enum Effect {
 }
 
 /**
- * Conditions for when an IAM Policy is in effect.
+ * Condition for when an IAM policy is in effect. Maps from the keys in a request's context to
+ * a string value or array of values. See the Conditions interface for more details.
+ */
+export type Condition = Record<string, any>;
+
+/**
+ * Conditions for when an IAM Policy is in effect, specified in the following structure:
  *
- * See https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html
+ * `{ "Operator": { "keyInRequestContext": "value" } }`
+ *
+ * The value can be either a single string value or an array of string values.
+ *
+ * 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 interface Conditions {
-  [operator: string]: { [key: string]: string | string[] };
-}
+export type Conditions = Record<string, Condition>;
 
 /**
  * Interface for creating a policy statement
diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts
index 5364d4310ee6b..e4298df86769a 100644
--- a/packages/@aws-cdk/aws-iam/lib/principals.ts
+++ b/packages/@aws-cdk/aws-iam/lib/principals.ts
@@ -1,6 +1,6 @@
 import * as cdk from '@aws-cdk/core';
 import { Default, RegionInfo } from '@aws-cdk/region-info';
-import { Conditions, PolicyStatement } from './policy-statement';
+import { Condition, Conditions, PolicyStatement } from './policy-statement';
 import { mergePrincipal } from './util';
 
 /**
@@ -78,6 +78,11 @@ export abstract class PrincipalBase implements IPrincipal {
     return JSON.stringify(this.policyFragment.principalJson);
   }
 
+  /**
+   * JSON-ify the principal
+   *
+   * Used when JSON.stringify() is called
+   */
   public toJSON() {
     // Have to implement toJSON() because the default will lead to infinite recursion.
     return this.policyFragment.principalJson;
@@ -104,23 +109,21 @@ export abstract class PrincipalBase implements IPrincipal {
  * https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html
  */
 export class PrincipalWithConditions<PrincipalType extends PrincipalBase> implements IPrincipal {
-  public readonly conditions: Conditions;
   public readonly grantPrincipal: IPrincipal = this;
-  public readonly assumeRoleAction: string = 'sts:AssumeRole';
+  public readonly assumeRoleAction: string = this.principal.assumeRoleAction;
+  private additionalConditions: Conditions;
 
   constructor(
-    public readonly principal: PrincipalType,
+    private readonly principal: PrincipalType,
     conditions: Conditions,
   ) {
-    // Copy the intiial conditions from the principal
-    this.conditions = JSON.parse(JSON.stringify(this.principal.policyFragment.conditions));
-    this.addConditions(conditions);
+    this.additionalConditions = conditions;
   }
 
   /**
    * Add a condition to the principal
    */
-  public addCondition(key: string, value: { [key: string]: string | string[] }) {
+  public addCondition(key: string, value: Condition) {
     const existingValue = this.conditions[key];
     this.conditions[key] = existingValue ? { ...existingValue, ...value } : value;
   }
@@ -137,6 +140,14 @@ export class PrincipalWithConditions<PrincipalType extends PrincipalBase> implem
     });
   }
 
+  /**
+   * The conditions under which the policy is in effect.
+   * See [the IAMdocumentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html).
+   */
+  public get conditions() {
+    return this.mergeConditions(this.principal.policyFragment.conditions, this.additionalConditions);
+  }
+
   public get policyFragment(): PrincipalPolicyFragment {
     return new PrincipalPolicyFragment(this.principal.policyFragment.principalJson, this.conditions);
   }
@@ -149,10 +160,31 @@ export class PrincipalWithConditions<PrincipalType extends PrincipalBase> implem
     return this.principal.toString();
   }
 
+  /**
+   * JSON-ify the principal
+   *
+   * Used when JSON.stringify() is called
+   */
   public toJSON() {
     // Have to implement toJSON() because the default will lead to infinite recursion.
     return this.policyFragment.principalJson;
   }
+
+  private mergeConditions(principalConditions: Conditions, additionalConditions: Conditions): Conditions {
+    const mergedConditions: Conditions = {};
+    Object.entries(principalConditions).forEach(([operator, condition]) => {
+      mergedConditions[operator] = condition;
+    });
+    Object.entries(additionalConditions).forEach(([operator, condition]) => {
+      const existingCondition = mergedConditions[operator];
+      if (!existingCondition) {
+        mergedConditions[operator] = condition;
+      } else {
+        mergedConditions[operator] = { ...existingCondition, ...condition };
+      }
+    });
+    return mergedConditions;
+  }
 }
 
 /**
@@ -164,6 +196,10 @@ export class PrincipalWithConditions<PrincipalType extends PrincipalBase> implem
 export class PrincipalPolicyFragment {
   constructor(
     public readonly principalJson: { [key: string]: string[] },
+    /**
+     * The conditions under which the policy is in effect.
+     * See [the IAMdocumentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html).
+     */
     public readonly conditions: Conditions = {}) {
   }
 }
@@ -284,6 +320,10 @@ export class FederatedPrincipal extends PrincipalBase {
 
   constructor(
     public readonly federated: string,
+    /**
+     * The conditions under which the policy is in effect.
+     * See [the IAMdocumentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html).
+     */
     public readonly conditions: Conditions,
     assumeRoleAction: string = 'sts:AssumeRole') {
     super();
@@ -395,6 +435,11 @@ class StackDependentToken implements cdk.IResolvable {
     return cdk.Token.asString(this);
   }
 
+  /**
+   * JSON-ify the token
+   *
+   * Used when JSON.stringify() is called
+   */
   public toJSON() {
     return `<unresolved-token>`;
   }
@@ -419,6 +464,11 @@ class ServicePrincipalToken implements cdk.IResolvable {
     });
   }
 
+  /**
+   * JSON-ify the token
+   *
+   * Used when JSON.stringify() is called
+   */
   public toJSON() {
     return `<${this.service}>`;
   }
diff --git a/packages/@aws-cdk/aws-iam/package.json b/packages/@aws-cdk/aws-iam/package.json
index 5bd259e74be96..3cba4c68b787f 100644
--- a/packages/@aws-cdk/aws-iam/package.json
+++ b/packages/@aws-cdk/aws-iam/package.json
@@ -124,7 +124,6 @@
       "docs-public-apis:@aws-cdk/aws-iam.CompositePrincipal",
       "docs-public-apis:@aws-cdk/aws-iam.CompositePrincipal.addPrincipals",
       "docs-public-apis:@aws-cdk/aws-iam.FederatedPrincipal",
-      "docs-public-apis:@aws-cdk/aws-iam.FederatedPrincipal.conditions",
       "docs-public-apis:@aws-cdk/aws-iam.FederatedPrincipal.federated",
       "docs-public-apis:@aws-cdk/aws-iam.Group",
       "docs-public-apis:@aws-cdk/aws-iam.LazyRole.roleId",
@@ -146,12 +145,7 @@
       "docs-public-apis:@aws-cdk/aws-iam.PolicyStatement.addResources",
       "docs-public-apis:@aws-cdk/aws-iam.PolicyStatement.toStatementJson",
       "docs-public-apis:@aws-cdk/aws-iam.PolicyStatement.toString",
-      "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",
-      "docs-public-apis:@aws-cdk/aws-iam.PrincipalWithConditions.principal",
-      "docs-public-apis:@aws-cdk/aws-iam.PrincipalWithConditions.toJSON",
       "docs-public-apis:@aws-cdk/aws-iam.ServicePrincipal.service",
       "props-default-doc:@aws-cdk/aws-iam.GrantOnPrincipalOptions.scope",
       "docs-public-apis:@aws-cdk/aws-iam.GroupProps",

From ffc2bdc035d8a3969891affa4662a975a256247f Mon Sep 17 00:00:00 2001
From: Zak Walters <24k@gmx.com>
Date: Thu, 2 Apr 2020 23:31:58 +0100
Subject: [PATCH 10/13] Revert type changes

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.
---
 packages/@aws-cdk/aws-iam/lib/policy-statement.ts | 10 ++--------
 packages/@aws-cdk/aws-iam/lib/principals.ts       |  4 ++--
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index 5747e82071c06..ec54827653b93 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -200,7 +200,7 @@ export class PolicyStatement {
   /**
    * Add a condition to the Policy
    */
-  public addCondition(key: string, value: { [key: string]: string | string[] }) {
+  public addCondition(key: string, value: any) {
     const existingValue = this.condition[key];
     this.condition[key] = existingValue ? { ...existingValue, ...value } : value;
   }
@@ -303,12 +303,6 @@ export enum Effect {
   DENY = 'Deny',
 }
 
-/**
- * Condition for when an IAM policy is in effect. Maps from the keys in a request's context to
- * a string value or array of values. See the Conditions interface for more details.
- */
-export type Condition = Record<string, any>;
-
 /**
  * Conditions for when an IAM Policy is in effect, specified in the following structure:
  *
@@ -319,7 +313,7 @@ export type Condition = Record<string, any>;
  * 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, Condition>;
+export type Conditions = Record<string, any>;
 
 /**
  * Interface for creating a policy statement
diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts
index e4298df86769a..cdded47ca1a6c 100644
--- a/packages/@aws-cdk/aws-iam/lib/principals.ts
+++ b/packages/@aws-cdk/aws-iam/lib/principals.ts
@@ -1,6 +1,6 @@
 import * as cdk from '@aws-cdk/core';
 import { Default, RegionInfo } from '@aws-cdk/region-info';
-import { Condition, Conditions, PolicyStatement } from './policy-statement';
+import { Conditions, PolicyStatement } from './policy-statement';
 import { mergePrincipal } from './util';
 
 /**
@@ -123,7 +123,7 @@ export class PrincipalWithConditions<PrincipalType extends PrincipalBase> implem
   /**
    * Add a condition to the principal
    */
-  public addCondition(key: string, value: Condition) {
+  public addCondition(key: string, value: { [key: string]: any }) {
     const existingValue = this.conditions[key];
     this.conditions[key] = existingValue ? { ...existingValue, ...value } : value;
   }

From e200f8d2e9461d843abae29fd9584552b2cfac27 Mon Sep 17 00:00:00 2001
From: Zak Walters <24k@gmx.com>
Date: Fri, 3 Apr 2020 22:56:35 +0100
Subject: [PATCH 11/13] Remove generics, simplify merging conditions

---
 packages/@aws-cdk/aws-iam/lib/principals.ts | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts
index cdded47ca1a6c..4fa5c32d24972 100644
--- a/packages/@aws-cdk/aws-iam/lib/principals.ts
+++ b/packages/@aws-cdk/aws-iam/lib/principals.ts
@@ -108,13 +108,13 @@ export abstract class PrincipalBase implements IPrincipal {
  * 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 {
+export class PrincipalWithConditions implements IPrincipal {
   public readonly grantPrincipal: IPrincipal = this;
   public readonly assumeRoleAction: string = this.principal.assumeRoleAction;
   private additionalConditions: Conditions;
 
   constructor(
-    private readonly principal: PrincipalType,
+    private readonly principal: IPrincipal,
     conditions: Conditions,
   ) {
     this.additionalConditions = conditions;
@@ -176,12 +176,7 @@ export class PrincipalWithConditions<PrincipalType extends PrincipalBase> implem
       mergedConditions[operator] = condition;
     });
     Object.entries(additionalConditions).forEach(([operator, condition]) => {
-      const existingCondition = mergedConditions[operator];
-      if (!existingCondition) {
-        mergedConditions[operator] = condition;
-      } else {
-        mergedConditions[operator] = { ...existingCondition, ...condition };
-      }
+      mergedConditions[operator] = { ...mergedConditions[operator], ...condition };
     });
     return mergedConditions;
   }

From 9fad94c670221af13dba5628b5ee2cc0b3390ee5 Mon Sep 17 00:00:00 2001
From: Zak Walters <24k@gmx.com>
Date: Sat, 4 Apr 2020 21:43:09 +0100
Subject: [PATCH 12/13] Reintroduce the Condition type

This reverts commit ffc2bdc035d8a3969891affa4662a975a256247f.

Also adds the required ignores for these "breaking" changes to be
accepted, and fixes the one remaining union type.
---
 allowed-breaking-changes.txt                      |  5 +++++
 packages/@aws-cdk/aws-iam/lib/policy-statement.ts | 10 ++++++++--
 packages/@aws-cdk/aws-iam/lib/principals.ts       |  4 ++--
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/allowed-breaking-changes.txt b/allowed-breaking-changes.txt
index a84098fb3ab06..1f78ca52d6422 100644
--- a/allowed-breaking-changes.txt
+++ b/allowed-breaking-changes.txt
@@ -41,3 +41,8 @@ change-return-type:@aws-cdk/aws-lambda-destinations.LambdaDestination.bind
 change-return-type:@aws-cdk/aws-lambda-destinations.SnsDestination.bind
 change-return-type:@aws-cdk/aws-lambda-destinations.SqsDestination.bind
 removed:@aws-cdk/cdk-assets-schema.DockerImageDestination.imageUri
+incompatible-argument:@aws-cdk/aws-iam.FederatedPrincipal.<initializer>
+incompatible-argument:@aws-cdk/aws-iam.PolicyStatement.addCondition
+incompatible-argument:@aws-cdk/aws-iam.PolicyStatement.addConditions
+incompatible-argument:@aws-cdk/aws-iam.PolicyStatement.addFederatedPrincipal
+incompatible-argument:@aws-cdk/aws-iam.PrincipalPolicyFragment.<initializer>
diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index ec54827653b93..711d8a5fa17c7 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -200,7 +200,7 @@ export class PolicyStatement {
   /**
    * Add a condition to the Policy
    */
-  public addCondition(key: string, value: any) {
+  public addCondition(key: string, value: Condition) {
     const existingValue = this.condition[key];
     this.condition[key] = existingValue ? { ...existingValue, ...value } : value;
   }
@@ -303,6 +303,12 @@ export enum Effect {
   DENY = 'Deny',
 }
 
+/**
+ * Condition for when an IAM policy is in effect. Maps from the keys in a request's context to
+ * a string value or array of string values. See the Conditions interface for more details.
+ */
+export type Condition = Record<string, any>;
+
 /**
  * Conditions for when an IAM Policy is in effect, specified in the following structure:
  *
@@ -313,7 +319,7 @@ export enum Effect {
  * 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>;
+export type Conditions = Record<string, Condition>;
 
 /**
  * Interface for creating a policy statement
diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts
index 4fa5c32d24972..c4aa5751554d4 100644
--- a/packages/@aws-cdk/aws-iam/lib/principals.ts
+++ b/packages/@aws-cdk/aws-iam/lib/principals.ts
@@ -1,6 +1,6 @@
 import * as cdk from '@aws-cdk/core';
 import { Default, RegionInfo } from '@aws-cdk/region-info';
-import { Conditions, PolicyStatement } from './policy-statement';
+import { Condition, Conditions, PolicyStatement } from './policy-statement';
 import { mergePrincipal } from './util';
 
 /**
@@ -123,7 +123,7 @@ export class PrincipalWithConditions implements IPrincipal {
   /**
    * Add a condition to the principal
    */
-  public addCondition(key: string, value: { [key: string]: any }) {
+  public addCondition(key: string, value: Condition) {
     const existingValue = this.conditions[key];
     this.conditions[key] = existingValue ? { ...existingValue, ...value } : value;
   }

From 6b2082866ee24a62fc2935fdaefd89e7271f2847 Mon Sep 17 00:00:00 2001
From: Zak Walters <24k@gmx.com>
Date: Mon, 6 Apr 2020 08:59:11 +0100
Subject: [PATCH 13/13] Fix docs typo

---
 packages/@aws-cdk/aws-iam/lib/principals.ts | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts
index c4aa5751554d4..4b52caed90d43 100644
--- a/packages/@aws-cdk/aws-iam/lib/principals.ts
+++ b/packages/@aws-cdk/aws-iam/lib/principals.ts
@@ -142,7 +142,7 @@ export class PrincipalWithConditions implements IPrincipal {
 
   /**
    * The conditions under which the policy is in effect.
-   * See [the IAMdocumentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html).
+   * See [the IAM documentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html).
    */
   public get conditions() {
     return this.mergeConditions(this.principal.policyFragment.conditions, this.additionalConditions);
@@ -193,7 +193,7 @@ export class PrincipalPolicyFragment {
     public readonly principalJson: { [key: string]: string[] },
     /**
      * The conditions under which the policy is in effect.
-     * See [the IAMdocumentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html).
+     * See [the IAM documentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html).
      */
     public readonly conditions: Conditions = {}) {
   }
@@ -317,7 +317,7 @@ export class FederatedPrincipal extends PrincipalBase {
     public readonly federated: string,
     /**
      * The conditions under which the policy is in effect.
-     * See [the IAMdocumentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html).
+     * See [the IAM documentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html).
      */
     public readonly conditions: Conditions,
     assumeRoleAction: string = 'sts:AssumeRole') {