Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Nov 30, 2019

Conversation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 26, 2019

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

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Nov 26, 2019

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

This comment has been minimized.

Copy link
Member

stof commented Nov 26, 2019

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

This comment has been minimized.

Copy link
Member

weaverryan commented Nov 26, 2019

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 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 nicolas-grekas force-pushed the nicolas-grekas:lazy-firewalls branch 8 times, most recently from 81bcfa3 to 1f760b6 Nov 26, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Nov 26, 2019

PR green in a minute :)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:lazy-firewalls branch 5 times, most recently from 622671b to a2ad5ba Nov 26, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Nov 26, 2019

PR ready, now with functional tests.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:lazy-firewalls branch from a2ad5ba to 64cc080 Nov 27, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Nov 27, 2019

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.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:lazy-firewalls branch 2 times, most recently from 82c27c1 to 4a74ba4 Nov 29, 2019
Copy link
Contributor

aschempp left a comment

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

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Nov 29, 2019

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

@@ -170,7 +170,7 @@ Security
Role hierarchies must implement the `getReachableRoleNames()` method instead and return roles as strings.
* The `getRoles()` method of the `TokenInterface` is deprecated. Tokens must implement the `getRoleNames()`
method instead and return roles as strings.
* The `ListenerInterface` is deprecated, turn your listeners into callables instead.
* The `ListenerInterface` is deprecated, extend `AbstractListener` instead.

This comment has been minimized.

Copy link
@fabpot

fabpot Nov 30, 2019

Member

You are in the 4.3 changelog but the PR targets 4.4.

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)

This comment has been minimized.

Copy link
@fabpot

fabpot Nov 30, 2019

Member

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:lazy-firewalls branch from 4a74ba4 to e8ef30f Nov 30, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:lazy-firewalls branch from e8ef30f to b20ebe6 Nov 30, 2019
@fabpot
fabpot approved these changes Nov 30, 2019
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Nov 30, 2019

Thank you @nicolas-grekas.

fabpot added a commit that referenced this pull request Nov 30, 2019
…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
@fabpot fabpot merged commit b20ebe6 into symfony:4.4 Nov 30, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
This was referenced Dec 1, 2019
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:lazy-firewalls branch Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.