Skip to content

[Security] deprecate non UserInterface users #34909

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

Closed
wants to merge 1 commit into from
Closed

[Security] deprecate non UserInterface users #34909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ public function setUser($user)
throw new \InvalidArgumentException('$user must be an instanceof UserInterface, an object implementing a __toString method, or a primitive string.');
}

if (!$user instanceof UserInterface) {
@trigger_error(sprintf('Storing a user not implementing the %s is deprecated since Symfony 5.1.', UserInterface::class));
Copy link
Contributor

@linaori linaori Dec 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't this cause deprecations on anonymous authentications? The AnonymousToken also calls setUser(). As proposed back in #3697, perhaps an AnonymousUser would make sense? This could be a stale object always returning the same information.

symfony/src/Symfony/Component/Security/Core/Authentication/Token/AnonymousToken.php

Line 33 in e60a876

$this->setUser($user);

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, for me pushed code should just be the starting point. Actually, I was a bit surprised that our code did not trigger any deprecations. Might be an indicator that we do miss some tests (the anonymous user is one of the examples where I did expect the new deprecation to be triggered).

Copy link
Contributor

Choose a reason for hiding this comment

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

my proposal applied to ControllerTrait::getUser(), to keep it consistent with Security::getUser().

no need to patch TokenInterface IMHO, as it's low-level API, unless we decide to refactor everything now..? 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change is good to make the usage consistent for now. At a later stage, I hope we can get rid of the UserInterface inside the token altogether.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ro0NL IMO it would be too confusing if the controller method behaved differently than the token storage.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it's confusing for Security::getUser to behave different already.

They way i see it is, Security/ControllerTrait::getUser() is high-level API, designed around UserInterface|null.

Whereas TokenInterface::getUser(), the low-level API, is designed around object|string|null.

So the high level API is a subset.

}

if (null === $this->user) {
$changed = false;
} elseif ($this->user instanceof UserInterface) {
Expand Down
7 changes: 3 additions & 4 deletions src/Symfony/Component/Security/Core/Authentication/Token/TokenInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

namespace Symfony\Component\Security\Core\Authentication\Token;

use Symfony\Component\Security\Core\User\UserInterface;

/**
* TokenInterface is the interface for the user authentication information.
*
Expand Down Expand Up @@ -55,10 +57,7 @@ public function getUser();
/**
* Sets the user in the token.
*
* The user can be a UserInterface instance, or an object implementing
* a __toString method or the username as a regular string.
*
* @param string|object $user The user
* @param UserInterface $user The user
*
* @throws \InvalidArgumentException
*/
Expand Down