aws-amplify / amplify-category-api

The AWS Amplify CLI is a toolchain for simplifying serverless web and mobile development. This plugin provides functionality for the API category, allowing for the creation and management of GraphQL and REST based backends for your amplify project.
https://docs.amplify.aws/
Apache License 2.0
84 stars 72 forks source link

@auth Combining Owner/Groups rules for Multi-Tenant Apps #449

Open lennybr opened 5 years ago

lennybr commented 5 years ago

Is your feature request related to a problem? Please describe. Ability to support multi-tenancy thru AppSync where individual items are "owned/belong" to a tenant instead of a user and we still have the ability to permission queries and mutations. Generated resolvers today effectively use isOwner || isInGroup(x for x in cognitoGroups) logic so multiple @auth rules cannot be combined to create more granular permissions.

Describe the solution you'd like A few ideas:

Describe alternatives you've considered Currently using the existing @auth owner strategy with custom ownerField and identityFIeld values, and setting the tid claim on the token with a pre-token generation Lambda function:

@auth(rules: [{allow: owner, ownerField: "tid", identityField: "claims.tid"}])

When used as the only @auth strategy, it works as intended (e.g. inserting the correct tid value during mutations; filters by tid value during queries, etc.).

But when I combine with @auth static groups strategy for permissions, the authorisation checks use OR logic instead of AND logic. I can't check for instance that a record both belongs to Tenant A (which the user belongs to) and has Permission X.

hisham commented 5 years ago

Probably related to https://github.com/aws-amplify/amplify-cli/issues/305 as well.

learningacct commented 5 years ago

This would also at least partially answer the question I posted over on amplify-js: aws-amplify/amplify-js#1926

mikeparisstuff commented 5 years ago

In regards to aws-amplify/amplify-cli#305, that issue has been fixed here https://github.com/aws-amplify/amplify-cli/pull/285. You may see the PR notes to see what has changed as well as an example of the complex auth flows that are possible now.

mikeparisstuff commented 5 years ago

In regards to this issue, it seems like the question relates to how to configure advanced auth flows such that rules may be composed together using "and" and "or" in flat and nested configurations.

Since this is really a feature request, let's try to define the ideal implementation. To allow for this use case we can extend the @auth directive definition to be the following:

directive @auth(rules: [AuthRule!]!) on OBJECT
input AuthRule {
    allow: AuthStrategy!
    ownerField: String # defaults to "owner"
    identityField: String # defaults to "username"
    groupsField: String
    groups: [String]
    queries: [ModelQuery]
    mutations: [ModelMutation]

    # New Additions
    and: [AuthRule]
    or: [AuthRule]
}
enum AuthStrategy { owner groups }
enum ModelQuery { get list }
enum ModelMutation { create update delete }

This follows the same structure that is used when we generate filter input objects for @model and @searchable types. The self-recursive structure allows nesting rules and combining logic with different boolean operators.

An example:

@auth(rules: [
    { and: [
        { allow: owner } # Call this A
        { or: [
            { allow: groups, groups: ["Admin"] } # Call this B
            { allow: groups, groupsField: "groups"] } # Call this C
        ]}
    ]}
])

would result in a final boolean expression:

A and (B or C)

Conceptually this makes sense and should be possible but I will have to spend more time with the details to figure out if there are any scenarios that will not work. If you have any feedback on this design please share as it would be helpful to have a few alternative to help guide the discussion.

Notes:

lennybr commented 5 years ago

This would definitely solve this specific use case and opens up many more complex auth options.

To solve the multi-tenant use case the auth rule becomes:

@auth(rules: [
    { and: [
        { allow: owner, ownerField: "tid", identityField: "claims.tid" } # Call this TenantId
        { or: [
            { allow: groups, groups: ["Admin"] } # Call this B
            { allow: groups, groupsField: "groups"] } # Call this C
        ]}
    ]}
])

would result in a final boolean expression:

TenantId and (B or C)

This would opaquely add/match the TenantId value during any mutations and filter any queries based on the TenantId. The subset of other auth rules would then further control access.

Happy to help test out various other combinations to validate this strategy either on the branch your are using for PR aws-amplify/amplify-cli#183 or if you want to handle this separately.

learningacct commented 5 years ago

To clarify, as @auth uses Cognito groups rather than something like a members: [ID] field in the schema, is there any way to use this functionality to create smaller groups within a tenant? It seems like the best solution I could come up is overly complex and adds considerable Cognito bloat.

@lennybr, I think an alternative to a new tenant rule (and a solution for a lot of the complexity I'm trying to work around) could be allowing @auth to pull tenant/group information from the schema rather than pushing it all to Cognito. I admittedly know too little about this to know if that would be breaking best practices (if it even worked in the first place), but it seems like that would enable a lot of functionality without having to build new custom auth rules for every use case.

lennybr commented 5 years ago

The queries and mutations argument only working on the top-level wouldn’t let us create a multi-tenant setup, since those arguments would be for each group in theory, I would want to do something like:

@auth(rules: [
    { and: [
        { allow: owner, ownerField: "tid", identityField: "claims.tid" } # Call this TenantId
        { or: [
            { allow: groups, groups: ["Admin"] }
            { allow: groups, groupsField: "readerGroups"], mutations: null }
            { allow: groups, groupsField: "editorGroups"], mutations: [update] }
        ]}
    ]}
])
lennybr commented 5 years ago

@learningacct interesting idea, might be able to accomplish that by creating a Tenant model and wrapping any other tenant-separated models with a nested resolver that first does a user/tenant lookup and then passes down the Tenant Id to the actual tenant model data resolver. There would likely be performance implications running both resolvers, so not sure about practicality. And linked Cognito Users to Tenants would be much easier after aws-amplify/amplify-cli#56 is solved.

A schema could look something like:

type TenantMembership @model {
  id: UserId!
  tid: Tenant!
}

type Data___Tenant @model {
    id: ID!
    data: Data
}

type Data @model(queries: null) {
  id: ID!
  tid: Tenant!
  content: String
}

# Queries would point to the ___Tenant model instead
type Query {
    getData(id: ID!): Data___Tenant
}

and the Data___Tenant resolvers

# Request template looks up the tenant Id for the current user:
{ 
  "version": "2017-02-28",
  "operation": "GetItem",
  "key": {
    "id": $util.dynamodb.toDynamoDBJson($ctx.identity.username),
  }
}

# Response template returns that Tenant Id:
$util.toJson($context.result.tid)

and the Data resolvers

# Request template looks up data, but also inherits $ctx.source with the Tenant Id context:
{ 
  "version": "2017-02-28",
  "operation": "GetItem",
  "key": {
    "id": $util.dynamodb.toDynamoDBJson($ctx.args.id),
  }
}

# Response template can check tenant authorisation:
## START: Validate Tenant. **
#if( $ctx.result.tid == $ctx.source.tid )
  #set($isTenantAuthorized = true)
#end
## END: Validate Tenant. **

## START: Throw if Unauthorized. **
#if( !$isTenantAuthorized )
  $util.unauthorized()
#end
## END: Throw if Unauthorized. **

$util.toJson($context.result)
mikeparisstuff commented 5 years ago

@learningacct As of aws-amplify/amplify-cli#285 you can use the multiple owner auth features to support the members: [ID] use case:

For example,

type Team @model @auth(rules: [{ allow: owner, ownerField: "members" }]) {
  id: ID!
  name: String
  members: [ID]
}

This would only allow team members (as defined by the list of ids in members) to CRUDL the team object.

mikeparisstuff commented 5 years ago

Also this discussion might be relevant to this https://github.com/aws-amplify/amplify-cli/issues/318. Here we talk about how you can use the schema to build complex relationships that also effectively act as auth rules for read operations. There are some implications of using this to protect mutations but it may offer some new ideas.

learningacct commented 5 years ago

Ah, great! I did try to look through that PR, but a lot of it went over my head. Thank you! And aws-amplify/amplify-cli#318 looks like it provides some helpful examples too. I'll look that over and do my best to apply it to what I'm doing. :)

@mikeparisstuff, am I able to use a variation of aws-amplify/amplify-cli#318 to sort of nest tasks within teams within tenants?

lennybr commented 5 years ago

@mikeparisstuff aws-amplify/amplify-cli#318 seems messy, especially with mutations and filling in owner fields. And while the members: [ID] use case works well to protect a single model, we are still missing the ability to protect related models. Any thoughts on the approach for that use case?

mikeparisstuff commented 5 years ago

@lennybr You are correct and that is actually the plan as soon as the feature is possible in AppSync. We are going to add new strategies for auth rules and one of them will be to check for membership in a many-to-many connection but this is blocked by a service feature that will be releasing later this year. A similar ask has been made in aws-amplify/amplify-cli#372.

lennybr commented 5 years ago

@mikeparisstuff great news. Any insights into the approach you are going to take for implementation? Will AppSync support middleware we can auth for Auth? Will you use nested resolvers? Or something else? I'm working on my own implementation now (currently using a root nested resolver against a tenantMembership object) but would love to stay in sync and align. Let us know if we can help and where to track progress on the feature. Thanks!

hisham commented 5 years ago

Pipeline resolvers just got released (https://aws.amazon.com/blogs/mobile/aws-appsync-releases-pipeline-resolvers-aurora-serverless-support-delta-sync/) and I assume this is the feature @mikeparisstuff is referring to. Looks very interesting! :)

mikeparisstuff commented 5 years ago

@hisham 👍 Exactly correct. We have a few changes that need to be made first but this is on the roadmap. Unfortunately no set date but I will reference these issues as soon as its in PR.

sollipse commented 5 years ago

Just chiming in to say that our team is also eagerly awaiting this feature.

nagey commented 5 years ago

@mikeparisstuff pipeline resolvers seem like a great step on this.

building an app using amplify right now (which we're finding super promising), and sorely need this feature. Should we bite the bullet and write our own pipeline resolvers, or should we expect to see this via the CLI soon?

amirmishani commented 5 years ago

@mikeparisstuff until this issue is solved, what is currently the best approach to a complex multi tenant auth where you need to check again both tenant_id and user's role? A custom resolver using VTL for every table that needs auth?

sebastienfi commented 5 years ago

@lennybr said:

Currently using the existing @auth owner strategy with custom ownerField and identityFIeld values, and setting the tid claim on the token with a pre-token generation Lambda function:

@auth(rules: [{allow: owner, ownerField: "tid", identityField: "claims.tid"}])

...and later, @lennybr said:

To solve the multi-tenant use case the auth rule becomes:

@auth(rules: [
    { and: [
        { allow: owner, ownerField: "tid", identityField: "claims.tid" } # Call this TenantId
        { or: [
            { allow: groups, groups: ["Admin"] } # Call this B
            { allow: groups, groupsField: "groups"] } # Call this C
        ]}
    ]}
])

...and later, @mikeparisstuff said in https://github.com/aws-amplify/amplify-cli/issues/1043:

Proposals 1 & 5 implemented in aws-amplify/amplify-cli#1262

⚡️🚀⚡️

Proposal 5: And/Or in @auth rules

Github Issues

  • aws-amplify/amplify-category-api#449

Problem

Currently all @auth rules are joined via a top level OR operation. For example, the schema below results in rules where you can access Post objects if you are the owner OR if you are member of the "Admin" group.

type Post @model @auth(rules: [{ allow: owner }, { allow: groups, groups: ["Admin"] }]) {
    id: ID!
    title: String
    author: User @connection(name: "UserPosts")
    owner: String
}

It would be useful if you could organize these auth rules using more complex rules combined with AND and OR.

Solution

We can accomplish this by adding to the the @auth definition.

directive @auth(rules: [TopLevelAuthRule!]!) on OBJECT, FIELD_DEFINITION
input TopLevelAuthRule {
    # For backwards compat, any rule specified at the same level as an "and"/"or" will be joined via an OR.
    allow: AuthStrategy!
    ownerField: String # defaults to "owner"
    identityField: String # defaults to "cognito:username" for UserPools, "username" for IAM, "sub" for OIDC
    groupsField: String
    groups: [String]

    # This only exists in top level rules and specifies operations for all the rules even when combined with and/or.
    # Neseted "operations" tags are not allowed because it would confuse evaluation logic.
    operations: [ModelOperation]

    # New recursive fields on AuthRule
    and: [AuthRule]
    or: [AuthRule]   
}
input AuthRule {
    allow: AuthStrategy!
    ownerField: String # defaults to "owner"
    identityField: String # defaults to "cognito:username" for UserPools, "username" for IAM, "sub" for OIDC
    groupsField: String
    groups: [String]

    # New recursive fields on AuthRule
    and: [AuthRule]
    or: [AuthRule]
}
enum AuthStrategy { owner groups }
# Reduces get/list to read. See explanation below.
enum ModelOperation { create update delete read }

This would allow users to define advanced auth configurations like:

type User
  @model 
  @auth(rules: [{
    and: [
      { allow: owner },
      { or: [
        { allow: groups, groups: ["Admin"] },
        { allow: owner, ownerField: ["admins"] },
      }
    ],
    operations: [read]
  }]) {
  id: ID!
  admins: [String]
  owner: String
}
# Logically: ( isOwner && ( isInAdminGroup || isMemberOfAdminsField ) )

The generated resolver logic will need to be updated to evaluate the expression tree.

@lennybr It seems that the solution you projected is now a reality. A solution for setting the tid claim on the token with a pre-token generation Lambda function would be something like

exports.handler = (event, context, callback) => {

  console.log(
    'Received eventObject {} in Invoke Request.',
    JSON.stringify(event, 3),
  );

  event.response = {
      "claimsOverrideDetails": {
          "claimsToAddOrOverride": {
              "tid": event.userPoolId,
          },
      }
  };

  // Return to Amazon Cognito
  callback(null, event);
};

That would kick-start any multi-tenant Amplify project. Comprehensive docs are here. Is that how you are doing this?

sebastienfi commented 5 years ago

I got mistaken, @mikeparisstuff meant "Proposals 1 & 4 implemented", so and and or operators are nowhere to be found in the code yet.

Lambda config itself does not allow setting the pre-token generation lambda trigger (and amplify-cli does not allow to set any lambda trigger...) so setting the trigger on the User Pool remains a manual action.

lenarmazitov commented 5 years ago

What about simple combination of group and owner ? Seems that is not so hard implementable feature.

In example: I need restrict access for CRUD to owner which have some group

In Graphql Transofrm it could looks like: @auth(rules: [{allow: ownerInGroups, ownerField: "owner", groups: ["PREM"]}])

tfendt commented 4 years ago

With this still in development what is the current approach for Multi-tenant auth? Is it modeling the memberships as outlined in this post: https://github.com/aws-amplify/amplify-cli/issues/318 or is it using the Pre Token Generation Lambda Trigger or something else completely?

dantasfiles commented 4 years ago

@tfendt The multi-tenant solution in aws-amplify/amplify-cli#318 from @RossWilliams uses pre-token-generation lambda triggers to create the virtual/fake cognito groups. The "custom claims" in the documentation don't seem to be available in the access token passed to the resolvers but virtual/fake cognito groups are. If there's a better way, I'd like to know as well.

RossWilliams commented 4 years ago

PreToken modifies the ID token, not the access token, which is just slightly problematic. There is another thread here about that.

I’ve added “and” rules in my cli fork and they work, but the code is fragile. it’s not done in a way that would likely get it merged in and relies on a lot of object cloning

dantasfiles commented 4 years ago

@RossWilliams In the pre-token-generation lambda, the "claimsToAddOrOverride" modifies only the ID token, but the "groupOverrideDetails" modifies both the ID and access tokens. When I use the Amplify GraphQL client, my resolvers are passed the access token, which is why your cool multi-tenant solution using "groupOverrideDetails" in aws-amplify/amplify-cli#318 works, while a custom claims technique using "claimsToAddOrOverride" isn't seen in my resolvers.

mrducky4 commented 4 years ago

If you want to use ID token instead of access token, here are some instructions.

dhmacs commented 4 years ago

Are there any update on this @mikeparisstuff ? Would love to see an example of multi tenant app with Amplify

RossWilliams commented 4 years ago

@macs91 I've got a branch that adds combining owner/group rules for a multi-tenant app here. I haven't done the cleanup work needed to merge it into the main repo, just dont have the time right now, but its solves the problem by adding an "and" argument to the @auth directive to combine multiple statements. Subscriptions doesnt work, but overall its not that many lines to implement. The branch isn't meant for general consumption and it hasn't been tested since I last rebased, so buyer-beware.

For anyone who wants to get this feature into the main project, it should be a good jumping off point.

dhmacs commented 4 years ago

@RossWilliams Thanks for pointing this out... Right now I'm checking out AWS Amplify for a new project, but If there's no official support for multi tenancy it's a no go for me

dantasfiles commented 4 years ago

@macs91 I have some sample multi-tenant code at https://medium.com/@dantasfiles/creating-a-simple-multi-tenant-aws-amplify-mobile-app-e26119ab8246

dhmacs commented 4 years ago

Hi @dantasfiles thanks for your answer! My use case is a bit more complex, I need to have authorization within the single tenant too (e.g. each member of a tenant should be able to read posts, but only tenant members with write permissions can update posts). I think that in order to do this AWS Amplify needs to add an AND clause to the @auth directive

ghost commented 4 years ago

Hi @mikeparisstuff Are there any update on this? Would love to use Amplify for my next project but multi-tenant is a must. Thanks.

AntonioAngelino commented 4 years ago

Hi @mikeparisstuff Are there any update on this? Would love to use Amplify for my next project but multi-tenant is a must. Thanks.

Same here!

An ETA would be highly appreciated :)

amirmishani commented 4 years ago

@mikeparisstuff while I think having proposal 5 (adding AND to @auth rules) would be fantastic, it has been a while since the proposal and multi tenancy is a big use case that should be solved.

Initially in my own app I did not mind custom adding a check at the top of each resolver but as my app started growing it became clear that the ideal situation is to have the amplify cli inject that check.

So I have a proposal. Can something like this be added?

@auth(
      rules: [
          {
             allow: tenant
             ownerField: "tenantId"
             identityClaim: "custom:tenant_id"
          }
      ]
)

Essentially by passing tenant as value for allow amplify cli would add that check at the top of every resolver and throw unauthorized if the check fails. Hopefully this is easier to implement than proposal 5 and also should cover many multi tenancy use cases.

mrducky4 commented 4 years ago

@amirmishani I don't understand what your proposal is adding. I am already using an auth rule like: [{allow: owner, ownerField: "tenantId", identityClaim: "custom:tenant_id", operations: [read, update]}] This works fine with the existing resolvers, I do not have to add anything to the generated resolvers. I did have to change some javascript configuration for the client to use the ID token instead of the access token, as described earlier in this thread. Can you please explain more about what your proposal adds beyond what amplify already supports? What additional behavior would amplify be supporting by using "allow: tenant" instead of "allow: owner"?

amirmishani commented 4 years ago

@mrducky4 well if you only have one auth rule you're fine. The problem is not having AND in the mix when you have more than one auth rule. This is because the current @auth array is based on logical OR. For example if you have the following:

type User
  @model
  @key(fields: ["tenantId", "userId"])
  @auth(
      rules: [
          {
             allow: owner
             ownerField: "tenantId"
             identityClaim: "custom:tenant_id"
          }
          {
            allow: owner
            ownerField: "userId"
            identityClaim: "sub"
            operations: [update, read]
         }
         { allow: groups, groups: ["Administrator"]   }
      ]
)

What I actually want from the model above is two things, I want the admin to be able to create/delete users and do all other operations while the user themselves can only read its own record and update it. And in both cases they should have the same tenant ID but what I end up getting right now is the user being allowed all operation because the tenantId check passes!

Essentially my proposal is a forced AND check that gets executed before all other auth rule checks. With allow: tenant that single auth rule is placed at the top of all resolvers and if tenant Id is does not match it just throws an unauthorized and wouldn't even check the other auth rules.

Edit: I gave this some thought and wanted to come up with the simplest solution while keeping with the current @auth rule writing convention.

mrducky4 commented 4 years ago

@amirmishani thanks, I understand now. It doesn't fit my use case though, because I still want to have an OR case where I can specify either the tenant has access, OR a member of a "superuser" group has access, where the superuser would not have the "custom:tenant_id" claim.

amirmishani commented 4 years ago

@mrducky4 got it. By the way, you raise an interesting point. I don't know if the current implementation allows for multiple group auth rules because the group auth rule takes an array so it's not currently needed. Also looking at the resolvers group rule I can see no numbers on the resolver vars like you'd see with owner rules indicating it may not be possible to have more than one group rule in your @auth. However once proposal 5 is implemented I can see a need to have multiple group rules. For example one tied to the tenant Id say the tenantAdmin and one without, like in your case the superAdmin or SysAdmin.

ghost commented 4 years ago

During my research, I found this https://github.com/aws-quickstart/saas-identity-cognito, having something like that with Amplify I think would be awesome for a lot of devs.

edlefebvre commented 4 years ago

Any news on the AND clause for the @auth?

RossWilliams commented 4 years ago

@edlefebvre aws-amplify/amplify-cli#3123 includes AND clause additions. its not an approved solution, and I don't have any insight on when or if it will be merged, so use with caution unless you want to maintain a fork.

edlefebvre commented 4 years ago

Thank you @RossWilliams ! I hope your solution will be merged, as I don't intend to use an unsupported version.

paulsson commented 4 years ago

I solved this using custom claims added by my cognito pre-token generation lambda trigger and using 'groupClaim' in combination with 'groupField' in my '@auth' rules to reference my custom claims. This requires you to pass the id_token instead of the access_token JWT in the 'Authorization' header unfortunately, but not a big deal. Just another hoop to jump through. This doesn't require any custom resolvers to be written at least. It is ashamed that this ticket has gotten no traction yet, as this would solve so many peoples problems to support multi-tenancy of customer data instead of having to slog through so much work and trouble shooting down in the weeds. You can see how many people are having this exact issue and hacking around it. Just google search 'amplify api auth multi tenancy' , for example: https://medium.com/@dantasfiles/multi-tenant-aws-amplify-cc3252c4def4 There are many github issues asking for this in different ways as well.

If we had the and/or functionality for defining '@auth' rules this would be super straight forward! @attilah @mikeparisstuff @RossWilliams @hisham @ammarkarachi we are all wasting hours or days or worse working around this and some are even giving up on Amplify as indicated by some of the above comments. Please prioritize our feedback...

PS, I love amplify

RossWilliams commented 4 years ago

I don’t work here. I submitted a PR to add this functionality but it was declined. There is a comment in that PR which suggests there are behind closed doors discussions for how to support expended auth options.

mvogttech commented 3 years ago

Any update on this? I have multiple apps now that I am waiting to launch -- the only thing holding it up is adding the OR and AND functionality for the @AUTH.

DevTGhosh commented 3 years ago

Is there any update on this? It's been 2 years since this has been requested. Not having the AND feature basically makes any kind of sass product an incredibly difficult thing to achieve with amplify.

bastianfitverse commented 3 years ago

Any update on this?

jgo80 commented 3 years ago

I solved this using custom claims added by my cognito pre-token generation lambda trigger and using 'groupClaim' in combination with 'groupField' in my '@auth' rules to reference my custom claims. This requires you to pass the id_token instead of the access_token JWT in the 'Authorization' header unfortunately, but not a big deal. Just another hoop to jump through. This doesn't require any custom resolvers to be written at least.

Hi @paulsson, your approach sounds promising, would it be possible you provide some more details on your resulting Schema and the lambda? Thank you in advance!

ronaldocpontes commented 3 years ago

@attilah @mikeparisstuff @RossWilliams @hisham @ammarkarachi is there any official estimate or workaround for adding the OR and AND functionality for the @AUTH or automating @paulsson solution? This would solve an important blocker to use Amplify as a solution to multi-tenant, multi-team apps.