Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 3c7172d

Browse filesBrowse files
fabpotfabpot
committed
feature #33584 [Security] Deprecate isGranted()/decide() on more than one attribute (wouterj)
This PR was squashed before being merged into the 4.4 branch (closes #33584). Discussion ---------- [Security] Deprecate isGranted()/decide() on more than one attribute | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | Deprecations? | yes | Tickets | - | License | MIT | Doc PR | tbd While I expect it not be used much, it is currently possible to call `isGranted()` on more than one attribute: ```php if ($this->authorizationChecker->isGranted(['ROLE_USER', 'ROLE_ADMIN'])) { // ... } ``` Supporting this includes a couple of problems/questions: - It is not clear whether this is `OR` or `AND`; - In fact, this is left over to the voter to decide upon. So it can vary for each voter and writers of new voters need to consider this (otherwise, you get issues like leaseweb/LswSecureControllerBundle#4 ); - It promotes to vote over roles instead of actions. I think we can do better. In the past, we've created all tooling for this to be self-explaining and easier: ```php // ExpressionLanguage component (also includes other functions, like `is_granted('EDIT')`) if ($this->authorizationChecker->isGranted("has_role('ROLE_USER') or has_role('ROLE_ADMIN')")) { // ... } // calling it multiple times in PHP (may reduce performance) if ($this->authorizationChecker->isGranted('ROLE_USER') || $this->authorizationChecker->isGranted('ROLE_ADMIN') ) { // ... } // or by using Role Hierarchy, if a user really wants to vote on roles ``` This PR deprecates passing more than one attribute to `isGranted()` and `decide()` to remove this confusing bit in Security usage. Backwards compatiblity help --- I need some help in how to approach changing the `VoterInterface::vote(TokenInterface $token, $subject, array $attributes)` method in a backwards compatible way. Removing `array` breaks all Voters, so does changing it to `string` and removed the parameter all together. Commits ------- c64b0be [Security] Deprecate isGranted()/decide() on more than one attribute
2 parents e84bd65 + c64b0be commit 3c7172d
Copy full SHA for 3c7172d

File tree

Expand file treeCollapse file tree

5 files changed

+26
-1
lines changed
Filter options
Expand file treeCollapse file tree

5 files changed

+26
-1
lines changed

‎UPGRADE-4.4.md

Copy file name to clipboardExpand all lines: UPGRADE-4.4.md
+18Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,24 @@ Security
197197
* The `LdapUserProvider` class has been deprecated, use `Symfony\Component\Ldap\Security\LdapUserProvider` instead.
198198
* Implementations of `PasswordEncoderInterface` and `UserPasswordEncoderInterface` should add a new `needsRehash()` method
199199
* Deprecated returning a non-boolean value when implementing `Guard\AuthenticatorInterface::checkCredentials()`. Please explicitly return `false` to indicate invalid credentials.
200+
* Deprecated passing more than one attribute to `AccessDecisionManager::decide()` and `AuthorizationChecker::isGranted()` (and indirectly the `is_granted()` Twig and ExpressionLanguage function)
201+
202+
**Before**
203+
```php
204+
if ($this->authorizationChecker->isGranted(['ROLE_USER', 'ROLE_ADMIN'])) {
205+
// ...
206+
}
207+
```
208+
209+
**After**
210+
```php
211+
if ($this->authorizationChecker->isGranted(new Expression("has_role('ROLE_USER') or has_role('ROLE_ADMIN')"))) {}
212+
213+
// or:
214+
if ($this->authorizationChecker->isGranted('ROLE_USER')
215+
|| $this->authorizationChecker->isGranted('ROLE_ADMIN')
216+
) {}
217+
```
200218

201219
Stopwatch
202220
---------

‎src/Symfony/Component/Security/CHANGELOG.md

Copy file name to clipboardExpand all lines: src/Symfony/Component/Security/CHANGELOG.md
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ CHANGELOG
1212
for "guard" authenticators that deal with user passwords
1313
* Marked all dispatched event classes as `@final`
1414
* Deprecated returning a non-boolean value when implementing `Guard\AuthenticatorInterface::checkCredentials()`.
15+
* Deprecated passing more than one attribute to `AccessDecisionManager::decide()` and `AuthorizationChecker::isGranted()`
1516

1617
4.3.0
1718
-----

‎src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,10 @@ public function __construct(iterable $voters = [], string $strategy = self::STRA
5757
*/
5858
public function decide(TokenInterface $token, array $attributes, $object = null)
5959
{
60+
if (\count($attributes) > 1) {
61+
@trigger_error('Passing more than one Security attribute to '.__METHOD__.' is deprecated since Symfony 4.4. Use multiple decide() calls or the expression language (e.g. "has_role(...) or has_role(...)") instead.', \E_USER_DEPRECATED);
62+
}
63+
6064
return $this->{$this->strategy}($token, $attributes, $object);
6165
}
6266

‎src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ final public function isGranted($attributes, $subject = null): bool
5555

5656
if (!\is_array($attributes)) {
5757
$attributes = [$attributes];
58+
} else {
59+
@trigger_error('Passing an array of Security attributes to '.__METHOD__.' is deprecated since Symfony 4.4. Use multiple isGranted() calls or the expression language (e.g. "has_role(...) or has_role(...)") instead.', \E_USER_DEPRECATED);
5860
}
5961

6062
return $this->accessDecisionManager->decide($token, $attributes, $subject);

‎src/Symfony/Component/Security/Core/Tests/Authorization/AccessDecisionManagerTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/Security/Core/Tests/Authorization/AccessDecisionManagerTest.php
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public function testStrategies($strategy, $voters, $allowIfAllAbstainDecisions,
3737
/**
3838
* @dataProvider getStrategiesWith2RolesTests
3939
*/
40-
public function testStrategiesWith2Roles($token, $strategy, $voter, $expected)
40+
public function testLegacyStrategiesWith2Roles($token, $strategy, $voter, $expected)
4141
{
4242
$manager = new AccessDecisionManager([$voter], $strategy);
4343

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.