From 2c39eb47dec79441fba86e35bd844217cf98c066 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Fri, 20 Oct 2017 13:32:52 +0200 Subject: [PATCH] Fixed issue #1563 - prevent admin from locking out. --- app/models/role.rb | 93 +++++++++++++++++---- app/models/user.rb | 57 ++++++++----- test/unit/role_test.rb | 24 +++++- test/unit/role_validate_agent_limit_test.rb | 67 ++++++++------- test/unit/user_test.rb | 19 +++++ test/unit/user_validate_agent_limit_test.rb | 90 +++++++++++++++----- 6 files changed, 263 insertions(+), 87 deletions(-) diff --git a/app/models/role.rb b/app/models/role.rb index 864fbd8d2..5130be108 100644 --- a/app/models/role.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 7bf3fdb24..762809762 100644 --- a/app/models/user.rb +++ b/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 diff --git a/test/unit/role_test.rb b/test/unit/role_test.rb index 65dedd62c..81bb767ef 100644 --- a/test/unit/role_test.rb +++ b/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 diff --git a/test/unit/role_validate_agent_limit_test.rb b/test/unit/role_validate_agent_limit_test.rb index f3505a594..5395d0c5e 100644 --- a/test/unit/role_validate_agent_limit_test.rb +++ b/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 diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index 5dc194b27..375ef2b81 100644 --- a/test/unit/user_test.rb +++ b/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 diff --git a/test/unit/user_validate_agent_limit_test.rb b/test/unit/user_validate_agent_limit_test.rb index 88860ed4b..855b640ce 100644 --- a/test/unit/user_validate_agent_limit_test.rb +++ b/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