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

Initial confirmation email not being sent #2688

Closed
krsyoung opened this Issue Oct 16, 2013 · 23 comments

Comments

Projects
None yet
@krsyoung

krsyoung commented Oct 16, 2013

Per the documentation "Confirmation instructions are sent to the user email after creating a record and when manually requested by a new confirmation instruction request.". However, in my deployment I'm currently not receiving the confirmation emails on initial user creation. The manual request for a confirmation is working perfectly.

I am unsure if it is some side effect of a configuration that I have. I am able to resolve the issue by adding the following code change to send_confirmation_notification? to consider the unconfirmed_email as the email value is initially blank:

# lib/devise/models/confirmable.rb
def send_confirmation_notification?
    confirmation_required? && !@skip_confirmation_notification && (!self.email.blank? || !self.unconfirmed_email.blank?)
end

I'm running with Devise 3.1.1 and have the following config settings related to confirmations:

# config/devise.rb
config.allow_unconfirmed_access_for = 0.days
config.confirm_within = 3.days
config.reconfirmable = true

Curious if anybody else is seeing this issue or comments / thoughts on the implications of the code change I have made?

Thanks!

@josevalim

This comment has been minimized.

Member

josevalim commented Oct 16, 2013

Hrm, this is weird. The send_confirmation_notification? is just required when creating the user and at this point unconfirmed_email is not supposed to be set. Maybe you are setting the unconfirmed_email on creation when the email attribute is meant to be set?

@krsyoung

This comment has been minimized.

krsyoung commented Oct 16, 2013

Thanks for the ideas José. Basically at the moment it seems that the resource.save call in the below code segment is nulling out the email and setting the unconfirmed_email (unclear if this is intentional). I have not had luck finding where the underlying method is doing this.

# app/controllers/devise/registrations_controller.rb

 # POST /resource
  def create
    build_resource(sign_up_params)
    byebug # add in debug statement for tracing
    if resource.save
    byebug # add in debug statement for tracing
      if resource.active_for_authentication?
        set_flash_message :notice, :signed_up if is_navigational_format?
# [...snip...]

Here is the output of the debug (I have a Membership and Person associated with my User model -- hence the noise):

Started POST "/users?locale=en" for 127.0.0.1 at 2013-10-16 08:43:54 -0400
Processing by Devise::RegistrationsController#create as HTML
  Parameters: {"utf8"=>"✓", "authenticity_token"=>"........", "user"=>{"first_name"=>"Chris", "last_name"=>"Young", "email"=>"user@example.com", "password"=>"[FILTERED]", "password_confirmation"=>"[FILTERED]", "accepted_eula"=>"1"}, "commit"=>"Sign up", "locale"=>"en"}

[10, 19] in /Users/rails/.rbenv/versions/2.0.0-p247/lib/ruby/gems/2.0.0/gems/devise-3.1.1/app/controllers/devise/registrations_controller.rb
   10: 
   11:   # POST /resource
   12:   def create
   13:     build_resource(sign_up_params)
   14:     byebug
=> 15:     if resource.save
   16:     byebug
   17:       if resource.active_for_authentication?
   18:         set_flash_message :notice, :signed_up if is_navigational_format?
   19:         sign_up(resource_name, resource)

(byebug) sign_up_params
{"email"=>"user@example.com", "password"=>"password", "password_confirmation"=>"password", "first_name"=>"Chris", "last_name"=>"Young", "accepted_eula"=>"1"}
(byebug) resource
#<User id: nil, email: "user@example.com", first_name: "Chris", last_name: "Young", membership_id: nil, encrypted_password: "........", accepted_eula: true, reset_password_token: nil, reset_password_sent_at: nil, remember_created_at: nil, sign_in_count: 0, current_sign_in_at: nil, last_sign_in_at: nil, current_sign_in_ip: nil, last_sign_in_ip: nil, confirmation_token: nil, confirmed_at: nil, confirmation_sent_at: nil, unconfirmed_email: nil, person_id: nil, mediator_id: nil, authentication_token: nil, provider: nil, uid: nil, name: nil, oauth_token: nil, oauth_expires_at: nil, created_at: nil, updated_at: nil>
(byebug) resource.persisted?
false
(byebug) resource.valid?
  User Exists (0.4ms)  SELECT 1 AS one FROM `users` WHERE `users`.`email` = BINARY 'user@example.com' LIMIT 1
true
(byebug) n
   (0.4ms)  BEGIN
  CACHE (0.6ms)  SELECT 1 AS one FROM `users` WHERE `users`.`email` = BINARY 'user@example.com' LIMIT 1
  User Load (0.6ms)  SELECT `users`.* FROM `users` WHERE `users`.`authentication_token` = '........' LIMIT 1
  User Load (0.5ms)  SELECT `users`.* FROM `users` WHERE `users`.`confirmation_token` = '........' LIMIT 1
  SQL (9.4ms)  INSERT INTO `users` (`accepted_eula`, `authentication_token`, `confirmation_sent_at`, `confirmation_token`, `created_at`, `email`, `encrypted_password`, `first_name`, `last_name`, `updated_at`) VALUES (1, '.........', '2013-10-16 12:44:20', '........', '2013-10-16 12:44:19', 'user@example.com', '......', 'Chris', 'Young', '2013-10-16 12:44:19')
  SQL (0.5ms)  INSERT INTO `memberships` (`activated`, `created_at`, `current`, `features`, `level`, `updated_at`) VALUES ('2013-10-16 00:00:00', '2013-10-16 12:44:20', 1, '--- []\n', 'personal', '2013-10-16 12:44:20')
  SQL (0.6ms)  UPDATE `memberships` SET `user_id` = 1, `updated_at` = '2013-10-16 12:44:20', `features` = '--- []\n' WHERE `memberships`.`id` = 1
  SQL (1.3ms)  INSERT INTO `people` (`created_at`, `first_name`, `last_name`, `updated_at`) VALUES ('2013-10-16 12:44:20', 'Chris', 'Young', '2013-10-16 12:44:20')
  User Exists (0.6ms)  SELECT 1 AS one FROM `users` WHERE (`users`.`email` = BINARY 'user@example.com' AND `users`.`id` != 1) LIMIT 1
  User Load (0.6ms)  SELECT `users`.* FROM `users` WHERE `users`.`confirmation_token` = '........' LIMIT 1
  SQL (0.7ms)  UPDATE `users` SET `encrypted_password` = '.........', `first_name` = 'Chris', `last_name` = 'Young', `accepted_eula` = 1, `authentication_token` = '........', `created_at` = '2013-10-16 12:44:19', `updated_at` = '2013-10-16 12:44:19', `confirmation_token` = '........', `confirmation_sent_at` = '2013-10-16 12:44:20', `id` = 1, `person_id` = 1, `unconfirmed_email` = 'user@example.com' WHERE `users`.`id` = 1
   (1.4ms)  COMMIT

[12, 21] in /Users/rails/.rbenv/versions/2.0.0-p247/lib/ruby/gems/2.0.0/gems/devise-3.1.1/app/controllers/devise/registrations_controller.rb
   12:   def create
   13:     build_resource(sign_up_params)
   14:     byebug
   15:     if resource.save
   16:     byebug
=> 17:       if resource.active_for_authentication?
   18:         set_flash_message :notice, :signed_up if is_navigational_format?
   19:         sign_up(resource_name, resource)
   20:         respond_with resource, :location => after_sign_up_path_for(resource)
   21:       else

(byebug) resource.persisted?
true
(byebug) resource.valid?
false
(byebug) resource
#<User id: 1, email: "", first_name: "Chris", last_name: "Young", membership_id: nil, encrypted_password: ".....", accepted_eula: true, reset_password_token: nil, reset_password_sent_at: nil, remember_created_at: nil, sign_in_count: 0, current_sign_in_at: nil, last_sign_in_at: nil, current_sign_in_ip: nil, last_sign_in_ip: nil, confirmation_token: "......", confirmed_at: nil, confirmation_sent_at: "2013-10-16 12:44:20", unconfirmed_email: "user@example.com", person_id: 1, mediator_id: nil, authentication_token: "....", provider: nil, uid: nil, name: nil, oauth_token: nil, oauth_expires_at: nil, created_at: "2013-10-16 12:44:19", updated_at: "2013-10-16 12:44:19">
(byebug) n

[17, 26] in /Users/rails/.rbenv/versions/2.0.0-p247/lib/ruby/gems/2.0.0/gems/devise-3.1.1/app/controllers/devise/registrations_controller.rb
   17:       if resource.active_for_authentication?
   18:         set_flash_message :notice, :signed_up if is_navigational_format?
   19:         sign_up(resource_name, resource)
   20:         respond_with resource, :location => after_sign_up_path_for(resource)
   21:       else
=> 22:         set_flash_message :notice, :"signed_up_but_#{resource.inactive_message}" if is_navigational_format?
   23:         expire_session_data_after_sign_in!
   24:         respond_with resource, :location => after_inactive_sign_up_path_for(resource)
   25:       end
   26:     else
@krsyoung

This comment has been minimized.

krsyoung commented Oct 16, 2013

Figures as soon as I post I think I found the method which is doing the nulling. Basically if there is a pending email change it will update the unconfirmed email and then set the confirmed email to the previous email value (which in my case was blank as it is a new user):

# lib/devise/models/confirmable.rb
def postpone_email_change_until_confirmation_and_regenerate_confirmation_token
  @reconfirmation_required = true
   self.unconfirmed_email = self.email
   self.email = self.email_was
   generate_confirmation_token
end

Here is the debug session where we can see the email being updated to blank:

[229, 238] in /Users/rails/.rbenv/versions/2.0.0-p247/lib/ruby/gems/2.0.0/gems/devise-3.1.1/lib/devise/models/confirmable.rb
   229:         end
   230: 
   231:         def postpone_email_change_until_confirmation_and_regenerate_confirmation_token
   232:                 byebug
   233:           @reconfirmation_required = true
=> 234:           self.unconfirmed_email = self.email
   235:           self.email = self.email_was
   236:           generate_confirmation_token
   237:         end
   238: 

(byebug) self.email
"user@example.com"
(byebug) self.unconfirmed_email 
nil
(byebug) n

[230, 239] in /Users/rails/.rbenv/versions/2.0.0-p247/lib/ruby/gems/2.0.0/gems/devise-3.1.1/lib/devise/models/confirmable.rb
   230: 
   231:         def postpone_email_change_until_confirmation_and_regenerate_confirmation_token
   232:                 byebug
   233:           @reconfirmation_required = true
   234:           self.unconfirmed_email = self.email
=> 235:           self.email = self.email_was
   236:           generate_confirmation_token
   237:         end
   238: 
   239:         def postpone_email_change?

(byebug)  self.email
"user@example.com"
(byebug) self.unconfirmed_email 
"user@example.com"
(byebug) n

[231, 240] in /Users/rails/.rbenv/versions/2.0.0-p247/lib/ruby/gems/2.0.0/gems/devise-3.1.1/lib/devise/models/confirmable.rb
   231:         def postpone_email_change_until_confirmation_and_regenerate_confirmation_token
   232:                 byebug
   233:           @reconfirmation_required = true
   234:           self.unconfirmed_email = self.email
   235:           self.email = self.email_was
=> 236:           generate_confirmation_token
   237:         end
   238: 
   239:         def postpone_email_change?
   240:           postpone = self.class.reconfirmable && email_changed? && !@bypass_confirmation_postpone && !self.email.blank?

(byebug) self.email
""
(byebug)  self.unconfirmed_email 
"user@example.com"

With all of that said it does seem to me that these two pieces aren't playing nicely together. Either we would need the type of code introduced which I presented in the initial description (check for email or unconfirmed_email) or possibly modify the above code to only swap if there was a previous email value?

@josevalim

This comment has been minimized.

Member

josevalim commented Oct 18, 2013

Thanks for the detailed response. We could fix this but the big question is: why is unconfirmed_email being set when email was not even set yet? If you are creating an account, you should be setting email.

@krsyoung

This comment has been minimized.

krsyoung commented Oct 18, 2013

Hey José, completely agree. Base on the trace email is originally set and unconfirmed_email is unset, but the postpone_email_change_until_confirmation_and_regenerate_confirmation_token method is unsetting it (not sure if that would have been the intended function).

Here is the flow again that shows how unconfirmed_email is set and email is not set in the two commented code lines:

# lib/devise/models/confirmable.rb
def postpone_email_change_until_confirmation_and_regenerate_confirmation_token
  @reconfirmation_required = true
   self.unconfirmed_email = self.email # unconfirmed_email being set to correct email value
   self.email = self.email_was # email being unset because self.email_was is ""
   generate_confirmation_token
end

I'm also a little confused because I can't find self.email_was defined anywhere (at least not in the devise code base).

Later today I'll see if I can test how this flow works after a registration (i.e. for a regular registered user is self.email_was still "")

@dougvk

This comment has been minimized.

dougvk commented Oct 23, 2013

I'm getting an error along these lines as well

# lib/devise/models/confirmable.rb
opts = pending_reconfirmation? ? { :to => unconfirmed_email } : { }
send_devise_notification(:confirmation_instructions, @raw_confirmation_token, opts)

unconfirmed_email is nil on RegistrationsController#create so devise sends a notification with blank to headers.

@josevalim

This comment has been minimized.

Member

josevalim commented Oct 23, 2013

Hey folks, can you provide a way to reproduce this error? Like, a dummy/simple application pushed to github? :)

@dougvk

This comment has been minimized.

dougvk commented Oct 23, 2013

https://github.com/dougvk/devisedebug - let me know what you find out. basically sign up, and it will try to send an email (and fail, because unconfirmed_email == nil)

@dougvk

This comment has been minimized.

dougvk commented Oct 24, 2013

Did that help?
On Oct 23, 2013 3:34 AM, "José Valim" notifications@github.com wrote:

Hey folks, can you provide a way to reproduce this error? Like, a
dummy/simple application pushed to github? :)


Reply to this email directly or view it on GitHubhttps://github.com//issues/2688#issuecomment-26885590
.

@nashby

This comment has been minimized.

Collaborator

nashby commented Oct 24, 2013

@dougvk hmm, I can't reproduce it with your app. It's actually trying to send email without any exception...

@krsyoung

This comment has been minimized.

krsyoung commented Oct 30, 2013

I'm going to try to go another round with this specific issue this weekend to see if I can reproduce in a clean way (i.e. new project). My initial debug trace clearly shows the issue however if nobody else can reproduce seems like it must be something env specific.

For the time being my override to send_confirmation_notification? has been working like a charm.

@josevalim

This comment has been minimized.

Member

josevalim commented Nov 6, 2013

Closing this for now as we can't reproduce. If this pops up, please do open up a new issue or ping this one and we will reopen it! Thanks guys!

@josevalim josevalim closed this Nov 6, 2013

@lkol

This comment has been minimized.

lkol commented Jan 22, 2014

I was experiencing the same problem and was only able to resolve it by setting config.reconfirmable = falsefor now. If the field is set as true, the original confirmation email won't be sent.

@antho1404

This comment has been minimized.

antho1404 commented Aug 8, 2014

I got this problem too and I found the problem...

In fact the postpone_email_change_until_confirmation_and_regenerate_confirmation_token method is called on before the update and so if you update your model when you create it (like with a update_attribute ...) this callback will be trigger and will change the email and the emails will not be send because it's call on the after_create callback and your email will be changed.

So to fix this check everywhere in your code if you have an update_attribute(s) somewhere that can be called during the creation

@blythburgh

This comment has been minimized.

blythburgh commented Aug 17, 2014

I got the problem (adding a default random-but-unique color attribute for charting after create) and confirmed the cause here. Easy enough for me to postpone these additions until after confirm by moving it out of the model to the after_confirmation_path (I already subvert this action to call other set-up tasks before returning the redirect). Might be one for the wiki if its to be left as a gotcha?

@taiwoayanleye

This comment has been minimized.

taiwoayanleye commented Feb 10, 2015

Same here, @lkol's "config.reconfirmable = false" did resolve this, wondering what the implication might be?

@waleedarshad

This comment has been minimized.

waleedarshad commented Aug 4, 2015

Same issue. Is there anyone who is able resolve it?

@benmaher

This comment has been minimized.

benmaher commented Oct 1, 2015

Same issue with three solutions. I discovered it using devise_invitable and when sending a new invite I got the error:

An SMTP To address is required to send a message. Set the message smtp_envelope_to, to, cc, or bcc address.

After tracing through devise_invitable and devise, I determined it happened during if save(:validate => false) in the class instance method invite! in lib/devise_invitable/model.rb. After checking the callbacks I focused on the devise before_update callback postpone_email_change_until_confirmation_and_regenerate_confirmation_token.

By itself, this callback isn't an issue even with devise_invitable. As described by others, the problem occurs when another save happens during creation (typically within after_create, after_save, or after_commit). This will trigger the update flow and callbacks which leads to postpone_email_change_until_confirmation_and_regenerate_confirmation_token.

        def postpone_email_change_until_confirmation_and_regenerate_confirmation_token
          @reconfirmation_required = true
          self.unconfirmed_email = self.email
          self.email = self.email_was
          self.confirmation_token = nil
          generate_confirmation_token
        end

At this point, self.email = self.email_was sets email to "". self.email_was is part of ActiveModel::Dirty. Because the same model instance is being used, self.email_was returns "" - because email did not have a previous value when the record was created. Devise is assuming that the change in email is due to an update and not because of the creation. This assumption plays out here as it relies upon email_changed? (also from ActiveModel::Dirty) and if this new email should be reconfirmed.

        def postpone_email_change?
          postpone = self.class.reconfirmable && email_changed? && !@bypass_confirmation_postpone && self.email.present?
          @bypass_confirmation_postpone = false
          postpone
        end

The following user model code produces the issue. In this example, sign_in_count will be set 1 only if 0 and then saves.

class User < ActiveRecord::Base
...

  after_save :reveal_issue

  def reveal_issue
    if self.sign_in_count == 0
      self.sign_in_count == 1
      self.save
    end
  end

You can then verify it in the console.

Create user and see that email is present in the user instance:
user = User.new(email: "sim@ple.ton", password:"simpleton", password_confirmation: "simpleton")

Save user:
user.save

Check user and email is now "":
user

Check last user stored and email is present:
User.last

Obviously a trivial example, but I found the issue because I had an after_save that was updating the username to a default that had the user id as part of the username. This could only happen after the record was saved so I could use the assigned id.


The following solutions have been tested only so far that the model instance email is not set to "".

Solution 1

Use skip_reconfirmation! to bypass postpone_email_change_until_confirmation_and_regenerate_confirmation_token during this update. I prefer this and it works well.

class User < ActiveRecord::Base
...

  after_save :reveal_issue

  def reveal_issue
    if self.sign_in_count == 0
      self.skip_reconfirmation!
      self.sign_in_count == 1
      self.save
    end
  end

Solution 2

Make the updates after creation only if confirmation has occurred. This only works for after_save and after_commit. after_create only fires once upon creation so it would never execute the code.

class User < ActiveRecord::Base
...

  after_save :reveal_issue

  def reveal_issue
    if self.confirmed? && self.sign_in_count == 0
      self.sign_in_count == 1
      self.save
    end
  end

Solution 3

Force a record reload before making updates. This fulfills the devise assumption concerning the state of the instance though it is another database hit.

class User < ActiveRecord::Base
...

  after_save :reveal_issue

  def reveal_issue
    if self.sign_in_count == 0
      self.reload
      self.sign_in_count == 1
      self.save
    end
  end
@gfauredumont

This comment has been minimized.

gfauredumont commented Feb 22, 2016

Hi,

I have the same issue, that is not resolved by setting config.reconfirmable = false.
I can't help notice that this issue is closed though :(

@gfauredumont

This comment has been minimized.

gfauredumont commented Feb 22, 2016

For now, I have a hugly workaround:
In my User model, I added this:

class User < ActiveRecord::Base
  devise :confirmable

  after_create: force_mail_confirmation

  def force_mail_confirmation
    self.send_confirmation_instructions
  end
end

Works like a charm... except that I suspect a skip_confirmation call would work anymore :/

Any help is more than welcome

@CJBrew

This comment has been minimized.

CJBrew commented Aug 31, 2016

I'm having this issue too. And it's apparently because of DeviseTokenAuth, for me:

class User < ActiveRecord::Base
  devise :registerable, :confirmable, :database_authenticatable # (minimal set)

  include DeviseTokenAuth::Concerns::User
end

Removing the DeviseTokenAuth concerns solves this issue, as does placing it above the devise settings. However, I've currently not tested whether the tokens still work...

My fixed User class:

class User < ActiveRecord::Base
  include DeviseTokenAuth::Concerns::User

  devise :database_authenticatable, :registerable,
         :recoverable, :rememberable, :trackable, :validatable,
         :confirmable
end
@davidsilvasmith

This comment has been minimized.

davidsilvasmith commented Sep 28, 2016

Wow, thanks to @krsyoung, @antho1404, and @benmaher

I spent at least two hours debugging this yesterday and wondering why in the world email was empty.

Thanks for the self.skip_reconfirmation!, worked like a charm!

@ethagnawl

This comment has been minimized.

ethagnawl commented Sep 15, 2017

@CJBrew The default behavior is explicitly disabled by DeviseTokenAuth.

  # this must be done from the controller so that additional params
  # can be passed on from the client
  def send_confirmation_notification?
    false
  end

AFAICT, the "additional params" they're referring to is just confirm_success_url.

beauraF added a commit to recipyhq/recipy that referenced this issue Aug 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment