Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: 7.4
Choose a base branch
from

Conversation

squrious
Copy link
Contributor

@squrious squrious commented Oct 17, 2023

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR symfony/symfony-docs#19079

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_* or ROLE_SHOP_*.

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_SHOP
  ROLE_SHOP_ADD_TO_CART:  ROLE_USER_SHOP

  ROLE_BLOG_POST:         ROLE_USER_BLOG
  ROLE_BLOG_REPORT:       ROLE_USER_BLOG
  ROLE_BLOG_COMMENT:      ROLE_USER_BLOG

With placeholders:

role_hierarchy:
  ROLE_USER_*:    ROLE_USER

  ROLE_SHOP_*:    ROLE_USER_SHOP
  ROLE_USER_SHOP: ROLE_SHOP_VIEW

  ROLE_BLOG_*:    ROLE_USER_BLOG
  ROLE_USER_BLOG: ROLE_BLOG_VIEW

This could also be useful to give ROLE_USER to any user having at least one role, like:

role_hierarchy:
  ROLE_*: ROLE_USER

Another, more complete, use case:

Let's have an application with:

  • A blog, with readers, authors, and moderators. Both authors and moderators are readers.
  • A shop, with basic users who can browse, buyers, sellers, and moderators. Sellers and moderators are not necessarily buyers.
  • A moderator zone, where moderators can see all pending requests or whatever, protected with access control (/moderation requires role ROLE_MODERATOR).

Current configuration:

role_hierarchy:
  # Blog readers can view posts and add comments
  ROLE_BLOG_READER: 
    - ROLE_USER
    - ROLE_BLOG_VIEW
    - ROLE_BLOG_COMMENT
  # Blog moderators are blog readers who can moderate comments and posts
  ROLE_BLOG_MODERATOR:
    - ROLE_MODERATOR
    - ROLE_BLOG_READER
    - ROLE_BLOG_MODERATE_COMMENT
    - ROLE_BLOG_MODERATE_POST
  # Blog authors are blog readers who can create posts
  ROLE_BLOG_AUTHOR: 
    - ROLE_BLOG_READER
    - ROLE_BLOG_CREATE_POST

  # Shop users can view items
  ROLE_SHOP_USER:
    - ROLE_USER
    - ROLE_SHOP_VIEW
  # Shop buyers are shop users who can view items, buy them, and post reviews
  ROLE_SHOP_BUYER: 
    - ROLE_SHOP_USER
    - ROLE_SHOP_BUY_ITEM
    - ROLE_SHOP_POST_REVIEW
  # Shop sellers are shop users who can add items 
  ROLE_SHOP_SELLER:
    - ROLE_SHOP_USER
    - ROLE_SHOP_CREATE_ITEM
  # Shop moderators are shop users who and moderate items and reviews
  ROLE_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_USER
  ROLE_*: ROLE_USER
  # All moderators have ROLE_MODERATOR
  ROLE_*_MODERATOR: ROLE_MODERATOR

  # Having a blog role allows to view the blog  
  ROLE_BLOG_*: ROLE_BLOG_VIEW
  # Blog readers can comment posts  
  ROLE_BLOG_READER:
    - ROLE_BLOG_COMMENT
  # Blog moderators can also moderate comments and posts
  ROLE_BLOG_MODERATOR:
    - ROLE_BLOG_READER
    - ROLE_BLOG_MODERATE_COMMENT
    - ROLE_BLOG_MODERATE_POST
  # Blog authors can also create posts 
  ROLE_BLOG_AUTHOR:
    - ROLE_BLOG_READER
    - ROLE_BLOG_CREATE_POST

  # Having a shop role allows to view the shop
  ROLE_SHOP_*: ROLE_SHOP_VIEW
  # Shop buyers can buy and review items
  ROLE_SHOP_BUYER:
    - ROLE_SHOP_BUY_ITEM
    - ROLE_SHOP_POST_REVIEW
  # Shop sellers can add items
  ROLE_SHOP_SELLER: ROLE_SHOP_CREATE_ITEM
  # Shop moderators can moderate items and reviews
  ROLE_SHOP_MODERATOR:
    - ROLE_SHOP_MODERATE_ITEM
    - ROLE_SHOP_MODERATE_REVIEW

The advantages I see here:

  • All roles grant ROLE_USER.
  • The role ROLE_MODERATOR is easy to configure globally.
  • It's easy to configure that anyone who can do something on the shop can view the shop, even if the role is not explicitly declared in the hierarchy configuration (for example a user who can only post reviews on items but not buy anything).
  • It's scalable: we can add more domain sections to the application, following the naming convention for moderators will handle their access to the moderator zone automatically.

The drawbacks:

  • It requires some naming conventions. But here the example is simple, I also consider more complex use cases for application having dozens of domain-specific roles, and I assume having a good naming convention is a must-have in this case.

@squrious squrious requested a review from chalasr as a code owner October 17, 2023 09:27
@carsonbot carsonbot added this to the 6.4 milestone Oct 17, 2023
@squrious squrious force-pushed the security/role_hierarchy_placeholder branch from df8fe61 to ae44f50 Compare October 17, 2023 09:29
@OskarStark
Copy link
Contributor

I really like this feature, but it adds a lot of complexity and I am not sure it's worth.
If this feature would get accepted, we would need a DebugRolesCommand IMHO 🤷‍♂️

@Neirda24
Copy link
Contributor

Neirda24 commented Oct 20, 2023

Nice ! I would suggest to not match on * only. But either prefix or suffixed with _. So * act as [a-zA-Z]+

@squrious
Copy link
Contributor Author

I really like this feature, but it adds a lot of complexity and I am not sure it's worth. If this feature would get accepted, we would need a DebugRolesCommand IMHO 🤷‍♂️

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?

$ bin/console debug:security:role-hierarchy

Role hierarchy configuration: 

ROLE_SHOP_* 
  => ROLE_USER_SHOP 
    => ROLE_USER_*
      => ROLE_USER
ROLE_BLOG_*
  => ROLE_USER_BLOG 
    => ROLE_USER_*
      => ROLE_USER
ROLE_USER_*
  => ROLE_USER

$ bin/console debug:security:role-hierarchy ROLE_SHOP_VIEW ROLE_BLOG_POST

Effective roles for ROLE_SHOP_VIEW, ROLE_BLOG_POST:
 - ROLE_SHOP_VIEW
 - ROLE_BLOG_POST
 - ROLE_USER_SHOP
 - ROLE_BLOG_USER
 - ROLE_USER

$ bin/console debug:security:role-hierarchy ROLE_SHOP_VIEW --tree
   
Effective roles for ROLE_SHOP_VIEW:
 - ROLE_SHOP_VIEW
 - ROLE_USER_SHOP
   - ROLE_USER

@squrious
Copy link
Contributor Author

Nice ! I would suggest to not match on * only. But either prefix or suffixed with _. So * act as [a-zA-Z]

@Neirda24 Thanks! Which use cases would you like to avoid with this restriction? ROLE_*_CREATE should not match roles like ROLE_BLOG_POST_CREATE? Or you think about the placeholder syntax itself, so ROLE* is not considered as a valid placeholder?

@Neirda24
Copy link
Contributor

Nice ! I would suggest to not match on * only. But either prefix or suffixed with _. So * act as [a-zA-Z]

@Neirda24 Thanks! Which use cases would you like to avoid with this restriction? ROLE_*_CREATE should not match roles like ROLE_BLOG_POST_CREATE? Or you think about the placeholder syntax itself, so ROLE* is not considered as a valid placeholder?

The latter. About your first suggestion you are right it should be matcheable. Maybe with ROLE_**_CREATE to allow any number of words in between ?

@squrious
Copy link
Contributor Author

Nice ! I would suggest to not match on * only. But either prefix or suffixed with _. So * act as [a-zA-Z]

@Neirda24 Thanks! Which use cases would you like to avoid with this restriction? ROLE_*_CREATE should not match roles like ROLE_BLOG_POST_CREATE? Or you think about the placeholder syntax itself, so ROLE* is not considered as a valid placeholder?

The latter. About your first suggestion you are right it should be matcheable. Maybe with ROLE_**_CREATE to allow any number of words in between ?

Got it, I'll add the restriction for placeholder syntax.

For **, I'm not sure, I mean it will force the use of _ as a word separator, and thus introduce the concept of words in the role syntax, but only when using placeholders. If find it a little bit confusing, while * could just mean match anything here.

@squrious squrious force-pushed the security/role_hierarchy_placeholder branch 2 times, most recently from 1cabcf9 to 0240d67 Compare October 20, 2023 19:02
@squrious
Copy link
Contributor Author

@Neirda24 I updated the placeholder pattern matching in this commit, looks fine to you?

@smnandre
Copy link
Member

I'm not sure your example really show the (real) advantage of this feature.

Current configuration:

role_hierarchy:
  ROLE_USER_SHOP:         ROLE_USER
  ROLE_USER_BLOG:         ROLE_USER
  
  ROLE_SHOP_VIEW:         ROLE_USER_SHOP
  ROLE_SHOP_BUY:          ROLE_USER_SHOP
  ROLE_SHOP_ADD_TO_CART:  ROLE_USER_SHOP
  
  ROLE_BLOG_VIEW:         ROLE_USER_BLOG 
  ROLE_BLOG_POST:         ROLE_USER_BLOG
  ROLE_BLOG_REPORT:       ROLE_USER_BLOG
  ROLE_BLOG_COMMENT:      ROLE_USER_BLOG

You kinda uses your "roles" as "permission", so you could have written it like that no ?

role_hierarchy:
  ROLE_USER:                 [ROLE_USER_BLOG, ROLE_USER_SHOP]
  ROLE_USER_BLOG:     [ROLE_BLOG_VIEW, ROLE_BLOG_POST, ROLE_BLOG_REPORT, ROLE_BLOG_COMMENT]
  ROLE_USER_SHOP:     [ROLE_SHOP_VIEW, ROLE_SHOP_BUY, ROLE_SHOP_ADD_TO_CART]

@OskarStark
Copy link
Contributor

100% OK for a debug roles command, maybe something like this?

Yes 👍

@squrious
Copy link
Contributor Author

You kinda uses your "roles" as "permission", so you could have written it like that no ?

role_hierarchy:
  ROLE_USER:                 [ROLE_USER_BLOG, ROLE_USER_SHOP]
  ROLE_USER_BLOG:     [ROLE_BLOG_VIEW, ROLE_BLOG_POST, ROLE_BLOG_REPORT, ROLE_BLOG_COMMENT]
  ROLE_USER_SHOP:     [ROLE_SHOP_VIEW, ROLE_SHOP_BUY, ROLE_SHOP_ADD_TO_CART]

This configuration is not really equivalent. By giving the role ROLE_USER to someone you also give all inherited roles. In my example, you can give the single role ROLE_BLOG_VIEW to a user, and thus ROLE_USER_BLOG and ROLE_USER are inherited, but not the others (ROLE_BLOG_POST, ROLE_SHOP_VIEW, ROLE_USER_SHOP, etc.).

@smnandre
Copy link
Member

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, i do 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

  1. even if Symfony sometimes uses both, like ROLE_ADMIN & ROLE_ALLOWED_TO_SWITCH.

@squrious
Copy link
Contributor Author

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

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 😁.

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 ?

I think the main advantage here is to map 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.

Again, i do 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.

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_SHOP
  ROLE_SHOP_ADD_TO_CART:  ROLE_USER_SHOP
  
  ROLE_BLOG_POST:         ROLE_USER_BLOG
  ROLE_BLOG_REPORT:       ROLE_USER_BLOG
  ROLE_BLOG_COMMENT:      ROLE_USER_BLOG

With placeholder:

role_hierarchy:
  ROLE_USER_*:    ROLE_USER
  
  ROLE_SHOP_*:    ROLE_USER_SHOP
  ROLE_USER_SHOP: ROLE_SHOP_VIEW
  
  ROLE_BLOG_*:    ROLE_USER_BLOG
  ROLE_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:

  • A blog, with readers, authors, and moderators. Both authors and moderators are readers.
  • A shop, with basic users who can browse, buyers, sellers, and moderators. Sellers and moderators are not necessarily buyers.
  • A moderator zone, where moderators can see all pending requests or whatever, protected with access control (/moderation requires role ROLE_MODERATOR).

Current configuration:

role_hierarchy:
  # Blog readers can view posts and add comments
  ROLE_BLOG_READER: 
    - ROLE_USER
    - ROLE_BLOG_VIEW
    - ROLE_BLOG_COMMENT
  # Blog moderators are blog readers who can moderate comments and posts
  ROLE_BLOG_MODERATOR:
    - ROLE_MODERATOR
    - ROLE_BLOG_READER
    - ROLE_BLOG_MODERATE_COMMENT
    - ROLE_BLOG_MODERATE_POST
  # Blog authors are blog readers who can create posts
  ROLE_BLOG_AUTHOR: 
    - ROLE_BLOG_READER
    - ROLE_BLOG_CREATE_POST

  # Shop users can view items
  ROLE_SHOP_USER:
    - ROLE_USER
    - ROLE_SHOP_VIEW
  # Shop buyers are shop users who can view items, buy them, and post reviews
  ROLE_SHOP_BUYER: 
    - ROLE_SHOP_USER
    - ROLE_SHOP_BUY_ITEM
    - ROLE_SHOP_POST_REVIEW
  # Shop sellers are shop users who can add items 
  ROLE_SHOP_SELLER:
    - ROLE_SHOP_USER
    - ROLE_SHOP_CREATE_ITEM
  # Shop moderators are shop users who and moderate items and reviews
  ROLE_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_USER
  ROLE_*: ROLE_USER
  # All moderators have ROLE_MODERATOR
  ROLE_*_MODERATOR: ROLE_MODERATOR

  # Having a blog role allows to view the blog  
  ROLE_BLOG_*: ROLE_BLOG_VIEW
  # Blog readers can comment posts  
  ROLE_BLOG_READER:
    - ROLE_BLOG_COMMENT
  # Blog moderators can also moderate comments and posts
  ROLE_BLOG_MODERATOR:
    - ROLE_BLOG_READER
    - ROLE_BLOG_MODERATE_COMMENT
    - ROLE_BLOG_MODERATE_POST
  # Blog authors can also create posts 
  ROLE_BLOG_AUTHOR:
    - ROLE_BLOG_READER
    - ROLE_BLOG_CREATE_POST

  # Having a shop role allows to view the shop
  ROLE_SHOP_*: ROLE_SHOP_VIEW
  # Shop buyers can buy and review items
  ROLE_SHOP_BUYER:
    - ROLE_SHOP_BUY_ITEM
    - ROLE_SHOP_POST_REVIEW
  # Shop sellers can add items
  ROLE_SHOP_SELLER: ROLE_SHOP_CREATE_ITEM
  # Shop moderators can moderate items and reviews
  ROLE_SHOP_MODERATOR:
    - ROLE_SHOP_MODERATE_ITEM
    - ROLE_SHOP_MODERATE_REVIEW

The advantages I see here:

  • All roles grant ROLE_USER.
  • The role ROLE_MODERATOR is easy to configure globally.
  • It's easy to configure that anyone who can do something on the shop can view the shop, even if the role is not explicitly declared in the hierarchy configuration (for example a user who can only post reviews on items but not buy anything).
  • It's scalable: we can add more domain sections to the application, following the naming convention for moderators will handle their access to the moderator zone automatically.

The drawbacks:

  • It requires some naming conventions. But here the example is simple, I also consider more complex use cases for application having dozens of domain-specific roles, and I assume having a good naming convention is a must-have in this case.

Wdyt about this one? If it's better I can update the PR description with it.

@squrious squrious force-pushed the security/role_hierarchy_placeholder branch from 1db2351 to 495b2ce Compare October 23, 2023 15:09
@smnandre
Copy link
Member

Wdyt about this one? If it's better I can update the PR description with it.

Absolutely perfect for me, thanks a lot 😄 (and you have now your documentation PR 100% ready 👏 )

@squrious
Copy link
Contributor Author

squrious commented Nov 7, 2023

Hi there!

I worked on the debug:roles command. Here are some usages:

bin/console debug:roles:

image

bin/console debug:roles --tree:

image

bin/console debug:roles ROLE_BLOG_MODERATOR:

image

bin/console debug:roles ROLE_BLOG_MODERATOR --tree:

image

However I'm not sure about the strategy to debug the RoleHierarchy. I added a wrapper so it can access needed variables through reflection, but in find it weak at the moment:

  • It doesn't work with custom implementations of RoleHierarchyInterface
  • I needed to access the method to match placeholders through reflection too, and at this point I wonder if a dedicated service for the placeholder matching logic would be better

Does it make sense to dynamically configure the command options depending on the current RoleHierarchyInterface implementation? The tree view is only applicable for the built in impl, but it could be nice to still resolve reachable roles for custom impl.

@squrious squrious force-pushed the security/role_hierarchy_placeholder branch 2 times, most recently from a2ef6a3 to d7209a0 Compare November 7, 2023 10:44
@Jean-Beru
Copy link
Contributor

Nice PR @squrious !

Wrapping RoleHierarchyInterface to access protected/private properties/methods could introduce bugs when using a custom implementation (as you said) or decorating it.

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);
Copy link
Member

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)

Copy link
Contributor Author

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

Copy link
Contributor Author

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
Copy link
Member

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 is null, not false

@squrious squrious force-pushed the security/role_hierarchy_placeholder branch 4 times, most recently from 714413c to f8db0a8 Compare November 14, 2023 10:04
@squrious
Copy link
Contributor Author

I think we have to found an other way or make this command simpler.

I reworked the solution to avoid the use of reflection. Now a dedicated debug.security.role_hierarchy is registered and injected in the command. If the role hierarchy uses a custom implementation, it is injected "as it is" and the command is only able to display reachable role names. I also added some tests to enforce this behavior.

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?

@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@squrious squrious force-pushed the security/role_hierarchy_placeholder branch from f8db0a8 to 58e48ce Compare January 2, 2024 10:52
@squrious squrious force-pushed the security/role_hierarchy_placeholder branch from 58e48ce to 5c63850 Compare March 18, 2024 09:53
@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@fabpot fabpot modified the milestones: 7.3, 7.4 May 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants