8000 [security] There is no user provider for user · Issue #35435 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[security] There is no user provider for user #35435

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
thewalkingcoder opened this issue Jan 22, 2020 · 20 comments
Closed

[security] There is no user provider for user #35435

thewalkingcoder opened this issue Jan 22, 2020 · 20 comments
Labels
Security

Comments

@thewalkingcoder
Copy link
thewalkingcoder commented Jan 22, 2020 •
edited by nicolas-grekas
Loading

Symfony version(s) affected: 4.4.3

Description
After update to 4.4.3 i have a RuntimeException "There is no user provider for user"

How to reproduce
if you have custom provider update to 4.4.3

Sample

security:
    providers:
        custom-security:
            id: custom.user_provider
    firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false
        login:
            pattern: ^/login
            security: false
        secured_area:
            pattern: ^/
            guard:
                authenticators:
                    - custom.authentificator
                entry_point: custom.authentificator
            logout:
                path: /logout

Possible Solution
Problem is on the file Symfony/Component/Security/Http/Firewall/ContextListener.php method refreshUser

Follows #35065
@thewalkingcoder
Copy link
Author

After more analyse, my customProvider return bad Class on method supportClass

@Pi-r-
Copy link
Pi-r- commented Jan 22, 2020

Hi I am having the same issue but with version 3.4.37 https://stackoverflow.com/questions/59858525/there-is-no-user-provider-for-user-symfony-3-4-37

Wen downgrading to 3.4.36 everything works fine.

@Pi-r-
Copy link
Pi-r- commented Jan 22, 2020

Same thing with version 3.4.37 the issue was from my custom user provider supportsClass methode returning a wrong class

@ahundiak
Copy link
Contributor

Just in case anybody is wondering, the 3.4.37 problem was also the result of an invalid supportsClass method. Looks like the older code was not properly using the method.

@terox
Copy link
terox commented Jan 23, 2020

Hello everybody!

I am experiencing the same issue on the Symfony 4.4.3. As all other guys before me, the issue is on the method supportsClass on my custom user provider. A little explanation about how fix that issue:

Before (fails with the described exception):

public function supportsClass($class)
{
    return $class === UserInterface::class;
}

After (works fine):

public function supportsClass($class)
{
    return $class === MyCustomModel::class;
}

I was checking the security component and I think that the issue is introduced in the next commit:
symfony/security@0cfaf60#diff-0cd860ec03e139a658ed3c258b615f21

Sincerely, I think that now it have the expected behaviour; but is breaking bad implemented custom providers. May be some one could correct my words.

Cheers!

@thewalkingcoder
Copy link
Author

@terox you are right, even if the last implementation is the good way and have sense to fix on 5.x, on 4.4.3 you break semserv.

ping @nicolas-grekas, what do you think ?

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jan 23, 2020 •
edited
Loading

No opinion. Any bug fix is a potential BC break, it's never black and white. Given all reports mention bad implementations of supportsClass(), it looks like fixing them is a good thing.
/cc @linaori WDYT?

@thewalkingcoder
Copy link
Author

you have right too

@kissifrot
Copy link
kissifrot commented Jan 23, 2020 •
edited
Loading

On our side we have the problem when a Doctrine proxy class is used by security component instead of the original one, such as Proxies\__CG__\App\Entity\OurUser instead of App\Entity\OurUser

So supportsClass will return false in that case thus preventing the user from logging in

@terox
Copy link
terox commented Jan 23, 2020

I agree with @nicolas-grekas.

Should the documentation mention or put some example that @kissifrot is commenting? I think that it could be a common issue.

@kissifrot
Copy link
kissifrot commented Jan 23, 2020 •
edited
Loading

As a follow-up to my previous comment, for most cases to support the proxies and fix the user provider error you would just have to edit your supportsClass like so:

public function supportsClass($class)
{
    return MyUserEntity::class === $class || is_subclass_of($class, MyUserEntity::class);
}

@wouterj
Copy link
Member
wouterj commented Jan 23, 2020

To get things clear: What we're seeing here is that supportsClass() returns false for the currently authenticated user, as it's using some "bad" condition? So if we remove the supportsClass() 👇 call we end up with a working application?

symfony/src/Symfony/Component/Security/Http/Firewall/ContextListener.php

Lines 178 to 180 in d3942cb

if (!$provider->supportsClass($userClass)) {
continue;
}

I think it's good to have this bug fix, but we can do better at reporting the bug. For instance, what about moving this if condition after 👇 in Symfony 5.x?

symfony/src/Symfony/Component/Security/Http/Firewall/ContextListener.php

Lines 182 to 185 in d3942cb

try {
$refreshedUser = $provider->refreshUser($user);
$newToken = clone $token;
$newToken->setUser($refreshedUser);

We can then improve the exception by saying something like:

There is no user provider for user. User provider X was able to refresh the user, but its supportUser() method returned false for this class. This will break in Symfony 6.

And then move the if statement to it's current position in Symfony 6, probably speeding up applications with many users providers a bit.

@linaori
Copy link
Contributor
linaori commented Jan 23, 2020

@nicolas-grekas this is a use-case that I have discussed with @chalasr I believe, I can't find it right now so must've been on slack as I couldn't see it in the PR or Issue. We considered it to be a bug if your logic in supportsClass() was not correct and not a case we should take care of in this bug fix. If this is really causing a lot of issues for people, I suggest we take a look at @wouterj's idea about the exception message.

To get things clear: What we're seeing here is that supportsClass() returns false for the currently authenticated user, as it's using some "bad" condition? So if we remove the supportsClass() 👇 call we end up with a working application?

That depends, it's not clear -especially in older Symfony versions- that the refreshUser() should throw an exception for this, as supportsClass() has always been there. In most scenarios this worked because the of non-complex authentication processes. 1 user with 1 authenticator and 1 user provider, and you'll not encounter this bug, which makes you believe supportsClass() works fine.

I think it's good to have this bug fix, but we can do better at reporting the bug. For instance, what about moving this if condition after 👇 in Symfony 5.x?

This would have to be done in the same in the ChainUserProvider. I'm still of opinion that a better check would be something like supportsUser($user), as that's basically what you want to do do. Using the refreshUser for this kind of logic, is easy to miss, even without supports (lack of @throws in php like java has).

can then improve the exception by saying something like [...]

So this suggestion (by moving the logic) would be a DX solution to hint people what is wrong? I would advice to do this in 3.4+ already if it's merely DX as part of this bug fix.

@MisatoTremor
Copy link
Contributor
MisatoTremor commented Jan 24, 2020 •
edited
Loading

I also ran into this, but think the change is a good one.

I suggest that the upgrade docs for the affected versions should mention this change and the is_subclass_of fix as this check seems to me to be the major cause for this issue.
Also the default implementation of the EntityUserProvider::supportsClass has got this right for years, but generated classes have not (see symfony/maker-bundle#532 and also mentioned in the docs) which might be why many of those affected have this issue.

@chalasr
Copy link
Member
chalasr commented Jan 24, 2020

I'm against reverting. It revealed many broken implementations which are likely to open the door for true security holes, especially on 3.4 where changed users are not deauthenticated by default at refresh-time.
PRs welcome for maker-bundle, core documentation and/or DX. But reverting the patch in order to provide better messages (namely upgrade paths) is a no-go to me.

@chalasr
Copy link
Member
chalasr commented Jan 24, 2020

Closing in favor of symfony/maker-bundle#532. Thanks for the report!

@chalasr chalasr closed this as completed Jan 24, 2020
@wouterj
Copy link
Member
wouterj commented Jan 24, 2020

@chalasr it's not so much about reverting, it's about improving the error message. Fixing the maker helps future applications, but doesn't help applications upgrading to Symfony 4.4. We should help the upgrading user to easier find this error (as "There is no user provider for this user" isn't a very good indicator to start debugging)

@linaori
Copy link
Contributor
linaori commented Jan 24, 2020

(as "There is no user provider for this user" isn't a very good indicator to start debugging)

What if we change this message to refer to the supportsUser() method?

@chalasr
Copy link
Member
chalasr commented Jan 24, 2020

@wouterj right, thanks for clarifying. 👍 on my side

@chalasr chalasr reopened this Jan 24, 2020
@chalasr chalasr added the Help wanted Issues and PRs which are looking for volunteers to complete them. label Jan 24, 2020
@wouterj wouterj mentioned this issue Jan 25, 2020
Merged
@wouterj
Copy link
Member
wouterj commented Jan 25, 2020

I've implemented @linaori's suggestion in #35472 . I think that's the easiest to not make code much complexer and still give some valuable hints to the user.

@chalasr chalasr removed the Help wanted Issues and PRs which are looking for volunteers to complete them. label Jan 26, 2020
nicolas-grekas added a commit that referenced this issue Jan 27, 2020
@nicolas-grekas
minor #35472 [Security] Improved error message when no supported user…
b90664b 
… provider is found (wouterj)

This PR was submitted for the 4.4 branch but it was merged into the 3.4 branch instead.

Discussion
----------

[Security] Improved error message when no supported user provider is found

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #35435
| License       | MIT
| Doc PR        | -

Commits
-------

6b2db6d Improved error message when no supported user provider is found
@sw-satoshi-nakano sw-satoshi-nakano mentioned this issue Oct 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4B80
Assignees
No one assigned
Labels
Security
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0