8000 [Security] SimpleAuthenticationProvider is broken · Issue #26775 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] SimpleAuthenticationProvider is broken #26775

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
leofeyer opened this issue Apr 3, 2018 · 13 comments
Closed

[Security] SimpleAuthenticationProvider is broken #26775

leofeyer opened this issue Apr 3, 2018 · 13 comments
Labels
Bug Security Status: Needs Review

Comments

@leofeyer
Copy link
Contributor
leofeyer commented Apr 3, 2018
Q A
Bug report? yes
Feature request? no
BC Break report? yes
RFC? no
Symfony version 3.4.7

The changes from #26370 break the simple authentication provider.

symfony/src/Symfony/Component/Security/Core/Authentication/Provider/SimpleAuthenticationProvider.php

Lines 47 to 48 in 341682e

$user = $authToken->getUser();
$this->userChecker->checkPreAuth($user);

$authToken->getUser() returns a "UserInterface instance, an object implementing a __toString method, or the username as a regular string", however the UserChecker::checkPreAuth() method allows a UserInterface instance only. An anonymous token results in the following exception:

Type error: Argument 1 passed to Symfony\Component\Security\Core\User\UserChecker::checkPreAuth() must be an instance of Symfony\Component\Security\Core\User\UserInterface, string given, called in …/vendor/symfony/symfony/src/Symfony/Component/Security/Core/Authentication/Provider/SimpleAuthenticationProvider.php on line 49

@i3or1s /cc
leofeyer added a commit to contao/core-bundle that referenced this issue Apr 3, 2018
@leofeyer
Add a conflict with Symfony 3.4.7.
05bfe38 
* symfony/symfony#26775
* symfony/symfony#26337
* contao/manager-bundle#61
leofeyer added a commit to contao/core-bundle that referenced this issue Apr 3, 2018
@leofeyer
Add a conflict with Symfony 3.4.7.
51b103b 
* symfony/symfony#26775
* symfony/symfony#26337
* contao/manager-bundle#61
@leofeyer leofeyer mentioned this issue Apr 3, 2018
Closed
leofeyer added a commit to contao/core-bundle that referenced this issue Apr 3, 2018
@leofeyer
Add a conflict with Symfony 3.4.7.
70da9c4 
* symfony/symfony#26775
* symfony/symfony#26337
* contao/manager-bundle#61
@linaori
Copy link
Contributor
linaori commented Apr 3, 2018

A few questions:

  • Is this a refresh or a new authentication attempt?
  • Is the firewall stateless?
  • Can you provide a reproducer?

@leofeyer
Copy link
Contributor Author
leofeyer commented Apr 3, 2018

I don't think it is necessary to set up a test case. Just compare the phpDocs of the two interfaces:

TokenInterface::getUser()

    /**
     * Returns a user representation.
     *
     * @return mixed Can be a UserInterface instance, an object implementing a __toString method,
     *               or the username as a regular string
     *
     * @see AbstractToken::setUser()
     */
    public function getUser();

UserCheckerInterface::checkPreAuth()

    /**
     * Checks the user account before authentication.
     *
     * @throws AccountStatusException
     */
    public function checkPreAuth(UserInterface $user);

You cannot pass the getUser() return value to the checkPreAuth() method without further validation.

@i3or1s
Copy link
Contributor
i3or1s commented Apr 4, 2018

Would it make sense to run checkPreAuth and checkPostAuth only in case $authToken->getUser() is instance of UserInterface

@chalasr chalasr mentioned this issue Apr 4, 2018
Merged
@chalasr
Copy link
Member
chalasr commented Apr 4, 2018

I propose to load the user in this case, see #26788.

nicolas-grekas added a commit that referenced this issue Apr 4, 2018
@nicolas-grekas
bug #26788 [Security] Load the user before pre/post auth checks when …
1605684 
…needed (chalasr)

This PR was merged into the 2.8 branch.

Discussion
----------

[Security] Load the user before pre/post auth checks when needed

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | n/a
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #26775
| License       | MIT
| Doc PR        | n/a

Commits
-------

c318306 [Security] Load the user before pre/post auth checks when needed
@aureliengiry
Copy link

Hi, I have same issue.
No issue on symfony 3.4.6 but on 3.4.7.
And I have same issue on symfony 2.8.37, but it's ok on 2.8.32

@chalasr
Copy link
Member
chalasr commented Apr 6, 2018

Hey, the patch has been merged but not released yet, it will be fixed in 3.4.8

@xabbuh
Copy link
Member
xabbuh commented Apr 8, 2018

3.4.8 has been released

@aschempp
Copy link
Contributor

Unfortunately, the new implementation is broken as well, see my comment in #26788 (comment)

@crmpicco
Copy link
crmpicco commented Jun 7, 2018 •
edited
Loading

@chalasr Was this issue fixed in 3.4 only? Has it been backported to 2.8?

@xabbuh
Copy link
Member
xabbuh commented Jun 7, 2018

@crmpicco #27044 was part of the 2.8.39 release

@umpirsky umpirsky mentioned this issue Sep 5, 2019
Closed
@BonBonSlick
Copy link

Dunno why started to receive this issue.

"symfony/security-acl": "^v3.0.4",
  "symfony/security-bundle": "^v5.2.10",
  "symfony/security-core": "^v5.2.10",
  "symfony/security-guard": "^v5.2.10",
  "symfony/security-http": "^v5.2.10",

So the issue persists somewhere


TypeError {#1794
  #message: "Return value of Symfony\Component\Security\Core\Authentication\Token\AbstractToken::getUserIdentifier() must be of the type string, null returned"
  #code: 0
  #file: "/var/www/hint_api/vendor/symfony/security-core/Authentication/Token/AbstractToken.php"
  #line: 79
  trace: {
    /var/www/hint_api/vendor/symfony/security-core/Authentication/Token/AbstractToken.php:79 {
      Symfony\Component\Security\Core\Authentication\Token\AbstractToken->getUserIdentifier(): string …
      ›     // @deprecated since 5.3, change to $user->getUserIdentifier() in 6.0
      ›     return method_exists($this->user, 'getUserIdentifier') ? $this->user->getUserIdentifier() : $this->user->getUsername();
      › }
    }
    /var/www/hint_api/vendor/symfony/security-core/Authentication/Token/AbstractToken.php:259 {
      Symfony\Component\Security\Core\Authentication\Token\AbstractToken->__toString() …
      › 
      ›     return sprintf('%s(user="%s", authenticated=%s, roles="%s")', $class, $this->getUserIdentifier(), json_encode($this->authenticated), implode(', ', $roles));
      › }
    }
    /var/www/hint_api/vendor/monolog/monolog/src/Monolog/Formatter/NormalizerFormatter.php:179 {
      Monolog\Formatter\NormalizerFormatter->normalize($data, int $depth = 0) …
      ›     /** @var string $value */
      ›     $value = $data->__toString();
      › } else {
    }
    /var/www/hint_api/vendor/monolog/monolog/src/Monolog/Formatter/NormalizerFormatter.php:159 {

@BonBonSlick
Copy link

This is because some package was updated with security package where was added new method

    /**
     * @deprecated
     */
    public function getUsername(): void { }

    public function getUserIdentifier(): string {
        return $this->secret()->value();
    }

@xabbuh
Copy link
Member
xabbuh commented Aug 14, 2021

@BonBonSlick Does it mean your issue has been solved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Security Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0