Skip to content

[SecurityBundle] Merging configuration requires access_decision_manager to be in every configuration #25609

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
biozshock opened this issue Dec 27, 2017 · 9 comments

Comments

@biozshock
Copy link
Contributor

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.4.2

In an application i need to change access_decision_manager.strategy to "unanimous".

After the change symfony/security-bundle@06393da#diff-5382c0b1af9e9c09693d49cd3b00aac9R62 i can not have a configuration in config_test.yml that just adds another firewall:

security:
    firewalls:
        default:
            http_basic: ~

Because here https://github.com/symfony/symfony/blob/v3.4.2/src/Symfony/Component/Config/Definition/BaseNode.php#L269 config variable $rightSide for the second file will contain [access_decision_manager][strategy] = 'affirmative' and after the merge it will overwrite any value in security.yml for access_decision_manager.strategy path.

Test case:

namespace Symfony\Bundle\SecurityBundle\Tests\DependencyInjection;

use PHPUnit\Framework\TestCase;
use Symfony\Bundle\SecurityBundle\DependencyInjection\MainConfiguration;
use Symfony\Component\Config\Definition\Processor;

class IssueTest extends TestCase
{
    /**
     * The minimal, required config needed to not have any required validation
     * issues.
     */
    protected static $minimalConfig = array(
        'providers' => array(
            'stub' => array(
                'id' => 'foo',
            ),
        ),
        'firewalls' => array(
            'stub' => array(),
            'logout_on_user_change' => true,
        ),
    );

    public function testConfigMergeWithAccessDecisionManager()
    {
        $config = array(
            'access_decision_manager' => array(
                'strategy' => 'unanimous',
            ),
        );
        $config = array_merge(static::$minimalConfig, $config);

        $config2 = array();

        $processor = new Processor();
        $configuration = new MainConfiguration(array(), array());
        $processedConfig = $processor->processConfiguration($configuration, array($config, $config2));

        $this->assertEquals('unanimous', $processedConfig['access_decision_manager']['strategy']);
    }
}
@biozshock biozshock changed the title [SecurityBundle] Merging configuration requires access_decision_manager to be in every configuration [SecurityBundle] Merging configuration requires access_decision_manager to be in every configuration Dec 27, 2017
@nesk
Copy link
Contributor

nesk commented Apr 2, 2019

This bug is still present in 4.2, is there any way we can help to solve this? I'm not sure where to start to fix this by myself. 😕

@loiclavoie
Copy link

loiclavoie commented May 27, 2019

This issue seem's to be setted in the Normalization of Symfony\Bundle\SecurityBundle\DependencyInjection\MainConfiguration::getConfigTreeBuilder

$rootNode
	->beforeNormalization()
		->ifTrue(function ($v) {
			if (!isset($v['access_decision_manager'])) {
				return true;
			}

			if (!isset($v['access_decision_manager']['strategy']) && !isset($v['access_decision_manager']['service'])) {
				return true;
			}

			return false;
		})
		->then(function ($v) {
			$v['access_decision_manager']['strategy'] = AccessDecisionManager::STRATEGY_AFFIRMATIVE;

			return $v;
		})
	->end()

If reproduce this issue by adding the config/package/security.yaml to the following:

access_decision_manager:
    strategy: unanimous
    allow_if_all_abstain: false

If you set something in the config/package/dev/security.yaml for instance but don't set the access_decision_managers, it will override if with the default value set in the MainConfiguration when processing the second (dev config) and override the main config with the default normalization value.

@VincentLanglet
Copy link
Contributor

This bug is still present in Symfony 4.3

@hamrak
Copy link

hamrak commented Dec 2, 2020

This bug is still present in Symfony 5.2, and when you wan to use own Access decision manager:

security:
    access_decision_manager:
        service: App\Security\OwnAccessDecisionManager
        # not strategy defined

it also returns an error related to this issue:

Invalid configuration for path "security.access_decision_manager": "strategy" and "service" cannot be used together.

ifTrue() returns false, but then() is still called.

@wouterj
Copy link
Member

wouterj commented Jan 23, 2021

Also reported in #39939

status: reviewed

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@VincentLanglet
Copy link
Contributor

I don't think this was solved.

@carsonbot carsonbot removed the Stalled label Jul 24, 2021
@Gielovic
Copy link

I agree. The bug i.m.o. is very relevant and can cause security incidents.

In our case, we created a custom voter, denying access to a certain resource for a user in 'demo-mode'.
With a unanimous decision, the user would get a 403.

however, in the prod environment, we enabled login throttling in config/packages/prod/security.yaml
to find out later, that users were able to access the content that should not be availbable to them.

using debug:config security told us that the decision strategy was now suddenly affermative
So some other voters that were also voting (f.i. by default access rules) where overruling our custom voter's result.

a workaround would be to copy/paste settings to the prod/security.yaml configuration. That did solve the issue in this case, but makes you wondering what other settings are overwritten by defaults.

On version 5.3.9

@romaricdrigon
Copy link
Contributor

Same issue here, in 5.3.10 - and I quite agree with @Gielovic, in my case it was causing tests to behave differently that the app, which could lead to (authorization) issues being pushed to production.

I'm not sure how to easily fix it (in Symfony code), though.

@fabpot fabpot closed this as completed Nov 3, 2021
fabpot added a commit that referenced this issue Nov 3, 2021
…option with merge (biozshock)

This PR was merged into the 4.4 branch.

Discussion
----------

[SecurityBundle] Default access_decision_manager.strategy option with merge

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #25609
| License       | MIT

Security bundle should set default `access_decision_manager.strategy` in extension instead of configuration.
Otherwise merging configurations will override first set option if the next configurations do not have the option set.

Commits
-------

f253995 Default access_decision_manager.strategy option with merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests