(WIP) feat: add list permission
#1921
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for starting this @genu ! It's something wanted by many. I think the basic implementation should be straightforward. The call path for
zenstack/packages/runtime/src/enhancements/node/policy/policy-utils.ts Lines 609 to 612 in b41fd93 I think what needs to be done is to further inject "list" policies. Also, the "list" policy should only govern "findFirst" and "findMany", not "findUnique". We should probably introduce a boolean flag to distinguish these two cases. I guess the proxy code is not entirely easy to read and understand 😅. Let me know if you need more clarifications. |
|
Another complication is, whether "list" should govern |
I'm not exactly sure, but I think I would expect |
genu
force-pushed
the
feature/list_policy
branch
from
2278bf4 to
0161f57
Compare
|
@ymc9 Do we need to distinguish between between zenstack/packages/runtime/src/enhancements/node/policy/handler.ts Lines 133 to 151 in b41fd93 or should Also, why would |
I think 'list' should govern let r = await db.foo.findFirst();
const knownIds: string[] = [];
while (r) {
knownIds.push(r.id);
r = await db.foo.findFirst({ where: { not: { id: { in: knownIds } } } });
}We can probably introduce a boolean parameter to |
genu
force-pushed
the
feature/list_policy
branch
from
d18c2b6 to
1ccc6ac
Compare
…ProxyHandler and PolicyUtil
| if (isList && !this.policyUtils.injectForList(this.prisma, this.model, _args)) { | ||
| if (this.shouldLogQuery) { | ||
| this.logger.info(`[policy] \`${actionName}\` ${this.model}: unconditionally denied`); | ||
| } | ||
|
|
||
| return handleRejection(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm assuming this is the kind of check we need to do here?
| injectForList(_db: CrudContract, _model: string, _args: any) { | ||
| // make select and include visible to the injection | ||
| return true; | ||
| } |
There was a problem hiding this comment.
@ymc9
I'm not exactly sure if this is needed. Should the list logic go inside of the injectForRead?
The isList flag is on the handler doFind I'm a little confused on how the flow is.
There was a problem hiding this comment.
Yes, I agree this list logic should go inside injectForRead. Maybe we just rename it to injectFoReadOrList and pass in the flag there. I think the only difference is the call to this.injectAuthGuardAsWhere, for read the arg is "read", and "list" for list.
There was a problem hiding this comment.
There was a problem hiding this comment.
That part of code is not entirely easy to comprehend 😄. Please feel free to pass the PR to me to finish when you reach a point where you feel the basics are working.
|
@ymc9 Here are some ideas from thinking through this: findMany
findFirst
Nested to-many relations during read
Aggregate (This is kind of tricky)
Count
Maybe for an initial implementation we should stick to the obvious/straight forward approach and polish it later thoughts? |
|
Hi @genu , sorry I almost missed this comment. I think my thoughts generally aligns with your description. I'm sharing the differences below.
I think field-level and model-level permissions should work independently (as they do today). Model-level "list" determines if you can list entities, and field-level rules determine what fields you can see. You can't define "list" rules at the field level because it's meaningless.
The permission checking of fetching relations doesn't inherit from its parent, instead, for "read", the relation's model's "read" rules are evaluated to filter out items that shouldn't be seen, regardless if the parent is readable. I think "list" can behave the same way.
I agree "count" should require "list" permission. I don't see a separate permission kind is needed for it for now. We can iterate in the future as we gather feedback.
I totally agree. There're many details to sort out. Just having a single
Another major problem is how we add the "list" permission without introducing breaking changes, plus allowing incremental adoption. Here are my thoughts:
What do you think? |
|
@ymc9 Do you mind taking over this PR, I'm not exactly sure how to proceed. I'll keep an eye out for an updates and review/test as needed. |
|
@ymc9 your comments above make sense on how you expect it to work. I do have a suggestion though.
What I'm gathering from this is that this will just transparently work and start filtering out entries if it's not specifically defined right but the user has read access? If I'm correct in this understanding then I recommend below.
Since this is a breaking change for some users, I would recommend a flag to enable the filtering behaviour and have it off by default. This would allow people to do updates without items unexpectedly disappearing from their lists. E.g. it could be a feature flag in the zmodel as behaviour on or off globally (I don't mean for each model). I'm really happy with that behaviour so I would switch it on, but since it would affect what shows up when users do a list, it's worth having it opt in imho. |
Resolves #982
This PR is an initial attempt to add a
listpermission. Putting here for visibility.@ymc9 If you have some time, could you offer some guidance on the relevant code that would need to be updated, or anything else that would be helpful here.