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: Allow access on all indexes of a table in GuDynamoDBPolicy #2024

Merged
merged 5 commits into from
Sep 18, 2023

Conversation

kenoir
Copy link
Contributor

@kenoir kenoir commented Sep 15, 2023

What does this change?

This change includes all table indexes in the blanket GuDynamoDBReadPolicy and GuDynamoDBWritePolicy classes.

Without this consumers will see failures querying or updating items in Global Secondary Indexes.

Adding read/write access to indexes of a table in these policies is intended to make it easier to use these policies by allowing access to indexes as might be expected.

How to test

  • Run the tests.
  • Include this version of the CDK via this branch and test locally the correct CF is provisioned.

See: https://github.com/guardian/crosswordv2/pull/38

How can we measure success?

Consumers are able to use the read/write policies in more expected use cases.

Have we considered potential risks?

This does expand the permissions of these policies, but not in a surprising way (probably not in issue as currently consumed).

@kenoir kenoir requested a review from a team as a code owner September 15, 2023 13:41
This change includes all table indexes in the blanket
GuDynamoDBReadPolicy and GuDynamoDBWritePolicy classes.

Without this consumers will see failures querying of updating items in
Global Secondary Indexes.

Adding read/write access to indexes of a table in these policies is
intended to make it easier to use these policies by allowing access to
indexes as might be expected.
@kenoir kenoir force-pushed the rk/allow-read-on-table-indexes branch from dc43562 to 0e463c3 Compare September 15, 2023 13:41
@kenoir kenoir changed the title Allow access on all indexes of a table in GuDynamoDBPolicy feat: Allow access on all indexes of a table in GuDynamoDBPolicy Sep 15, 2023
@kenoir kenoir closed this Sep 15, 2023
@kenoir kenoir reopened this Sep 15, 2023
@kenoir kenoir force-pushed the rk/allow-read-on-table-indexes branch from 6c74202 to 0a86d82 Compare September 15, 2023 14:20
resources: [`arn:aws:dynamodb:${scope.region}:${scope.account}:table/${props.tableName}`],
resources: [
`arn:aws:dynamodb:${scope.region}:${scope.account}:table/${props.tableName}`,
`arn:aws:dynamodb:${scope.region}:${scope.account}:table/${props.tableName}/index/*`,
Copy link

@adamnfish adamnfish Sep 18, 2023

Choose a reason for hiding this comment

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

Are we sure this will work for the actions people are using in practice? There aren't many actions that support indexes as a resource, just the basics like Scan:
https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazondynamodb.html

If people have added other action props, this might break their stacks if CloudFormation then rejects the index resource for that action?

Copy link
Contributor Author

@kenoir kenoir Sep 18, 2023

Choose a reason for hiding this comment

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

Good question. I've tested that the CloudFormation that gets generated will successfully apply by deploying https://github.com/guardian/crosswordv2/pull/38 to our CODE environment (CF updates successfully and the expected behaviour can be observed with regard to permissions).

I think this is similar to the case where wildcard resources can be applied to sets of actions. See the AmazonDynamoDBReadOnlyAccess managed policy attached.

Screenshot 2023-09-18 at 10 42 29

The documentation that I can find is not explicit on this point, but does state:

If you specify a resource type in a statement with an action that does not support that resource type, then the statement doesn't allow access.

So I believe we're safe from disrupting existing stacks here. I will add a comment describing this to the change as it does seem like a potential point of confusion.

@kenoir kenoir merged commit 63c5f7f into main Sep 18, 2023
3 checks passed
@kenoir kenoir deleted the rk/allow-read-on-table-indexes branch September 18, 2023 12:19
@github-actions
Copy link
Contributor

🎉 This PR is included in version 50.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants