Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 19 commits into from
May 5, 2023
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
72417a5
Use add_filter instead of add_action on the sensei_send_emails filter
fjorgemota May 3, 2023
5baf174
Improve documentation of the sensei_send_emails filter
fjorgemota May 3, 2023
bf8d031
Use sensei_send_emails to determine whether to send e-mails from the …
fjorgemota May 3, 2023
fb44b01
Block wp_mail from being triggered for guest users
fjorgemota May 3, 2023
2bf8b94
Simplify condition to match addresses with name and e-mail
fjorgemota May 3, 2023
7844880
Remove early check for the $return value on the skip_wp_mail
fjorgemota May 3, 2023
3856ddb
Fix PHPDoc for the skip_wp_mail method
fjorgemota May 3, 2023
391db0c
Use short array notation in skip_wp_mail
fjorgemota May 3, 2023
73416d5
Make it also process headers that aren't raw
fjorgemota May 3, 2023
a37916a
Add Unit tests for the skip_wp_mail method
fjorgemota May 3, 2023
82538d8
Add changelog entry
fjorgemota May 3, 2023
b8d95cd
Use constants to save the email domain used for preview/guest user em…
fjorgemota May 3, 2023
a3697b3
Add static method to detect if an email should be blocked or not
fjorgemota May 3, 2023
86e6137
Use should_block_email in skip_wp_mail for Sensei_Guest_User
fjorgemota May 3, 2023
b3208eb
Add pre_wp_mail filter for preview users too
fjorgemota May 3, 2023
054302a
Fix PHPDoc for skip_wp_mail parameters and return type
fjorgemota May 3, 2023
6ac456b
Add tests for the preview user logic related to blocking wp_mail
fjorgemota May 3, 2023
520a4f5
Use short array notation on test classes for guest/preview users
fjorgemota May 3, 2023
18d73db
Fix comment in should_block_email to refer to temporary users
fjorgemota May 4, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/fix-email-guest-users
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fixed

Don't send e-mails to Guest/Preview users
10 changes: 5 additions & 5 deletions includes/class-sensei-emails.php
Original file line number Diff line number Diff line change
Expand Up @@ -167,16 +167,16 @@ function send( $to, $subject, $message, $headers = "Content-Type: text/html\r\n"
add_filter( 'wp_mail_from_name', array( $this, 'get_from_name' ) );
add_filter( 'wp_mail_content_type', array( $this, 'get_content_type' ) );

// Send
$send_email = true;

/**
* Filter Sensei's ability to send out emails.
*
* @since 1.8.0
* @param bool $send_email default true
* @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 ) ) {
Comment on lines +174 to +179
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


wp_mail( $to, $subject, $message, $headers, $attachments );

Expand Down
35 changes: 33 additions & 2 deletions includes/class-sensei-guest-user.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,15 @@ class Sensei_Guest_User {
*/
const LOGIN_PREFIX = 'sensei_guest_';

/**
* Email domain used for guest users.
*
* @since $$next-version$$
*
* @var string
*/
const EMAIL_DOMAIN = 'guest.senseilms';

/**
* Guest user id.
*
Expand Down Expand Up @@ -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 );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)


$this->create_guest_student_role_if_not_exists();

Expand Down Expand Up @@ -327,7 +337,7 @@ public static function create_guest_user() {
[
'user_pass' => wp_generate_password(),
'user_login' => $user_name,
'user_email' => $user_name . '@guest.senseilms',
'user_email' => $user_name . '@' . self::EMAIL_DOMAIN,
'display_name' => 'Guest Student ' . str_pad( $user_count, 3, '0', STR_PAD_LEFT ),
'role' => self::ROLE,
]
Expand Down Expand Up @@ -466,7 +476,28 @@ private function is_action( $field, $nonce ) {
*/
public function skip_sensei_email( $send_email ) {
return $this->is_current_user_guest() ? false : $send_email;
}

/**
* Prevent emails related to the guest user from being dispatched via wp_mail.
*
* @access private
* @since $$next-version$$
*
* @param bool|null $return Null if we should send the email, a boolean if not.
* @param array $atts Email attributes.
* @return bool|null Null if we should send the email, a boolean if not.
*/
public function skip_wp_mail( $return, $atts ) {
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;
}
Comment on lines +492 to +499
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

return $return;
}

/**
Expand Down
36 changes: 34 additions & 2 deletions includes/class-sensei-preview-user.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ class Sensei_Preview_User {
const SWITCH_ON_ACTION = 'sensei-preview-as-student';
const SWITCH_OFF_ACTION = 'sensei-exit-student-preview';

/**
* Email domain used for preview users.
*
* @since $$next-version$$
*
* @var string
*/
const EMAIL_DOMAIN = 'preview.senseilms';

/**
* Meta key for the associated preview user ID.
* Used to link the original teacher and the preview user, in both directions.
Expand Down Expand Up @@ -83,6 +92,7 @@ public function init() {
add_action( 'show_admin_bar', [ $this, 'show_admin_bar_to_preview_user' ], 90 );
add_action( 'admin_bar_menu', [ $this, 'add_user_switch_to_admin_bar' ], 90 );
add_filter( 'sensei_is_enrolled', [ $this, 'preview_user_always_enrolled' ], 90, 3 );
add_filter( 'pre_wp_mail', [ $this, 'skip_wp_mail' ], 10, 2 );

$this->create_role();
}
Expand All @@ -97,7 +107,7 @@ public function add_preview_user_filters() {
add_filter( 'map_meta_cap', [ $this, 'allow_post_preview' ], 10, 4 );
add_filter( 'pre_get_posts', [ $this, 'count_unpublished_lessons' ], 10 );
add_filter( 'sensei_notice', [ $this, 'hide_notices' ], 10, 1 );
add_action( 'sensei_send_emails', '__return_false' );
add_filter( 'sensei_send_emails', '__return_false' );

}

Expand Down Expand Up @@ -244,6 +254,28 @@ private function can_switch_to_preview_user( $course_id ) {
return Sensei_Course::can_current_user_edit_course( $course_id );
}

/**
* Prevent emails related to the preview user from being dispatched via wp_mail.
*
* @access private
* @since $$next-version$$
*
* @param bool|null $return Null if we should send the email, a boolean if not.
* @param array $atts Email attributes.
* @return bool|null Null if we should send the email, a boolean if not.
*/
public function skip_wp_mail( $return, $atts ) {
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;
}
Comment on lines +268 to +275
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

return $return;
}

/**
* Check if the request is for the given action.
*
Expand Down Expand Up @@ -272,7 +304,7 @@ private function create_preview_user( $course_id ) {
[
'user_pass' => wp_generate_password(),
'user_login' => $user_name,
'user_email' => $user_name . '@preview.senseilms',
'user_email' => $user_name . '@' . self::EMAIL_DOMAIN,
'display_name' => $display_name,
'last_name' => $display_name,
'role' => self::ROLE,
Expand Down
53 changes: 53 additions & 0 deletions includes/class-sensei-temporary-user.php
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,59 @@ public static function filter_learners_query( string $query ) {
);
}

/**
* 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 ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice, TIL!

// If this is an e-mail address for a guest user, don't send it.
return true;
}
}
return false;
}

/**
* Make sure temporary users are not counted.
* When the user has an ungraded quiz, they are still counted, since they will show up in the grading list, as per self::filter_sensei_activity.
Expand Down
24 changes: 16 additions & 8 deletions includes/internal/emails/class-email-sender.php
Original file line number Diff line number Diff line change
Expand Up @@ -116,14 +116,22 @@ public function send_email( $email_name, $replacements, $usage_tracking_type ) {
$replacements = apply_filters( 'sensei_email_replacements', $replacements, $email_name, $email_post, $this );

foreach ( $replacements as $recipient => $replacement ) {
wp_mail(
$recipient,
$this->get_email_subject( $email_post, $replacement ),
$this->get_email_body( $email_post, $replacement ),
$this->get_email_headers(),
null
);
sensei_log_event( 'email_send', [ 'type' => $usage_tracking_type ] );
$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 ] );
}
Comment on lines +119 to +134
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

Expand Down
Loading