Amplify-cli: Field @auth transformer combinaisons can lead to unexpected authorized mutation

[dz0l038]

Hi!

I am facing a strange behavior for a few days now on the "field @auth transformer". Authorization to perform some mutations are granted when it shouldn't, and the fields order seems to have an impact on mutation execution while I would have think that it is not the expected behavior.
I am still unsure if I misunderstood this transformer or if it is an actual bug, but it is odd enough to me to open it as an issue. Hopefully an Amplify guru will see that and enlighten me if I didn't get something.

Amplify version: 1.12.0

How the model should behave:

Only two users have access to the model, user1 and user2. At creation, user1Sub and user2Sub fields are populated so those two users will have read access. The user2 can read any fields but cannot update anything. User1 can read any fields and can update the content and updatedAt field but CANNOT update user1Sub and user2Sub.

So the schema I first came up with was this one:

schema nbr1

type Agreement
  @model(subscriptions: null)
  @key(fields: ["id"])
  @key(name: "ByUser1", fields: ["user1Sub", "updatedAt"], queryField: "agreementsByUser1")
  @key(name: "ByUser2", fields: ["user2Sub", "updatedAt"], queryField: "agreementsByUser2")

  @auth(rules: [{ allow: groups, groups: ["Admin"], operations: [create, read, update, delete] },
                { allow: owner, ownerField: "user1Sub", operations: [read, create] },
                { allow: owner, ownerField: "user2Sub", operations: [read] }])
{
  id: ID!
  user1Sub: ID!
  user2Sub: ID!
  updatedAt: AWSDateTime! @auth(rules: [{ allow: owner, ownerField: "user1Sub", operations: [update] }])
  content: String! @auth(rules: [{ allow: owner, ownerField: "user1Sub", operations: [update] }])
}

From my understanding of the doc, the model above should behave as expected. The global @auth prevents any update, but fields @auth allow it for user1 on updatedAt and content.

When I run (as user1):
Mutation nbr1: only user1Sub

mutation UpdateAgreement {
  updateAgreement(input: {
    id: "d28eb0e-7db3-a61f-bde2-132c131e54",
    user1Sub: "50ae7808-aed8-4bf3-9078-5ab653f0bc36"
  }) {
    id
    updatedAt
    user1Sub
    user2Sub
    content
  }
}

I got an error Unauthorized -> Perfect, that's what I want.

Unfortunately if I run (as user1):
Mutation nbr 2: user1Sub and updatedAt

mutation UpdateAgreement {
  updateAgreement(input: {
    id: "d28eb0e-7db3-a61f-bde2-132c131e54",
    updatedAt: "1970-01-01T00:00:00.000Z",
    user1Sub: "50ae7808-aed8-4bf3-9078-5ab653f0bc36"
  }) {
    id
    updatedAt
    user1Sub
    user2Sub
    content
  }
}

Then the mutation is executed 😲 -> definitely not what I was expected. If a field mutation is considered as Unauthorized, being allowed to update another field should not change that and the whole mutation should be rejected right?

Anyway, I tried to find a workaround by explicitly block update mutations on a field level using both Group authorization (only authorizing an empty group), and owner authorization (giving ownership to a null field).
So here is the two modifications:

2nd schema
Forbid to update user1Sub using group (user1 not in it obiously)

user1Sub: ID! @auth(rules: [{ allow: groups, groups: ["Forbidden"], operations: [update] }])

3rd schema
Forbid to update user1Sub using ownerfield set to null

user1Sub: ID! @auth(rules: [{ allow: owner, ownerField: "forbidden", operations: [update] }])

Here is what I obtained:

Mutation | 1st schema | 2nd schema | 3rd schema
------------ | ------------- | ------------- | -------------
Mutate user1Sub | Unauthorized | Unauthorized | DynamoDB:ConditionalCheckFailedException
Mutate user1Sub and updatedAt | !Mutation executed! | !Mutation executed! | DynamoDB:ConditionalCheckFailedException

So it appears that setting the ownership to a null field gives the expected behavior, but more by failing because the null is probably not handled at one point, so not super satisfying...

Part 2, with field order changes:

Then I tried to modify the field's order by putting updatedAt field before the user1Sub, so the schema nbr 1 is now:

  id: ID!
  updatedAt: AWSDateTime! @auth(rules: [{ allow: owner, ownerField: "user1Sub", operations: [update] }])
  user1Sub: ID!
  user2Sub: ID!
  content: String! @auth(rules: [{ allow: owner, ownerField: "user1Sub", operations: [update] }])

Here is the same table than above, using the same mutation and the same schema modification for the 2nd and 3rd schema:

Mutation | 1st schema | 2nd schema | 3rd schema
------------ | ------------- | ------------- | -------------
Mutate user1Sub | Unauthorized | Unauthorized | DynamoDB:ConditionalCheckFailedException
Mutate user1Sub and updatedAt | !Mutation executed! | Unauthorized | !Mutation executed!

So this is even odder, the field order totally change the behavior... Now using group field authorization return what I was expecting but the mutation is executed with the ownership method.

I honestly have no clue on what is happening. Any workarounds/explanations are very welcome.

Cheers

[RossWilliams]

Hiya, this does sound strange. I can give an overview of how authentication works for field level updates that can explain why you are seeing this behaviour. I does seem like a problem in Amplify 1.12, I have not checked if this still exists in the latest versions.

There are two main phases of authorisation evaluation for updates, field-level and object level. Inside each phase, there are static group checks, dynamic group checks, and owner checks. Static group checks happen inside AppSync, and the two others happen inside DynamoDB, as they rely on fields where AppSync does not have information.

If a field has its static group check pass (allowed operation), the field will not have dynamic group or owner permissions checked. Otherwise the dynamic groups and owner fields are checked. How they are checked is problematic, because each field over-writes the previous field's checks for that auth type. This is a bug. If two fields are both in the update object, and both fields are protected by owner auth, only the second field's rules will be evaluated. If two fields are both in the update object, and one field is protected by owner auth, and the other field is protected by dynamic group auth, both field's rules will be evaluated.

If a static group check fails for a field or object and there is no dynamic group rule or owner rule previously set for any previous rule, AppSync returns as unauthorised. This is likely also a bug.

If the object has a rule to enforce updates using dynamic group or owner rules, then these rules will override any field level controls. Unless any field has its static rule check pass, or the object's field level static group rule check passes, in which case dynamic groups and owner rules are skipped.

Using the above shorthand, here is why you are seeing each behaviour:

1a. Object level static group rule for update fails, server-side ruleset is empty, AppSync returns as unauthorised.
1b. Object level static group rule for update fails, but owner auth rule has already been set, evaluate server-side field rules for updatedAt, rule passes, update works.
2a. Field level rule for user1Sub static group fails, server-side ruleset is empty, AppSync returns as unauthorised.
2b. Field level rule for user1Sub static group fails, but updatedAt rule has been processed and rule set, rule runs server-side and passses.
3a. Field level rule for user1Sub runs server-side, rule fails.
3b. Field level rule for user1Sub evaluated after updatedAt, over-writes updatedAt conditions, rule fails.

When you modified the field order, fields over-writing gets swapped around:

2b. Field level rule for user1Sub runs first, static group check fails, no dynamic group or owner rule yet set, AppSync returns as unauthorised.
3b. Field level rule for user1Sub runs first, is server-side rule. Field level rule for updatedAt runs, over-writes user1Sub auth checks, rule runs server-side and passes.

What is interesting is if you test by setting a groupsField rule on user1Sub (and adding that field to your model). Then the rules should not overwrite each other as long as your schema does not change.

At a code level, these issues mainly occur because field-level authorisation re-uses methods for object-level authorisation, but doesn't account for the fact that variables declared in the velocity template are being re-used or over-written. Reducing the amount of work done at the field level, changing variable declarations to use defaultIfNull, re-setting isStaticGroupAuthorised for each field evaluation, and some property naming changes could tighten this all up.

I'm also confused about the auth design that allows updating field level properties when there is no permission to update at the object level. It should be documented how object-level and field-level rules interact, currently docs give the impression that field level rules can only further lock down access.

[RossWilliams]

@dz0l038 This is a really well written bug report with tons of information. I'm surprised there has not been follow up from AWS to confirm the issue and provide temporary workarounds. Authorisation clearly isn't conforming to the Amplify documentation.

[UnleashedMind]

@dz0l038 @RossWilliams
Just wanted to confirm to you guys that we are investigating this issue, and going in depth on your feedbacks above. Thank you for the detailed report.

[dz0l038]

@RossWilliams Thanks a lot for this super detail answer, the black magic behind auth makes more sense now. Maybe a word on the doc to prevent people to use field level too much if they are dealing with sensitive data would make sense.

Anyway if someone is also stuck with that, here is my workaround:
I simply prevented all mutations on my model and manually the specified those I want to allowed, thought a lambda:

Create a function where you can put you logic

amplify add function

Prevent mutations

type Agreement
  @model(subscriptions: null, mutations: null)
{
...
}

Create the specific mutation you want to allow:

type Mutation {
  updateAgreement(id: ID!, updatedAt: String!, ...): Agreement @function(name: "updateAgreementFunction-${env}")
}

In the function

const AWS = require('aws-sdk')
const uuid = require('uuid/v4')
const ddb = new AWS.DynamoDB();
const agrementTable = "Agreement-rv5frrhfdvelrems45yutse7su-master";

exports.handler = function (event, _, callback) {
      let date = new Date().toISOString();
      const input = { ...event.arguments, id: uuid(), postedAt: date, user1Sub: event.identity.username }
      ...
      // All your checks and updates here
      ...

      // If succeed
      callback(null, input);
}

Doing that I don't have more work to do for queries, I can call it from my frontend the exact same way that any mutation or queries, and I can handle complexe business logic. Probably not optimized compare to a resolver in .vtl, but quick and flexible solution for my use case ☺️

[RossWilliams]

@SwaySway Does cli v3.7 resolve this, or are there more changes expected?

[kaustavghosh06]

@dz0l038 Thanks for the detailed report. It helped us diagnose an issue and root-cause it. The issue you've mentioned in the "part 2" of your description was a bug and the authorization of the fields shouldn't be dependent on the order of the fields. We fixed this with the latest version of the CLI - v3.8.0. We adhere to the authorization policy for both the fields you're updating - independent of the order in the schema and provide unauthorized/authorized response for the same.

Regarding "part 1" of the issue, you were going in the right direction by explicitly providing authorization rules to your fields - by adding the forbidden groups/owner rules - since we don't deny access to the field by default on the operation if the operation is not included in the top level auth rule. To add more clarity we've updated our docs describing this behavior - https://aws-amplify.github.io/docs/cli-toolchain/graphql#owner-authorization (top level @auth operations) and per field level @auth operations - https://aws-amplify.github.io/docs/cli-toolchain/graphql#field-level-authorization . Please try out the latest version of the CLI and let us know if you've any questions around this behavior.

[dz0l038]

@kaustavghosh06 Happy to hear that the report help you a bit.
Thanks for the update on the doc, I ll definitely try that as soon I have some time (as well as the public authorization that seems really cool)!

[RossWilliams]

@kaustavghosh06 You updated the documentation last week to add the following in response to this issue,

An owner of an object, has the authorization to create Todo types and read all the objects of type Todo. In addition an owner can perform an update operation on the Todo object, only when the content field is present as a part of the input.

https://github.com/aws-amplify/docs/blame/master/cli-toolchain/graphql.md#L1276

The critical part for me: is present as a part of the input. This reads to say that if an owner includes content as a field in a mutation, then they are authorised to update any other field, even if the object-level auth forbids the update, and potentially even if the other field is protected by auth rules.

In your above comment you state

since we don't deny access to the field by default on the operation if the operation is not included in the top level auth rule.

Taking the inverse, you suggest the intention is to deny access to the field by default on the operation if the operation is included in the top level auth rule.

In addition, the Employee salary example in the document clearly states

here is a model where owners and admins can read employee salaries but only admins may create or update them

Can you advise on the apparent conflict in these statements. Does the language in your documentation update last week need to be changed?

[RossWilliams]

@kaustavghosh06 A second feedback on testing the documentation. e2e tests have the following comment

  # Fields with ownership conditions take precendence to the Object @auth.
  # That means that both the @auth on Object AND the @auth on the field must
  # be satisfied.

the documentation has this schema and comment:

type Todo
  @model @auth(rules: [{allow: groups, groups: ["Admin"], operations:[update] }]
{
  id: ID! 
  updatedAt: AWSDateTime! 
  content: String! @auth(rules: [{ allow: owner, operations: [update] }])
}

In addition an owner can perform an update operation on the Todo object, only when the content field is present as a part of the input

The e2e test comment and documentation are in conflict. According to e2e tests, Todo content field can only be updated by the owner if the owner is also an Admin, where the comment suggests being an owner is sufficient to update the content field.

[forstmeier]

Just wanted to bump this issue to see if any progress had been made on a possible, complete solution. I've run into issues using multiple allow: owner rules together since the evaluation overwrites values in the velocity code, as @RossWilliams points out.

If there is no ETA, what should I start looking at to provide a potential pull request to address the issue?

[SwaySway]

Hi @forstmeier the enhancement to ownerField is a backlog item, at this time we don’t have an ETA but can look at adding this functionality in the future. For the time being I would recommend using the groupsField for multiple users which is the recommended way of granting multiple users access to an action.

We are open to PRs and we can help as well. The code for this resides in resources.ts
amplify-cli/resources.ts at master · aws-amplify/amplify-cli · GitHub

Another potential solution could be to use a group rule like so:

{ allow: groups, provider: oidc, groupsField: "<users&team>",
  groupClaim: "teams", operations: [create, update, delete, read]}

[SwaySway]

Hi @RossWilliams the comment in the e2e test is an old comment that never was removed but that’s independent of our supported functionality which is in documentation. What is in the documentation is an accurate statement.

The critical part for me: is present as a part of the input. This reads to say that if an owner includes content as a field in a mutation, then they are authorised to update any other field, even if the object-level auth forbids the update, and potentially even if the other field is protected by auth rules.

Explict rules are needed as mentioned in the documentation:

GraphQL Transform Field Level Authorization

type Todo 
  @model
{
  id: ID! 
  updatedAt: AWSDateTime! @auth(rules: [{ allow: groups, groups: ["ForbiddenGroup"] }])
  content: String! @auth(rules: [{ allow: owner, operations: [update] }])
}

[RossWilliams]

@SwaySway Can you explicitly state that my interpretation of your object level documentation is correct? Your quote from me was referencing the documentation where there is object level authorisation. You have posted the documentation example using field level authorisation as a solution. I want to know if the intention of the field-level auth author is as I described, that an object level auth which blocks updates can nevertheless have any of its fields updated, as long as a single field in the update grants access?

type Todo
  @model @auth(rules: [{allow: groups, groups: ["Admin"], operations:[update] }]
{
  id: ID! 
  updatedAt: AWSDateTime! 
  content: String! @auth(rules: [{ allow: owner, operations: [update] }])
}

In this example from the documentation, who is allowed to update the "updatedAt" field? I'm very carefully reading the documentation, and it implies that an owner is allowed to update this field, even though they are blocked from updates at an object level, and there is no field level auth on the 'updatedAt' field. The only catch is that they need to update the 'content' field at the same time.

I'm trying to point out that the field level authorisation is simply too difficult to get right. If it is hard to read a model and understand if its secure, the design is lacking. I want to assume best intentions, but it almost feels like there is post hoc rationalisation of the design.

[forstmeier]

@SwaySway Thanks for the response.

I tried out your recommendation:

Another potential solution could be to use a group rule like so:

{ allow: groups, provider: oidc, groupsField: "",
groupClaim: "teams", operations: [create, update, delete, read]}

But that throws the following error:

@auth directive with 'groups' strategy only supports 'userPools' provider, but found 'oidc' assigned.

From what I can tell, using the groups excludes the use of oidc as a provider - is that correct?

[SteffeyDev]

I'm now running into the same issue, thanks @RossWilliams for explaining it so well because I was just starting to understand the problem. The variable re-use is still a problem but one that seems like it should have an easy fix by using custom variable names for each section. For example, you could use $<fieldName>_isOwnerAuthorized for field-level auth checks and $<objectName>_isOwnerAuthorized for object level, instead of using $isOwnerAuthorized for both types. It looks like you are already using this strategy for static group level authorization (as I discovered in an inspection of the generated transformer) so I was surprised you weren't doing the same for owner auth.

[RossWilliams]

@SteffeyDev it is harder for owner and dynamic group auth because AppSync can’t determine if owner is authorised for update operations, instead it needs to write DynamoDB condition expressions. These are not easy to generate in complex cases. Not impossible, but not a small amount of work. Dynamo also complains if you do things such as wrap an expression in and extra set of parenthesis, have unused variables, and so on which requires extra attention.