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.

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

feat(aws-sqs): allow direct grants from a Queue #1004

Merged
merged 3 commits into from
Oct 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions packages/@aws-cdk/aws-sqs/lib/perms.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export const QUEUE_GET_ACTIONS = [
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to expose these I think. Might as well put them in the queue.ts file and not export them.

"sqs:ReceiveMessage",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should have DeleteMessage(s) and SetVisibilityTimeout (and maybe others?) as well I think. Those are common receiver actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I based this off the templated permissions the user expects in the console and the examples that the IAM documentation states are needed (doesn't mean it's right in this case!). Maybe more detailed grants might work for those cases?

];

export const QUEUE_CONSUME_ACTIONS = [
"sqs:ChangeMessageVisibility",
"sqs:DeleteMessage",
];

export const QUEUE_PUT_ACTIONS = [
"sqs:SendMessage",
];
60 changes: 60 additions & 0 deletions packages/@aws-cdk/aws-sqs/lib/queue.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import iam = require('@aws-cdk/aws-iam');
import kms = require('@aws-cdk/aws-kms');
import cdk = require('@aws-cdk/cdk');
import perms = require('./perms');
import { QueueRef } from './queue-ref';
import { cloudformation } from './sqs.generated';
import { validateProps } from './validate-props';
Expand Down Expand Up @@ -275,6 +277,64 @@ export class Queue extends QueueRef {
}
}

/**
* Grant permissions to consume messages from a queue
*
* This will grant the following permissions:
*
* - sqs:ChangeMessageVisibility
* - sqs:DeleteMessage
* - sqs:ReceiveMessage
*
* @param identity Principal to grant consume rights to
*/
public grantConsumeMessages(identity?: iam.IPrincipal) {
this.grant(identity, perms.QUEUE_GET_ACTIONS.concat(perms.QUEUE_CONSUME_ACTIONS));
}

/**
* Grant access to receive messages from a queue to
* the given identity.
*
* This will grant sqs:ReceiveMessage
*
* @param identity Principal to grant receive rights to
*/
public grantReceiveMessages(identity?: iam.IPrincipal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In my head this was always called grantConsume (since you're consuming from the queue). What do you think? More accurate or too pretentious?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went for Receive to match AWS documentation nomenclature but open to opinion here. It might well be that both names are right and that having both to enable the user to easily find it might be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

"Consume" has the advantage (in my mind) that it's very clear that you'll be removing messages from the queue as well.

Maybe we can squash both discussions by exposing both grantConsume (receive+delete+setvisibilitytimeout) and grantReceive (just receive).

But I think that will be a compromise to appease the naysayers. I don't see a realistic scenario in which you ever want to give someone just receiveMessage permissions. Maybe a snooper, but not an active component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger that - I'll change accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should have a public docstring

this.grant(identity, perms.QUEUE_GET_ACTIONS);
}

/**
* Grant access to send messages to a queue to the
* given identity.
*
* This will grant sqs:SendMessage
*
* @param identity Principal to grant send rights to
*/
public grantSendMessages(identity?: iam.IPrincipal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should have a public docstring

this.grant(identity, perms.QUEUE_PUT_ACTIONS);
}

/**
* Grant the actions defined in queueActions
* to the identity Principal given.
*
* @param identity Principal to grant right to
* @param queueActions The actions to grant
*/
private grant(identity: iam.IPrincipal | undefined,
Copy link
Contributor

@rix0rrr rix0rrr Oct 24, 2018

Choose a reason for hiding this comment

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

Might have a docstring as well, why not.

queueActions: string[]) {

if (!identity) {
return;
}

identity.addToPolicy(new iam.PolicyStatement()
.addResource(this.queueArn)
.addActions(...queueActions));
}

/**
* Look at the props, see if the FIFO props agree, and return the correct subset of props
*/
Expand Down
90 changes: 89 additions & 1 deletion packages/@aws-cdk/aws-sqs/test/test.sqs.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect, haveResource } from '@aws-cdk/assert';
import { ArnPrincipal, PolicyStatement } from '@aws-cdk/aws-iam';
import { ArnPrincipal, PolicyStatement, Role, ServicePrincipal } from '@aws-cdk/aws-iam';
import kms = require('@aws-cdk/aws-kms');
import s3 = require('@aws-cdk/aws-s3');
import { resolve, Stack } from '@aws-cdk/cdk';
Expand Down Expand Up @@ -113,6 +113,94 @@ export = {
test.done();
},

'iam': {
'grants permission to consume messages'(test: Test) {
const stack = new Stack();
const role = new Role(stack, 'Role', { assumedBy: new ServicePrincipal('lambda.amazonaws.com') });
const queue = new sqs.Queue(stack, 'Queue');
queue.grantConsumeMessages(role);

expect(stack).to(haveResource('AWS::IAM::Policy', {
"PolicyDocument": {
"Statement": [
{
"Action": [
"sqs:ReceiveMessage",
"sqs:ChangeMessageVisibility",
"sqs:DeleteMessage"
],
"Effect": "Allow",
"Resource": {
"Fn::GetAtt":
[
"Queue4A7E3555",
"Arn"
]
}
}
]
}
}));

test.done();
},

'grants permission to receive messages'(test: Test) {
const stack = new Stack();
const role = new Role(stack, 'Role', { assumedBy: new ServicePrincipal('lambda.amazonaws.com') });
const queue = new sqs.Queue(stack, 'Queue');
queue.grantReceiveMessages(role);

expect(stack).to(haveResource('AWS::IAM::Policy', {
"PolicyDocument": {
"Statement": [
{
"Action": "sqs:ReceiveMessage",
"Effect": "Allow",
"Resource": {
"Fn::GetAtt":
[
"Queue4A7E3555",
"Arn"
]
}
}
]
}
}));

test.done();
},

'grants permission to send messages'(test: Test) {
const stack = new Stack();
const role = new Role(stack, 'Role', { assumedBy: new ServicePrincipal('lambda.amazonaws.com') });
const queue = new sqs.Queue(stack, 'Queue');
queue.grantSendMessages(role);

expect(stack).to(haveResource('AWS::IAM::Policy', {
"PolicyDocument": {
"Statement": [
{
"Action": "sqs:SendMessage",
"Effect": "Allow",
"Resource": {
"Fn::GetAtt":
[
"Queue4A7E3555",
"Arn"
]
}
}
]
}
}));

test.done();
}

},

'queue encryption': {
'encryptionMasterKey can be set to a custom KMS key'(test: Test) {
const stack = new Stack();
Expand Down