Now that the User entity is EntityNG, we should ensure that it serializes as expected.

CommentFileSizeAuthor
#1 2017787-user-serialization-tests_includes2003392.patch21.42 KBlinclark
#1 2017787-user-serialization-tests-do-not-test.patch3.92 KBlinclark

Comments

linclark’s picture

linclark commented
Status: Active » Needs review
StatusFileSize
new 2017787-user-serialization-tests-do-not-test.patch3.92 KB
new 2017787-user-serialization-tests_includes2003392.patch21.42 KB

This adds the test for UserNormalization in serialization. There should also be one in HAL, but I'd like to get feedback on this before doing the HAL one.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented 
+++ b/core/modules/serialization/lib/Drupal/serialization/Tests/Entity/UserNormalizationTest.php
@@ -0,0 +1,105 @@
+      'uid' => array(
+        array('value' => $user->get('uid')->offsetGet(0)->get('value')->getValue()),

This is a bit ugly - why does $user->uid->value not work? Same for the other fields.

+++ b/core/modules/serialization/lib/Drupal/serialization/Tests/Entity/UserNormalizationTest.php
@@ -0,0 +1,105 @@
+      // The serialization module doesn't do any access checking, so the
+      // password field is exposed if the calling function does not run access
+      // checks before calling serialize.
+      'pass' => array(
+        array('value' => $user->get('pass')->offsetGet(0)->get('value')->getValue()),

Is there an issue already to implement the access check for the password field? I only know the general issue #2028027: [META] Missing access control for base fields.

Why do we need a special normalize test case for users? It is just another entity + field normalization we already test, right?

linclark’s picture

linclark commented

This is a bit ugly - why does $user->uid->value not work? Same for the other fields.

I thought we planned to get rid of the magic getters and setters.

Is there an issue already to implement the access check for the password field?

We talked about this on the call. Didn't Moshe say he was starting one?

Why do we need a special normalize test case for users? It is just another entity + field normalization we already test, right?

If people think we don't need it, than that's ok with me.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Some people were in favor of getting rid of the magic entity field getters/setters, but I don't see that happening in an issue. Maybe I missed it, please post it here if you know it.

As long as we don't encounter any bugs in the actual serialization process of users I don't think we need an explicit test for that entity type. And the leaking password problem is an access problem, which should be tackled at a dedicated issue.

So I think we can leave this here lying around for later, when we find problems with user entities.

linclark’s picture

linclark commented
Status: Needs review » Postponed

Sounds good.

moshe weitzman’s picture

moshe weitzman commented

I just created an issue for access control of password and similar user Fields. It is linked from #2029855: Missing access control for user base fields

damiankloip’s picture

damiankloip commented

@klausi, what's the deal with this issue now? Do you think this should still be postponed?

linclark’s picture

linclark commented

I think Klaus thought that this was unnecessary, so it should be postponed until it was obviously necessary (when a bug was found in User that is not in other entities).

Given Berdir's comment in #1979260: Automatically populate the author default value with the current user, it seems like there might be enough of a difference (the 'uid' property means different things on User entities vs other entities) which is relevant to serialization.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Status: Postponed » Closed (won't fix)

AFAIK this is irrelevant now.