Skip to content

[Security] Deprecate empty user identifier #58007

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

ajgarlag
Copy link
Contributor

Q A
Branch? 7.2
Bug fix? no
New feature? no
Deprecations? yes
Issues Fix #57982
License MIT

@carsonbot

This comment was marked as resolved.

@ajgarlag ajgarlag marked this pull request as ready for review August 14, 2024 07:02
@ajgarlag ajgarlag requested a review from chalasr as a code owner August 14, 2024 07:02
@carsonbot carsonbot added this to the 7.2 milestone Aug 14, 2024
@ajgarlag ajgarlag force-pushed the deprecate-empty-user-identifier branch 3 times, most recently from 4fd33c8 to 06ed6e6 Compare August 14, 2024 07:31
@carsonbot carsonbot changed the title Deprecate empty user identifier [Security] Deprecate empty user identifier Aug 14, 2024
@ajgarlag ajgarlag force-pushed the deprecate-empty-user-identifier branch 2 times, most recently from 38e3e37 to f468116 Compare August 14, 2024 12:31
@chalasr
Copy link
Member

chalasr commented Aug 14, 2024

Can you also update the Security/Http's CHANGELOG file? The same wording as in the UPGRADE file should work

@ajgarlag
Copy link
Contributor Author

Can you also update the Security/Http's CHANGELOG file? The same wording as in the UPGRADE file should work

Should the phpdoc addition to UserInterface be documented in any UPGRADE or CHANGELOG file?

@ajgarlag ajgarlag force-pushed the deprecate-empty-user-identifier branch from f468116 to 6dabe28 Compare August 14, 2024 12:48
@chalasr
Copy link
Member

chalasr commented Aug 14, 2024

Please yes in Security\Core

@ajgarlag ajgarlag force-pushed the deprecate-empty-user-identifier branch from 6dabe28 to a3ec916 Compare August 14, 2024 20:14
@ajgarlag ajgarlag requested a review from chalasr August 14, 2024 20:15
@ajgarlag ajgarlag force-pushed the deprecate-empty-user-identifier branch 2 times, most recently from 959dbbf to 7987bc6 Compare August 17, 2024 14:45
@ajgarlag ajgarlag force-pushed the deprecate-empty-user-identifier branch from 7987bc6 to e24f8c8 Compare August 19, 2024 07:53
@ajgarlag ajgarlag force-pushed the deprecate-empty-user-identifier branch from e24f8c8 to 63690ec Compare August 19, 2024 08:23
@fabpot
Copy link
Member

fabpot commented Aug 19, 2024

Thank you @ajgarlag.

@fabpot fabpot merged commit c4da8e6 into symfony:7.2 Aug 19, 2024
8 of 10 checks passed
@ajgarlag
Copy link
Contributor Author

Thank you all for your comments and support!

@VincentLanglet
Copy link
Contributor

Hi, I wonder if this PR is only half-done @nicolas-grekas @chalasr @ajgarlag.

What's expected for UserProviderInterface::loadUserByIdentifier ?

  • Should the param be updated to non-empty-string ?
  • Should it just throw a UserNotFoundException when called with '' ?
  • Should we introduce a new exception InvalidIdentifierException ?

And it seems like there is some other method with a $identifier parameter...

@chalasr
Copy link
Member

chalasr commented Jun 4, 2025

@VincentLanglet This is enforced at the authenticator level so I think the current behavior is fine functionally speaking. Making user providers throw a specific exception when passed an empty string would be another feature to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants