diff --git a/app/controllers/user_access_token_controller.rb b/app/controllers/user_access_token_controller.rb index 56d0d4077..782189cad 100644 --- a/app/controllers/user_access_token_controller.rb +++ b/app/controllers/user_access_token_controller.rb @@ -32,7 +32,7 @@ curl http://localhost/api/v1/user_access_token -v -u #{login}:#{password} .order(updated_at: :desc, label: :asc) base_query = Permission.order(:name).where(active: true) - permission_names = current_user.permissions.keys + permission_names = current_user.permissions.pluck(:name) ancestor_names = permission_names.flat_map { |name| Permission.with_parents(name) }.uniq - permission_names descendant_names = permission_names.map { |name| "#{name}.%" } diff --git a/app/models/concerns/can_be_authorized.rb b/app/models/concerns/can_be_authorized.rb new file mode 100644 index 000000000..beaeb67b4 --- /dev/null +++ b/app/models/concerns/can_be_authorized.rb @@ -0,0 +1,40 @@ +module CanBeAuthorized + extend ActiveSupport::Concern + +=begin + +true or false for permission + + user = User.find(123) + user.permissions?('permission.key') # access to certain permission.key + user.permissions?(['permission.key1', 'permission.key2']) # access to permission.key1 or permission.key2 + + user.permissions?('permission') # access to all sub keys + + user.permissions?('permission.*') # access if one sub key access exists + +returns + + true|false + +=end + + def permissions?(auth_query) + verbatim, wildcards = acceptable_permissions_for(auth_query) + + permissions.where(name: verbatim).then do |base_query| + wildcards.reduce(base_query) do |query, name| + query.or(permissions.where('permissions.name LIKE ?', name.sub('.*', '.%'))) + end + end.exists? + end + + private + + def acceptable_permissions_for(auth_query) + Array(auth_query) + .reject { |name| Permission.lookup(name: name)&.active == false } # See "chain-of-ancestry quirk" in spec file + .flat_map { |name| Permission.with_parents(name) }.uniq + .partition { |name| name.end_with?('.*') }.reverse + end +end diff --git a/app/models/token.rb b/app/models/token.rb index 48e41ef60..0c84aca5a 100644 --- a/app/models/token.rb +++ b/app/models/token.rb @@ -1,6 +1,8 @@ # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ class Token < ApplicationModel + include CanBeAuthorized + before_create :generate_token belongs_to :user, optional: true store :preferences @@ -99,15 +101,17 @@ cleanup old token true end - def permissions?(permissions) - return false if !user.permissions?(permissions) - return false if preferences[:permission].blank? + def permissions + Permission.where( + name: Array(preferences[:permission]), + active: true, + ) + end - Array(permissions).any? do |parentless| - Permission.with_parents(parentless).any? do |permission| - preferences[:permission].include?(permission) - end - end + def permissions?(names) + return false if !user.permissions?(names) + + super(names) end private diff --git a/app/models/user.rb b/app/models/user.rb index 317a94da7..a3d376d1b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2,6 +2,7 @@ require_dependency 'karma/user' class User < ApplicationModel + include CanBeAuthorized include CanBeImported include HasActivityStreamLog include ChecksClientNotification @@ -22,6 +23,7 @@ class User < ApplicationModel has_many :tokens, after_add: :cache_update, after_remove: :cache_update has_many :authorizations, after_add: :cache_update, after_remove: :cache_update belongs_to :organization, inverse_of: :members, optional: true + has_many :permissions, -> { where(roles: { active: true }, active: true) }, through: :roles before_validation :check_name, :check_email, :check_login, :ensure_uniq_email, :ensure_password, :ensure_roles, :ensure_identifier before_validation :check_mail_delivery_failed, on: :update @@ -42,6 +44,8 @@ class User < ApplicationModel :image_source, :preferences + association_attributes_ignored :permissions + history_attributes_ignored :password, :last_login, :image, @@ -377,69 +381,6 @@ returns =begin -get all permissions of user - - user = User.find(123) - user.permissions - -returns - - { - 'permission.key' => true, - # ... - } - -=end - - def permissions - ::Permission.joins(roles: :users) - .where(users: { id: id }, roles: { active: true }, active: true) - .pluck(:name, :preferences) - .each_with_object({}) do |(name, preferences), hash| - hash[name] = true if preferences['selectable'] != false - end - end - -=begin - -true or false for permission - - user = User.find(123) - user.permissions?('permission.key') # access to certain permission.key - user.permissions?(['permission.key1', 'permission.key2']) # access to permission.key1 or permission.key2 - - user.permissions?('permission') # access to all sub keys - - user.permissions?('permission.*') # access if one sub key access exists - -returns - - true|false - -=end - - def permissions?(names) - base_query = ::Permission.joins(roles: :users) - .where(users: { id: id }) - .where(roles: { active: true }) - .where(active: true) - - permission_names = Array(names).reject { |name| ::Permission.lookup(name: name)&.active == false } - verbatim_names = permission_names.flat_map { |name| ::Permission.with_parents(name) }.uniq - wildcard_names = permission_names.select { |name| name.end_with?('.*') } - .map { |name| name.sub('.*', '.%') } - - permissions = base_query.where(name: verbatim_names) - - wildcard_names.each do |name| - permissions = permissions.or(base_query.where('permissions.name LIKE ?', name)) - end - - permissions.exists? - end - -=begin - returns all accessable permission ids of user user = User.find(123) @@ -454,7 +395,7 @@ returns def permissions_with_child_ids where = '' where_bind = [true] - permissions.each_key do |permission_name| + permissions.pluck(:name).each do |permission_name| where += ' OR ' if where != '' where += 'permissions.name = ? OR permissions.name LIKE ?' where_bind.push permission_name diff --git a/spec/models/token_spec.rb b/spec/models/token_spec.rb index f499c59e6..bf145577b 100644 --- a/spec/models/token_spec.rb +++ b/spec/models/token_spec.rb @@ -127,6 +127,135 @@ RSpec.describe Token, type: :model do end end + describe '#permissions?' do + subject(:token) do + create(:token, user: user, preferences: { permission: [permission_name] }) + end + + let(:user) { create(:user, roles: [role]) } + let(:role) { create(:role, permissions: [permission]) } + let(:permission) { create(:permission, name: permission_name) } + + context 'with privileges for a root permission (e.g., "foo", not "foo.bar")' do + let(:permission_name) { 'foo' } + + context 'when given that exact permission' do + it 'returns true' do + expect(token.permissions?('foo')).to be(true) + end + end + + context 'when given a sub-permission (i.e., child permission)' do + let(:subpermission) { create(:permission, name: 'foo.bar') } + + context 'that exists' do + before { subpermission } + + it 'returns true' do + expect(token.permissions?('foo.bar')).to be(true) + end + end + + context 'that is inactive' do + before { subpermission.update(active: false) } + + it 'returns false' do + expect(token.permissions?('foo.bar')).to be(false) + end + end + + context 'that does not exist' do + it 'returns true' do + expect(token.permissions?('foo.bar')).to be(true) + end + end + end + + context 'when given a glob' do + context 'matching that permission' do + it 'returns true' do + expect(token.permissions?('foo.*')).to be(true) + end + end + + context 'NOT matching that permission' do + it 'returns false' do + expect(token.permissions?('bar.*')).to be(false) + end + end + end + end + + context 'with privileges for a sub-permission (e.g., "foo.bar", not "foo")' do + let(:permission_name) { 'foo.bar' } + + context 'when given that exact sub-permission' do + it 'returns true' do + expect(token.permissions?('foo.bar')).to be(true) + end + + context 'but the permission is inactive' do + before { permission.update(active: false) } + + it 'returns false' do + expect(token.permissions?('foo.bar')).to be(false) + end + end + end + + context 'when given a sibling sub-permission' do + let(:sibling_permission) { create(:permission, name: 'foo.baz') } + + context 'that exists' do + before { sibling_permission } + + it 'returns false' do + expect(token.permissions?('foo.baz')).to be(false) + end + end + + context 'that does not exist' do + it 'returns false' do + expect(token.permissions?('foo.baz')).to be(false) + end + end + end + + context 'when given the parent permission' do + it 'returns false' do + expect(token.permissions?('foo')).to be(false) + end + end + + context 'when given a glob' do + context 'matching that sub-permission' do + it 'returns true' do + expect(token.permissions?('foo.*')).to be(true) + end + + context 'but the permission is inactive' do + before { permission.update(active: false) } + + context 'and user.permissions?(...) doesn’t fail' do + let(:role) { create(:role, permissions: [parent_permission]) } + let(:parent_permission) { create(:permission, name: permission_name.split('.').first) } + + it 'returns false' do + expect(token.permissions?('foo.*')).to be(false) + end + end + end + end + + context 'NOT matching that sub-permission' do + it 'returns false' do + expect(token.permissions?('bar.*')).to be(false) + end + end + end + end + end + describe 'Attributes:' do describe '#persistent' do context 'when not set on creation' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6a84c691c..8735b0637 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -374,44 +374,24 @@ RSpec.describe User, type: :model do let(:role) { user.roles.first } let(:permissions) { role.permissions } - it 'returns a hash of => true' do - expect(user.permissions).to eq(permissions.each.with_object({}) { |p, hash| hash[p.name] = true }) - end - - shared_examples 'for omissions' do - it 'omits them from the returned hash' do - expect(user.permissions.keys).not_to include(*omitted_permissions.map(&:name)) - end - end - - context 'for permissions that do not belong to this user' do - let(:omitted_permissions) { Permission.all - permissions } - - include_examples 'for omissions' + it 'is a simple association getter' do + expect(user.permissions).to match_array(permissions) end context 'for inactive permissions' do - before { omitted_permissions.each { |p| p.update(active: false) } } + before { permissions.first.update(active: false) } - let!(:omitted_permissions) { permissions.first(1) } - - include_examples 'for omissions' + it 'omits them from the returned hash' do + expect(user.permissions).not_to include(permissions.first) + end end context 'for permissions on inactive roles' do before { role.update(active: false) } - let(:omitted_permissions) { permissions } - - include_examples 'for omissions' - end - - context 'for permissions with !preferences["selectable"]' do - before { omitted_permissions.each { |p| p.update(preferences: { selectable: false }) } } - - let(:omitted_permissions) { permissions.first(1) } - - include_examples 'for omissions' + it 'omits them from the returned hash' do + expect(user.permissions).not_to include(*role.permissions) + end end end @@ -430,28 +410,28 @@ RSpec.describe User, type: :model do end end - context 'when given a sub-permission (i.e., child permission)' do - let(:subpermission) { create(:permission, name: 'foo.bar') } + context 'when given an active sub-permission' do + before { create(:permission, name: 'foo.bar') } - context 'that exists' do - before { subpermission } + it 'returns true' do + expect(user.permissions?('foo.bar')).to be(true) + end + end - it 'returns true' do - expect(user.permissions?('foo.bar')).to be(true) + describe 'chain-of-ancestry quirk' do + context 'when given an inactive sub-permission' do + before { create(:permission, name: 'foo.bar.baz', active: false) } + + it 'returns false, even with active ancestors' do + expect(user.permissions?('foo.bar.baz')).to be(false) end end - context 'that is inactive' do - before { subpermission.update(active: false) } + context 'when given a sub-permission that does not exist' do + before { create(:permission, name: 'foo.bar', active: false) } - it 'returns false' do - expect(user.permissions?('foo.bar')).to be(false) - end - end - - context 'that does not exist' do - it 'returns true' do - expect(user.permissions?('foo.bar')).to be(true) + it 'can return true, even with inactive ancestors' do + expect(user.permissions?('foo.bar.baz')).to be(true) end end end @@ -522,7 +502,7 @@ RSpec.describe User, type: :model do before { permission.update(active: false) } it 'returns false' do - expect(user.permissions?('foo.bar')).to be(false) + expect(user.permissions?('foo.*')).to be(false) end end end