#43438 closed enhancement (fixed)
Add filters and Ajax support for personal data export
Reported by: | azaozz | Owned by: | desrosj |
---|---|---|---|
Milestone: | 5.2 | Priority: | low |
Severity: | normal | Version: | 4.9.6 |
Component: | Privacy | Keywords: | has-unit-tests has-patch commit |
Focuses: | Cc: |
Description (last modified by )
It is a GDPR requirement that users should be able to export their private data from one site and (eventually) import/reuse it at another.
As discussed in Slack, this is the data the users entered themselves like name, email, nickname, etc. This is not automatically generated data about the user like ID, IP addresses, browser UA (for comments), etc.
All of this data is already available/visible on the user's profile screen. It also has to be downloadable in a common format like JSON.
Attachments (29)
Change History (126)
#3
@
7 years ago
A few us of at Automattic chatted about a personal data exporter design that could support WooCommerce and its extensions needs (and be applicable to core itself or any plugin)
In a nutshell, we eventually thought it would be best to try having plugins (and extensions) register a callback function(s) for the export of personal data, e.g. have a plugin do something like this):
add_filter( "wp_privacy_register_export_data_callback", array( $this, 'register_export_data_callback' ) ); function register_export_data_callback( $export_callbacks ) { $export_callbacks[] = array( 'slug' => 'stripe', 'plugin_friendly_name' => __( 'Stripe Payments for WooCommerce' ), 'callback' => array( $this, 'export_data' ) ); return $export_callbacks; }
The plugin’s actual data-exporting callback would then look something like this, accepting the search parameter (e.g. the email address to export for) and a page parameter (more about that below).
function export_data( $email_address, $page ) { // do something here to get the next page of data for the email address given return array( 'data' => array( // data goes here ), 'done' => true // or return false if we want to be called again for another page of data ); }
For now, we are thinking the data should be super simple, an array of name/value pairs, to keep the barrier to entry low – we could always add more things to the pairs later, e.g:
[ { name: “billing address order #1000”, value: “1444 Seattle Hill Rd” }, { name: “billing address order #998”, value: “17725 108th St SE” }, ]
Then, ajax on a core provided wp-admin page (or the REST API) could be used to fetch a list of all the registered callbacks (and their plugins), e.g. ala:
$registered_callbacks_array = apply_filter( “wp_privacy_register_plugin_export_data_callback”, array() );
and work with each plugin in the $registered_callbacks_array in turn calling its callback(s) to fetch how ever many pages of data that plugin decides to return for the email address of interest.
The web CLIENT would assemble the responses in memory (not the server) and then the user could do whatever they want with the data.
Thoughts?
#4
follow-up:
↓ 5
@
7 years ago
Instead of returning arrays, would it be a safer approach, to use objects, like in WooCommerce and adding items to cart dynamicly ?
like
$object->add_callback('plugin_slug','callback');
#5
in reply to:
↑ 4
@
7 years ago
Replying to jesperher:
Instead of returning arrays, would it be a safer approach, to use objects, like in WooCommerce and adding items to cart dynamicly ?
like
$object->add_callback('plugin_slug','callback');
Instead of using a filter to register our callback, we could certainly use a global method, like core does today with things like register_post_type.
I do appreciate what you proposed is not entirely dissimilar from the approach your GDPRWP project is using by having plugins call methods on an instance of GdprToolbox, but I don't think calling instance methods will be as familiar to your average plugin developer as using a filter or calling a global method.
What do you think?
#6
@
7 years ago
- Summary changed from Export registered user's private data on request to Export registered user's personal data on request
#7
follow-up:
↓ 8
@
7 years ago
@allendav The method of just using a filter, is not far away from my current code, and was infact the first thought of how to approach this.
I can change my code to do the filter/array method istead but i am hitting an dead end. And can see it still will be an issue even when using the filter/array method.
when we are calling 1 callback at a time, the $variabel who contains all the callbacks, from the
´wp_privacy_register_export_data_callback´ filter.
when this is going into the javascript, the instance of the objects used to inform the callbacks dies.
example:
--Random plugin--
´´´
add_filter( "wp_privacy_register_export_data_callback", array( $this, 'register_export_data_callback' ) );
function register_export_data_callback( $export_callbacks ) {
$export_callbacks[] = array(
'slug' => 'stripe',
'plugin_friendly_name' => ( 'Stripe Payments for WooCommerce' ),
'callback' => array( $this, 'export_data' )
);
return $export_callbacks;
}
$all_callbacks = apply_filters('wp_privacy_register_export_data_callback',array() );
localize_script.... $all_callbacs
´´´
The $this, object dies as it goes into the javascript.
if creating Ajax call, and sending the 'callback' => array( $this, 'export_data' )´ to REST endpoint, "$this" doesnt work anymore, and using the ´call_user_func´ wont work.
A "solution" (but not a good one) would be to build "$all_callbacks" in every callback, and call the specific callback by index.
Do you have any thought on how to get around this issue.?
@
7 years ago
First pass at an extensible personal data exporter - includes core comments personal data export
#8
in reply to:
↑ 7
@
7 years ago
Replying to jesperher:
@allendav The method of just using a filter, is not far away from my current code, and was infact the first thought of how to approach this.
I can change my code to do the filter/array method istead but i am hitting an dead end. And can see it still will be an issue even when using the filter/array method.
when we are calling 1 callback at a time, the $variabel who contains all the callbacks, from the
´wp_privacy_register_export_data_callback´ filter.
when this is going into the javascript, the instance of the objects used to inform the callbacks dies.
A "solution" (but not a good one) would be to build "$all_callbacks" in every callback, and call the specific callback by index.
Do you have any thought on how to get around this issue.?
My patch attached is index based. The JS doesn't even need to really know about any particular callback other than how many there are.
What do you think?
#9
@
7 years ago
- OK, to try my patch, you'll need a site where someone (anyone) has left at least one comment (the more the merrier)
- Then, go wp-admin > Tools > Privacy
- Enter the commentor's username or email address in the box provided
- Hit the button to start the export
- You should see a table of all the personal data from their comments assemble
edit: next steps - expand coverage to usermeta and options, come to agreement on what to do with the export (i.e. making it email-able to the data subject, downloadable, how to handle things like images)
edit: edit: this patch will work against revision 42806 (i will update shortly to account for the merge of 43435 in revision 42814)
#10
follow-up:
↓ 11
@
7 years ago
Few initial points on 43438.2.diff:
- The admin-ajax endpoint will need a nonce check and
current_user_can()
check. - Think that we should have a specific DB query for this export, something like
SELECT * FROM {$wpdb->comments} WHERE comment_author_email = '[email]' LIMIT 10000
. Then can pick only the unique values from that, no need to repeat the same data all the time. Also, that DB query can possibly be more complex so we get only the unique data out of the DB. - It's using AJAX but overwrites the whole page. That is somewhat unexpected and can be confusing. IMHO better to just show it inline. Can move the PHP bits in the AJAX endpoint and just send the user_name or email to get it started.
@
7 years ago
Added Ajax referrer check; current_user_can check; only get comments with email match; break comments up into batches of 100; coding standards
#11
in reply to:
↑ 10
@
7 years ago
- Keywords has-patch added; needs-patch removed
Replying to azaozz:
Few initial points on 43438.2.diff:
Thank you so much for the fast turn around!
- The admin-ajax endpoint will need a nonce check and
current_user_can()
check.
Done!
- Think that we should have a specific DB query for this export, something like
SELECT * FROM {$wpdb->comments} WHERE comment_author_email = '[email]' LIMIT 10000
. Then can pick only the unique values from that, no need to repeat the same data all the time. Also, that DB query can possibly be more complex so we get only the unique data out of the DB.
Done - instead of custom query, get_comments was up to the task. I set the limit to 100 per page - let me know if you want something higher. 10000 seemed uncomfortably high :)
- It's using AJAX but overwrites the whole page. That is somewhat unexpected and can be confusing. IMHO better to just show it inline. Can move the PHP bits in the AJAX endpoint and just send the user_name or email to get it started.
Yep - I didn't make any changes here - this is not meant to be the final UX - I'd like us to all discuss what we want to do with the gathered data and if name/value is going to work in general.
#12
follow-up:
↓ 13
@
7 years ago
src/wp-admin/includes/ajax-actions.php
$exporters = apply_filters( 'wp_privacy_personal_data_exporters', array() );
This line/filter will be called once pr callback added to it. + once for counting how big an index we need.
so for each callback, we are calling all plugins and asking witch callbacks we need. Although the function here is asynchronous, I imagine it can be a heavy process. is there a way that we can only get the retrieved callbacks once, instead of n-times?
#13
in reply to:
↑ 12
@
7 years ago
Replying to jesperher:
src/wp-admin/includes/ajax-actions.php$exporters = apply_filters( 'wp_privacy_personal_data_exporters', array() );
This line/filter will be called once pr callback added to it. + once for counting how big an index we need.
so for each callback, we are calling all plugins and asking witch callbacks we need. Although the function here is asynchronous, I imagine it can be a heavy process. is there a way that we can only get the retrieved callbacks once, instead of n-times?
I don't think the add_filter for each plugin will be heavy - it really just needs to add its callback and do no other processing. The callback isn't called when the filter is added. It is not that different from how custom post types are registered by every plugin on every page load
Does that make sense?
#14
follow-up:
↓ 17
@
7 years ago
There have been a few questions about why User Agent is personal data. I think we should include it because combined with IP address, you can probably identify a single device in a home or business and possibly then identify a user.
This also aligns with the definitions in Article 4(1) of the GDPR https://gdpr-info.eu/art-4-gdpr/
#15
follow-up:
↓ 18
@
7 years ago
I have an idea for cutting down on the repetition in the export - we drop the identifier from the name (making it not specific to any one comment, etc) and then collect all the "findings" in the final export with numbers indicating where there is more than one finding with that value, e.g. see images attached immediately above.
#16
@
7 years ago
Question to those watching this ticket - what sorts of non-string-able personal data do you need export to handle - in the examples above we have an image which is thankfully URL reference-able - is there anything should we support at the get-go?
#17
in reply to:
↑ 14
@
7 years ago
Replying to allendav:
There have been a few questions about why User Agent is personal data. I think we should include it because combined with IP address, you can probably identify a single device in a home or business and possibly then identify a user.
You may be able to identify a device (with high probability of errors), but you cannot identify a person.
This also aligns with the definitions in Article 4(1) of the GDPR https://gdpr-info.eu/art-4-gdpr/
Right, the text there is:
...an identifiable natural person is one who can be identified, directly or indirectly, in particular by reference to an identifier such as a name, an identification number, location data, an online identifier or to one or more factors specific to the physical, physiological, genetic, mental, economic, cultural or social identity of that natural person;
The things we are looking for here are:
- a name
- an identification number
- location data
- an online identifier
of that natural person.
Where is the identification of a software a person may or may not have used listed there? :)
In any case, thinking we need to clear that with a lawyer.
For the time being I agree we should probably err on the side of "safety" and include the browser UA.
#18
in reply to:
↑ 15
;
follow-up:
↓ 23
@
7 years ago
Replying to allendav:
Yep, the screenshots look a lot better now, not even sure we need the count after the different bits of data.
Also thinking we can perhaps remove the "Provider" column. This is showing the personal data for a user of a particular website. There is no difference which plugin stores the data, the important part is that the data is on the site and they get to see and download it.
For getting only unique data from the comments query in 43438.3.diff I think we can try something like:
$name = $url = $ip = $ua = array(); // we know the email is unique foreach ( $comments as $comment ) { if ( ! in_array( $comment->comment_author, $name ) ) { $name[] = $comment->comment_author; } if ( ! in_array( $comment->comment_author_url, $url ) ) { $url[] = $comment->comment_author_url; } .....
or alternatively just do:
foreach ( $comments as $comment ) { $name[ $comment->comment_author ] = 1; $url[ $comment->comment_author_url ] = 1; ..... } $name = array_filter( array_keys( $name ) ); ....
Maybe 10000 is too much but this way we can easily process 5000-6000 comment rows in one go and get only the unique data. The idea is to get all of the user's comments at once and not have to merge these arrays later in js.
#19
follow-up:
↓ 25
@
7 years ago
So since the actual content of a post or comment does not qualify as user data, and I agree it should not, what happens when that data contains an email address for example? Might be worth looking into excepts and post content to make sure no personal information is present there and then display that?
#20
follow-up:
↓ 24
@
7 years ago
@allendav
$email_address = trim( strtolower( $email_address ) );
Why don't we do this before passing to the data exporter? Also do we need mb_ function here for special chars?
#21
@
7 years ago
- Summary changed from Export registered user's personal data on request to Add filters and Ajax support for collecting personal data
Re-focusing this ticket to just "Add filters and Ajax support for personal data export" per ticket scrub discussion in Making WordPress gdpr-compliance chat today.
#22
@
7 years ago
- Summary changed from Add filters and Ajax support for collecting personal data to Add filters and Ajax support for personal data export
#23
in reply to:
↑ 18
@
7 years ago
Replying to azaozz:
Replying to allendav:
Yep, the screenshots look a lot better now, not even sure we need the count after the different bits of data.
Also thinking we can perhaps remove the "Provider" column. This is showing the personal data for a user of a particular website. There is no difference which plugin stores the data, the important part is that the data is on the site and they get to see and download it.
Agreed.
For getting only unique data from the comments query in 43438.3.diff I think we can try something like:
$name = $url = $ip = $ua = array(); // we know the email is unique foreach ( $comments as $comment ) { if ( ! in_array( $comment->comment_author, $name ) ) { $name[] = $comment->comment_author; } if ( ! in_array( $comment->comment_author_url, $url ) ) { $url[] = $comment->comment_author_url; } .....or alternatively just do:
foreach ( $comments as $comment ) { $name[ $comment->comment_author ] = 1; $url[ $comment->comment_author_url ] = 1; ..... } $name = array_filter( array_keys( $name ) ); ....Maybe 10000 is too much but this way we can easily process 5000-6000 comment rows in one go and get only the unique data. The idea is to get all of the user's comments at once and not have to merge these arrays later in js.
Agreed. I am going to split out the comments into #43440 so we'll pick this up over there.
#24
in reply to:
↑ 20
@
7 years ago
Replying to mikejolley:
@allendav
$email_address = trim( strtolower( $email_address ) );
Why don't we do this before passing to the data exporter? Also do we need mb_ function here for special chars?
Agreed. Good catch. Also, replaced strtolower with mb_strtolower
#25
in reply to:
↑ 19
@
7 years ago
Replying to Kloon:
So since the actual content of a post or comment does not qualify as user data, and I agree it should not, what happens when that data contains an email address for example? Might be worth looking into excepts and post content to make sure no personal information is present there and then display that?
Not a bad idea. At the very least, we could look for the email address in the the comment content I suppose? Since we're breaking the comment exporter out of this ticket, let's move this discussion over to #43440
#27
follow-up:
↓ 28
@
7 years ago
@allendav Working great, I've got an implementation going for WooCommerce now.
Just one bug. The callback is passed $page
but it’s a string, not an int. Can you cast it?
#28
in reply to:
↑ 27
@
7 years ago
Replying to mikejolley:
@allendav Working great, I've got an implementation going for WooCommerce now.
Just one bug. The callback is passed
$page
but it’s a string, not an int. Can you cast it?
Awesome! Yes - can do - I'm going to cast exporter index too, and sanitize_text_field all the post vars for good measure.
#29
@
7 years ago
Hang on - one more tweak needed - I want wp_privacy_personal_data_export_page
to be a filter, not an action, so we can use it to return the assembled ZIP file url in the response on the last loop in #43551 - will post a tweaked patch in a sec
#30
follow-up:
↓ 31
@
7 years ago
Note: It is possible that we will need to export not just obvious unique personal data for a user (e.g. author_email) from objects like comments, but every "field" in a comment that user made. If so, this means that the name->value approach would result in extremely large files since we 1) wouldn't be able to de-duplicate since 2) we'd probably keep each exported comment object "intact" (i.e. with all its fields together, so the export made more sense.)
Hope to have an update shortly from discussion with stakeholders.
cc @azaozz
#31
in reply to:
↑ 30
@
7 years ago
Replying to allendav:
Note: It is possible that we will need to export ... every "field" in a comment that user made.
If that's the case we are doing it all wrong :) We'll need to "dump" most fields from the comments table from the database.
However the guidance for now is that the "personal data export" should include only "private personal data" like email, street address, IP address, etc. That does not include publicly published data as it is covered under other laws and perhaps other licenses, and is available on the internet.
It may be nice to have a way to export all comments by author email, but not sure if this is needed for law enforcement compliance.
#34
follow-ups:
↓ 35
↓ 39
@
7 years ago
- Keywords needs-patch added; has-patch removed
[42889] has a few issues which need to be addressed:
wp_ajax_wp_privacy_export_personal_data()
has no DocBlock.- The mbstring PHP extension might be not available, so
mb_strtolower()
needsfunction_exists()
check. Note thatis_email()
currently doesn't support email addresses with multibyte characters sostrtolower()
should be enough. - The docs for the
wp_privacy_personal_data_exporters
filter doesn't match the docs standards. - It seems like
$_POST['exporter']
is an numeric index of an exporter. Wouldn't it be easier if exporters are referenced by a name instead? - What's the idea behind the error messages? Should probably be translatable or a code instead.
- The nonce check should be enabled.
This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.
7 years ago
This ticket was mentioned in Slack in #gdpr-compliance by joemcgill. View the logs.
7 years ago
#38
@
7 years ago
- Milestone changed from 5.0 to 4.9.6
- Owner set to desrosj
- Status changed from new to assigned
Other functionality depends on this, so it needs to land in 4.9.6. Going to review this and address the remaining concerns.
#39
in reply to:
↑ 34
@
7 years ago
Replying to ocean90:
[42889] has a few issues which need to be addressed:
wp_ajax_wp_privacy_export_personal_data()
has no DocBlock.
A DocBlock was added in [43012].
- The mbstring PHP extension might be not available, so
mb_strtolower()
needsfunction_exists()
check. Note thatis_email()
currently doesn't support email addresses with multibyte characters sostrtolower()
should be enough.
The mb_strtolower()
call was removed in [43012] when the email was switched to use the email property of a WP_User_Request
object. I looked at that, though, and it does not seem to account for any multibyte characters, so added strtolower()
back in 43438.7.diff.
- The docs for the
wp_privacy_personal_data_exporters
filter doesn't match the docs standards.
Fixed in 43438.7.diff.
- It seems like
$_POST['exporter']
is an numeric index of an exporter. Wouldn't it be easier if exporters are referenced by a name instead?
I agree with this. @allendav, @azaozz any objection? If not, a new ticket may be best for this, I think.
- What's the idea behind the error messages? Should probably be translatable or a code instead.
Ensured every error message is translatable in 43438.7.diff.
- The nonce check should be enabled.
Was also addressed in [43012].
#40
@
7 years ago
Looked at the email again, and I don't think a strtolower()
is needed at all. 43438.8.diff does not include that.
This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.
7 years ago
#42
@
7 years ago
The patch in 43438.9.diff:
- Adds checks if array keys are set in
$_POST
, else send an error, for these cases:
$request_id = (int) $_POST['id']; $exporter_index = (int) $_POST['exporter']; $eraser_index = (int) $_POST['eraser']; $page = (int) $_POST['page'];
- Adds order to multiple placeholders in translation strings. For example
%s, %d
to'%1$s, %2$d
. - Adds "translators:" comments.
- Removes
Error:
in error messages. We already have "An error occurred while attempting to erase/export personal data" displayed before the error message. Also for consistency, because not all error messages start withError:
. See error_response_without_Error_string_part.jpg. - Adjusts the doc block for the
wp_privacy_personal_data_export_page filter
- Adds the possible
WP_Error
to inline doc for$response
. - Removes the "zero-based" from the inline doc for
$page
.
- Adds the possible
- Adjusts the doc block for the
wp_privacy_personal_data_erasure_page
filter- Adds the possible
WP_Error
to inline doc for$response
. - Removes the "zero-based" from the inline doc for
$page
.
- Adds the possible
- Further adjustments to the Coding Standard.
#43
@
7 years ago
Further possible changes could be to adjust the error messages for more consistency.
Here are the possible error messages raised in wp_ajax_wp_privacy_export_personal_data()
for the WordPress Comments exporter, as an example:
- Missing request ID.
- Invalid request type.
- Invalid request ID.
- Invalid request.
- A valid email address must be given.
- Missing exporter index.
- Missing page index.
- An exporter has improperly used the registration filter.
- Exporter index cannot be negative.
- Exporter index out of range.
- Page index cannot be less than one.
- Expected an array describing the exporter at index 1.
- Exporter array at index 1 does not include a friendly name.
- Exporter does not include a callback: WordPress Comments.
- Exporter callback is not a valid callback: WordPress Comments.
- Expected response as an array from exporter: WordPress Comments.
- Expected data in response array from exporter: WordPress Comments.
- Expected data array in response array from exporter: WordPress Comments.
- Expected done (boolean) in response array from exporter: WordPress Comments.
Here are the possible error messages raised in wp_ajax_wp_privacy_erase_personal_data()
for the WordPress Comments exporter, as an example:
- Missing request ID.
- Invalid request type.
- Invalid request ID.
- Invalid request.
- A valid email address must be given.
- Missing exporter index.
- Missing page index.
- Eraser index cannot be negative.
- Eraser index out of range.
- Page index cannot be less than one.
- Expected an array describing the exporter at index 1.
- Eraser array at index 1 does not include a callback.
- Eraser callback at index 1 is not a valid callback.
- Eraser array at index 1 does not include a friendly name.
- Did not receive array from WordPress Comments eraser (index 1).
- Expected num_items_removed key in response array from WordPress Comments eraser (index 1).
- Expected num_items_retained key in response array from WordPress Comments eraser (index 1).
- Expected messages key in response array from WordPress Comments eraser (index 1).
- Expected messages key to reference an array in response array from WordPress Comments eraser (index 1).
- Expected done flag in response array from WordPress Comments eraser (index 1).
For example:
- Expected done (boolean) in response array from exporter: WordPress Comments.
- Expected done flag in response array from WordPress Comments eraser (index 1).
and
- Expected response as an array from exporter: WordPress Comments.
- Did not receive array from WordPress Comments eraser (index 1).
Since plugins can define their own friendly name, there's a possible confusion if the friendly name is a bad one like "response eraser". Then we have e.g.:
- Expected messages key in response array from response eraser eraser (index 1).
that is difficult to understand.
I wonder if we should use translatable quotes, so it would display like:
- Expected messages key in response array from "response eraser" eraser (index 1).
or keep the friendly name at last, like:
- Expected messages key in response array (index 1) from eraser: response eraser.
#44
@
7 years ago
An additional important thing here is that we need to sanitize/escape the exporter/eraser friendly name.
We don't want it to contain HTML and stuff, but currently it goes through.
Here's an example:
wp_send_json_error( sprintf( /* translators: %s: exporter friendly name */ __( 'Exporter does not include a callback: %s.' ), $exporter['exporter_friendly_name'] ) );
The wp_send_json_error()
is a wrapper for wp_send_json()
that uses wp_encode_json()
.
What comes to mind are esc_js()
, esc_html()
and wp_encode_json()
but I'm thinking sanitize_text_field()
on $exporter['exporter_friendly_name']
should do it here.
What do you think?
ps: I did some testing here:
Raw friendly name:
WordPress Comments " \' \ - & & <b>Hello</b> <script>alert(1)</script>
and here's the how the ajax error is displayed in the HTML table:
sanitize_text_field: ...<li>Exporter does not include a callback: WordPress Comments " ' \ - & & Hello.</li>... esc_html: ...<li>Exporter does not include a callback: WordPress Comments " ' \ - & & <b>Hello</b> <script>alert(1)</script>.</li>... esc_js: ...<li>Exporter does not include a callback: WordPress Comments " \' - & & <b>Hello</b> <script>alert(1)</script>.</li>... esc_html + sanitize_text_field: ...<li>Exporter does not include a callback: WordPress Comments " ' \ - & & Hello.</li>... esc_js + sanitize_text_field: ...<li>Exporter does not include a callback: WordPress Comments " \' - & & Hello.</li>... wp_json_encode: ...<li>Exporter does not include a callback: "WordPress Comments \" ' \\ - & & <b>Hello<\/b> <script>alert(1)<\/script>".</li></ul></div></td></tr></tbody></table></script></b></li>...
So I would now think that we should do (at least) esc_html
for the escaping and then decide if we want the sanitizing part.
#45
@
7 years ago
43438.10.diff adds escaping for the friendly exporter/eraser names.
This ticket was mentioned in Slack in #gdpr-compliance by iandunn. View the logs.
7 years ago
#48
@
7 years ago
43438.11.diff is a first pass for adding tests for the wp_ajax_wp_privacy_export_personal_data()
function.
This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.
7 years ago
This ticket was mentioned in Slack in #core by desrosj. View the logs.
7 years ago
This ticket was mentioned in Slack in #gdpr-compliance by allendav. View the logs.
7 years ago
This ticket was mentioned in Slack in #core by desrosj. View the logs.
7 years ago
#58
@
7 years ago
- Keywords has-unit-tests added; needs-unit-tests removed
43438.12.diff adds tests for the Ajax handlers:
wp_ajax_wp_privacy_erase_personal_data()
wp_ajax_wp_privacy_export_personal_data()
#59
@
7 years ago
- Priority changed from normal to low
Set priority to low for tickets that only need unit tests.
This ticket was mentioned in Slack in #core by desrosj. View the logs.
6 years ago
This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.
6 years ago
This ticket was mentioned in Slack in #gdpr-compliance by birgire. View the logs.
6 years ago
#65
@
6 years ago
- Keywords needs-refresh added
- Milestone changed from 4.9.8 to 4.9.7
- Version set to 4.9.6
Moving to 4.9.7 so that these unit tests can get added. Some thoughts on 43438.12.diff (mostly a check list of what is left):
wp_ajax_wp_privacy_export_personal_data()
tests:
- A test for
Invalid request ID.
scenario is missing. test_wp_ajax_wp_privacy_export_personal_data_should_error_when_current_user_missing_required_capability()
test's documentation is confusing. It says that the function should successfully send exporters response data when the user is an administrator, but it then sets the user as an Author, and tests that the response was not a success.- Missing tests for the
Invalid request type
scenario. - MIssing tests for the
A valid email address must be given.
scenario. - Missing tests for the
Missing exporter index.
scenario. - Missing tests for the
Missing page index.
scenario. - Missing tests for all of the errors indicating incorrect exporter usage.
wp_ajax_wp_privacy_erase_personal_data()
tests:
- This is also missing a large handful of error scenarios, as well as scenarios indicating incorrect erasure usage by plugins.
#66
@
6 years ago
@desrosj thanks, the ajax tests have a reputation of being slow, so it was a very reserved patch regarding the number of tests.
I haven't looked into it where that slowness might come from, if it's related to the usage of WP_Ajax_UnitTestCase
, _handleAjax()
or elsewhere.
I can try to add more tests and see how it affects the run time.
Then there's
this kind of approach that uses WP_UnitTestCase
with selected bits from the WP_Ajax_UnitTestCase
class setup. Wonder if this approach is faster.
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
6 years ago
#68
@
6 years ago
43438.13.diff updates the Tests_Ajax_PrivacyExportPersonalData
class with more test cases.
An update for Tests_Ajax_PrivacyErasePersonalData
is in progress.
#69
@
6 years ago
- Milestone changed from 4.9.7 to 4.9.8
4.9.7 has been released, moving to next milestone.
#70
@
6 years ago
- Keywords gdpr removed
Removing the GDPR keyword. This has been replaced by the new Privacy component and privacy focuses in Trac.
#72
@
6 years ago
- Keywords has-patch added; needs-refresh removed
- Change
@since
to4.9.9
. - Fix some docblock indentation and spelling/grammar issues.
- Moved
$this->_setRole( 'administrator' );
calls to thesetUp()
method. This code was in every method except one, and that one set the role toauthor
. - Added test method for when an incorrect request type is passed (export request sent for an erase request).
- Added missing test assertions for erase functionality.
- Moved the eraser friendly name check higher up in
wp_ajax_wp_privacy_erase_personal_data()
. - Include the eraser friendly name in more error messages to be more informative.
- Fixes an incorrect error message when attempting to erase personal data and the request is not the remove type.
When the user submitting the request does not have the correct capability, the error returned is Invalid request.
. Thinking that this should be changed to something more descriptive, such as Incorrect permissions.
.
I believe all potential scenarios in these two functions are now covered. @birgire if you could review, that would be great. I am wondering if we can cut down on some code repetition.
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
6 years ago
#74
@
6 years ago
43438.15.2.diff has a few small changes. (ignore 43438.15.diff, uploaded the patch wrong file).
- Make the test method names more precise.
- Remove
$_POST = array()
in thetearDown()
method (this is handled inparent::tearDown()
). - Add a test method in both classes to validate the correct error is encountered when a nonce does not properly validate.
- Add a test method validating a
WP_Error
returned by a data exporter callback is correctly passed as the response. - Consolidated repetitive code that unset exporter/eraser indexes and changed exporter/eraser callback values into one method each.
I think these tests are good to go.
#76
@
6 years ago
great @desrosj
Some minor adjustments in 43438.16.diff:
In both test classes:
- Fix a typo in the property inline documentation by replacing
$key_to_unset
to$new_callback_value
. - Fix a typo: Replace "An value to change the ..." to "A value to change the ..."
- Change for better readability from
'security' => 'not-a-valid-nonce'
to'security' => 'invalid-nonce'
intest_failure_with_invalid_nonce()
. - Adds an inline comment in
test_error_when_incorrect_request_type(): // Incorrect request type, expects 'remove_personal_data'/'export_personal_data'.
- Adds to the inline comment in
test_error_when_request_id_invalid(): // Invalid request I, less than 1.
Adds inline comments in the following Tests_Ajax_PrivacyErasePersonalData
methods:
test_error_when_missing_request_id(): // Missing request ID.
test_error_when_incorrect_request_type(): // Incorrect request type, expects 'remove_personal_data'.
test_error_when_invalid_email: // Invalid requester's email address.
test_error_when_missing_eraser_index: // Missing eraser index.
test_error_when_missing_page_index: // Missing page index.
---
PS: I wonder if we could simplify and dry it more with a helper method like _set_exporter_callback( $callback )
?
Example:
$this->_set_exporter_callback( 'callback_return_wp_error' ); $this->_make_ajax_call();
instead of:
$this->new_callback_value = array( $this, 'callback_return_wp_error' ); add_filter( 'wp_privacy_personal_data_exporters', array( $this, 'filter_exporter_callback_index_value' ), 20 ); $this->_make_ajax_call();
#78
follow-up:
↓ 79
@
6 years ago
Thanks, @birgire.
I made that code consolidation change in 43438.17.diff. I also fixed the @group ajax1
typo and fixed some typos in the erasure tests that incorrectly referred to exporting.
#79
in reply to:
↑ 78
@
6 years ago
Replying to desrosj:
Thanks, @birgire.
I made that code consolidation change in 43438.17.diff. I also fixed the
@group ajax1
typo and fixed some typos in the erasure tests that incorrectly referred to exporting.
We've both _unset_..._key()
and _erase_..._key()
. Should we use same naming format in both test classes?
Should we use protected/private for the underscore helper methods, that don't need public visibility?
@
6 years ago
Use _unset_..._key()
instead of _erase_..._key()
for consistency, use protected
visibility on helper methods.
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
6 years ago
#81
@
6 years ago
- Keywords commit added
43438.19.diff has minor adjustments that:
- Renames the
_erase_response_key()
method to_unset_response_key()
. - Changes the inline doc for the eraser's
$key_to_unset
to* An array key in the test eraser to unset.
- Changes
int he
toin the
in the inline doc for the_unset_exporter_key()
method.
@desrosj I mark it with commit.
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-privacy by webdevlaw. View the logs.
6 years ago
#85
follow-up:
↓ 87
@
6 years ago
- Keywords needs-refresh added; commit removed
Thanks @birgire this still applies cleanly and all tests run successfully.
Minor note, the translator comments are now invalid as the string placeholder is now %s
and not %d
.
/* translators: %d: array index */
*There's two instances of this.
Can probably just reuse this comment found earlier in the file;
/* translators: %s: exporter friendly name */
Also, It will need a refresh for version numbers once it gets milestoned.
This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.
6 years ago
#87
in reply to:
↑ 85
@
6 years ago
- Milestone changed from Future Release to 5.2
Let's move this into 5.2, @birgire are you able to refresh your patch to apply version numbers as well as address these points;
garrett-eclipse comment 85:
Minor note, the translator comments are now invalid as the string placeholder is now
%s
and not%d
.
/* translators: %d: array index */
*There's two instances of this.
Can probably just reuse this comment found earlier in the file;
/* translators: %s: exporter friendly name */
Appreciated
#88
@
6 years ago
The 43438-20.diff:
- Adds
test_success_when_no_items_to_erase()
- Adds
test_success_when_no_items_to_export()
- Adds
::
prefix to@cover
- Updates to
@since
5.2.0
- Updates the tests regarding the string change in #44833.
- Adjusts an assert in
test_success_when_current_user_has_required_capabilities()
to useself::$request_email
.
Thanks @garrett-eclipse, regarding the translators comments, do you mean these:
https://core.trac.wordpress.org/browser/tags/5.1/src/wp-admin/includes/ajax-actions.php#L4708
in wp_ajax_wp_privacy_erase_personal_data()
?
@
6 years ago
Updated invalid translator comments, and clarified a few others by defining which array index eraser/exporter
#89
@
6 years ago
- Keywords commit added; needs-refresh removed
Thanks @birgire for the refresh and additional tests cases. Everything seems to be working nicely and passes tests.
As to the translator comments I've uploaded 43438-21.diff to avoid wasting your time with that. It also places the eraser/exporter prefix on some other translator comments for the array index.
Moving this forward to commit and we can do a final review in Monday's Bug Scrub.
#92
@
6 years ago
The unit tests added in r44909 do not seem to work properly on multisite installation, as export_others_personal_data
and erase_others_personal_data
meta capabilities are mapped to manage_network
capability on multisite (only super admins have this cap by default).
The issue can be reproduced by running phpunit -c tests/phpunit/multisite.xml --group ajax
.
#93
@
6 years ago
- Keywords needs-testing added; commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
Thanks for catching this, @davidbinda. Your patch looks good at first glance. Tests can be committed during the beta period, so let's get this fixed after this Thursday.
These were able to sneak in because the ajax
group is not being tested for multisite on Travis or in any grunt tasks (such as grunt precommit
). I have opened #46567 to propose adding that so AJAX related multisite issues can be caught moving forward.
#94
@
6 years ago
Nice @davidbinda good catch. I ran the test before patching to see what I was dealing with and got 1 error and 46 failures with 42 of them being in the privacy component. Applying your patch and retesting I still have the 1 error but only 4 failures and none within privacy. That's awesome, thank you.
That error and failures will be dealt with in #46567 that @desrosj opened there, so I posted the remaining error and failures there.
As to the refreshed patch I just uploaded 43438.21.diff. It simply switches from the unnecessary yoda conditionals to just check is_multisite() which follows the current convention in core. *Also there was a minor spaces instead of tabs thing which is also corrected.
Cheers
P.S. @desrosj this is good to commit in my opinion but I left in needs-testing
so you can give it a once through.
#95
@
6 years ago
- Keywords commit added; needs-testing removed
43438.22.diff adds two test methods to ensure the right message is returned when the current user is not a network admin on multisite.
I think that a more common file extension for exports like this is XML, but it's nice to give both options.
Regarding generated data, I believe from your list, the only generated data is the user ID, but I can also identify something with that data.
Other examples like browser UA and IP addresses can also be used to identify an individual.