[Security/Http] call auth listeners/guards eagerly when they "support" the request #34627

nicolas-grekas · 2019-11-26T10:56:27Z

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().

nicolas-grekas · 2019-11-26T11:04:34Z

~~Not that we might consider always calling guard's "supports()" methods non-lazily, but I don't think the current design allows it easily. Let me check.~~ PR updated.

stof · 2019-11-26T11:06:49Z

In this case, it looks legit to me to require either the corresponding firewall to be stateless or anonymous to be set to true (both options disabling laziness).

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)

weaverryan · 2019-11-26T13:38:03Z

Not that we might consider always calling guard's "supports()" methods non-lazily, but I don't think the current design allows it easily. Let me check

I was wondering about this too. On a high-level, with anonymous: lazy, it "should" be ok to call supports() on authentiicators. If the request should truly remain anonymous, then they authenticator will do nothing (i.e. return false from supports()). In other words: the authenticators would always be called, but then they could decide to allow the lazy anonymous to continue by doing nothing or to authenticate. However, I realize on a technical level, that might be difficult :).

Here's another problematic example: OAuth. When an OAuth server redirects back to my site...

In this case, it looks legit to me to require either the corresponding firewall to be stateless or
anonymous to be set to true (both options disabling laziness).

Also, what about the json_login example @dunglas gave here: #34614 (comment) - I use json_login on stateful firewalls as a simple way to allow my JavaScript to authenticate. Would this also still require manually changing to anonymous: true?

Thanks :)

nicolas-grekas · 2019-11-26T19:11:13Z

PR green in a minute :)

nicolas-grekas · 2019-11-26T21:33:25Z

PR ready, now with functional tests.

nicolas-grekas · 2019-11-27T20:45:55Z

PR updated, last change: laziness now mandates extending AbstractListener.
This makes things more BC/FC: one needs to explicitly opt-in to benefit from laziness.
We might consider deprecating not extending AbstractListener in 5.1.

src/Symfony/Component/Security/Http/Firewall/AbstractListener.php

aschempp

If I understand correctly, the whole lazy firewall behavior is meant to prevent unnecessary session access? Why not just skip/lazy-load the ContextListener and always call all other listeners?

nicolas-grekas · 2019-11-29T15:19:55Z

Why not just skip/lazy-load the ContextListener and always call all other listeners?

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 :)

UPGRADE-4.3.md

fabpot · 2019-11-30T09:18:19Z

src/Symfony/Bundle/SecurityBundle/Security/LazyFirewallContext.php


-    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)


Isn't it a BC break? This class is not marked as being internal.

…" the request

fabpot · 2019-11-30T09:56:30Z

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

nicolas-grekas added this to the 4.4 milestone Nov 26, 2019

carsonbot added Status: Needs Review SecurityBundle Bug labels Nov 26, 2019

nicolas-grekas changed the title ~~[SecurityBundle] apply firewall laziness to cacheable requests only~~ [Security/Http] call auth listeners/guards eagerly when they "support" the request Nov 26, 2019

nicolas-grekas force-pushed the lazy-firewalls branch 8 times, most recently from 81bcfa3 to 1f760b6 Compare November 26, 2019 19:10

nicolas-grekas force-pushed the lazy-firewalls branch 5 times, most recently from 622671b to a2ad5ba Compare November 26, 2019 21:33

nicolas-grekas force-pushed the lazy-firewalls branch from a2ad5ba to 64cc080 Compare November 27, 2019 20:42

chalasr reviewed Nov 29, 2019

View reviewed changes

src/Symfony/Component/Security/Http/Firewall/AbstractListener.php Show resolved Hide resolved

nicolas-grekas force-pushed the lazy-firewalls branch 2 times, most recently from 82c27c1 to 4a74ba4 Compare November 29, 2019 13:46

chalasr approved these changes Nov 29, 2019

View reviewed changes

carsonbot removed the Status: Needs Review label Nov 29, 2019

carsonbot added the Status: Reviewed label Nov 29, 2019

aschempp reviewed Nov 29, 2019

View reviewed changes

fabpot requested changes Nov 30, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Nov 30, 2019

nicolas-grekas force-pushed the lazy-firewalls branch from 4a74ba4 to e8ef30f Compare November 30, 2019 09:46

[Security/Http] call auth listeners/guards eagerly when they "support…

b20ebe6

…" the request

nicolas-grekas force-pushed the lazy-firewalls branch from e8ef30f to b20ebe6 Compare November 30, 2019 09:49

fabpot approved these changes Nov 30, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Nov 30, 2019

fabpot merged commit b20ebe6 into symfony:4.4 Nov 30, 2019

nicolas-grekas mentioned this pull request Dec 1, 2019

[Security] Auto-login after registration doesn't work with lazy firewalls. #34736

Closed

This was referenced Dec 1, 2019

Release v4.4.1 #34742

Merged

Release v5.0.1 #34743

Merged

nicolas-grekas deleted the lazy-firewalls branch December 2, 2019 09:03

chalasr mentioned this pull request Dec 2, 2019

Method triggerRememberMe not called because of LazyResponseException #34761

Closed

tristanbes mentioned this pull request Mar 25, 2020

Login a user in a programatic way is no longer possible using Guard and anonymous: lazy #36208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security/Http] call auth listeners/guards eagerly when they "support" the request #34627

[Security/Http] call auth listeners/guards eagerly when they "support" the request #34627

nicolas-grekas commented Nov 26, 2019 •

edited

Loading

nicolas-grekas commented Nov 26, 2019 •

edited

Loading

stof commented Nov 26, 2019

weaverryan commented Nov 26, 2019

nicolas-grekas commented Nov 26, 2019

nicolas-grekas commented Nov 26, 2019

nicolas-grekas commented Nov 27, 2019 •

edited

Loading

aschempp left a comment •

edited

Loading

nicolas-grekas commented Nov 29, 2019

fabpot Nov 30, 2019

fabpot commented Nov 30, 2019


		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)

[Security/Http] call auth listeners/guards eagerly when they "support" the request #34627

[Security/Http] call auth listeners/guards eagerly when they "support" the request #34627

Conversation

nicolas-grekas commented Nov 26, 2019 • edited Loading

nicolas-grekas commented Nov 26, 2019 • edited Loading

stof commented Nov 26, 2019

weaverryan commented Nov 26, 2019

nicolas-grekas commented Nov 26, 2019

nicolas-grekas commented Nov 26, 2019

nicolas-grekas commented Nov 27, 2019 • edited Loading

aschempp left a comment • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Nov 29, 2019

fabpot Nov 30, 2019

Choose a reason for hiding this comment

fabpot commented Nov 30, 2019

nicolas-grekas commented Nov 26, 2019 •

edited

Loading

nicolas-grekas commented Nov 26, 2019 •

edited

Loading

nicolas-grekas commented Nov 27, 2019 •

edited

Loading

aschempp left a comment •

edited

Loading