Fix “Email changed” notification sometimes having wrong e-mail (#13475)

* 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]: 406915cb78/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
This commit is contained in:
ThibG 2020-04-15 16:13:44 +02:00 committed by GitHub
parent 80c04b2819
commit 5524258da9
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 31 additions and 3 deletions

View file

@ -98,6 +98,7 @@ class User < ApplicationRecord
before_validation :sanitize_languages before_validation :sanitize_languages
before_create :set_approved before_create :set_approved
after_commit :send_pending_devise_notifications
# This avoids a deprecation warning from Rails 5.1 # This avoids a deprecation warning from Rails 5.1
# It seems possible that a future release of devise-two-factor will # It seems possible that a future release of devise-two-factor will
@ -306,11 +307,38 @@ class User < ApplicationRecord
protected protected
def send_devise_notification(notification, *args) 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 end
private 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 def set_approved
self.approved = open_registrations? || valid_invitation? || external? self.approved = open_registrations? || valid_invitation? || external?
end end

View file

@ -38,7 +38,7 @@
%table.input{ align: 'center', cellspacing: 0, cellpadding: 0 } %table.input{ align: 'center', cellspacing: 0, cellpadding: 0 }
%tbody %tbody
%tr %tr
%td= @resource.try(:unconfirmed_email) ? @resource.unconfirmed_email : @resource.email %td= @resource.unconfirmed_email
%table.email-table{ cellspacing: 0, cellpadding: 0 } %table.email-table{ cellspacing: 0, cellpadding: 0 }
%tbody %tbody

View file

@ -4,6 +4,6 @@
<%= t 'devise.mailer.email_changed.explanation' %> <%= t 'devise.mailer.email_changed.explanation' %>
<%= @resource.try(:unconfirmed_email) ? @resource.unconfirmed_email : @resource.email %> <%= @resource.unconfirmed_email %>
<%= t 'devise.mailer.email_changed.extra' %> <%= t 'devise.mailer.email_changed.extra' %>