zenstack icon indicating copy to clipboard operation
zenstack copied to clipboard

[Feature Request] Devise a way to reuse policies from a related model

Open ymc9 opened this issue 2 years ago • 11 comments

image

ymc9 avatar Mar 19 '23 13:03 ymc9

@ymc9 this feature would be a huge help in simplifying the access controls. Currently we have to use 7 layer deep nested access, and copying it to each child models is a laborious task, as well as maintaining it over time.

I will try working on the feature once I get some time free. Would like to learn the internals of zenstack :)

sidharthv96 avatar Mar 21 '24 05:03 sidharthv96

@ymc9 this feature would be a huge help in simplifying the access controls. Currently we have to use 7 layer deep nested access, and copying it to each child models is a laborious task, as well as maintaining it over time.

I will try working on the feature once I get some time free. Would like to learn the internals of zenstack :)

Contribution would be great @sidharthv96 ! @jiashengguo and I were discussing this feature two days ago 😄.

Just to confirm, the duplication you currently have is caused by the need to duplicate rules in a model and in other models where there's a relation to that model, right? As explained in the main post of this issue.

ymc9 avatar Mar 21 '24 17:03 ymc9

I think it might be slightly different from the mentioned issue.

In our simplified use case, a user in a team should be able to access all diagrams in that team. So we have some complex rules in diagrams table, based on the user role, team membership, whether the diagram is public or not, etc.

When adding a related object to the diagram, say attachments or comments, now we have to write all these rules, with one extra layer (diagram.), in each of the child models.

By your suggested approach, we could simply add a check(diagram, 'action') instead to the complex rules. Which would be much cleaner and maintainable.


@ymc9 do you have some pointers on where I should get started? Or some general idea on where all the changes would need to be made.

sidharthv96 avatar Mar 21 '24 17:03 sidharthv96

I think it might be slightly different from the mentioned issue.

In our simplified use case, a user in a team should be able to access all diagrams in that team. So we have some complex rules in diagrams table, based on the user role, team membership, whether the diagram is public or not, etc.

When adding a related object to the diagram, say attachments or comments, now we have to write all these rules, with one extra layer (diagram.), in each of the child models.

By your suggested approach, we could simply add a check(diagram, 'action') instead to the complex rules. Which would be much cleaner and maintainable.

@ymc9 do you have some pointers on where I should get started? Or some general idea on where all the changes would need to be made.

Got it. Yes, I agree a check function could really help here.

I thought about it, and maybe it's not too complex. Here're some ideas:

  1. The check function should be declared in stdlib.zmodel. It should be marked to be used only in access policy context. You can use the future() function as a reference.

  2. Access policies are translated by the "@core/access-policy" (/packages/schema/src/plugins/access-policy) built-in plugin into typescript functions. Each model has separate functions for CRUD permissions. The functions take a context as input, and return a partial Prisma query filter. At runtime, ZenStack calls these functions and then inject into query payload.

    Here's an example:

    model List {
      id Int
      todos Todo[]
      owner User @relation(...)
    
      @@allow('read', owner == auth())
    }
    
    model Todo {
      id Int
      list List @relation(...)
    
      @@allow('read', list.owner == auth())
    }
    

    The generated function will look like (simplified):

    function List_read(context) {
      return { owner: { id: context.user.id } };
    }
    
    return Todo_read(context) {
      return { list: {  owner: { id: context.user.id } } };
    }
    

    Now with the check function, the model can be changed to:

    model List {
      id Int
      todos Todo[]
      owner User @relation(...)
    
      @@allow('read', owner == auth())
    }
    
    model Todo {
      id Int
      list List @relation(...)
    
      @@allow('read', check('read', list))
    }
    

    And we can generate a function call to List_read from Todo_read:

    function List_read(context) {
      return { owner: { id: context.user.id } };
    }
    
    return Todo_read(context) {
      return { list: List_read(context) };
    }
    

The code that transforms ZModel AST to the policy functions as shown above resides in packages/schema/src/plugins/access-policy/expression-writer.

One caveat is that there can potentially be cyclic reference, resulting in infinite recursion. I'm not sure what's the best way to handle it ... Maybe we just disallow cycles first during validation?

ymc9 avatar Mar 22 '24 04:03 ymc9

Thank you.

Maybe we just disallow cycles first during validation?

This seems like the best option.

sidharthv96 avatar Mar 22 '24 10:03 sidharthv96

@ymc9 is this test, what should be the output?

it('auth using parent relation', async () => {
        await check(
            `
                model User {
                    id String @id
                    posts Post[]
                }

                model Post {
                    id String @id
                    owner User @relation(fields: [ownerId], references: [id])
                    ownerId String @unique
                    comments Test[]
                    @@allow('all', auth() == owner)
                }

                model Test {
                    id String @id
                    post Post @relation(fields: [postId], references: [id])
                    postId String @unique
                    @@allow('all', check(post, 'read'))
                }
                `,
            (model) => model.attributes[0].args[1].value,
            `<what should be the case?>`
        );
    });
const option1 = {
  post: {
    ...(user == null
      ? { OR: [] }
      : {
          owner: {
            is: {
              id: user.id,
            },
          },
        }),
  },
};
const option2 = user == null ? { OR: [] } : { post: { owner: { is: { id: user.id } } } };
image image

This is what I'm assuming needs to be done. Please let me know if I'm on the right path.

sidharthv96 avatar Mar 22 '24 20:03 sidharthv96

auth using parent relation

I think it should be the option2 because the object will be directly as as a filter for model Test.

We shouldn't need to change TypeScriptExpressionTransformer because that class is for compiling ZModel expressions into TS expressions (for supporting @@validate etc.). The target should be the ExpressionWriter class which transforms expressions into Prisma query objects.

Btw, I suggest we swap the parameters of check to check('read', post), to make it look more consistent with @@allow.

You're making changes of the dev branch, right?

ymc9 avatar Mar 22 '24 20:03 ymc9

Btw, I suggest we swap the parameters of check to check('read', post), to make it look more consistent with @@allow.

image

check(post, 'read') is consistent with all the other function like search, startsWith, etc.


And we can generate a function call to List_read from Todo_read:

function List_read(context) {
  return { owner: { id: context.user.id } };
}
     
function Todo_read(context) {
  return { list: List_read(context) };
}

I think it should be the option2 because the object will be directly as as a filter for model Test.

const option1 = { post: user == null ? { OR: [] } : { owner: { is: { id: user.id } } } };
const option2 = user == null ? { OR: [] } : { post: { owner: { is: { id: user.id } } } }

Option 1 seems more inline with the example you mentioned earlier. There, we're only manipulating the children, in option 2, we'd have to modify the parent based on the conditions that the children might have. Is that feasible?

You're making changes of the dev branch, right?

Yes 👍

sidharthv96 avatar Mar 23 '24 04:03 sidharthv96

From my understanding so far, given a statement like @@allow(<action>, <expression>), ExpressionWriter only handles the <expression> part, and does not bother with <action>.

So, if we have to write @@allow('read', check(post, 'read')), ExpressionWriter would have to somehow use PolicyGenerator to figure out what expressions are allowing/denying post's read.

Is there any way for ExpressionWriter to figure out user == null ? { OR: [] } : { owner: { is: { id: user.id } } } without using PolicyGenerator? If Post had another allow operation, the condition would be different.

sidharthv96 avatar Mar 23 '24 04:03 sidharthv96

From my understanding so far, given a statement like @@allow(<action>, <expression>), ExpressionWriter only handles the <expression> part, and does not bother with <action>.

So, if we have to write @@allow('read', check(post, 'read')), ExpressionWriter would have to somehow use PolicyGenerator to figure out what expressions are allowing/denying post's read.

Is there any way for ExpressionWriter to figure out user == null ? { OR: [] } : { owner: { is: { id: user.id } } } without using PolicyGenerator? If Post had another allow operation, the condition would be different.

I see what you mean now.

ExpressionWriter's job is to turn a boolean expression into a prisma filter. In a more complex use case one can mix check call with other conditions to form a deep boolean tree, like: @@allow('read', check(post, 'read') && ...). ExpressionWriter deals with traversing such a tree. I think it's easier to let it continue processing the entire policy condition expression.

My thought is that when transforming check(post, 'read'), we don't really need to generate an inlined filter based on "read" policies of "post". Instead, we can generate a TS function call to Post_read (which is generated by PolicyGenerator), so the final composition of the policy rules happens at runtime. Still, it will result in some implicit coupling between ExpressionWriter and PolicyGenerator because the writer needs to know the naming convention of the generated policy functions. But I think it's OK since these two guys are tightly coupled anyway.

Also, I want to make a correction to my previous reply. I think option1 and option2 are equivalent so either of them should work. If we take the function call approach, it'll be just whatever the result is with the call 😄.

I'm not sure if I explained my thoughts clearly. Let me know if anything is confusing or doesn't sound right.

ymc9 avatar Mar 23 '24 20:03 ymc9

Btw, you can use this method to see he pre-compiled policies in TS form.

plugin policy {
  provider = '@core/access-policy'
  preserveTsFiles = true

After running zenstack generate, you can find the "policy.ts" file under "node_modules/.zenstack" folder.

ymc9 avatar Mar 24 '24 16:03 ymc9