WordPress.org

Make WordPress Core

Opened 17 months ago

Closed 16 months ago

Last modified 15 months ago

#43547 closed enhancement (fixed)

Add personal data from usermeta/userdata to personal data export

Reported by: allendav Owned by: tz-media
Milestone: 4.9.6 Priority: normal
Severity: normal Version: 5.1
Component: Privacy Keywords: gdpr needs-patch needs-unit-tests
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Add personal data from usermeta/userdata to the personal data export using the infrastructure added in #43438.

See also #43440

Attachments (4)

43440.patch (4.4 KB) - added by TZ Media 16 months ago.
Exporter for user and user_meta
43547.diff (3.7 KB) - added by desrosj 16 months ago.
43547.2.patch (4.2 KB) - added by TZ Media 16 months ago.
43547.patch (5.5 KB) - added by desrosj 16 months ago.

Download all attachments as: .zip

Change History (31)

#1 @allendav
17 months ago

  • Keywords gdpr needs-patch added

This ticket was mentioned in Slack in #gdpr-compliance by tz-media. View the logs.


17 months ago

@TZ Media
16 months ago

Exporter for user and user_meta

#3 @TZ Media
16 months ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

43440.patch adds a data exporter for user and user_meta, based on the code for the comments exporter.

It currently does not export the hashed password, and any data related to roles and capabilities. Should these be added, too?

#4 @desrosj
16 months ago

  • Milestone changed from Awaiting Review to 4.9.6
  • Owner set to tz-media
  • Status changed from new to assigned

#5 @desrosj
16 months ago

  • Keywords needs-refresh added

This ticket was mentioned in Slack in #core by desrosj. View the logs.


16 months ago

@desrosj
16 months ago

#7 @desrosj
16 months ago

  • Keywords needs-testing needs-unit-tests added; needs-refresh removed

I refreshed the patch to apply correctly. While I was at it, I also included the following updates:

  • Added missing space at the beginning and/or end of the add_filter() call and control structures.
  • Added @since tags to inline documentation for added functions.
  • Aligned the $data_to_export array at the =>.
  • Associative arrays should have each item on its own line when there are more than one.
  • Add missing = sign in the elseif ($user->data->user_status = '1' ) condition. Also, converted the user_status conditionals to be Yoda conditions to follow the coding standards. (Should these use strict comparisons as well?)

Questions:

  • Email is passed through strtolower(). I think might cause issues with mixed case emails.
  • wp_user_personal_data_exporter() has a second parameter, $page. For the comments export (the original source of this code), this seems to represent the page of results. If this is needed for the user data export, there should be a description added to that paremter's @param tag. Side note: I think the description for the comment $page parameter could be confusing. Comment page. could be interpreted as a few different things: the page post that the comments belong to, page of the data results, etc..
  • I removed the password related items. I don't think that needs to be included. It is something that is a secret to you. But I could be wrong.
  • The $done variable at the end of wp_user_personal_data_exporter() is not tied into any logic. If $page needs to be utilized, then this needs to be tied into that like in comments.
  • The comments export checks that the email address is not empty and returns early if it is. Should this do the same for consistency? An early return could also be added if the get_user_by() result is empty.

I would also like to see some tests for this. I can come back and write some if I am not beaten to it.

#8 follow-up: @desrosj
16 months ago

Also, I just noticed that there is no default for the switch(). As it stands now, I don't think any user meta would display that is not whitelisted in the switch(). Not sure if this is intentional or not, but should this be more flexible?

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


16 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


16 months ago

#11 @TZ Media
16 months ago

  • Keywords needs-refresh added

@TZ Media
16 months ago

#12 @TZ Media
16 months ago

  • Keywords needs-refresh removed
  • refreshed patch to correctly apply with current trunk.
  • removed $page as it is not needed fur user export.
  • simplified done return parameter to 'done' => true.
  • early return when no user is found for email address.
    • Will the implementation cause problems processing the export?

The mentioned early return if email is empty is on the comments remover, not on the exporter. Should this be added to the exporters as well?

Have not written any tests. @desrosj I would be glad if you could review my changes and do the unit tests for this. Haven's written tests before for core.

#13 in reply to: ↑ 8 @TZ Media
16 months ago

Replying to desrosj:

Also, I just noticed that there is no default for the switch(). As it stands now, I don't think any user meta would display that is not whitelisted in the switch(). Not sure if this is intentional or not, but should this be more flexible?

Same reasoning here as with user exporter. If some theme or plugin adds user meta data, it is responsible for exporting these into the post.

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


16 months ago

@desrosj
16 months ago

#15 @desrosj
16 months ago

  • Keywords 2nd-opinion needs-unit-tests removed

In 43547.patch:

  • Fixed inconsistency with tab vs space indentation.
  • Moved filter hook registration with the other privacy hooks.
  • Added missing break; on the final switch conditional.
  • Added unit tests.

Also, I checked the comments exporter, and that one was returning an empty array when a nothing was found, so that should also be enough for users.

@TZ Media That makes sense about other user meta.

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


16 months ago

#17 follow-up: @azaozz
16 months ago

Hmm, why do we export "user_status"? We don't export the comment status, right? Imho 0 or 1 is not "personal data" by a long shot :)

#18 in reply to: ↑ 17 @allendav
16 months ago

Replying to azaozz:

Hmm, why do we export "user_status"? We don't export the comment status, right? Imho 0 or 1 is not "personal data" by a long shot :)

I agree - it doesn't seem like personal data - i cannot use it to identify a person. Let's not export that piece of meta.

#19 @allendav
16 months ago

  • Keywords needs-refresh added

#20 @azaozz
16 months ago

In 43055:

Privacy: add user information to the personal data export file.

Props TZ-Media, desrosj.
See #43547.

#21 @azaozz
16 months ago

  • Keywords commit fixed-major added; needs-testing needs-refresh removed

This seems to work well, ready for merging :)

#22 @pento
16 months ago

  • Keywords needs-patch needs-unit-tests added; has-patch commit fixed-major removed

[43055] is causing unit test failures on Travis, and locally.

1) Tests_User::test_wp_user_personal_data_exporter
Failed asserting that 11 is identical to 12.

/srv/www/wordpress-develop/public_html/tests/phpunit/tests/user.php:1619

#23 @SergeyBiryukov
16 months ago

In 43116:

Privacy: Correct unit test for wp_user_personal_data_exporter() added in [43055].

user_status is not considered personal data, so the total number of exported user properties is 11.

See #43547.

#24 @SergeyBiryukov
16 months ago

  • Description modified (diff)

#25 @SergeyBiryukov
16 months ago

In 43117:

Privacy: add user information to the personal data export file.

Props TZ-Media, desrosj.
Merges [43055] and [43116] to the 4.9 branch.
See #43547.

#26 @SergeyBiryukov
16 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

#27 @desrosj
15 months ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

Note: See TracTickets for help on using tickets.