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 "lazy_authentication" mode to firewalls#27817

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

Closed
nicolas-grekas wants to merge1 commit intosymfony:4.4fromnicolas-grekas:sec-lazy

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedJul 3, 2018
edited
Loading

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#26769et al.
LicenseMIT
Doc PR-

Wouhou, here is my first significant PR on the Security component :)

This PR is aimed at allowing unauthenticated access to resources that don't read the user token. It applies to stateful firewalls only. Right now, when a stateful firewall is configured, a user token is always hydrated from the session. This has the effect of making the response uncacheable.

When access control defines no specific roles for a resource and when no furtheris_granted() checks are made when computing it, one doesn't really need statefulness.

This PR allows differentiating the "anonymous" mode from the "unauthenticated" one: "anonymous" is stateful and allows e.g. tracking the journey of a user on a website even if we don't know who this is. "unauthenticated" on the other end is anonymous+stateless - i.e. we don't track navigation.

When "lazy_authentication" is enabled, all security listeners are called lazily only when the user token is actually read from the token storage.

A visible side effect is that ESI fragments can be more easily cached (see linked issue). Another one is that the debug toolbar will report as "unauthenticated" when browsing such a page. And a last one is that once#27806 is merged,TokenProcessor won't add the username on the logs when a resource didn't start the authentication stack.

The next step should be to enable this rule by default inhttps://github.com/symfony/recipes/blob/master/symfony/security-bundle/3.3/config/packages/security.yaml:

security:firewalls:main:anonymous:truelazy_authentication:true

TODO:

  • add tests
  • doc PR

WDYT?

EmmanuelVella, welcoMattic, HeahDude, and DavidBadura reacted with thumbs up emojiKoc reacted with confused emoji
@xavismeh
Copy link
Contributor

Really interesting feature! Still, the web debug toolbar should force the display of authenticated status even when not necessary on the page since it could be very confusing for DX. But I have no relevant suggestion to make on how to deal with this...

<serviceid="data_collector.security"class="Symfony\Bundle\SecurityBundle\DataCollector\SecurityDataCollector">
<tagname="data_collector"template="@Security/Collector/security.html.twig"id="security"priority="270" />
<argumenttype="service"id="security.token_storage"on-invalid="ignore" />
<argumenttype="service"id="security.actual_token_storage" />
Copy link
Contributor

Choose a reason for hiding this comment

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

this name feels weird (I don't have a better suggestion though)

<serviceid="Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface"alias="security.authorization_checker" />

<serviceid="security.token_storage"class="Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage"public="true">
<serviceid="security.token_storage"class="Symfony\Component\Security\Core\Authentication\Token\Storage\LazyTokenStorage"public="true">
Copy link
Contributor

Choose a reason for hiding this comment

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

how about implementing a decoration for theactual_token_storage here, instead ? So that you can rename ittoken_storage and let this service decorate the token_storage ?

@linaori
Copy link
Contributor

A few questions:

  • Does this mean that if my minimal authentication level isIS_AUTHENTICATED_ANONYMOUSLY, this option is useless? This will be quite some work to configure properly in an application, because you can't wild-card anymore with exceptions in access control (it is always triggered).
  • Is this actually useful if you have even 1isGrantedanywhere? Let's say I have a small part where I always check if you're allowed to login or not, it would have to be rewritten to always check if there's a token first. But if I get the token, it will initialize, correct? Meaning this would make ESI impossible if you were to only check if you're allowed to login.

@nicolas-grekas
Copy link
MemberAuthor

the web debug toolbar should force the display of authenticated status

that's impossible, by the very definition of what this feature achieves.

implementing a decoration for the actual_token_storage here

we need to access both the lazy and the actual storage services independently, so decoration cannot work here.

Does this mean that if my minimal authentication level is IS_AUTHENTICATED_ANONYMOUSLY, this option is useless?

yes, because this asks for an anonymous token.

Is this actually useful if you have even 1 isGranted anywhere?

as soon as you have one is_granted, the state is created, that's the purpose yes.

@EmmanuelVella
Copy link
Contributor

@nicolas-grekas 👍 won't the esi cache issue be fixed in the 3.4 branch ?

@nicolas-grekas
Copy link
MemberAuthor

There is no issue in the 3.4 branch finally: enabling the anonymous firewall asks for the session. You'd need to fix your config here.

@weaverryan
Copy link
Member

iltar: Does this mean that if my minimal authentication level is IS_AUTHENTICATED_ANONYMOUSLY, this option is useless?

nicolasgrekas yes, because this asks for an anonymous token.

I think this is a problem, becauseIS_AUTHENTICATED_ANONYMOUSLY can be used underaccess_control to basically mean "this URL requires NO security". But, we could fix this by adding some new way of doing this - e.g.roles: false under theaccess_control.

xavismeh: the web debug toolbar should force the display of authenticated status

nicolasgrekas: that's impossible, by the very definition of what this feature achieves.

This feature makes so much sense, that it should probably be on by default (and would, "effectively" be on by default thanks to the recipe update). But... this part IS a DX problem. Imagine if you're working on your authentication system, and itappears that it's not working, but really, it is.

IfContextListener never loaded the token, could we store the serialized token in the profiler data, and unserialize it there? This "laziness" should really be an invisible feature... and other than the profiler, I think it would be (e.g. because as soon as you try to "use" security, it would initialize itself).

@linaori
Copy link
Contributor

@weaverryan it feels hacky though, but I think the main problem is the separation of the access_control rules and the firewall config. If that would be merged, it might solve problems (more than just this one). I'm currently just not able to figure out a viable alternative for this.

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedJul 3, 2018
edited
Loading

IS_AUTHENTICATED_ANONYMOUSLY basically mean "this URL requires NO security"

as you know that's no totally accurate, hence this PR :)

e.g. roles: false

roles: [] - or just no "roles" entry at all.

could we store the serialized token in the profiler data, and unserialize it there?

nope we can't: the session is the storage and we don't have direct access to the backend.

I don't think showing the truth is against DX. Actually, trying to hide the truth is the not-DX thing to me.
So here, we could display something that tells what the situation is: security is enabled but has not been used so we don't know who is here, anonymous or not.

HeahDude reacted with thumbs up emoji

@stof
Copy link
Member

as you know that's no totally accurate, hence this PR :)

but currently, there is no way to have an access_control rule requiring no security at all. So we indeed need to add a new feature.

@stof
Copy link
Member

So here, we could display something that tells what the situation is: security is enabled but has not been used so we don't know who is here, anonymous or not.

if the profiler also displays whether authentication was attempted or no, it could indeed help.

@nicolas-grekas
Copy link
MemberAuthor

I found a better way, see#28089.

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedAug 31, 2018
edited
Loading

Reopening as there might be a reason to prefer this over#28089: here, the session lock is not created unless needed. But I would remove thelazy_authentication config entry and replace it withanonymous: lazy instead. See#28325

@nicolas-grekas
Copy link
MemberAuthor

Continued in#33676

@nicolas-grekasnicolas-grekas deleted the sec-lazy branchSeptember 23, 2019 19:46
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
@nicolas-grekasnicolas-grekas modified the milestones:next,4.4Oct 27, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@TaluuTaluuTaluu 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.

8 participants

@nicolas-grekas@xavismeh@linaori@EmmanuelVella@weaverryan@stof@Taluu@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp