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 AnonymousToken, non-UserInterface users, and token credentials#42423
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
be5dd61 tof1b28f4Comparef1b28f4 to8cced53Comparewouterj commentedAug 8, 2021
Psalm disliking the old signatures in the legacy integrations/tests is to be expected, this PR is ready imho |
8cced53 tob451883Comparesrc/Symfony/Bridge/Monolog/Tests/Processor/SwitchUserTokenProcessorTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
b451883 to8554c93Compare
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.
Thanks! Just minor comments.
The CHANGELOG and UPGRADE files need to be updated also
src/Symfony/Component/Security/Core/Authentication/Token/PreAuthenticatedToken.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Security/Core/Authentication/Token/RememberMeToken.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Security/Core/Authentication/Token/TokenInterface.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
7cc109b toba70257Compare
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.
I just have a few questions/comments.
Can you also confirm that all the psalm errors on unmodified files are false positives?
| $originalToken =newUsernamePasswordToken(newInMemoryUser('original_user','password', ['ROLE_SUPER_ADMIN']),'provider', ['ROLE_SUPER_ADMIN']); | ||
| $switchUserToken =newSwitchUserToken(newInMemoryUser('user','passsword', ['ROLE_USER']),'provider', ['ROLE_USER'],$originalToken); | ||
| }else { | ||
| $originalToken =newUsernamePasswordToken(newUser('original_user','password', ['ROLE_SUPER_ADMIN']),null,'provider', ['ROLE_SUPER_ADMIN']); |
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.
Maybe make a comment here so it is more easily found when we remove code in Symfony 6.0.
(or you maybe have added this code to your list of future PRs)
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Security/Core/Authentication/Token/SwitchUserToken.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| $user =$token->getUser(); | ||
| // in case it's not an object we cannot do anything with it; E.g. "anon." | ||
| // @deprecated since 5.4 |
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.
| // @deprecated since 5.4 | |
| // @deprecated since 5.4, $user will always be an UserInterface in 6.0 |
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.
| // @deprecated since 5.4 | |
| // @deprecated since 5.4 , $user will always be a UserInterface in 6.0 |
:)
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.
Yes. You are correct.
ba70257 to44b843aComparefabpot commentedAug 13, 2021
Thank you@wouterj. |
This PR was merged into the 5.4 branch.Discussion----------[Security] Deprecate remaining anonymous checks| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | no| New feature? | yes| Deprecations? | yes| Tickets | Ref#41613| License | MIT| Doc PR | tbdDeprecates 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_AUTHENTICATED` and `AuthenticationTrustResolver::isAutenticated()`. Previously, `IS_AUTHENTICATED_ANONYMOUSLY` was considered to be the "bottom type" for authenticated requests. As this is no longer true, `IS_AUTHENTICATED_REMEMBERME` is 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.Commits-------e3aca7f [Security] Deprecate remaining anonymous checks
| * @param string[] $roles | ||
| */ | ||
| publicfunction__construct($user,$credentials,string$firewallName,array$roles = []) | ||
| publicfunction__construct($user,/*string*/$firewallName,/*array*/$roles = []) |
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.
Hi guys, I'm not sure how to useUsernamePasswordToken without$credentials as this change is not explained in documentation:https://symfony.com/doc/current/components/security/authentication.html
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.
You probably get faster answers if you use one of the support channels:https://symfony.com/support (e.g. GitHub discussions) Not a lot of people are watching already merged PRs.
The security component is refactored during Symfony 5. Tokens now only represent authenticated tokens/sessions, so they no longer have credentials. The "pre authentication tokens" are now called passports.
We haven't updated the components docs yet, but the framework docs are upgraded to the new system. Hopefully, you'll be able to figure stuff out using e.g.https://symfony.com/doc/current/security/custom_authenticator.html
Uh oh!
There was an error while loading.Please reload this page.
This is a continuation of@xabbuh's experiment in#34909 and@chalasr's work in#42050. This hopefully is the last cleanup of
TokenInterface:string|\Stringableunion fromToken::getUser()and other helper methods and require a user to be an instance ofUserInterface.Token::eraseCredentials()as this is still used to remove credentials fromUserInterface.AnonymousToken, which we forgot in 5.3. This token is not used anymore in the new system (anonymous does no longer exists). This was also the only token in core that didn't fulfill theUserInterfacerequirement for authenticated tokens.