Improved user validation.

app/controllers/users_controller.rb

@@ -176,7 +176,7 @@ class UsersController < ApplicationController
     end
 
     # check if user already exists
-    if user.email
+    if !user.email.empty?
       exists = User.where(email: user.email.downcase).first
       raise Exceptions::UnprocessableEntity, 'User already exists!' if exists
     end
@@ -187,7 +187,7 @@ class UsersController < ApplicationController
       Setting.set('system_init_done', true)
 
       # fetch org logo
-      if user.email
+      if !user.email.empty?
         Service::Image.organization_suggest(user.email)
       end
 

app/models/user.rb

@@ -32,8 +32,9 @@ class User < ApplicationModel
   load 'user/search_index.rb'
   include User::SearchIndex
 
-  before_create   :check_name, :check_email, :check_login, :check_password, :check_preferences_default, :validate_roles
+  before_validation :check_name, :check_email, :check_login, :check_password
-  before_update   :check_password, :check_name, :check_email, :check_login, :check_preferences_default, :validate_roles
+  before_create   :check_preferences_default, :validate_roles
+  before_update   :check_preferences_default, :validate_roles
   after_create    :avatar_for_email_check
   after_update    :avatar_for_email_check
   after_destroy   :avatar_destroy
@@ -751,14 +752,17 @@ returns
   end
 
   def check_email
-    return if !email
+    return if email.empty?
-    self.email = email.downcase
+    self.email = email.downcase.strip
+    return if id == 1
+    raise Exceptions::UnprocessableEntity, 'Invalid email' if email !~ /@/
+    raise Exceptions::UnprocessableEntity, 'Invalid email' if email =~ /\s/
   end
 
   def check_login
 
     # use email as login if not given
-    if !login && email
+    if login.empty? && !email.empty?
       self.login = email
     end
 
@@ -769,10 +773,13 @@ returns
       end
     end
 
-    # check if login already exists
+    # if no email, complain about missing login
-    return if !login
+    if id != 1 && login.empty? && email.empty?
+      raise Exceptions::UnprocessableEntity, 'Attribute \'login\' required!'
+    end
 
-    self.login = login.downcase
+    # check if login already exists
+    self.login = login.downcase.strip
     check      = true
     while check
       exists = User.find_by(login: login)

test/controllers/user_organization_controller_test.rb

@@ -239,10 +239,12 @@ class UserOrganizationControllerTest < ActionDispatch::IntegrationTest
     assert(user.role?('Admin'))
     assert_not(user.role?('Agent'))
     assert_not(user.role?('Customer'))
+    assert_equal('new_admin_by_admin@example.com', result['login'])
+    assert_equal('new_admin_by_admin@example.com', result['email'])
 
     # create user with agent role
     role = Role.lookup(name: 'Agent')
-    params = { firstname: 'Agent First', lastname: 'Agent Last', email: 'new_agent_by_admin@example.com', role_ids: [ role.id ] }
+    params = { firstname: 'Agent First', lastname: 'Agent Last', email: 'new_agent_by_admin1@example.com', role_ids: [ role.id ] }
     post '/api/v1/users', params.to_json, @headers.merge('Authorization' => credentials)
     assert_response(201)
     result = JSON.parse(@response.body)
@@ -251,6 +253,53 @@ class UserOrganizationControllerTest < ActionDispatch::IntegrationTest
     assert_not(user.role?('Admin'))
     assert(user.role?('Agent'))
     assert_not(user.role?('Customer'))
+    assert_equal('new_agent_by_admin1@example.com', result['login'])
+    assert_equal('new_agent_by_admin1@example.com', result['email'])
+
+    role = Role.lookup(name: 'Agent')
+    params = { firstname: 'Agent First', email: 'new_agent_by_admin2@example.com', role_ids: [ role.id ] }
+    post '/api/v1/users', params.to_json, @headers.merge('Authorization' => credentials)
+    assert_response(201)
+    result = JSON.parse(@response.body)
+    assert(result)
+    user = User.find(result['id'])
+    assert_not(user.role?('Admin'))
+    assert(user.role?('Agent'))
+    assert_not(user.role?('Customer'))
+    assert_equal('new_agent_by_admin2@example.com', result['login'])
+    assert_equal('new_agent_by_admin2@example.com', result['email'])
+    assert_equal('Agent', result['firstname'])
+    assert_equal('First', result['lastname'])
+
+    role = Role.lookup(name: 'Agent')
+    params = { firstname: 'Agent First', email: 'new_agent_by_admin2@example.com', role_ids: [ role.id ] }
+    post '/api/v1/users', params.to_json, @headers.merge('Authorization' => credentials)
+    assert_response(422)
+    result = JSON.parse(@response.body)
+    assert(result)
+    assert_equal('User already exists!', result['error'])
+
+    # missing required attributes
+    params = { note: 'some note' }
+    post '/api/v1/users', params.to_json, @headers.merge('Authorization' => credentials)
+    assert_response(422)
+    result = JSON.parse(@response.body)
+    assert(result)
+    assert_equal('Attribute \'login\' required!', result['error'])
+
+    params = { firstname: 'newfirstname123', note: 'some note' }
+    post '/api/v1/users', params.to_json, @headers.merge('Authorization' => credentials)
+    assert_response(422)
+    result = JSON.parse(@response.body)
+    assert(result)
+    assert_equal('Attribute \'login\' required!', result['error'])
+
+    params = { firstname: 'newfirstname123', email: 'some_what', note: 'some note' }
+    post '/api/v1/users', params.to_json, @headers.merge('Authorization' => credentials)
+    assert_response(422)
+    result = JSON.parse(@response.body)
+    assert(result)
+    assert_equal('Invalid email', result['error'])
 
   end
 
@@ -291,7 +340,7 @@ class UserOrganizationControllerTest < ActionDispatch::IntegrationTest
 
     # create user with customer role
     role = Role.lookup(name: 'Customer')
-    params = { firstname: "Customer#{firstname}", lastname: 'Customer Last', email: 'new_agent_by_agent@example.com', role_ids: [ role.id ] }
+    params = { firstname: "Customer#{firstname}", lastname: 'Customer Last', email: 'new_customer_by_agent@example.com', role_ids: [ role.id ] }
     post '/api/v1/users', params.to_json, @headers.merge('Authorization' => credentials)
     assert_response(201)
     result_user1 = JSON.parse(@response.body)
@@ -300,6 +349,8 @@ class UserOrganizationControllerTest < ActionDispatch::IntegrationTest
     assert_not(user.role?('Admin'))
     assert_not(user.role?('Agent'))
     assert(user.role?('Customer'))
+    assert_equal('new_customer_by_agent@example.com', result_user1['login'])
+    assert_equal('new_customer_by_agent@example.com', result_user1['email'])
 
     # search as agent
     Scheduler.worker(true)
@@ -328,8 +379,8 @@ class UserOrganizationControllerTest < ActionDispatch::IntegrationTest
     result = JSON.parse(@response.body)
     assert_equal(Array, result.class)
     assert_equal(result_user1['id'], result[0]['id'])
-    assert_equal("Customer#{firstname} Customer Last <new_agent_by_agent@example.com>", result[0]['label'])
+    assert_equal("Customer#{firstname} Customer Last <new_customer_by_agent@example.com>", result[0]['label'])
-    assert_equal("Customer#{firstname} Customer Last <new_agent_by_agent@example.com>", result[0]['value'])
+    assert_equal("Customer#{firstname} Customer Last <new_customer_by_agent@example.com>", result[0]['value'])
     assert_not(result[0]['role_ids'])
     assert_not(result[0]['roles'])
   end

test/unit/activity_stream_test.rb

@@ -236,7 +236,8 @@ class ActivityStreamTest < ActiveSupport::TestCase
         create: {
           user: {
             login: 'someemail@example.com',
-            email: 'Bob Smith II <someemail@example.com>',
+            email: 'someemail@example.com',
+            firstname: 'Bob Smith II',
             updated_by_id: current_user.id,
             created_by_id: current_user.id,
           },
@@ -302,7 +303,8 @@ class ActivityStreamTest < ActiveSupport::TestCase
         create: {
           user: {
             login: 'someemail@example.com',
-            email: 'Bob Smith II <someemail@example.com>',
+            email: 'someemail@example.com',
+            firstname: 'Bob Smith II',
             updated_by_id: current_user.id,
             created_by_id: current_user.id,
           },