Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Confirmation request on email change added #1120

Closed
wants to merge 6 commits into from

4 participants

Mandaryn Rainer Keller Brian Rose vkeziah
Mandaryn

This should handle the case when user changes the email. Before it was updated with no confirmation, now the update is pending untill user confirms the new email.

Rainer Keller

In this line:
... +reconfirmable+: requires any email changes to be confirmed (exctly the same way as
'exctly' should be 'exactly'

But I really have a bigger question:
If I understand the code correctly this will not allow me to change an unconfirmed_email until it is confirmed. That seems a bit undesirable to me: If I want to change my email and I made a simple typo, why would I not be allowed to fix the typo? I would assume that just a new confirmation email be sent out to the revised (unconfirmed) email address and I can confirm that one.

I can see that my suggestion could be used to spam, but I am checking password when I allow the user to change the email.

Would it be possible to make the 'prevent_email_change' logic optional? Or am I missing another security hole that would get introduced by this?

Rainer Keller

Sorry. I just got confused by the naming 'prevent_email_change'. Now I stepped through the code and it is working just how I need it.
Perhaps a better name would be 'postpone_email_change_until_confirmation'?
prevent_email_change sounds so prohibiting...

Rainer Keller
rke commented

Hmmm, but the way the code is currently it sends confirmation emails to the old email address. For me at least the confirmations serve the purpose that I know that an email address is valid, so I would expect them to go to the new address. Perhaps also a notification to the old one, but the main concern is to confirm the new one.
Also if I change the email twice (without confirming the first change) then record.email (not unconfirmed_email) is overwritten even without an accepted confirmation email.

Mandaryn

Shame on me, now I really wonder how the hell did I screw this so much... It's really embarrassing, will try fixing ASAP

Mandaryn Mandaryn closed this
Mandaryn

Shame on me, now I really wonder how the hell did I screw this so much... It's really embarrassing, will try fixing ASAP

Rainer Keller

Not at all! Your code was extremely helpful to me!
I had to hack a bit because the project I am working on was using devise 1.2.1, but you can look at my patch file for that slightly older devise version and how I got it to do what I needed. I am very busy atm so not sure that I can clean it up for a pull request myself but in case it might help I put my patch in git://gist.github.com/1083882.git
Thanks for your code!

Brian Rose

Hey Mandaryn, did you get a chance to update this code at all? I'd love to see this feature implemented.

Brian Rose

I've created a new pull request with updated code located here: #1266

vkeziah

I had a problem with reconfirmable, I have set up everything to go with reconfirmable and when I change email address it tries to send email i.e I could see on rails console that devise send email to changed email , but the problem is I am not getting email at all , this is only in the case of reconfirmable , in all remaining cases (signup,password recover) I am getting the email .

I am not sure what is the (since its not raising any errors) , how to fix this ?

and I checked my db , devise is setting the unconfirmed_email value too !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 11, 2011
  1. Mandaryn
  2. Mandaryn
Commits on Apr 17, 2011
  1. Mandaryn

    renamed confirmation_on_email_change property to reconfirmable and ad…

    Mandaryn authored
    …ded reconfirmable explanations
  2. Mandaryn
  3. Mandaryn
Commits on Jun 8, 2011
  1. Mandaryn

    Merge remote-tracking branch 'upstream/master'

    Mandaryn authored
    Conflicts:
    	lib/devise/models/validatable.rb
    	test/models/validatable_test.rb
This page is out of date. Refresh to see the latest.
2  app/views/devise/mailer/confirmation_instructions.html.erb
View
@@ -1,5 +1,5 @@
<p>Welcome <%= @resource.email %>!</p>
-<p>You can confirm your account through the link below:</p>
+<p>You can confirm your account email through the link below:</p>
<p><%= link_to 'Confirm my account', confirmation_url(@resource, :confirmation_token => @resource.confirmation_token) %></p>
3  lib/devise.rb
View
@@ -131,6 +131,9 @@ module Strategies
mattr_accessor :confirmation_keys
@@confirmation_keys = [ :email ]
+ mattr_accessor :reconfirmable
+ @@reconfirmable = false
+
# Time interval to timeout the user session without activity.
mattr_accessor :timeout_in
@@timeout_in = 30.minutes
79 lib/devise/models/confirmable.rb
View
@@ -14,6 +14,10 @@ module Models
# use this to let your user access some features of your application without
# confirming the account, but blocking it after a certain period (ie 7 days).
# By default confirm_within is zero, it means users always have to confirm to sign in.
+ # * +reconfirmable+: requires any email changes to be confirmed (exctly the same way as
+ # initial account confirmation) to be applied. Requires additional unconfirmed_email
+ # db field to be setup (see migrations). Until confirmed new email is stored in
+ # unconfirmed email column, and copied to email column on successful confirmation.
#
# == Examples
#
@@ -24,18 +28,50 @@ module Models
module Confirmable
extend ActiveSupport::Concern
+ # email uniqueness validation in unconfirmed_email column, works only if unconfirmed_email is defined on record
+ class ConfirmableValidator < ActiveModel::Validator
+ def validate(record)
+ if unconfirmed_email_defined?(record) && email_exists_in_unconfirmed_emails?(record)
+ record.errors.add(:email, :taken)
+ end
+ end
+
+ protected
+ def unconfirmed_email_defined?(record)
+ record.respond_to?(:unconfirmed_email)
+ end
+
+ def email_exists_in_unconfirmed_emails?(record)
+ query = record.class
+ unless record.new_record?
+ if record.respond_to?(:_id)
+ query = query.where(:_id => {'$ne' => record._id})
+ else
+ query = query.where('id <> ?', record.id)
+ end
+ end
+ query = query.where(:unconfirmed_email => record.email)
+ query.exists?
+ end
+ end
+
included do
before_create :generate_confirmation_token, :if => :confirmation_required?
after_create :send_confirmation_instructions, :if => :confirmation_required?
+ before_update :prevent_email_change, :if => :prevent_email_change?
+ after_update :send_confirmation_instructions, :if => :email_change_confirmation_required?
end
# Confirm a user by setting it's confirmed_at to actual time. If the user
- # is already confirmed, add en error to email field
+ # is already confirmed, add en error to email field. If the user is invalid
+ # add errors
def confirm!
unless_confirmed do
self.confirmation_token = nil
self.confirmed_at = Time.now
- save(:validate => false)
+ self.email = unconfirmed_email if unconfirmed_email.present?
+ self.unconfirmed_email = nil
+ save
end
end
@@ -46,6 +82,7 @@ def confirmed?
# Send confirmation instructions by email
def send_confirmation_instructions
+ @email_change_confirmation_required = false
generate_confirmation_token! if self.confirmation_token.nil?
::Devise.mailer.confirmation_instructions(self).deliver
end
@@ -104,10 +141,10 @@ def confirmation_period_valid?
confirmation_sent_at && confirmation_sent_at.utc >= self.class.confirm_within.ago
end
- # Checks whether the record is confirmed or not, yielding to the block
+ # Checks whether the record is confirmed or not or a new email has been added, yielding to the block
# if it's already confirmed, otherwise adds an error to email.
def unless_confirmed
- unless confirmed?
+ unless confirmed? && unconfirmed_email.blank?
yield
else
self.errors.add(:email, :already_confirmed)
@@ -118,7 +155,6 @@ def unless_confirmed
# Generates a new random token for confirmation, and stores the time
# this token is being generated
def generate_confirmation_token
- self.confirmed_at = nil
self.confirmation_token = self.class.confirmation_token
self.confirmation_sent_at = Time.now.utc
end
@@ -127,13 +163,30 @@ def generate_confirmation_token!
generate_confirmation_token && save(:validate => false)
end
+ def prevent_email_change
+ @email_change_confirmation_required = true
+ self.unconfirmed_email = self.email
+ self.email = self.email_was
+ end
+
+ def prevent_email_change?
+ self.class.reconfirmable && email_changed? && email != unconfirmed_email_was
+ end
+
+ def email_change_confirmation_required?
+ self.class.reconfirmable && @email_change_confirmation_required
+ end
+
module ClassMethods
# Attempt to find a user by it's email. If a record is found, send new
- # confirmation instructions to it. If not user is found, returns a new user
- # with an email not found error.
+ # confirmation instructions to it. If not try searching the user by unconfirmed_email field.
+ # If no user is found, returns a new user with an email not found error.
# Options must contain the user email
def send_confirmation_instructions(attributes={})
- confirmable = find_or_initialize_with_errors(confirmation_keys, attributes, :not_found)
+ confirmable = find_by_unconfirmed_email_with_errors(attributes) if reconfirmable
+ unless confirmable.try(:persisted?)
+ confirmable = find_or_initialize_with_errors(confirmation_keys, attributes, :not_found)
+ end
confirmable.resend_confirmation_token if confirmable.persisted?
confirmable
end
@@ -153,7 +206,15 @@ def confirmation_token
generate_token(:confirmation_token)
end
- Devise::Models.config(self, :confirm_within, :confirmation_keys)
+ # Find a record for confirmation by unconfirmed email field
+ def find_by_unconfirmed_email_with_errors(attributes = {})
+ unconfirmed_required_attributes = confirmation_keys.map{ |k| k == :email ? :unconfirmed_email : k }
+ unconfirmed_attributes = attributes.symbolize_keys
+ unconfirmed_attributes[:unconfirmed_email] = unconfirmed_attributes.delete(:email)
+ find_or_initialize_with_errors(unconfirmed_required_attributes, unconfirmed_attributes, :not_found)
+ end
+
+ Devise::Models.config(self, :confirm_within, :confirmation_keys, :reconfirmable)
end
end
end
1  lib/devise/models/validatable.rb
View
@@ -25,6 +25,7 @@ def self.included(base)
validates_presence_of :email, :if => :email_required?
validates_uniqueness_of :email, :case_sensitive => (case_insensitive_keys != false), :allow_blank => true, :if => :email_changed?
validates_format_of :email, :with => email_regexp, :allow_blank => true, :if => :email_changed?
+ validates_with Devise::Models::Confirmable::ConfirmableValidator
validates_presence_of :password, :if => :password_required?
validates_confirmation_of :password, :if => :password_required?
1  lib/devise/schema.rb
View
@@ -36,6 +36,7 @@ def confirmable
apply_devise_schema :confirmation_token, String
apply_devise_schema :confirmed_at, DateTime
apply_devise_schema :confirmation_sent_at, DateTime
+ apply_devise_schema :unconfirmed_email, String
end
# Creates reset_password_token and reset_password_sent_at.
6 lib/generators/templates/devise.rb
View
@@ -65,6 +65,12 @@
# (ie 2 days).
# config.confirm_within = 2.days
+ # If true, requires any email changes to be confirmed (exctly the same way as
+ # initial account confirmation) to be applied. Requires additional unconfirmed_email
+ # db field (see migrations). Until confirmed new email is stored in
+ # unconfirmed email column, and copied to email column on successful confirmation.
+ # config.reconfirmable = false
+
# Defines which key will be used when confirming an account
# config.confirmation_keys = [ :email ]
59 test/integration/confirmable_test.rb
View
@@ -130,3 +130,62 @@ def visit_user_confirmation_with_token(confirmation_token)
assert response.body.include? %(<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<errors>)
end
end
+
+class ConfirmationOnChangeTest < ConfirmationTest
+
+ def create_second_user(options={})
+ @user = nil
+ create_user(options)
+ end
+
+ def setup
+ Devise.reconfirmable = true
+ end
+
+ def teardown
+ Devise.reconfirmable = false
+ end
+
+ test 'user should be able to request a new confirmation after email changed' do
+ user = create_user(:confirm => true)
+ user.update_attributes(:email => 'new_test@example.com')
+ ActionMailer::Base.deliveries.clear
+
+ visit new_user_session_path
+ click_link "Didn't receive confirmation instructions?"
+
+ fill_in 'email', :with => user.unconfirmed_email
+ click_button 'Resend confirmation instructions'
+
+ assert_current_url '/users/sign_in'
+ assert_contain 'You will receive an email with instructions about how to confirm your account in a few minutes'
+ assert_equal 1, ActionMailer::Base.deliveries.size
+ end
+
+ test 'user with valid confirmation token should be able to confirm email after email changed' do
+ user = create_user(:confirm => true)
+ user.update_attributes(:email => 'new_test@example.com')
+ assert 'new_test@example.com', user.unconfirmed_email
+ visit_user_confirmation_with_token(user.confirmation_token)
+
+ assert_contain 'Your account was successfully confirmed.'
+ assert_current_url '/'
+ assert user.reload.confirmed?
+ end
+
+ test 'user email should be unique also within unconfirmed_email' do
+ user = create_user(:confirm => true)
+ user.update_attributes(:email => 'new_test@example.com')
+ assert 'new_test@example.com', user.unconfirmed_email
+
+ get new_user_registration_path
+
+ fill_in 'email', :with => 'new_test@example.com'
+ fill_in 'password', :with => 'new_user123'
+ fill_in 'password confirmation', :with => 'new_user123'
+ click_button 'Sign up'
+
+ assert_have_selector '#error_explanation'
+ assert_contain /Email.*already.*taken/
+ end
+end
85 test/models/confirmable_test.rb
View
@@ -236,3 +236,88 @@ def setup
end
end
end
+
+class ConfirmableOnChangeTest < ConfirmableTest
+ def setup
+ Devise.reconfirmable = true
+ end
+
+ def teardown
+ Devise.reconfirmable = false
+ end
+
+ def test_should_not_resend_email_instructions_if_the_user_change_his_email
+ #behaves differently
+ end
+
+ def test_should_not_reset_confirmation_status_or_token_when_updating_email
+ #behaves differently
+ end
+
+ test 'should generate confirmation token after changing email' do
+ user = create_user
+ assert user.confirm!
+ assert_nil user.confirmation_token
+ assert user.update_attributes(:email => 'new_test@example.com')
+ assert_not_nil user.confirmation_token
+ end
+
+ test 'should send confirmation instructions by email after changing email' do
+ user = create_user
+ assert user.confirm!
+ assert_email_sent do
+ assert user.update_attributes(:email => 'new_test@example.com')
+ end
+ end
+
+ test 'should not send confirmation by email after changing password' do
+ user = create_user
+ assert user.confirm!
+ assert_email_not_sent do
+ assert user.update_attributes(:password => 'newpass', :password_confirmation => 'newpass')
+ end
+ end
+
+ test 'should stay confirmed when email is changed' do
+ user = create_user
+ assert user.confirm!
+ assert user.update_attributes(:email => 'new_test@example.com')
+ assert user.confirmed?
+ end
+
+ test 'should update email only when it is confirmed' do
+ user = create_user
+ assert user.confirm!
+ assert user.update_attributes(:email => 'new_test@example.com')
+ assert_not_equal 'new_test@example.com', user.email
+ assert user.confirm!
+ assert_equal 'new_test@example.com', user.email
+ end
+
+ test 'should find a user by send confirmation instructions with unconfirmed_email' do
+ user = create_user
+ assert user.confirm!
+ assert user.update_attributes(:email => 'new_test@example.com')
+ confirmation_user = User.send_confirmation_instructions(:email => user.unconfirmed_email)
+ assert_equal confirmation_user, user
+ end
+
+ test 'should return a new user if no email or unconfirmed_email was found' do
+ confirmation_user = User.send_confirmation_instructions(:email => "invalid@email.com")
+ assert_not confirmation_user.persisted?
+ end
+
+ test 'should add error to new user email if no email or unconfirmed_email was found' do
+ confirmation_user = User.send_confirmation_instructions(:email => "invalid@email.com")
+ assert confirmation_user.errors[:email]
+ assert_equal "not found", confirmation_user.errors[:email].join
+ end
+
+ test 'should find user with email in unconfirmed_emails' do
+ user = create_user
+ user.unconfirmed_email = "new_test@email.com"
+ assert user.save
+ user = User.find_by_unconfirmed_email_with_errors(:email => "new_test@email.com")
+ assert user.persisted?
+ end
+end
12 test/models/validatable_test.rb
View
@@ -97,6 +97,18 @@ class ValidatableTest < ActiveSupport::TestCase
assert_not (user.errors[:password].join =~ /is too long/)
end
+ test 'should check if email is unique in unconfirmed_email column' do
+ swap Devise, :reconfirmable => [:username, :email] do
+ user = create_user
+ user.update_attributes({:email => 'new_test@email.com'})
+ assert 'new_test@email.com', user.unconfirmed_email
+
+ @user = nil
+ user = new_user(:email => 'new_test@email.com')
+ assert user.invalid?
+ end
+ end
+
test 'should complain about length even if possword is not required' do
user = new_user(:password => 'x'*129, :password_confirmation => 'x'*129)
user.stubs(:password_required?).returns(false)
Something went wrong with that request. Please try again.