Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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

Merged
fabpot merged 1 commit intosymfony:4.4fromnicolas-grekas:lazy-firewalls
Nov 30, 2019

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedNov 26, 2019•
edited
Loading

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#34614,Fix#34679
LicenseMIT
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 newAbstractListener 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 whensupports() allows so - lazily or not depending on the return value ofsupports().

Of course, this remains compatible with non-lazy logics, seeAbstractListener::__invoke().

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedNov 26, 2019•
edited
Loading

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
Copy link
Member

In this case, it looks legit to me to require either the corresponding firewall to be stateless or anonymous to be set totrue (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
Copy link
Member

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, withanonymous: lazy, it "should" be ok to callsupports() on authentiicators. If the request should truly remain anonymous, then they authenticator will do nothing (i.e. return false fromsupports()). In other words: the authenticators would always be called, but then they could decide to allow the lazy anonymous to continue by doing nothingor 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 thejson_login example@dunglas gave here:#34614 (comment) - I usejson_login on stateful firewalls as a simple way to allow my JavaScript to authenticate. Would this also still require manually changing toanonymous: true?

Thanks :)

@nicolas-grekasnicolas-grekas changed the title[SecurityBundle] apply firewall laziness to cacheable requests only[Security/Http] call auth listeners/guards eagerly when they "support" the requestNov 26, 2019
@nicolas-grekasnicolas-grekasforce-pushed thelazy-firewalls branch 8 times, most recently from81bcfa3 to1f760b6CompareNovember 26, 2019 19:10
@nicolas-grekas
Copy link
MemberAuthor

PR green in a minute :)

@nicolas-grekasnicolas-grekasforce-pushed thelazy-firewalls branch 5 times, most recently from622671b toa2ad5baCompareNovember 26, 2019 21:33
@nicolas-grekas
Copy link
MemberAuthor

PR ready, now with functional tests.

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedNov 27, 2019•
edited
Loading

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

@nicolas-grekasnicolas-grekasforce-pushed thelazy-firewalls branch 2 times, most recently from82c27c1 to4a74ba4CompareNovember 29, 2019 13:46
Copy link
Contributor

@aschemppaschempp left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@nicolas-grekas
Copy link
MemberAuthor

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

private$map;

publicfunction__construct(iterable$listeners, ?ExceptionListener$exceptionListener, ?LogoutListener$logoutListener, ?FirewallConfig$config,AccessListener$accessListener,TokenStorage$tokenStorage,AccessMapInterface$map)
publicfunction__construct(iterable$listeners, ?ExceptionListener$exceptionListener, ?LogoutListener$logoutListener, ?FirewallConfig$config,TokenStorage$tokenStorage)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@fabpot
Copy link
Member

Thank you@nicolas-grekas.

fabpot added a commit that referenced this pull requestNov 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
@fabpotfabpot merged commitb20ebe6 intosymfony:4.4Nov 30, 2019
This was referencedDec 1, 2019
@nicolas-grekasnicolas-grekas deleted the lazy-firewalls branchDecember 2, 2019 09:03
nicolas-grekas added a commit that referenced this pull requestJun 23, 2025
…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       | MITAfter 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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@chalasrchalasrchalasr approved these changes

+1 more reviewer

@aschemppaschemppaschempp left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

7 participants

@nicolas-grekas@stof@weaverryan@fabpot@aschempp@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp