Fixed issue #1563 - prevent admin from locking out.

app/models/role.rb

@@ -10,12 +10,12 @@ class Role < ApplicationModel
   include Role::Assets
 
   has_and_belongs_to_many :users, after_add: :cache_update, after_remove: :cache_update
-  has_and_belongs_to_many :permissions, after_add: :cache_update, after_remove: :cache_update, before_add: :validate_agent_limit
+  has_and_belongs_to_many :permissions, after_add: :cache_update, after_remove: :cache_update, before_add: :validate_agent_limit_by_permission, before_remove: :last_admin_check_by_permission
   validates               :name,  presence: true
   store                   :preferences
 
   before_create  :validate_permissions
-  before_update  :validate_permissions
+  before_update  :validate_permissions, :last_admin_check_by_attribute, :validate_agent_limit_by_attributes
 
   association_attributes_ignored :users
 
@@ -102,24 +102,52 @@ returns
 =end
 
   def self.with_permissions(keys)
+    permission_ids = Role.permission_ids_by_name(keys)
+    Role.joins(:roles_permissions).joins(:permissions).where(
+      'permissions_roles.permission_id IN (?) AND roles.active = ? AND permissions.active = ?', permission_ids, true, true
+    ).distinct()
+  end
+
+=begin
+
+check if roles is with permission
+
+  role = Role.find(123)
+  role.with_permission?('admin.session')
+
+get if role has permission of "admin.session" or "ticket.agent"
+
+  role.with_permission?(['admin.session', 'ticket.agent'])
+
+returns
+
+  true | false
+
+=end
+
+  def with_permission?(keys)
+    permission_ids = Role.permission_ids_by_name(keys)
+    return true if Role.joins(:roles_permissions).joins(:permissions).where(
+      'roles.id = ? AND permissions_roles.permission_id IN (?) AND permissions.active = ?', id, permission_ids, true
+    ).distinct().count.nonzero?
+    false
+  end
+
+  private_class_method
+
+  def self.permission_ids_by_name(keys)
     if keys.class != Array
       keys = [keys]
     end
-    roles = []
     permission_ids = []
     keys.each do |key|
-      Object.const_get('Permission').with_parents(key).each do |local_key|
-        permission = Object.const_get('Permission').lookup(name: local_key)
+      ::Permission.with_parents(key).each do |local_key|
+        permission = ::Permission.lookup(name: local_key)
         next if !permission
         permission_ids.push permission.id
       end
-      next if permission_ids.empty?
-      Role.joins(:roles_permissions).joins(:permissions).where('permissions_roles.permission_id IN (?) AND roles.active = ? AND permissions.active = ?', permission_ids, true, true).distinct().each do |role|
-        roles.push role
-      end
     end
-    return [] if roles.empty?
-    roles
+    permission_ids
   end
 
   private
@@ -140,14 +168,47 @@ returns
     true
   end
 
-  def validate_agent_limit(permission)
-    return true if !Setting.get('system_agent_limit')
-    return true if permission.name != 'ticket.agent'
+  def last_admin_check_by_attribute
+    return true if !will_save_change_to_attribute?('active')
+    return true if active != false
+    return true if !with_permission?(['admin', 'admin.user'])
+    raise Exceptions::UnprocessableEntity, 'Minimum one user needs to have admin permissions.' if last_admin_check_admin_count < 1
+    true
+  end
 
-    ticket_agent_role_ids = Role.joins(:permissions).where(permissions: { name: 'ticket.agent' }).pluck(:id)
+  def last_admin_check_by_permission(permission)
+    return true if Setting.get('import_mode')
+    return true if permission.name != 'admin' && permission.name != 'admin.user'
+    raise Exceptions::UnprocessableEntity, 'Minimum one user needs to have admin permissions.' if last_admin_check_admin_count < 1
+    true
+  end
+
+  def last_admin_check_admin_count
+    admin_role_ids = Role.joins(:permissions).where(permissions: { name: ['admin', 'admin.user'], active: true }, roles: { active: true }).where.not(id: id).pluck(:id)
+    User.joins(:roles).where(roles: { id: admin_role_ids }, users: { active: true }).count
+  end
+
+  def validate_agent_limit_by_attributes
+    return true if !Setting.get('system_agent_limit')
+    return true if !will_save_change_to_attribute?('active')
+    return true if active != true
+    return true if !with_permission?('ticket.agent')
+    ticket_agent_role_ids = Role.joins(:permissions).where(permissions: { name: 'ticket.agent', active: true }, roles: { active: true }).pluck(:id)
+    currents = User.joins(:roles).where(roles: { id: ticket_agent_role_ids }, users: { active: true }).pluck(:id)
+    news = User.joins(:roles).where(roles: { id: id }, users: { active: true }).pluck(:id)
+    count = currents.concat(news).uniq.count
+    raise Exceptions::UnprocessableEntity, 'Agent limit exceeded, please check your account settings.' if count > Setting.get('system_agent_limit')
+    true
+  end
+
+  def validate_agent_limit_by_permission(permission)
+    return true if !Setting.get('system_agent_limit')
+    return true if active != true
+    return true if permission.active != true
+    return true if permission.name != 'ticket.agent'
+    ticket_agent_role_ids = Role.joins(:permissions).where(permissions: { name: 'ticket.agent' }, roles: { active: true }).pluck(:id)
     ticket_agent_role_ids.push(id)
     count = User.joins(:roles).where(roles: { id: ticket_agent_role_ids }, users: { active: true }).count
-
     raise Exceptions::UnprocessableEntity, 'Agent limit exceeded, please check your account settings.' if count > Setting.get('system_agent_limit')
     true
   end

app/models/user.rb

@@ -40,12 +40,12 @@ class User < ApplicationModel
 
   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
+  before_update   :check_preferences_default, :validate_roles, :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, before_remove: :last_admin_check, 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, 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
@@ -283,7 +283,7 @@ returns
 
     sleep 1
     user.login_failed += 1
-    user.save
+    user.save!
     nil
   end
 
@@ -977,10 +977,10 @@ returns
     raise Exceptions::UnprocessableEntity, 'out of office no such replacement user' if !User.find_by(id: out_of_office_replacement_id)
     true
   end
+
 =begin
 
-checks if the current user is the last one
-with admin permissions.
+checks if the current user is the last one with admin permissions.
 
 Raises
 
@@ -988,28 +988,47 @@ raise 'Minimum one user need to have admin permissions'
 
 =end
 
-  def last_admin_check(role)
-    return true if Setting.get('import_mode')
-
-    ticket_admin_role_ids = Role.joins(:permissions).where(permissions: { name: ['admin', 'admin.user'] }).pluck(:id)
-    count                 = User.joins(:roles).where(roles: { id: ticket_admin_role_ids }, users: { active: true }).count
-    if ticket_admin_role_ids.include?(role.id)
-      count -= 1
-    end
-
-    raise Exceptions::UnprocessableEntity, 'Minimum one user needs to have admin permissions.' if count < 1
+  def last_admin_check_by_attribute
+    return true if !will_save_change_to_attribute?('active')
+    return true if active != false
+    return true if !permissions?(['admin', 'admin.user'])
+    raise Exceptions::UnprocessableEntity, 'Minimum one user needs to have admin permissions.' if last_admin_check_admin_count < 1
     true
   end
 
-  def validate_agent_limit(role)
-    return true if !Setting.get('system_agent_limit')
+  def last_admin_check_by_role(role)
+    return true if Setting.get('import_mode')
+    return true if !role.with_permission?(['admin', 'admin.user'])
+    raise Exceptions::UnprocessableEntity, 'Minimum one user needs to have admin permissions.' if last_admin_check_admin_count < 1
+    true
+  end
 
-    ticket_agent_role_ids = Role.joins(:permissions).where(permissions: { name: 'ticket.agent' }).pluck(:id)
+  def last_admin_check_admin_count
+    admin_role_ids = Role.joins(:permissions).where(permissions: { name: ['admin', 'admin.user'], active: true }, roles: { active: true }).pluck(:id)
+    User.joins(:roles).where(roles: { id: admin_role_ids }, users: { active: true }).count - 1
+  end
+
+  def validate_agent_limit_by_attributes
+    return true if !Setting.get('system_agent_limit')
+    return true if !will_save_change_to_attribute?('active')
+    return true if active != true
+    return true if !permissions?('ticket.agent')
+    ticket_agent_role_ids = Role.joins(:permissions).where(permissions: { name: 'ticket.agent', active: true }, roles: { active: true }).pluck(:id)
+    count                 = User.joins(:roles).where(roles: { id: ticket_agent_role_ids }, users: { active: true }).count + 1
+    raise Exceptions::UnprocessableEntity, 'Agent limit exceeded, please check your account settings.' if count > Setting.get('system_agent_limit')
+    true
+  end
+
+  def validate_agent_limit_by_role(role)
+    return true if !Setting.get('system_agent_limit')
+    return true if active != true
+    return true if role.active != true
+    return true if !role.with_permission?('ticket.agent')
+    ticket_agent_role_ids = Role.joins(:permissions).where(permissions: { name: 'ticket.agent', active: true }, roles: { active: true }).pluck(:id)
     count                 = User.joins(:roles).where(roles: { id: ticket_agent_role_ids }, users: { active: true }).count
     if ticket_agent_role_ids.include?(role.id)
       count += 1
     end
-
     raise Exceptions::UnprocessableEntity, 'Agent limit exceeded, please check your account settings.' if count > Setting.get('system_agent_limit')
     true
   end

test/unit/role_test.rb

@@ -91,7 +91,7 @@ class RoleTest < ActiveSupport::TestCase
 
   test 'permission default' do
     roles = Role.with_permissions('not_existing')
-    assert(roles.empty?)
+    assert(roles.blank?)
 
     roles = Role.with_permissions('admin')
     assert_equal('Admin', roles.first.name)
@@ -116,4 +116,26 @@ class RoleTest < ActiveSupport::TestCase
 
   end
 
+  test 'with permission' do
+    permission_test1 = Permission.create_or_update(
+      name: 'test-with-permission1',
+      note: 'parent test permission 1',
+    )
+    permission_test2 = Permission.create_or_update(
+      name: 'test-with-permission2',
+      note: 'parent test permission 2',
+    )
+    name = rand(999_999_999)
+    role = Role.create(
+      name: "Test with Permission? #{name}",
+      note: "Test with Permission? #{name} Role.",
+      permissions: [permission_test2],
+      updated_by_id: 1,
+      created_by_id: 1
+    )
+    assert_not(role.with_permission?('test-with-permission1'))
+    assert(role.with_permission?('test-with-permission2'))
+    assert(role.with_permission?(['test-with-permission2', 'some_other_permission']))
+  end
+
 end

test/unit/role_validate_agent_limit_test.rb

@@ -4,47 +4,48 @@ require 'test_helper'
 class RoleValidateAgentLimit < ActiveSupport::TestCase
   test 'role_validate_agent_limit' do
 
-    users = User.of_role('Agent')
+    agent_max = User.with_permissions('ticket.agent').count
     UserInfo.current_user_id = 1
-    Setting.set('system_agent_limit', users.count + 2)
+    Setting.set('system_agent_limit', agent_max + 2)
 
     permission_ticket_agent = Permission.where(name: 'ticket.agent')
 
-    role_agent_limit_success = Role.create(
+    role_agent_limit_success = Role.create!(
       name: 'agent-limit-test-success',
       note: 'agent-limit-test-success Role.',
       permissions: [],
-      updated_by_id: 1,
-      created_by_id: 1
+      active: true,
     )
-    role_agent_limit_fail = Role.create(
+    role_agent_limit_fail = Role.create!(
       name: 'agent-limit-test-fail',
       note: 'agent-limit-test-fail Role.',
       permissions: [],
-      updated_by_id: 1,
-      created_by_id: 1
+      active: true,
     )
 
-    user1 = User.create(
-      firstname:   'Firstname',
-      lastname:    'Lastname',
-      email:       'some@example.com',
-      login:       'some-agentlimit@example.com',
-      roles:       [role_agent_limit_success],
+    user1 = User.create!(
+      firstname: 'Firstname',
+      lastname:  'Lastname',
+      email:     'some-agentlimit-role@example.com',
+      login:     'some-agentlimit-role@example.com',
+      roles:     [role_agent_limit_success],
+      active:    true,
     )
-    user2 = User.create(
-      firstname:   'Firstname1',
-      lastname:    'Lastname1',
-      email:       'some-agentlimit-1@example.com',
-      login:       'some-agentlimit-1@example.com',
-      roles:       [role_agent_limit_success],
+    user2 = User.create!(
+      firstname: 'Firstname1',
+      lastname:  'Lastname1',
+      email:     'some-agentlimit-role-1@example.com',
+      login:     'some-agentlimit-role-1@example.com',
+      roles:     [role_agent_limit_success],
+      active:    true,
     )
-    user3 = User.create(
+    user3 = User.create!(
       firstname: 'Firstname2',
       lastname:  'Lastname2',
-      email:     'some-agentlimit-2@example.com',
-      login:     'some-agentlimit-2@example.com',
+      email:     'some-agentlimit-role-2@example.com',
+      login:     'some-agentlimit-role-2@example.com',
       roles:     [role_agent_limit_fail],
+      active:    true,
     )
 
     role_agent_limit_success.permissions = permission_ticket_agent
@@ -52,11 +53,21 @@ class RoleValidateAgentLimit < ActiveSupport::TestCase
       role_agent_limit_fail.permissions = permission_ticket_agent
     end
 
-    user1.destroy
-    user2.destroy
-    user3.destroy
-    role_agent_limit_success.destroy
-    role_agent_limit_fail.destroy
+    role_agent_limit_fail.active = false
+    role_agent_limit_fail.save!
+
+    role_agent_limit_fail.permissions = permission_ticket_agent
+
+    assert_raises(Exceptions::UnprocessableEntity) do
+      role_agent_limit_fail.active = true
+      role_agent_limit_fail.save!
+    end
+
+    user1.destroy!
+    user2.destroy!
+    user3.destroy!
+    role_agent_limit_success.destroy!
+    role_agent_limit_fail.destroy!
     Setting.set('system_agent_limit', nil)
   end
 end

test/unit/user_test.rb

@@ -864,6 +864,25 @@ class UserTest < ActiveSupport::TestCase
 
     admin_count_inital = User.with_permissions('admin').count
     assert_equal(1, admin_count_inital)
+
+    assert_raises(Exceptions::UnprocessableEntity) do
+      admin3.active = false
+      admin3.save!
+    end
+
+    assert_equal(1, User.with_permissions('admin').count)
+    admin_role = Role.find_by(name: 'Admin')
+    assert_raises(Exceptions::UnprocessableEntity) do
+      admin_role.active = false
+      admin_role.save!
+    end
+
+    assert_raises(Exceptions::UnprocessableEntity) do
+      admin_role.permission_revoke('admin')
+    end
+
+    assert_equal(1, User.with_permissions('admin').count)
+
   end
 
   test 'only valid agent in group permission check' do

test/unit/user_validate_agent_limit_test.rb

@@ -4,52 +4,96 @@ require 'test_helper'
 class UserValidateAgentLimit < ActiveSupport::TestCase
   test 'user_validate_agent_limit' do
 
-    users = User.of_role('Agent')
     UserInfo.current_user_id = 1
-    Setting.set('system_agent_limit', users.count + 2)
+    agent_max = User.with_permissions('ticket.agent').count + 2
+    Setting.set('system_agent_limit', agent_max)
     role_agent = Role.lookup(name: 'Agent')
     role_customer = Role.lookup(name: 'Customer')
 
-    user1 = User.create(
-      firstname:   'Firstname',
-      lastname:    'Lastname',
-      email:       'some@example.com',
-      login:       'some-agentlimit@example.com',
-      roles:       [role_agent],
+    user1 = User.create!(
+      firstname: 'Firstname',
+      lastname:  'Lastname',
+      email:     'some-agentlimit-user@example.com',
+      login:     'some-agentlimit-user@example.com',
+      roles:     [role_agent],
+      active:    true,
     )
-    user2 = User.create(
-      firstname:   'Firstname1',
-      lastname:    'Lastname1',
-      email:       'some-agentlimit-1@example.com',
-      login:       'some-agentlimit-1@example.com',
-      roles:       [role_agent],
+    user2 = User.create!(
+      firstname: 'Firstname1',
+      lastname:  'Lastname1',
+      email:     'some-agentlimit-user-1@example.com',
+      login:     'some-agentlimit-user-1@example.com',
+      roles:     [role_agent],
+      active:    true,
     )
 
     assert_raises(Exceptions::UnprocessableEntity) do
-      user3 = User.create(
+      user3 = User.create!(
         firstname: 'Firstname2',
         lastname:  'Lastname2',
-        email:     'some-agentlimit-2@example.com',
-        login:     'some-agentlimit-2@example.com',
+        email:     'some-agentlimit-user-2@example.com',
+        login:     'some-agentlimit-user-2@example.com',
         roles:     [role_agent],
+        active:    true,
       )
     end
 
-    user3 = User.create(
+    user3 = User.create!(
       firstname: 'Firstname2',
       lastname:  'Lastname2',
-      email:     'some-agentlimit-2@example.com',
-      login:     'some-agentlimit-2@example.com',
+      email:     'some-agentlimit-user-2@example.com',
+      login:     'some-agentlimit-user-2@example.com',
       roles:     [role_customer],
+      active:    true,
     )
 
     assert_raises(Exceptions::UnprocessableEntity) do
       user3.roles = [role_agent]
     end
 
-    user1.destroy
-    user2.destroy
-    user3.destroy
+    assert_equal(User.with_permissions('ticket.agent').count, agent_max)
+
+    Setting.set('system_agent_limit', agent_max + 1)
+    user3.reload
+    user3.roles = [role_agent]
+    user3.save!
+
+    user3.active = false
+    user3.save!
+
+    Setting.set('system_agent_limit', agent_max)
+
+    # try to activate inactive agent again
+    assert_raises(Exceptions::UnprocessableEntity) do
+      user3 = User.find(user3.id)
+      user3.active = true
+      user3.save!
+    end
+
+    assert_equal(User.with_permissions('ticket.agent').count, agent_max)
+
+    # try to activate inactive role again
+    role_agent_limit = Role.create!(
+      name: 'agent-limit-test-invalid-role',
+      note: 'agent-limit-test-invalid-role Role.',
+      permissions: Permission.where(name: 'ticket.agent'),
+      active: false,
+    )
+    user3.roles =  [role_agent_limit]
+    user3.active = true
+    user3.save!
+
+    assert_raises(Exceptions::UnprocessableEntity) do
+      role_agent_limit.active = true
+      role_agent_limit.save!
+    end
+
+    assert_equal(User.with_permissions('ticket.agent').count, agent_max)
+
+    user1.destroy!
+    user2.destroy!
+    user3.destroy!
+    role_agent_limit.destroy!
     Setting.set('system_agent_limit', nil)
   end
 end