From 04072e79ef034856c8811221d4f67a1449b86362 Mon Sep 17 00:00:00 2001 From: Niklas van Schrick Date: Mon, 16 Mar 2026 22:22:52 +0100 Subject: [PATCH] Prevent role deletion if no member has a different admin role --- .../roles/assign_abilities_service.rb | 1 + .../namespaces/roles/delete_service.rb | 1 + .../roles/assign_abilities_mutation_spec.rb | 1 + .../namespace/roles/delete_mutation_spec.rb | 1 + .../roles/assign_abilities_service_spec.rb | 25 ++++++++++++++++++- .../namespaces/roles/delete_service_spec.rb | 25 ++++++++++++++++++- 6 files changed, 52 insertions(+), 2 deletions(-) diff --git a/app/services/namespaces/roles/assign_abilities_service.rb b/app/services/namespaces/roles/assign_abilities_service.rb index c33fb28d..326bec64 100644 --- a/app/services/namespaces/roles/assign_abilities_service.rb +++ b/app/services/namespaces/roles/assign_abilities_service.rb @@ -63,6 +63,7 @@ def check_admin_existing(t) unless role.namespace.roles.where.not(id: role.id) .joins(:abilities) + .joins(:member_roles) .exists?(abilities: { ability: :namespace_administrator }) || abilities.include?(:namespace_administrator) t.rollback_and_return! ServiceResponse.error( diff --git a/app/services/namespaces/roles/delete_service.rb b/app/services/namespaces/roles/delete_service.rb index 425c6175..10477130 100644 --- a/app/services/namespaces/roles/delete_service.rb +++ b/app/services/namespaces/roles/delete_service.rb @@ -20,6 +20,7 @@ def execute if !namespace_role.namespace.has_owner? && !namespace_role.namespace.roles.where.not(id: namespace_role.id) .joins(:abilities) + .joins(:member_roles) .exists?(abilities: { ability: :namespace_administrator }) return ServiceResponse.error(message: 'Cannot delete last administrator role', error_code: :cannot_delete_last_admin_role) diff --git a/spec/requests/graphql/mutation/namespace/roles/assign_abilities_mutation_spec.rb b/spec/requests/graphql/mutation/namespace/roles/assign_abilities_mutation_spec.rb index 237a1648..8ca71c58 100644 --- a/spec/requests/graphql/mutation/namespace/roles/assign_abilities_mutation_spec.rb +++ b/spec/requests/graphql/mutation/namespace/roles/assign_abilities_mutation_spec.rb @@ -30,6 +30,7 @@ before do create(:namespace_role, namespace: namespace_role.namespace).tap do |role| create(:namespace_role_ability, namespace_role: role, ability: :namespace_administrator) + create(:namespace_member_role, role: role, member: create(:namespace_member, namespace: role.namespace)) end end diff --git a/spec/requests/graphql/mutation/namespace/roles/delete_mutation_spec.rb b/spec/requests/graphql/mutation/namespace/roles/delete_mutation_spec.rb index 01afc875..55b86365 100644 --- a/spec/requests/graphql/mutation/namespace/roles/delete_mutation_spec.rb +++ b/spec/requests/graphql/mutation/namespace/roles/delete_mutation_spec.rb @@ -38,6 +38,7 @@ before do create(:namespace_role, namespace: namespace).tap do |role| create(:namespace_role_ability, namespace_role: role, ability: :namespace_administrator) + create(:namespace_member_role, role: role, member: create(:namespace_member, namespace: role.namespace)) end end diff --git a/spec/services/namespaces/roles/assign_abilities_service_spec.rb b/spec/services/namespaces/roles/assign_abilities_service_spec.rb index 37872314..95f2b1ac 100644 --- a/spec/services/namespaces/roles/assign_abilities_service_spec.rb +++ b/spec/services/namespaces/roles/assign_abilities_service_spec.rb @@ -9,12 +9,17 @@ let(:role) { create(:namespace_role) } let(:abilities) { [] } - let!(:admin_role) do + let(:admin_role) do create(:namespace_role, namespace: role.namespace).tap do |role| create(:namespace_role_ability, namespace_role: role, ability: :namespace_administrator) end end + let!(:admin_role_member) do + member = create(:namespace_member, namespace: admin_role.namespace, user: create(:user)) + create(:namespace_member_role, role: admin_role, member: member) + end + context 'when user is nil' do let(:current_user) { nil } @@ -74,6 +79,24 @@ expect { service_response }.not_to create_audit_event end end + + context 'when another role has the namespace_administrator ability but no members' do + let(:abilities) { [] } + + before do + stub_allowed_ability(NamespacePolicy, :assign_role_abilities, user: current_user, subject: role.namespace) + create(:namespace_role_ability, namespace_role: role, ability: :namespace_administrator) + admin_role_member.delete + end + + it { is_expected.not_to be_success } + it { expect(service_response.payload[:error_code]).to eq(:cannot_remove_last_admin_ability) } + it { expect { service_response }.not_to change { NamespaceRoleAbility.count } } + + it do + expect { service_response }.not_to create_audit_event + end + end end context 'when adding an ability' do diff --git a/spec/services/namespaces/roles/delete_service_spec.rb b/spec/services/namespaces/roles/delete_service_spec.rb index 28689843..c48b9a1f 100644 --- a/spec/services/namespaces/roles/delete_service_spec.rb +++ b/spec/services/namespaces/roles/delete_service_spec.rb @@ -48,6 +48,7 @@ end it { is_expected.not_to be_success } + it { expect(service_response.payload[:error_code]).to eq(:cannot_delete_last_admin_role) } it { expect { service_response }.not_to change { NamespaceRole.count } } it do @@ -55,11 +56,33 @@ end end - context 'when user is a member' do + context 'when another role has the namespace_administrator ability but no members' do let(:current_user) { create(:user) } before do create(:namespace_member, namespace: namespace_role.namespace, user: current_user) + create(:namespace_role_ability, namespace_role: namespace_role, ability: :namespace_administrator) + create(:namespace_member_role, member: create(:namespace_member, namespace: namespace_role.namespace), + role: namespace_role) + stub_allowed_ability(NamespacePolicy, :delete_namespace_role, user: current_user, + subject: namespace_role.namespace) + end + + it { is_expected.not_to be_success } + it { expect(service_response.payload[:error_code]).to eq(:cannot_delete_last_admin_role) } + it { expect { service_response }.not_to change { NamespaceRole.count } } + + it do + expect { service_response }.not_to create_audit_event + end + end + + context 'when user is a member' do + let(:current_user) { create(:user) } + + before do + member = create(:namespace_member, namespace: namespace_role.namespace, user: current_user) + create(:namespace_member_role, member: member, role: admin_role) stub_allowed_ability(NamespacePolicy, :delete_namespace_role, user: current_user, subject: namespace_role.namespace) end