[Security] deprecate the Role and SwitchUserRole classes #22048

xabbuh · 2017-03-17T21:18:26Z

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#11047

In #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.

This 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

xabbuh · 2017-07-12T17:09:03Z

@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

nicolas-grekas · 2017-08-31T09:29:03Z

@fabpot @stof you might be the best to help here?

chalasr · 2017-08-31T09:34:05Z

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 · 2017-08-31T09:40:44Z

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

@chalasr I guess the SwitchUserToken (or maybe better SwitchedUserToken) suggested by @HeahDude could be a good solution.

chalasr · 2017-08-31T09:45:34Z

Agreed, that would remove the need for isUserSwitched.

nicolas-grekas · 2017-09-26T12:48:24Z

@xabbuh will you have tome to finish this PR for 3.4?

xabbuh · 2017-09-26T13:47:58Z

Yes, I am working on it.

nicolas-grekas · 2017-10-08T15:52:19Z

should we move to 4.1?

xabbuh · 2017-10-08T16:07:13Z

I think so.

xabbuh · 2019-02-22T23:57:31Z

tests pass now

Status: Needs Review

javiereguiluz

Nice!

src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php

fabpot · 2019-02-25T16:04:40Z

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#11047 In #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 classes see symfony/symfony#22048 Commits ------- 0fbef77 document the deprecation of the role classes

stof · 2019-02-26T11:41:12Z

src/Symfony/Component/Security/Core/Role/RoleHierarchyInterface.php

@@ -15,6 +15,8 @@
 * RoleHierarchyInterface is the interface for a role hierarchy.
 *
 * @author Fabien Potencier <fabien@symfony.com>
+ *
+ * @deprecated since Symfony 4.3, to be removed in 5.0.


Why deprecating this interface instead of asking to implement the new method in it ? Removing the interface would remove a supported extension point.

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 its getReachableRoles() method and add a new getReachableRoleNames() method in 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 its getReachableRoles() method and add a new getReachableRoleNames() method in Symfony 5. Commits ------- 2d3f2b7 undeprecate the RoleHierarchyInterface

gmponos · 2019-06-13T20:54:52Z

src/Symfony/Component/Security/Guard/Token/PostAuthenticationGuardToken.php

-     * @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


Although the Role object is deprecated the $roles argument can still accept Roles 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?

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.

We normally do not document the deprecated arguments anymore.

I guess the same does not apply here because it is a return argument?

It should be changed there as well.

carsonbot added Status: Needs Review Security Deprecation labels Mar 17, 2017

xabbuh force-pushed the deprecate-role-class branch from 729a3aa to 71b26ae Compare March 17, 2017 22:04

xabbuh mentioned this pull request Mar 17, 2017

[Security] simplify the SwitchUserListenerTest #22049

Merged

xabbuh force-pushed the deprecate-role-class branch 2 times, most recently from a8df9f2 to 1aec9b7 Compare March 18, 2017 09:13

nicolas-grekas added this to the 3.x milestone Mar 24, 2017

HeahDude reviewed Mar 26, 2017

View reviewed changes

src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php Outdated Show resolved Hide resolved

xabbuh changed the base branch from master to 3.4 May 18, 2017 07:16

xabbuh mentioned this pull request Jun 3, 2017

[Security] Make Role::$role protected #23025

Closed

xabbuh mentioned this pull request Jun 12, 2017

Allow getReachableRoles() to accept string array #23153

Closed

xabbuh mentioned this pull request Aug 31, 2017

[Security] Why RoleInterface will be removed on 4.0 release? #24043

Closed

nicolas-grekas reviewed Aug 31, 2017

View reviewed changes

src/Symfony/Component/Security/Core/Authentication/Token/UsernamePasswordToken.php Outdated Show resolved Hide resolved

nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017

xabbuh force-pushed the deprecate-role-class branch from 1aec9b7 to daf09cb Compare November 15, 2017 19:46

xabbuh changed the base branch from 3.4 to master November 15, 2017 19:48

xabbuh force-pushed the deprecate-role-class branch from daf09cb to 7194244 Compare November 15, 2017 19:51

xabbuh force-pushed the deprecate-role-class branch from 7194244 to 2054024 Compare November 23, 2017 09:54

xabbuh force-pushed the deprecate-role-class branch 3 times, most recently from b871a65 to b2cfbb3 Compare February 22, 2019 23:11

deprecate the Role and SwitchUserRole classes

d7aaa61

xabbuh force-pushed the deprecate-role-class branch from b2cfbb3 to d7aaa61 Compare February 22, 2019 23:57

carsonbot added Status: Needs Review and removed Status: Needs Work labels Feb 22, 2019

fabpot approved these changes Feb 23, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 23, 2019

javiereguiluz approved these changes Feb 23, 2019

View reviewed changes

src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php Show resolved Hide resolved

chalasr approved these changes Feb 25, 2019

View reviewed changes

fabpot merged commit d7aaa61 into symfony:master Feb 25, 2019

xabbuh deleted the deprecate-role-class branch February 25, 2019 16:17

stof reviewed Feb 26, 2019

View reviewed changes

xabbuh mentioned this pull request Feb 26, 2019

[Security] undeprecate the RoleHierarchyInterface #30388

Merged

HeahDude mentioned this pull request Mar 11, 2019

[Security][SecurityBundle] Add is_granted feature for impersonator on AuthorizationChecker #28794

Closed

2 tasks

nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

fabpot mentioned this pull request May 9, 2019

Release v4.3.0-BETA1 #31435

Merged

gmponos reviewed Jun 13, 2019

View reviewed changes

estebanolm mentioned this pull request Feb 14, 2020

ManyToMany IndexBy not working on multiple fields after upgrade to symfony 4.3 doctrine/orm#8018

Open

kristandre mentioned this pull request Oct 27, 2020

Add "louout_on_user_change: true" to firewall options vektorprogrammet/vektorprogrammet#1446

Merged

ajgarlag mentioned this pull request Dec 16, 2021

Upgrading from Symfony 4.x to Symfony 5.x logouts all users #44676

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] deprecate the Role and SwitchUserRole classes #22048

[Security] deprecate the Role and SwitchUserRole classes #22048

xabbuh commented Mar 17, 2017 •

edited

Loading

xabbuh commented Jul 12, 2017

nicolas-grekas commented Aug 31, 2017

chalasr commented Aug 31, 2017

xabbuh commented Aug 31, 2017

chalasr commented Aug 31, 2017

nicolas-grekas commented Sep 26, 2017

xabbuh commented Sep 26, 2017

nicolas-grekas commented Oct 8, 2017

xabbuh commented Oct 8, 2017

xabbuh commented Feb 22, 2019

javiereguiluz left a comment

fabpot commented Feb 25, 2019

stof Feb 26, 2019

xabbuh Feb 26, 2019

gmponos Jun 13, 2019 •

edited

Loading

Tobion Jun 14, 2019

gmponos Jun 14, 2019

Tobion Jun 14, 2019

[Security] deprecate the Role and SwitchUserRole classes #22048

[Security] deprecate the Role and SwitchUserRole classes #22048

Conversation

xabbuh commented Mar 17, 2017 • edited Loading

xabbuh commented Jul 12, 2017

nicolas-grekas commented Aug 31, 2017

chalasr commented Aug 31, 2017

xabbuh commented Aug 31, 2017

chalasr commented Aug 31, 2017

nicolas-grekas commented Sep 26, 2017

xabbuh commented Sep 26, 2017

nicolas-grekas commented Oct 8, 2017

xabbuh commented Oct 8, 2017

xabbuh commented Feb 22, 2019

javiereguiluz left a comment

Choose a reason for hiding this comment

fabpot commented Feb 25, 2019

stof Feb 26, 2019

Choose a reason for hiding this comment

xabbuh Feb 26, 2019

Choose a reason for hiding this comment

gmponos Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Tobion Jun 14, 2019

Choose a reason for hiding this comment

gmponos Jun 14, 2019

Choose a reason for hiding this comment

Tobion Jun 14, 2019

Choose a reason for hiding this comment

xabbuh commented Mar 17, 2017 •

edited

Loading

gmponos Jun 13, 2019 •

edited

Loading