-
-
Notifications
You must be signed in to change notification settings - Fork 8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add possibility to set "required" RBAC conditions #10185
Add possibility to set "required" RBAC conditions #10185
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10185 /- ##
==========================================
- Coverage 57.86% 57.82% -0.04%
==========================================
Files 183 184 1
Lines 6348 6393 45
Branches 1392 1395 3
==========================================
Hits 3673 3697 24
- Misses 2215 2233 18
- Partials 460 463 3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
1f361f8
to
133d4c5
Compare
packages/strapi-admin/services/permission/permissions-manager/query-builers.js
Outdated
Show resolved
Hide resolved
2354f4d
to
4ebb1eb
Compare
d6691fb
to
0cc30da
Compare
0cc30da
to
9c60f66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What does it do?
Add a new
willRegister
hook for the permission engine. This hook exposes a context which allow to add raw conditions to the permission.A handler for this hook would then be able to add condition with or / and logic to the final permission
Example:
All conditions added with an
and
are required to pass for the condition to be valid, whereas only one of the conditions added with anor
need to pass.Why is it needed?
Currently, all conditions are evaluated with an
or
logic. It can lead to issues when multiples conditions need to be matched (eg:is-creator
(rbac)has-locale-access
(i18n) => if one of the condition is matched, then the other doesn't matter)Related issue(s)/PR(s)
fix #10159
Notes
I'll submit a PR after this one to add more complete unit tests for the permission engine.