Skip to content

[Security] Rename User to InMemoryUser #40443

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
fabpot merged 1 commit into from
Mar 16, 2021
Merged

[Security] Rename User to InMemoryUser #40443

fabpot merged 1 commit into from
Mar 16, 2021

Conversation

@chalasr
Copy link
Member

@chalasr chalasr commented Mar 10, 2021

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? yes
Tickets Closes #26348
License MIT
Doc PR -

This PR aims to clarify that the User class should only be used by the InMemoryUserProvider, as documented:

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

Lines 15 to 17 in c06a76c

* User is the user implementation used by the in-memory user provider.
*
* This should not be used for anything else.

It also renames UserChecker to InMemoryUserChecker because it only works with the in-memory user class:

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

Lines 31 to 32 in c06a76c

if (!$user instanceof User) {
return;

@chalasr chalasr requested a review from wouterj as a code owner March 10, 2021 21:37
@chalasr chalasr force-pushed the inmemory-user branch 5 times, most recently from 3614510 to 5228300 Compare March 11, 2021 11:20
@chalasr chalasr added this to the 5.x milestone Mar 11, 2021
@chalasr chalasr force-pushed the inmemory-user branch 2 times, most recently from 56b2a0c to 8773b6b Compare March 11, 2021 11:41
@chalasr
Copy link
Member Author

chalasr commented Mar 11, 2021 •
edited
Loading

The travis failure with deps=high is expected. This is ready

rosier
rosier reviewed Mar 11, 2021
View reviewed changes
@chalasr chalasr force-pushed the inmemory-user branch 2 times, most recently from 5a42ae2 to f24f084 Compare March 12, 2021 11:00
@chalasr chalasr mentioned this pull request Mar 12, 2021
Closed
@chalasr
Copy link
Member Author

chalasr commented Mar 12, 2021

Thanks for the review @rosier, all comments have been addressed.

fabpot
fabpot reviewed Mar 12, 2021
View reviewed changes
src/Symfony/Component/Security/Core/User/InMemoryUserChecker.php
throw $ex;
}

// @deprecated since Symfony 5.3
Copy link
Member

@fabpot fabpot Mar 12, 2021

Choose a reason for hiding this comment

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

What about NOT supporting the deprecated stuff in the new class?

Copy link
Member Author

@chalasr chalasr Mar 12, 2021

Choose a reason for hiding this comment

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

Given this makes the security.user_checker service use the new class, not supporting those checks would be a behavior change. As this is about security and all is done via configuration (you don't need to manipulate those classes to use them), I think it's worth keeping those checks until 6.0

derrabus
derrabus reviewed Mar 14, 2021
View reviewed changes
@chalasr chalasr force-pushed the inmemory-user branch 6 times, most recently from 883cd52 to 1c6cdd3 Compare March 14, 2021 14:13
@chalasr chalasr added the Ready label Mar 14, 2021
wouterj
wouterj approved these changes Mar 15, 2021
View reviewed changes
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Thanks Robin!

Am I missing something, or are the 2 src/Symfony/Component/Security/Core/Tests/Authentication/Token/Fixtures/switch-user-token-*.txt files not used?

wouterj
wouterj requested changes Mar 15, 2021
View reviewed changes
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Sorry, I selected the wrong choice 🤦

@wouterj wouterj removed the Ready label Mar 15, 2021
@chalasr
Copy link
Member Author

chalasr commented Mar 15, 2021

Am I missing something, or are the 2 src/Symfony/Component/Security/Core/Tests/Authentication/Token/Fixtures/switch-user-token-*.txt files not used?

Only one of two, actually! It's used in SwitchUserListenerTest.

Thanks for the review Wouter, all fixed.

@wouterj wouterj added the Ready label Mar 16, 2021
@fabpot
Copy link
Member

fabpot commented Mar 16, 2021

Are the broken tests related to the changes here?

@chalasr
Copy link
Member Author

chalasr commented Mar 16, 2021

@fabpot yes but only deps=high fails, I think it will be fixed once merged.
In case I'm wrong, be sure that I'll send a PR to fix it right away.

@fabpot
Copy link
Member

fabpot commented Mar 16, 2021

Thank you @chalasr.

@fabpot fabpot mentioned this pull request Mar 16, 2021
Closed
@fabpot fabpot merged commit db87d72 into symfony:5.x Mar 16, 2021
@chalasr chalasr deleted the inmemory-user branch March 16, 2021 18:13
@fabpot fabpot mentioned this pull request Apr 18, 2021
Merged
@jrushlow jrushlow mentioned this pull request Apr 23, 2021
Merged
@chalasr chalasr mentioned this pull request May 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@derrabus derrabus derrabus left review comments

@fabpot fabpot fabpot approved these changes

@wouterj wouterj wouterj approved these changes

+2 more reviewers

@rosier rosier rosier left review comments

@noniagriconomie noniagriconomie noniagriconomie left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Deprecation Feature Ready Security Status: Reviewed

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

[RFC][Security] Simplify the User and UserChecker

7 participants

@chalasr @fabpot @rosier @wouterj @derrabus @noniagriconomie @carsonbot