From a13af7be381759e0bd67e4b143adb6f9f430cc56 Mon Sep 17 00:00:00 2001
From: Matthew Bonig <matthew.bonig@gmail.com>
Date: Thu, 27 Feb 2020 09:00:03 -0700
Subject: [PATCH 01/14] initial unit tests added

---
 .../@aws-cdk/aws-iam/lib/policy-document.ts   |  10 +
 .../aws-iam/test/policy-statement.test.ts     | 203 ++++++++++++++++++
 2 files changed, 213 insertions(+)
 create mode 100644 packages/@aws-cdk/aws-iam/test/policy-statement.test.ts

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-document.ts b/packages/@aws-cdk/aws-iam/lib/policy-document.ts
index e55cdf910674d..d69d11a17c0fb 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-document.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-document.ts
@@ -24,6 +24,16 @@ export interface PolicyDocumentProps {
  * A PolicyDocument is a collection of statements
  */
 export class PolicyDocument implements cdk.IResolvable {
+
+  /**
+   * Creates a new PolicyDocument based on the object provided.
+   * This will accept an object created from the `.toJSON()` call
+   * @param obj the PolicyDocument in object form.
+   */
+  public static fromJson(obj: any): PolicyDocument {
+    return obj;
+  }
+
   public readonly creationStack: string[];
   private readonly statements = new Array<PolicyStatement>();
   private readonly autoAssignSids: boolean;
diff --git a/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts b/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
new file mode 100644
index 0000000000000..79903ae3d7d8d
--- /dev/null
+++ b/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
@@ -0,0 +1,203 @@
+import '@aws-cdk/assert/jest';
+import { Stack } from '@aws-cdk/core';
+import { PolicyDocument, PolicyStatement } from '../lib';
+
+describe('IAM policy statement', () => {
+
+  describe('from JSON', () => {
+    test('parses with no principal', () => {
+      const stack = new Stack();
+
+      const s = new PolicyStatement();
+      s.addActions('service:action1', 'service:action2');
+      s.addAllResources();
+      s.addCondition('key', { equals: 'value' });
+
+      const doc1 = new PolicyDocument();
+      doc1.addStatements(s);
+
+      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
+
+      const resolvedDoc1 = stack.resolve(doc1);
+      console.log(resolvedDoc1);
+      const resolvedDoc2 = stack.resolve(doc2);
+      console.log(resolvedDoc2);
+      expect(resolvedDoc2).toEqual(resolvedDoc1);
+
+    });
+
+    test('parses a given arnPrincipal', () => {
+      const stack = new Stack();
+
+      const s = new PolicyStatement();
+      s.addActions('service:action1', 'service:action2');
+      s.addAllResources();
+      s.addArnPrincipal('somearn');
+      s.addCondition('key', { equals: 'value' });
+
+      const doc1 = new PolicyDocument();
+      doc1.addStatements(s);
+
+      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
+
+      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
+
+    });
+
+    test('parses a given anyPrincipal', () => {
+      const stack = new Stack();
+
+      const s = new PolicyStatement();
+      s.addActions('service:action1', 'service:action2');
+      s.addAllResources();
+      s.addAnyPrincipal();
+      s.addCondition('key', { equals: 'value' });
+
+      const doc1 = new PolicyDocument();
+      doc1.addStatements(s);
+
+      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
+
+      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
+
+    });
+
+    test('parses an awsAccountPrincipal', () => {
+      const stack = new Stack();
+
+      const s = new PolicyStatement();
+      s.addActions('service:action1', 'service:action2');
+      s.addAllResources();
+      s.addAwsAccountPrincipal('someaccountid');
+      s.addCondition('key', { equals: 'value' });
+
+      const doc1 = new PolicyDocument();
+      doc1.addStatements(s);
+
+      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
+
+      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
+
+    });
+
+    test('parses a given canonicalUserPrincipal', () => {
+      const stack = new Stack();
+
+      const s = new PolicyStatement();
+      s.addActions('service:action1', 'service:action2');
+      s.addAllResources();
+      s.addCanonicalUserPrincipal('someconnonicaluser');
+      s.addCondition('key', { equals: 'value' });
+
+      const doc1 = new PolicyDocument();
+      doc1.addStatements(s);
+
+      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
+
+      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
+
+    });
+
+    test('parses a given federatedPrincipal', () => {
+      const stack = new Stack();
+
+      const s = new PolicyStatement();
+      s.addActions('service:action1', 'service:action2');
+      s.addAllResources();
+      s.addFederatedPrincipal('federated', {});
+      s.addCondition('key', { equals: 'value' });
+
+      const doc1 = new PolicyDocument();
+      doc1.addStatements(s);
+
+      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
+
+      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
+
+    });
+
+    test('parses a given servicePrincipal', () => {
+      const stack = new Stack();
+
+      const s = new PolicyStatement();
+      s.addActions('service:action1', 'service:action2');
+      s.addAllResources();
+      s.addServicePrincipal('serviceprincipal', { conditions: { one: "two" }, region: 'us-west-2' });
+      s.addCondition('key', { equals: 'value' });
+
+      const doc1 = new PolicyDocument();
+      doc1.addStatements(s);
+
+      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
+
+      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
+
+    });
+
+    test('parses with notAction', () => {
+      const stack = new Stack();
+
+      const s = new PolicyStatement();
+      s.addNotActions('service:action3');
+      s.addAllResources();
+
+      const doc1 = new PolicyDocument();
+      doc1.addStatements(s);
+
+      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
+
+      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
+
+    });
+
+    test('parses with notActions', () => {
+      const stack = new Stack();
+
+      const s = new PolicyStatement();
+      s.addNotActions('service:action3', 'service:action4');
+      s.addAllResources();
+
+      const doc1 = new PolicyDocument();
+      doc1.addStatements(s);
+
+      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
+
+      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
+
+    });
+
+    test('parses with notResource', () => {
+      const stack = new Stack();
+
+      const s = new PolicyStatement();
+      s.addActions('service:action3', 'service:action4');
+      s.addNotResources('resource1');
+
+      const doc1 = new PolicyDocument();
+      doc1.addStatements(s);
+
+      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
+
+      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
+
+    });
+
+    test('parses with notResources', () => {
+      const stack = new Stack();
+
+      const s = new PolicyStatement();
+      s.addActions('service:action3', 'service:action4');
+      s.addNotResources('resource1', 'resource2');
+
+      const doc1 = new PolicyDocument();
+      doc1.addStatements(s);
+
+      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
+
+      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
+
+    });
+
+  });
+
+});

From 50a363523d56951667101ea66b86d1f59d3772b4 Mon Sep 17 00:00:00 2001
From: Matthew Bonig <matthew.bonig@gmail.com>
Date: Thu, 27 Feb 2020 09:39:05 -0700
Subject: [PATCH 02/14] filling out tests and Document/Statement

---
 .../@aws-cdk/aws-iam/lib/policy-document.ts   |  6 ++-
 .../@aws-cdk/aws-iam/lib/policy-statement.ts  | 42 +++++++++++++++++++
 .../aws-iam/test/policy-statement.test.ts     | 10 ++---
 3 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-document.ts b/packages/@aws-cdk/aws-iam/lib/policy-document.ts
index d69d11a17c0fb..021f8683f8d90 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-document.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-document.ts
@@ -31,7 +31,11 @@ export class PolicyDocument implements cdk.IResolvable {
    * @param obj the PolicyDocument in object form.
    */
   public static fromJson(obj: any): PolicyDocument {
-    return obj;
+    const newPolicyDocument = new PolicyDocument();
+    if (obj.Statement && Array.isArray(obj.Statement)) {
+      newPolicyDocument.addStatements(...obj.Statement.map((statement: any) => PolicyStatement.fromJson(statement)));
+    }
+    return newPolicyDocument;
   }
 
   public readonly creationStack: string[];
diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index 2c07104874c1a..1113cd21fc62e 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -7,6 +7,48 @@ import { mergePrincipal } from './util';
  * Represents a statement in an IAM policy document.
  */
 export class PolicyStatement {
+  /**
+   * Creates a new PolicyStatement based on the object provided.
+   * This will accept an object created from the `.toJSON()` call
+   * @param obj the PolicyStatement in object form.
+   */
+  static fromJson(obj: any) {
+    const toArray = (field: any) => Array.isArray(field) ? field : field ? [field] : undefined;
+
+    const statement = new PolicyStatement({
+      actions: toArray(obj.Action),
+      resources: toArray(obj.Resource),
+      conditions: obj.Condition,
+      effect: obj.Effect,
+      notActions: toArray(obj.NotAction),
+      notResources: toArray(obj.NotResource)
+    });
+
+    // Since the principals are a more complex object, not just a string or an array of strings,
+    // then just passing them through on the constructor doesn't work.
+    if (obj.Principal && obj.Principal.AWS) {
+      statement.addArnPrincipal(obj.Principal.AWS);
+    }
+
+    if (obj.Principal === "*") {
+      statement.addAnyPrincipal();
+    }
+
+    if (obj.Principal && obj.Principal.CanonicalUser) {
+      statement.addCanonicalUserPrincipal(obj.Principal.CanonicalUser);
+    }
+
+    if (obj.Principal && obj.Principal.Federated) {
+      statement.addFederatedPrincipal(obj.Principal.Federated, {});
+    }
+
+    if (obj.Principal && obj.Principal.Service) {
+      statement.addServicePrincipal(obj.Principal.Service.replace(/.amazonaws.com/i, ''));
+    }
+
+    return statement;
+
+  }
   /**
    * Statement ID for this statement
    */
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 79903ae3d7d8d..bfaae8f6c2a3f 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
@@ -6,6 +6,7 @@ describe('IAM policy statement', () => {
 
   describe('from JSON', () => {
     test('parses with no principal', () => {
+      // given
       const stack = new Stack();
 
       const s = new PolicyStatement();
@@ -16,14 +17,11 @@ describe('IAM policy statement', () => {
       const doc1 = new PolicyDocument();
       doc1.addStatements(s);
 
+      // when
       const doc2 = PolicyDocument.fromJson(doc1.toJSON());
 
-      const resolvedDoc1 = stack.resolve(doc1);
-      console.log(resolvedDoc1);
-      const resolvedDoc2 = stack.resolve(doc2);
-      console.log(resolvedDoc2);
-      expect(resolvedDoc2).toEqual(resolvedDoc1);
-
+      // then
+      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
     });
 
     test('parses a given arnPrincipal', () => {

From f52cf414b992c3521b1ca83b848940d1b1c8ed1c Mon Sep 17 00:00:00 2001
From: Matthew Bonig <matthew.bonig@gmail.com>
Date: Thu, 27 Feb 2020 09:59:28 -0700
Subject: [PATCH 03/14] updating README with example usage

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

diff --git a/packages/@aws-cdk/aws-iam/README.md b/packages/@aws-cdk/aws-iam/README.md
index 13ff25b951e71..06963efaa25e6 100644
--- a/packages/@aws-cdk/aws-iam/README.md
+++ b/packages/@aws-cdk/aws-iam/README.md
@@ -188,6 +188,46 @@ const role = new iam.Role(this, 'MyRole', {
 });
 ```
 
+### Parsing JSON Policy Documents
+
+The `PolicyDocument.fromJson` and `PolicyStatement.fromJSON` static methods can be used to parse JSON objects. For example:
+
+```ts
+const policyDocument = {
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Sid": "FirstStatement",
+      "Effect": "Allow",
+      "Action": ["iam:ChangePassword"],
+      "Resource": "*"
+    },
+    {
+      "Sid": "SecondStatement",
+      "Effect": "Allow",
+      "Action": "s3:ListAllMyBuckets",
+      "Resource": "*"
+    },
+    {
+      "Sid": "ThirdStatement",
+      "Effect": "Allow",
+      "Action": [
+        "s3:List*",
+        "s3:Get*"
+      ],
+      "Resource": [
+        "arn:aws:s3:::confidential-data",
+        "arn:aws:s3:::confidential-data/*"
+      ],
+      "Condition": {"Bool": {"aws:MultiFactorAuthPresent": "true"}}
+    }
+  ]
+};
+
+const newPolicyDocument = PolicyDocument.fromJson(policyDocument);
+
+```
+
 ### Features
 
  * Policy name uniqueness is enforced. If two policies by the same name are attached to the same

From 2e0929076c6cf995a28c919d397c6da53834ce45 Mon Sep 17 00:00:00 2001
From: Matthew Bonig <matthew.bonig@gmail.com>
Date: Thu, 27 Feb 2020 14:21:57 -0700
Subject: [PATCH 04/14] refactoring for readability

---
 .../@aws-cdk/aws-iam/lib/policy-statement.ts  | 40 ++++++++-----------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index 1113cd21fc62e..b8942952ff68d 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -12,38 +12,30 @@ export class PolicyStatement {
    * This will accept an object created from the `.toJSON()` call
    * @param obj the PolicyStatement in object form.
    */
-  static fromJson(obj: any) {
-    const toArray = (field: any) => Array.isArray(field) ? field : field ? [field] : undefined;
+  public static fromJson(obj: any) {
+    const ensureArrayOrUndefined = (field: any) => field === undefined ? field : [].concat(field);
 
     const statement = new PolicyStatement({
-      actions: toArray(obj.Action),
-      resources: toArray(obj.Resource),
+      actions: ensureArrayOrUndefined(obj.Action),
+      resources: ensureArrayOrUndefined(obj.Resource),
       conditions: obj.Condition,
       effect: obj.Effect,
-      notActions: toArray(obj.NotAction),
-      notResources: toArray(obj.NotResource)
+      notActions: ensureArrayOrUndefined(obj.NotAction),
+      notResources: ensureArrayOrUndefined(obj.NotResource)
     });
 
+    statement.sid = obj.Sid;
+
     // Since the principals are a more complex object, not just a string or an array of strings,
     // then just passing them through on the constructor doesn't work.
-    if (obj.Principal && obj.Principal.AWS) {
-      statement.addArnPrincipal(obj.Principal.AWS);
-    }
-
-    if (obj.Principal === "*") {
-      statement.addAnyPrincipal();
-    }
-
-    if (obj.Principal && obj.Principal.CanonicalUser) {
-      statement.addCanonicalUserPrincipal(obj.Principal.CanonicalUser);
-    }
-
-    if (obj.Principal && obj.Principal.Federated) {
-      statement.addFederatedPrincipal(obj.Principal.Federated, {});
-    }
-
-    if (obj.Principal && obj.Principal.Service) {
-      statement.addServicePrincipal(obj.Principal.Service.replace(/.amazonaws.com/i, ''));
+    if (obj.Principal) {
+      /* tslint:disable:no-unused-expression */
+      obj.Principal === "*" && statement.addAnyPrincipal();
+      obj.Principal.AWS && statement.addArnPrincipal(obj.Principal.AWS);
+      obj.Principal.CanonicalUser && statement.addCanonicalUserPrincipal(obj.Principal.CanonicalUser);
+      obj.Principal.Federated && statement.addFederatedPrincipal(obj.Principal.Federated, {});
+      obj.Principal.Service && statement.addServicePrincipal(obj.Principal.Service.replace(/.amazonaws.com/i, ''));
+      /* tslint:enable:no-unused-expression */
     }
 
     return statement;

From 97a63b3e7b771932aed9ac3533faaa77d16ed0e7 Mon Sep 17 00:00:00 2001
From: Matthew Bonig <matthew.bonig@gmail.com>
Date: Thu, 27 Feb 2020 14:22:49 -0700
Subject: [PATCH 05/14] fixing casing on the .fromJson in the readme

Co-Authored-By: Elad Ben-Israel <benisrae@amazon.com>
---
 packages/@aws-cdk/aws-iam/README.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/@aws-cdk/aws-iam/README.md b/packages/@aws-cdk/aws-iam/README.md
index 06963efaa25e6..59f0db61645c6 100644
--- a/packages/@aws-cdk/aws-iam/README.md
+++ b/packages/@aws-cdk/aws-iam/README.md
@@ -190,7 +190,7 @@ const role = new iam.Role(this, 'MyRole', {
 
 ### Parsing JSON Policy Documents
 
-The `PolicyDocument.fromJson` and `PolicyStatement.fromJSON` static methods can be used to parse JSON objects. For example:
+The `PolicyDocument.fromJson` and `PolicyStatement.fromJson` static methods can be used to parse JSON objects. For example:
 
 ```ts
 const policyDocument = {

From 56e47084dc796302e408dd447ae424e329c246e4 Mon Sep 17 00:00:00 2001
From: Matthew Bonig <matthew.bonig@gmail.com>
Date: Thu, 27 Feb 2020 14:39:53 -0700
Subject: [PATCH 06/14] adding kitchen sink test for a little more coverage.

---
 .../@aws-cdk/aws-iam/lib/policy-statement.ts  | 10 ++++-
 .../aws-iam/test/policy-statement.test.ts     | 41 +++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index b8942952ff68d..3d47948998bbc 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -13,7 +13,15 @@ export class PolicyStatement {
    * @param obj the PolicyStatement in object form.
    */
   public static fromJson(obj: any) {
-    const ensureArrayOrUndefined = (field: any) => field === undefined ? field : [].concat(field);
+    const ensureArrayOrUndefined = (field: any) => {
+      if (field === undefined) {
+        return undefined;
+      }
+      if (typeof (field) !== "string" && !Array.isArray(field)) {
+        throw new Error("Fields must be either a string or an array of strings");
+      }
+      return Array.isArray(field) ? field : [field];
+    };
 
     const statement = new PolicyStatement({
       actions: ensureArrayOrUndefined(obj.Action),
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 bfaae8f6c2a3f..34035cda07290 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
@@ -196,6 +196,47 @@ describe('IAM policy statement', () => {
 
     });
 
+    test('the kitchen sink', () => {
+      const stack = new Stack();
+
+      /* tslint:disable */
+      const policyDocument = {
+        "Version": "2012-10-17",
+        "Statement": [
+          {
+            "Sid": "FirstStatement",
+            "Effect": "Allow",
+            "Action": "iam:ChangePassword",
+            "Resource": "*"
+          },
+          {
+            "Sid": "SecondStatement",
+            "Effect": "Allow",
+            "Action": "s3:ListAllMyBuckets",
+            "Resource": "*"
+          },
+          {
+            "Sid": "ThirdStatement",
+            "Effect": "Allow",
+            "Action": [
+              "s3:List*",
+              "s3:Get*"
+            ],
+            "Resource": [
+              "arn:aws:s3:::confidential-data",
+              "arn:aws:s3:::confidential-data/*"
+            ],
+            "Condition": {"Bool": {"aws:MultiFactorAuthPresent": "true"}}
+          }
+        ]
+      };
+      /* tslint:enable */
+
+      const doc = PolicyDocument.fromJson(policyDocument);
+
+      expect(stack.resolve(doc)).toEqual(policyDocument);
+    });
+
   });
 
 });

From e31ea464ac678e6f5c2d0197ae29e68d0c04e7a7 Mon Sep 17 00:00:00 2001
From: Matthew Bonig <matthew.bonig@gmail.com>
Date: Thu, 27 Feb 2020 14:55:59 -0700
Subject: [PATCH 07/14] adding additional tests

---
 packages/@aws-cdk/aws-iam/lib/policy-statement.ts |  3 +++
 .../aws-iam/test/policy-statement.test.ts         | 15 +++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index 3d47948998bbc..83c223ddbd957 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -20,6 +20,9 @@ export class PolicyStatement {
       if (typeof (field) !== "string" && !Array.isArray(field)) {
         throw new Error("Fields must be either a string or an array of strings");
       }
+      if (Array.isArray(field) && !field.reduce((acc: boolean, f: any) => (acc && typeof (f) === "string"), true)) {
+        throw new Error("Fields must be either a string or an array of strings");
+      }
       return Array.isArray(field) ? field : [field];
     };
 
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 34035cda07290..8496a865f228f 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
@@ -237,6 +237,21 @@ describe('IAM policy statement', () => {
       expect(stack.resolve(doc)).toEqual(policyDocument);
     });
 
+    test('throws error with field data being object', () => {
+      expect(() => {
+        PolicyStatement.fromJson({
+          Action: {}
+        });
+      }).toThrow(/Fields must be either a string or an array of strings/);
+    });
+
+    test('throws error with field data being object', () => {
+      expect(() => {
+        PolicyStatement.fromJson({
+          Action: [{}]
+        });
+      }).toThrow(/Fields must be either a string or an array of strings/);
+    });
   });
 
 });

From fca47070cd52031a8ac1d53dedf71a1180d1cfb6 Mon Sep 17 00:00:00 2001
From: Matthew Bonig <matthew.bonig@gmail.com>
Date: Fri, 28 Feb 2020 08:46:34 -0700
Subject: [PATCH 08/14] adding test to explicitly check for the Statement being
 an array

---
 packages/@aws-cdk/aws-iam/lib/policy-document.ts       |  2 ++
 packages/@aws-cdk/aws-iam/test/policy-document.test.ts | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-document.ts b/packages/@aws-cdk/aws-iam/lib/policy-document.ts
index 021f8683f8d90..76b515dc0fce4 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-document.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-document.ts
@@ -34,6 +34,8 @@ export class PolicyDocument implements cdk.IResolvable {
     const newPolicyDocument = new PolicyDocument();
     if (obj.Statement && Array.isArray(obj.Statement)) {
       newPolicyDocument.addStatements(...obj.Statement.map((statement: any) => PolicyStatement.fromJson(statement)));
+    } else {
+      throw new Error('Statement must be an array');
     }
     return newPolicyDocument;
   }
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 3f5388e6d75de..c5f7e2e5e7017 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
@@ -572,4 +572,14 @@ describe('IAM polocy document', () => {
 
     expect(stack.resolve(doc1)).toEqual(stack.resolve(doc2));
   });
+
+  describe('fromJson', () => {
+    test("throws error when Statement isn't an array", () => {
+      expect(() => {
+        PolicyDocument.fromJson({
+          Statement: ''
+        });
+      }).toThrow(/Statement must be an array/);
+    });
+  });
 });

From 0c4da491ae56071ae61ecd6aebd01a8af5c9282e Mon Sep 17 00:00:00 2001
From: Matthew Bonig <matthew.bonig@gmail.com>
Date: Mon, 2 Mar 2020 08:20:31 -0700
Subject: [PATCH 09/14] better failure order

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

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-document.ts b/packages/@aws-cdk/aws-iam/lib/policy-document.ts
index 76b515dc0fce4..803b314cc4b7f 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-document.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-document.ts
@@ -32,11 +32,11 @@ export class PolicyDocument implements cdk.IResolvable {
    */
   public static fromJson(obj: any): PolicyDocument {
     const newPolicyDocument = new PolicyDocument();
-    if (obj.Statement && Array.isArray(obj.Statement)) {
-      newPolicyDocument.addStatements(...obj.Statement.map((statement: any) => PolicyStatement.fromJson(statement)));
-    } else {
+    const statement = obj.Statement ?? [];
+    if (statement && !Array.isArray(statement)) {
       throw new Error('Statement must be an array');
     }
+    newPolicyDocument.addStatements(...obj.Statement.map((s: any) => PolicyStatement.fromJson(s)));
     return newPolicyDocument;
   }
 

From 00001d38e867f92defedbab36ea9430d99f5f311 Mon Sep 17 00:00:00 2001
From: Matthew Bonig <matthew.bonig@gmail.com>
Date: Mon, 2 Mar 2020 08:30:02 -0700
Subject: [PATCH 10/14] reworking field logic

---
 packages/@aws-cdk/aws-iam/lib/policy-statement.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index 83c223ddbd957..fdbd8970149f3 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -20,7 +20,7 @@ export class PolicyStatement {
       if (typeof (field) !== "string" && !Array.isArray(field)) {
         throw new Error("Fields must be either a string or an array of strings");
       }
-      if (Array.isArray(field) && !field.reduce((acc: boolean, f: any) => (acc && typeof (f) === "string"), true)) {
+      if (Array.isArray(field) && !!field.find((f: any) => typeof (f) !== "string")) {
         throw new Error("Fields must be either a string or an array of strings");
       }
       return Array.isArray(field) ? field : [field];

From 347c63e51cec5d8da29422f46089c48f0c040702 Mon Sep 17 00:00:00 2001
From: Matthew Bonig <matthew.bonig@gmail.com>
Date: Mon, 2 Mar 2020 08:38:55 -0700
Subject: [PATCH 11/14] pulled ensure function to module level

fixing bad test
---
 .../@aws-cdk/aws-iam/lib/policy-statement.ts  | 27 ++++++++++---------
 .../aws-iam/test/policy-document.test.ts      |  2 +-
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index fdbd8970149f3..520f370055adf 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -3,29 +3,30 @@ import { AccountPrincipal, AccountRootPrincipal, Anyone, ArnPrincipal, Canonical
   FederatedPrincipal, IPrincipal, ServicePrincipal, ServicePrincipalOpts } from './principals';
 import { mergePrincipal } from './util';
 
+const ensureArrayOrUndefined = (field: any) => {
+  if (field === undefined) {
+    return undefined;
+  }
+  if (typeof (field) !== "string" && !Array.isArray(field)) {
+    throw new Error("Fields must be either a string or an array of strings");
+  }
+  if (Array.isArray(field) && !!field.find((f: any) => typeof (f) !== "string")) {
+    throw new Error("Fields must be either a string or an array of strings");
+  }
+  return Array.isArray(field) ? field : [field];
+};
+
 /**
  * Represents a statement in an IAM policy document.
  */
 export class PolicyStatement {
+
   /**
    * Creates a new PolicyStatement based on the object provided.
    * This will accept an object created from the `.toJSON()` call
    * @param obj the PolicyStatement in object form.
    */
   public static fromJson(obj: any) {
-    const ensureArrayOrUndefined = (field: any) => {
-      if (field === undefined) {
-        return undefined;
-      }
-      if (typeof (field) !== "string" && !Array.isArray(field)) {
-        throw new Error("Fields must be either a string or an array of strings");
-      }
-      if (Array.isArray(field) && !!field.find((f: any) => typeof (f) !== "string")) {
-        throw new Error("Fields must be either a string or an array of strings");
-      }
-      return Array.isArray(field) ? field : [field];
-    };
-
     const statement = new PolicyStatement({
       actions: ensureArrayOrUndefined(obj.Action),
       resources: ensureArrayOrUndefined(obj.Resource),
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 c5f7e2e5e7017..e5fe969d80e3c 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
@@ -577,7 +577,7 @@ describe('IAM polocy document', () => {
     test("throws error when Statement isn't an array", () => {
       expect(() => {
         PolicyDocument.fromJson({
-          Statement: ''
+          Statement: 'asdf'
         });
       }).toThrow(/Statement must be an array/);
     });

From 89127a1652713629c9118c7c30bfe9b47066b223 Mon Sep 17 00:00:00 2001
From: Elad Ben-Israel <benisrae@amazon.com>
Date: Tue, 10 Mar 2020 10:12:39 +0200
Subject: [PATCH 12/14] simplify principal parsing

Use a "raw" principal class that implements IPrincipal in order to simplify parsing of Principal and NotPrincipal in fromJson.
---
 .../@aws-cdk/aws-iam/lib/policy-statement.ts  | 55 ++++++++++++-------
 packages/@aws-cdk/aws-iam/lib/util.ts         |  4 +-
 2 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index 520f370055adf..b82a637e76ab3 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -1,6 +1,6 @@
 import * as cdk from '@aws-cdk/core';
 import { AccountPrincipal, AccountRootPrincipal, Anyone, ArnPrincipal, CanonicalUserPrincipal,
-  FederatedPrincipal, IPrincipal, ServicePrincipal, ServicePrincipalOpts } from './principals';
+  FederatedPrincipal, IPrincipal, PrincipalBase, PrincipalPolicyFragment, ServicePrincipal, ServicePrincipalOpts } from './principals';
 import { mergePrincipal } from './util';
 
 const ensureArrayOrUndefined = (field: any) => {
@@ -27,32 +27,19 @@ export class PolicyStatement {
    * @param obj the PolicyStatement in object form.
    */
   public static fromJson(obj: any) {
-    const statement = new PolicyStatement({
+    return new PolicyStatement({
+      sid: obj.Sid,
       actions: ensureArrayOrUndefined(obj.Action),
       resources: ensureArrayOrUndefined(obj.Resource),
       conditions: obj.Condition,
       effect: obj.Effect,
       notActions: ensureArrayOrUndefined(obj.NotAction),
-      notResources: ensureArrayOrUndefined(obj.NotResource)
+      notResources: ensureArrayOrUndefined(obj.NotResource),
+      principals: obj.Principal ? [ new JsonPrincipal(obj.Principal) ] : undefined,
+      notPrincipals: obj.NotPrincipal ? [ new JsonPrincipal(obj.NotPrincipal) ] : undefined
     });
-
-    statement.sid = obj.Sid;
-
-    // Since the principals are a more complex object, not just a string or an array of strings,
-    // then just passing them through on the constructor doesn't work.
-    if (obj.Principal) {
-      /* tslint:disable:no-unused-expression */
-      obj.Principal === "*" && statement.addAnyPrincipal();
-      obj.Principal.AWS && statement.addArnPrincipal(obj.Principal.AWS);
-      obj.Principal.CanonicalUser && statement.addCanonicalUserPrincipal(obj.Principal.CanonicalUser);
-      obj.Principal.Federated && statement.addFederatedPrincipal(obj.Principal.Federated, {});
-      obj.Principal.Service && statement.addServicePrincipal(obj.Principal.Service.replace(/.amazonaws.com/i, ''));
-      /* tslint:enable:no-unused-expression */
-    }
-
-    return statement;
-
   }
+
   /**
    * Statement ID for this statement
    */
@@ -75,6 +62,7 @@ export class PolicyStatement {
       }
     }
 
+    this.sid = props.sid;
     this.effect = props.effect || Effect.ALLOW;
 
     this.addActions(...props.actions || []);
@@ -318,6 +306,15 @@ export enum Effect {
  * Interface for creating a policy statement
  */
 export interface PolicyStatementProps {
+  /**
+   * The Sid (statement ID) is an optional identifier that you provide for the
+   * policy statement. You can assign a Sid value to each statement in a
+   * statement array. In services that let you specify an ID element, such as
+   * SQS and SNS, the Sid value is just a sub-ID of the policy document's ID. In
+   * IAM, the Sid value must be unique within a JSON policy.
+   */
+  readonly sid?: string;
+
   /**
    * List of actions to add to the statement
    *
@@ -384,3 +381,21 @@ function noUndef(x: any): any {
   }
   return ret;
 }
+
+class JsonPrincipal extends PrincipalBase {
+  public readonly policyFragment: PrincipalPolicyFragment;
+
+  constructor(json: any = { }) {
+    super();
+
+    // special case: if principal is a string, turn it into an "AWS" principal
+    if (typeof(json) === 'string') {
+      json = { AWS: json };
+    }
+
+    this.policyFragment = {
+      principalJson: json,
+      conditions: []
+    };
+  }
+}
\ No newline at end of file
diff --git a/packages/@aws-cdk/aws-iam/lib/util.ts b/packages/@aws-cdk/aws-iam/lib/util.ts
index cd1a29cd7d7c3..14aa2fa58c7de 100644
--- a/packages/@aws-cdk/aws-iam/lib/util.ts
+++ b/packages/@aws-cdk/aws-iam/lib/util.ts
@@ -69,9 +69,9 @@ export function mergePrincipal(target: { [key: string]: string[] }, source: { [k
   for (const key of Object.keys(source)) {
     target[key] = target[key] || [];
 
-    const value = source[key];
+    let value = source[key];
     if (!Array.isArray(value)) {
-      throw new Error(`Principal value must be an array (it will be normalized later): ${value}`);
+      value = [ value ];
     }
 
     target[key].push(...value);

From 756c618f0cd05e8e854880c92ab0145d7c0997e9 Mon Sep 17 00:00:00 2001
From: Matthew Bonig <matthew.bonig@gmail.com>
Date: Tue, 10 Mar 2020 08:45:07 -0600
Subject: [PATCH 13/14] adding notPrinciapl test

---
 .../@aws-cdk/aws-iam/lib/policy-statement.ts  |  4 +-
 .../aws-iam/test/policy-document.test.ts      |  7 +-
 .../aws-iam/test/policy-statement.test.ts     | 80 +------------------
 3 files changed, 11 insertions(+), 80 deletions(-)

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
index f8b0a7dc37d6b..01d0c3934909b 100644
--- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
+++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -313,6 +313,8 @@ export interface PolicyStatementProps {
    * statement array. In services that let you specify an ID element, such as
    * SQS and SNS, the Sid value is just a sub-ID of the policy document's ID. In
    * IAM, the Sid value must be unique within a JSON policy.
+   *
+   * @default - no sid
    */
   readonly sid?: string;
 
@@ -399,4 +401,4 @@ class JsonPrincipal extends PrincipalBase {
       conditions: []
     };
   }
-}
\ No newline at end of file
+}
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 58651727c0fb4..a1cb97fed9821 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
@@ -580,20 +580,21 @@ describe('IAM polocy document', () => {
           Statement: 'asdf'
         });
       }).toThrow(/Statement must be an array/);
+    });
   });
 
-      
   test('adding another condition with the same operator does not delete the original', () => {
     const stack = new Stack();
 
     const p = new PolicyStatement();
 
-    p.addCondition('StringEquals', { 'kms:ViaService': 'service' });
+    p.addCondition('StringEquals', {'kms:ViaService': 'service'});
 
     p.addAccountCondition('12221121221');
 
     expect(stack.resolve(p.toStatementJson())).toEqual({
       Effect: 'Allow',
-      Condition: { StringEquals: { 'kms:ViaService': 'service', 'sts:ExternalId': '12221121221' } }
+      Condition: {StringEquals: {'kms:ViaService': 'service', 'sts:ExternalId': '12221121221'}}
+    });
   });
 });
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 8496a865f228f..217c89b76132f 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
@@ -1,6 +1,6 @@
 import '@aws-cdk/assert/jest';
 import { Stack } from '@aws-cdk/core';
-import { PolicyDocument, PolicyStatement } from '../lib';
+import {AnyPrincipal, PolicyDocument, PolicyStatement} from '../lib';
 
 describe('IAM policy statement', () => {
 
@@ -24,7 +24,7 @@ describe('IAM policy statement', () => {
       expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
     });
 
-    test('parses a given arnPrincipal', () => {
+    test('parses a given Principal', () => {
       const stack = new Stack();
 
       const s = new PolicyStatement();
@@ -42,85 +42,13 @@ describe('IAM policy statement', () => {
 
     });
 
-    test('parses a given anyPrincipal', () => {
-      const stack = new Stack();
-
-      const s = new PolicyStatement();
-      s.addActions('service:action1', 'service:action2');
-      s.addAllResources();
-      s.addAnyPrincipal();
-      s.addCondition('key', { equals: 'value' });
-
-      const doc1 = new PolicyDocument();
-      doc1.addStatements(s);
-
-      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
-
-      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
-
-    });
-
-    test('parses an awsAccountPrincipal', () => {
-      const stack = new Stack();
-
-      const s = new PolicyStatement();
-      s.addActions('service:action1', 'service:action2');
-      s.addAllResources();
-      s.addAwsAccountPrincipal('someaccountid');
-      s.addCondition('key', { equals: 'value' });
-
-      const doc1 = new PolicyDocument();
-      doc1.addStatements(s);
-
-      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
-
-      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
-
-    });
-
-    test('parses a given canonicalUserPrincipal', () => {
-      const stack = new Stack();
-
-      const s = new PolicyStatement();
-      s.addActions('service:action1', 'service:action2');
-      s.addAllResources();
-      s.addCanonicalUserPrincipal('someconnonicaluser');
-      s.addCondition('key', { equals: 'value' });
-
-      const doc1 = new PolicyDocument();
-      doc1.addStatements(s);
-
-      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
-
-      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
-
-    });
-
-    test('parses a given federatedPrincipal', () => {
-      const stack = new Stack();
-
-      const s = new PolicyStatement();
-      s.addActions('service:action1', 'service:action2');
-      s.addAllResources();
-      s.addFederatedPrincipal('federated', {});
-      s.addCondition('key', { equals: 'value' });
-
-      const doc1 = new PolicyDocument();
-      doc1.addStatements(s);
-
-      const doc2 = PolicyDocument.fromJson(doc1.toJSON());
-
-      expect(stack.resolve(doc2)).toEqual(stack.resolve(doc1));
-
-    });
-
-    test('parses a given servicePrincipal', () => {
+    test('parses a given arnPrincipal', () => {
       const stack = new Stack();
 
       const s = new PolicyStatement();
       s.addActions('service:action1', 'service:action2');
       s.addAllResources();
-      s.addServicePrincipal('serviceprincipal', { conditions: { one: "two" }, region: 'us-west-2' });
+      s.addNotPrincipals(new AnyPrincipal());
       s.addCondition('key', { equals: 'value' });
 
       const doc1 = new PolicyDocument();

From 76e027242d13cc50d676b2ae1da11e07448d58e1 Mon Sep 17 00:00:00 2001
From: Matthew Bonig <matthew.bonig@gmail.com>
Date: Tue, 10 Mar 2020 08:52:19 -0600
Subject: [PATCH 14/14] fixing test name

---
 packages/@aws-cdk/aws-iam/test/policy-statement.test.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 217c89b76132f..11207b7ed541b 100644
--- a/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
+++ b/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts
@@ -42,7 +42,7 @@ describe('IAM policy statement', () => {
 
     });
 
-    test('parses a given arnPrincipal', () => {
+    test('parses a given notPrincipal', () => {
       const stack = new Stack();
 
       const s = new PolicyStatement();