WordPress.org

Make WordPress Core

Opened 4 weeks ago

Last modified 11 days ago

#44379 new defect (bug)

GDPR filters should provide either $request or $request_id

Reported by: garrett-eclipse Owned by:
Milestone: 4.9.8 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch commit
Focuses: Cc:

Description (last modified by Otto42)

Hello,

While discussing #44376 and reviewing #44235 I realized that the GDPR email filters should provide either the $request object or $request_id so if users are trying to replace $email_data using the filter they can recreate some of it that relies on the request.

That affects these filters;

  • user_confirmed_action_email_content
  • user_request_action_email_subject

By supplying the request (id or object) in the filter then customizations can be made which insert request specific information, etc.

For instance if you want to replace the confirm_url with a custom one you would want the request information on that custom confirm_url so your custom interception function knows what request is being confirmed.

Hope that makes sense, Cheers

Attachments (1)

44379.diff (431 bytes) - added by desrosj 4 weeks ago.
Add missing $request to $email_data to match the inline documentation for the following filters.

Download all attachments as: .zip

Change History (14)

This ticket was mentioned in Slack in #meta by garrett-eclipse. View the logs.


4 weeks ago

#2 @Otto42
4 weeks ago

  • Description modified (diff)

#3 @garrett-eclipse
4 weeks ago

Depending on the results of #44314 more filters will need to be covered by this. The current patch on that ticket adds a new filter namely;
user_erasure_fulfillment_email_content

As well in my ticket I mentioned user_request_action_email_subject which will also need the request data but had meant user_request_action_email_content so both of them should be addressed.

#4 @desrosj
4 weeks ago

  • Focuses privacy removed
  • Keywords reporter-feedback added
  • Version changed from trunk to 4.9.6

@garrett-eclipse can you clarify a bit more. I see that both of those filters are already providing the $request object as an argument.

#5 follow-up: @garrett-eclipse
4 weeks ago

Hi @desrosj

I assume you're speaking on the mention of $request in the docblocks like here; https://github.com/WordPress/WordPress/blob/3ee58b55b14683af4c568bce1845c37408c88fbb/wp-includes/user.php#L3379

In actuality that's incorrect as the first object in $email_data is $request->email and not the $request or $request_id, and unless wp_get_user_request_data supports lookup via the email then it doesn't give you the request which is needed if you try to recreate say confirm_url in a similar fashion as it does now but by supplying a front-end page instead of wp-login.php;

		'confirm_url' => add_query_arg( array(
			'action'      => 'confirmaction',
			'request_id'  => $request_id,
			'confirm_key' => wp_generate_user_request_key( $request_id ),
		), site_url( 'wp-login.php' ) ),

*This stems from the request in #44376, currently he'd have to recreate the wp-login.php code for confirmaction in his own page but without the request available to these filters it's hard to generate a similar confirm_url.

So looking at the docblock again I think it was expected that the first item in the email_data array was supposed to be the request but it's not actually there. So either the filters should provide it as a param or it should be part of email_data.

Cheers

#6 @Otto42
4 weeks ago

Putting the request into the email data at https://core.trac.wordpress.org/browser/trunk/src/wp-includes/user.php#L3353 would seem to be the simplest way to solve the problem.

#7 @garrett-eclipse
4 weeks ago

Agreed @Otto42

That would introduce it for both user_request_action_email_content and user_request_action_email_subject.

Looking at both cases for user_confirmed_action_email_content looks like request is in the email_data, that filter changes in another ticket but uses the same email_data array so shouldn't change.

So I don't believe would need to be introduce anywhere aside from the one mentioned by @Otto42

Cheers

@desrosj
4 weeks ago

Add missing $request to $email_data to match the inline documentation for the following filters.

#8 in reply to: ↑ 5 @desrosj
4 weeks ago

  • Keywords has-patch added; reporter-feedback removed
  • Type changed from enhancement to defect (bug)

Replying to garrett-eclipse:

I assume you're speaking on the mention of $request in the docblocks like here; https://github.com/WordPress/WordPress/blob/3ee58b55b14683af4c568bce1845c37408c88fbb/wp-includes/user.php#L3379

Ah, yes. I see not that the request is not included. We should definitely add this. I am changing this to a bug because the documentation is inaccurate.

I also discovered it was difficult to replace the login URL working on #44353, which I detailed in 44353:comment:6. TL;DR: replacing site_url( 'wp-login.php' ) with wp_login_url() will allow plugins to change the login URL without having to rebuild the confirm request URL.

#9 @desrosj
4 weeks ago

  • Milestone changed from Awaiting Review to 4.9.7

#10 @garrett-eclipse
4 weeks ago

Thanks @desrosj for the update and good catch on switching to use wp_login_url

#11 @desrosj
4 weeks ago

  • Keywords commit added

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


3 weeks ago

#13 @ocean90
11 days ago

  • Milestone changed from 4.9.7 to 4.9.8

4.9.7 has been released, moving to next milestone.

Note: See TracTickets for help on using tickets.