8000 Fixed the sample security voter to take into account the role hierarchy by javiereguiluz · Pull Request #5024 · symfony/symfony-docs · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@javiereguiluz
Copy link
Member
Q A
Doc fix? yes
New docs? no
Applies to 2.3+
Fixed tickets #4389

I applied the solution suggested by @jdachtera because it seems to be the best one.

Copy link
Member

Choose a reason for hiding this comment

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

typo: $roleHirarchy

Copy link
Member

Choose a reason for hiding this comment

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

wouldn't userHasRole be a better name. It now looks like a hasser/isser of the class itself

Copy link
Member

Choose a reason for hiding this comment

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

And it should be private imho.

@stof
Copy link
Member
stof commented Feb 21, 2015

The RoleHierarchy service will not exist if you don't use the hierarchy in your security config

@javiereguiluz
Copy link
Member Author

@stof thanks for the tip about the security.role_hierarchy service. I've just updated the sample code to take that into account.

Copy link
Member

Choose a reason for hiding this comment

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

this is wrong. You should still use $userToken->getRoles() (which are the only reachable roles in this case).

Copy link
Member

Choose a reason for hiding this comment

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

[...] hierarchy if defined [...] or [...] hierarchy as defined [...]?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should pass UserInterface here.

private function userHasRole(UserInterface $user, $roleName)

Copy link
Member Author

Choose a reason for hiding this comment

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

Much better. Fixed now. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

this is wrong. You should be using the roles from the token to be consistent with the way the RoleVoter is working

Copy link
Contributor

Choose a reason for hiding this comment

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

@javiereguiluz
Copy link
Member Author

Closing it because we can't fix what we want to fix (because of the comments of @stof and @aitboudad)

@javiereguiluz javiereguiluz deleted the fix_4389 branch January 3, 2018 16:34
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.

5 participants

0