Skip to content
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

[Security] Rename User to InMemoryUser #40443

Merged
merged 1 commit into from Mar 16, 2021
Merged

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:

* 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:
if (!$user instanceof User) {
return;

@chalasr
Copy link
Member Author

chalasr commented Mar 11, 2021

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

UPGRADE-6.0.md Outdated Show resolved Hide resolved
UPGRADE-6.0.md Outdated Show resolved Hide resolved
src/Symfony/Component/Security/Core/User/InMemoryUser.php Outdated Show resolved Hide resolved
@chalasr
Copy link
Member Author

chalasr commented Mar 12, 2021

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

throw $ex;
}

// @deprecated since Symfony 5.3
Copy link
Member

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

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

@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
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?

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 🤦

@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.

@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.

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.

[RFC][Security] Simplify the User and UserChecker
7 participants