Skip to content

Wrong advice in "How to load Security Users from the Database (the Entity Provider)" cookbook entry #3165

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
Closed
fberci opened this issue Nov 6, 2013 · 2 comments
Closed

Wrong advice in "How to load Security Users from the Database (the Entity Provider)" cookbook entry #3165

fberci opened this issue Nov 6, 2013 · 2 comments
Labels
actionable Clear and specific issues ready for anyone to take them. bug good first issue Ideal for your first contribution! (some Symfony experience may be required) hasPR A Pull Request has already been submitted for this issue. Security

Comments

@fberci
Copy link

fberci commented Nov 6, 2013

The cookbook recommends implementing a custom serialize() method on your User object that only persists the id. However, this creates an unexpected issue. As the cookbook says, the User object will be refreshed after deserialization, but because the username field (among others) is missing from the deserialized object, Symfony will detect that the object has changed, and therefore it will set the token's authenticated flag to false. This should not happen.

The creators of the FOSBundle have also noticed this issue, please see their comment.
@wouterj
Copy link
Member

wouterj commented Dec 3, 2013

When not implementing the EquatableInterface, the hasUserChanged method uses the username, password and salt to compare the user. These three should be serialized.

When not having a custom entity user repositroy, the EntityUserProvider uses the identifier to refresh a user.

So at least 4 properties should be serialized: $this->username, $this->password, $this->salt and $this->id. The first example in the cookbook referenced in the start should be updated to follow this.

@xabbuh xabbuh mentioned this issue Dec 9, 2013
Merged
@weaverryan
Copy link
Member

Fixed up now - thanks very much Bertalan for the report - this was a good one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
actionable Clear and specific issues ready for anyone to take them. bug good first issue Ideal for your first contribution! (some Symfony experience may be required) hasPR A Pull Request has already been submitted for this issue. Security
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@weaverryan @fberci @wouterj