zenstackhq / zenstack

Fullstack TypeScript toolkit that enhances Prisma ORM with flexible Authorization layer for RBAC/ABAC/PBAC/ReBAC, offering auto-generated type-safe APIs and frontend hooks.
https://zenstack.dev
MIT License
2.07k stars 88 forks source link

@@deny comparing user in model and related model not working #786

Closed boptom closed 4 months ago

boptom commented 11 months ago

Taking the example here: https://zenstack.dev/docs/reference/zmodel-language#a-more-complex-example-with-multi-user-spaces

I would like to prevent deletions of the member who is also the space owner.

Within model Membership:

@@deny('delete', space.owner == user)

I would expect this would stop delete of the space owner's membership. However, the deletion is still allowed.

Is this related to the limitations here? https://zenstack.dev/docs/reference/limitations#comparison-between-fields-in-access-policies

If so, I apologise for this bug report, and suggest adding an example such as the above to this page.

Thanks!

antran511 commented 11 months ago

This comparison of space.owner == user may result in false so @@deny doesn't work in this case

boptom commented 11 months ago

Sorry, I’m not following. I’m trying to prevent the removal of the owner from the membership model. That means I want it to be false for every user except the owner.

I thought the conditional/comparison here would work. If I’m mistaken then how could I achieve this with zenatack?

jiashengguo commented 11 months ago

@boptom. If you want to prevent deletions even for the owner, so it looks like no one would be able to delete it, right? If so you can simply just remove the delete operation from that policy rule:

    // space owner can create/update
    @@allow('create,update', space.owner == auth())

Or in your case, you should use auth() which represent the current user instead of user

@@deny('delete', space.owner == auth())
Azzerty23 commented 11 months ago

@jiashengguo I believe he wants to prevent the deletion of the owner membership, not a membership by the owner.

jiashengguo commented 11 months ago

@Azzerty23 You are right, thanks for correcting me!

@boptom Unfortunately due to the restoration of Prisma client the two fields have to be in the same model, that one is not working in ZenStack now: https://www.prisma.io/docs/reference/api-reference/prisma-client-reference#fields-must-be-in-the-same-model

However, I guess using the below alternative rule to prevent the owner deleting himself should meet your requirement:

@@deny('delete', space.owner == auth() && user == auth())

ymc9 commented 11 months ago

The unsupported usage of field comparison should trigger a validation error though:

@@deny('delete', space.owner == user)
boptom commented 11 months ago

@Azzerty23 You are right, thanks for correcting me!

@boptom Unfortunately due to the restoration of Prisma client the two fields have to be in the same model, that one is not working in ZenStack now: https://www.prisma.io/docs/reference/api-reference/prisma-client-reference#fields-must-be-in-the-same-model

However, I guess using the below alternative rule to prevent the owner deleting himself should meet your requirement:

@@deny('delete', space.owner == auth() && user == auth())

Unfortunately, I'd like other users other than the owner (e.g. administrators) be able to remove members.

The unsupported usage of field comparison should trigger a validation error though:

@@deny('delete', space.owner == user)

I don't think it does. Is there a playground, or a quick way for me to test it?

jiashengguo commented 11 months ago

_Unfortunately, I'd like other users other than the owner (e.g. administrators) be able to remove members.

The access is denied by default if no access rule is specified. So for your case, you can use white-list to turn it on like this:

enum SpaceUserRole {
    USER
    ADMIN
}
model User {
    ...
    role SpaceUserRole
   ...
}
model Membership {
     ...
    // space owner can create/update
    @@allow('create,delete', space.owner == auth())

   // space admin could delete
    @@allow('delete', auth().role == ADMIN)
}

Just make sure the user object you passed to enhance wrapper has the role property

I don't think it does. Is there a playground, or a quick way for me to test it?

you are right, that's an issue we need to fix. We don't have a playground yet, but I agree we should make one. Now, I usually use this simple express.js example to do test: https://github.com/zenstackhq/saas-backend-template

boptom commented 11 months ago

_Unfortunately, I'd like other users other than the owner (e.g. administrators) be able to remove members.

The access is denied by default if no access rule is specified. So for your case, you can use white-list to turn it on like this:

enum SpaceUserRole {
    USER
    ADMIN
}
model User {
    ...
    role SpaceUserRole
   ...
}
model Membership {
     ...
    // space owner can create/update
    @@allow('create,delete', space.owner == auth())

   // space admin could delete
    @@allow('delete', auth().role == ADMIN)
}

Just make sure the user object you passed to enhance wrapper has the role property

That's what I'm currently doing atm. Am I correct in thinking this wouldn't solved the issue of not allowing the owner to be removed from the membership table?

jiashengguo commented 11 months ago

@boptom, you are right. Unless you are willing to duplicate spaceOwnerId in the Membership model, there is no way to express it using the access policy now.

model Membership {
     ...

    // one-to-many from Space
    space Space @relation(fields: [spaceId], references: [id])
    spaceId String

    spaceOwnerId String

    // one-to-many from User
    user User @relation(fields: [userId], references: [id])
    userId String

       // don't allow to delete owner
    @@deny('delete', spaceOwnerId == userId)
}
boptom commented 11 months ago

Ok good to know. Thanks!

ymc9 commented 10 months ago

Reopening this issue for thinking about a real solution. The main problem is that there's no way to compare fields from different models in Prisma, and to get it to work, we need to do it in a read-then-check fashion, which can result in bad performance when the number of rows needed to read is large.

@Azzerty23 suggested that we can still have such an implementation but somehow let the user know the potential perf problem. He proposed:

@@deny('delete', @fetch(space.owner.id) == userId)

Alternative, I think we can also keep the schema clean but detect such expensive operation at zenstack generate time (or maybe runtime too) and emit warnings.

ymc9 commented 4 months ago

Fixed in v2.2.0