Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
11662aa to251957aComparewadjeroudi commentedSep 24, 2019
with this option enabled, the untracked_token_storage becomes useless if I'm correct ? |
nicolas-grekas commentedSep 24, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 commentedSep 24, 2019
Now tested functionally, PR ready on my side, waiting for#33663. |
src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AnonymousFactory.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/SecurityBundle/Security/LazyFirewallContext.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Security/Core/Authentication/Token/Storage/TokenStorage.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
406ec49 to983be66Comparenicolas-grekas commentedSep 24, 2019
Status: needs review |
…-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
src/Symfony/Component/Security/Core/Exception/LazyResponseException.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
chalasr left a comment
There was a problem hiding this 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 :)
chalasr commentedSep 27, 2019
Thank you@nicolas-grekas. |
…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
aschempp left a comment
There was a problem hiding this 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?
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedNov 2, 2019
@aschempp please open a dedicated issue if you're suggesting we should change some things or discuss this further. |
nicolas-grekas commentedNov 7, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@aschempp here are some thoughts:
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).
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?
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.
We can make it lazy, but it would be resolved immedately, to do the check I mentionned just above.
Absolutely, that's the very reason why this is opt-in. |
…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
aschempp commentedNov 20, 2019
Ah, maybe that's what I got wrong. So essentially, wecould use Btw. I'm on my way to SymfonyCon if you're interested to discuss this in person 🙃 |
nicolas-grekas commentedNov 20, 2019
stof commentedNov 20, 2019
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 commentedNov 29, 2019
Just to wrap this up 🙃 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 🙈) |
Uh oh!
There was an error while loading.Please reload this page.
Contains#33663 until it is merged.
This PR allows defining a firewall as such:
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#33663, this PR doesn't have the drawback spotted in#27817: here, the profiler works as expected.
Recipe update pending atsymfony/recipes#649