Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] deprecate the Role and SwitchUserRole classes#22048
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
a8df9f2 to1aec9b7CompareThis PR was merged into the 2.7 branch.Discussion----------[Security] simplify the SwitchUserListenerTest| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |While working on#22048 I noticed that the `SwitchUserListenerTest` was more complicated than necessary by mocking a lot of stuff that didn't need to be mocked.Commits-------923bbdb [Security] simplify the SwitchUserListenerTest
src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
xabbuh commentedJul 12, 2017
@symfony/deciders I would like to hear your opinion on this topic? Do you think it's worth to finish this approach? Should we take another road? |
src/Symfony/Component/Security/Core/Authentication/Token/UsernamePasswordToken.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedAug 31, 2017
chalasr commentedAug 31, 2017
IMO it's a good move. We still need to be able to get both impersonator/impersonated user from the token, I guess it's planed |
xabbuh commentedAug 31, 2017
chalasr commentedAug 31, 2017
Agreed, that would remove the need for |
nicolas-grekas commentedSep 26, 2017
@xabbuh will you have tome to finish this PR for 3.4? |
xabbuh commentedSep 26, 2017
Yes, I am working on it. |
nicolas-grekas commentedOct 8, 2017
should we move to 4.1? |
xabbuh commentedOct 8, 2017
I think so. |
1aec9b7 todaf09cbComparedaf09cb to7194244Compare7194244 to2054024Compareb871a65 tob2cfbb3Compareb2cfbb3 tod7aaa61Comparexabbuh commentedFeb 22, 2019
tests pass now Status: Needs Review |
javiereguiluz left a comment
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.
Nice!
src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
fabpot commentedFeb 25, 2019
Thank you@xabbuh. |
…es (xabbuh)This PR was merged into the 4.3-dev branch.Discussion----------[Security] deprecate the Role and SwitchUserRole classes| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#20824| License | MIT| Doc PR |symfony/symfony-docs#11047In#20801, we deprecated the `RoleInterface`. The next logical step would be to also deprecate the `Role` class. However, we currently have the `SwitchUserRole` class (a sub-class of `Role`) that acts as an indicator to check whether or not the authenticated user switched to another user.This PR proposes an alternative solution to the usage of the special `SwitchUserRole` class by storing the original token inside the `UsernamePasswordToken`. This PR is not complete, but rather acts as a proof of concept of how we could get rid of the `Role` and the `SwitchUserRole` classes.Please share your opinions whether you think this is a valid approach and I will be happy to finalise the PR.Commits-------d7aaa61 deprecate the Role and SwitchUserRole classes
This PR was merged into the master branch.Discussion----------document the deprecation of the role classesseesymfony/symfony#22048Commits-------0fbef77 document the deprecation of the role classes
| * | ||
| * @author Fabien Potencier <fabien@symfony.com> | ||
| * | ||
| * @deprecated since Symfony 4.3, to be removed in 5.0. |
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.
Why deprecating this interface instead of asking to implement the new method in it ? Removing the interface would remove a supported extension point.
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 don't remember why I did that initially. I guess it was due to an earlier implementation.#30388 will revert this part.
…buh)This PR was merged into the 4.3-dev branch.Discussion----------[Security] undeprecate the RoleHierarchyInterface| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#22048 (comment)| License | MIT| Doc PR |Instead of deprecating the interface it is sufficient to deprecate itsgetReachableRoles() method and add a new getReachableRoleNames() methodin Symfony 5.Commits-------2d3f2b7a74 undeprecate the RoleHierarchyInterface
…buh)This PR was merged into the 4.3-dev branch.Discussion----------[Security] undeprecate the RoleHierarchyInterface| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#22048 (comment)| License | MIT| Doc PR |Instead of deprecating the interface it is sufficient to deprecate itsgetReachableRoles() method and add a new getReachableRoleNames() methodin Symfony 5.Commits-------2d3f2b7 undeprecate the RoleHierarchyInterface
| * @param(Role|string)[] $roles An array of roles | ||
| * @param UserInterface $user The user! | ||
| * @param string $providerKey The provider (firewall) key | ||
| * @param string[] $roles An array of roles |
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.
Although theRole object is deprecated the$roles argument can still acceptRoles object..
this change breaks my phpstan when I have something like this:
$newToken = new PostAuthenticationGuardToken($user, 'secured_area', $user->getRoles());
would it be acceptable to revert this docblock change?
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.
We normally do not document the deprecated arguments anymore. Same as if we deprecate an argument and use func_get_arg for BC. Otherwise people could think the deprecated way is still the right way and write code accordingly. So IMO we should not change that. Either fix the deprecation or ignore the phpstan error.
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.
We normally do not document the deprecated arguments anymore.
I guess the same does not applyhere because it is a return argument?
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.
It should be changed there as well.
Uh oh!
There was an error while loading.Please reload this page.
In#20801, we deprecated the
RoleInterface. The next logical step would be to also deprecate theRoleclass. However, we currently have theSwitchUserRoleclass (a sub-class ofRole) that acts as an indicator to check whether or not the authenticated user switched to another user.This PR proposes an alternative solution to the usage of the special
SwitchUserRoleclass by storing the original token inside theUsernamePasswordToken. This PR is not complete, but rather acts as a proof of concept of how we could get rid of theRoleand theSwitchUserRoleclasses.Please share your opinions whether you think this is a valid approach and I will be happy to finalise the PR.