diff --git a/app/models/user.rb b/app/models/user.rb index 762809762..f39ee8cb9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -39,13 +39,13 @@ class User < ApplicationModel include User::SearchIndex before_validation :check_name, :check_email, :check_login, :ensure_uniq_email, :ensure_password, :ensure_roles, :ensure_identifier - before_create :check_preferences_default, :validate_roles, :validate_ooo, :domain_based_assignment, :set_locale - before_update :check_preferences_default, :validate_roles, :validate_ooo, :reset_login_failed, :validate_agent_limit_by_attributes, :last_admin_check_by_attribute + before_create :check_preferences_default, :validate_ooo, :domain_based_assignment, :set_locale + before_update :check_preferences_default, :validate_ooo, :reset_login_failed, :validate_agent_limit_by_attributes, :last_admin_check_by_attribute after_create :avatar_for_email_check after_update :avatar_for_email_check after_destroy :avatar_destroy, :user_device_destroy - has_and_belongs_to_many :roles, after_add: [:cache_update, :check_notifications], after_remove: :cache_update, before_add: :validate_agent_limit_by_role, before_remove: :last_admin_check_by_role, class_name: 'Role' + has_and_belongs_to_many :roles, after_add: [:cache_update, :check_notifications], after_remove: :cache_update, before_add: [:validate_agent_limit_by_role, :validate_roles], before_remove: :last_admin_check_by_role, class_name: 'Role' has_and_belongs_to_many :organizations, after_add: :cache_update, after_remove: :cache_update, class_name: 'Organization' #has_many :permissions, class_name: 'Permission', through: :roles, class_name: 'Role' has_many :tokens, after_add: :cache_update, after_remove: :cache_update @@ -953,17 +953,14 @@ returns raise Exceptions::UnprocessableEntity, 'Email address is already used for other user.' end - def validate_roles - return true if !role_ids - role_ids.each do |role_id| - role = Role.lookup(id: role_id) - raise "Unable to find role for id #{role_id}" if !role - next if !role.preferences[:not] - role.preferences[:not].each do |local_role_name| - local_role = Role.lookup(name: local_role_name) - next if !local_role - raise "Role #{role.name} conflicts with #{local_role.name}" if role_ids.include?(local_role.id) - end + def validate_roles(role) + return true if !role_ids # we need role_ids for checking in role_ids below, in this method + return true if role.preferences[:not].blank? + role.preferences[:not].each do |local_role_name| + local_role = Role.lookup(name: local_role_name) + next if !local_role + next if role_ids.exclude?(local_role.id) + raise "Role #{role.name} conflicts with #{local_role.name}" end true end diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index 375ef2b81..27d216538 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -491,6 +491,7 @@ class UserTest < ActiveSupport::TestCase roles = Role.where(name: 'Agent') customer1.roles = roles + customer1.save! assert_equal(customer1.role_ids.count, 1) @@ -532,6 +533,38 @@ class UserTest < ActiveSupport::TestCase assert_equal(customer2.role_ids.sort, Role.signup_role_ids) customer2.destroy! + customer3 = User.create_or_update( + login: "user-ensure-role2-#{name}@example.com", + firstname: 'Role', + lastname: "Customer#{name}", + email: "user-ensure-role2-#{name}@example.com", + password: 'customerpw', + roles: roles, + active: true, + updated_by_id: 1, + created_by_id: 1, + ) + + assert_raises(RuntimeError) do + customer3.roles = Role.where(name: %w(Customer Admin)) + end + assert_raises(RuntimeError) do + customer3.roles = Role.where(name: %w(Customer Agent)) + end + customer3.roles = Role.where(name: %w(Admin Agent)) + customer3.roles.each do |role| + assert_not_equal(role.name, 'Customer') + end + customer3.roles = Role.where(name: 'Admin') + customer3.roles.each do |role| + assert_not_equal(role.name, 'Customer') + end + customer3.roles = Role.where(name: 'Agent') + customer3.roles.each do |role| + assert_not_equal(role.name, 'Customer') + end + customer3.destroy! + admin.destroy! end