Skip to content

Commit 623d964

Browse files
fabpotfabpot
committed
feature #22048 [Security] deprecate the Role and SwitchUserRole classes (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 ------- d7aaa615b9 deprecate the Role and SwitchUserRole classes
2 parents 7924871 + 677fcfd commit 623d964

File tree

4 files changed

+67
-38
lines changed

4 files changed

+67
-38
lines changed

‎DataCollector/SecurityDataCollector.php‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,12 @@
1818
use Symfony\Component\HttpKernel\DataCollector\DataCollector;
1919
use Symfony\Component\HttpKernel\DataCollector\LateDataCollectorInterface;
2020
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
21+
use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken;
2122
use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface;
2223
use Symfony\Component\Security\Core\Authorization\TraceableAccessDecisionManager;
2324
use Symfony\Component\Security\Core\Authorization\Voter\TraceableVoter;
2425
use Symfony\Component\Security\Core\Role\Role;
26+
use Symfony\Component\Security\Core\Role\RoleHierarchy;
2527
use Symfony\Component\Security\Core\Role\RoleHierarchyInterface;
2628
use Symfony\Component\Security\Core\Role\SwitchUserRole;
2729
use Symfony\Component\Security\Http\Firewall\SwitchUserListener;
@@ -91,18 +93,32 @@ public function collect(Request $request, Response $response, \Exception $except
9193
];
9294
} else {
9395
$inheritedRoles = [];
94-
$assignedRoles = $token->getRoles();
96+
97+
if (method_exists($token, 'getRoleNames')) {
98+
$assignedRoles = $token->getRoleNames();
99+
} else {
100+
$assignedRoles = array_map(function (Role $role) { return $role->getRole(); }, $token->getRoles(false));
101+
}
95102

96103
$impersonatorUser = null;
97-
foreach ($assignedRoles as $role) {
98-
if ($role instanceof SwitchUserRole) {
99-
$impersonatorUser = $role->getSource()->getUsername();
100-
break;
104+
if ($token instanceof SwitchUserToken) {
105+
$impersonatorUser = $token->getOriginalToken()->getUsername();
106+
} else {
107+
foreach ($token->getRoles(false) as $role) {
108+
if ($role instanceof SwitchUserRole) {
109+
$impersonatorUser = $role->getSource()->getUsername();
110+
break;
111+
}
101112
}
102113
}
103114

104115
if (null !== $this->roleHierarchy) {
105-
$allRoles = $this->roleHierarchy->getReachableRoles($assignedRoles);
116+
if ($this->roleHierarchy instanceof RoleHierarchy) {
117+
$allRoles = $this->roleHierarchy->getReachableRoleNames($assignedRoles);
118+
} else {
119+
$allRoles = array_map(function (Role $role) { return (string) $role; }, $this->roleHierarchy->getReachableRoles($token->getRoles(false)));
120+
}
121+
106122
foreach ($allRoles as $role) {
107123
if (!\in_array($role, $assignedRoles, true)) {
108124
$inheritedRoles[] = $role;
@@ -129,8 +145,8 @@ public function collect(Request $request, Response $response, \Exception $except
129145
'token_class' => $this->hasVarDumper ? new ClassStub(\get_class($token)) : \get_class($token),
130146
'logout_url' => $logoutUrl,
131147
'user' => $token->getUsername(),
132-
'roles' => array_map(function (Role $role) { return $role->getRole(); }, $assignedRoles),
133-
'inherited_roles' => array_unique(array_map(function (Role $role) { return $role->getRole(); }, $inheritedRoles)),
148+
'roles' => $assignedRoles,
149+
'inherited_roles' => array_unique($inheritedRoles),
134150
'supports_role_hierarchy' => null !== $this->roleHierarchy,
135151
];
136152
}

‎Resources/config/security.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@
9898
<argument>%security.role_hierarchy.roles%</argument>
9999
</service>
100100
<service id="Symfony\Component\Security\Core\Role\RoleHierarchyInterface" alias="security.role_hierarchy" />
101+
<service id="Symfony\Component\Security\Core\Role\RoleHierarchy" alias="security.role_hierarchy" />
101102

102103

103104
<!-- Security Voters -->

‎Tests/DataCollector/SecurityDataCollectorTest.php‎

Lines changed: 41 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,12 @@
1818
use Symfony\Bundle\SecurityBundle\Security\FirewallMap;
1919
use Symfony\Component\EventDispatcher\EventDispatcher;
2020
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
21+
use Symfony\Component\HttpFoundation\Request;
22+
use Symfony\Component\HttpFoundation\Response;
2123
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
2224
use Symfony\Component\HttpKernel\HttpKernelInterface;
2325
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage;
26+
use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken;
2427
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
2528
use Symfony\Component\Security\Core\Authorization\AccessDecisionManager;
2629
use Symfony\Component\Security\Core\Authorization\TraceableAccessDecisionManager;
@@ -38,7 +41,7 @@ class SecurityDataCollectorTest extends TestCase
3841
public function testCollectWhenSecurityIsDisabled()
3942
{
4043
$collector = new SecurityDataCollector();
41-
$collector->collect($this->getRequest(), $this->getResponse());
44+
$collector->collect(new Request(), new Response());
4245

4346
$this->assertSame('security', $collector->getName());
4447
$this->assertFalse($collector->isEnabled());
@@ -58,7 +61,7 @@ public function testCollectWhenAuthenticationTokenIsNull()
5861
{
5962
$tokenStorage = new TokenStorage();
6063
$collector = new SecurityDataCollector($tokenStorage, $this->getRoleHierarchy());
61-
$collector->collect($this->getRequest(), $this->getResponse());
64+
$collector->collect(new Request(), new Response());
6265

6366
$this->assertTrue($collector->isEnabled());
6467
$this->assertFalse($collector->isAuthenticated());
@@ -80,7 +83,7 @@ public function testCollectAuthenticationTokenAndRoles(array $roles, array $norm
8083
$tokenStorage->setToken(new UsernamePasswordToken('hhamon', 'P4$$w0rD', 'provider', $roles));
8184

8285
$collector = new SecurityDataCollector($tokenStorage, $this->getRoleHierarchy());
83-
$collector->collect($this->getRequest(), $this->getResponse());
86+
$collector->collect(new Request(), new Response());
8487
$collector->lateCollect();
8588

8689
$this->assertTrue($collector->isEnabled());
@@ -95,6 +98,9 @@ public function testCollectAuthenticationTokenAndRoles(array $roles, array $norm
9598
$this->assertSame('hhamon', $collector->getUser());
9699
}
97100

101+
/**
102+
* @group legacy
103+
*/
98104
public function testCollectImpersonatedToken()
99105
{
100106
$adminToken = new UsernamePasswordToken('yceruto', 'P4$$w0rD', 'provider', ['ROLE_ADMIN']);
@@ -108,7 +114,7 @@ public function testCollectImpersonatedToken()
108114
$tokenStorage->setToken(new UsernamePasswordToken('hhamon', 'P4$$w0rD', 'provider', $userRoles));
109115

110116
$collector = new SecurityDataCollector($tokenStorage, $this->getRoleHierarchy());
111-
$collector->collect($this->getRequest(), $this->getResponse());
117+
$collector->collect(new Request(), new Response());
112118
$collector->lateCollect();
113119

114120
$this->assertTrue($collector->isEnabled());
@@ -122,10 +128,32 @@ public function testCollectImpersonatedToken()
122128
$this->assertSame('hhamon', $collector->getUser());
123129
}
124130

131+
public function testCollectSwitchUserToken()
132+
{
133+
$adminToken = new UsernamePasswordToken('yceruto', 'P4$$w0rD', 'provider', ['ROLE_ADMIN']);
134+
135+
$tokenStorage = new TokenStorage();
136+
$tokenStorage->setToken(new SwitchUserToken('hhamon', 'P4$$w0rD', 'provider', ['ROLE_USER', 'ROLE_PREVIOUS_ADMIN'], $adminToken));
137+
138+
$collector = new SecurityDataCollector($tokenStorage, $this->getRoleHierarchy());
139+
$collector->collect(new Request(), new Response());
140+
$collector->lateCollect();
141+
142+
$this->assertTrue($collector->isEnabled());
143+
$this->assertTrue($collector->isAuthenticated());
144+
$this->assertTrue($collector->isImpersonated());
145+
$this->assertSame('yceruto', $collector->getImpersonatorUser());
146+
$this->assertSame(SwitchUserToken::class, $collector->getTokenClass()->getValue());
147+
$this->assertTrue($collector->supportsRoleHierarchy());
148+
$this->assertSame(['ROLE_USER', 'ROLE_PREVIOUS_ADMIN'], $collector->getRoles()->getValue(true));
149+
$this->assertSame([], $collector->getInheritedRoles()->getValue(true));
150+
$this->assertSame('hhamon', $collector->getUser());
151+
}
152+
125153
public function testGetFirewall()
126154
{
127155
$firewallConfig = new FirewallConfig('dummy', 'security.request_matcher.dummy', 'security.user_checker.dummy');
128-
$request = $this->getRequest();
156+
$request = new Request();
129157

130158
$firewallMap = $this
131159
->getMockBuilder(FirewallMap::class)
@@ -138,7 +166,7 @@ public function testGetFirewall()
138166
->willReturn($firewallConfig);
139167

140168
$collector = new SecurityDataCollector(null, null, null, null, $firewallMap, new TraceableFirewallListener($firewallMap, new EventDispatcher(), new LogoutUrlGenerator()));
141-
$collector->collect($request, $this->getResponse());
169+
$collector->collect($request, new Response());
142170
$collector->lateCollect();
143171
$collected = $collector->getFirewall();
144172

@@ -158,8 +186,8 @@ public function testGetFirewall()
158186

159187
public function testGetFirewallReturnsNull()
160188
{
161-
$request = $this->getRequest();
162-
$response = $this->getResponse();
189+
$request = new Request();
190+
$response = new Response();
163191

164192
// Don't inject any firewall map
165193
$collector = new SecurityDataCollector();
@@ -192,9 +220,9 @@ public function testGetFirewallReturnsNull()
192220
*/
193221
public function testGetListeners()
194222
{
195-
$request = $this->getRequest();
223+
$request = new Request();
196224
$event = new GetResponseEvent($this->getMockBuilder(HttpKernelInterface::class)->getMock(), $request, HttpKernelInterface::MASTER_REQUEST);
197-
$event->setResponse($response = $this->getResponse());
225+
$event->setResponse($response = new Response());
198226
$listener = $this->getMockBuilder(ListenerInterface::class)->getMock();
199227
$listener
200228
->expects($this->once())
@@ -345,7 +373,7 @@ public function testCollectDecisionLog(string $strategy, array $decisionLog, arr
345373
->willReturn($decisionLog);
346374

347375
$dataCollector = new SecurityDataCollector(null, null, null, $accessDecisionManager);
348-
$dataCollector->collect($this->getRequest(), $this->getResponse());
376+
$dataCollector->collect(new Request(), new Response());
349377

350378
$this->assertEquals($dataCollector->getAccessDecisionLog(), $expectedDecisionLog, 'Wrong value returned by getAccessDecisionLog');
351379

@@ -367,7 +395,7 @@ public function provideRoles()
367395
[],
368396
],
369397
[
370-
[new Role('ROLE_USER')],
398+
[new Role('ROLE_USER', false)],
371399
['ROLE_USER'],
372400
[],
373401
],
@@ -378,7 +406,7 @@ public function provideRoles()
378406
['ROLE_USER', 'ROLE_ALLOWED_TO_SWITCH'],
379407
],
380408
[
381-
[new Role('ROLE_ADMIN')],
409+
[new Role('ROLE_ADMIN', false)],
382410
['ROLE_ADMIN'],
383411
['ROLE_USER', 'ROLE_ALLOWED_TO_SWITCH'],
384412
],
@@ -397,20 +425,4 @@ private function getRoleHierarchy()
397425
'ROLE_OPERATOR' => ['ROLE_USER'],
398426
]);
399427
}
400-
401-
private function getRequest()
402-
{
403-
return $this
404-
->getMockBuilder('Symfony\Component\HttpFoundation\Request')
405-
->disableOriginalConstructor()
406-
->getMock();
407-
}
408-
409-
private function getResponse()
410-
{
411-
return $this
412-
->getMockBuilder('Symfony\Component\HttpFoundation\Response')
413-
->disableOriginalConstructor()
414-
->getMock();
415-
}
416428
}

‎composer.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
"symfony/config": "^4.2",
2222
"symfony/dependency-injection": "^4.2",
2323
"symfony/http-kernel": "^4.1",
24-
"symfony/security-core": "~4.2",
24+
"symfony/security-core": "~4.3",
2525
"symfony/security-csrf": "~4.2",
2626
"symfony/security-guard": "~4.2",
2727
"symfony/security-http": "~4.2"

0 commit comments

Comments
 (0)