FIX: Confirm new email not sent for staff if email disabled with "non-staff" option (PR #10794)

See https://meta.discourse.org/t/email-address-change-confirmation-email-not-sent-but-every-other-notification-emails-are/165358

In short: with disable emails set to non-staff, email address change confirmation emails (those sent to the new address) are not sent for staff or admin members.

This was happening because we were looking up the staff user with the to_address of the email, but the to address was the new email address and thus the user could not be found. We didn’t need to do this anyway because we are passing the user into the Email::Sender class anyway.

GitHub

Hey @davidtaylorhq would you mind taking a look at this one? :slight_smile:

What happens if there are multiple EmailChangeRequest rows? I can imagine it happening like

Now there will be two EmailChangeRequest rows for test@example.com. One for admin1, and one for admin2. I believe find_by will always return the record with the lowest id, which may cause problems?

Wow :facepalm: did not even think of that. Thanks for pointing this out, will try think of something else. I am not 100% sure I need to do this, can maybe just add the email type to the list of allowed bypass types.

@davidtaylorhq I think this is fine now, I just skip looking up the EmailChangeRequest altogether, it is not necessary because @user is always present for UserEmails which the confirm email change one definitely falls under.

Let’s take a step back here. The text for the setting says:

Prevent Discourse from sending any kind of emails. Select ‘yes’ to disable emails for all users. Select ‘non-staff’ to disable emails for non-staff users only.

The operative word here is for. By the time we get to the Email::Sender, we don’t know who the instigator of this action is. We’re assuming that the person that we are sending the email to is the same as the person that is being served, but they aren’t necessarily the same, for example with invites.

We might assume that emails are either send by staff or they are automatic, and if they are automatic, @user is set. In which case, we should never lookup a user by the to address.

@danielwaterworth I am not sure what you would like me to do here? I don’t see why we wouldn’t look up the user by the to address to see if we are sending to a staff user IF the @user is not present? I would like to merge this particular PR because this process is currently broken.

When is that actually necessary?

@danielwaterworth there are a few places that call Sender.new and do not pass in a user:

app/jobs/regular/invite_email.rb Email::Sender.new(message, :invite).send
app/jobs/scheduled/version_check.rb Email::Sender.new(message, :new_version).send
app/jobs/regular/group_smtp_email.rb Email::Sender.new(message, :group_smtp).send                                                                                                                                               
app/jobs/regular/download_backup_email.rb Email::Sender.new(message, :download_backup_message).send                                                                                                                             
app/jobs/regular/admin_confirmation_email.rb Email::Sender.new(message, :admin_confirmation_message).send                                                                                                                       
app/jobs/regular/invite_password_instructions_email.rb Email::Sender.new(message, :invite_password_instructions).send

I am not sure if any of these warrant checking if the outgoing to address is an admin user? If not we could probably get rid of the finding User by to_address.

Let’s go through them in order:

  • For invites, I thought it was only staff that could send invites, but it’s TL2 and above. Checking the to_address does not make sense here. I think the proper behaviour is only allowing staff to send invites when disable emails is set to non-staff,
  • Version check emails are sent to the contact_email site setting. I wouldn’t expect these emails to stop even if the email was not associated with a staff account,
  • Group SMTP emails are tricky, I’m not sure what should happen,
  • Backups can only be triggered in the admin interface,
  • Admin confirmation emails are only sent to admins,
  • Invite password instructions emails are also tricky, it’s up for interpretation,

Hmm well I think I can just bypass the check for all of these using the BYPASS const except for invite password instructions, and make the change to the invite rules for staff sending. I think I’d rather make this second change in a new PR though because it’s more substantial. I’ll ping you once I’ve made the other changes.

cc @danielwaterworth

@danielwaterworth I’ve made the bypass types change, can you please take another look, and I will do a follow up PR for the send invite change.

Yes, I will look today.

I am approving this, because it fixes the problem, but I dislike BYPASS_DISABLE_TYPES, because it is giving the email sender knowledge about its callers, which is fragile and resists refactoring. I would prefer to see either:

  • the caller making the judgement on whether to send the email,
  • the caller passing the instigating user in so that the sender can make a less arbitrary decision,

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/email-address-change-confirmation-email-not-sent-but-every-other-notification-emails-are/165358/5