zenstack icon indicating copy to clipboard operation
zenstack copied to clipboard

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

Open boptom opened this issue 2 years ago • 12 comments

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!

  • ZenStack version: 1.1.1
  • Prisma version: 5.4.1
  • Database type: postgresql 15.1

boptom avatar Oct 24 '23 06:10 boptom

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

antran511 avatar Oct 27 '23 09:10 antran511

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?

boptom avatar Oct 27 '23 10:10 boptom

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

jiashengguo avatar Oct 27 '23 12:10 jiashengguo

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

Azzerty23 avatar Oct 27 '23 13:10 Azzerty23

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

jiashengguo avatar Oct 27 '23 14:10 jiashengguo

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

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

ymc9 avatar Oct 29 '23 14:10 ymc9

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

boptom avatar Oct 30 '23 06:10 boptom

_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

jiashengguo avatar Oct 30 '23 10:10 jiashengguo

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

boptom avatar Oct 31 '23 01:10 boptom

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

jiashengguo avatar Nov 04 '23 11:11 jiashengguo

Ok good to know. Thanks!

boptom avatar Nov 04 '23 23:11 boptom

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 avatar Dec 10 '23 23:12 ymc9

Fixed in v2.2.0

ymc9 avatar Jun 10 '24 04:06 ymc9