[SecurityBundle] support custom functions inside allow_if expressions #26263

dmaicher · 2018-02-21T19:01:10Z

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

This is a follow up for #24309.

Problem:
The SecurityExtension was instantiating an \Symfony\Component\Security\Core\Authorization\ExpressionLanguage instance to parse the expressions defined in the access control config via allow_if: "...". So it used a pre-parsed SerializedParsedExpression instead of a raw Expression probably for performance reasons.

This has the serious drawback (and that's the bug) that it does not use the service 'security.expression_language' for this during the compiler pass. So any customizations for the service (like custom providers with functions) are completely ignored here on compile time.

Proposed fix
I propose to not dump parsed expressions at all and to leave this for run-time and caching.

I did some benchmarks and would like to hear your thoughts on this.
Using this small benchmark script (https://gist.github.com/dmaicher/ae506ff436a4647f3eaf7e59ec4c4aa1) I get the following numbers:

Using NullAdapter:

raw:    39.814957857132s 
parsed: 19.232628822327s

Using ArrayAdapter for caching there is almost no difference:

raw:    21.031932115555s 
parsed: 19.204074144363s

So on my laptop the difference for the NullAdapter case is roughly 20s for 500 thousand evaluations. Which means for one evaluation it's in the range of 40 microseconds or 0.04ms.

Maybe we can make use of cache.system for the expression language on 3.3+?

WDYT?

nicolas-grekas · 2018-02-27T17:12:47Z

Should this really be done as a bug fix? Can't we consider it's a limitation for sure, but that's how it works for now? If you agree, then the cache warmup should be added in the PR on master.

dmaicher · 2018-02-27T17:17:35Z

We can also consider it as a undocumented limitation and I can add it as a feature to master. I just find the current behavior kind of weird to be honest. But I don't really mind 😊

Should we use cache.system then for the expression language cache?

nicolas-grekas · 2018-02-27T17:18:55Z

Should we use cache.system then for the expression language cache?

a dedicated pool instead, so userland items cannot collide with internals - see eg cache.validator

stof · 2018-02-27T17:19:53Z

An option for 2.7 could be to keep trying to parse in the extension at compile time, but fallback to runtime parsing (using Expression instead of SerializedParsedExpression) in case of an invalid function being used (as this is the only kind of parse error which can change once registering custom functions). This way, only projects using a custom function in their allow_if expression would be affected by the slower code path.

stof · 2018-02-27T17:21:49Z

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

@@ -36,10 +36,10 @@ public function process(ContainerBuilder $container)
        }

        // security
-        if ($container->has('security.access.expression_voter')) {
-            $definition = $container->findDefinition('security.access.expression_voter');
+        if ($container->has('security.expression_language')) {


why changing the registration logic ?

hmm, registering providers on the right service rather than relying on delegation still makes sense though, to ensure that the security.expression_language service is usable even if it is instantiated earlier than the ExpressionVoter due to injecting it elsewhere and due to voters being lazy-loaded).
We should probably even deprecated \Symfony\Component\Security\Core\Authorization\Voter\ExpressionVoter::addExpressionLanguageProvider in master as it only delegates the call and the ExpressionLanguage instance is always injected from the outside.

I think adding it to security.expression_language directly makes more sense? Not even sure why ExpressionVoter::addExpressionLanguageProvider exists. Maybe I'm missing something?

If someone injects security.expression_language and uses it without the ExpressionVoter service ever being instantiated then the custom providers would be missing?

But if it makes merging etc harder I can also revert it.

@stof exactly 😊

👍 for deprecating it

is this change required for 2.7? if not, better revert, isn't it, and do it on master only?

No its not required 👍 will revert and apply to master together with the deprecation then

@nicolas-grekas removed 😉

dmaicher · 2018-02-27T17:32:26Z

An option for 2.7 could be to keep trying to parse in the extension at compile time, but fallback to runtime parsing (using Expression instead of SerializedParsedExpression) in case of an invalid function being used (as this is the only kind of parse error which can change once registering custom functions). This way, only projects using a custom function in their allow_if expression would be affected by the slower code path.

@stof yes I was also thinking about that.

We could catch SyntaxError but this can also mean the expression simply has a wrong syntax 🙈 ~~So as a side effect instead of at compile time the error would show up on run-time. But I guess thats not a big problem?~~ => actually that's the case already with my current fix here 😊

dmaicher · 2018-02-27T19:37:42Z

@stof @nicolas-grekas I updated it so we keep trying to parse the expression on compile time and we simply fallback to using a raw expression if it fails.

Let me know what you think 😉

I could create a separate PR for master to use a new cache pool + deprecate ExpressionVoter::addExpressionLanguageProvider.

nicolas-grekas

I'm wondering if the runtime error is an acceptable DX downgrade (vs the current compile time failure)
WDYT?

dmaicher · 2018-02-28T19:57:39Z

Hmm not sure its a big problem. I mean when using the FrameworkExtraBundle @Security annotation with an expression it also appears on run-time for example?

What do you think @stof ?

dmaicher · 2018-03-07T20:42:57Z

@nicolas-grekas @stof should I continue to work on this? Or should we close the issue as "won't fix"?

nicolas-grekas · 2018-03-19T17:04:33Z

@dmaicher move as new feature on master?

dmaicher · 2018-03-19T19:05:30Z

@nicolas-grekas ok 😉 Will prepare a new PR soon for master that

does not pre-parse expressions anymore at compile-time
deprecates ExpressionVoter::addExpressionLanguageProvider
adds a cache pool for the security expression language

@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

carsonbot added Status: Needs Review SecurityBundle Bug labels Feb 21, 2018

dmaicher force-pushed the fix-23208 branch from d9e6743 to 9c9a725 Compare February 21, 2018 19:02

dmaicher mentioned this pull request Feb 21, 2018

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

Closed

nicolas-grekas added this to the 2.7 milestone Feb 22, 2018

dmaicher mentioned this pull request Feb 26, 2018

[SecurityBundle] fix allow_if expression service compilation to support custom functions #24309

Closed

stof reviewed Feb 27, 2018

View reviewed changes

dmaicher force-pushed the fix-23208 branch 2 times, most recently from 69f6e3c to 537222f Compare February 27, 2018 19:34

dmaicher changed the title ~~[SecurityBundle] do not pre-parse allow_if expressions to support custom functions~~ [SecurityBundle] support custom functions inside allow_if expressions Feb 27, 2018

[SecurityBundle] fallback to raw expressions to support custom functions

d791549

dmaicher force-pushed the fix-23208 branch from 537222f to d791549 Compare February 28, 2018 19:42

nicolas-grekas reviewed Feb 28, 2018

View reviewed changes

dmaicher closed this Mar 19, 2018

dmaicher mentioned this pull request Mar 23, 2018

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

Merged

4 tasks

Uh oh!

[SecurityBundle] support custom functions inside allow_if expressions #26263

[SecurityBundle] support custom functions inside allow_if expressions #26263

Uh oh!

Conversation

dmaicher commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Feb 27, 2018

Uh oh!

dmaicher commented Feb 27, 2018

Uh oh!

nicolas-grekas commented Feb 27, 2018

Uh oh!

stof commented Feb 27, 2018

Uh oh!

stof Feb 27, 2018

Choose a reason for hiding this comment

Uh oh!

stof Feb 27, 2018

Choose a reason for hiding this comment

Uh oh!

dmaicher Feb 27, 2018

Choose a reason for hiding this comment

Uh oh!

dmaicher Feb 27, 2018

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Feb 28, 2018

Choose a reason for hiding this comment

Uh oh!

dmaicher Feb 28, 2018

Choose a reason for hiding this comment

Uh oh!

dmaicher Feb 28, 2018

Choose a reason for hiding this comment

Uh oh!

dmaicher commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmaicher commented Feb 27, 2018

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

dmaicher commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmaicher commented Mar 7, 2018

Uh oh!

nicolas-grekas commented Mar 19, 2018

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

dmaicher commented Feb 21, 2018 •

edited

Loading

dmaicher commented Feb 27, 2018 •

edited

Loading

dmaicher commented Feb 28, 2018 •

edited

Loading

dmaicher commented Mar 19, 2018 •

edited

Loading