NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[Security] Add retrieval of encompassing role names#53998

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

Jump to bottom

Open

PierreCapel wants to merge1 commit intosymfony:7.4

base:7.4

Choose a base branch

fromPierreCapel:security-get-encompassing-role-names

Open

[Security] Add retrieval of encompassing role names#53998

PierreCapel wants to merge1 commit intosymfony:7.4fromPierreCapel:security-get-encompassing-role-names

Conversation

Copy link

Contributor

PierreCapel commentedFeb 19, 2024•
edited by nicolas-grekas
Loading

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
License	MIT

The aim of this method is to provide a handy way of getting the roles that encompass (or are parent of) an array of roles.

It is similar to theRoleHierarchyInterface::getReachableRoleNames(array $roles) but instead of retrieving the roles and children roles it retrieves the roles and parent roles.

A typical use case would be when we get a user role from a database and need to get all the roles that also have access to whatever this role can access.

Also what do you guys think of renaming the existinggetReachableRoleNames (that retrieves the "children roles" of an array of roles) as well asgetEncompassingRoleNames (that retrieves the "parent roles" of an array of roles) togetParentRoles andgetChildrenRoles in order to better reflect their intention ?

For the sake of this PR I tried to use a naming that is consistent with the existinggetReachableRoleNames method.

PierreCapel requested a review fromchalasr as acode owner

February 19, 2024 14:54

carsonbot added Status: Needs Review Feature Security labels

Feb 19, 2024

carsonbot added this to the7.1 milestone

Feb 19, 2024

Copy link

carsonbot commentedFeb 19, 2024

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

PierreCapel force-pushed thesecurity-get-encompassing-role-names branch 2 times, most recently fromc48453b tof1cc872Compare

February 19, 2024 15:03

nicolas-grekas reviewed

Feb 20, 2024

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

random review :)

src/Symfony/Component/Security/Core/CHANGELOG.md OutdatedShow resolvedHide resolved

src/Symfony/Component/Security/Core/Role/RoleHierarchy.php OutdatedShow resolvedHide resolved

PierreCapel force-pushed thesecurity-get-encompassing-role-names branch fromf1cc872 toa21bceeCompare

February 20, 2024 13:06

[Security] Add retrieval of encompassing role names

983742e

The aim of this method is to provide a handy way of getting the roles that encompass (or are parent of) an array of roles.It is similar to the getReachableRoleNames from the same interface but instead of retrieving the children roles it retrieves the parent roles.A typical use case would be when we get a user role from a database and need to get all the roles that also have access to what this role can access.

PierreCapel force-pushed thesecurity-get-encompassing-role-names branch froma21bcee to983742eCompare

February 20, 2024 13:08