Make WordPress Core

Opened 6 years ago

Last modified 6 years ago

#44500 new enhancement

Mark data requests failed when an expired link is clicked

Reported by: desrosj's profile desrosj Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch needs-testing has-unit-tests
Focuses: administration Cc:

Description

When a user data request expires (the user does not click the confirmation link within the specified timeframe), it is transitioned to the request-failed status.

Currently, data export/erasure requests are only marked as request-failed when an administrator visits the Export/Erase Personal Data pages (which will be pretty infrequent). #44498 aims to move transitioning of expired requests to request-failed with a cron, but a request could also be marked as expired when a user clicks an expired link.

Attachments (3)

44500.diff (2.0 KB) - added by desrosj 6 years ago.
44500.2.diff (10.9 KB) - added by desrosj 6 years ago.
44500.3.diff (15.9 KB) - added by desrosj 6 years ago.

Download all attachments as: .zip

Change History (11)

@desrosj
6 years ago

#1 @desrosj
6 years ago

  • Keywords needs-unit-tests added

44500.diff introduces wp_mark_user_request_failed() to transition user requests to request-failed and erase post passwords and utilizes it in _wp_personal_data_cleanup_requests() and when validating a confirmaction request in wp-login.php.

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


6 years ago

#3 @garrett-eclipse
6 years ago

  • Keywords needs-refresh added

Reviewing the patch @desrosj it doesn't appear there's any consideration for if the request is already completed and the user is visiting the link for a second time. I would suggest updating wp_mark_user_request_failed to check the state of the request and only fail it if it's in the Confirmed state or maybe Pending & Confirmed. If instead you suppress the update if in Completed state then we should also suppress if in archived state which is possible if #44222 moves forward.

@desrosj
6 years ago

#4 @desrosj
6 years ago

  • Focuses administration added
  • Keywords has-unit-tests added; needs-unit-tests needs-refresh removed
  • Milestone changed from Awaiting Review to Future Release

@garrett-eclipse in wp_validate_user_request_key(), there is a check that returns an error if the request is not pending or failed. I found a bug with that, though (see #44685). Once that is resolved, wp_mark_user_request_failed() will not be called by core in those scenarios.

That said, it won't handle situations where wp_mark_user_request_failed() is called directly by plugins. For those, I aded an additional WP_Error in wp_mark_user_request_failed() for when a request has already been completed. I did not add an error for attempting to set a confirmed request as failed. I could see a scenario where a plugin might want to mark a confirmed request as failed, maybe if the plugin generates and sends its own export file and it does not succeed.

44500.2.diff also has unit tests for wp_mark_user_request_failed() and the _wp_personal_data_cleanup_requests() function, the only place that calls the first function in core.

#5 @garrett-eclipse
6 years ago

Thanks @desrosj

First a few notes on 44500.2.diff;

  1. An extraneous string made its way into line 1131 of wp-admin/includes/user.php;

'he specified post is not a user_requ'

  1. I feel we can improve the language used on the user_request post-type check in wp_mark_user_request_failed to make it concise like the invalid request id one;

'Invalid request post type.'

I also took a look at wp_validate_user_request_key and the check that request status isn't request-pending or request-failed and wonder if it should live here as calling the function on a request-confirmed or request-completed would throw an error but even in those states the user request key is valid. This could cause confusion for plugin devs, etc as they may use it to check the validity of confirmed/completed request keys. So i wonder if that check should be moved from the function and called directly in the specific cases where that check is needed such as in the confirmaction check. That check could be made into it's own function. And just thinking of a core case where this may be an issue would be if the wp_ajax_wp_privacy_export_personal_data was updated to use wp_validate_user_request_key then it would fail to export as the request would be in the completed state.

And I tend to agree with the check for completed added to wp_mark_user_request_failed while not checking the confirmed as I feel your explained use case is valid.

Cheers

#6 @desrosj
6 years ago

Made adjustments based on your feedback, @garrett-eclipse.

Not opposed to a discussion about moving the status check out of the function, but there could be some backward compatibility issues there so we should discuss this in a separate ticket.

I don't know that a key would ever have to be re-validated, but I can see the potential for other flows that may want to validate a key. I have been drawing a parallel to the password reset process. An admin can change passwords without verification (user is notified, though). A user must confirm the request in an email. Once that request is confirmed, the key is erased and the process needs to be started over.

44500.3.diff also adds unit tests for wp_validate_user_request_key ().

@desrosj
6 years ago

#7 @birgire
6 years ago

Just a sidenote:

I was wondering about the wp_mark_user_request_failed() function, if there would be any benefit of generalizing it to e.g. wp_set_user_request_status() so it would not only support the failed status.

Similarly we have a function like wp_set_comment_status(), that sets the status of a comment and runs the wp_transition_comment_status() to call hooks for comment status transitions.

https://developer.wordpress.org/reference/functions/wp_set_comment_status/
https://developer.wordpress.org/reference/functions/wp_transition_comment_status/

Version 0, edited 6 years ago by birgire (next)

#8 @garrett-eclipse
6 years ago

Thanks @desrosj some more notes here.

Generalize User Request Status Transitions
I tend to agree with @birgire on a more generalized function instead of wp_mark_user_request_failed and now would be the time to do that as it's a new function. So we'd have a wp_set_user_request_status with a switch for the new status so when it's going to fail and is already completed that's blocked, and tie it with a wp_transition_user_request_status() for calling the necessary hooks which we'd want documented and unit tested.
The following are locations I found that this transition function would be used;
https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/includes/user.php#L633
https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/includes/user.php#L781
https://github.com/WordPress/WordPress/blob/7ee752e40cf47feb2591711efb8074217b6961f2/wp-includes/user.php#L2949
https://github.com/WordPress/WordPress/blob/7ee752e40cf47feb2591711efb8074217b6961f2/wp-includes/user.php#L3515

Minor Comments

  1. We've been referencing the user_request post as just user request so the comment 'Marks a user_request post as failed.' should be 'Marks a user request as failed.'
  2. The get_post method can return null which isn't a boolean so should be instead checking for if ( ! empty( $post ) )
  3. You missed a space after the opening brace here } elseif ('user_request' !== $post->post_type ) {
  4. Is there a reason we're setting post_status to an empty string?

Rewriting wp_validate_user_request_key
Aside from the note to pull the status check out of this method we may want to revisit and follow the convention of the wp_validate_auth_cookie function which returns false or an int instead of WP_Error and instead fires actions for all the errors before returning false. By doing this we follow an existing convention and then the function can be called as a boolean without the is_wp_error check as well the call to mark request failed when expired can be done through a separate function attached to the action that's thrown in that case.
The wp_validate_auth_cookie function - https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-includes/pluggable.php#L602

These might deserve extra tickets but wanted to discuss further before spawning any.

Note: See TracTickets for help on using tickets.