Don't dispatch e-mails related to Temporary Users #6855

fjorgemota · 2023-05-03T02:16:47Z

Proposed Changes

Use add_filter instead of add_action for hooking on the sensei_send_emails hook;
Check on the sensei_send_emails hook before dispatching e-mail for the new e-mail project;
Block wp_mail from being triggered for any e-mail related to the guest/preview user or when the guest/preview user is logged in - The idea is to block third-party plugins from sending e-mails to these users;

Testing Instructions

Using a plugin build based on this branch's code, and a plugin to log calls to wp_mail (I recommend the email-log plugin):

Create a course thas has the Open Access setting enabled;
As an anonymous user, visit that course's page and "Take the Course". Make sure you can take the course as usual with the guest user created for you;
Make sure welcome e-mails (and any other transactional e-mail, actually) isn't triggered for guest users;
Now, as an administrator/teacher, visit the page of the course you just created and click on "Preview as Student" on the admin bar:
Make sure that no welcome e-mails are triggered for these guest users;
Also make sure that the added unit tests run as expected

Pre-Merge Checklist

codecov · 2023-05-03T02:26:54Z

Codecov Report

Merging #6855 (18d73db) into trunk (9b0597b) will increase coverage by 0.09%.
The diff coverage is 94.00%.

@@             Coverage Diff              @@
##              trunk    #6855      +/-   ##
============================================
+ Coverage     47.37%   47.47%   +0.09%     
- Complexity    10124    10142      +18     
============================================
  Files           499      499              
  Lines         35995    36017      +22     
  Branches        283      283              
============================================
+ Hits          17054    17098      +44     
+ Misses        18729    18707      -22     
  Partials        212      212

Impacted Files	Coverage Δ
includes/class-sensei-emails.php	`0.00% <0.00%> (ø)`
includes/class-sensei-guest-user.php	`88.33% <88.88%> (+2.49%)`	⬆️
includes/class-sensei-preview-user.php	`82.31% <88.88%> (+0.15%)`	⬆️
includes/class-sensei-temporary-user.php	`85.88% <100.00%> (+5.23%)`	⬆️
includes/internal/emails/class-email-sender.php	`98.64% <100.00%> (+0.01%)`	⬆️

... and 5 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76cf796...18d73db. Read the comment docs.

…new project

Or even for e-mails related to the guest_user

…ails

fjorgemota

Just a few comments to help with the review process. =)

fjorgemota · 2023-05-04T13:56:11Z

includes/internal/emails/class-email-sender.php

+			$subject = $this->get_email_subject( $email_post, $replacement );
+			$message = $this->get_email_body( $email_post, $replacement );
+
+			/*
+			 * For documentation of the filter check class-sensei-emails.php file.
+			 */
+			if ( apply_filters( 'sensei_send_emails', true, $recipient, $subject, $message ) ) {
+				wp_mail(
+					$recipient,
+					$subject,
+					$message,
+					$this->get_email_headers(),
+					null
+				);
+				sensei_log_event( 'email_send', [ 'type' => $usage_tracking_type ] );
+			}


This is the "real" fix for this issue.

Basically, the Email Sender added in a recent release wasn't using the filter sensei_send_emails, which was used by the guest/preview classes to block e-mails from...guest users.

fjorgemota · 2023-05-04T13:56:47Z

includes/class-sensei-emails.php

+		 * @param bool $send_email Whether to send the email or not.
+		 * @param mixed $to The email address(es) to send the email to.
+		 * @param mixed $subject The subject of the email.
+		 * @param mixed $message The message of the email.
 		 */
-		if ( apply_filters( 'sensei_send_emails', $send_email, $to, $subject, $message ) ) {
+		if ( apply_filters( 'sensei_send_emails', true, $to, $subject, $message ) ) {


This is just a small change to inline the variable and improve the PHPDoc documentation. It is quite an old filter. :)

fjorgemota · 2023-05-04T14:00:58Z

includes/class-sensei-temporary-user.php

+	/**
+	 * Detect if the e-mail attributes relate to an e-mail from a temporary user.
+	 *
+	 * @access private
+	 * @since  $$next-version$$
+	 *
+	 * @param array  $atts   Email attributes.
+	 * @param string $email_domain The email domain to search for.
+	 * @return boolean Whether to block the email or not.
+	 */
+	public static function should_block_email( $atts, $email_domain ) {
+		$emails = $atts['to'];
+		if ( ! is_array( $emails ) ) {
+			$emails = explode( ',', $emails );
+		}
+		if ( ! empty( $atts['headers'] ) ) {
+			$headers = $atts['headers'];
+			if ( ! is_array( $headers ) ) {
+				// Explode the headers out, so this function can take
+				// both string headers and an array of headers.
+				$temp_headers = explode( "\n", str_replace( "\r\n", "\n", $headers ) );
+			} else {
+				$temp_headers = $headers;
+			}
+			// If it's actually got contents.
+			if ( ! empty( $temp_headers ) ) {
+				foreach ( $temp_headers as $name => $content ) {
+					if ( is_int( $name ) && str_contains( $content, ':' ) ) {
+						list ( $name, $content) = explode( ':', trim( $content ), 2 );
+					}
+
+					// Cleanup crew.
+					$name    = trim( $name );
+					$content = trim( $content );
+
+					if ( in_array( strtolower( $name ), [ 'from', 'cc', 'bcc', 'reply-to' ], true ) ) {
+						$emails = array_merge( (array) $emails, explode( ',', $content ) );
+					}
+				}
+			}
+		}
+		foreach ( $emails as $address ) {
+			if ( preg_match( '/(.*)<(.+)>/', $address, $matches ) && count( $matches ) === 3 ) {
+				$address = $matches[2];
+			}
+			if ( str_ends_with( $address, '@' . $email_domain ) ) {
+				// If this is an e-mail address for a temporary user, don't send it.
+				return true;
+			}
+		}
+		return false;
+	}
+


This is primarily an attempt on blocking any third party plugins (maybe something related to automation?) from dispatching e-mails (related?) to the temporary user.

We basically look out at the $to, From, Cc , Bcc and Reply-To headers to look out for any e-mails used by temporary users, and, if there's any reference there, we consider the e-mail "blocked".

fjorgemota · 2023-05-04T14:02:05Z

includes/class-sensei-guest-user.php

+		if ( $this->is_current_user_guest() ) {
+			// If this e-mail is being dispatched while the current user is a guest, just... don't send it.
+			return false;
+		}
+		if ( Sensei_Temporary_User::should_block_email( $atts, self::EMAIL_DOMAIN ) ) {
+			// If this e-mail is being dispatched to a guest user, don't send it.
+			return false;
+		}


In this block, we're returning false so wp_mail returns false too. This is considered an "error". It might not be the most friendly way of doing this (for any third party developer debugging this), but I believe it is better than returning true and faking a possible success..

fjorgemota · 2023-05-04T14:02:25Z

includes/class-sensei-preview-user.php

+		if ( $this->is_preview_user_active() ) {
+			// If this e-mail is being dispatched while the current user is a previwe user, just... don't send it.
+			return false;
+		}
+		if ( Sensei_Temporary_User::should_block_email( $atts, self::EMAIL_DOMAIN ) ) {
+			// If this e-mail is being dispatched to a preview user, don't send it.
+			return false;
+		}


Similar to the Guest Users situation, in this block, we're returning false so wp_mail returns false too. This is considered an "error". It might not be the most friendly way of doing this (for any third party developer debugging this), but I believe it is better than returning true and faking a possible success..

fjorgemota · 2023-05-04T14:05:05Z

includes/class-sensei-guest-user.php

@@ -137,7 +146,8 @@ public function init() {
 		add_action( 'sensei_is_enrolled', [ $this, 'open_course_always_enrolled' ], 10, 3 );
 		add_action( 'sensei_can_access_course_content', [ $this, 'open_course_enable_course_access' ], 10, 2 );
 		add_action( 'sensei_can_user_manually_enrol', [ $this, 'open_course_user_can_manualy_enroll' ], 10, 2 );
-		add_action( 'sensei_send_emails', [ $this, 'skip_sensei_email' ] );
+		add_filter( 'sensei_send_emails', [ $this, 'skip_sensei_email' ] );
+		add_filter( 'pre_wp_mail', [ $this, 'skip_wp_mail' ], 10, 2 );


Notice that I decided to add a pre_wp_mail filter for guest users and one for preview users.

The idea is that the user might want - for some reason - to disable one or another. I'm not sure why someone would want to do that. But maybe someone can consider that preview users should receive e-mails as usual, while guest users shouldn't? Not sure.

I can quickly change that to a single pre_wp_mail filter, but, given that most of the logic is shared, for me it seems OK to leave as it is. :)

m1r0

Looks good and tests well.

Really appreciate the detailed explanation! 🙇

m1r0 · 2023-05-04T23:11:36Z

includes/class-sensei-temporary-user.php

+			if ( preg_match( '/(.*)<(.+)>/', $address, $matches ) && count( $matches ) === 3 ) {
+				$address = $matches[2];
+			}
+			if ( str_ends_with( $address, '@' . $email_domain ) ) {


Haha, the str_ends_with function was introduced in PHP 8.0 but it looks like the symfony/polyfill-php80 is working. Nice!

@m1ro Actually, even without symfony/polyfill-php80 this would work well.

The reason? WordPress also implements some polyfills for functions from PHP 8.0...since WP 5.9. Here's the polyfill for str_ends_with: https://github.com/WordPress/WordPress/blob/f2d315036b0d4bba651852b4d32ffdd58e97b2f1/wp-includes/compat.php#L467-L489 😄

Oh nice, TIL!

fjorgemota added the [Type] Bug label May 3, 2023

fjorgemota self-assigned this May 3, 2023

fjorgemota added 11 commits May 3, 2023 13:36

Use add_filter instead of add_action on the sensei_send_emails filter

72417a5

Improve documentation of the sensei_send_emails filter

5baf174

Use sensei_send_emails to determine whether to send e-mails from the …

bf8d031

…new project

Block wp_mail from being triggered for guest users

fb44b01

Or even for e-mails related to the guest_user

Simplify condition to match addresses with name and e-mail

2bf8b94

Remove early check for the $return value on the skip_wp_mail

7844880

Fix PHPDoc for the skip_wp_mail method

3856ddb

Use short array notation in skip_wp_mail

391db0c

Make it also process headers that aren't raw

73416d5

Add Unit tests for the skip_wp_mail method

a37916a

Add changelog entry

82538d8

fjorgemota force-pushed the fix/email-guest-users branch from b19f3be to 82538d8 Compare May 3, 2023 16:39

fjorgemota added 7 commits May 3, 2023 15:32

Use constants to save the email domain used for preview/guest user em…

b8d95cd

…ails

Add static method to detect if an email should be blocked or not

a3697b3

Use should_block_email in skip_wp_mail for Sensei_Guest_User

86e6137

Add pre_wp_mail filter for preview users too

b3208eb

Fix PHPDoc for skip_wp_mail parameters and return type

054302a

Add tests for the preview user logic related to blocking wp_mail

6ac456b

Use short array notation on test classes for guest/preview users

520a4f5

fjorgemota marked this pull request as ready for review May 3, 2023 20:09

fjorgemota requested review from a team May 3, 2023 20:10

Fix comment in should_block_email to refer to temporary users

18d73db

fjorgemota commented May 4, 2023

View reviewed changes

fjorgemota changed the title ~~Don't dispatch e-mails related to the Guest User~~ Don't dispatch e-mails related to Temporary Users May 4, 2023

m1r0 approved these changes May 4, 2023

View reviewed changes

fjorgemota merged commit e03b821 into trunk May 5, 2023

fjorgemota deleted the fix/email-guest-users branch May 5, 2023 01:17

fjorgemota mentioned this pull request May 9, 2023

Fix wp_mail still working for temporary users #6868

Merged

14 tasks

Imran92 added this to the 4.14.0 milestone May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't dispatch e-mails related to Temporary Users #6855

Don't dispatch e-mails related to Temporary Users #6855

fjorgemota commented May 3, 2023 •

edited

Loading

codecov bot commented May 3, 2023 •

edited

Loading

fjorgemota left a comment

fjorgemota May 4, 2023

fjorgemota May 4, 2023

fjorgemota May 4, 2023

fjorgemota May 4, 2023

fjorgemota May 4, 2023

fjorgemota May 4, 2023

m1r0 left a comment

m1r0 May 4, 2023

fjorgemota May 4, 2023

m1r0 May 5, 2023

Don't dispatch e-mails related to Temporary Users #6855

Don't dispatch e-mails related to Temporary Users #6855

Conversation

fjorgemota commented May 3, 2023 • edited Loading

Proposed Changes

Testing Instructions

Pre-Merge Checklist

codecov bot commented May 3, 2023 • edited Loading

Codecov Report

fjorgemota left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1r0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjorgemota commented May 3, 2023 •

edited

Loading

codecov bot commented May 3, 2023 •

edited

Loading