-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
export const QUEUE_GET_ACTIONS = [ | ||
"sqs:ReceiveMessage", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should have There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
]; |
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'; | ||
|
@@ -275,6 +277,64 @@ export class Queue extends QueueRef { | |
} | ||
} | ||
|
||
/** | ||
* Grant access to Consume a queue to the given identity. | ||
* | ||
* This will grant the following permissions: | ||
* | ||
* * sqs:ChangeMessageVisibility | ||
* * sqs:DeleteMessage | ||
* * sqs:ReceiveMessage | ||
* | ||
* @param identity Principal to grant consume rights to | ||
*/ | ||
public grantConsume(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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my head this was always called There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Roger that - I'll change accordingly. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
*/ | ||
|
There was a problem hiding this comment.
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 notexport
them.