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] 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 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:masterfromwouterj:security/remove-anonymous
May 3, 2020

Conversation

wouterj
Copy link
Member

@wouterjwouterj commentedApr 24, 2020
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?no
Tickets-
LicenseMIT
Doc PRtbd

This was one of the "Future considerations" of#33558:

Drop the AnonymousToken and AnonymousAuthenticator: Anonymous authentication has never made much sense and complicates things (e.g. the user can be a string). For access control, an anonymous user has the same meaning as an un-authenticated one (null). This require changes in the AccessListener and AuthorizationChecker and probably also a new Security attribute (to replace IS_AUTHENTICATED_ANONYMOUSLY). Related issues:#34909,#30609

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:

  • Spring Security, which is where this originated from, makes Anonymous a very special case:

    Finally, there is an AnonymousAuthenticationFilter, which is chained after the normal authentication mechanisms and automatically adds an AnonymousAuthenticationToken to the SecurityContextHolder if there is no existing Authentication held there.

    Note that there is no real conceptual difference between a user who is “anonymously authenticated” and an unauthenticated user. Spring Security's anonymous authentication just gives you a more convenient way to configure your access-control attributes. Calls to servlet API calls such as getCallerPrincipal, for example, will still return null even though there is actually an anonymous authentication object in the SecurityContextHolder.

  • Symfony uses AnonymousToken much more than "just for convience in access-control attributes".Removing anonymous tokens allows us to move towards only allowingUserInterface users:[Security] deprecate non UserInterface users #34909


Removing anonymous tokens do have an impact onAccessListener 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. theTwigis_granted() function explicitly catching this exception.

  • To make the changes inAccessListener 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.
  • IntroducedPUBLIC_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

fbourigault, keichinger, apfelbox, sstok, andreybolonin, kissifrot, bigfoot90, sukei, tyx, yceruto, and romaricdrigon reacted with heart emojilinaori, keichinger, apfelbox, sstok, and andreybolonin reacted with rocket emoji
@simonberger
Copy link
Contributor

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

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 thisIS_AUTHENTICATED_ANONYMOUSLYpermission? I hope we can get rid of everything about "anonymous + authenticated".

Copy link
Member

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.

Copy link
MemberAuthor

@wouterjwouterjApr 28, 2020
edited
Loading

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:

  1. PUBLIC_ACCESS
  2. ALLOW_ACCESS_TO_ALL
  3. NO_SECURITY

Do you,@javiereguiluz, or someone else favor one of these options (or another suggestion)?

Copy link
Contributor

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.

sstok reacted with eyes emoji

Choose a reason for hiding this comment

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

Random idea ... add aexclude option foraccess_control entries:

access_control:    -{ path: '^/admin/users', roles: ROLE_SUPER_ADMIN }    -{ path: '^/admin', roles: ROLE_ADMIN, exclude: ['^/admin/login', '^/admin/public/*'] }

Copy link
MemberAuthor

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 usePUBLIC_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".

@javiereguiluz
Copy link
Member

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

@nicolas-grekasnicolas-grekas added this to thenext milestoneApr 26, 2020
@wouterjwouterjforce-pushed thesecurity/remove-anonymous branch 2 times, most recently from29d2cd8 to1679e6aCompareMay 2, 2020 13:44
@wouterjwouterj marked this pull request as ready for reviewMay 2, 2020 13:47
@wouterjwouterj changed the title[Security][POC] Removed anonymous in the new security system[Security] Removed anonymous in the new security systemMay 2, 2020
@wouterjwouterjforce-pushed thesecurity/remove-anonymous branch from1679e6a tod8e5affCompareMay 2, 2020 15:56
@wouterj
Copy link
MemberAuthor

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"
@fabpotfabpotforce-pushed thesecurity/remove-anonymous branch fromd8e5aff toac84a6cCompareMay 3, 2020 06:43
@fabpot
Copy link
Member

Thank you@wouterj.

@kappa1389
Copy link

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
I face this problem with new symfony security integrated with lexik
Any ideas?

@derrabus
Copy link
Member

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:

Seehttps://symfony.com/support for more support options.

@kappa1389
Copy link

kappa1389 commentedAug 4, 2021 via email

Dear AlexanderThank you for your reply i will take it into accountBest 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 Seehttps://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> .

wiese added a commit to wiese/docs that referenced this pull requestOct 20, 2021
The anonymous option for the firewall config was removed in symfony 5.1as explained here:symfony/symfony#36574Using the option caused an error but it can be skipped. "[A] user thatis not authenticated is similar to an "unknown" user: They both have norights at all."Resolvesapi-platform#1445
alanpoulain pushed a commit to api-platform/docs that referenced this pull requestOct 20, 2021
The anonymous option for the firewall config was removed in symfony 5.1as explained here:symfony/symfony#36574Using the option caused an error but it can be skipped. "[A] user thatis not authenticated is similar to an "unknown" user: They both have norights at all."Resolves#1445
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz left review comments

@weaverryanweaverryanweaverryan left review comments

@simonbergersimonbergersimonberger left review comments

@fabpotfabpotfabpot approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

9 participants
@wouterj@simonberger@javiereguiluz@fabpot@kappa1389@derrabus@weaverryan@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp