diff --git a/app/models/user.rb b/app/models/user.rb index bbf2c60dd3..ea80ef8132 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -457,6 +457,10 @@ def can_escalate? admin? || can?(:escalate_roles) end + def can_assign_for_usergroup?(role_ids, usergroup, new_role_ids) + can_escalate_excluding_roles?(usergroup, new_role_ids) || role_ids.all? { |r| role_ids_was.include?(r) } + end + # only admin can change admin flag def can_change_admin_flag? admin? @@ -610,6 +614,16 @@ def authenticate_by_personal_access_token(token) private + def can_escalate_excluding_roles?(usergroup, role_ids) + return true if admin? + tainted_user_role_ids = UserRole.where(owner: usergroup, role_id: role_ids).pluck(:id) + cached_user_roles + .where.not(user_role_id: tainted_user_role_ids) + .joins(role: { filters: :permissions }) + .where(permissions: { name: :escalate_roles }) + .exists? + end + def prepare_password if password.present? self.password_salt = salt_password diff --git a/app/models/usergroup.rb b/app/models/usergroup.rb index ff4c738078..3575b2b103 100644 --- a/app/models/usergroup.rb +++ b/app/models/usergroup.rb @@ -41,6 +41,7 @@ class Usergroup < ApplicationRecord scoped_search :relation => :roles, :on => :id, :rename => :role_id, :complete_enabled => false, :only_explicit => true, :validator => ScopedSearch::Validators::INTEGER validate :ensure_uniq_name, :ensure_last_admin_remains_admin validate :admin_can_be_set, :if => ->(o) { o.changed.include?('admin') } + validate :ensure_roles_not_escalated accepts_nested_attributes_for :external_usergroups, :reject_if => ->(a) { a[:name].blank? }, :allow_destroy => true @@ -131,4 +132,16 @@ def other_admins def admin_can_be_set errors.add :admin, _('admin flag can only be modified by admins') unless User.current.admin? end + + def ensure_roles_not_escalated + return if User.current.nil? + + roles_check = new_record? ? role_ids.present? : role_ids_changed? + if roles_check + new_role_ids = new_record? ? role_ids : role_ids - role_ids_was + unless User.current.can_assign_for_usergroup?(role_ids, self, new_role_ids) + errors.add :role_ids, _("you can't assign some of roles you selected") + end + end + end end diff --git a/test/models/usergroup_test.rb b/test/models/usergroup_test.rb index 1f198c63bd..0ab0e87e0d 100644 --- a/test/models/usergroup_test.rb +++ b/test/models/usergroup_test.rb @@ -209,7 +209,119 @@ def populate_usergroups assert_equal recipients, [users[0]] end - # TODO test who can modify usergroup roles and who can assign users!!! possible privileges escalation + context 'role assignment (privilege escalation)' do + setup do + @owned_role = Role.where(:name => "usergroup_test_owned_role").first_or_create + @foreign_role = Role.where(:name => "usergroup_test_foreign_role").first_or_create + end + + test "non-admin with create_usergroups cannot assign roles they do not have" do + setup_user "create", "usergroups" + usergroup = Usergroup.new(:name => "escalation-blocked-create", :role_ids => [@foreign_role.id]) + refute usergroup.valid? + assert_includes usergroup.errors.attribute_names, :role_ids + end + + test "non-admin with create_usergroups can assign roles they already hold" do + setup_user "create", "usergroups" do |user| + user.roles << @owned_role + end + usergroup = Usergroup.new(:name => "delegated-roles-ok", :role_ids => [@owned_role.id]) + assert_valid usergroup + assert usergroup.save + end + + test "non-admin with edit_usergroups cannot add roles they do not hold" do + usergroup = as_admin { FactoryBot.create(:usergroup, :name => "escalation-update-target", :role_ids => [@owned_role.id]) } + setup_user "edit", "usergroups" do |user| + user.roles << @owned_role + end + usergroup.role_ids = usergroup.role_ids + [@foreign_role.id] + refute usergroup.valid? + assert_includes usergroup.errors.attribute_names, :role_ids + end + + test "non-admin member of usergroup cannot add a role granting escalate_roles" do + usergroup = as_admin { FactoryBot.create(:usergroup, :name => "escalate-via-membership") } + escalate_role = as_admin do + role = FactoryBot.create(:role, :name => "role_with_escalate") + FactoryBot.create(:filter, :role => role, :permissions => [permissions(:escalate_roles)]) + role + end + setup_user "edit", "usergroups" do |user| + usergroup.users << user + end + usergroup.role_ids = [escalate_role.id] + refute usergroup.valid? + assert_includes usergroup.errors.attribute_names, :role_ids + end + + test "member of usergroup with escalate_roles can add other roles to that usergroup" do + escalate_role = as_admin do + role = FactoryBot.create(:role, :name => "role_with_escalate_for_member") + FactoryBot.create(:filter, :role => role, :permissions => [permissions(:escalate_roles)]) + role + end + usergroup = as_admin { FactoryBot.create(:usergroup, :name => "escalate-member-adds-role", :role_ids => [escalate_role.id]) } + setup_user "edit", "usergroups" do |user| + usergroup.users << user + end + usergroup.role_ids = usergroup.role_ids + [@foreign_role.id] + assert_valid usergroup + end + + test "transitive member of usergroup cannot add a role granting escalate_roles" do + parent_usergroup = as_admin { FactoryBot.create(:usergroup, :name => "escalate-transitive-parent") } + child_usergroup = as_admin { FactoryBot.create(:usergroup, :name => "escalate-transitive-child") } + as_admin { parent_usergroup.usergroups << child_usergroup } + escalate_role = as_admin do + role = FactoryBot.create(:role, :name => "role_with_escalate_transitive") + FactoryBot.create(:filter, :role => role, :permissions => [permissions(:escalate_roles)]) + role + end + setup_user "edit", "usergroups" do |user| + child_usergroup.users << user + end + parent_usergroup.role_ids = [escalate_role.id] + refute parent_usergroup.valid? + assert_includes parent_usergroup.errors.attribute_names, :role_ids + end + + test "non-admin cannot create usergroup with self as member and a role granting escalate_roles" do + escalate_role = as_admin do + role = FactoryBot.create(:role, :name => "role_with_escalate_create_self") + FactoryBot.create(:filter, :role => role, :permissions => [permissions(:escalate_roles)]) + role + end + setup_user "create", "usergroups" + usergroup = Usergroup.new(:name => "escalate-create-self", :role_ids => [escalate_role.id], :user_ids => [User.current.id]) + refute usergroup.valid? + assert_includes usergroup.errors.attribute_names, :role_ids + end + + test "admin can assign arbitrary roles to usergroup" do + as_admin do + usergroup = Usergroup.new(:name => "admin-assigns-roles", :role_ids => [@foreign_role.id]) + assert_valid usergroup + assert usergroup.save + end + end + + test "user with escalate_roles permission can assign roles they do not hold to usergroup" do + user = FactoryBot.create(:user, :admin => false) + escalate_role = FactoryBot.create(:role) + FactoryBot.create(:user_role, :owner => user, :role => escalate_role) + FactoryBot.create(:filter, :role => escalate_role, :permissions => [permissions(:escalate_roles)]) + usergroup_role = FactoryBot.create(:role) + FactoryBot.create(:user_role, :owner => user, :role => usergroup_role) + FactoryBot.create(:filter, :role => usergroup_role, :permissions => [permissions(:create_usergroups)]) + as_user user do + usergroup = Usergroup.new(:name => "escalate-roles-ok", :role_ids => [@foreign_role.id]) + assert_valid usergroup + assert usergroup.save + end + end + end context 'external usergroups' do setup do