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] add "anonymous: lazy" mode to firewalls#33676

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
chalasr merged 1 commit intosymfony:4.4fromnicolas-grekas:sec-lazy2
Sep 27, 2019

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedSep 23, 2019
edited
Loading

QA
Branch?4.4
Bug fix?no
New feature?yes
Deprecations?no
TicketsFixes#26769 et al.
LicenseMIT
Doc PR-

Contains#33663 until it is merged.

This PR allows defining a firewall as such:

security:firewalls:main:anonymous:lazy

This means that the corresponding area should not start the session / load the user unless the application actively gets access to it. On pages that don't fetch the user at all, this means the session is not started, which means the corresponding token neither is. Lazily, when the user is accessed, e.g. via a call tois_granted(), the user is loaded, starting the session if needed.

See#27817 for previous explanations on the topic also.

Note that thanks to the logic in#33663, this PR doesn't have the drawback spotted in#27817: here, the profiler works as expected.

Recipe update pending atsymfony/recipes#649

wadjeroudi and cybernet reacted with thumbs up emoji
@wadjeroudi
Copy link

with this option enabled, the untracked_token_storage becomes useless if I'm correct ?
Does this PR really need to contains#33663 ?

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedSep 24, 2019
edited
Loading

@wadjeroudi yes it's needed, we still need to be able to differentiate usages that affect the response from others. See eg penultimate sentence in the description above.

@nicolas-grekas
Copy link
MemberAuthor

Now tested functionally, PR ready on my side, waiting for#33663.

@nicolas-grekas
Copy link
MemberAuthor

Status: needs review

nicolas-grekas added a commit that referenced this pull requestSep 24, 2019
…-grekas)This PR was merged into the 4.3 branch.Discussion----------[Security/Http] fix typo in deprecation message| Q             | A| ------------- | ---| Branch?       | 4.3| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -spotted by@stof in#33676Commits-------e70057a [Security/Http] fix typo in deprecation message
Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

The CHANGELOG needs to be updated :)

nicolas-grekas reacted with thumbs up emoji
@chalasr
Copy link
Member

Thank you@nicolas-grekas.

chalasr added a commit that referenced this pull requestSep 27, 2019
…colas-grekas)This PR was merged into the 4.4 branch.Discussion----------[Security] add "anonymous: lazy" mode to firewalls| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       |Fixes#26769 et al.| License       | MIT| Doc PR        | -Contains#33663 until it is merged.This PR allows defining a firewall as such:```yamlsecurity:    firewalls:        main:            anonymous: lazy```This means that the corresponding area should not start the session / load the user unless the application actively gets access to it. On pages that don't fetch the user at all, this means the session is not started, which means the corresponding token neither is. Lazily, when the user is accessed, e.g. via a call to `is_granted()`, the user is loaded, starting the session if needed.See#27817 for previous explanations on the topic also.Note that thanks to the logic in#33633, this PR doesn't have the drawback spotted in#27817: here, the profiler works as expected.Recipe update pending atsymfony/recipes#649Commits-------5cd1d7b [Security] add "anonymous: lazy" mode to firewalls
@chalasrchalasr merged commit5cd1d7b intosymfony:4.4Sep 27, 2019
@nicolas-grekasnicolas-grekas deleted the sec-lazy2 branchSeptember 30, 2019 14:33
@nicolas-grekasnicolas-grekas modified the milestones:next,4.4Oct 27, 2019
Copy link
Contributor

@aschemppaschempp left a comment

Choose a reason for hiding this comment

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

Sorry for being late, I only saw this in the blog post. I'm really confused by this implementation.

First of all, the statement that anonymous tokens are stateful "to track users" (#27817) seems incorrect to me, because anonymous tokens are not stored in the session here:https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Security/Http/Firewall/ContextListener.php#L168

What I find difficult to understand is the relation between anonymous users and lazy loading a firewall. A firewall can choose to not have an anonymous authenticator, but lazily loading the session data might still make sense. The two are not related at all to me. This can be best seen by the changes in theSecurityExtension. Using the lazy loading context is decided uponnot stateless and ifanonymous islazy. It's not anonymous that is lazy, it's the firewall itself, isn't it? Or ratherstateless: 'lazy'?

On a side note, did you consider the problem that firewall listeners are no longer executed on their previouskernel.request priority (firewall defaults to 8) but can be executed anywhere in the code flow? I assume someone affected by this would simply need to disable lazy-ness?

@nicolas-grekas
Copy link
MemberAuthor

@aschempp please open a dedicated issue if you're suggesting we should change some things or discuss this further.

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedNov 7, 2019
edited
Loading

@aschempp here are some thoughts:

First of all, the statement that anonymous tokens are stateful "to track users" (#27817) seems incorrect to me, because anonymous tokens are not stored in the session

The session is still kept open for the browsing session so that it's possible to track users of the website (they wouldn't be "users of the app" from Security pov that's true).

What I find difficult to understand is the relation between anonymous users and lazy loading a firewall.

anonymous is the type where we know we can be lazy - ie we don't need to enforce any authorizations before allowing anything else on the app.

as soon as a non-anonymous user is needed, we need to check its authorizations before going anywhere further, which means starting the session eagerly. That's the relation to me, does this make better sense?

A firewall can choose to not have an anonymous authenticator, but lazily loading the session data might still make sense.

Not really, because the very first thing that must be done to ensure that one isnot anonymous is to start the session to know who is this non-anonymous.

The two are not related at all to me. This can be best seen by the changes in the SecurityExtension. Using the lazy loading context is decided upon not stateless and if anonymous is lazy. It's not anonymous that is lazy, it's the firewall itself, isn't it? Or rather stateless: 'lazy'?

We can make it lazy, but it would be resolved immedately, to do the check I mentionned just above.

On a side note, did you consider the problem that firewall listeners are no longer executed on their previous kernel.request priority (firewall defaults to 8) but can be executed anywhere in the code flow? I assume someone affected by this would simply need to disable lazy-ness?

Absolutely, that's the very reason why this is opt-in.

nicolas-grekas added a commit that referenced this pull requestNov 7, 2019
…cyweb)This PR was merged into the 5.0-dev branch.Discussion----------[SecurityBundle] Remove deprecated service and code| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -Remove deprecated service in#25839 and deprecated code in#33676.Commits-------ad61d6f [SecurityBundle] Remove deprecated service and code
This was referencedNov 12, 2019
@aschempp
Copy link
Contributor

The two are not related at all to me. This can be best seen by the changes in the SecurityExtension. Using the lazy loading context is decided upon not stateless and if anonymous is lazy. It's not anonymous that is lazy, it's the firewall itself, isn't it? Or rather stateless: 'lazy'?

We can make it lazy, but it would be resolved immedately, to do the check I mentionned just above.

Ah, maybe that's what I got wrong. So essentially, wecould uselazy for non-anonymous (the code would work), but it does not make sense because it would always be resolved immediately? Is that also the case if there is noaccess_control in the security configuration? Can you tell what other service than routing needs the firewall user before any controller?

Btw. I'm on my way to SymfonyCon if you're interested to discuss this in person 🙃

@nicolas-grekas
Copy link
MemberAuthor

@aschempp see you soon :)

Please have a look at#34358 and#34357 on the topic.

aschempp reacted with thumbs up emojiaschempp reacted with hooray emoji

@stof
Copy link
Member

Is that also the case if there is noaccess_control in the security configuration? Can you tell what other service than routing needs the firewall user before any controller?

if you don't have any access_control in your configuration, enabling lazy mode would actually let user access your site anonymously (which should not be allowed) for any page not adding additional checks. That's why#34358 now forces the check in this case (which makes lazyness useless in this case as well)

@aschempp
Copy link
Contributor

Just to wrap this up 🙃
I finally got my head around whylazy only works with "anonymous: true": Because the firewall will not allow access to the route, if no user is logged in. Which means it can't be lazy. Until now I was assuming there would simple be no token instead of anAnonymousToken. But I agree that would not make sense.

However, quickly afterwards I told@Toflar "That wont work with REMEMBERME" and he remembered something about that, so we found#34679. I'll see about that and eventually contribute in the still-open PRs (instead of bugging you in a closed PR 🙈)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof requested 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.

6 participants

@nicolas-grekas@wadjeroudi@chalasr@aschempp@stof@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp