-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] Removed anonymous in the new security system #36574
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
Conversation
I really like this change. The anonymous part of the authentication process was always troublesome to handle. Your changes in #30914 are generally much appreciated. |
@@ -72,12 +74,20 @@ public function authenticate(RequestEvent $event) | ||
$attributes = $request->attributes->get('_access_control_attributes'); | ||
$request->attributes->remove('_access_control_attributes'); | ||
|
||
if (!$attributes || ([AuthenticatedVoter::IS_AUTHENTICATED_ANONYMOUSLY] === $attributes && $event instanceof LazyResponseEvent)) { | ||
if (!$attributes || [AuthenticatedVoter::IS_AUTHENTICATED_ANONYMOUSLY] === $attributes) { |
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.
I don't understand the internals of this ... but are we going to keep this IS_AUTHENTICATED_ANONYMOUSLY
permission? I hope we can get rid of everything about "anonymous + authenticated".
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.
There DOES still need to be something like this, as it’s used when you are “whitelisting” paths in access_control. But it doesn’t need to have this name... though changing the name could be done later - that may simplify things.
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.
We indeed need an attribute to whitelist pages that are within the firewall, but accessible for unauthenticated users (e.g. the login page).
A couple suggestions after a chat about this:
PUBLIC_ACCESS
ALLOW_ACCESS_TO_ALL
NO_SECURITY
Do you, @javiereguiluz, or someone else favor one of these options (or another suggestion)?
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.
Also that we want to get rid of anonymous authentication the trigger word anonymous is still very good to describe an unprotected access. So another option could be ANONYMOUS_ACCESS in this context.
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.
Random idea ... add a exclude
option for access_control
entries:
access_control:
- { path: '^/admin/users', roles: ROLE_SUPER_ADMIN }
- { path: '^/admin', roles: ROLE_ADMIN, exclude: ['^/admin/login', '^/admin/public/*'] }
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.
Thanks for thinking outside the box @javiereguiluz! I'm not totally sure if this would make things much clearer, given the access control is already a list of patterns. Having a sub-list of excluded pattern for each pattern might make it more confusing to see which rule is used for which request. Maybe we have to completely revisit the access control configuration in another version of Symfony 😄
I've updated the PR to use PUBLIC_ACCESS
. Using anonymous is correct, it's maybe best to (at least for the next versions) avoid that term to remove confusion. Also, any logged in user is not accessing such a page "anonymously".
@wouterj thanks for this 🙇 Trying to explain that "anonymous users" are "authenticated" was one of the most WTF moments in Symfony training. It never made sense to anyone. So glad to get rid of this. |
src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AnonymousFactory.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Authenticator/RememberMeAuthenticator.php
Outdated
Show resolved
Hide resolved
29d2cd8
to
1679e6a
Compare
1679e6a
to
d8e5aff
Compare
This PR is now tested in the Symfony Demo application and works like charm 🎉 . I've updated the PR description with the current state. Ready for review & merge imho. I managed to break fabbot's exception message format and I'm unable to fix the error using concatenation 😢 The other build failures are also unrelated. |
* Anonymous users are actual to unauthenticated users, both are now represented by no token * Added a PUBLIC_ACCESS Security attribute to be used in access_control * Deprecated "anonymous: lazy" in favor of "lazy: true"
d8e5aff
to
ac84a6c
Compare
Thank you @wouterj. |
If token is not provided in request and no access control is defined for called route then firewall gets bypassed, but the expected behavior is to get an authentication exception with code of 401 |
Hello @kappa1389 and thank you for reaching out. This tracker is meant for tracking bugs and feature requests. If you are looking for support, you'll get faster responses by using one of our official support channels:
See https://symfony.com/support for more support options. |
Dear Alexander
Thank you for your reply i will take it into account
Best regards
…On Tuesday, August 3, 2021, Alexander M. Turek ***@***.***> wrote:
Hello @kappa1389 <https://github.com/kappa1389> and thank you for
reaching out.
This tracker is meant for tracking bugs and feature requests. If you are
looking for support, you'll get faster responses by using one of our
official support channels:
- StackOverflow: https://stackoverflow.com/questions/tagged/symfony
- Slack: https://symfony.com/slack
See https://symfony.com/support for more support options.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#36574 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVAFHYD6YCTTJZR3I4KH7XLT26Q3TANCNFSM4MQNG6OA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
The anonymous option for the firewall config was removed in symfony 5.1 as explained here: symfony/symfony#36574 Using the option caused an error but it can be skipped. "[A] user that is not authenticated is similar to an "unknown" user: They both have no rights at all." Resolves api-platform#1445
The anonymous option for the firewall config was removed in symfony 5.1 as explained here: symfony/symfony#36574 Using the option caused an error but it can be skipped. "[A] user that is not authenticated is similar to an "unknown" user: They both have no rights at all." Resolves #1445
This was one of the "Future considerations" of #33558:
This new experimental system is probably a once-in-a-lifetime change to make this change. @weaverryan and I have had some brainstorming about this. Some reasons why we think it makes 100% sense to do this change:
From a Security perspective, a user that is not authenticated is similar to an "unknown" user: They both have no rights at all.
The higher level consequences of the AnonymousToken are confusing and inconsistent:
is_authenticated()
expression language functionIS_AUTHENTICATED
security attribute being removed from [Security] Use new IS_* attributes in the expression language functions #35854, as there was no clear consensus on what its meaning should beSpring Security, which is where this originated from, makes Anonymous a very special case:
Symfony uses AnonymousToken much more than "just for convience in access-control attributes". Removing anonymous tokens allows us to move towards only allowing
UserInterface
users: [Security] deprecate non UserInterface users #34909Removing anonymous tokens do have an impact on
AccessListener
andAuthorizationChecker
. These currently throw an exception if there is no token in the storage, instead of treating them like "unknown users" (i.e. no roles). See #30609 on a RFC about removing this exception. We can also see e.g. the Twigis_granted()
function explicitly catching this exception.AccessListener
andAuthorizationChecker
BC, a flag has been added - default enabled - to throw an exception when no token is present (which is automatically disabled when the new system is used). In Symfony 5.4 (or whenever the new system is no longer experimental), we can deprecate this flag and in 6.0 we can never throw the exception anymore.anonymous: lazy
has been deprecated in favor of{ anonymous: true, lazy: true }
This fixes the dependency onAnonymousFactory
from theSecurityExtension
and allows removing theanonymous
option.PUBLIC_ACCESS
Security attribute as alternative ofIS_AUTHENTICATED_ANONYMOUSLY
. Both work in the new system, the latter only triggers a deprecation notice (but may be usefull to allow switching back and forth between old and new system).cc @javiereguiluz you might be interested, as I recently talked with you about this topic