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

fix: authRole,unauthRole define resources according to @auth directive operations #2117

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fossamagna
Copy link
Contributor

@fossamagna fossamagna commented Dec 5, 2023

Description of changes

I added resource definitions for authRole and unauthRole based on resource definitions for each operations obtained from Access Control Matrix.

CDK / CloudFormation Parameters Changed

N/A

Issue #, if available

fix #2111

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Any CDK or CloudFormation parameter changes are called out explicitly

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@fossamagna fossamagna requested a review from a team as a code owner December 5, 2023 00:02
@fossamagna fossamagna marked this pull request as draft December 5, 2023 03:35
@fossamagna fossamagna marked this pull request as ready for review December 5, 2023 04:47
@palpatim
Copy link
Member

palpatim commented Jan 4, 2024

This seems like a good change, but it's a pretty wide scope so we'll want to ensure it doesn't break existing installations. Running against my snapshot tests, it looks like it does just what @fossamagna intends--removes policy resources for unauthorized operations--so I am tentatively OK with this. If the @aws-amplify/amplify-data team can verify this with e2e tests I think we'll be good. I also want to make sure it works as expected with admin role enabled, since I haven't tested that yet.

As implied by "running against my snapshot tests": please note that if this change is merged after #2166, it will break the newly-added auth operations snapshot tests. The breakages are what we expect, but we'll need to re-generate the snapshot file (packages/amplify-graphql-auth-transformer/src/tests/snapshots/auth-operations-combination-snapshots.test.ts.snap) to allow the PR workflow tests to pass.

@fossamagna
Copy link
Contributor Author

@palpatim I have merged main into this PR branch. It does not seem necessary to regenerate the snapshot files you commented on. All tests passed without additional modifications or re-generating snapshots.

@fossamagna
Copy link
Contributor Author

@palpatim Is there anything blocking to merging this PR? Is there anything I can do to merge my PR?
Thanks.

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