From 866614122db0947d07da6280782850dc9b945a29 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Sat, 15 Jul 2017 00:35:44 +0200 Subject: [PATCH] Implemented issue #1251 - Config option to have uniq email addresses for users. --- app/controllers/users_controller.rb | 9 +- app/models/user.rb | 14 ++- .../20170714000002_user_email_multiple_use.rb | 34 +++++++ db/seeds/settings.rb | 25 +++++ .../user_organization_controller_test.rb | 12 ++- test/integration/geo_location_test.rb | 2 +- test/unit/activity_stream_test.rb | 20 ++-- test/unit/history_test.rb | 50 ++++------ test/unit/user_test.rb | 94 +++++++++++++++++++ 9 files changed, 207 insertions(+), 53 deletions(-) create mode 100644 db/migrate/20170714000002_user_email_multiple_use.rb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index acb82ed35..a51a6ac0c 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -132,6 +132,10 @@ class UsersController < ApplicationController raise Exceptions::UnprocessableEntity, 'Attribute \'email\' required!' end + # check if user already exists + exists = User.find_by(email: clean_params[:email].downcase.strip) + raise Exceptions::UnprocessableEntity, 'Email address is already used for other user.' if exists + user = User.new(clean_params) user.associations_from_param(params) user.updated_by_id = 1 @@ -171,11 +175,6 @@ class UsersController < ApplicationController user.associations_from_param(params) end - # check if user already exists - if user.email.present? - exists = User.where(email: user.email.downcase).first - raise Exceptions::UnprocessableEntity, 'User already exists!' if exists - end user.save! # if first user was added, set system init done diff --git a/app/models/user.rb b/app/models/user.rb index e447b961d..c9896d8dc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -38,7 +38,7 @@ class User < ApplicationModel load 'user/search_index.rb' include User::SearchIndex - before_validation :check_name, :check_email, :check_login, :ensure_password, :ensure_roles, :ensure_identifier + before_validation :check_name, :check_email, :check_login, :ensure_uniq_email, :ensure_password, :ensure_roles, :ensure_identifier before_create :check_preferences_default, :validate_roles, :domain_based_assignment, :set_locale before_update :check_preferences_default, :validate_roles, :reset_login_failed after_create :avatar_for_email_check @@ -845,7 +845,7 @@ returns def check_email return true if Setting.get('import_mode') - return true if email.empty? + return true if email.blank? self.email = email.downcase.strip return true if id == 1 raise Exceptions::UnprocessableEntity, 'Invalid email' if email !~ /@/ @@ -897,6 +897,16 @@ returns raise Exceptions::UnprocessableEntity, 'Minimum one identifier (login, firstname, lastname, phone or email) for user is required.' end + def ensure_uniq_email + return true if Setting.get('user_email_multiple_use') + return true if Setting.get('import_mode') + return true if email.blank? + return true if !changes + return true if !changes['email'] + return true if !User.find_by(email: email.downcase.strip) + raise Exceptions::UnprocessableEntity, 'Email address is already used for other user.' + end + def validate_roles return true if !role_ids role_ids.each { |role_id| diff --git a/db/migrate/20170714000002_user_email_multiple_use.rb b/db/migrate/20170714000002_user_email_multiple_use.rb new file mode 100644 index 000000000..5d89d8d59 --- /dev/null +++ b/db/migrate/20170714000002_user_email_multiple_use.rb @@ -0,0 +1,34 @@ +class UserEmailMultipleUse < ActiveRecord::Migration + def up + + # return if it's a new setup + return if !Setting.find_by(name: 'system_init_done') + + Setting.create_if_not_exists( + title: 'User email for muliple users', + name: 'user_email_multiple_use', + area: 'Model::User', + description: 'Allow to use email address for muliple users.', + options: { + form: [ + { + display: '', + null: true, + name: 'user_email_multiple_use', + tag: 'boolean', + options: { + true => 'yes', + false => 'no', + }, + }, + ], + }, + state: false, + preferences: { + permission: ['admin'], + }, + frontend: false + ) + end + +end diff --git a/db/seeds/settings.rb b/db/seeds/settings.rb index 6e5bd08d0..5802c1063 100644 --- a/db/seeds/settings.rb +++ b/db/seeds/settings.rb @@ -705,6 +705,31 @@ Setting.create_if_not_exists( }, frontend: true ) +Setting.create_if_not_exists( + title: 'User email for muliple users', + name: 'user_email_multiple_use', + area: 'Model::User', + description: 'Allow to use email address for muliple users.', + options: { + form: [ + { + display: '', + null: true, + name: 'user_email_multiple_use', + tag: 'boolean', + options: { + true => 'yes', + false => 'no', + }, + }, + ], + }, + state: false, + preferences: { + permission: ['admin'], + }, + frontend: false +) Setting.create_if_not_exists( title: 'Authentication via %s', name: 'auth_ldap', diff --git a/test/controllers/user_organization_controller_test.rb b/test/controllers/user_organization_controller_test.rb index 1a64030a4..4754a75de 100644 --- a/test/controllers/user_organization_controller_test.rb +++ b/test/controllers/user_organization_controller_test.rb @@ -126,7 +126,15 @@ class UserOrganizationControllerTest < ActionDispatch::IntegrationTest assert_response(422) result = JSON.parse(@response.body) assert(result['error']) - assert_equal('User already exists!', result['error']) + assert_equal('Email address is already used for other user.', result['error']) + + # email missing with enabled feature + params = { firstname: 'some firstname', signup: true } + post '/api/v1/users', params.to_json, headers + assert_response(422) + result = JSON.parse(@response.body) + assert(result['error']) + assert_equal('Attribute \'email\' required!', result['error']) # email missing with enabled feature params = { firstname: 'some firstname', signup: true } @@ -330,7 +338,7 @@ class UserOrganizationControllerTest < ActionDispatch::IntegrationTest assert_response(422) result = JSON.parse(@response.body) assert(result) - assert_equal('User already exists!', result['error']) + assert_equal('Email address is already used for other user.', result['error']) # missing required attributes params = { note: 'some note' } diff --git a/test/integration/geo_location_test.rb b/test/integration/geo_location_test.rb index 388378255..8bd148a9a 100644 --- a/test/integration/geo_location_test.rb +++ b/test/integration/geo_location_test.rb @@ -51,7 +51,7 @@ class GeoLocationTest < ActiveSupport::TestCase login: 'some_geo_login2', firstname: 'First', lastname: 'Last', - email: 'some_geo_login1@example.com', + email: 'some_geo_login2@example.com', password: 'test', street: 'Marienstrasse 13', city: 'Berlin', diff --git a/test/unit/activity_stream_test.rb b/test/unit/activity_stream_test.rb index 27b905ad1..8da295f33 100644 --- a/test/unit/activity_stream_test.rb +++ b/test/unit/activity_stream_test.rb @@ -10,7 +10,7 @@ class ActivityStreamTest < ActiveSupport::TestCase login: 'admin', firstname: 'Bob', lastname: 'Smith', - email: 'bob@example.com', + email: 'bob+active_stream@example.com', password: 'some_pass', active: true, roles: roles, @@ -23,7 +23,7 @@ class ActivityStreamTest < ActiveSupport::TestCase end test 'ticket+user' do - ticket = Ticket.create( + ticket = Ticket.create!( group_id: Group.lookup(name: 'Users').id, customer_id: @current_user.id, owner_id: User.lookup(login: '-').id, @@ -35,7 +35,7 @@ class ActivityStreamTest < ActiveSupport::TestCase ) travel 2.seconds - article = Ticket::Article.create( + article = Ticket::Article.create!( ticket_id: ticket.id, updated_by_id: @current_user.id, created_by_id: @current_user.id, @@ -86,12 +86,12 @@ class ActivityStreamTest < ActiveSupport::TestCase assert(stream.empty?) # cleanup - ticket.destroy + ticket.destroy! travel_back end test 'organization' do - organization = Organization.create( + organization = Organization.create!( name: 'some name', updated_by_id: @current_user.id, created_by_id: @current_user.id, @@ -125,12 +125,12 @@ class ActivityStreamTest < ActiveSupport::TestCase assert(stream.empty?) # cleanup - organization.destroy + organization.destroy! travel_back end test 'user with update check false' do - user = User.create( + user = User.create!( login: 'someemail@example.com', email: 'someemail@example.com', firstname: 'Bob Smith II', @@ -157,12 +157,12 @@ class ActivityStreamTest < ActiveSupport::TestCase assert(stream.empty?) # cleanup - user.destroy + user.destroy! travel_back end test 'user with update check true' do - user = User.create( + user = User.create!( login: 'someemail@example.com', email: 'someemail@example.com', firstname: 'Bob Smith II', @@ -204,7 +204,7 @@ class ActivityStreamTest < ActiveSupport::TestCase assert(stream.empty?) # cleanup - user.destroy + user.destroy! travel_back end diff --git a/test/unit/history_test.rb b/test/unit/history_test.rb index f8061c68d..989b92152 100644 --- a/test/unit/history_test.rb +++ b/test/unit/history_test.rb @@ -155,12 +155,12 @@ class HistoryTest < ActiveSupport::TestCase # use transaction ActiveRecord::Base.transaction do - ticket = Ticket.create(test[:ticket_create][:ticket]) + ticket = Ticket.create!(test[:ticket_create][:ticket]) test[:ticket_create][:article][:ticket_id] = ticket.id - article = Ticket::Article.create(test[:ticket_create][:article]) + article = Ticket::Article.create!(test[:ticket_create][:article]) - assert_equal(ticket.class.to_s, 'Ticket') - assert_equal(article.class.to_s, 'Ticket::Article') + assert_equal(ticket.class, Ticket) + assert_equal(article.class, Ticket::Article) # update ticket if test[:ticket_update][:ticket] @@ -185,25 +185,21 @@ class HistoryTest < ActiveSupport::TestCase } # delete tickets - tickets.each { |ticket| - ticket_id = ticket.id - ticket.destroy - found = Ticket.where(id: ticket_id).first - assert_not(found, 'Ticket destroyed') - } + tickets.each(&:destroy!) end test 'user' do + name = rand(999_999) tests = [ # test 1 { user_create: { user: { - login: 'some_login_test', + login: "some_login_test-#{name}", firstname: 'Bob', lastname: 'Smith', - email: 'somebody@example.com', + email: "somebody-#{name}@example.com", active: true, updated_by_id: current_user.id, created_by_id: current_user.id, @@ -213,7 +209,7 @@ class HistoryTest < ActiveSupport::TestCase user: { firstname: 'Bob', lastname: 'Master', - email: 'master@example.com', + email: "master-#{name}@example.com", active: false, }, }, @@ -236,8 +232,8 @@ class HistoryTest < ActiveSupport::TestCase history_object: 'User', history_type: 'updated', history_attribute: 'email', - value_from: 'somebody@example.com', - value_to: 'master@example.com', + value_from: "somebody-#{name}@example.com", + value_to: "master-#{name}@example.com", }, { result: true, @@ -258,9 +254,8 @@ class HistoryTest < ActiveSupport::TestCase # user transaction ActiveRecord::Base.transaction do - user = User.create(test[:user_create][:user]) - - assert_equal(user.class.to_s, 'User') + user = User.create!(test[:user_create][:user]) + assert_equal(user.class, User) # update user if test[:user_update][:user] @@ -277,12 +272,7 @@ class HistoryTest < ActiveSupport::TestCase } # delete user - users.each { |user| - user_id = user.id - user.destroy - found = User.where(id: user_id).first - assert_not(found, 'User destroyed') - } + users.each(&:destroy!) end test 'organization' do @@ -328,9 +318,8 @@ class HistoryTest < ActiveSupport::TestCase # user transaction ActiveRecord::Base.transaction do - organization = Organization.create(test[:organization_create][:organization]) - - assert_equal(organization.class.to_s, 'Organization') + organization = Organization.create!(test[:organization_create][:organization]) + assert_equal(organization.class, Organization) # update organization if test[:organization_update][:organization] @@ -346,12 +335,7 @@ class HistoryTest < ActiveSupport::TestCase } # delete user - organizations.each { |organization| - organization_id = organization.id - organization.destroy - found = Organization.where(id: organization_id).first - assert_not(found, 'Organization destroyed') - } + organizations.each(&:destroy!) end def history_check(history_list, history_check) diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index d18dafa6e..89447737c 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -368,6 +368,100 @@ class UserTest < ActiveSupport::TestCase admin.destroy! end + test 'uniq email' do + name = rand(999_999_999) + + email1 = "admin1-role_without_email#{name}@example.com" + admin1 = User.create!( + login: email1, + firstname: 'Role', + lastname: "Admin1#{name}", + email: email1, + password: 'adminpw', + active: true, + roles: Role.where(name: %w(Admin Agent)), + updated_by_id: 1, + created_by_id: 1, + ) + + assert(admin1.id) + assert_equal(admin1.email, email1) + + assert_raises(Exceptions::UnprocessableEntity) { + User.create!( + login: "#{email1}-1", + firstname: 'Role', + lastname: "Admin1#{name}", + email: email1, + password: 'adminpw', + active: true, + roles: Role.where(name: %w(Admin Agent)), + updated_by_id: 1, + created_by_id: 1, + ) + } + + email2 = "admin2-role_without_email#{name}@example.com" + admin2 = User.create!( + firstname: 'Role', + lastname: "Admin2#{name}", + email: email2, + password: 'adminpw', + active: true, + roles: Role.where(name: %w(Admin Agent)), + updated_by_id: 1, + created_by_id: 1, + ) + + assert_raises(Exceptions::UnprocessableEntity) { + admin2.email = email1 + admin2.save! + } + + admin1.email = admin1.email + admin1.save! + + admin2.destroy! + admin1.destroy! + end + + test 'uniq email - multiple use' do + Setting.set('user_email_multiple_use', true) + name = rand(999_999_999) + + email1 = "admin1-role_without_email#{name}@example.com" + admin1 = User.create!( + login: email1, + firstname: 'Role', + lastname: "Admin1#{name}", + email: email1, + password: 'adminpw', + active: true, + roles: Role.where(name: %w(Admin Agent)), + updated_by_id: 1, + created_by_id: 1, + ) + + assert(admin1.id) + assert_equal(admin1.email, email1) + + admin2 = User.create!( + login: "#{email1}-1", + firstname: 'Role', + lastname: "Admin1#{name}", + email: email1, + password: 'adminpw', + active: true, + roles: Role.where(name: %w(Admin Agent)), + updated_by_id: 1, + created_by_id: 1, + ) + assert_equal(admin2.email, email1) + admin2.destroy! + admin1.destroy! + Setting.set('user_email_multiple_use', false) + end + test 'ensure roles' do name = rand(999_999_999)