Skip to content

Custom AccessDecisionManager not getting list of voters as first argument in __construct #42095

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sarbanha opened this issue Jul 14, 2021 · 14 comments · Fixed by #42177
Closed

Comments

@sarbanha
Copy link
Contributor

sarbanha commented Jul 14, 2021

With reference to the issue #41123

I encountered this issue with different symptoms!

Although I knew the service is automatically registered but I did this in service.yml:

    App\Security\SpecialAccessDecisionManager:
        class: App\Security\SpecialAccessDecisionManager  
        autowire: false
        autoconfigure: false
        public: false

then I added decision manager to security.yml:

security:
    access_decision_manager:
        service: App\Security\SpecialAccessDecisionManager

As you can see below, arguments haven't been passed to the decision manager:

[root@dev7 rouzonline.test]# bin/console debug:container debug.security.access.decision_manager.inner --show-arguments                            

 // This service is a private alias for the service                                                                     
 // App\Security\SpecialAccessDecisionManager                                                                           

Information for Service "App\Security\SpecialAccessDecisionManager"
===================================================================

 Description of SpecialAccessDecisionManager

 ---------------- ------------------------------------------- 
  Option           Value                                      
 ---------------- ------------------------------------------- 
  Service ID       App\Security\SpecialAccessDecisionManager  
  Class            App\Security\SpecialAccessDecisionManager  
  Tags             -                                          
  Public           no                                         
  Synthetic        no                                         
  Lazy             no                                         
  Shared           yes                                        
  Abstract         no                                         
  Autowired        no                                         
  Autoconfigured   no                                         
 ---------------- ------------------------------------------- 


 ! [NOTE] The "debug.security.access.decision_manager.inner" service or alias has been removed or inlined when the      
 !        container was compiled.                                                                                       

[root@dev7 rouzonline.test]#

When I use default AccessDecisionManager voters are passed. I need to implement special authorization so I need to create my own decision manager. By the way, my decision manager implements AccessDecisionManagerInterface

I also checked \SecurityBundle\DependencyInjection\Compiler\AddSecurityVotersPass->process() , it is not being called when my decision manager is set.

Any idea?

UPDATE
For testing, I temporarily renamed default AccessDecisionManager.php and placed my decision manager, it surprisingly worked. But it is not the solution. Still looking for the correct solution.

@xabbuh
Copy link
Member

xabbuh commented Jul 14, 2021

Can you create a small example application that allows to reproduce your issue?

@sarbanha
Copy link
Contributor Author

I simply copied the default AccessDecisionManager.php into App\Security directory (namespace) then renamed it and tried to use it.

@derrabus
Copy link
Member

What arguments would you expect to be passed to your custom ADM?

@sarbanha
Copy link
Contributor Author

Here you can find the regular arguments that standard ADM receives upon object construction:

[root@dev7 rouzonline.test]# bin/console debug:container debug.security.access.decision_manager.inner --show-arguments

Information for Service "debug.security.access.decision_manager.inner"
======================================================================

 Description of SpecialAccessDecisionManager

 ---------------- -----------------------------------------------------------------------
  Option           Value
 ---------------- -----------------------------------------------------------------------
  Service ID       debug.security.access.decision_manager.inner
  Class            Symfony\Component\Security\Core\Authorization\AccessDecisionManager
  Tags             -
  Public           no
  Synthetic        no
  Lazy             no
  Shared           yes
  Abstract         no
  Autowired        no
  Autoconfigured   no
  Arguments        Iterator (5 element(s))
                   - Service(debug.security.voter.security.access.authenticated_voter)
                   - Service(debug.security.voter.security.access.simple_role_voter)
                   - Service(debug.security.voter.security.access.expression_voter)
                   - Service(debug.security.voter.App\Security\AuthorizationRoleVoter)
                   - Service(debug.security.voter.App\Security\AuthorizationStatusVoter)
                   affirmative

                   1
 ---------------- -----------------------------------------------------------------------


 ! [NOTE] The "debug.security.access.decision_manager.inner" service or alias has been removed or inlined when the
 !        container was compiled.

[root@dev7 rouzonline.test]#

Also, you can see that I have two custom voters ( AuthorizationRoleVoter , AuthorizationStatusVoter ) injected into security system.

@ghost
Copy link

ghost commented Jul 16, 2021

By default Symfony sets it's AccesDecisionManager class as a service definition inside the container with an id security.access.decision_manager.

However when you set

access_decision_manager:
        service: <service_id>

then SecurityExtension removes previously set security.access.decision_manager definition from the container and uses it as an an alias to your <service_id>.
AddSecurityVotersPass will return at the very first line as it looks for a definition in the container and security.access.decision_manager is the alias at this moment.

This worked for the guy in the referenced issue becaused he registered his manager with the same id (i.e. security.access.decision_manager) in his services.yaml. (but he didn't get the rest of the constructor arguments, only voters)

As I said there: "Right now, when you have custom access decision manager you will have to manage all the dependencies by yourself."

@sarbanha
Copy link
Contributor Author

Thank you guys for your replies and helps, I figured it out.

Workaround

When Symfony compiles the code, it (somehow) has its own service definitions, so it doesn't get confused. I traced the code into the compiled cache and I found that it can't determine the service definitions of my ADM. So I changed my service.yml and defined arguments as follows:

    security.access.decision_manager:
        class: App\Security\SpecialAccessDecisionManager  
        arguments:
            - []
            - 'affirmative'
            - false
            - true

Inside vendor/symfony/security-bundle/DependencyInjection/Compiler/AddSecurityVotersPass.php I found that there is argument replacement which enumerates service arguments by number, that is why I added unnamed service arguments.

        $container->getDefinition('security.access.decision_manager')
            ->replaceArgument(0, new IteratorArgument($voterServices));
    }

The traces showed that Symfony collects all voters correctly up to this point, so it is not needed to manage dependencies manually.

By using this service.yml, I didn't need to define the access_decision_manager in security.yml.

After finding the problem, I tried to use access_decision_manager but it caused the same error, means it didn't receive list of voters. Then I decided to stop working on the problem as it is working for me now.

Conclusion

I know AccessDecisionManager is somehow a deep Symfony concept which is rarely used for common projects, but:

  1. Documentation doesn't adequately cover this part
  2. When a developer implements an interface and injects it to Symfony, it is supposed either to be treated exactly same as the native code or provide enough descriptive logs or details to show noncompliance
  3. The method I used to fix the problem is not standard and might need modification in the future
  4. Symfony misbehaves when it is processing access_decision_manager from security.yml

Probably this part of Symfony needs to be reviewed.

@derrabus
Copy link
Member

It is indeed pretty uncommon to provide a custom implementation of the ADM and I must admit I fail to see the use-case of doing so.

The problem here is that if you build your own ADM, it might have an entirely different constructor. Symfony can't possibly know how to wire the list of voters into your custom ADM. In fact, Symfony cannot even make the general assumption that your ADM implementation uses voters at all.

If you build your own ADM, you're basically also responsible for collection the voters that it should use.

May I ask why you want to replace the core ADM?

@sarbanha
Copy link
Contributor Author

Thanks Alex, you are right, but this is the design matter, I think Symfony should assume that developer might need minor expansions otherwise the design should allow more customization interfaces; to allow collection of further or different parameters and inject them into the new expansions. As a developer, I think Symfony doesn't follow its design patterns which we are accustomed to.

However, it seems Symfony follows its pattern half way, it collects voters and prepares parameters as if it were dealing with its core ADM, at some point it doesn't use collected data. I believe it should be like:

  • collecting voters and arguments
  • treat the custom ADM same as native ADM or fail gracefully if it is not complying
    Otherwise
  • let developer implement more interfaces to handle all data collection manually
  • inject them all into system
  • Symfony treat the ADM as it is defined by the new custom codes

The reason why I want to replace core ADM is that I have more than 10 objects that I need two voters to unanimously vote for them, none of predefined strategies worked for me, when I change voters strategy to unanimous, the regular operation of security checking on Request object is affected. I need a mixed unanimous+affirmative decision management, when decision manager is consulted for my objects, I need to change strategy to unanimous otherwise affirmative strategy should be used.

@derrabus
Copy link
Member

Okay, but wouldn't the better fix be to allow custom strategies? 🤔

@sarbanha
Copy link
Contributor Author

sarbanha commented Jul 17, 2021

Yea, that was exactly what I am doing, here is my ADM and the service configuration which worked for me:

src/Security/SpecialAccessDecisionManager.php

<?php

namespace App\Security;

use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
use Symfony\Component\Security\Core\Exception\InvalidArgumentException;

/**
 * Description of SpecialAccessDecisionManager
 *
 * @author Mohammad-Ali
 */
class SpecialAccessDecisionManager implements AccessDecisionManagerInterface {
    
    public const STRATEGY_AFFIRMATIVE = 'unanimous';
    public const STRATEGY_SPECIAL = 'special';
    
    protected $validObjects = [
        'Object1',
        'Object2',
        'Object3',
        .
        .
        .
        .
        'ObjectN',
    ];
    
    private $voters;
    private $strategy;
    private $allowIfAllAbstainDecisions;
    private $allowIfEqualGrantedDeniedDecisions;
    
    /**
     * @param iterable|VoterInterface[] $voters                             An array or an iterator of VoterInterface instances
     * @param string                    $strategy                           The vote strategy
     * @param bool                      $allowIfAllAbstainDecisions         Whether to grant access if all voters abstained or not
     * @param bool                      $allowIfEqualGrantedDeniedDecisions Whether to grant access if result are equals
     *
     * @throws \InvalidArgumentException
     */
    public function __construct(iterable $voters = [], string $strategy = self::STRATEGY_AFFIRMATIVE, bool $allowIfAllAbstainDecisions = false, bool $allowIfEqualGrantedDeniedDecisions = true)
    {
        $strategyMethod = 'decide'.ucfirst($strategy);
        if ('' === $strategy || !\is_callable([$this, $strategyMethod])) {
            throw new \InvalidArgumentException(sprintf('The strategy "%s" is not supported.', $strategy));
        }

        $this->voters = $voters;
        $this->strategy = $strategyMethod;
        $this->allowIfAllAbstainDecisions = $allowIfAllAbstainDecisions;
        $this->allowIfEqualGrantedDeniedDecisions = $allowIfEqualGrantedDeniedDecisions;
    }
    
    /**
     * @param bool $allowMultipleAttributes Whether to allow passing multiple values to the $attributes array
     *
     * {@inheritdoc}
     */
    public function decide(TokenInterface $token, array $attributes, $object = null/*, bool $allowMultipleAttributes = false*/)
    {
        $allowMultipleAttributes = 3 < \func_num_args() && func_get_arg(3);

        // Special case for AccessListener, do not remove the right side of the condition before 6.0
        if (\count($attributes) > 1 && !$allowMultipleAttributes) {
            throw new InvalidArgumentException(sprintf('Passing more than one Security attribute to "%s()" is not supported.', __METHOD__));
        }

        return $this->{$this->strategy}($token, $attributes, $object);
    }
    
    /**
     * Grants access if any voter returns an affirmative response.
     *
     * If all voters abstained from voting, the decision will be based on the
     * allowIfAllAbstainDecisions property value (defaults to false).
     */
    private function decideUnanimous(TokenInterface $token, array $attributes, $object = null): bool
    {
        $grant = 0;
        foreach ($this->voters as $voter) {
            foreach ($attributes as $attribute) {
                $result = $voter->vote($token, $object, [$attribute]);

                if (VoterInterface::ACCESS_DENIED === $result) {
                    return false;
                }

                if (VoterInterface::ACCESS_GRANTED === $result) {
                    ++$grant;
                }
            }
        }

        // no deny votes
        if ($grant > 0) {
            return true;
        }

        return $this->allowIfAllAbstainDecisions;
    }
    
    /**
     * Grants access if only grant (or abstain) votes were received.
     *
     * If all voters abstained from voting, the decision will be based on the
     * allowIfAllAbstainDecisions property value (defaults to false).
     */
    private function decideSpecial(TokenInterface $token, array $attributes, $object = null): bool
    { 
        if(in_array($this->getObj($object), $this->validObjects)){
            return $this->decideUnanimous($token, $attributes, $object);
        }

        $deny = 0;
        foreach ($this->voters as $voter) {
            $result = $voter->vote($token, $object, $attributes);

            if (VoterInterface::ACCESS_GRANTED === $result) {
                return true;
            }

            if (VoterInterface::ACCESS_DENIED === $result) {
                ++$deny;
            }
        }

        if ($deny > 0) {
            return false;
        }

        return $this->allowIfAllAbstainDecisions;
    }
    
    private function getObj($subject) {
        $obj = gettype($subject);
        switch ($obj){
            case 'object':
                $rc = new \ReflectionClass($subject);
                $obj = $rc->getShortName();
                break;
            case 'string':
                $obj = $subject;
        }
        return $obj;
    }
}

service.yml

    security.access.decision_manager:
        class: App\Security\SpecialAccessDecisionManager  
        arguments:
            - []
            - 'special'
            - false
            - true

Note: I didn't add any extra configuration in security.yml

@ghost
Copy link

ghost commented Jul 18, 2021

You can easily inject voters to your custom ADM with !tagged_iterator security.voter argument. The only drawback is that voters will not be decorated with TraceableVoter (as in AddSecurityVotersPass). However this problem can be worked around as well. You can always copy past the code from AddSecurityVotersPass and create you own CompilerPass adjusted to your needs.

You can read about tagged iterators here https://symfony.com/doc/current/service_container/tags.html#reference-tagged-services

@sarbanha
Copy link
Contributor Author

@marbulk , thanks for the tip, I didn't know about !tagged_iterator security.voter , I will read more about it. I however don't think it works for me since it override the other core voters and I would end up with manual handling the entire process from top to bottom :-) while I just need a little expansion.

My point is that Symfony doesn't comply with the design pattern, I have seen other guys that they found their own methods to fix their problems with ADM as I did.

As web applications are getting more complicated, core functionality of frameworks are expected to be more complying. That is why I raised this issue here.

@derrabus
Copy link
Member

I tried to implement custom strategies: #42177

Can you evaluate this PR for me and tell me if that feature would allow you to use the default access decision manager again?

@xabbuh
Copy link
Member

xabbuh commented Oct 12, 2021

@sarbanha Do you have some feedback regarding the PR that @derrabus opened?

@fabpot fabpot closed this as completed Oct 29, 2021
fabpot added a commit that referenced this issue Oct 29, 2021
… dedicated classes (derrabus)

This PR was merged into the 5.4 branch.

Discussion
----------

[Security][SecurityBundle] Implement ADM strategies as dedicated classes

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | Fix #42095
| License       | MIT
| Doc PR        | TODO

We currently allow to replace the access decision manager with a fully custom implementation. However, if the app makes use of that feature just because it needs a custom decision strategy, it also has to take care of wiring the voters into the custom implementation. This is a bit cumbersome.

This PR introduces a new interface `AccessDecisionStrategyInterface` that allows the app to implement a custom strategy that can be plugged into the default access decision manager. All built-in strategies have been migrated to that new interface.

Furthermore, a new option is added to SecurityBundle to leverage this feature:

```YAML
security:
    access_decision_manager:
        strategy_service: app.custom_access_decision_strategy
```

The "old" way of configuring the strategy will continue to work:

```YAML
security:
    access_decision_manager:
        strategy: unanimous
        allow_if_all_abstain: true
        allow_if_equal_granted_denied: false
```

In that case, SecurityBundle will wire the new strategy classes for us, so we don't have to change anything in our app when upgrading to Symfony 5.4.

Additionally, I decided to finalize `AccessDecisionManager` because once the strategies are pluggable, there shouldn't be a use-case left for extending the class that could not be covered with a decorator.

Implementing a custom strategy is straightforward. Your class implenting `AccessDecisionStrategyInterface` will receive a generator of voter results and is expected to return its decision as a boolean. This way, the interaction with the individual voters is abstracted away from the strategy and the strategy can stop the voter iteration whenever the decision is final.

```php
/**
 * Always picks the third voter.
 */
class ThirdVoterStrategy implements AccessDecisionStrategyInterface
{
    public function decide(\Traversable $results): bool
    {
        $votes = 0;
        foreach ($results as $result) {
            if (++$votes === 3) {
                return $result === VoterInterface::ACCESS_GRANTED;
            }
        }

        return false;
    }
}
```

Commits
-------

d25956a [Security] Implement ADM strategies as dedicated classes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants