[Security] Decouple passwords from UserInterface #40267

chalasr · 2021-02-21T12:06:24Z

Q	A
Branch?	5.x
Bug fix?	no
New feature?	yes
Deprecations?	yes
Tickets	#23081, helps with #39308
License	MIT
Doc PR	todo

This PR addresses a long-standing issue of the Security component: UserInterface is coupled to passwords.
It does it by moving the getPassword() method from UserInterface to a PasswordAuthenticatedUserInterface, and the getSalt() method to a LegacyPasswordAuthenticatedUserInterface.

Steps:

In 5.3, we add the new interface and, at places where password-based authentication happens, trigger deprecation notices when a UserInterface object does not implement the new interface(s). The UserInterface is kept as-is until 6.0.
In 6.0, we can remove the methods from UserInterface as well as support for using password authentication with user objects not implementing the new interface(s).

As a side-effect, some password-related interfaces (UserPasswordHasherInterface and PasswordUpgraderInterface) must change their signatures to type-hint against the new interface.
That is done in a BC way, which is to make the concerned methods virtual until 6.0, with deprecation notices triggered from callers and concrete implementations.

Benefits:
In 6.0, applications that use password-less authentication (e.g. login links) won't need to write no-op getPassword() and getSalt() in order to fulfil the UserInterface contract.

For applications that do use password-based authentication, they will need to opt-in explicitly by implementing the relevant interface(s).

This build on great discussions with @wouterj and @nicolas-grekas, and it is part of the overall rework of the Security component.

chalasr · 2021-02-21T13:08:37Z

Build failure with high-deps expected (will be fixed once merged).

derrabus

Thank you for working on this feature!

src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php

src/Symfony/Component/Ldap/Security/LdapUserProvider.php

src/Symfony/Component/PasswordHasher/Hasher/UserPasswordHasher.php

src/Symfony/Component/PasswordHasher/Tests/Hasher/UserPasswordHasherTest.php

wouterj

Looking great. It doesn't look like it causes much of change in the internals, which is great :). Just some small comments found while looking at the diff. I'll find some time this week to test it in a real project.

I think we should also add this to the guard. This is the most tricky, but I think we can trigger the deprecation in GuardAuthenticationProvider and GuardBridgeAuthenticator if we check for $guard instanceof PasswordAuthenticatedInterface && !$user instanceof PasswordAuthenticatedUserInterface (or maybe directly test whether the user's getPassword or getSalt don't return null?).

src/Symfony/Component/Security/Core/User/UserInterface.php

chalasr · 2021-02-21T23:39:41Z

I think we can trigger the deprecation in GuardAuthenticationProvider and GuardBridgeAuthenticator if we check for $guard instanceof PasswordAuthenticatedInterface && !$user instanceof PasswordAuthenticatedUserInterface

Good idea. Done, thank you.

Note that UserPasswordHasher already triggers a deprecation notice when getPassword() is defined but the new interface is not implemented (and another for getSalt()). GuardAuthenticationProvider uses it for password upgrades, and userland authenticators created via make:auth also use it for password verification.
Guard is now well covered I guess :)

chalasr · 2021-03-05T09:41:00Z

Added one compile-time deprecation to ease upgrading. Fixed psalm reports also.
This is ready.

fabpot · 2021-03-06T11:00:16Z

Thank you @chalasr.

…oic425) This PR was merged into the 1.10-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | 1.7, 1.8 or master  | Bug fix? | no/yes | New feature? | no/yes | BC breaks? | no/yes | Deprecations? | no/yes  | Related tickets | fixes #X, partially #Y, mentioned in #Z | License | MIT  When using a SSO such as Keycloak, you don't need a password. It's better to allow null instead of setting plain password with a random value. Moreover, password will be decoupled from user interface on Symfony security user. symfony/symfony#40267 Commits ------- e20f871 Allow user password to be null 9daecf6 Move migration into core bundle

…oic425) This PR was merged into the 1.10-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | 1.7, 1.8 or master  | Bug fix? | no/yes | New feature? | no/yes | BC breaks? | no/yes | Deprecations? | no/yes  | Related tickets | fixes #X, partially #Y, mentioned in #Z | License | MIT  When using a SSO such as Keycloak, you don't need a password. It's better to allow null instead of setting plain password with a random value. Moreover, password will be decoupled from user interface on Symfony security user. symfony/symfony#40267 Commits ------- e20f871d9a49c6028ba7c1cd3708f7986ebe32aa Allow user password to be null 9daecf696b3c489dbe56f7ba26b162527a7948c9 Move migration into core bundle

@chalasr

…serIdentifier() (wouterj) This PR was squashed before being merged into the 5.3-dev branch. Discussion ---------- [Security] Rename UserInterface::getUsername() to getUserIdentifier() | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | yes | Deprecations? | yes | Tickets | Fix part of #39308 | License | MIT | Doc PR | tbd This continues the great work by @chalasr in #40267 and rounds up the `UserInterface` cleanup. The `getUsername()` has been a point of confusion for many years. In today's applications, many domains no longer have a username, but instead rely on a user's email address or the like. Even more, this username has to be unique for all Security functionality to work correctly - so it's more confusing in complex applications relying on e.g. "username+company" to be unique. **This PR proposes to rename the method to `getUserIdentifier()`**, to more clearly indicate the goal of the method (note: I'm open for any other suggestion). For BC, the `getUsername()` method is deprecated in 5.3 and `getUserIdentifier()` will be added to the `UserInterface` in 6.0. PHPdocs are used to improve type-hinting for 5.3 & 5.4. Both authentication managers (the legacy and experimental one) check the authenticated user object and trigger a deprecation if `getUserIdentifier()` is not implemented. * [x] **judge whether we need to have better deprecations for calling `getUsername()` in the application code.** Consistent with these changes, I've also done the same BC change for `TokenInterface::getUsername()`. * [x] **do the same for remember me's `PersistentTokenInterface::getUsername()`** * [x] **also rename `UserProviderInterface::loadUserByUsername()` & `UsernameNotFoundException`** * [x] **also rename `UserLoaderInterface::loadUserByUsername()`** I'm very much looking forward for feedback and help for this important, but BC-heavy, rename. 😃 Commits ------- 8afd7a3 [Security] Rename UserInterface::getUsername() to getUserIdentifier()

@chalasr

…dUserInterface (jrushlow) This PR was squashed before being merged into the 1.0-dev branch. Discussion ---------- [make:user] user entities implement PasswordAuthenticatedUserInterface Starting in Symfony 5.3 - PR symfony/symfony#40267 introduces the new `PasswordAuthenticatedUserInterface` ~& `LegacyPasswordAuthenticatedUserInterface`~ aimed at decoupling passwords from the `UserInterface`. - Generated `User` entity implements `PasswordAuthenticatedUserInterface` if it exists. - Adds correct type hint to the generated `UserRepository::upgradePassword()` method. Authentication related changes are handled in #824 // cc @chalasr Commits ------- f9d9d03 [make:user] user entities implement PasswordAuthenticatedUserInterface

…rInterface (chalasr) This PR was merged into the 6.0 branch. Discussion ---------- [Security] Remove getPassword() and getSalt() from UserInterface | Q | A | ------------- | --- | Branch? | 6.0 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - with BC layers from #40267 Commits ------- 30e2c00 [Security] Remove getPassword() and getSalt() from UserInterface

chalasr added the Security label Feb 21, 2021

chalasr added this to the 5.x milestone Feb 21, 2021

chalasr requested a review from wouterj as a code owner February 21, 2021 12:06

carsonbot added Status: Needs Review Deprecation Feature labels Feb 21, 2021

chalasr force-pushed the passwordless-user branch 3 times, most recently from f46b6eb to fa3f599 Compare February 21, 2021 12:36

This was referenced Feb 21, 2021

[Security] Decoupling password from UserInterface #23081

Closed

[Security] The road to a stable authenticator-based authentication #39308

Closed

chalasr force-pushed the passwordless-user branch 3 times, most recently from 4063d16 to c4a7af5 Compare February 21, 2021 13:02

chalasr force-pushed the passwordless-user branch 3 times, most recently from fbdf20c to 08e04e6 Compare February 21, 2021 13:24

derrabus requested changes Feb 21, 2021

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 21, 2021

chalasr force-pushed the passwordless-user branch from 08e04e6 to c2a72bd Compare February 21, 2021 13:38

chalasr added Status: Needs Review and removed Status: Needs Work labels Feb 21, 2021

chalasr force-pushed the passwordless-user branch from c2a72bd to 74330bc Compare February 21, 2021 14:07

wouterj reviewed Feb 21, 2021

View reviewed changes

src/Symfony/Component/Security/Core/User/UserInterface.php Show resolved Hide resolved

src/Symfony/Component/Security/Core/User/UserInterface.php Show resolved Hide resolved

chalasr force-pushed the passwordless-user branch 2 times, most recently from 8716067 to 1054ba7 Compare February 21, 2021 23:17

chalasr force-pushed the passwordless-user branch 5 times, most recently from 770e25b to aacfbd0 Compare March 5, 2021 09:23

chalasr added the Ready label Mar 5, 2021

chalasr force-pushed the passwordless-user branch 3 times, most recently from 5066319 to 8e1a5d6 Compare March 5, 2021 23:32

[Security] Decouple passwords from UserInterface

2764225

chalasr force-pushed the passwordless-user branch from 8e1a5d6 to 2764225 Compare March 5, 2021 23:37

fabpot approved these changes Mar 6, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Mar 6, 2021

fabpot mentioned this pull request Mar 6, 2021

[Security] Decouple passwords from UserInterface symfony/symfony-docs#15065

Closed

fabpot merged commit 8de664d into symfony:5.x Mar 6, 2021

chalasr deleted the passwordless-user branch March 6, 2021 11:16

This was referenced Mar 7, 2021

[Security] Rename UserInterface::getUsername() to getUserIdentifier() #40403

Merged

[RFC] Symfony Security rework tracking issue #30914

Closed

loic425 mentioned this pull request Mar 11, 2021

[UserBundle] Allow user password to be null for SSO Sylius/Sylius#12441

Merged

jrushlow mentioned this pull request Mar 24, 2021

[make:user] user entities implement PasswordAuthenticatedUserInterface symfony/maker-bundle#849

Merged

fabpot mentioned this pull request Apr 18, 2021

Release v5.3.0-BETA1 #40853

Merged

chalasr mentioned this pull request Jul 4, 2021

[Security] Remove getPassword() and getSalt() from UserInterface #41982

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Decouple passwords from UserInterface #40267

[Security] Decouple passwords from UserInterface #40267

chalasr commented Feb 21, 2021 •

edited

chalasr commented Feb 21, 2021

derrabus left a comment

wouterj left a comment

chalasr commented Feb 21, 2021 •

edited

chalasr commented Mar 5, 2021

fabpot commented Mar 6, 2021

[Security] Decouple passwords from UserInterface #40267

[Security] Decouple passwords from UserInterface #40267

Conversation

chalasr commented Feb 21, 2021 • edited

chalasr commented Feb 21, 2021

derrabus left a comment

Choose a reason for hiding this comment

wouterj left a comment

Choose a reason for hiding this comment

chalasr commented Feb 21, 2021 • edited

chalasr commented Mar 5, 2021

fabpot commented Mar 6, 2021

chalasr commented Feb 21, 2021 •

edited

chalasr commented Feb 21, 2021 •

edited