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(graphql-auth-transformer): add compound auth rules #3123

Closed

Conversation

RossWilliams
Copy link

adds 'and' property on auth rules to force each auth rule with the same 'and' property to all match before allowing operations. Allows combining owner, group, and dynamic group authentication rules. Works on objects and fields for user pools.

Issue #, if available: aws-amplify/amplify-category-api#433 aws-amplify/amplify-category-api#449 aws-amplify/amplify-category-api#452

Description of changes:
The RFC for @auth directive improvements describes in option 5 the ability to join auth rules together using 'AND' and 'OR', with an arbitrary level of nesting. This PR takes a slightly different approach in only implementing AND, and not allowing nesting. This feels reasonable as @auth rules are OR'd by default and any nesting can be decomposed into a flat structure.

To implement this a new property on an auth rule is created, "and". And is assigned a string, and all rules with the same assigned and string are considered part of a group whose rules must all pass to allow the operation.

Compound rules are tracked by counting how many of the rules have passed, rejecting the operation if no compound rule sets have all their rules pass.

List operations require a deep clone of the object holding the counts via json stringify and json parse operations so that their count tracking is independent of each other.

Update and Delete operations are somewhat more challenging as their dynamic group and owner rules are evaluated on the server, while the static auth is evaluated on the client. This requires nesting condition expressions, and only executing DynamoDB calls if the static group rules have been satisfied.

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

@codecov-io
Copy link

codecov-io commented Jan 5, 2020

Codecov Report

Merging #3123 into master will increase coverage by 0.14%.
The diff coverage is 92.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    aws-amplify/amplify-cli#3123      +/-   ##
==========================================
+ Coverage   60.49%   60.64%   +0.14%     
==========================================
  Files         254      254              
  Lines       12619    12676      +57     
  Branches     2646     2504     -142     
==========================================
+ Hits         7634     7687      +53     
- Misses       4646     4666      +20     
+ Partials      339      323      -16
Impacted Files Coverage Δ
...aphql-auth-transformer/src/ModelAuthTransformer.ts 87.74% <100%> (+0.04%) ⬆️
packages/graphql-auth-transformer/src/resources.ts 89.47% <90.47%> (+0.62%) ⬆️
...ges/graphql-transformer-core/src/util/fileUtils.ts 11.94% <0%> (ø) ⬆️
...psync-simulator/src/resolvers/pipeline-resolver.ts 9.37% <0%> (ø) ⬆️
.../graphql-transformer-core/src/util/amplifyUtils.ts 5.83% <0%> (ø) ⬆️
.../graphql-transformer-core/src/util/sanity-check.ts 11.71% <0%> (ø) ⬆️
packages/amplify-appsync-simulator/src/index.ts 73.68% <0%> (ø) ⬆️
...c-simulator/src/schema/directives/aws-subscribe.ts 26.66% <0%> (ø) ⬆️
packages/amplify-cli/src/context-extensions.ts 25.69% <0%> (ø) ⬆️
...amplify-appsync-simulator/src/server/operations.ts 28.45% <0%> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a684439...589d5d5. Read the comment docs.

adds 'and' property on auth rules to force each auth rule with the same 'and' property to all match before allowing operations.
@davekiss
Copy link

davekiss commented Feb 6, 2020

This is a simple and elegant solution to a sorely missing feature in Amplify. What is the likelihood that this PR gets merged?

@RossWilliams My only note is and: "ruleGroupName" sort of mixes readability with parameterization. Not sure what sort of complexities this may introduce, but I wonder if a key/value of ruleGroup: "ruleGroupName" might make more sense from an implementation standpoint.

@RossWilliams
Copy link
Author

@davekiss that is an easy find/replace change once a good name is found. "And" was chosen to match aws-amplify/amplify-category-api#433 Proposal 5 naming, and it reads well when it is the last parameter, but I can see it being problematic otherwise. 'ruleGroup' itself does not feel like the final solution, as the word 'group' would be an overload for the group authorisation parameters. 'ruleSet' may be more descriptive.

In terms of timeline, I'm not aware of the process, but #2463 appears to be consuming most of the review energy right now, might need to wait for that to be completed first.

@attilah
Copy link
Contributor

attilah commented Apr 16, 2020

@RossWilliams thanks for this contribution. We cannot merge it as-is at this point, as it deviates the expression based syntax in the proposal, which is kind of a shared syntax when building expressions with the CLI’s directives, and the unknown performance implications what is also called out by you in the code. In addition, we’re experimenting with some auth logic change for VTL to simplify customers’ use cases and that work affects AND, OR rules as well.

@attilah attilah closed this Apr 16, 2020
@RossWilliams
Copy link
Author

RossWilliams commented Apr 16, 2020

@attilah

In addition, we’re experimenting with some auth logic change for VTL to simplify customers’ use cases and that work affects AND, OR rules as well

Do you mind sharing the auth logic changes you are playing with? I'm more than happy to help move it along and stop having to rebase these changes for another 9 months.

@attilah
Copy link
Contributor

attilah commented Apr 16, 2020

@RossWilliams nothing concrete to share at this point, but sure, I agree, no need to rebase this PR until we've anything.

@davekiss
Copy link

@attilah @RossWilliams wondering what the status is here. Ross, are you still using your custom fork for this?

@RossWilliams
Copy link
Author

Im still using a variant of this code with additional customisations and performance improvements.

I would not use this PR as-is, as there have been subsequent security issues found and fixed in the auth module.

@ronaldocpontes
Copy link

@attilah as you close this PR... are you still working on a solution?

@RossWilliams would you be kind enough to share your updated code with additional customisations and performance improvements?

@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants