heartcombo / devise

Flexible authentication solution for Rails with Warden.
http://blog.plataformatec.com.br/tag/devise/
MIT License
24k stars 5.55k forks source link

Email case sensitivity and reconfirmation mail #3517

Closed olegykz closed 9 years ago

olegykz commented 9 years ago

If the user change it's email from "test@domain.com" to "tEsT@domain.com" the reconfirmation mail will be sended.

The problem is that even if we have setting "config.case_insensitive_keys = [ :email ]" - email attribute will be marked as changed because downcase! was triggered for our insensitive key.

We need to check - is the email attribute was really changed?

def postpone_email_change?
  postpone = self.class.reconfirmable && email_changed? && 
    (email_change.to_a.uniq.size == 2) && !@bypass_confirmation_postpone && self.email.present?
  @bypass_confirmation_postpone = false
  postpone

end

Rails (3.2.19) Devise (2.1.0)

josevalim commented 9 years ago

Thanks but it seems to be a bug in Rails. Why is e-mail_changed? returning true if it wasn't really changed? Also, keep in mind that e-mails are case sensitive. Many providers choose to provide them case insensitive.

acutemedical commented 9 years ago

@josevalim The way I got around this in my use case was two fold. 1.) Provide a regex validation on the User model that forces downcase email addresses on form submission. 2.) I wrote some jQuery that binds to the form field and downcases the email format in case of insensitivity. This may or may not address @olegykz concern. But it's now I worked around it in a Rails 3.2.20 Devise 2.1.2 app.

olegykz commented 9 years ago

If I set email key as insensitive I expect that system won't send any reconfirmation emails. Actually, this is not bug of Rails or Ruby(downcase! - method of ruby std library, http://ruby-doc.org/core-2.1.0/String.html#method-i-downcase-21).

And the simple solution is avoiding downcase! method which marks attribute as changed.

authenticatable.rb:

def downcase_keys
  self.class.case_insensitive_keys.each { |k| self[k] = self[k].try(:downcase) }
end

update it looks like this issue can show itself on strip_whitespace_keys option too

josevalim commented 9 years ago

@olegykz great find. Can you please send a pull request that fix both downcase_keys and strip_whitespace_keys? Thank you!

olegykz commented 9 years ago

ok, I'll provide pull request ASAP.

acutemedical commented 9 years ago

@olegykz This is good work. Kind of eliminates my workarounds. Would love to see this pulled into master. @josevalim will this be backported into older versions? I'm on 2.1.2 and due to my legacy app I'm concerned with upgrading and breaking something.

josevalim commented 9 years ago

We won't backport as those versions are no longer maintained.

acutemedical commented 9 years ago

@josevalim Ok, sounds like I'll need to keep my "fix" in place for now and eventually upgrade.

olegykz commented 9 years ago

I looked through code in master branch. It looks like issue isn't exists here because non-bang methods downcase and strip are used

def downcase_keys
  self.class.case_insensitive_keys.each { |k| apply_to_attribute_or_variable(k, :downcase) }
end

def strip_whitespace
  self.class.strip_whitespace_keys.each { |k| apply_to_attribute_or_variable(k, :strip) }
end

def apply_to_attribute_or_variable(attr, method)
  if self[attr]
    self[attr] = self[attr].try(method)
  elsif respond_to?(attr) && respond_to?("#{attr}=")
    new_value = send(attr).try(method)
    send("#{attr}=", new_value)
  end
end
josevalim commented 9 years ago

awesome! CLosing this then! Thank you!

olegykz commented 9 years ago

@teknull For my case I use simple workaround

user.rb:

# Skip reconfirmation if email actually wasn't changed.
before_validation do
  skip_reconfirmation! if pending_reconfirmation? && email_change.to_a.uniq.size == 1
end