Add tests for User serialization [#2017787]

Comment	File	Size	Author
#1	2017787-user-serialization-tests_includes2003392.patch	21.42 KB	linclark

#1	2017787-user-serialization-tests-do-not-test.patch	3.92 KB	linclark

Comment #1

linclark CreditAttribution: linclark commented 12 June 2013 at 14:45

Status:

Active

» Needs review

File	Size
2017787-user-serialization-tests-do-not-test.patch	3.92 KB

2017787-user-serialization-tests_includes2003392.patch	21.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.

Log in or register to post comments

Comment #2

klausi

he/him||they/them

German

🇦🇹 Vienna

CreditAttribution: klausi commented 27 June 2013 at 15:01

+++ 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?

Log in or register to post comments

Comment #3

linclark CreditAttribution: linclark commented 27 June 2013 at 15:14

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.

Log in or register to post comments

Comment #4

klausi

he/him||they/them

German

🇦🇹 Vienna

CreditAttribution: klausi commented 27 June 2013 at 15:29

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.

Log in or register to post comments

Comment #5

linclark CreditAttribution: linclark commented 27 June 2013 at 15:31

Status:

Needs review

» Postponed

Sounds good.

Log in or register to post comments

Comment #6

moshe weitzman CreditAttribution: moshe weitzman commented 27 June 2013 at 16:26

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

Log in or register to post comments

Comment #7

damiankloip CreditAttribution: damiankloip commented 2 September 2013 at 16:43

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

Log in or register to post comments

Comment #8

linclark CreditAttribution: linclark commented 23 September 2013 at 12:12

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.