Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions app/models/usergroup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
114 changes: 113 additions & 1 deletion test/models/usergroup_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading