From dc7354223a780dc17cbc6b801ced079555a493b2 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Thu, 22 Sep 2016 21:05:29 +0200 Subject: [PATCH 1/3] Added permission.active attribute to disable features. --- app/assets/javascripts/app/models/user.coffee | 8 +- .../user_access_token_controller.rb | 2 +- app/models/role.rb | 2 +- app/models/user.rb | 10 ++- db/migrate/20120101000001_create_base.rb | 1 + .../20160921000001_permission_active.rb.rb | 10 +++ test/unit/permission_test.rb | 73 +++++++++++++------ 7 files changed, 76 insertions(+), 30 deletions(-) create mode 100644 db/migrate/20160921000001_permission_active.rb.rb diff --git a/app/assets/javascripts/app/models/user.coffee b/app/assets/javascripts/app/models/user.coffee index b5237eaa9..6dce53e05 100644 --- a/app/assets/javascripts/app/models/user.coffee +++ b/app/assets/javascripts/app/models/user.coffee @@ -188,6 +188,11 @@ class App.User extends App.Model # if any permission exists return true if _.contains(keys, '*') + # verify direct permissions + for key in keys + permission = App.Permission.findByAttribute('name', key) + return false if permission && permission.active is false + # get all permissions of user permissions = {} for role_id in @role_ids @@ -195,7 +200,8 @@ class App.User extends App.Model if role.active is true for permission_id in role.permission_ids permission = App.Permission.find(permission_id) - permissions[permission.name] = true + if permission.active is true + permissions[permission.name] = true for localKey in keys requiredPermissions = localKey.split('+') diff --git a/app/controllers/user_access_token_controller.rb b/app/controllers/user_access_token_controller.rb index 33d9e291c..a125df2dd 100644 --- a/app/controllers/user_access_token_controller.rb +++ b/app/controllers/user_access_token_controller.rb @@ -26,7 +26,7 @@ class UserAccessTokenController < ApplicationController } } permissions = [] - Permission.all.order(:name).each { |permission| + Permission.all.where(active: true).order(:name).each { |permission| next if !local_permissions_new.key?(permission.name) && !current_user.permissions?(permission.name) permission_attributes = permission.attributes if local_permissions_new[permission.name] == false diff --git a/app/models/role.rb b/app/models/role.rb index 133208f4a..c668f6b4b 100644 --- a/app/models/role.rb +++ b/app/models/role.rb @@ -107,7 +107,7 @@ returns permission_ids.push permission.id } next if permission_ids.empty? - Role.joins(:roles_permissions).where('permissions_roles.permission_id IN (?) AND roles.active = ?', permission_ids, true).uniq().each { |role| + Role.joins(:roles_permissions).joins(:permissions).where('permissions_roles.permission_id IN (?) AND roles.active = ? AND permissions.active = ?', permission_ids, true, true).uniq().each { |role| roles.push role } } diff --git a/app/models/user.rb b/app/models/user.rb index dabdc7f41..0cb5218e1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -334,7 +334,7 @@ returns def permissions list = {} - Object.const_get('Permission').select('permissions.name, permissions.preferences').joins(:roles).where('roles.id IN (?)', role_ids).pluck(:name, :preferences).each { |permission| + Object.const_get('Permission').select('permissions.name, permissions.preferences').joins(:roles).where('roles.id IN (?) AND permissions.active = ?', role_ids, true).pluck(:name, :preferences).each { |permission| next if permission[1]['selectable'] == false list[permission[0]] = true } @@ -375,10 +375,12 @@ returns if local_key =~ /\.\*$/ local_key.sub!('.*', '.%') permissions = Object.const_get('Permission').with_parents(local_key) - list = Object.const_get('Permission').select('preferences').joins(:roles).where('roles.id IN (?) AND roles.active = ? AND (permissions.name IN (?) OR permissions.name LIKE ?)', role_ids, true, permissions, local_key).pluck(:preferences) + list = Object.const_get('Permission').select('preferences').joins(:roles).where('roles.id IN (?) AND roles.active = ? AND (permissions.name IN (?) OR permissions.name LIKE ?) AND permissions.active = ?', role_ids, true, permissions, local_key, true).pluck(:preferences) else + permission = Object.const_get('Permission').lookup(name: local_key) + break if permission && permission.active == false permissions = Object.const_get('Permission').with_parents(local_key) - list = Object.const_get('Permission').select('preferences').joins(:roles).where('roles.id IN (?) AND roles.active = ? AND permissions.name IN (?)', role_ids, true, permissions).pluck(:preferences) + list = Object.const_get('Permission').select('preferences').joins(:roles).where('roles.id IN (?) AND roles.active = ? AND permissions.name IN (?) AND permissions.active = ?', role_ids, true, permissions, true).pluck(:preferences) end list.each { |preferences| next if preferences[:selectable] == false @@ -420,7 +422,7 @@ returns permission_ids.push permission.id } next if permission_ids.empty? - Role.joins(:roles_permissions).where('permissions_roles.permission_id IN (?) AND roles.active = ?', permission_ids, true).uniq().pluck(:id).each { |role_id| + Role.joins(:roles_permissions).joins(:permissions).where('permissions_roles.permission_id IN (?) AND roles.active = ? AND permissions.active = ?', permission_ids, true, true).uniq().pluck(:id).each { |role_id| role_ids.push role_id } total_role_ids.push role_ids diff --git a/db/migrate/20120101000001_create_base.rb b/db/migrate/20120101000001_create_base.rb index c73d971e4..13cffdab4 100644 --- a/db/migrate/20120101000001_create_base.rb +++ b/db/migrate/20120101000001_create_base.rb @@ -109,6 +109,7 @@ class CreateBase < ActiveRecord::Migration t.string :name, limit: 255, null: false t.string :note, limit: 500, null: true t.string :preferences, limit: 10_000, null: true + t.boolean :active, null: false, default: true t.timestamps limit: 3, null: false end add_index :permissions, [:name], unique: true diff --git a/db/migrate/20160921000001_permission_active.rb.rb b/db/migrate/20160921000001_permission_active.rb.rb new file mode 100644 index 000000000..e70098a4f --- /dev/null +++ b/db/migrate/20160921000001_permission_active.rb.rb @@ -0,0 +1,10 @@ +class PermissionActive < ActiveRecord::Migration + def up + # return if it's a new setup + return if !Setting.find_by(name: 'system_init_done') + + add_column :permissions, :active, :boolean, null: false, default: true + + Cache.clear + end +end diff --git a/test/unit/permission_test.rb b/test/unit/permission_test.rb index 8f66b5826..1dc2a59e1 100644 --- a/test/unit/permission_test.rb +++ b/test/unit/permission_test.rb @@ -12,10 +12,17 @@ class PermissionTest < ActiveSupport::TestCase test 'user permission' do - Permission.create_if_not_exists( + permission1 = Permission.create_or_update( name: 'admin.permission1', note: 'Admin Interface', preferences: {}, + active: true, + ) + permission2 = Permission.create_or_update( + name: 'admin.permission2', + note: 'Admin Interface', + preferences: {}, + active: true, ) role_permission1 = Role.create_or_update( name: 'AdminPermission1', @@ -27,6 +34,7 @@ class PermissionTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) + role_permission1.permission_revoke('admin') role_permission1.permission_grand('admin.permission1') user_with_permission1 = User.create_or_update( login: 'setting-permission1', @@ -39,24 +47,43 @@ class PermissionTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) + assert_equal(true, user_with_permission1.permissions?('admin.permission1')) assert_equal(true, user_with_permission1.permissions?('admin.*')) assert_equal(false, user_with_permission1.permissions?('admi.*')) assert_equal(false, user_with_permission1.permissions?('admin.permission2')) assert_equal(false, user_with_permission1.permissions?('admin')) + permission1.active = false + permission1.save! + + assert_equal(false, user_with_permission1.permissions?('admin.permission1')) + assert_equal(false, user_with_permission1.permissions?('admin.*')) + assert_equal(false, user_with_permission1.permissions?('admi.*')) + assert_equal(false, user_with_permission1.permissions?('admin.permission2')) + assert_equal(false, user_with_permission1.permissions?('admin')) + + role_permission1.permission_grand('admin') + + assert_equal(false, user_with_permission1.permissions?('admin.permission1')) + assert_equal(true, user_with_permission1.permissions?('admin.*')) + assert_equal(false, user_with_permission1.permissions?('admi.*')) + assert_equal(true, user_with_permission1.permissions?('admin.permission2')) + assert_equal(true, user_with_permission1.permissions?('admin')) + end test 'user permission with invalid role' do - Permission.create_if_not_exists( - name: 'admin.permission2', + permission3 = Permission.create_or_update( + name: 'admin.permission3', note: 'Admin Interface', preferences: {}, + active: true, ) - role_permission2 = Role.create_or_update( + role_permission3 = Role.create_or_update( name: 'AdminPermission2', - note: 'To configure your permission2.', + note: 'To configure your permission3.', preferences: { not: ['Customer'], }, @@ -65,32 +92,32 @@ class PermissionTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) - role_permission2.permission_grand('admin.permission2') - user_with_permission2 = User.create_or_update( - login: 'setting-permission2', + role_permission3.permission_grand('admin.permission3') + user_with_permission3 = User.create_or_update( + login: 'setting-permission3', firstname: 'Setting', lastname: 'Admin Permission2', - email: 'setting-admin-permission2@example.com', + email: 'setting-admin-permission3@example.com', password: 'some_pw', active: true, - roles: [role_permission2], + roles: [role_permission3], updated_by_id: 1, created_by_id: 1, ) - assert_equal(true, user_with_permission2.permissions?('admin.permission2')) - assert_equal(true, user_with_permission2.permissions?('admin.*')) - assert_equal(false, user_with_permission2.permissions?('admi.*')) - assert_equal(false, user_with_permission2.permissions?('admin.permission3')) - assert_equal(false, user_with_permission2.permissions?('admin')) + assert_equal(true, user_with_permission3.permissions?('admin.permission3')) + assert_equal(true, user_with_permission3.permissions?('admin.*')) + assert_equal(false, user_with_permission3.permissions?('admi.*')) + assert_equal(false, user_with_permission3.permissions?('admin.permission4')) + assert_equal(false, user_with_permission3.permissions?('admin')) - role_permission2.active = false - role_permission2.save - user_with_permission2.reload - assert_equal(false, user_with_permission2.permissions?('admin.permission2')) - assert_equal(false, user_with_permission2.permissions?('admin.*')) - assert_equal(false, user_with_permission2.permissions?('admi.*')) - assert_equal(false, user_with_permission2.permissions?('admin.permission3')) - assert_equal(false, user_with_permission2.permissions?('admin')) + role_permission3.active = false + role_permission3.save + user_with_permission3.reload + assert_equal(false, user_with_permission3.permissions?('admin.permission3')) + assert_equal(false, user_with_permission3.permissions?('admin.*')) + assert_equal(false, user_with_permission3.permissions?('admi.*')) + assert_equal(false, user_with_permission3.permissions?('admin.permission4')) + assert_equal(false, user_with_permission3.permissions?('admin')) end From b937ef93711cd2da43cbba425fe36ceb5866ef32 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Tue, 18 Oct 2016 18:28:38 +0200 Subject: [PATCH 2/3] Improved error handling. --- app/assets/javascripts/app/models/user.coffee | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/assets/javascripts/app/models/user.coffee b/app/assets/javascripts/app/models/user.coffee index 4a19eca1c..71bd25604 100644 --- a/app/assets/javascripts/app/models/user.coffee +++ b/app/assets/javascripts/app/models/user.coffee @@ -195,6 +195,8 @@ class App.User extends App.Model if role.active is true for permission_id in role.permission_ids permission = App.Permission.findNative(permission_id) + if !permission + throw "No such permission for id #{permission_id}" permissions[permission.name] = true for localKey in keys From b2d2b047d9733956cc912fa0bea4493e8a70b810 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Tue, 18 Oct 2016 21:52:51 +0200 Subject: [PATCH 3/3] Added cleanup. --- test/unit/permission_test.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/unit/permission_test.rb b/test/unit/permission_test.rb index 1dc2a59e1..90c47eca6 100644 --- a/test/unit/permission_test.rb +++ b/test/unit/permission_test.rb @@ -71,6 +71,8 @@ class PermissionTest < ActiveSupport::TestCase assert_equal(true, user_with_permission1.permissions?('admin.permission2')) assert_equal(true, user_with_permission1.permissions?('admin')) + role_permission1.permission_revoke('admin') + end test 'user permission with invalid role' do