Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(dynamodb): grant also gives access to indexes #1564

Merged
merged 2 commits into from
Jan 17, 2019
Merged

fix(dynamodb): grant also gives access to indexes #1564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8577c9
fix(dynamodb): grant also gives access to indexes
rix0rrr Jan 17, 2019
bf1c24d
Fix line length linter error
rix0rrr Jan 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/@aws-cdk/aws-dynamodb/lib/table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just always add the index?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this won't throw up IAM diffs to everyone who's already deployed an indexless table.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha! not a bad reason. thanks.

also, wondering perhaps addResource can just ignore undefined, so you don't need to specify noValue. A bit more "cdk-native", no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, although according to the typing addResource() takes a string and not a string | undefined. It would be annoying if a stringified Token would change its type halfway through processing...

(Nothing preventing you from doing that today, but I'm not sure our code would be robust against it).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. No worries.

.addActions(...actions));
}

Expand Down Expand Up @@ -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 {
Expand Down
110 changes: 92 additions & 18 deletions packages/@aws-cdk/aws-dynamodb/test/integ.dynamodb.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,16 @@
"KeyType": "HASH"
}
],
"ProvisionedThroughput": {
"ReadCapacityUnits": 5,
"WriteCapacityUnits": 5
},
"AttributeDefinitions": [
{
"AttributeName": "hashKey",
"AttributeType": "S"
}
]
],
"ProvisionedThroughput": {
"ReadCapacityUnits": 5,
"WriteCapacityUnits": 5
}
}
},
"TableWithGlobalAndLocalSecondaryIndexBC540710": {
Expand All @@ -34,10 +34,6 @@
"KeyType": "RANGE"
}
],
"ProvisionedThroughput": {
"ReadCapacityUnits": 5,
"WriteCapacityUnits": 5
},
"AttributeDefinitions": [
{
"AttributeName": "hashKey",
Expand Down Expand Up @@ -251,6 +247,10 @@
"PointInTimeRecoverySpecification": {
"PointInTimeRecoveryEnabled": true
},
"ProvisionedThroughput": {
"ReadCapacityUnits": 5,
"WriteCapacityUnits": 5
},
"SSESpecification": {
"SSEEnabled": true
},
Expand Down Expand Up @@ -278,10 +278,6 @@
"KeyType": "HASH"
}
],
"ProvisionedThroughput": {
"ReadCapacityUnits": 5,
"WriteCapacityUnits": 5
},
"AttributeDefinitions": [
{
"AttributeName": "hashKey",
Expand Down Expand Up @@ -309,7 +305,11 @@
"WriteCapacityUnits": 5
}
}
]
],
"ProvisionedThroughput": {
"ReadCapacityUnits": 5,
"WriteCapacityUnits": 5
}
}
},
"TableWithLocalSecondaryIndex4DA3D08F": {
Expand All @@ -325,10 +325,6 @@
"KeyType": "RANGE"
}
],
"ProvisionedThroughput": {
"ReadCapacityUnits": 5,
"WriteCapacityUnits": 5
},
"AttributeDefinitions": [
{
"AttributeName": "hashKey",
Expand Down Expand Up @@ -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"
}
]
}
}
Expand Down
5 changes: 5 additions & 0 deletions packages/@aws-cdk/aws-dynamodb/test/integ.dynamodb.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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();
51 changes: 44 additions & 7 deletions packages/@aws-cdk/aws-dynamodb/test/test.dynamodb.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1353,7 +1353,46 @@ 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();
},

},
};

Expand Down Expand Up @@ -1387,12 +1426,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"
Expand Down
17 changes: 14 additions & 3 deletions packages/@aws-cdk/cloudformation-diff/lib/render-intrinsics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 + '}'; }
Expand All @@ -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;
}
Expand All @@ -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]);
}
Expand All @@ -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;
Expand Down
35 changes: 35 additions & 0 deletions packages/@aws-cdk/cloudformation-diff/test/test.render-intrinsics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' }] }),
Expand Down Expand Up @@ -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();
},
};