8000 [Security] Add generic to UserProviderInterface by VincentLanglet · Pull Request #50399 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Add generic to UserProviderInterface #50399

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 3, 2023
Merged

[Security] Add generic to UserProviderInterface #50399

merged 1 commit into from
Aug 3, 2023

Conversation

VincentLanglet
Copy link
Contributor
@VincentLanglet VincentLanglet commented May 23, 2023 •
edited by stof
Loading
Q A
Branch? 6.4
Bug fix? no
New feature? yes (but phpdoc only)
Deprecations? no
Tickets Fix #...
License MIT
Doc PR symfony/symfony-docs#...

Similar to #48750 @nicolas-grekas

@carsonbot carsonbot added this to the 6.3 milestone May 23, 2023
stof
stof requested changes May 23, 2023
View reviewed changes
Copy link
Member
@stof stof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add the proper @template-implements on implementations.

@stof
Copy link
Member
stof commented May 23, 2023

This will be for 6.4, not for 6.3 (we are 2 months after the feature freeze)

stof
stof requested changes May 23, 2023
View reviewed changes
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@VincentLanglet VincentLanglet requested a review from stof May 23, 2023 14:19
@VincentLanglet
Copy link
Contributor Author

Hi @stof, is it ok for you now ?

derrabus
derrabus reviewed Jul 27, 2023
View reviewed changes
@VincentLanglet VincentLanglet force-pushed the generic-user branch 2 times, most recently from 42476d4 to 7ee64db Compare July 27, 2023 15:32
wouterj
wouterj reviewed Aug 1, 2023
View reviewed changes
@carsonbot carsonbot changed the title Add generic to UserProviderInterface [Security] Add generic to UserProviderInterface Aug 3, 2023
@nicolas-grekas
Copy link
Member

Thank you @VincentLanglet.

@nicolas-grekas nicolas-grekas merged commit cf3f52a into symfony:6.4 Aug 3, 2023
@wouterj wouterj mentioned this pull request Aug 5, 2023
Merged
fabpot added a commit that referenced this pull request Aug 5, 2023
@fabpot
bug #51283 [Security] Do not make PasswordUpgraderInterface a generic…
34f46c6 
… (wouterj)

This PR was merged into the 6.4 branch.

Discussion
----------

[Security] Do not make PasswordUpgraderInterface a generic

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Ref #50399 (comment)
| License       | MIT
| Doc PR        | -

Making `PasswordUpgraderInterface` a generic in 6.3 (#48750) was a mistake. Nothing guarantees that the calling side will only pass `TUser` into the `upgradePassword()` method, as there is no way to check which users are supported. Passing a potentially unsupported user is expected behavior and we document in the PHPdoc that the method should silently fail in such cases. The referenced GitHub discussion contains some more details.

Removing the generic from the interface creates a static analysis failure in both [Psalm](https://psalm.dev/r/c86a59ab67) and [PHPstan](https://phpstan.org/r/3c85447a-533c-4196-b0c8-730687c497cc). For this reason, I selected the 6.4 branch although this is a bug fix for 6.3. I'm fine with merging this in 6.3 as well, if this feels better.

Commits
-------

d473b90 [Security] Do not make PasswordUpgraderInterface a generic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@wouterj wouterj wouterj left review comments

@derrabus derrabus derrabus left review comments

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

@stof stof Awaiting requested review from stof

Assignees
No one assigned
Labels
Feature Security Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
6 participants
@VincentLanglet @stof @nicolas-grekas @wouterj @derrabus @carsonbot
0