diff --git a/app/models/mail_handler.rb b/app/models/mail_handler.rb index 81af7e411..8e8d6568f 100644 --- a/app/models/mail_handler.rb +++ b/app/models/mail_handler.rb @@ -347,8 +347,8 @@ class MailHandler < ActionMailer::Base @full_sanitizer ||= HTML::FullSanitizer.new end - def self.assign_string_attribute_with_limit(object, attribute, value) - limit = object.class.columns_hash[attribute.to_s].limit || 255 + def self.assign_string_attribute_with_limit(object, attribute, value, limit=nil) + limit ||= object.class.columns_hash[attribute.to_s].limit || 255 value = value.to_s.slice(0, limit) object.send("#{attribute}=", value) end @@ -359,7 +359,7 @@ class MailHandler < ActionMailer::Base # Truncating the email address would result in an invalid format user.mail = email_address - assign_string_attribute_with_limit(user, 'login', email_address) + assign_string_attribute_with_limit(user, 'login', email_address, User::LOGIN_LENGTH_LIMIT) names = fullname.blank? ? email_address.gsub(/@.*$/, '').split('.') : fullname.split assign_string_attribute_with_limit(user, 'firstname', names.shift) diff --git a/app/models/user.rb b/app/models/user.rb index 9be6143dc..02aecd341 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -71,16 +71,19 @@ class User < Principal attr_accessor :last_before_login_on # Prevents unauthorized assignments attr_protected :login, :admin, :password, :password_confirmation, :hashed_password - + + LOGIN_LENGTH_LIMIT = 60 + MAIL_LENGTH_LIMIT = 60 + validates_presence_of :login, :firstname, :lastname, :mail, :if => Proc.new { |user| !user.is_a?(AnonymousUser) } validates_uniqueness_of :login, :if => Proc.new { |user| !user.login.blank? }, :case_sensitive => false validates_uniqueness_of :mail, :if => Proc.new { |user| !user.mail.blank? }, :case_sensitive => false # Login must contain lettres, numbers, underscores only validates_format_of :login, :with => /^[a-z0-9_\-@\.]*$/i - validates_length_of :login, :maximum => 30 + validates_length_of :login, :maximum => LOGIN_LENGTH_LIMIT validates_length_of :firstname, :lastname, :maximum => 30 validates_format_of :mail, :with => /^([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})$/i, :allow_blank => true - validates_length_of :mail, :maximum => 60, :allow_nil => true + validates_length_of :mail, :maximum => MAIL_LENGTH_LIMIT, :allow_nil => true validates_confirmation_of :password, :allow_nil => true validates_inclusion_of :mail_notification, :in => MAIL_NOTIFICATION_OPTIONS.collect(&:first), :allow_blank => true validate :validate_password_length diff --git a/db/migrate/20120205111326_change_users_login_limit.rb b/db/migrate/20120205111326_change_users_login_limit.rb new file mode 100644 index 000000000..4508a5c40 --- /dev/null +++ b/db/migrate/20120205111326_change_users_login_limit.rb @@ -0,0 +1,9 @@ +class ChangeUsersLoginLimit < ActiveRecord::Migration + def self.up + change_column :users, :login, :string, :limit => nil, :default => '', :null => false + end + + def self.down + change_column :users, :login, :string, :limit => 30, :default => '', :null => false + end +end diff --git a/lib/tasks/migrate_from_trac.rake b/lib/tasks/migrate_from_trac.rake index 26dec382b..ed45cad56 100644 --- a/lib/tasks/migrate_from_trac.rake +++ b/lib/tasks/migrate_from_trac.rake @@ -231,7 +231,7 @@ namespace :redmine do u = User.find_by_login(username) if !u # Create a new user if not found - mail = username[0,limit_for(User, 'mail')] + mail = username[0, User::MAIL_LENGTH_LIMIT] if mail_attr = TracSessionAttribute.find_by_sid_and_name(username, 'email') mail = mail_attr.value end @@ -249,7 +249,7 @@ namespace :redmine do :firstname => fn[0, limit_for(User, 'firstname')], :lastname => ln[0, limit_for(User, 'lastname')] - u.login = username[0,limit_for(User, 'login')].gsub(/[^a-z0-9_\-@\.]/i, '-') + u.login = username[0, User::LOGIN_LENGTH_LIMIT].gsub(/[^a-z0-9_\-@\.]/i, '-') u.password = 'trac' u.admin = true if TracPermission.find_by_username_and_action(username, 'admin') # finally, a default user is used if the new user is not valid diff --git a/test/unit/mail_handler_test.rb b/test/unit/mail_handler_test.rb index d21c46183..205f68607 100644 --- a/test/unit/mail_handler_test.rb +++ b/test/unit/mail_handler_test.rb @@ -547,14 +547,13 @@ class MailHandlerTest < ActiveSupport::TestCase ['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'] + ['jsmith@example.net', 'John AVeryLongLastnameThatExceedsTheMaximumLength'] => ['jsmith@example.net', 'John', 'AVeryLongLastnameThatExceedsTh'] } to_test.each do |attrs, expected| user = MailHandler.new_user_from_attributes(attrs.first, attrs.last) - assert user.valid? + assert user.valid?, user.errors.full_messages assert_equal attrs.first, user.mail assert_equal expected[0], user.login assert_equal expected[1], user.firstname @@ -571,12 +570,10 @@ class MailHandlerTest < ActiveSupport::TestCase 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') + user = MailHandler.new_user_from_attributes('foo+bar@example.net') assert user.valid? assert user.login =~ /^user[a-f0-9]+$/ + assert_equal 'foo+bar@example.net', user.mail end private diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index a9484501d..b75555383 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -58,6 +58,16 @@ class UserTest < ActiveSupport::TestCase u.errors[:mail].to_s end + def test_login_length_validation + user = User.new(:firstname => "new", :lastname => "user", :mail => "newuser@somenet.foo") + user.login = "x" * (User::LOGIN_LENGTH_LIMIT+1) + assert !user.valid? + + user.login = "x" * (User::LOGIN_LENGTH_LIMIT) + assert user.valid? + assert user.save + end + def test_create user = User.new(:firstname => "new", :lastname => "user", :mail => "newuser@somenet.foo")