amplify-category-api icon indicating copy to clipboard operation
amplify-category-api copied to clipboard

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

Open lennybr opened this issue 7 years ago • 91 comments

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:

  • Provide the ability to declare the combination logic before transformation so we could generate isOwner && isInGroup(x for x in cognitoGroups) when we have both rules types declared
  • Create a new @auth tenant strategy which uses the existing ownership transformation code behind the scenes but automatically changes the combination logic to isTenant && (isOwner || isInGroup(x for x in cognitoGroups))

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.

lennybr avatar Oct 18 '18 19:10 lennybr

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

hisham avatar Oct 18 '18 20:10 hisham

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

learningacct avatar Oct 19 '18 00:10 learningacct

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 avatar Oct 19 '18 19:10 mikeparisstuff

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:

  • The queries & mutations argument would only be respected on the top level rule. i.e. a nested rule such as { and: [{allow: owner, queries: null}] } will ignore the queries: null as it is only valid at the top level.
  • Providing multiple top level rules will continue to be joined by an OR. If you require only AND then use a single rule with the and key filled out.

mikeparisstuff avatar Oct 19 '18 20:10 mikeparisstuff

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.

lennybr avatar Oct 20 '18 17:10 lennybr

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.

learningacct avatar Oct 20 '18 19:10 learningacct

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 avatar Oct 20 '18 19:10 lennybr

@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)

lennybr avatar Oct 23 '18 16:10 lennybr

@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 avatar Oct 23 '18 23:10 mikeparisstuff

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.

mikeparisstuff avatar Oct 23 '18 23:10 mikeparisstuff

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?

learningacct avatar Oct 23 '18 23:10 learningacct

@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?

lennybr avatar Oct 31 '18 13:10 lennybr

@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.

mikeparisstuff avatar Nov 06 '18 22:11 mikeparisstuff

@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!

lennybr avatar Nov 13 '18 15:11 lennybr

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! :)

hisham avatar Nov 23 '18 22:11 hisham

@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.

mikeparisstuff avatar Dec 05 '18 20:12 mikeparisstuff

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

sollipse avatar Jan 02 '19 23:01 sollipse

@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?

nagey avatar Jan 03 '19 01:01 nagey

@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?

amirmishani avatar Mar 03 '19 21:03 amirmishani

@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 avatar May 02 '19 21:05 sebastienfi

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.

sebastienfi avatar May 02 '19 22:05 sebastienfi

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"]}])

lenarmazitov avatar Jun 03 '19 16:06 lenarmazitov

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?

tfendt avatar Nov 12 '19 01:11 tfendt

@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.

dantasfiles avatar Nov 12 '19 07:11 dantasfiles

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

RossWilliams avatar Nov 12 '19 11:11 RossWilliams

@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.

dantasfiles avatar Nov 12 '19 15:11 dantasfiles

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

mrducky4 avatar Nov 12 '19 18:11 mrducky4

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

dhmacs avatar Dec 10 '19 08:12 dhmacs

@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.

RossWilliams avatar Dec 10 '19 13:12 RossWilliams

@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

dhmacs avatar Dec 10 '19 14:12 dhmacs