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(graphql-auth-transformer): fixes @auth directives for Admin UI #7035

Merged
merged 6 commits into from
May 4, 2021

Conversation

hsspain
Copy link
Contributor

@hsspain hsspain commented Apr 7, 2021

Description of changes

This fixes a scenario in with the Manage - Content section Admin UI no longer functioned in specific scenarios due to authorization errors from the AppSync API.

Issue

If a user deployed an API with models that had less than full @auth operations directives (blank or ["create", "read", "update, "delete"]], the generated schema would not include the @aws_iam directive on the mutations as needed by the Manage - Content functionality.

For instance, if the schema looked like this:

type Post @model @auth(rules: [{allow: public, operations: [read, update]}]) {
      id: ID!
      title: String!
      createdAt: String
      updatedAt: String
  }

The transformed schema would have an update mutation that looked like this

updatePost(input: CreatePostInput!): Post

instead of what it needed to look like to authenticate properly

updatePost(input: CreatePostInput!): Post @aws_api_key @aws_iam'

This would ultimately present itself with the Manage - Content section of the Admin UI failing to initialize DataStore or with failures to delete or update objects on the affected data model.

Fix

This changes the order in which the directives are transformed, to add the requisite iam @auth rules to the underlying schema before the rules are split into operations.

Because the operation split had been happening before the iam rule was added, the mutations were ignored when adding proper directives to the transformed schema.

Description of how you validated changes

Unit tests were added for the affected scenarios. Use cases were also tested by using the development branch CLI to push schema.graphql changes directly to a previously broken app. The Manage - Content section was verified to work.

Checklist

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

@hsspain hsspain requested a review from a team as a code owner April 7, 2021 23:15
Copy link
Contributor

@SwaySway SwaySway left a comment

Choose a reason for hiding this comment

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

Doing this change will add this rule {allow: 'private', provider: 'iam'} into operation rules since the split happens after we do the amplify admin check. When passed into the mutation functions it'll add the directive and the policy into the auth role.

Example if you have the following schema

type Post @model @auth(rules: [
  { allow: private, operations: [read, update], provider: iam }]) {
  id: ID!
  title: String
  description: String
  createdAt: AWSDateTime
  updatedAt: AWSDateTime
}

It will generate a policy to allow private (in this case the auth role) access to create a post.

Can we do the admin check in addDirectivesToOperation and add a unit test to verify the policy generation? This way we don't create unwanted policies.

@hsspain
Copy link
Contributor Author

hsspain commented Apr 8, 2021

Don’t we need that policy to allow the IAM users of the Admin UI to manage content?

@akshbhu
Copy link
Contributor

akshbhu commented Apr 8, 2021

Doing this change will add this rule {allow: 'private', provider: 'iam'} into operation rules since the split happens after we do the amplify admin check. When passed into the mutation functions it'll add the directive and the policy into the auth role.

Example if you have the following schema

type Post @model @auth(rules: [
  { allow: private, operations: [read, update], provider: iam }]) {
  id: ID!
  title: String
  description: String
  createdAt: AWSDateTime
  updatedAt: AWSDateTime
}

It will generate a policy to allow private (in this case the auth role) access to create a post.

Can we do the admin check in addDirectivesToOperation and add a unit test to verify the policy generation? This way we don't create unwanted policies.

Won't this add the rule and policy if AdminUI is enabled? I think this is fine.
Do we need to check for duplicates when adding this rule if AdminUI is enabled?

@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2021

Codecov Report

Merging #7035 (11af910) into master (0b20951) will increase coverage by 0.01%.
The diff coverage is 86.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7035      +/-   ##
==========================================
+ Coverage   52.27%   52.29%   +0.01%     
==========================================
  Files         491      491              
  Lines       25248    25259      +11     
  Branches     4952     4961       +9     
==========================================
+ Hits        13199    13208       +9     
  Misses      11097    11097              
- Partials      952      954       +2     
Impacted Files Coverage Δ
...aphql-auth-transformer/src/ModelAuthTransformer.ts 88.67% <86.53%> (-0.09%) ⬇️

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 0b20951...11af910. Read the comment docs.

@SwaySway SwaySway self-assigned this Apr 30, 2021
hsspain and others added 5 commits April 30, 2021 16:24
Updated the way IAM auth gets added when AdminUI is enabled by ensuring all the the GraphQL
operations for all the type get IAM private authroization added and no IAM policy gets added Auth or
UnAuth user roles as AdminUI uses a different role to access the types and fields
@yuth yuth force-pushed the graphql-auth-transformer/adminui-auth branch from bcd6253 to e76be9b Compare April 30, 2021 23:56
Copy link
Contributor

@SwaySway SwaySway left a comment

Choose a reason for hiding this comment

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

aside from nit LGTM!

@yuth yuth merged commit d4f2f1e into master May 4, 2021
@github-actions github-actions bot added the referenced-in-release Issues referenced in a published release changelog label May 14, 2021
@github-actions
Copy link

👋 Hi, this pull request was referenced in the v4.51.0 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v4.51.0.

cjihrig pushed a commit to ctjlewis/amplify-cli that referenced this pull request Jul 12, 2021
…ws-amplify#7035)

This fixes a scenario in with the Manage - Content section Admin UI no longer functioned in specific scenarios due to authorization errors from the AppSync API.
Issue

If a user deployed an API with models that had less than full @auth operations directives (blank or ["create", "read", "update, "delete"]], the generated schema would not include the @aws_iam directive on the mutations as needed by the Manage - Content functionality.

For instance, if the schema looked like this:

type Post @model @auth(rules: [{allow: public, operations: [read, update]}]) {
      id: ID!
      title: String!
      createdAt: String
      updatedAt: String
  }

The transformed schema would have an update mutation that looked like this

updatePost(input: CreatePostInput!): Post

instead of what it needed to look like to authenticate properly

updatePost(input: CreatePostInput!): Post @aws_api_key @aws_iam'

This would ultimately present itself with the Manage - Content section of the Admin UI failing to initialize DataStore or with failures to delete or update objects on the affected data model.
Fix

Updated the way IAM auth gets added when AdminUI is enabled by ensuring all the the GraphQL
operations for all the type get IAM private authroization added. Update the code to ensure no IAM policy gets added Auth or UnAuth user roles since AdminUI uses a different IAM role to access the types and fields


Unit tests were added for the affected scenarios. Use cases were also tested by using the development branch CLI to push schema.graphql changes directly to a previously broken app. The Manage - Content section was verified to work.
@danielleadams danielleadams deleted the graphql-auth-transformer/adminui-auth branch December 15, 2022 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
referenced-in-release Issues referenced in a published release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants