Improved user creation from incoming email.

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@7952 e93f8b46-1217-0410-a6f0-8f06a7374b81

app/models/mail_handler.rb

@@ -61,7 +61,7 @@ class MailHandler < ActionMailer::Base
       when 'accept'
         @user = User.anonymous
       when 'create'
-        @user = MailHandler.create_user_from_email(email)
+        @user = create_user_from_email(email)
         if @user
           logger.info "MailHandler: [#{@user.login}] account created" if logger && logger.info
           Mailer.deliver_account_information(@user, @user.password)
@@ -322,22 +322,53 @@ class MailHandler < ActionMailer::Base
     @full_sanitizer ||= HTML::FullSanitizer.new
   end
 
-  # Creates a user account for the +email+ sender
-  def self.create_user_from_email(email)
+  def self.assign_string_attribute_with_limit(object, attribute, value)
+    limit = object.class.columns_hash[attribute.to_s].limit || 255
+    value = value.to_s.slice(0, limit)
+    object.send("#{attribute}=", value)
+  end
+
+  # Returns a User from an email address and a full name
+  def self.new_user_from_attributes(email_address, fullname=nil)
+    user = User.new
+
+    # Truncating the email address would result in an invalid format
+    user.mail = email_address
+    assign_string_attribute_with_limit(user, 'login', email_address)
+
+    names = fullname.blank? ? email_address.gsub(/@.*$/, '').split('.') : fullname.split
+    assign_string_attribute_with_limit(user, 'firstname', names.shift)
+    assign_string_attribute_with_limit(user, 'lastname', names.join(' '))
+    user.lastname = '-' if user.lastname.blank?
+
+    password_length = [Setting.password_min_length.to_i, 10].max
+    user.password = ActiveSupport::SecureRandom.hex(password_length / 2 + 1)
+    user.language = Setting.default_language
+
+    unless user.valid?
+      user.login = "user#{ActiveSupport::SecureRandom.hex(6)}" if user.errors.on(:login)
+      user.firstname = "-" if user.errors.on(:firstname)
+      user.lastname = "-" if user.errors.on(:lastname)
+    end
+
+    user
+  end
+
+  # Creates a User for the +email+ sender
+  # Returns the user or nil if it could not be created
+  def create_user_from_email(email)
     addr = email.from_addrs.to_a.first
     if addr && !addr.spec.blank?
-      user = User.new
-      user.mail = addr.spec
-
-      names = addr.name.blank? ? addr.spec.gsub(/@.*$/, '').split('.') : addr.name.split
-      user.firstname = names.shift
-      user.lastname = names.join(' ')
-      user.lastname = '-' if user.lastname.blank?
-
-      user.login = user.mail
-      user.password = ActiveSupport::SecureRandom.hex(5)
-      user.language = Setting.default_language
-      user.save ? user : nil
+      user = self.class.new_user_from_attributes(addr.spec, addr.name)
+      if user.save
+        user
+      else
+        logger.error "MailHandler: failed to create User: #{user.errors.full_messages}" if logger
+        nil
+      end
+    else
+      logger.error "MailHandler: failed to create User: no FROM address found" if logger
+      nil
     end
   end
 

test/unit/mail_handler_test.rb

@@ -485,6 +485,46 @@ class MailHandlerTest < ActiveSupport::TestCase
     assert_equal issue.subject, 'New ticket on a given project with a very long subject line which exceeds 255 chars and should not be ignored but chopped off. And if the subject line is still not long enough, we just add more text. And more text. Wow, this is really annoying. Especially, if you have nothing to say...'[0,255]
   end
 
+  def test_new_user_from_attributes_should_return_valid_user
+    to_test = {
+      # [address, name] => [login, firstname, lastname]
+      ['jsmith@example.net', nil] => ['jsmith@example.net', 'jsmith', '-'],
+      ['jsmith@example.net', 'John'] => ['jsmith@example.net', 'John', '-'],
+      ['jsmith@example.net', 'John Smith'] => ['jsmith@example.net', 'John', 'Smith'],
+      ['jsmith@example.net', 'John Paul Smith'] => ['jsmith@example.net', 'John', 'Paul Smith'],
+      ['jsmith@example.net', 'AVeryLongFirstnameThatExceedsTheMaximumLength Smith'] => ['jsmith@example.net', 'AVeryLongFirstnameThatExceedsT', 'Smith'],
+      ['jsmith@example.net', 'John AVeryLongLastnameThatExceedsTheMaximumLength'] => ['jsmith@example.net', 'John', 'AVeryLongLastnameThatExceedsTh'],
+      ['alongemailaddressthatexceedsloginlength@example.net', 'John Smith'] => ['alongemailaddressthatexceedslo', 'John', 'Smith']
+    }
+
+    to_test.each do |attrs, expected|
+      user = MailHandler.new_user_from_attributes(attrs.first, attrs.last)
+
+      assert user.valid?
+      assert_equal attrs.first, user.mail
+      assert_equal expected[0], user.login
+      assert_equal expected[1], user.firstname
+      assert_equal expected[2], user.lastname
+    end
+  end
+
+  def test_new_user_from_attributes_should_respect_minimum_password_length
+    with_settings :password_min_length => 15 do
+      user = MailHandler.new_user_from_attributes('jsmith@example.net')
+      assert user.valid?
+      assert user.password.length >= 15
+    end
+  end
+ 
+  def test_new_user_from_attributes_should_use_default_login_if_invalid
+    MailHandler.new_user_from_attributes('alongemailaddressthatexceedsloginlength-1@example.net').save!
+
+    # another long address that would result in duplicate login
+    user = MailHandler.new_user_from_attributes('alongemailaddressthatexceedsloginlength-2@example.net')
+    assert user.valid?
+    assert user.login =~ /^user[a-f0-9]+$/
+  end
+
   private
 
   def submit_email(filename, options={})