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: add granular read ops enum #2720

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AaronZyLee
Copy link

@AaronZyLee AaronZyLee commented Jan 31, 2023

Issue #

aws-amplify/amplify-flutter#2526
#3038

Description

Add granular read operation enums in model auth operation rule
Refer https://docs.amplify.aws/cli/graphql/authorization-rules/#how-it-works

The purpose of adding enum in library is to ensure the model files generated by modelgen will be compiled. However, this is not intended for Datastore usage but for those who use modelgen files along with API plugin. It is not recommended to use them in the datastore due to the prerequisite of sync and listen. This is documented in the callout

General Checklist

  • Added new tests to cover change, if needed
  • Build succeeds with all target using Swift Package Manager
  • All unit tests pass
  • All integration tests pass
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
  • Documentation update for the change if required
  • PR title conforms to conventional commit style
  • New or updated tests include Given When Then inline code documentation and are named accordingly testThing_condition_expectation()
  • If breaking change, documentation/changelog update with migration instructions

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

Copy link

@phani-srikar phani-srikar left a comment

Choose a reason for hiding this comment

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

Are there any existing tests associated with these additional operations?

@5d
Copy link
Member

5d commented Jun 26, 2023

Are there any existing tests associated with these additional operations?

@phani-srikar ; No, we don't have test cases cover these additional operations.

@AaronZyLee ; Have you performed manual testing to ensure that these additional operation types are functioning as expected?

Comment on lines +25 to 31
// granular read access
case `get`
case list
case sync
case listen
case search
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we start off by attaching the generated Model swift types that utilizes these new granular read access cases? By doing so, we can update the source schema that we use to reference the generated types.

Is there an sort of relevant API calls that we can write, either in the unit or integration tests, utilizing these models, and asserting the expected behavior?

Copy link
Author

Choose a reason for hiding this comment

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

The example schema using it is

type Todo @model
@auth(rules:[
  { allow: public, operations: [get, list, listen, sync, search] },
  { allow: owner }
])
{
  id: ID!
  name: String
}

The model files can be generated by the current version of modelgen, which has the following line generated

    model.authRules = [
      rule(allow: .public, operations: [.get, .list, .listen, .sync, .search]),
      rule(allow: .owner, ownerField: "owner", identityClaim: "cognito:username", provider: .userPools, operations: [.create, .update, .delete, .read])
    ]

@lawmicha
Copy link
Member

lawmicha commented Sep 1, 2023

In DataStore, we get the authtypes based on .read. Since developers can use this schema with DataStore as long as they allow listen and sync. then this code probably needs to be updated to getting auth types for .read, sync or listen. ?

var authTypes = await authModeStrategy.authTypesFor(schema: modelSchema, operation: .read)

Comment on lines 24 to +30
case read
// granular read access
case `get`
case list
case sync
case listen
case search
Copy link
Member

Choose a reason for hiding this comment

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

I bet a lot of logic is determined by checking if the operation is .read and does something. Now that we allow compiling the more granular "read" types, that means all logic based on "read" should now be updated to

  1. If the previous check for read expected all granular read access then update the code to "if operation == read || operations.containsAll(list, sync, listen, search)"
  2. If the previous check for read expected just the sync API (for example DataStore's sync operation), then code can be updated to "if operation == read || operation == sync)"
  3. If previous check for read expected that the subscription API is used then --> "operation == read or operation == "listen"

@AaronZyLee
Copy link
Author

In DataStore, we get the authtypes based on .read. Since developers can use this schema with DataStore as long as they allow listen and sync. then this code probably needs to be updated to getting auth types for .read, sync or listen. ?

var authTypes = await authModeStrategy.authTypesFor(schema: modelSchema, operation: .read)

The datastore usage is out of scope of this PR as mentioned in the description. This is for the API users to get the modelgen files compiled

@lawmicha
Copy link
Member

lawmicha commented Sep 1, 2023

In DataStore, we get the authtypes based on .read. Since developers can use this schema with DataStore as long as they allow listen and sync. then this code probably needs to be updated to getting auth types for .read, sync or listen. ?

var authTypes = await authModeStrategy.authTypesFor(schema: modelSchema, operation: .read)

The datastore usage is out of scope of this PR as mentioned in the description. This is for the API users to get the modelgen files compiled

Customers can use DataStore, and if they have more granular access including listen and sync, then their models may not work as expected because DataStore's sync code checks for "read" access, not "read" or "sync" access. By allowing them to compile the App with the generated models, will cause other unintended behaviors when using DataStore at runtime

@lawmicha
Copy link
Member

lawmicha commented Oct 2, 2023

Next steps, this comment #2720 (comment) captures some the concern over introducing more granular operations that was "sub-operations" of read. Logic that checks for "read" should consider checking for "read" OR one or more of the granular operations

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

Successfully merging this pull request may close these issues.

None yet

5 participants