Don't allow global memberships on placeholder users

The view_all_principals migration added global roles to all users with
manage_members.

On QA, we had placeholder users with this permission. Placeholder users
should never receive global roles, but there was no validation in place.

This commit adds a migration to remove those, and adds a validation to
the contract.
This commit is contained in:
Oliver Günther
2025-10-29 14:47:38 +01:00
parent 771618d664
commit 8456e1a2e5
4 changed files with 35 additions and 1 deletions
+8
View File
@@ -32,6 +32,7 @@ module Members
class BaseContract < ::ModelContract
delegate :principal,
:project,
:project_id,
:new_record?,
to: :model
@@ -41,6 +42,7 @@ module Members
validate :roles_grantable
validate :project_set
validate :project_manageable
validate :validate_no_global_placeholder
def assignable_projects
Project
@@ -50,6 +52,12 @@ module Members
private
def validate_no_global_placeholder
return unless principal.is_a?(PlaceholderUser)
errors.add :principal, :invalid if project_id.nil?
end
def user_allowed_to_manage
errors.add :base, :error_unauthorized unless user_allowed_to_manage?
end
@@ -80,8 +80,10 @@ class AddViewAllPrincipalsPermissionToExistingRoles < ActiveRecord::Migration[8.
return [] if project_roles_with_manage_members.empty?
# Find all users who have manage_members permission in any project
Member.joins(member_roles: :role)
# but avoid selecting PlaceholderUser
Member.joins(:principal, member_roles: :role)
.where(member_roles: { roles: { id: project_roles_with_manage_members.pluck(:id) } })
.where.not("users.type": "PlaceholderUser")
.pluck(:user_id)
.uniq
end
@@ -0,0 +1,17 @@
# frozen_string_literal: true
class AvoidGlobalRoleOnPlaceholderUser < ActiveRecord::Migration[8.0]
def up
execute <<~SQL.squish
DELETE FROM members
USING users
WHERE members.user_id = users.id
AND members.project_id IS NULL
AND users.type = 'PlaceholderUser'
SQL
end
def down
# Nothing to do
end
end
@@ -61,6 +61,13 @@ RSpec.describe Members::CreateContract do
it_behaves_like "contract is invalid", principal: :unassignable
end
context "if the principal is a placeholder user and the project is nil" do
let(:member_project) { nil }
let(:member_principal) { build(:placeholder_user) }
it_behaves_like "contract is invalid", principal: :invalid
end
end
describe "#assignable_projects" do