[WIP/RFC] Use framework expression language service instead of custom one by Toflar · Pull Request #2289 · api-platform/core

Toflar · 2018-10-29T16:02:36Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#2265
License	MIT
Doc PR

Using the framework security.expression_language service rather than our own one allows us to extend the expression language with our own functions easily. Symfony already provides an easy way to register custom functions in AddExpressionLanguageProvidersPass. ApiPlatform currently does not. But I figured instead of providing our own compiler pass we can just use the built-in security.expression_language, extend that with our is_granted() function and thus at the same time also allow developers to add even more functions. Wdyt?

dunglas · 2018-10-29T18:35:21Z

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

    <services>
        <service id="api_platform.security.resource_access_checker" class="ApiPlatform\Core\Security\ResourceAccessChecker" public="false">
-            <argument type="service" id="api_platform.security.expression_language" on-invalid="null" />
+            <argument type="service" id="security.expression_language" on-invalid="null" />


Can you create an alias api_platform.security.expression_language please? It will be useful for people wanting to inject a custom service.

dunglas · 2018-10-29T18:36:22Z

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

            <tag name="kernel.event_listener" event="kernel.request" method="onKernelRequest" priority="1" />
        </service>
+
+        <service id="api_platform.security.expression_language_provider" class="ApiPlatform\Core\Security\Core\Authorization\ExpressionLanguageProvider">


Suggested change

<service id="api_platform.security.expression_language_provider" class="ApiPlatform\Core\Security\Core\Authorization\ExpressionLanguageProvider">

<service id="api_platform.security.expression_language_provider" class="ApiPlatform\Core\Security\Core\Authorization\ExpressionLanguageProvider" public="false">

dunglas · 2018-10-29T18:36:54Z

src/Security/Core/Authorization/ExpressionLanguageProvider.php

+use Symfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface;
+
+/**
+ * Adding ApiPlatform specific Expression Language functions.


Suggested change

* Adding ApiPlatform specific Expression Language functions.

* Registers API Platform's Language functions.

dunglas · 2018-10-29T18:37:01Z

src/Security/Core/Authorization/ExpressionLanguageProvider.php

+ *
+ * @author Yanick Witschi <yanick.witschi@terminal42.ch>
+ */
+class ExpressionLanguageProvider implements ExpressionFunctionProviderInterface


Suggested change

class ExpressionLanguageProvider implements ExpressionFunctionProviderInterface

final class ExpressionLanguageProvider implements ExpressionFunctionProviderInterface

dunglas · 2018-10-29T18:37:13Z

src/Security/Core/Authorization/ExpressionLanguageProvider.php

+ */
+class ExpressionLanguageProvider implements ExpressionFunctionProviderInterface
+{
+    public function getFunctions()


Suggested change

public function getFunctions()

public function getFunctions(): array

dunglas · 2018-10-29T18:37:52Z

src/Security/Core/Authorization/ExpressionLanguageProvider.php

+    public function getFunctions()
+    {
+        return [
+            new ExpressionFunction('is_granted', function ($attributes, $object = 'null') {


Wouldn't it conflict with the one registered by SensioFrameworkExtraBundle?

One would just override the other (depending on the order they are called). And as far as I could see the idea was to provide the same logic, right? I mean, we could only register it if the bundle is not loaded if that makes you more comfortable? Or just tell the users to require the framework-extra-bundle if they want this function?

I just want to be sure it works as intended in all cases :)

It should :)

src/Security/ExpressionLanguage.php

Toflar · 2018-10-30T07:59:10Z

Comments addressed :)

dunglas · 2018-11-05T10:53:06Z

@Toflar errors look related to this PR, could you take a look (I'll tag a new release soon). Thanks

Toflar · 2018-11-06T13:37:30Z

Done!

teohhanhui · 2018-11-07T15:05:54Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php

        $bundles = $container->getParameter('kernel.bundles');
        if (isset($bundles['SecurityBundle'])) {
-            if (class_exists(ExpressionLanguage::class)) {
-                $loader->load('security_expression_language.xml');


Actually, symfony/expression-language is in require-dev of symfony/security-bundle: https://github.com/symfony/symfony/blob/b5c6892911fc7454a76528cb0d4d1af4336c7413/src/Symfony/Bundle/SecurityBundle/composer.json#L42

So we still need to check that the ExpressionLanguage component is actually available.

Are you sure? security.expression_language would not be present so the alias would become invalid and as we use on-invalid="null" on api_platform.security.resource_access_checker that should work. Haven't checked though.

I'm not sure. But I think we might as well just keep them in a separate file.

Toflar · 2019-01-07T09:24:40Z

Can I do anything here? I still think the way it is now is correct :)

soyuka · 2019-02-10T17:34:24Z

Can you rebase? LGTM, we should merge this!

Toflar · 2019-02-11T08:44:36Z

Rebased onto master, not sure if I should've gone for the 2.4 branch? The deps=low test fails but apparently that's also the case for master.

soyuka · 2019-02-11T09:03:43Z

Master is fine, yeah don't worry about deps=low I'll try to fix it later, thanks!

alanpoulain · 2019-02-11T09:37:41Z

@soyuka the issue with deps=low comes from Composer. It will be resolved in the next version or if you do composer selfupdate --snapshot.

Toflar mentioned this pull request Oct 29, 2018

access_control, expression language instance #2265

Closed

Toflar force-pushed the use-security-expression-language branch 2 times, most recently from 6b0fc67 to 190e85a Compare October 29, 2018 16:08

dunglas requested changes Oct 29, 2018

View reviewed changes

Toflar force-pushed the use-security-expression-language branch 3 times, most recently from 91a460b to f23ca61 Compare November 6, 2018 12:37

dunglas approved these changes Nov 6, 2018

View reviewed changes

soyuka approved these changes Nov 6, 2018

View reviewed changes

teohhanhui suggested changes Nov 7, 2018

View reviewed changes

Toflar added 2 commits February 11, 2019 09:35

Use framework expression language service instead of custom one

e38fd98

Fixed tests

59039a5

Toflar force-pushed the use-security-expression-language branch from 0fe5cfc to 59039a5 Compare February 11, 2019 08:36

phpunit ^7.5.4 and bump cs fixer

22d350d

soyuka merged commit e1f92d0 into api-platform:master Feb 11, 2019

Toflar deleted the use-security-expression-language branch February 11, 2019 09:54

tezvi mentioned this pull request Feb 21, 2019

Use Symfony ExpressionFunctionProviders with api_platform.security.expression_language api-platform/api-platform#944

Closed

	<service id="api_platform.security.expression_language_provider" class="ApiPlatform\Core\Security\Core\Authorization\ExpressionLanguageProvider">
	<service id="api_platform.security.expression_language_provider" class="ApiPlatform\Core\Security\Core\Authorization\ExpressionLanguageProvider" public="false">

	* Adding ApiPlatform specific Expression Language functions.
	* Registers API Platform's Language functions.

	class ExpressionLanguageProvider implements ExpressionFunctionProviderInterface
	final class ExpressionLanguageProvider implements ExpressionFunctionProviderInterface

	public function getFunctions()
	public function getFunctions(): array

Uh oh!

Comments

Conversation

Toflar commented Oct 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Toflar commented Oct 30, 2018

Uh oh!

dunglas commented Nov 5, 2018

Uh oh!

Toflar commented Nov 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Toflar commented Jan 7, 2019

Uh oh!

soyuka commented Feb 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Toflar commented Feb 11, 2019

Uh oh!

soyuka commented Feb 11, 2019

Uh oh!

alanpoulain commented Feb 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

teohhanhui Nov 19, 2018 •

edited

Loading

soyuka commented Feb 10, 2019 •

edited

Loading