wrong target branch? (fixed)

Thank you@chalasr.

Unfortunately, this implementation is broken as well.

• If$user is notUserInterface, the new implementation calls theUserProviderwith the$user, which might be *an object implementing a__toString` method*.
• TheUserProvider only expects a string to be the argument, so an object would be wrong.
• Also, the implementation enforces the return value of the user provider to be aUserInterface. That again is incorrect, as the return value still could bean object implementing a__toString method.

The only viable solution is to only run the UserChecker if$user isinstanceof UserInterface.

The UserProvider only expects a string to be the argument, so an object would be wrong.

Right, casting to string beforehand should be enough to fix that.

Also, the implementation enforces the return value of the user provider to be a UserInterface. That again is incorrect, as the return value still could be an object implementing a __toString method.

No, it mustreturn an instance of UserInterface.

I'll submit a patch tomorrow, thanks for reporting.

That will not solve the problem, at least not for us (Contao CMS).

For BC reasons, our current LTS version (Contao 4.4) is usinga custom Authenticator registeredin the firewall config. Our legacy user classes do not implement theUserInterface. We alsodon't actually load the user by username, but byfirewall scope. Based on the firewall name,users are loaded from legacy classes.

It's all rather complex, but worked perfectly fine until now. Our legacy user class is stored in the token, and thanks to the__toString method the actual username is shown in the profiler etc.

Now, casting the user object to a string would not work, because the object's username would not be suitable for our user provider.

I also think the whole approach is flawed. The code is essentially calling the user provideragain to get a user. It did not get one in the first place, why would it get one if you call it again? Even worse, we're cancelling the authentication if the user provider does not return aUserInterface on the second attempt, but that case is absolutely valid! There does not need to be aUserInterface, the user can always just be a string and be fully authenticated.

the user can always just be a string and be fully authenticated.

And this is one of the reasons why the User object should be restricted as data object and be limited to the security internal behavior, never be exposed to the outside.

While I agree that the current approach is not really ideal, I think that theSimple* variants are not really the way to go anymore. I know it doesn't fix your problem, but Guard has better boundries and restricts far more, leading to less edge-cases and bugs.

I'm curious though, why a custom authenticator to eventually return an anonymous user? I personally think that you should not be altering theAnonymous behavior that Symfony has. So far I've seen all issues that occur with this change, related to anonymous users and strings in custom authenticators.

I totally agreeGuardwould have been the way. We implemented this in Symfony 2.6 or 2.7, so there was no Guard. Since Contao 4.5 (latest) we switched to full Symfony authentication (which iskinda complex as well due to all the legacy stuff), but we're not using Simple Authentication anymore and our users actually do implementUserInterface now.

Still, I think there are valid use cases for not having aUserInterface. I did some work withShibboleth a while ago, where the actual authentication was done through the Apache server (mod_shibboleth). So you simply have a server variable that gives you a username. And for some cases that might just be enough to have aIS_FULLY_AUTHENTICATED user.

Regardless of examples, I think my last paragraph was still valid. It does not make sense to try to load a useragain from the user provider. The authenticator is responsible for that, and if there is noUserInterface then we just can't use theUserChecker.

Still, I think there are valid use cases for not having a UserInterface.

I agree, and I'm 100% sure that we can make Symfonynever expose theUserInterface, by storing only authentication (identification) information in the token, rather than the user object. It would simplify a lot.

Point is the contract saysUserInterface, you're on your own as soon as you return something else (see#26788 (comment)). That is consistent with the rest of the codebase which has the same kind of checks everywhere else (e.g. for guard authenticator'sgetUser() return value).
Considering that our BC promise doesn't cover its use case, can't Contao just make its user classes implement UserInterface?

The authenticator is responsible for that, and if there is no UserInterface then we just can't use the UserChecker.

That means we would have a different behavior when passing a username (or object with__toString() returning the username), not sure it's desirable.

At this stage I suggest to open a new issue to discuss about what can/should be done.

Considering that our BC promise doesn't cover its use case, can't Contao just make its user classes implement UserInterface?

I guess we could. It would be wonky as we're implementingUserInterface so the user can be passed toUserChecker which does nothing because the user is notAdvancedUserInterface. But yet, it would work… 😂

@aschempp That should be solved if this RFC would be implemented:#26348

Add a NullUserChecker, which features 0 checks. In 5.0 this would become the default UserChecker in the configuration.

Unfortunately, there are more issues with the implementation. OurSimplePreAuthenticatorInterface implementation returns an anonymous token if there is no user logged in. This again will result in an exception because is now enforcing aUserInterface 😞

@aschempp see#26871