fix(dynamodb): grant also gives access to indexes

rix0rrr · rix0rrr · commit e8577c90f98d · 2019-01-17T10:50:41.000+01:00
If a table has an index, the `grant()` methods will give permissions to the index as well. Updates the security diff engine to support `AWS::NoValue`. Fixes #1540.
diff --git a/packages/@aws-cdk/aws-dynamodb/lib/table.ts b/packages/@aws-cdk/aws-dynamodb/lib/table.ts
@@ -429,7 +429,7 @@ export class Table extends Construct {
       return;
     }
     principal.addToPolicy(new iam.PolicyStatement()
-      .addResource(this.tableArn)
+      .addResources(this.tableArn, new cdk.Token(() => this.hasIndex ? `${this.tableArn}/index/*` : new cdk.Aws().noValue).toString())
       .addActions(...actions));
   }
 
@@ -622,6 +622,13 @@ export class Table extends Construct {
       })
     });
   }
+
+  /**
+   * Whether this table has indexes
+   */
+  private get hasIndex(): boolean {
+    return this.globalSecondaryIndexes.length + this.localSecondaryIndexes.length > 0;
+  }
 }
 
 export enum AttributeType {
diff --git a/packages/@aws-cdk/aws-dynamodb/test/integ.dynamodb.expected.json b/packages/@aws-cdk/aws-dynamodb/test/integ.dynamodb.expected.json
@@ -9,16 +9,16 @@
             "KeyType": "HASH"
           }
         ],
-        "ProvisionedThroughput": {
-          "ReadCapacityUnits": 5,
-          "WriteCapacityUnits": 5
-        },
         "AttributeDefinitions": [
           {
             "AttributeName": "hashKey",
             "AttributeType": "S"
           }
-        ]
+        ],
+        "ProvisionedThroughput": {
+          "ReadCapacityUnits": 5,
+          "WriteCapacityUnits": 5
+        }
       }
     },
     "TableWithGlobalAndLocalSecondaryIndexBC540710": {
@@ -34,10 +34,6 @@
             "KeyType": "RANGE"
           }
         ],
-        "ProvisionedThroughput": {
-          "ReadCapacityUnits": 5,
-          "WriteCapacityUnits": 5
-        },
         "AttributeDefinitions": [
           {
             "AttributeName": "hashKey",
@@ -251,6 +247,10 @@
         "PointInTimeRecoverySpecification": {
           "PointInTimeRecoveryEnabled": true
         },
+        "ProvisionedThroughput": {
+          "ReadCapacityUnits": 5,
+          "WriteCapacityUnits": 5
+        },
         "SSESpecification": {
           "SSEEnabled": true
         },
@@ -278,10 +278,6 @@
             "KeyType": "HASH"
           }
         ],
-        "ProvisionedThroughput": {
-          "ReadCapacityUnits": 5,
-          "WriteCapacityUnits": 5
-        },
         "AttributeDefinitions": [
           {
             "AttributeName": "hashKey",
@@ -309,7 +305,11 @@
               "WriteCapacityUnits": 5
             }
           }
-        ]
+        ],
+        "ProvisionedThroughput": {
+          "ReadCapacityUnits": 5,
+          "WriteCapacityUnits": 5
+        }
       }
     },
     "TableWithLocalSecondaryIndex4DA3D08F": {
@@ -325,10 +325,6 @@
             "KeyType": "RANGE"
           }
         ],
-        "ProvisionedThroughput": {
-          "ReadCapacityUnits": 5,
-          "WriteCapacityUnits": 5
-        },
         "AttributeDefinitions": [
           {
             "AttributeName": "hashKey",
@@ -360,6 +356,84 @@
               "ProjectionType": "ALL"
             }
           }
+        ],
+        "ProvisionedThroughput": {
+          "ReadCapacityUnits": 5,
+          "WriteCapacityUnits": 5
+        }
+      }
+    },
+    "User00B015A1": {
+      "Type": "AWS::IAM::User"
+    },
+    "UserDefaultPolicy1F97781E": {
+      "Type": "AWS::IAM::Policy",
+      "Properties": {
+        "PolicyDocument": {
+          "Statement": [
+            {
+              "Action": [
+                "dynamodb:BatchGetItem",
+                "dynamodb:GetRecords",
+                "dynamodb:GetShardIterator",
+                "dynamodb:Query",
+                "dynamodb:GetItem",
+                "dynamodb:Scan"
+              ],
+              "Effect": "Allow",
+              "Resource": [
+                {
+                  "Fn::GetAtt": [
+                    "TableCD117FA1",
+                    "Arn"
+                  ]
+                },
+                {
+                  "Ref": "AWS::NoValue"
+                }
+              ]
+            },
+            {
+              "Action": [
+                "dynamodb:BatchGetItem",
+                "dynamodb:GetRecords",
+                "dynamodb:GetShardIterator",
+                "dynamodb:Query",
+                "dynamodb:GetItem",
+                "dynamodb:Scan"
+              ],
+              "Effect": "Allow",
+              "Resource": [
+                {
+                  "Fn::GetAtt": [
+                    "TableWithGlobalAndLocalSecondaryIndexBC540710",
+                    "Arn"
+                  ]
+                },
+                {
+                  "Fn::Join": [
+                    "",
+                    [
+                      {
+                        "Fn::GetAtt": [
+                          "TableWithGlobalAndLocalSecondaryIndexBC540710",
+                          "Arn"
+                        ]
+                      },
+                      "/index/*"
+                    ]
+                  ]
+                }
+              ]
+            }
+          ],
+          "Version": "2012-10-17"
+        },
+        "PolicyName": "UserDefaultPolicy1F97781E",
+        "Users": [
+          {
+            "Ref": "User00B015A1"
+          }
         ]
       }
     }
diff --git a/packages/@aws-cdk/aws-dynamodb/test/integ.dynamodb.ts b/packages/@aws-cdk/aws-dynamodb/test/integ.dynamodb.ts
@@ -1,3 +1,4 @@
+import iam = require('@aws-cdk/aws-iam');
 import { App, Stack } from '@aws-cdk/cdk';
 import { Attribute, AttributeType, ProjectionType, StreamViewType, Table } from '../lib';
 
@@ -118,4 +119,8 @@ tableWithLocalSecondaryIndex.addLocalSecondaryIndex({
   sortKey: LSI_SORT_KEY
 });
 
+const user = new iam.User(stack, 'User');
+table.grantReadData(user);
+tableWithGlobalAndLocalSecondaryIndex.grantReadData(user);
+
 app.run();
diff --git a/packages/@aws-cdk/aws-dynamodb/test/test.dynamodb.ts b/packages/@aws-cdk/aws-dynamodb/test/test.dynamodb.ts
@@ -1353,7 +1353,39 @@ export = {
 
     '"grantFullAccess" allows the principal to perform any action on the table ("*")'(test: Test) {
       testGrant(test, [ '*' ], (p, t) => t.grantFullAccess(p));
-    }
+    },
+
+    'if table has an index grant gives access to the index'(test: Test) {
+      // GIVEN
+      const stack = new Stack();
+
+      const table = new Table(stack, 'my-table');
+      table.addPartitionKey({ name: 'ID', type: AttributeType.String });
+      table.addGlobalSecondaryIndex({ indexName: 'MyIndex', partitionKey: { name: 'Age', type: AttributeType.Number }});
+      const user = new iam.User(stack, 'user');
+
+      // WHEN
+      table.grantReadData(user);
+
+      // THEN
+      expect(stack).to(haveResource('AWS::IAM::Policy', {
+        "PolicyDocument": {
+          "Statement": [
+            {
+              "Action": [ 'dynamodb:BatchGetItem', 'dynamodb:GetRecords', 'dynamodb:GetShardIterator', 'dynamodb:Query', 'dynamodb:GetItem', 'dynamodb:Scan' ],
+              "Effect": "Allow",
+              "Resource": [
+                { "Fn::GetAtt": ["mytable0324D45C", "Arn"] },
+                { "Fn::Join": [ "", [ { "Fn::GetAtt": [ "mytable0324D45C", "Arn" ] }, "/index/*" ] ] }
+              ]
+            }
+          ],
+          "Version": "2012-10-17"
+        },
+      }));
+      test.done();
+    },
+
   },
 };
 
@@ -1387,12 +1419,10 @@ function testGrant(test: Test, expectedActions: string[], invocation: (user: iam
         {
           "Action": action,
           "Effect": "Allow",
-          "Resource": {
-            "Fn::GetAtt": [
-              "mytable0324D45C",
-              "Arn"
-            ]
-          }
+          "Resource": [
+            { "Fn::GetAtt": [ "mytable0324D45C", "Arn" ] },
+            { "Ref" : "AWS::NoValue" }
+          ]
         }
       ],
       "Version": "2012-10-17"
diff --git a/packages/@aws-cdk/cloudformation-diff/lib/render-intrinsics.ts b/packages/@aws-cdk/cloudformation-diff/lib/render-intrinsics.ts
@@ -13,14 +13,18 @@
  *
  * Evaluates Fn::Join directly if the second argument is a literal list of strings.
  *
+ * Removes list and object values evaluating to { Ref: 'AWS::NoValue' }.
+ *
  * For other intrinsics we choose a string representation that CloudFormation
  * cannot actually parse, but is comprehensible to humans.
  */
 export function renderIntrinsics(x: any): any {
   if (Array.isArray(x)) {
-    return x.map(renderIntrinsics);
+    return x.filter(el => !isNoValue(el)).map(renderIntrinsics);
   }
 
+  if (isNoValue(x)) { return undefined; }
+
   const intrinsic = getIntrinsic(x);
   if (intrinsic) {
     if (intrinsic.fn === 'Ref') { return '${' + intrinsic.args + '}'; }
@@ -32,7 +36,9 @@ export function renderIntrinsics(x: any): any {
   if (typeof x === 'object' && x !== null) {
     const ret: any = {};
     for (const [key, value] of Object.entries(x)) {
-      ret[key] = renderIntrinsics(value);
+      if (!isNoValue(value)) {
+        ret[key] = renderIntrinsics(value);
+      }
     }
     return ret;
   }
@@ -41,7 +47,7 @@ export function renderIntrinsics(x: any): any {
 
 function unCloudFormationFnJoin(separator: string, args: any) {
   if (Array.isArray(args)) {
-    return args.map(renderIntrinsics).join(separator);
+    return args.filter(el => !isNoValue(el)).map(renderIntrinsics).join(separator);
   }
   return stringifyIntrinsic('Fn::Join', [separator, args]);
 }
@@ -57,6 +63,11 @@ function getIntrinsic(x: any): Intrinsic | undefined {
   return keys.length === 1 && (keys[0] === 'Ref' || keys[0].startsWith('Fn::')) ? { fn: keys[0], args: x[keys[0]] } : undefined;
 }
 
+function isNoValue(x: any) {
+  const int = getIntrinsic(x);
+  return int && int.fn === 'Ref' && int.args === 'AWS::NoValue';
+}
+
 interface Intrinsic {
   fn: string;
   args: any;
diff --git a/packages/@aws-cdk/cloudformation-diff/test/test.render-intrinsics.ts b/packages/@aws-cdk/cloudformation-diff/test/test.render-intrinsics.ts
@@ -27,6 +27,15 @@ export = {
     test.done();
   },
 
+  'removes AWS::NoValue from Fn::Join'(test: Test) {
+    test.equals(
+      renderIntrinsics({ 'Fn::Join': ['/', ['a', { Ref: 'AWS::NoValue' }, 'b', 'c']] }),
+      'a/b/c'
+    );
+
+    test.done();
+  },
+
   'does not resolve Fn::Join if the second argument is not a list literal'(test: Test) {
     test.equals(
       renderIntrinsics({ 'Fn::Join': ['/', { Ref: 'ListParameter' }] }),
@@ -63,4 +72,30 @@ export = {
     );
     test.done();
   },
+
+  'removes NoValue from object'(test: Test) {
+    test.deepEqual(
+      renderIntrinsics({
+        Deeper1: { Ref: 'SomeLogicalId' },
+        Deeper2: { Ref: 'AWS::NoValue' }
+      }),
+      {
+        Deeper1: '${SomeLogicalId}',
+      }
+    );
+    test.done();
+  },
+
+  'removes NoValue from array'(test: Test) {
+    test.deepEqual(
+      renderIntrinsics([
+        { Ref: 'SomeLogicalId' },
+        { Ref: 'AWS::NoValue' },
+      ]),
+      [
+        '${SomeLogicalId}',
+      ]
+    );
+    test.done();
+  },
 };

Original file line number	Diff line number	Diff line change
`@@ -429,7 +429,7 @@ export class Table extends Construct {`
`429`	`429`	`return;`
`430`	`430`	`}`
`431`	`431`	`principal.addToPolicy(new iam.PolicyStatement()`
`432`		`- .addResource(this.tableArn)`
	`432`	+ .addResources(this.tableArn, new cdk.Token(() => this.hasIndex ? `${this.tableArn}/index/*` : new cdk.Aws().noValue).toString())
`433`	`433`	`.addActions(...actions));`
`434`	`434`	`}`
`435`	`435`
`@@ -622,6 +622,13 @@ export class Table extends Construct {`
`622`	`622`	`})`
`623`	`623`	`});`
`624`	`624`	`}`
	`625`	`+`
	`626`	`+ /**`
	`627`	`+ * Whether this table has indexes`
	`628`	`+ */`
	`629`	`+ private get hasIndex(): boolean {`
	`630`	`+ return this.globalSecondaryIndexes.length + this.localSecondaryIndexes.length > 0;`
	`631`	`+ }`
`625`	`632`	`}`
`626`	`633`
`627`	`634`	`export enum AttributeType {`