Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Security] Handle placeholders in role hierarchy#52099
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
Conversation
df8fe61
toae44f50
CompareI really like this feature, but it adds a lot of complexity and I am not sure it's worth. |
Neirda24 commentedOct 20, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Nice ! I would suggest to not match on |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
I agree that adds complexity in the hierarchy config, but since it's optional we keep simple what should remain simple, no? The drawback I see is the config could be less meaningful as all roles involved in the hierarchy are not necessary listed, but here again, we let the choice to the developer ^^ 100% OK for a debug roles command, maybe something like this?
|
@Neirda24 Thanks! Which use cases would you like to avoid with this restriction? |
The latter. About your first suggestion you are right it should be matcheable. Maybe with |
Got it, I'll add the restriction for placeholder syntax. For |
1cabcf9
to0240d67
Compare@Neirda24 I updated the placeholder pattern matching inthis commit, looks fine to you? |
I'm not sure your example really show the (real) advantage of this feature. Current configuration:
You kinda uses your "roles" as "permission", so you could have written it like that no ?
|
Yes 👍 |
This configuration is not really equivalent. By giving the role |
That is why i used the term "permission", that for me is quite different from "role" (a role beeing itself an abstraction for a set of permissions)1 In your example, today you'd have to give this user "ROLE_BLOG_VIEW" and let's say "ROLE_SHOP_VIEW", and then you inherit ROLE_BLOG_USER and ROLE_SHOP_USER, then ROLE_USER. How does the new placeholder system help/ease things ? Again, ido think it's a great idea, but i'd like to see a before/after example to illustrate. In the one you gave, i don't see someone with ROLE_BLOG_REPORT but not ROLE_BLOG_VIEW. Footnotes
|
Uh oh!
There was an error while loading.Please reload this page.
I think it depends on the usage. Without a role hierarchy, what's behind a role depends on the code that is actually protected with it. If the role is used only to protect a single action (ex: create a specific kind of entity in database), then it acts like a permission. If it's used to protect multiple actions in various places, then it "grants" multiple things and acts like a set of permissions. With a role hierarchy, yes, roles used in keys are a set of permissions, as they inherit other ones. But I would think that roles used only in the values could then be either permissions or sets of permissions, with the same reasoning as above. I see roles as a special type of argument for the authorization layer, that triggers the role voter and look for matching elements in the user's token. But that's my understanding, I may be wrong 😁.
I think the main advantage here is tomap all potential roles that matches the pattern to a well-defined role name, that could be used to protect a domain section of the application easily. In the future, it will be easy to add/rename roles that will be covered with the pattern, without rewriting the hierarchy.
I agree that my first example is not sufficient to demonstrate the potential usages of the feature, and doesn't reflect a real configuration. Thanks for pointing this out! Rewriting this example like this could feel more real? Current configuration: role_hierarchy:ROLE_USER_SHOP:[ROLE_USER, ROLE_SHOP_VIEW]ROLE_USER_BLOG:[ROLE_USER, ROLE_BLOG_VIEW]ROLE_SHOP_BUY:ROLE_USER_SHOPROLE_SHOP_ADD_TO_CART:ROLE_USER_SHOPROLE_BLOG_POST:ROLE_USER_BLOGROLE_BLOG_REPORT:ROLE_USER_BLOGROLE_BLOG_COMMENT:ROLE_USER_BLOG With placeholder: role_hierarchy:ROLE_USER_*:ROLE_USERROLE_SHOP_*:ROLE_USER_SHOPROLE_USER_SHOP:ROLE_SHOP_VIEWROLE_BLOG_*:ROLE_USER_BLOGROLE_USER_BLOG:ROLE_BLOG_VIEW But now I find this use case too simple. It was intended to show the placeholder syntax, but it doesn't fit a real situation. I made another one that, I hope, better illustrates the advantages: Let's have an application with:
Current configuration: role_hierarchy:# Blog readers can view posts and add commentsROLE_BLOG_READER: -ROLE_USER -ROLE_BLOG_VIEW -ROLE_BLOG_COMMENT# Blog moderators are blog readers who can moderate comments and postsROLE_BLOG_MODERATOR: -ROLE_MODERATOR -ROLE_BLOG_READER -ROLE_BLOG_MODERATE_COMMENT -ROLE_BLOG_MODERATE_POST# Blog authors are blog readers who can create postsROLE_BLOG_AUTHOR: -ROLE_BLOG_READER -ROLE_BLOG_CREATE_POST# Shop users can view itemsROLE_SHOP_USER: -ROLE_USER -ROLE_SHOP_VIEW# Shop buyers are shop users who can view items, buy them, and post reviewsROLE_SHOP_BUYER: -ROLE_SHOP_USER -ROLE_SHOP_BUY_ITEM -ROLE_SHOP_POST_REVIEW# Shop sellers are shop users who can add itemsROLE_SHOP_SELLER: -ROLE_SHOP_USER -ROLE_SHOP_CREATE_ITEM# Shop moderators are shop users who and moderate items and reviewsROLE_SHOP_MODERATOR: -ROLE_SHOP_USER -ROLE_MODERATOR -ROLE_SHOP_MODERATE_ITEM -ROLE_SHOP_MODERATE_REVIEW With placeholders: role_hierarchy:# All users with roles have ROLE_USERROLE_*:ROLE_USER# All moderators have ROLE_MODERATORROLE_*_MODERATOR:ROLE_MODERATOR# Having a blog role allows to view the blogROLE_BLOG_*:ROLE_BLOG_VIEW# Blog readers can comment postsROLE_BLOG_READER: -ROLE_BLOG_COMMENT# Blog moderators can also moderate comments and postsROLE_BLOG_MODERATOR: -ROLE_BLOG_READER -ROLE_BLOG_MODERATE_COMMENT -ROLE_BLOG_MODERATE_POST# Blog authors can also create postsROLE_BLOG_AUTHOR: -ROLE_BLOG_READER -ROLE_BLOG_CREATE_POST# Having a shop role allows to view the shopROLE_SHOP_*:ROLE_SHOP_VIEW# Shop buyers can buy and review itemsROLE_SHOP_BUYER: -ROLE_SHOP_BUY_ITEM -ROLE_SHOP_POST_REVIEW# Shop sellers can add itemsROLE_SHOP_SELLER:ROLE_SHOP_CREATE_ITEM# Shop moderators can moderate items and reviewsROLE_SHOP_MODERATOR: -ROLE_SHOP_MODERATE_ITEM -ROLE_SHOP_MODERATE_REVIEW The advantages I see here:
The drawbacks:
Wdyt about this one? If it's better I can update the PR description with it. |
1db2351
to495b2ce
Compare
Absolutely perfect for me, thanks a lot 😄 (and you have now your documentation PR 100% ready 👏 ) |
Hi there! I worked on the
However I'm not sure about the strategy to debug the
Does it make sense to dynamically configure the command options depending on the current |
a2ef6a3
tod7209a0
CompareNice PR@squrious ! Wrapping I think we have to found an other way or make this command simpler. |
&& method_exists($this->decorated, 'getMatchingPlaceholders') | ||
) { | ||
$this->map = (new \ReflectionProperty($this->decorated, 'map'))->getValue($this->decorated); | ||
$this->hierarchy = (new \ReflectionProperty($this->decorated, 'hierarchy'))->getValue($this->decorated); |
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.
Using Reflection to access private properties is a bad idea. Using reflection to access private properties in a different package is a nightmare, as there is strictly no BC guarantee for those (the fact that the property exist does not even mean that it has the type that you expect here, as it might be a custom implementation as well)
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.
That's what I thought... Still trying to find a better solution
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.
I removed reflection stuff in favor of inheritance and a dedicated service for the debug purposes.
* | ||
* @return string|false The regex pattern, or false if there is no valid wildcard in the role | ||
*/ | ||
private function getPlaceholderPattern(string $role): string|false |
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.
The proper representation of the absence of value isnull
, notfalse
714413c
tof8db0a8
Compare
I reworked the solution to avoid the use of reflection. Now a dedicated I also added a changelog entry to the Security Bundle. But the tests fail because the command is in a different package, so maybe it would be better to add the command in another PR when/if this one gets merged? |
f8db0a8
to58e48ce
Compare58e48ce
to5c63850
Compare
Uh oh!
There was an error while loading.Please reload this page.
Hello!
The current role hierarchy configuration could be improved with placeholders. This could be useful for applications having roles whose naming structure reflects a domain organization, like
ROLE_BLOG_*
orROLE_SHOP_*
.Current configuration:
With placeholders:
This could also be useful to give
ROLE_USER
to any user having at least one role, like:Another, more complete, use case:
Let's have an application with:
/moderation
requires roleROLE_MODERATOR
).Current configuration:
With placeholders:
The advantages I see here:
ROLE_USER
.ROLE_MODERATOR
is easy to configure globally.The drawbacks: