[SecurityBundle] allow using custom function inside allow_if expressions #26660

dmaicher · 2018-03-23T16:55:44Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	no
Fixed tickets	#23208
License	MIT
Doc PR	symfony/symfony-docs#9552

This is a follow-up for #26263

As discussed there I propose to allow using custom functions as a new feature only and thus targeting master here.

If we agree to move forward with this there are some todos:

fix tests
add cache warmer for allow_if expressions
update documentation to mention this new feature
update UPGRADE files

ping @nicolas-grekas @stof

nicolas-grekas · 2018-03-23T16:57:13Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/cache.xml

@@ -31,6 +31,10 @@
            <tag name="cache.pool" />
        </service>

+        <service id="cache.security_expression_language" parent="cache.system" public="false">


public="false"

~~no need, that's already the default~~ oups no sorry, having "parent", this is required!

shouldn't this service be defined in SecurityBundle?

Yes makes more sense 👍

And we need to require symfony/framework-bundle then to have cache.system available?

Or we only use the cache if the parent service cache.system is available?

framework-bundle is an implicit dep of any bundle, so no need

and the fact that you reference cache.security_expression_language in SecurityBundle already creates this dependency anyway.

I agree about moving it to SecurityBundle

stof · 2018-03-27T12:47:24Z

...y/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddExpressionLanguageProvidersPass.php

            foreach ($container->findTaggedServiceIds('security.expression_language_provider', true) as $id => $attributes) {
-                $definition->addMethodCall('addExpressionLanguageProvider', array(new Reference($id)));
+                $definition->addMethodCall('registerProvider', array(new Reference($id)));


This change should be applied to the 3.4 branch IMO, so that 3.4 does not trigger a deprecation warning when using the 4.1 component (registering it during the instantiation of the right service is a bugfix anyway, even though the bug was unlikely to happen before this PR because the security.expression_language was not injected anywhere else than in security.access.expression_voter by default)

Will create a separate PR for 3.4 then 👍

See #26802.

@symfony/mergers We should merge #26802 first (and propagate it to master) and then rebase this branch before merging it, so that this diff disappears.

#26802 has now been merged up to master.

I squashed my commits and rebased. Diff is gone now 👍

stof · 2018-03-27T12:48:38Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/cache.xml

@@ -31,6 +31,10 @@
            <tag name="cache.pool" />
        </service>

+        <service id="cache.security_expression_language" parent="cache.system" public="false">


and the fact that you reference cache.security_expression_language in SecurityBundle already creates this dependency anyway.

I agree about moving it to SecurityBundle

stof · 2018-03-27T12:49:45Z

src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml

@@ -195,5 +197,12 @@
            <argument type="service" id="security.token_storage" />
            <argument type="service" id="security.encoder_factory" />
        </service>
+
+        <!-- Cache Warmers -->
+        <service id="security.cache_warmer.expression_cache_warmer" class="Symfony\Bundle\SecurityBundle\CacheWarmer\ExpressionCacheWarmer">


If ExpressionLanguage is not available, this cache warmer should not be registered, because it would still try to instantiate security.expression_language.
We already have code in the DI extension removing the relevant services. This one must be added in the list (and a simpler way for maintenance could be to move all services related to ExpressionLanguage to a separate XML file, which could be loaded conditionally).

Its done now here as well: adab5ce

If the ExpressionLanguage is not available then it fails when using allow_if. So we can safely remove the definition if there are no expressions and it should be fine?

stof · 2018-03-27T12:51:32Z

src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml

+        <!-- Cache Warmers -->
+        <service id="security.cache_warmer.expression_cache_warmer" class="Symfony\Bundle\SecurityBundle\CacheWarmer\ExpressionCacheWarmer">
+            <tag name="kernel.cache_warmer"></tag>
+            <argument type="tagged" tag="security.expression"></argument>


instead of relying on a tag, which creates an external extension point, you could inject this in the DI extension (based on $this->expressions)

Is this what you meant?

adab5ce

dmaicher · 2018-03-27T14:11:36Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

@@ -136,10 +134,6 @@ private function createRoleHierarchy(array $config, ContainerBuilder $container)

    private function createAuthorization($config, ContainerBuilder $container)
    {
-        if (!$config['access_control']) {


@stof is this safe to just remove? It should always be an array?

nicolas-grekas

LGTM, here are some minor comments

nicolas-grekas · 2018-04-04T14:38:08Z

src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml

+
+        <!-- Cache Warmers -->
+        <service id="security.cache_warmer.expression" class="Symfony\Bundle\SecurityBundle\CacheWarmer\ExpressionCacheWarmer">
+            <tag name="kernel.cache_warmer"></tag>


would use an empty <tag /> instead of a closing one (same below for argument)

nicolas-grekas · 2018-04-04T14:38:26Z

src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml

+        <!-- Cache Warmers -->
+        <service id="security.cache_warmer.expression" class="Symfony\Bundle\SecurityBundle\CacheWarmer\ExpressionCacheWarmer">
+            <tag name="kernel.cache_warmer"></tag>
+            <argument /> <!-- collection of expression services -->


type="collection"

nicolas-grekas · 2018-04-04T14:39:30Z

src/Symfony/Component/Security/Core/Authorization/Voter/ExpressionVoter.php

@@ -37,8 +37,13 @@ public function __construct(ExpressionLanguage $expressionLanguage, Authenticati
        $this->roleHierarchy = $roleHierarchy;
    }

+    /**
+     * @deprecated since Symfony 4.1, register the provider directly on the injected ExpressionLanguage instance instead


missing dot at EOL (same below on runtime notice)

nicolas-grekas · 2018-04-04T14:40:33Z

src/Symfony/Bundle/SecurityBundle/CacheWarmer/ExpressionCacheWarmer.php

+class ExpressionCacheWarmer implements CacheWarmerInterface
+{
+    /**
+     * @var iterable|Expression[]


I'd suggest to move this as @param on the constructor and remove any docblock for properties

nicolas-grekas · 2018-04-04T14:40:48Z

src/Symfony/Bundle/SecurityBundle/CacheWarmer/ExpressionCacheWarmer.php

+    private $expressions;
+
+    /**
+     * @va
F438
r ExpressionLanguage


should be removed (no need to put in on the constructor)

dmaicher · 2018-04-04T17:45:30Z

Doc PR: symfony/symfony-docs#9552

@nicolas-grekas @stof final review? 😊

@stof

… directly (dmaicher) This PR was merged into the 3.4 branch. Discussion ---------- [Security] register custom providers on ExpressionLanguage directly | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This is a fix on 3.4 related to #26660. See the comment from @stof here: #26660 (comment) - fixes a bug where custom providers would not be registered when retrieving the `security.expression_language` instance without the `ExpressionVoter` being instantiated. - avoids deprecations on 3.4 when using the 4.1 patch in #26660 Commits ------- 3a55a86 [Security] register custom providers on ExpressionLanguage directly

fabpot · 2018-04-06T06:19:02Z

Thank you @dmaicher.

@nicolas-grekas

…low_if expressions (dmaicher) This PR was merged into the 4.1-dev branch. Discussion ---------- [SecurityBundle] allow using custom function inside allow_if expressions | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | no | Fixed tickets | #23208 | License | MIT | Doc PR | symfony/symfony-docs#9552 This is a follow-up for #26263 As discussed there I propose to allow using custom functions as a new feature only and thus targeting `master` here. If we agree to move forward with this there are some todos: - [x] fix tests - [x] add cache warmer for allow_if expressions - [x] update documentation to mention this new feature - [x] update UPGRADE files ping @nicolas-grekas @stof Commits ------- 41552cd [SecurityBundle] allow using custom function inside allow_if expressions

…, javiereguiluz) This PR was merged into the master branch. Discussion ---------- [Security] add tip about using custom functions Since 4.1 its possible to use custom functions inside security `allow_if` expressions. See symfony/symfony#26660 Commits ------- 44708ad Reword 8a96822 [Security] add tip about using custom functions

carsonbot added Status: Needs Review SecurityBundle Feature Deprecation labels Mar 23, 2018

nicolas-grekas reviewed Mar 23, 2018

View reviewed changes

stof reviewed Mar 27, 2018

View reviewed changes

dmaicher commented Mar 27, 2018

View reviewed changes

nicolas-grekas reviewed Apr 4, 2018

View reviewed changes

This was referenced Apr 4, 2018

[Security] register custom providers on ExpressionLanguage directly #26802

Merged

Custom Expression Language function not working in security.yml allow_if #23208

Closed

dmaicher mentioned this pull request Apr 4, 2018

[Security] add tip about using custom functions symfony/symfony-docs#9552

Merged

[SecurityBundle] allow using custom function inside allow_if expressions

41552cd

dmaicher force-pushed the fix-23208 branch from 5835f37 to 41552cd Compare April 4, 2018 18:31

fabpot approved these changes Apr 6, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 6, 2018

fabpot merged commit 41552cd into symfony:master Apr 6, 2018

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

dmaicher deleted the fix-23208 branch July 11, 2018 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SecurityBundle] allow using custom function inside allow_if expressions #26660

[SecurityBundle] allow using custom function inside allow_if expressions #26660

[SecurityBundle] allow using custom function inside allow_if expressions #26660

[SecurityBundle] allow using custom function inside allow_if expressions #26660

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment