Skip to content

Commit

Permalink
feature #33584 [Security] Deprecate isGranted()/decide() on more than…
Browse files Browse the repository at this point in the history
… 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
  • Loading branch information
fabpot committed Sep 24, 2019
2 parents e84bd65 + c64b0be commit 3c7172d
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 1 deletion.
18 changes: 18 additions & 0 deletions UPGRADE-4.4.md
Expand Up @@ -197,6 +197,24 @@ Security
* The `LdapUserProvider` class has been deprecated, use `Symfony\Component\Ldap\Security\LdapUserProvider` instead.
* Implementations of `PasswordEncoderInterface` and `UserPasswordEncoderInterface` should add a new `needsRehash()` method
* Deprecated returning a non-boolean value when implementing `Guard\AuthenticatorInterface::checkCredentials()`. Please explicitly return `false` to indicate invalid credentials.
* Deprecated passing more than one attribute to `AccessDecisionManager::decide()` and `AuthorizationChecker::isGranted()` (and indirectly the `is_granted()` Twig and ExpressionLanguage function)

**Before**
```php
if ($this->authorizationChecker->isGranted(['ROLE_USER', 'ROLE_ADMIN'])) {
// ...
}
```

**After**
```php
if ($this->authorizationChecker->isGranted(new Expression("has_role('ROLE_USER') or has_role('ROLE_ADMIN')"))) {}

// or:
if ($this->authorizationChecker->isGranted('ROLE_USER')
|| $this->authorizationChecker->isGranted('ROLE_ADMIN')
) {}
```

Stopwatch
---------
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/Security/CHANGELOG.md
Expand Up @@ -12,6 +12,7 @@ CHANGELOG
for "guard" authenticators that deal with user passwords
* Marked all dispatched event classes as `@final`
* Deprecated returning a non-boolean value when implementing `Guard\AuthenticatorInterface::checkCredentials()`.
* Deprecated passing more than one attribute to `AccessDecisionManager::decide()` and `AuthorizationChecker::isGranted()`

4.3.0
-----
Expand Down
Expand Up @@ -57,6 +57,10 @@ public function __construct(iterable $voters = [], string $strategy = self::STRA
*/
public function decide(TokenInterface $token, array $attributes, $object = null)
{
if (\count($attributes) > 1) {
@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);
}

return $this->{$this->strategy}($token, $attributes, $object);
}

Expand Down
Expand Up @@ -55,6 +55,8 @@ final public function isGranted($attributes, $subject = null): bool

if (!\is_array($attributes)) {
$attributes = [$attributes];
} else {
@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);
}

return $this->accessDecisionManager->decide($token, $attributes, $subject);
Expand Down
Expand Up @@ -37,7 +37,7 @@ public function testStrategies($strategy, $voters, $allowIfAllAbstainDecisions,
/**
* @dataProvider getStrategiesWith2RolesTests
*/
public function testStrategiesWith2Roles($token, $strategy, $voter, $expected)
public function testLegacyStrategiesWith2Roles($token, $strategy, $voter, $expected)
{
$manager = new AccessDecisionManager([$voter], $strategy);

Expand Down

0 comments on commit 3c7172d

Please sign in to comment.