Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[SecurityBundle] allow using custom function inside allow_if expressions#26660
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| <tagname="cache.pool" /> | ||
| </service> | ||
| <serviceid="cache.security_expression_language"parent="cache.system"public="false"> |
nicolas-grekasMar 23, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
public="false"
no need, that's already the default oups no sorry, having "parent", this is required!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn't this service be defined in SecurityBundle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes makes more sense 👍
And we need to requiresymfony/framework-bundle then to havecache.system available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or we only use the cache if the parent servicecache.system is available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
framework-bundle is an implicit dep of any bundle, so no need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
and the fact that you referencecache.security_expression_language in SecurityBundle already creates this dependency anyway.
I agree about moving it to SecurityBundle
| $definition =$container->findDefinition('security.expression_language'); | ||
| foreach ($container->findTaggedServiceIds('security.expression_language_provider',true)as$id =>$attributes) { | ||
| $definition->addMethodCall('addExpressionLanguageProvider',array(newReference($id))); | ||
| $definition->addMethodCall('registerProvider',array(newReference($id))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 thesecurity.expression_language was not injected anywhere else than insecurity.access.expression_voter by default)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Will create a separate PR for 3.4 then 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See#26802.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
#26802 has now been merged up to master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I squashed my commits and rebased. Diff is gone now 👍
| <tagname="cache.pool" /> | ||
| </service> | ||
| <serviceid="cache.security_expression_language"parent="cache.system"public="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
and the fact that you referencecache.security_expression_language in SecurityBundle already creates this dependency anyway.
I agree about moving it to SecurityBundle
| </service> | ||
| <!-- Cache Warmers--> | ||
| <serviceid="security.cache_warmer.expression_cache_warmer"class="Symfony\Bundle\SecurityBundle\CacheWarmer\ExpressionCacheWarmer"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If ExpressionLanguage is not available, this cache warmer should not be registered, because it would still try to instantiatesecurity.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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Its done now here as well:adab5ce
If the ExpressionLanguage is not available then it fails when usingallow_if. So we can safely remove the definition if there are no expressions and it should be fine?
| <!-- Cache Warmers--> | ||
| <serviceid="security.cache_warmer.expression_cache_warmer"class="Symfony\Bundle\SecurityBundle\CacheWarmer\ExpressionCacheWarmer"> | ||
| <tagname="kernel.cache_warmer"></tag> | ||
| <argumenttype="tagged"tag="security.expression"></argument> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
instead of relying on a tag, which creates an external extension point, you could inject this in the DI extension (based on$this->expressions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
True 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this what you meant?
| privatefunctioncreateAuthorization($config,ContainerBuilder$container) | ||
| { | ||
| if (!$config['access_control']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@stof is this safe to just remove? It should always be an array?
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, here are some minor comments
| <!-- Cache Warmers--> | ||
| <serviceid="security.cache_warmer.expression"class="Symfony\Bundle\SecurityBundle\CacheWarmer\ExpressionCacheWarmer"> | ||
| <tagname="kernel.cache_warmer"></tag> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
would use an empty<tag /> instead of a closing one (same below for argument)
| <!-- Cache Warmers--> | ||
| <serviceid="security.cache_warmer.expression"class="Symfony\Bundle\SecurityBundle\CacheWarmer\ExpressionCacheWarmer"> | ||
| <tagname="kernel.cache_warmer"></tag> | ||
| <argument /><!-- collection of expression services--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
type="collection"
| } | ||
| /** | ||
| * @deprecated since Symfony 4.1, register the provider directly on the injected ExpressionLanguage instance instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing dot at EOL (same below on runtime notice)
| class ExpressionCacheWarmerimplements CacheWarmerInterface | ||
| { | ||
| /** | ||
| * @var iterable|Expression[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd suggest to move this as@param on the constructor and remove any docblock for properties
| private$expressions; | ||
| /** | ||
| * @var ExpressionLanguage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be removed (no need to put in on the constructor)
dmaicher commentedApr 4, 2018
Doc PR:symfony/symfony-docs#9552 @nicolas-grekas@stof final review? 😊 |
… 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#26660Commits-------3a55a86 [Security] register custom providers on ExpressionLanguage directly
fabpot commentedApr 6, 2018
Thank you@dmaicher. |
…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#9552This is a follow-up for#26263As 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 filesping@nicolas-grekas@stofCommits-------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 functionsSince 4.1 its possible to use custom functions inside security `allow_if` expressions.Seesymfony/symfony#26660Commits-------44708ad Reword8a96822 [Security] add tip about using custom functions
Uh oh!
There was an error while loading.Please reload this page.
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
masterhere.If we agree to move forward with this there are some todos:
ping@nicolas-grekas@stof