From ba684348ef82fbc826e28f0f27ee71870177d7a4 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Thu, 15 Sep 2016 00:56:04 +0200 Subject: [PATCH] Improved user validation. --- app/controllers/users_controller.rb | 4 +- app/models/user.rb | 23 +++++--- .../user_organization_controller_test.rb | 59 +++++++++++++++++-- test/unit/activity_stream_test.rb | 6 +- 4 files changed, 76 insertions(+), 16 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 40e31fb62..21760a54c 100644 --- a/app/controllers/users_controller.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index b4ac8b592..dabdc7f41 100644 --- a/app/models/user.rb +++ b/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_update :check_password, :check_name, :check_email, :check_login, :check_preferences_default, :validate_roles + before_validation :check_name, :check_email, :check_login, :check_password + 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 - self.email = email.downcase + return if email.empty? + 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 - return if !login + # if no email, complain about missing 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) diff --git a/test/controllers/user_organization_controller_test.rb b/test/controllers/user_organization_controller_test.rb index 6ead25eb4..23f81c6c1 100644 --- a/test/controllers/user_organization_controller_test.rb +++ b/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 ", result[0]['label']) - assert_equal("Customer#{firstname} Customer Last ", result[0]['value']) + assert_equal("Customer#{firstname} Customer Last ", result[0]['label']) + assert_equal("Customer#{firstname} Customer Last ", result[0]['value']) assert_not(result[0]['role_ids']) assert_not(result[0]['roles']) end diff --git a/test/unit/activity_stream_test.rb b/test/unit/activity_stream_test.rb index 6aa1a817f..74b18c087 100644 --- a/test/unit/activity_stream_test.rb +++ b/test/unit/activity_stream_test.rb @@ -236,7 +236,8 @@ class ActivityStreamTest < ActiveSupport::TestCase create: { user: { login: 'someemail@example.com', - email: 'Bob Smith II ', + 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 ', + email: 'someemail@example.com', + firstname: 'Bob Smith II', updated_by_id: current_user.id, created_by_id: current_user.id, },