feature #26660 [SecurityBundle] allow using custom function inside allow_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 | https://github.com/symfony/symfony/issues/23208
| License       | MIT
| Doc PR        | https://github.com/symfony/symfony-docs/pull/9552

This is a follow-up for https://github.com/symfony/symfony/pull/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
-------

41552cd896 [SecurityBundle] allow using custom function inside allow_if expressions

UPGRADE-4.1.md

@@ -81,6 +81,7 @@ Security
    functionality, create a custom user-checker based on the
    `Symfony\Component\Security\Core\User\UserChecker`.
  * `AuthenticationUtils::getLastUsername()` now always returns a string.
+ * The `ExpressionVoter::addExpressionLanguageProvider()` method is deprecated. Register the provider directly on the injected ExpressionLanguage instance instead.
 
 SecurityBundle
 --------------

UPGRADE-5.0.md

@@ -76,6 +76,7 @@ Security
 
  * The `ContextListener::setLogoutOnUserChange()` method has been removed.
  * The `Symfony\Component\Security\Core\User\AdvancedUserInterface` has been removed.
+ * The `ExpressionVoter::addExpressionLanguageProvider()` method has been removed.
 
 SecurityBundle
 --------------

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

@@ -0,0 +1,43 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\SecurityBundle\CacheWarmer;
+
+use Symfony\Component\ExpressionLanguage\Expression;
+use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;
+use Symfony\Component\Security\Core\Authorization\ExpressionLanguage;
+
+class ExpressionCacheWarmer implements CacheWarmerInterface
+{
+    private $expressions;
+    private $expressionLanguage;
+
+    /**
+     * @param iterable|Expression[] $expressions
+     */
+    public function __construct(iterable $expressions, ExpressionLanguage $expressionLanguage)
+    {
+        $this->expressions = $expressions;
+        $this->expressionLanguage = $expressionLanguage;
+    }
+
+    public function isOptional()
+    {
+        return true;
+    }
+
+    public function warmUp($cacheDir)
+    {
+        foreach ($this->expressions as $expression) {
+            $this->expressionLanguage->parse($expression, array('token', 'user', 'object', 'subject', 'roles', 'request', 'trust_resolver'));
+        }
+    }
+}

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

@@ -26,7 +26,6 @@ use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Parameter;
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\Config\FileLocator;
-use Symfony\Component\Security\Core\Authorization\ExpressionLanguage;
 use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
 use Symfony\Component\Security\Core\Encoder\Argon2iPasswordEncoder;
 use Symfony\Component\Security\Http\Controller\UserValueResolver;
@@ -45,7 +44,6 @@ class SecurityExtension extends Extension
     private $listenerPositions = array('pre_auth', 'form', 'http', 'remember_me');
     private $factories = array();
     private $userProviderFactories = array();
-    private $expressionLanguage;
 
     public function __construct()
     {
@@ -136,10 +134,6 @@ class SecurityExtension extends Extension
 
     private function createAuthorization($config, ContainerBuilder $container)
     {
-        if (!$config['access_control']) {
-            return;
-        }
-
         foreach ($config['access_control'] as $access) {
             $matcher = $this->createRequestMatcher(
                 $container,
@@ -157,6 +151,14 @@ class SecurityExtension extends Extension
             $container->getDefinition('security.access_map')
                       ->addMethodCall('add', array($matcher, $attributes, $access['requires_channel']));
         }
+
+        // allow cache warm-up for expressions
+        if (count($this->expressions)) {
+            $container->getDefinition('security.cache_warmer.expression')
+                ->replaceArgument(0, new IteratorArgument(array_values($this->expressions)));
+        } else {
+            $container->removeDefinition('security.cache_warmer.expression');
+        }
     }
 
     private function createFirewalls($config, ContainerBuilder $container)
@@ -636,11 +638,14 @@ class SecurityExtension extends Extension
             return $this->expressions[$id];
         }
 
+        if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) {
+            throw new \RuntimeException('Unable to use expressions as the Symfony ExpressionLanguage component is not installed.');
+        }
+
         $container
-            ->register($id, 'Symfony\Component\ExpressionLanguage\SerializedParsedExpression')
+            ->register($id, 'Symfony\Component\ExpressionLanguage\Expression')
             ->setPublic(false)
             ->addArgument($expression)
-            ->addArgument(serialize($this->getExpressionLanguage()->parse($expression, array('token', 'user', 'object', 'roles', 'request', 'trust_resolver'))->getNodes()))
         ;
 
         return $this->expressions[$id] = new Reference($id);
@@ -703,16 +708,4 @@ class SecurityExtension extends Extension
         // first assemble the factories
         return new MainConfiguration($this->factories, $this->userProviderFactories);
     }
-
-    private function getExpressionLanguage()
-    {
-        if (null === $this->expressionLanguage) {
-            if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) {
-                throw new \RuntimeException('Unable to use expressions as the Symfony ExpressionLanguage component is not installed.');
-            }
-            $this->expressionLanguage = new ExpressionLanguage();
-        }
-
-        return $this->expressionLanguage;
-    }
 }

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

@@ -80,7 +80,9 @@
 
         <service id="security.user_checker" class="Symfony\Component\Security\Core\User\UserChecker" />
 
-        <service id="security.expression_language" class="Symfony\Component\Security\Core\Authorization\ExpressionLanguage" />
+        <service id="security.expression_language" class="Symfony\Component\Security\Core\Authorization\ExpressionLanguage">
+            <argument type="service" id="cache.security_expression_language"></argument>
+        </service>
 
         <service id="security.authentication_utils" class="Symfony\Component\Security\Http\Authentication\AuthenticationUtils" public="true">
             <argument type="service" id="request_stack" />
@@ -195,5 +197,17 @@
             <argument type="service" id="security.token_storage" />
             <argument type="service" id="security.encoder_factory" />
         </service>
+
+        <!-- Cache -->
+        <service id="cache.security_expression_language" parent="cache.system" public="false">
+            <tag name="cache.pool" />
+        </service>
+
+        <!-- Cache Warmers -->
+        <service id="security.cache_warmer.expression" class="Symfony\Bundle\SecurityBundle\CacheWarmer\ExpressionCacheWarmer">
+            <tag name="kernel.cache_warmer" />
+            <argument type="collection" /> <!-- expressions -->
+            <argument type="service" id="security.expression_language" />
+        </service>
     </services>
 </container>

src/Symfony/Bundle/SecurityBundle/Tests/CacheWarmer/ExpressionCacheWarmerTest.php

@@ -0,0 +1,35 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\SecurityBundle\Tests\CacheWarmer;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\Bundle\SecurityBundle\CacheWarmer\ExpressionCacheWarmer;
+use Symfony\Component\ExpressionLanguage\Expression;
+use Symfony\Component\Security\Core\Authorization\ExpressionLanguage;
+
+class ExpressionCacheWarmerTest extends TestCase
+{
+    public function testWarmUp()
+    {
+        $expressions = array(new Expression('A'), new Expression('B'));
+
+        $expressionLang = $this->createMock(ExpressionLanguage::class);
+        $expressionLang->expects($this->exactly(2))
+            ->method('parse')
+            ->withConsecutive(
+                array($expressions[0], array('token', 'user', 'object', 'subject', 'roles', 'request', 'trust_resolver')),
+                array($expressions[1], array('token', 'user', 'object', 'subject', 'roles', 'request', 'trust_resolver'))
+            );
+
+        (new ExpressionCacheWarmer($expressions, $expressionLang))->warmUp('');
+    }
+}

src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/SecurityExtensionTest.php

@@ -15,7 +15,10 @@ use PHPUnit\Framework\TestCase;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\SecurityExtension;
 use Symfony\Bundle\SecurityBundle\SecurityBundle;
 use Symfony\Bundle\SecurityBundle\Tests\DependencyInjection\Fixtures\UserProvider\DummyProvider;
+use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\ExpressionLanguage\Expression;
 
 class SecurityExtensionTest extends TestCase
 {
@@ -207,6 +210,66 @@ class SecurityExtensionTest extends TestCase
         $this->addToAssertionCount(1);
     }
 
+    public function testRegisterRequestMatchersWithAllowIfExpression()
+    {
+        $container = $this->getRawContainer();
+
+        $rawExpression = "'foo' == 'bar' or 1 in [1, 3, 3]";
+
+        $container->loadFromExtension('security', array(
+            'providers' => array(
+                'default' => array('id' => 'foo'),
+            ),
+            'firewalls' => array(
+                'some_firewall' => array(
+                    'pattern' => '/.*',
+                    'http_basic' => array(),
+                ),
+            ),
+            'access_control' => array(
+                array('path' => '/', 'allow_if' => $rawExpression),
+            ),
+        ));
+
+        $container->compile();
+        $accessMap = $container->getDefinition('security.access_map');
+        $this->assertCount(1, $accessMap->getMethodCalls());
+        $call = $accessMap->getMethodCalls()[0];
+        $this->assertSame('add', $call[0]);
+        $args = $call[1];
+        $this->assertCount(3, $args);
+        $expressionId = $args[1][0];
+        $this->assertTrue($container->hasDefinition($expressionId));
+        $expressionDef = $container->getDefinition($expressionId);
+        $this->assertSame(Expression::class, $expressionDef->getClass());
+        $this->assertSame($rawExpression, $expressionDef->getArgument(0));
+
+        $this->assertTrue($container->hasDefinition('security.cache_warmer.expression'));
+        $this->assertEquals(
+            new IteratorArgument(array(new Reference($expressionId))),
+            $container->getDefinition('security.cache_warmer.expression')->getArgument(0)
+        );
+    }
+
+    public function testRemovesExpressionCacheWarmerDefinitionIfNoExpressions()
+    {
+        $container = $this->getRawContainer();
+        $container->loadFromExtension('security', array(
+            'providers' => array(
+                'default' => array('id' => 'foo'),
+            ),
+            'firewalls' => array(
+                'some_firewall' => array(
+                    'pattern' => '/.*',
+                    'http_basic' => array(),
+                ),
+            ),
+        ));
+        $container->compile();
+
+        $this->assertFalse($container->hasDefinition('security.cache_warmer.expression'));
+    }
+
     protected function getRawContainer()
     {
         $container = new ContainerBuilder();

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

@@ -37,8 +37,13 @@ class ExpressionVoter implements VoterInterface
         $this->roleHierarchy = $roleHierarchy;
     }
 
+    /**
+     * @deprecated since Symfony 4.1, register the provider directly on the injected ExpressionLanguage instance instead.
+     */
     public function addExpressionLanguageProvider(ExpressionFunctionProviderInterface $provider)
     {
+        @trigger_error(sprintf('The %s() method is deprecated since Symfony 4.1, register the provider directly on the injected ExpressionLanguage instance instead.', __METHOD__), E_USER_DEPRECATED);
+
         $this->expressionLanguage->registerProvider($provider);
     }