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

chalasr · 2018-04-04T08:00:26Z

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

linaori · 2018-04-04T08:17:28Z

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

@@ -45,6 +46,11 @@ public function authenticate(TokenInterface $token)
        }

        $user = $authToken->getUser();
+
+        if (!$user instanceof UserInterface) {


What if the user is Anon.? It will fail

No it will not. instanceof handles strings.

it will call this: $user = $this->userProvider->loadUserByUsername('Anon.');, which will fail to retrieve the user and result in NULL

Yea, but loadUserByUsername() will throw a UsernameNotFoundException. Should we throw a specific exception before?

It will also handle null: https://3v4l.org/Xrt8u

* @return UserInterface does enforce it to me, it doesn't allow null

Except that people can just do return null; and it won't fail.

well, if they don't respect the contract of the interface, they enter unknown land. Things might break badly at any time (and the typehint error on the next call is such a failure), and the failure can even change in any Symfony patch release. We don't guarantee any backward compatibility for code not respecting the interface contract (and yes, any new API added in Symfony 4.1+ will use return types to enforce it better)

While I fully agree, sadly (especially older implementations) are often based of "reverse engineered" authenticators, due to lack of better (SF2.0~2.6). Thus, I can't fully blame developers for making this "mistake".

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

Lines 74 to 78 in 9f1c017

$user = $this->userProvider->loadUserByUsername($username);

if (!$user instanceof UserInterface) {

throw new AuthenticationServiceException('The user provider must return a UserInterface object.');

}

same check added for consistency

nicolas-grekas · 2018-04-04T09:57:28Z

~~wrong target branch?~~ (fixed)

dmaicher · 2018-04-04T13:08:17Z

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

+                $user = $this->userProvider->loadUserByUsername($user);
+
+                if (!$user instanceof UserInterface) {
+                    throw new AuthenticationServiceException('The user provider must return a UserInterface object.');


~~should we also call setToken for this one?~~ forget this comment 😉

nicolas-grekas · 2018-04-04T13:33:35Z

Thank you @chalasr.

…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

aschempp · 2018-04-11T20:34:37Z

Unfortunately, this implementation is broken as well.

If $user is not UserInterface, the new implementation calls the UserProviderwith the$user, which might be *an object implementing a __toString` method*.
The UserProvider 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 a UserInterface. That again is incorrect, as the return value still could be an object implementing a __toString method.

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

chalasr · 2018-04-11T22:38:56Z

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 must return an instance of UserInterface.

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

aschempp · 2018-04-12T08:51:01Z

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

For BC reasons, our current LTS version (Contao 4.4) is using a custom Authenticator registered in the firewall config. Our legacy user classes do not implement the UserInterface. We also don't actually load the user by username, but by firewall 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 provider again 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 a UserInterface on the second attempt, but that case is absolutely valid! There does not need to be a UserInterface, the user can always just be a string and be fully authenticated.

linaori · 2018-04-12T09:06:50Z

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 the Simple* 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 the Anonymous 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.

aschempp · 2018-04-12T09:26:26Z

I totally agree Guard would 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 is kinda complex as well due to all the legacy stuff), but we're not using Simple Authentication anymore and our users actually do implement UserInterface now.

Still, I think there are valid use cases for not having a UserInterface. I did some work with Shibboleth 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 a IS_FULLY_AUTHENTICATED user.

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

linaori · 2018-04-12T09:38:51Z

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

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

chalasr · 2018-04-12T15:24:11Z

Point is the contract says UserInterface, 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's getUser() 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.

aschempp · 2018-04-13T06:48:30Z

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 implementing UserInterface so the user can be passed to UserChecker which does nothing because the user is not AdvancedUserInterface. But yet, it would work… 😂

linaori · 2018-04-13T06:53:06Z

@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.

see symfony/symfony#26788 (comment)

aschempp · 2018-04-13T08:55:30Z

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

chalasr · 2018-04-14T13:42:56Z

@aschempp see #26871

chalasr added Security Bug labels Apr 4, 2018

chalasr added this to the 2.8 milestone Apr 4, 2018

carsonbot added the Status: Needs Review label Apr 4, 2018

chalasr mentioned this pull request Apr 4, 2018

[Security] SimpleAuthenticationProvider is broken #26775

Closed

leofeyer approved these changes Apr 4, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 4, 2018

linaori reviewed Apr 4, 2018

View reviewed changes

nicolas-grekas changed the base branch from master to 2.8 April 4, 2018 12:02

chalasr force-pushed the simple-auth-user-checks branch 3 times, most recently from f40ce0f to 21f6216 Compare April 4, 2018 12:59

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

c318306

chalasr force-pushed the simple-auth-user-checks branch from 21f6216 to c318306 Compare April 4, 2018 13:01

nicolas-grekas approved these changes Apr 4, 2018

View reviewed changes

dmaicher reviewed Apr 4, 2018

View reviewed changes

nicolas-grekas merged commit c318306 into symfony:2.8 Apr 4, 2018

chalasr deleted the simple-auth-user-checks branch April 4, 2018 13:34

This was referenced Apr 4, 2018

Bugfix/simple authentication provider anonymous token #26808

Closed

[Security] Backwards breaking change in SimpleAuthenticationProvider #26807

Closed

This was referenced Apr 6, 2018

Release v2.8.38 #26841

Merged

Release v3.4.8 #26844

Merged

Release v4.0.8 #26849

Merged

aschempp mentioned this pull request Apr 11, 2018

Symfony 3.4.8: A Token was not found in the TokenStorage contao/core-bundle#1466

Closed

aschempp added a commit to aschempp/contao-core-bundle that referenced this pull request Apr 13, 2018

Implement UserInterface to work around Symfony core changes

a00deb2

see symfony/symfony#26788 (comment)

aschempp mentioned this pull request Apr 13, 2018

Implement UserInterface to work around Symfony core changes contao/core-bundle#1477

Closed

	$user = $this->userProvider->loadUserByUsername($username);

	if (!$user instanceof UserInterface) {
	throw new AuthenticationServiceException('The user provider must return a UserInterface object.');
	}

Uh oh!

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

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

Uh oh!

Conversation

chalasr commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmaicher Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Apr 4, 2018

Uh oh!

aschempp commented Apr 11, 2018

Uh oh!

chalasr commented Apr 11, 2018

Uh oh!

aschempp commented Apr 12, 2018

Uh oh!

linaori commented Apr 12, 2018

Uh oh!

aschempp commented Apr 12, 2018

Uh oh!

linaori commented Apr 12, 2018

Uh oh!

chalasr commented Apr 12, 2018

Uh oh!

aschempp commented Apr 13, 2018

Uh oh!

linaori commented Apr 13, 2018

Uh oh!

aschempp commented Apr 13, 2018

Uh oh!

chalasr commented Apr 14, 2018

Uh oh!

Uh oh!

nicolas-grekas commented Apr 4, 2018 •

edited

Loading

dmaicher Apr 4, 2018 •

edited

Loading