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] Deprecate remaining anonymous checks#42510
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
Nyholm 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.
Thank you. Well done
After first review, It looks good. Just waiting for it to remove WIP.
src/Symfony/Component/Security/Core/Authentication/AuthenticationTrustResolverInterface.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
wouterj commentedAug 12, 2021
Thanks for the reviews@Nyholm!
The WIP is just because of the intentionally broken tests. Everything except |
chalasr commentedAug 13, 2021
The most common usage of firewalls:main:pattern:^/form_login:login_path:logincheck_path:loginaccess_control: -{ path: ^/login, roles: IS_AUTHENTICATED_ANONYMOUSLY } -{ path: ^/another_public_resource, roles: IS_AUTHENTICATED_ANONYMOUSLY } -{ path: ^/, roles: IS_AUTHENTICATED_FULLY } What is the new way to achieve that? How should one allow a single route to be accessed without being authenticated? |
fabpot commentedAug 13, 2021
#42423 has been merged now. |
d6de7d5 toab23e6fComparewouterj commentedAug 14, 2021
Good idea@chalasr. The great thing: this made me realize that I was suggesting a wrong replacement. I've added your example to the UPGRADE guide. Meanwhile, I also fixed the tests and updated the I think I have to disagree with fabbot here, their suggestion doesn't make sense: (I know we like to use the pixels at the right side of the screen, but I'm going to need a much much wider screen to be able to read this line 😄 ) diff -ru src/Symfony/Component/Security/Http/Tests/Firewall/ContextListenerTest.php src/Symfony/Component/Security/Http/Tests/Firewall/ContextListenerTest.php--- src/Symfony/Component/Security/Http/Tests/Firewall/ContextListenerTest.php2021-08-14 09:17:15.170647398 +0000+++ src/Symfony/Component/Security/Http/Tests/Firewall/ContextListenerTest.php2021-08-14 09:17:18.381551885 +0000@@ -416,11 +416,9 @@ $tokenStorage = new TokenStorage(); $usageIndex = $session->getUsageIndex();- $tokenStorage = new UsageTrackingTokenStorage($tokenStorage, new class(- (new \ReflectionClass(UsageTrackingTokenStorage::class))->hasMethod('getSession') ? [- 'request_stack' => function () use ($requestStack) {- return $requestStack;- }] : [+ $tokenStorage = new UsageTrackingTokenStorage($tokenStorage, new class((new \ReflectionClass(UsageTrackingTokenStorage::class))->hasMethod('getSession') ? ['request_stack' => function () use ($requestStack) {+ return $requestStack;+ }] : [ // BC for symfony/framework-bundle < 5.3 'session' => function () use ($session) { return $session; |
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.
👍
fabpot commentedAug 14, 2021
Thank you@wouterj. |
…:isAuthenticated()` non-virtual (chalasr)This PR was merged into the 6.0 branch.Discussion----------[Security] Make `AuthenticationTrustResolverInterface::isAuthenticated()` non-virtual| Q | A| ------------- | ---| Branch? | 6.0| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -The method has been added in#42510 (5.4) as a replacement for `isAnonymous()`.Commits-------32952ca [Security] Make `AuthenticationTrustResolverInterface::isAuthenticated()` non-virtual
Uh oh!
There was an error while loading.Please reload this page.
Deprecates the remaining checks for anonymous found in#41613. It's WIP because the tests are failing until#42423 is merged and this PR is rebased (didn't update one test to avoid merge conflicts).
Besides this, it also introduced
IS_AUTHENTICATEDandAuthenticationTrustResolver::isAutenticated(). Previously,IS_AUTHENTICATED_ANONYMOUSLYwas considered to be the "bottom type" for authenticated requests. As this is no longer true,IS_AUTHENTICATED_REMEMBERMEis now the new "bottom type". I suggest we use an explicit bottom type (the ones introduced) instead to avoid another such update if we change something with remember me. It's also more clear on the exact intent of the check.