[Security/Http] call auth listeners/guards eagerly when they "support" the request #34627
Conversation
|
|
maybe we should allow to define a list of path for which you want to disable lazyness (the check paths of your OAuth callbacks could then be registered there) |
I was wondering about this too. On a high-level, with
Also, what about the Thanks :) |
81bcfa3 to
1f760b6
Compare
|
PR green in a minute :) |
622671b to
a2ad5ba
Compare
|
PR ready, now with functional tests. |
a2ad5ba to
64cc080
Compare
|
PR updated, last change: laziness now mandates extending |
82c27c1 to
4a74ba4
Compare
I don't understand what you mean sorry. The order is important so we cannot call one listener lazily. "just" here is missing the fact all this work is happening in the Security component, nothing is simple there :) |
|
|
||
| public function __construct(iterable $listeners, ?ExceptionListener $exceptionListener, ?LogoutListener $logoutListener, ?FirewallConfig $config, AccessListener $accessListener, TokenStorage $tokenStorage, AccessMapInterface $map) | ||
| public function __construct(iterable $listeners, ?ExceptionListener $exceptionListener, ?LogoutListener $logoutListener, ?FirewallConfig $config, TokenStorage $tokenStorage) |
There was a problem hiding this comment.
Isn't it a BC break? This class is not marked as being internal.
4a74ba4 to
e8ef30f
Compare
e8ef30f to
b20ebe6
Compare
|
Thank you @nicolas-grekas. |
…ey "support" the request (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [Security/Http] call auth listeners/guards eagerly when they "support" the request | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #34614, Fix #34679 | License | MIT | Doc PR | - This fixes the form authenticator linked to #34614. Since laziness is here to provide compatibility with HTTP caching, it should be disabled when the request cannot be cached. Tests don't pass yet, but I'm on the path to something here. The PR now introduces a new `AbstractListener` that splits the handling logic in two: - `supports(Request): ?bool` is always called eagerly and tells whether the listener matches the request for an earger call or a lazy call - `authenticate(RequestEvent)` does the rest of the job when `supports()` allows so - lazily or not depending on the return value of `supports()`. Of course, this remains compatible with non-lazy logics, see `AbstractListener::__invoke()`. Commits ------- b20ebe6 [Security/Http] call auth listeners/guards eagerly when they "support" the request
…heCat) This PR was merged into the 7.4 branch. Discussion ---------- [Security] Deprecate callable firewall listeners | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | no | Deprecations? | yes | Issues | N/A | License | MIT After spending some time in the Security component it occurred to me callable firewall listeners are obsolete now that we got the `FirewallListenerInterface`. Their deprecation has already been suggested (like in #34627 (comment) or #38751 (review)), so this PR does it. Commits ------- 510e506 [Security] Deprecate callable firewall listeners
This fixes the form authenticator linked to #34614.
Since laziness is here to provide compatibility with HTTP caching, it should be disabled when the request cannot be cached.
Tests don't pass yet, but I'm on the path to something here.
The PR now introduces a new
AbstractListenerthat splits the handling logic in two:supports(Request): ?boolis always called eagerly and tells whether the listener matches the request for an earger call or a lazy callauthenticate(RequestEvent)does the rest of the job whensupports()allows so - lazily or not depending on the return value ofsupports().Of course, this remains compatible with non-lazy logics, see
AbstractListener::__invoke().