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

wouterj · 2021-03-07T16:29:00Z

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.

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().

do the same for remember me's PersistentTokenInterface::getUsername()
also rename UserProviderInterface::loadUserByUsername() & UsernameNotFoundException
also rename UserLoaderInterface::loadUserByUsername()

I'm very much looking forward for feedback and help for this important, but BC-heavy, rename. 😃

chalasr

Looks good! Thank you for this, Wouter.

Given that DebugClassLoader will automatically trigger a deprecation notice thanks to the added @method phpdoc, I'm fine with the proposed BC layer and the additional notices it introduces.

Regarding tests, we should keep some legacy ones using the deprecated method to make sure everything keeps working.

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

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

src/Symfony/Component/Security/Core/Authentication/AuthenticationProviderManager.php

src/Symfony/Bridge/Monolog/Processor/AbstractTokenProcessor.php

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

src/Symfony/Component/Security/Core/Authentication/Token/TokenInterface.php

wouterj · 2021-03-07T17:51:06Z

Thanks Robin!

Given that DebugClassLoader will automatically trigger a deprecation notice thanks to the added @method phpdoc, I'm fine with the proposed BC layer and the additional notices it introduces.

Oh, that's nice. The powers of the DebugClassLoader keeps surprising me :)

Regarding tests, we should keep some legacy ones using the deprecated method to make sure everything keeps working.

Yes, sure thing. I've basically been find&replacing getUsername() occurrences in the tests (to see if this actually works). This PR needs lots more love before it's ready.

...ony/Bundle/SecurityBundle/Tests/Functional/Bundle/GuardedBundle/AuthenticationController.php

chalasr · 2021-03-09T09:35:16Z

I guess we will need to rename UserProviderInterface::loadUserByUsername() here

wouterj · 2021-03-11T22:49:08Z

src/Symfony/Component/Security/Core/Exception/UserNotFoundException.php

+     */
+    public function getMessageKey()
+    {
+        return 'Username could not be found.';


How would we consider BC on this public error message? I think we cannot change it?

No, it's not BC break. We can change it, but (not a blocker) that invalidates all translations (so, we need updates on translations as well).

that also break things if a project was customizing the translation messages in their own messages, as they won't be overriding the translation being used anymore.

After @stof's comment, I think it's best to keep the message unchanged.

Also, User identifier could not be found is a much to technical error message to have as a default user-friendly message imho. I think most applications that care already updated this to say e.g. "E-mailaddress could not be found", so "Username" seems a good default for the others.

wouterj · 2021-03-11T22:58:53Z

For reference, I consider this PR to be "feature complete". However, we do need to have more tests to make sure all authentication providers/listeners can work with both the old and new names. I'll add these over the next few days.

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

chalasr

Huge work!

src/Symfony/Component/Security/Core/Authentication/AuthenticationProviderManager.php

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

src/Symfony/Component/Security/Core/Authentication/Provider/DaoAuthenticationProvider.php

src/Symfony/Component/Security/Core/Authentication/Token/NullToken.php

src/Symfony/Component/Security/Core/Tests/Exception/UsernameNotFoundExceptionTest.php

src/Symfony/Component/Security/Http/EventListener/UserProviderListener.php

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

src/Symfony/Bridge/Monolog/Tests/Processor/SwitchUserTokenProcessorTest.php

wouterj · 2021-03-16T11:28:59Z

I think this PR is ready. However, 2 comments are left unresolved (open discussion), and the travis high builds are failing (I guess I need to submit another PR for 5.2 to change all tests to fix them?).

appveyor builds have a timeout, they were green before.

fabpot · 2021-03-16T18:13:40Z

The merge of the User rename makes a lot of conflicts here, sorry about that :(

ro0NL · 2021-03-16T18:56:07Z

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

@@ -26,6 +26,8 @@
 *
 * @see UserProviderInterface
 *
+ * @method string getUserIdentifier() returns the identifier for this user (e.g. its username or e-mailaddress)


getIdentifier? a user's ID and a user's user ID are the same thing IMHO.

i think only token needs to convey it's the user identifier

90% of the time (honestly, I think more), people are using entities as users. I don't want them to confuse getIdentifier() as getId(): your users don't login with a uuid, but with some other identifier (e-mailaddres). See also #40403 (review)

in that regard, i think getId vs getUserIdentifier is still as confusing

also AFAIK doctrine entities actually forwarding getIdentifier to getId would still be legit. (even while logging in by email)

fabpot · 2021-03-28T06:41:47Z

@wouterj Do you have time to rebase this PR?

wouterj · 2021-03-28T13:17:24Z

Rebased. I believe the deps=high builds will be fixed when #40609 is merged (other failures are unrelated). I'll rebase this PR again when that PR is merged up to 5.x.

…terface classes in the tests (wouterj) This PR was merged into the 5.2 branch. Discussion ---------- [Security] Use concrete UserInterface and UserProviderInterface classes in the tests | Q | A | ------------- | --- | Branch? | 5.2 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | n/a Fixes failing deps=high builds in #40403 Commits ------- 89d9de2 Use concrete user related classes in the tests

chalasr · 2021-03-28T17:39:57Z

(Hopefully last) rebase needed

wouterj · 2021-03-28T19:42:33Z

If we ignore psalm (which we should in this PR), we're finally green. Thanks for your help @chalasr!

chalasr · 2021-03-28T20:25:44Z

Thank you @wouterj.

…shlow) This PR was squashed before being merged into the 1.0-dev branch. Discussion ---------- [make:user] implement getUserIdentifier if required handle `getUsername()` -> `getUserIdentifier()` in Symfony 5.3 symfony/symfony#40403 Commits ------- 1d83924 [make:user] implement getUserIdentifier if required

VincentLanglet · 2021-06-02T07:41:04Z

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

-     *
-     * @return string The username
-     */
-    public function getUsername();


@wouterj @fabpot Removing a method from an interface isn't a BC-break ?

If I use $user->getUsername() in 5.2, I now can't be sure it still works in 5.3.
Also keeping the method would solve the psalm issues.

…of '{user_identifier}' in LDAP configuration (EXT - THERAGE Kevin) This PR was merged into the 6.2 branch. Discussion ---------- [Ldap] Deprecate '{username}' parameter use in favour of '{user_identifier}' in LDAP configuration | Q | A | ------------- | --- | Branch? | 6.2 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | This PR aims to fix a missing forward compatibility as done in src/Symfony/Component/Ldap/Security/LdapUserProvider.php at line 80 when authenticating with LDAP and using the following configuration : ```yaml security: // ... firewalls:œ // ... main: form_login_ldap: // ... query_string: '(whatever={user_identifier})' dn_string: 'uid={user_identifier},dc=example,dc=com' ``` instead of : ```yaml security: // ... firewalls: // ... main: form_login_ldap: // ... query_string: '(whatever={username})' dn_string: 'uid={username},dc=example,dc=com' ``` maybe related to : #40403 Commits ------- 6dd1221 [Ldap] Deprecate '{username}' parameter use in favour of '{user_identifier}' in LDAP configuration

wouterj requested a review from chalasr as a code owner March 7, 2021 16:29

carsonbot added Status: Needs Review Deprecation Feature Security labels Mar 7, 2021

carsonbot changed the title ~~[WIP/POC][Security] Rename UserInterface::getUsername() to getUserIdentifier()~~ [Security] [WIP/POC] Rename UserInterface::getUsername() to getUserIdentifier() Mar 7, 2021

chalasr reviewed Mar 7, 2021

View reviewed changes

wouterj mentioned this pull request Mar 7, 2021

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

Closed

OskarStark reviewed Mar 7, 2021

View reviewed changes

...ony/Bundle/SecurityBundle/Tests/Functional/Bundle/GuardedBundle/AuthenticationController.php Show resolved Hide resolved

nicolas-grekas added this to the 5.x milestone Mar 9, 2021

wouterj force-pushed the security-username-identifier branch from e800acc to 1c7b338 Compare March 11, 2021 20:49

wouterj commented Mar 11, 2021

View reviewed changes

wouterj force-pushed the security-username-identifier branch from 8a13540 to cc659eb Compare March 11, 2021 22:49

noniagriconomie reviewed Mar 12, 2021

View reviewed changes

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

chalasr reviewed Mar 12, 2021

View reviewed changes

chalasr reviewed Mar 14, 2021

View reviewed changes

src/Symfony/Bridge/Monolog/Tests/Processor/SwitchUserTokenProcessorTest.php Outdated Show resolved Hide resolved

wouterj force-pushed the security-username-identifier branch 6 times, most recently from a0a9bfe to 279f410 Compare March 15, 2021 22:46

wouterj changed the title ~~[Security] [WIP/POC] Rename UserInterface::getUsername() to getUserIdentifier()~~ [Security] Rename UserInterface::getUsername() to getUserIdentifier() Mar 15, 2021

wouterj force-pushed the security-username-identifier branch from 279f410 to e3c8b30 Compare March 15, 2021 23:13

ro0NL reviewed Mar 16, 2021

View reviewed changes

wouterj force-pushed the security-username-identifier branch from e3c8b30 to 835cb92 Compare March 28, 2021 11:26

wouterj mentioned this pull request Mar 28, 2021

[Security] Use concrete UserInterface and UserProviderInterface classes in the tests #40609

Merged

wouterj force-pushed the security-username-identifier branch from 835cb92 to 08dcf39 Compare March 28, 2021 16:52

wouterj force-pushed the security-username-identifier branch from 08dcf39 to 33bee77 Compare March 28, 2021 19:29

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

8afd7a3

chalasr force-pushed the security-username-identifier branch from 33bee77 to 8afd7a3 Compare March 28, 2021 20:25

chalasr merged commit 40a2c19 into symfony:5.x Mar 28, 2021

wouterj deleted the security-username-identifier branch March 28, 2021 20:35

chalasr mentioned this pull request Mar 28, 2021

[Security] Rename UserInterface::getUsername() to getUserIdentifier() symfony/symfony-docs#15164

Closed

fabpot mentioned this pull request Apr 18, 2021

Release v5.3.0-BETA1 #40853

Merged

jrushlow mentioned this pull request Apr 19, 2021

[make:user] implement getUserIdentifier if required symfony/maker-bundle#862

Merged

JKapitein mentioned this pull request May 31, 2021

A critical interface method was just removed in 5.3.0 #41470

Closed

Jean85 mentioned this pull request Jun 1, 2021

Fix static analysis due to Symfony 5.3 release getsentry/sentry-symfony#518

Merged

VincentLanglet reviewed Jun 2, 2021

View reviewed changes

This was referenced Jun 15, 2022

[Ldap] Fixing missing 'user_identifier' forward compatibility in CheckLdapCredentialsListener #46681

Closed

[Ldap] Deprecate '{username}' parameter use in favour of '{user_identifier}' in LDAP configuration #46683

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wouterj commented Mar 7, 2021 •

edited

chalasr left a comment

wouterj commented Mar 7, 2021

chalasr commented Mar 9, 2021

wouterj Mar 11, 2021

fabpot Mar 12, 2021

stof Mar 12, 2021

wouterj Mar 15, 2021

wouterj commented Mar 11, 2021

chalasr left a comment

wouterj commented Mar 16, 2021

fabpot commented Mar 16, 2021

ro0NL Mar 16, 2021 •

edited

wouterj Mar 16, 2021

ro0NL Mar 16, 2021

ro0NL Mar 16, 2021

fabpot commented Mar 28, 2021

wouterj commented Mar 28, 2021 •

edited

chalasr commented Mar 28, 2021

wouterj commented Mar 28, 2021

chalasr commented Mar 28, 2021

VincentLanglet Jun 2, 2021

wouterj Jun 2, 2021

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

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

Conversation

wouterj commented Mar 7, 2021 • edited

chalasr left a comment

Choose a reason for hiding this comment

wouterj commented Mar 7, 2021

chalasr commented Mar 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Mar 11, 2021

chalasr left a comment

Choose a reason for hiding this comment

wouterj commented Mar 16, 2021

fabpot commented Mar 16, 2021

ro0NL Mar 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Mar 28, 2021

wouterj commented Mar 28, 2021 • edited

chalasr commented Mar 28, 2021

wouterj commented Mar 28, 2021

chalasr commented Mar 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Mar 7, 2021 •

edited

ro0NL Mar 16, 2021 •

edited

wouterj commented Mar 28, 2021 •

edited