From 5524258da9bc1d62b1396d19c672d3a8335ffbc5 Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 15 Apr 2020 16:13:44 +0200 Subject: [PATCH] =?UTF-8?q?Fix=20=E2=80=9CEmail=20changed=E2=80=9D=20notif?= =?UTF-8?q?ication=20sometimes=20having=20wrong=20e-mail=20(#13475)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix “Email changed” notification sometimes having wrong e-mail Fixes #6778 The root of the issue is that `send_devise_notification` was called before the changes were properly commited to the database, causing the mailer to pick previous values if running too early. Devise's documentation provides guidance on how to handle that[1][2], however, I have found it to not be working, as the following happens, in that order: - `send_devise_notification` is called for the `email_changed` notification. In that case, `changed?` is false and `saved_changes?` is true, so if we use the former, we have the same issue. - the `after_commit` hook is called - `send_devise_notification` is called for the `confirmation_instructions` notification. In that case, `changed?` is still false, and `saved_changes?` still true, so if we use the latter, that second notification email is simply not going to be sent (as we would be queuing the notification *after* executing the after_commit hook). This is because it may be called from either an `after_update` or `after_commit` hook, the difference not being a call to `save` but the transaction actually being committed to the database. This may arguably be a bug in Devise, or Devise's notification. The proposed workaround is inspired by Devise's documentation but checks whether a transaction is open to make the call whether to immediately send the notification or defer it to the `after_commit` hook. [1]: https://www.rubydoc.info/github/plataformatec/devise/Devise%2FModels%2FAuthenticatable:send_devise_notification [2]: https://github.com/heartcombo/devise/blob/406915cb781e38255a30ad2a0609e33952b9ec50/lib/devise/models/authenticatable.rb#L133-L194 * Fix cases when sending notifications without changing the model * Defer sending if and only if in transaction including current record --- app/models/user.rb | 30 ++++++++++++++++++- app/views/user_mailer/email_changed.html.haml | 2 +- app/views/user_mailer/email_changed.text.erb | 2 +- 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 85ee5cd064..ae0cdf29d1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -98,6 +98,7 @@ class User < ApplicationRecord before_validation :sanitize_languages before_create :set_approved + after_commit :send_pending_devise_notifications # This avoids a deprecation warning from Rails 5.1 # It seems possible that a future release of devise-two-factor will @@ -306,11 +307,38 @@ class User < ApplicationRecord protected def send_devise_notification(notification, *args) - devise_mailer.send(notification, self, *args).deliver_later + # This method can be called in `after_update` and `after_commit` hooks, + # but we must make sure the mailer is actually called *after* commit, + # otherwise it may work on stale data. To do this, figure out if we are + # within a transaction. + if ActiveRecord::Base.connection.current_transaction.try(:records)&.include?(self) + pending_devise_notifications << [notification, args] + else + render_and_send_devise_message(notification, *args) + end end private + def send_pending_devise_notifications + pending_devise_notifications.each do |notification, args| + render_and_send_devise_message(notification, *args) + end + + # Empty the pending notifications array because the + # after_commit hook can be called multiple times which + # could cause multiple emails to be sent. + pending_devise_notifications.clear + end + + def pending_devise_notifications + @pending_devise_notifications ||= [] + end + + def render_and_send_devise_message(notification, *args) + devise_mailer.send(notification, self, *args).deliver_later + end + def set_approved self.approved = open_registrations? || valid_invitation? || external? end diff --git a/app/views/user_mailer/email_changed.html.haml b/app/views/user_mailer/email_changed.html.haml index 7e91e87ad1..96be8624d9 100644 --- a/app/views/user_mailer/email_changed.html.haml +++ b/app/views/user_mailer/email_changed.html.haml @@ -38,7 +38,7 @@ %table.input{ align: 'center', cellspacing: 0, cellpadding: 0 } %tbody %tr - %td= @resource.try(:unconfirmed_email) ? @resource.unconfirmed_email : @resource.email + %td= @resource.unconfirmed_email %table.email-table{ cellspacing: 0, cellpadding: 0 } %tbody diff --git a/app/views/user_mailer/email_changed.text.erb b/app/views/user_mailer/email_changed.text.erb index 345b16a2ce..2b58415f56 100644 --- a/app/views/user_mailer/email_changed.text.erb +++ b/app/views/user_mailer/email_changed.text.erb @@ -4,6 +4,6 @@ <%= t 'devise.mailer.email_changed.explanation' %> -<%= @resource.try(:unconfirmed_email) ? @resource.unconfirmed_email : @resource.email %> +<%= @resource.unconfirmed_email %> <%= t 'devise.mailer.email_changed.extra' %>