EZP-24852: Add UserReference support in Authentication/User providers #1965

andrerom · 2017-04-21T14:50:59Z

JIRA: https://jira.ez.no/browse/EZP-24852

As we anyway always refresh APIUser on load, this changes our symfony User to not serialize APIUser to session anymore to reduce size of session being serialized back and forth.

On top of that make sure WrappedUser does not get self or User injected to avoid possibility of wrong use and bloated session value.

…f or User To protect against injecting api user several times bloating session value.

andrerom · 2017-04-27T20:55:10Z

Review ping @lolautruche

NOTE: I assumed we kind of need to continue load users on load to make sure user is still existing and enabled (however not needed for permission checks), so only made sure user is no longer serialized to and from session, but that assumption might be wrong.

lolautruche

I don't see the UserReference class, is it already present somewhere?

I assumed we kind of need to continue load users on load to make sure user is still existing and enabled

This is indeed needed when user is refreshed from the session. It's not specific to eZ, but needed from the Security component

lolautruche · 2017-04-28T07:47:14Z

eZ/Publish/Core/MVC/Symfony/Security/User.php

+        if (!$this->user instanceof APIUser) {
+            throw new \LogicException(
+                'Attempts to get APIUser before it has been set by UserProvider, APIUser is not serialized to session'
+            );


This exception needs to be documented in the phpdoc

re added in d6a6803

Would it be worth using an exception of our own ?

hmm, maybe, however unsure what that should be and what it should extend, and it's not a condition that should happen for end users given provider deals with loading it before you typically get your hands on it.

As we always refresh APIUser on load anyway, changes our symfony User to not serialize APIUser to session to reduce size of session being serialized back and forth. This approch assumed the refresh is needed for security reasons *(as in check if user is still exists and is enabled, permissions lookup don't need this)*. However if that is not the case this can be done closer to how it is described in JIRA issue and aim to deprecate this logic instead.

Not a must, but test where failing since MVC User was calling method and this part of the code property.

andrerom · 2017-04-28T09:13:59Z

but needed from the Security component

Maybe, this doc on UserProviderInterface::refreshUser() made it a bit unclear, and made it sound it depends on our domain needs:

     * It is up to the implementation to decide if the user data should be
     * totally reloaded (e.g. from the database), or if the UserInterface
     * object can just be merged into some internal array of users / identity
     * map.

I don't see the UserReference class, is it already present somewhere?

Depends on what you mean, you have:

Then in this PR there is ReferenceUserInterface which is implemented by eZ\Publish\Core\MVC\Symfony\Security\User.

lolautruche · 2017-04-28T10:54:50Z

Maybe, this doc on UserProviderInterface::refreshUser() made it a bit unclear, and made it sound it depends on our domain needs

It indeed depends on our domain needs, but this is highly recommended and done in every implementation provided by Symfony. Only exception I can imagine would be stateless authentication.

As for UserReference, it is used in the Symfony user constructor, but I couldn't see it anywhere else, hence my question.

andrerom · 2017-04-28T14:34:24Z

It indeed depends on our domain needs, but this is highly recommended and done in every implementation provided by Symfony. Only exception I can imagine would be stateless authentication.

ok, then approach here is at least going in the right direction, our old issue was implying to get rid of it, but then we aim to keep the object refresh, and instead look for way to make it lighter later in api.

As for UserReference, it is used in the Symfony user constructor, but I couldn't see it anywhere else, hence my question.

Actually it is used a few places already:
https://github.com/ezsystems/ezpublish-kernel/search?l=PHP&q=%22new+UserReference%22&type=&utf8=%E2%9C%93

alongosz

Looks good, 2 not important nitpicks.

alongosz · 2017-05-08T09:21:28Z

eZ/Publish/Core/MVC/Symfony/Security/Tests/UserTest.php

@@ -93,7 +96,11 @@ public function testIsNotEqualTo()
    public function testIsEqualToNotSameUserType()
    {
        $user = new User();
-        $user2 = $this->getMock('Symfony\Component\Security\Core\User\UserInterface');
+        $user2 = $this->getMock('eZ\Publish\Core\MVC\Symfony\Security\ReferenceUserInterface');


Nitpick: ReferenceUserInterface::class please :)

alongosz · 2017-05-08T09:22:31Z

eZ/Publish/Core/MVC/Symfony/Security/Tests/UserWrappedTest.php

@@ -127,7 +129,7 @@ public function testRegularUser()

    public function testIsEqualTo()
    {
-        $originalUser = $this->getMock('eZ\Publish\Core\MVC\Symfony\Security\User');
+        $originalUser = $this->getMock('eZ\Publish\Core\MVC\Symfony\Security\Tests\UserEquatableInterface');


UserEquatableInterface::class

Ref: ezsystems/ezpublish-kernel#1965

…ezsystems#1965) * Throw InvalidArgumentException if WrappedUser recives instance of self or User To protect against injecting api user several times bloating session value. * EZP-24852: Add UserReference support in Authentication/User providers As we always refresh APIUser on load anyway, changes our symfony User to not serialize APIUser to session to reduce size of session being serialized back and forth. This approch assumed the refresh is needed for security reasons *(as in check if user is still exists and is enabled, permissions lookup don't need this)*. However if that is not the case this can be done closer to how it is described in JIRA issue and aim to deprecate this logic instead. * [UnitTests] Adapt Tests for changes * Adapt RestAuthenticator to use getUserId() also Not a must, but test where failing since MVC User was calling method and this part of the code property. * Deprecate UserInterface::setAPIUser() so update is not forgotten in 7.0 * CS

andrerom force-pushed the sfuser_improvments branch 4 times, most recently from dd6a6d9 to 531c958 Compare April 24, 2017 15:37

Throw InvalidArgumentException if WrappedUser recives instance of sel…

0cb00fe

…f or User To protect against injecting api user several times bloating session value.

andrerom force-pushed the sfuser_improvments branch 3 times, most recently from 081f5e5 to b5c3fe3 Compare April 27, 2017 20:33

andrerom changed the title ~~[WIP] EZP-24852: Add UserReference support in Authentication/User providers~~ EZP-24852: Add UserReference support in Authentication/User providers Apr 27, 2017

andrerom force-pushed the sfuser_improvments branch from c7ea670 to 5e83ce9 Compare April 27, 2017 20:52

andrerom requested review from bdunogier and glye April 27, 2017 20:53

lolautruche approved these changes Apr 28, 2017

View reviewed changes

glye approved these changes Apr 28, 2017

View reviewed changes

andrerom added 3 commits April 28, 2017 11:09

[UnitTests] Adapt Tests for changes

f8e60e6

Adapt RestAuthenticator to use getUserId() also

1f50078

Not a must, but test where failing since MVC User was calling method and this part of the code property.

andrerom force-pushed the sfuser_improvments branch from 5e83ce9 to 1f50078 Compare April 28, 2017 09:16

andrerom requested a review from alongosz April 30, 2017 08:05

Deprecate UserInterface::setAPIUser() so update is not forgotten in 7.0

9731a7d

alongosz approved these changes May 8, 2017

View reviewed changes

CS

75de786

andrerom merged commit 62c24a0 into master May 8, 2017

andrerom deleted the sfuser_improvments branch May 8, 2017 13:47

emodric added a commit to netgen/NetgenEzSyliusBundle that referenced this pull request May 8, 2017

Add support for eZ UserReference in User object

9f20986

Ref: ezsystems/ezpublish-kernel#1965

emodric added a commit to ezsystems/LegacyBridge that referenced this pull request Jun 21, 2017

Fix tests after ezsystems/ezpublish-kernel#1965

e7984b3

emodric mentioned this pull request Jun 21, 2017

Fix tests ezsystems/LegacyBridge#97

Merged

andrerom pushed a commit to ezsystems/LegacyBridge that referenced this pull request Jun 22, 2017

Fix tests after ezsystems/ezpublish-kernel#1965 (#97)

252793b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-24852: Add UserReference support in Authentication/User providers #1965

EZP-24852: Add UserReference support in Authentication/User providers #1965

andrerom commented Apr 21, 2017 •

edited

Loading

andrerom commented Apr 27, 2017 •

edited

Loading

lolautruche left a comment

lolautruche Apr 28, 2017

andrerom Apr 28, 2017

bdunogier Apr 28, 2017

andrerom May 4, 2017 •

edited

Loading

andrerom commented Apr 28, 2017 •

edited

Loading

lolautruche commented Apr 28, 2017

andrerom commented Apr 28, 2017 •

edited

Loading

alongosz left a comment

alongosz May 8, 2017

alongosz May 8, 2017

EZP-24852: Add UserReference support in Authentication/User providers #1965

EZP-24852: Add UserReference support in Authentication/User providers #1965

Conversation

andrerom commented Apr 21, 2017 • edited Loading

andrerom commented Apr 27, 2017 • edited Loading

lolautruche left a comment

Choose a reason for hiding this comment

lolautruche Apr 28, 2017

Choose a reason for hiding this comment

andrerom Apr 28, 2017

Choose a reason for hiding this comment

bdunogier Apr 28, 2017

Choose a reason for hiding this comment

andrerom May 4, 2017 • edited Loading

Choose a reason for hiding this comment

andrerom commented Apr 28, 2017 • edited Loading

lolautruche commented Apr 28, 2017

andrerom commented Apr 28, 2017 • edited Loading

alongosz left a comment

Choose a reason for hiding this comment

alongosz May 8, 2017

Choose a reason for hiding this comment

alongosz May 8, 2017

Choose a reason for hiding this comment

andrerom commented Apr 21, 2017 •

edited

Loading

andrerom commented Apr 27, 2017 •

edited

Loading

andrerom May 4, 2017 •

edited

Loading

andrerom commented Apr 28, 2017 •

edited

Loading

andrerom commented Apr 28, 2017 •

edited

Loading