diff --git a/app/contracts/members/base_contract.rb b/app/contracts/members/base_contract.rb index c1775a416ca..112fbfb05dc 100644 --- a/app/contracts/members/base_contract.rb +++ b/app/contracts/members/base_contract.rb @@ -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 diff --git a/db/migrate/20250929070310_add_view_all_principals_permission_to_existing_roles.rb b/db/migrate/20250929070310_add_view_all_principals_permission_to_existing_roles.rb index f9d38a93111..60d8cf7b09f 100644 --- a/db/migrate/20250929070310_add_view_all_principals_permission_to_existing_roles.rb +++ b/db/migrate/20250929070310_add_view_all_principals_permission_to_existing_roles.rb @@ -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 diff --git a/db/migrate/20251029134217_avoid_global_role_on_placeholder_user.rb b/db/migrate/20251029134217_avoid_global_role_on_placeholder_user.rb new file mode 100644 index 00000000000..e67fda49e96 --- /dev/null +++ b/db/migrate/20251029134217_avoid_global_role_on_placeholder_user.rb @@ -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 diff --git a/spec/contracts/members/create_contract_spec.rb b/spec/contracts/members/create_contract_spec.rb index 88d5e4c7e30..7b60c17d051 100644 --- a/spec/contracts/members/create_contract_spec.rb +++ b/spec/contracts/members/create_contract_spec.rb @@ -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