Add runtime check to validate PolicyStatementProps

aws · May 19, 2020 · e4470b2 · e4470b2
1 parent ae838a1
commit e4470b2
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 44 deletions.
diff --git a/packages/@aws-cdk/aws-iam/README.md b/packages/@aws-cdk/aws-iam/README.md
@@ -251,7 +251,7 @@ const newPolicyDocument = PolicyDocument.fromJson(policyDocument);
 
 ### Policy Statement
 
-As Per the doc of [IAM policy grammar](https://docs.aws.amazon.com/en_us/IAM/latest/UserGuide/reference_policies_grammar.html#policies-grammar-bnf), the action and resource blocks are mandatory. Hence, the new policy stament using `PolicyStatementProps` can be created as:
+As per the doc of [IAM policy grammar](https://docs.aws.amazon.com/en_us/IAM/latest/UserGuide/reference_policies_grammar.html#policies-grammar-bnf), the action and resource blocks are mandatory. Hence, the new policy stament using `PolicyStatementProps` can be created as:
 
 ```ts
 const policy = new Policy(stack, 'HelloPolicy', { policyName: 'Default' });

diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -33,8 +33,8 @@ export class PolicyStatement {
       resources: ensureArrayOrUndefined(obj.Resource),
       conditions: obj.Condition,
       effect: obj.Effect,
-      notActions: ensureArrayOrUndefined(obj.NotAction) ?? [],
-      notResources: ensureArrayOrUndefined(obj.NotResource) ?? [],
+      notActions: ensureArrayOrUndefined(obj.NotAction),
+      notResources: ensureArrayOrUndefined(obj.NotResource),
       principals: obj.Principal ? [ new JsonPrincipal(obj.Principal) ] : undefined,
       notPrincipals: obj.NotPrincipal ? [ new JsonPrincipal(obj.NotPrincipal) ] : undefined,
     });
@@ -58,7 +58,7 @@ export class PolicyStatement {
   private readonly condition: { [key: string]: any } = { };
   private principalConditionsJson?: string;
 
-  constructor(props: PolicyStatementProps = {actions: [], resources: []}) {
+  constructor(props: PolicyStatementProps = {}) {
     // Validate actions
     for (const action of [...props.actions || [], ...props.notActions || []]) {
       if (!/^(\*|[a-zA-Z0-9-]+:[a-zA-Z0-9*]+)$/.test(action)) {
@@ -78,6 +78,7 @@ export class PolicyStatement {
     if (props.conditions !== undefined) {
       this.addConditions(props.conditions);
     }
+    this.validateProps(props);
   }
 
   //
@@ -408,6 +409,27 @@ export class PolicyStatement {
     }
     this.addConditions(conditions);
   }
+
+  /**
+   * Validate PolicyStatementProps
+   *
+   * As per [IAM policy grammar](https://docs.aws.amazon.com/en_us/IAM/latest/UserGuide/reference_policies_grammar.html#policies-grammar-bnf),
+   * the action and resource blocks are mandatory.
+   *
+   * @param props
+   */
+  private validateProps(props: PolicyStatementProps) {
+    if (props.conditions || props.effect || props.notPrincipals || props.notResources || props.principals || props.resources || props.sid) {
+      if (!props.actions && !props.notActions) {
+        throw new Error('Action block is mandatory. Either `actions` or `notActions` prop must be specified');
+      }
+    }
+    if (props.actions || props.conditions || props.effect || props.notActions || props.notPrincipals || props.principals || props.sid) {
+      if (!props.resources && !props.notResources) {
+        throw new Error('Resource block is mandatory. Either `resources` or `notResources` prop must be specified');
+      }
+    }
+  }
 }
 
 /**
@@ -462,7 +484,7 @@ export type Conditions = Record<string, Condition>;
 /**
  * Interface for creating a policy statement
  */
-export interface PolicyStatementBase {
+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
@@ -531,44 +553,6 @@ export interface PolicyStatementBase {
   readonly effect?: Effect;
 }
 
-interface PolicyStatementActions extends PolicyStatementBase {
-  /**
-   * List of actions to add to the statement
-   *
-   * @default - no actions
-   */
-  readonly actions: string[];
-}
-
-interface PolicyStatementNotActions extends PolicyStatementBase {
-  /**
-   * List of not actions to add to the statement
-   *
-   * @default - no not-actions
-   */
-  readonly notActions: string[];
-}
-
-interface PolicyStatementResources extends PolicyStatementBase {
-  /**
-   * Resource ARNs to add to the statement
-   *
-   * @default - no resources
-   */
-  readonly resources: string[];
-}
-
-interface PolicyStatementNotResources extends PolicyStatementBase {
-  /**
-   * NotResource ARNs to add to the statement
-   *
-   * @default - no not-resources
-   */
-  readonly notResources: string[];
-}
-
-export type PolicyStatementProps =  (PolicyStatementActions | PolicyStatementNotActions) & (PolicyStatementResources | PolicyStatementNotResources);
-
 function noUndef(x: any): any {
   const ret: any = {};
   for (const [key, value] of Object.entries(x)) {

diff --git a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts
@@ -98,12 +98,29 @@ describe('IAM policy document', () => {
     }).toThrow(/Action 'in:val:id' is invalid/);
   });
 
+  test('Throws with actions and notActions both undefined', () => {
+    expect(() => {
+      new PolicyStatement({
+        principals: [new CanonicalUserPrincipal('abc')],
+        resources: ['*'],
+      });
+    }).toThrow(/Action block is mandatory. Either `actions` or `notActions` prop must be specified/);
+  });
+
+  test('Throws with resources and notResources both undefined', () => {
+    expect(() => {
+      new PolicyStatement({
+        actions: ['abc:def'],
+      });
+    }).toThrow(/Resource block is mandatory. Either `resources` or `notResources` prop must be specified/);
+  });
+
   test('Cannot combine Resources and NotResources', () => {
     expect(() => {
       new PolicyStatement({
         actions: ['abc:def'],
         resources: ['abc'],
-        notResources: ['abc'],
+        notResources: ['def'],
       });
     }).toThrow(/Cannot add 'NotResources' to policy statement if 'Resources' have been added/);
   });
@@ -130,6 +147,19 @@ describe('IAM policy document', () => {
     }).toThrow(/Cannot add 'Principals' to policy statement if 'NotPrincipals' have been added/);
   });
 
+  test('combine NotActions and NotResources', () => {
+    const stack = new Stack();
+    const statement = new PolicyStatement({
+      notActions: ['abc:def'],
+      notResources: ['def'],
+    });
+    expect(stack.resolve(statement.toStatementJson())).toEqual({
+      Effect: 'Allow',
+      NotAction: 'abc:def',
+      NotResource: 'def',
+    });
+  });
+
   test('Permission allows specifying multiple actions upon construction', () => {
     const stack = new Stack();
     const perm = new PolicyStatement();