Merged pull request #1518 - Fixes issue #1509: Conflicting roles can get saved. Huge thanks to @muhammadn .

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_create   :check_preferences_default, :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_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
+  def validate_roles(role)
-    return true if !role_ids
+    return true if !role_ids # we need role_ids for checking in role_ids below, in this method
-    role_ids.each do |role_id|
+    return true if role.preferences[:not].blank?
-      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)
+      next if role_ids.exclude?(local_role.id)
-      end
+      raise "Role #{role.name} conflicts with #{local_role.name}"
     end
     true
   end

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