From 8a0dd684664df15d1d5c6ea51c5f3161176e490a Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Tue, 21 Oct 2025 09:26:07 +1300 Subject: [PATCH 1/5] Allow instance admins to hard-delete AI items Provides a `forceHardDelete` argument for AI Catalog item mutations that can only be used by an instance admin. It will always hard-delete the item, and never soft-delete. This is a measure of security in the case where something must be deleted and any active uses of the item must also be deleted. Part of https://gitlab.com/gitlab-org/gitlab/-/issues/575127 --- doc/api/graphql/reference/_index.md | 3 ++ .../mutations/ai/catalog/agent/delete.rb | 19 ++++++-- .../mutations/ai/catalog/flow/delete.rb | 19 ++++++-- .../ai/catalog/third_party_flow/delete.rb | 19 ++++++-- ee/app/policies/ai/catalog/item_policy.rb | 6 ++- .../ai/catalog/items/base_destroy_service.rb | 48 ++++++++++++++----- .../mutations/ai/catalog/agent/delete_spec.rb | 2 +- .../mutations/ai/catalog/flow/delete_spec.rb | 2 +- .../catalog/third_party_flow/delete_spec.rb | 2 +- .../policies/ai/catalog/item_policy_spec.rb | 15 ++++++ .../mutations/ai/catalog/agent/delete_spec.rb | 20 ++++++++ .../mutations/ai/catalog/flow/delete_spec.rb | 24 +++++++++- .../catalog/third_party_flow/delete_spec.rb | 20 ++++++++ .../base_destroy_service_shared_examples.rb | 36 ++++++++++++++ 14 files changed, 207 insertions(+), 28 deletions(-) diff --git a/doc/api/graphql/reference/_index.md b/doc/api/graphql/reference/_index.md index 0798b9f78063d3..d081843768c8c5 100644 --- a/doc/api/graphql/reference/_index.md +++ b/doc/api/graphql/reference/_index.md @@ -2561,6 +2561,7 @@ Input type: `AiCatalogAgentDeleteInput` | Name | Type | Description | | ---- | ---- | ----------- | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `forceHardDelete` | [`Boolean`](#boolean) | When true, the flow will always be hard deleted and never soft deleted. Can only be used by instance admins. | | `id` | [`AiCatalogItemID!`](#aicatalogitemid) | Global ID of the catalog Agent to delete. | #### Fields @@ -2674,6 +2675,7 @@ Input type: `AiCatalogFlowDeleteInput` | Name | Type | Description | | ---- | ---- | ----------- | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `forceHardDelete` | [`Boolean`](#boolean) | When true, the flow will always be hard deleted and never soft deleted. Can only be used by instance admins. | | `id` | [`AiCatalogItemID!`](#aicatalogitemid) | Global ID of the catalog flow to delete. | #### Fields @@ -2858,6 +2860,7 @@ Input type: `AiCatalogThirdPartyFlowDeleteInput` | Name | Type | Description | | ---- | ---- | ----------- | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `forceHardDelete` | [`Boolean`](#boolean) | When true, the Third Party Flow will always be hard deleted and never soft deleted. Can only be used by instance admins. | | `id` | [`AiCatalogItemID!`](#aicatalogitemid) | Global ID of the catalog Third Party Flow to delete. | #### Fields diff --git a/ee/app/graphql/mutations/ai/catalog/agent/delete.rb b/ee/app/graphql/mutations/ai/catalog/agent/delete.rb index a3c7bab6813744..ea6134ad3dd6a4 100644 --- a/ee/app/graphql/mutations/ai/catalog/agent/delete.rb +++ b/ee/app/graphql/mutations/ai/catalog/agent/delete.rb @@ -15,15 +15,28 @@ class Delete < BaseMutation required: true, description: 'Global ID of the catalog Agent to delete.' + argument :force_hard_delete, GraphQL::Types::Boolean, + required: false, + description: 'When true, the flow will always be hard deleted and never soft deleted. ' \ + 'Can only be used by instance admins' + authorize :admin_ai_catalog_item def resolve(args) - agent = authorized_find!(id: args[:id]) + id = args.delete(:id) + + item = authorized_find!(id: id) + + if args[:force_hard_delete] && !current_user.can_admin_all_resources? + raise_resource_not_available_error!('You must be an instance admin to use forceHardDelete') + end + + service_args = args.merge(item: item) result = ::Ai::Catalog::Agents::DestroyService.new( - project: agent.project, + project: service_args[:item].project, current_user: current_user, - params: { item: agent }).execute + params: service_args).execute { success: result.success?, diff --git a/ee/app/graphql/mutations/ai/catalog/flow/delete.rb b/ee/app/graphql/mutations/ai/catalog/flow/delete.rb index 6fa4c6193f3aea..1154aedbda9a9e 100644 --- a/ee/app/graphql/mutations/ai/catalog/flow/delete.rb +++ b/ee/app/graphql/mutations/ai/catalog/flow/delete.rb @@ -15,15 +15,28 @@ class Delete < BaseMutation required: true, description: 'Global ID of the catalog flow to delete.' + argument :force_hard_delete, GraphQL::Types::Boolean, + required: false, + description: 'When true, the flow will always be hard deleted and never soft deleted. ' \ + 'Can only be used by instance admins' + authorize :admin_ai_catalog_item def resolve(args) - flow = authorized_find!(id: args[:id]) + id = args.delete(:id) + + item = authorized_find!(id: id) + + if args[:force_hard_delete] && !current_user.can_admin_all_resources? + raise_resource_not_available_error!('You must be an instance admin to use forceHardDelete') + end + + service_args = args.merge(item: item) result = ::Ai::Catalog::Flows::DestroyService.new( - project: flow.project, + project: service_args[:item].project, current_user: current_user, - params: { item: flow }).execute + params: service_args).execute { success: result.success?, diff --git a/ee/app/graphql/mutations/ai/catalog/third_party_flow/delete.rb b/ee/app/graphql/mutations/ai/catalog/third_party_flow/delete.rb index f9717cda260fb9..5f2757b02c040c 100644 --- a/ee/app/graphql/mutations/ai/catalog/third_party_flow/delete.rb +++ b/ee/app/graphql/mutations/ai/catalog/third_party_flow/delete.rb @@ -15,15 +15,28 @@ class Delete < BaseMutation required: true, description: 'Global ID of the catalog Third Party Flow to delete.' + argument :force_hard_delete, GraphQL::Types::Boolean, + required: false, + description: 'When true, the Third Party Flow will always be hard deleted and never soft deleted. ' \ + 'Can only be used by instance admins' + authorize :admin_ai_catalog_item def resolve(args) - third_party_flow = authorized_find!(id: args[:id]) + id = args.delete(:id) + + item = authorized_find!(id: id) + + if args[:force_hard_delete] && !current_user.can_admin_all_resources? + raise_resource_not_available_error!('You must be an instance admin to use forceHardDelete') + end + + service_args = args.merge(item: item) result = ::Ai::Catalog::ThirdPartyFlows::DestroyService.new( - project: third_party_flow.project, + project: service_args[:item].project, current_user: current_user, - params: { item: third_party_flow }).execute + params: service_args).execute { success: result.success?, diff --git a/ee/app/policies/ai/catalog/item_policy.rb b/ee/app/policies/ai/catalog/item_policy.rb index 74a193441dd1c1..f21e815cbf98fb 100644 --- a/ee/app/policies/ai/catalog/item_policy.rb +++ b/ee/app/policies/ai/catalog/item_policy.rb @@ -27,6 +27,10 @@ class ItemPolicy < ::BasePolicy can?(:maintainer_access, @subject.project) end + condition(:admin_access, scope: :user) do + can?(:admin_all_resources, @user) + end + condition(:public_item, scope: :subject, score: 0) do @subject.public? end @@ -56,7 +60,7 @@ class ItemPolicy < ::BasePolicy prevent :admin_ai_catalog_item end - rule { deleted_item }.policy do + rule { deleted_item & ~admin_access }.policy do prevent :admin_ai_catalog_item end diff --git a/ee/app/services/ai/catalog/items/base_destroy_service.rb b/ee/app/services/ai/catalog/items/base_destroy_service.rb index 510d7196e2d958..0ba3931e5a04ab 100644 --- a/ee/app/services/ai/catalog/items/base_destroy_service.rb +++ b/ee/app/services/ai/catalog/items/base_destroy_service.rb @@ -13,15 +13,11 @@ def execute return error_no_permissions unless allowed? return error_no_item unless valid? - result = delete_item_consumer - return error(result.errors) if result.error? + result = force_hard_delete? ? perform_hard_delete : perform_soft_delete - if delete_item - track_ai_item_events('delete_ai_catalog_item', { label: item.item_type }) - return success - end + track_deletion_event if result.success? - error_response + result end private @@ -44,11 +40,25 @@ def error_no_item error('Item not found') end - def error_response - error(item.errors.full_messages) + def perform_hard_delete + item.class.transaction do + item.consumers.each_batch do |batch| + batch.delete_all + end + item.destroy + end + + ServiceResponse.success end - def delete_item_consumer + def perform_soft_delete + result = destroy_item_consumer + return result if result.error? + + destroy_item_with_strategy + end + + def destroy_item_consumer consumer = project.configured_ai_catalog_items.for_item(item).first return ServiceResponse.success unless consumer @@ -56,10 +66,22 @@ def delete_item_consumer Ai::Catalog::ItemConsumers::DestroyService.new(consumer, current_user).execute end - def delete_item - return item.soft_delete if item.consumers.any? || item.dependents.any? + def destroy_item_with_strategy + success = if item.consumers.any? || item.dependents.any? + item.soft_delete + else + item.destroy + end + + success ? ServiceResponse.success : error(item.errors.full_messages) + end + + def force_hard_delete? + params[:force_hard_delete] == true && current_user.can_admin_all_resources? + end - item.destroy + def track_deletion_event + track_ai_item_events('delete_ai_catalog_item', { label: item.item_type }) end end end diff --git a/ee/spec/graphql/mutations/ai/catalog/agent/delete_spec.rb b/ee/spec/graphql/mutations/ai/catalog/agent/delete_spec.rb index e7349b88035ab4..8cd311dcd1a46a 100644 --- a/ee/spec/graphql/mutations/ai/catalog/agent/delete_spec.rb +++ b/ee/spec/graphql/mutations/ai/catalog/agent/delete_spec.rb @@ -13,5 +13,5 @@ it { is_expected.to have_graphql_fields(:success, :errors, :client_mutation_id) } - it { is_expected.to have_graphql_arguments(:id, :client_mutation_id) } + it { is_expected.to have_graphql_arguments(:id, :force_hard_delete, :client_mutation_id) } end diff --git a/ee/spec/graphql/mutations/ai/catalog/flow/delete_spec.rb b/ee/spec/graphql/mutations/ai/catalog/flow/delete_spec.rb index 197187f594232c..cfbf1bd1f500ca 100644 --- a/ee/spec/graphql/mutations/ai/catalog/flow/delete_spec.rb +++ b/ee/spec/graphql/mutations/ai/catalog/flow/delete_spec.rb @@ -13,5 +13,5 @@ it { is_expected.to have_graphql_fields(:success, :errors, :client_mutation_id) } - it { is_expected.to have_graphql_arguments(:id, :client_mutation_id) } + it { is_expected.to have_graphql_arguments(:id, :force_hard_delete, :client_mutation_id) } end diff --git a/ee/spec/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb b/ee/spec/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb index 1a95108b26edbd..f203641fa5721b 100644 --- a/ee/spec/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb +++ b/ee/spec/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb @@ -13,5 +13,5 @@ it { is_expected.to have_graphql_fields(:success, :errors, :client_mutation_id) } - it { is_expected.to have_graphql_arguments(:id, :client_mutation_id) } + it { is_expected.to have_graphql_arguments(:id, :force_hard_delete, :client_mutation_id) } end diff --git a/ee/spec/policies/ai/catalog/item_policy_spec.rb b/ee/spec/policies/ai/catalog/item_policy_spec.rb index ae1b43c4c0469b..63cb61ba88ff15 100644 --- a/ee/spec/policies/ai/catalog/item_policy_spec.rb +++ b/ee/spec/policies/ai/catalog/item_policy_spec.rb @@ -9,6 +9,7 @@ let_it_be(:maintainer) { create(:user) } let_it_be(:reporter) { create(:user) } let_it_be(:guest) { create(:user) } + let_it_be(:admin) { create(:admin) } let_it_be_with_reload(:private_project) do create(:project, :private, guests: guest, reporters: reporter, developers: developer, maintainers: maintainer) end @@ -239,4 +240,18 @@ it_behaves_like 'read-only permissions' end end + + context 'when admin', :enable_admin_mode do + let(:item) { private_item } + let(:current_user) { admin } + + context 'with deleted item' do + before do + item.deleted_at = 1.day.ago + end + + it { is_expected.to be_allowed(:admin_ai_catalog_item) } + it { is_expected.to be_allowed(:read_ai_catalog_item) } + end + end end diff --git a/ee/spec/requests/api/graphql/mutations/ai/catalog/agent/delete_spec.rb b/ee/spec/requests/api/graphql/mutations/ai/catalog/agent/delete_spec.rb index 425095ccfb29de..7a4d6d85722d05 100644 --- a/ee/spec/requests/api/graphql/mutations/ai/catalog/agent/delete_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/ai/catalog/agent/delete_spec.rb @@ -82,5 +82,25 @@ it 'destroy the agent versions' do expect { execute }.to change { Ai::Catalog::ItemVersion.count }.by(-1) end + + context 'with `forceHardDelete` argument', :enable_admin_mode do + let(:params) { super().merge(force_hard_delete: true) } + + it_behaves_like 'a mutation that returns top-level errors', errors: + ['You must be an instance admin to use forceHardDelete'] + + it 'does not destroy the agent' do + expect { execute }.not_to change { Ai::Catalog::Item.count } + end + + context 'when user is an admin' do + let(:current_user) { create(:admin) } + + it 'destroy the agent and returns a success response' do + expect { execute }.to change { Ai::Catalog::Item.count }.by(-1) + expect(graphql_data_at(:ai_catalog_agent_delete, :success)).to be(true) + end + end + end end end diff --git a/ee/spec/requests/api/graphql/mutations/ai/catalog/flow/delete_spec.rb b/ee/spec/requests/api/graphql/mutations/ai/catalog/flow/delete_spec.rb index 2ba25d003d3d25..08c61fafdc830c 100644 --- a/ee/spec/requests/api/graphql/mutations/ai/catalog/flow/delete_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/ai/catalog/flow/delete_spec.rb @@ -74,13 +74,33 @@ end context 'when destroy service succeeds' do - it 'destroy the agent and returns a success response' do + it 'destroy the flow and returns a success response' do expect { execute }.to change { Ai::Catalog::Item.count }.by(-1) expect(graphql_data_at(:ai_catalog_flow_delete, :success)).to be(true) end - it 'destroy the agent versions' do + it 'destroy the flow versions' do expect { execute }.to change { Ai::Catalog::ItemVersion.count }.by(-1) end + + context 'with `forceHardDelete` argument', :enable_admin_mode do + let(:params) { super().merge(force_hard_delete: true) } + + it_behaves_like 'a mutation that returns top-level errors', errors: + ['You must be an instance admin to use forceHardDelete'] + + it 'does not destroy the flow' do + expect { execute }.not_to change { Ai::Catalog::Item.count } + end + + context 'when user is an admin' do + let(:current_user) { create(:admin) } + + it 'destroy the flow and returns a success response' do + expect { execute }.to change { Ai::Catalog::Item.count }.by(-1) + expect(graphql_data_at(:ai_catalog_flow_delete, :success)).to be(true) + end + end + end end end diff --git a/ee/spec/requests/api/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb b/ee/spec/requests/api/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb index 47b9e075135965..a7f18e887142ae 100644 --- a/ee/spec/requests/api/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb @@ -82,5 +82,25 @@ it 'destroy the third_party_flow versions' do expect { execute }.to change { Ai::Catalog::ItemVersion.count }.by(-1) end + + context 'with `forceHardDelete` argument', :enable_admin_mode do + let(:params) { super().merge(force_hard_delete: true) } + + it_behaves_like 'a mutation that returns top-level errors', errors: + ['You must be an instance admin to use forceHardDelete'] + + it 'does not delete the third_party_flow' do + expect { execute }.not_to change { Ai::Catalog::Item.count } + end + + context 'when user is an admin' do + let(:current_user) { create(:admin) } + + it 'destroy the third_party_flow and returns a success response' do + expect { execute }.to change { Ai::Catalog::Item.count }.by(-1) + expect(graphql_data_at(:ai_catalog_third_party_flow_delete, :success)).to be(true) + end + end + end end end diff --git a/ee/spec/support/shared_examples/services/ai/catalog/items/base_destroy_service_shared_examples.rb b/ee/spec/support/shared_examples/services/ai/catalog/items/base_destroy_service_shared_examples.rb index 756f0a94f6da99..2d2fec28517a3c 100644 --- a/ee/spec/support/shared_examples/services/ai/catalog/items/base_destroy_service_shared_examples.rb +++ b/ee/spec/support/shared_examples/services/ai/catalog/items/base_destroy_service_shared_examples.rb @@ -3,6 +3,7 @@ RSpec.shared_examples Ai::Catalog::Items::BaseDestroyService do let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } + let_it_be(:admin) { create(:admin) } let(:params) { { item: item } } let(:service) { described_class.new(project: project, current_user: user, params: params) } @@ -13,6 +14,8 @@ it 'returns insufficient permissions error' do result = execute_service + binding.pry + expect(result).to be_error expect(result.errors).to contain_exactly('You have insufficient permissions') end @@ -62,6 +65,7 @@ expect { execute_service } .to trigger_internal_events('delete_ai_catalog_item') .with(user: user, project: project, additional_properties: { label: item.item_type }) + .and increment_usage_metrics('counts.count_total_delete_ai_catalog_item') end it 'returns success response' do @@ -88,6 +92,22 @@ end it_behaves_like 'soft deletes the item' + + context 'with `force_hard_delete` param', :enable_admin_mode do + let(:params) { super().merge(force_hard_delete: true) } + + it_behaves_like 'returns insufficient permissions error' + + context 'when the user is an admin' do + let(:user) { admin } + + it_behaves_like 'hard deletes the item' + + it 'destroys the item consumers' do + expect { execute_service }.to change { Ai::Catalog::ItemConsumer.count }.by(-1) + end + end + end end end @@ -149,6 +169,22 @@ end it_behaves_like 'soft deletes the item' + + context 'with `force_hard_delete` param', :enable_admin_mode do + let(:params) { super().merge(force_hard_delete: true) } + + it_behaves_like 'returns insufficient permissions error' + + context 'when the user is an admin' do + let(:user) { admin } + + it_behaves_like 'hard deletes the item' + + it 'destroys the version dependencies' do + expect { execute_service }.to change { Ai::Catalog::ItemVersionDependency.count }.by(-1) + end + end + end end end -- GitLab From bb8057edea6442faaa9015891bc7b2656bc918fb Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 22 Oct 2025 10:23:43 +1300 Subject: [PATCH 2/5] Add reviewer feedback --- ee/app/policies/ai/catalog/item_policy.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/ee/app/policies/ai/catalog/item_policy.rb b/ee/app/policies/ai/catalog/item_policy.rb index f21e815cbf98fb..f4f2e8970029ed 100644 --- a/ee/app/policies/ai/catalog/item_policy.rb +++ b/ee/app/policies/ai/catalog/item_policy.rb @@ -27,10 +27,6 @@ class ItemPolicy < ::BasePolicy can?(:maintainer_access, @subject.project) end - condition(:admin_access, scope: :user) do - can?(:admin_all_resources, @user) - end - condition(:public_item, scope: :subject, score: 0) do @subject.public? end @@ -60,7 +56,7 @@ class ItemPolicy < ::BasePolicy prevent :admin_ai_catalog_item end - rule { deleted_item & ~admin_access }.policy do + rule { deleted_item & ~admin }.policy do prevent :admin_ai_catalog_item end -- GitLab From 6fd1a4ab437bf2cde3f7060077508868a5ef114f Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 23 Oct 2025 16:15:19 +1300 Subject: [PATCH 3/5] Add reviewer feedback --- .../mutations/ai/catalog/agent/delete.rb | 2 +- .../mutations/ai/catalog/flow/delete.rb | 2 +- .../ai/catalog/third_party_flow/delete.rb | 2 +- ee/app/policies/ai/catalog/item_policy.rb | 16 ++++-- .../ai/catalog/items/base_destroy_service.rb | 10 +++- .../policies/ai/catalog/item_policy_spec.rb | 49 +++++++++++++------ .../mutations/ai/catalog/agent/delete_spec.rb | 2 +- .../mutations/ai/catalog/flow/delete_spec.rb | 2 +- .../catalog/third_party_flow/delete_spec.rb | 2 +- .../base_destroy_service_shared_examples.rb | 2 - 10 files changed, 61 insertions(+), 28 deletions(-) diff --git a/ee/app/graphql/mutations/ai/catalog/agent/delete.rb b/ee/app/graphql/mutations/ai/catalog/agent/delete.rb index ea6134ad3dd6a4..ec40660b59114a 100644 --- a/ee/app/graphql/mutations/ai/catalog/agent/delete.rb +++ b/ee/app/graphql/mutations/ai/catalog/agent/delete.rb @@ -27,7 +27,7 @@ def resolve(args) item = authorized_find!(id: id) - if args[:force_hard_delete] && !current_user.can_admin_all_resources? + if args[:force_hard_delete] && !Ability.allowed?(current_user, :force_hard_delete_ai_catalog_item, item) raise_resource_not_available_error!('You must be an instance admin to use forceHardDelete') end diff --git a/ee/app/graphql/mutations/ai/catalog/flow/delete.rb b/ee/app/graphql/mutations/ai/catalog/flow/delete.rb index 1154aedbda9a9e..1aa2a4a00852c2 100644 --- a/ee/app/graphql/mutations/ai/catalog/flow/delete.rb +++ b/ee/app/graphql/mutations/ai/catalog/flow/delete.rb @@ -27,7 +27,7 @@ def resolve(args) item = authorized_find!(id: id) - if args[:force_hard_delete] && !current_user.can_admin_all_resources? + if args[:force_hard_delete] && !Ability.allowed?(current_user, :force_hard_delete_ai_catalog_item, item) raise_resource_not_available_error!('You must be an instance admin to use forceHardDelete') end diff --git a/ee/app/graphql/mutations/ai/catalog/third_party_flow/delete.rb b/ee/app/graphql/mutations/ai/catalog/third_party_flow/delete.rb index 5f2757b02c040c..c1e883e1070f66 100644 --- a/ee/app/graphql/mutations/ai/catalog/third_party_flow/delete.rb +++ b/ee/app/graphql/mutations/ai/catalog/third_party_flow/delete.rb @@ -27,7 +27,7 @@ def resolve(args) item = authorized_find!(id: id) - if args[:force_hard_delete] && !current_user.can_admin_all_resources? + if args[:force_hard_delete] && !Ability.allowed?(current_user, :force_hard_delete_ai_catalog_item, item) raise_resource_not_available_error!('You must be an instance admin to use forceHardDelete') end diff --git a/ee/app/policies/ai/catalog/item_policy.rb b/ee/app/policies/ai/catalog/item_policy.rb index f4f2e8970029ed..e86871219bd1bd 100644 --- a/ee/app/policies/ai/catalog/item_policy.rb +++ b/ee/app/policies/ai/catalog/item_policy.rb @@ -48,24 +48,32 @@ class ItemPolicy < ::BasePolicy end rule { maintainer_access }.policy do - enable :admin_ai_catalog_item + enable :admin_ai_catalog_item # (Create and update) + enable :delete_ai_catalog_item end - rule { ~ai_catalog_enabled }.policy do + rule { admin }.policy do + enable :force_hard_delete_ai_catalog_item + end + + rule { ~ai_catalog_enabled & ~admin }.policy do prevent :read_ai_catalog_item prevent :admin_ai_catalog_item + prevent :delete_ai_catalog_item end rule { deleted_item & ~admin }.policy do prevent :admin_ai_catalog_item + prevent :delete_ai_catalog_item end - rule { ~public_item & ~project_ai_catalog_available }.policy do + rule { ~public_item & ~project_ai_catalog_available & ~admin }.policy do prevent :read_ai_catalog_item end - rule { ~project_ai_catalog_available }.policy do + rule { ~project_ai_catalog_available & ~admin }.policy do prevent :admin_ai_catalog_item + prevent :delete_ai_catalog_item end rule { flow & ~flows_enabled }.policy do diff --git a/ee/app/services/ai/catalog/items/base_destroy_service.rb b/ee/app/services/ai/catalog/items/base_destroy_service.rb index 0ba3931e5a04ab..2ccc61ca37e083 100644 --- a/ee/app/services/ai/catalog/items/base_destroy_service.rb +++ b/ee/app/services/ai/catalog/items/base_destroy_service.rb @@ -25,7 +25,13 @@ def execute attr_reader :item def allowed? - super && Ability.allowed?(current_user, :admin_ai_catalog_item, item) + allowed = super && Ability.allowed?(current_user, :delete_ai_catalog_item, item) + + if force_hard_delete? # rubocop:disable Style/IfUnlessModifier -- Improves readability + allowed &= Ability.allowed?(current_user, :force_hard_delete_ai_catalog_item, item) + end + + allowed end def valid? @@ -77,7 +83,7 @@ def destroy_item_with_strategy end def force_hard_delete? - params[:force_hard_delete] == true && current_user.can_admin_all_resources? + params[:force_hard_delete] == true end def track_deletion_event diff --git a/ee/spec/policies/ai/catalog/item_policy_spec.rb b/ee/spec/policies/ai/catalog/item_policy_spec.rb index 63cb61ba88ff15..5fdb2f02546ec6 100644 --- a/ee/spec/policies/ai/catalog/item_policy_spec.rb +++ b/ee/spec/policies/ai/catalog/item_policy_spec.rb @@ -37,13 +37,21 @@ shared_examples 'no permissions' do it { is_expected.to be_disallowed(:admin_ai_catalog_item) } + it { is_expected.to be_disallowed(:delete_ai_catalog_item) } + it { is_expected.to be_disallowed(:force_hard_delete_ai_catalog_item) } it { is_expected.to be_disallowed(:read_ai_catalog_item) } + + include_examples 'all permissions when admin' end shared_examples 'read-only permissions' do it { is_expected.to be_disallowed(:admin_ai_catalog_item) } + it { is_expected.to be_disallowed(:delete_ai_catalog_item) } + it { is_expected.to be_disallowed(:force_hard_delete_ai_catalog_item) } it { is_expected.to be_allowed(:read_ai_catalog_item) } + include_examples 'all permissions when admin' + it_behaves_like 'no permissions with global_ai_catalog feature flag disabled' it_behaves_like 'no permissions with project stage check false, unless item is public' it_behaves_like 'no permissions when project Duo features disabled, unless item is public' @@ -52,7 +60,11 @@ shared_examples 'read-write permissions' do it { is_expected.to be_allowed(:admin_ai_catalog_item) } + it { is_expected.to be_allowed(:delete_ai_catalog_item) } it { is_expected.to be_allowed(:read_ai_catalog_item) } + it { is_expected.to be_disallowed(:force_hard_delete_ai_catalog_item) } + + include_examples 'all permissions when admin' it_behaves_like 'no permissions with global_ai_catalog feature flag disabled' it_behaves_like 'no permissions with project stage check false, unless item is public' @@ -60,6 +72,17 @@ it_behaves_like 'read-only permissions with deleted item' end + shared_examples 'all permissions when admin' do + context 'when admin', :enable_admin_mode do + let(:current_user) { admin } + + it { is_expected.to be_allowed(:admin_ai_catalog_item) } + it { is_expected.to be_allowed(:delete_ai_catalog_item) } + it { is_expected.to be_allowed(:force_hard_delete_ai_catalog_item) } + it { is_expected.to be_allowed(:read_ai_catalog_item) } + end + end + shared_examples 'no permissions with global_ai_catalog feature flag disabled' do before do stub_feature_flags(global_ai_catalog: false) @@ -74,31 +97,43 @@ end it { is_expected.to be_disallowed(:admin_ai_catalog_item) } + it { is_expected.to be_disallowed(:delete_ai_catalog_item) } + it { is_expected.to be_disallowed(:force_hard_delete_ai_catalog_item) } it { is_expected.to be_allowed(:read_ai_catalog_item) } + + include_examples 'all permissions when admin' end shared_examples 'no permissions with project stage check false, unless item is public' do let(:stage_check) { false } it { is_expected.to be_disallowed(:admin_ai_catalog_item) } + it { is_expected.to be_disallowed(:delete_ai_catalog_item) } + it { is_expected.to be_disallowed(:force_hard_delete_ai_catalog_item) } it 'is expected not to allow read_ai_catalog_item, unless item is public' do allowed = item.public? expect(policy.allowed?(:read_ai_catalog_item)).to eq(allowed) end + + include_examples 'all permissions when admin' end shared_examples 'no permissions when project Duo features disabled, unless item is public' do let(:duo_features_enabled) { false } it { is_expected.to be_disallowed(:admin_ai_catalog_item) } + it { is_expected.to be_disallowed(:delete_ai_catalog_item) } + it { is_expected.to be_disallowed(:force_hard_delete_ai_catalog_item) } it 'is expected not to allow read_ai_catalog_item, unless item is public' do allowed = item.public? expect(policy.allowed?(:read_ai_catalog_item)).to eq(allowed) end + + include_examples 'all permissions when admin' end context 'when maintainer' do @@ -240,18 +275,4 @@ it_behaves_like 'read-only permissions' end end - - context 'when admin', :enable_admin_mode do - let(:item) { private_item } - let(:current_user) { admin } - - context 'with deleted item' do - before do - item.deleted_at = 1.day.ago - end - - it { is_expected.to be_allowed(:admin_ai_catalog_item) } - it { is_expected.to be_allowed(:read_ai_catalog_item) } - end - end end diff --git a/ee/spec/requests/api/graphql/mutations/ai/catalog/agent/delete_spec.rb b/ee/spec/requests/api/graphql/mutations/ai/catalog/agent/delete_spec.rb index 7a4d6d85722d05..f811da6a5b5ae5 100644 --- a/ee/spec/requests/api/graphql/mutations/ai/catalog/agent/delete_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/ai/catalog/agent/delete_spec.rb @@ -96,7 +96,7 @@ context 'when user is an admin' do let(:current_user) { create(:admin) } - it 'destroy the agent and returns a success response' do + it 'destroys the agent and returns a success response' do expect { execute }.to change { Ai::Catalog::Item.count }.by(-1) expect(graphql_data_at(:ai_catalog_agent_delete, :success)).to be(true) end diff --git a/ee/spec/requests/api/graphql/mutations/ai/catalog/flow/delete_spec.rb b/ee/spec/requests/api/graphql/mutations/ai/catalog/flow/delete_spec.rb index 08c61fafdc830c..e9be32510162ef 100644 --- a/ee/spec/requests/api/graphql/mutations/ai/catalog/flow/delete_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/ai/catalog/flow/delete_spec.rb @@ -96,7 +96,7 @@ context 'when user is an admin' do let(:current_user) { create(:admin) } - it 'destroy the flow and returns a success response' do + it 'destroys the flow and returns a success response' do expect { execute }.to change { Ai::Catalog::Item.count }.by(-1) expect(graphql_data_at(:ai_catalog_flow_delete, :success)).to be(true) end diff --git a/ee/spec/requests/api/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb b/ee/spec/requests/api/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb index a7f18e887142ae..5e4b208d99d4b9 100644 --- a/ee/spec/requests/api/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb @@ -96,7 +96,7 @@ context 'when user is an admin' do let(:current_user) { create(:admin) } - it 'destroy the third_party_flow and returns a success response' do + it 'destroys the third_party_flow and returns a success response' do expect { execute }.to change { Ai::Catalog::Item.count }.by(-1) expect(graphql_data_at(:ai_catalog_third_party_flow_delete, :success)).to be(true) end diff --git a/ee/spec/support/shared_examples/services/ai/catalog/items/base_destroy_service_shared_examples.rb b/ee/spec/support/shared_examples/services/ai/catalog/items/base_destroy_service_shared_examples.rb index 2d2fec28517a3c..69bb5576e9c048 100644 --- a/ee/spec/support/shared_examples/services/ai/catalog/items/base_destroy_service_shared_examples.rb +++ b/ee/spec/support/shared_examples/services/ai/catalog/items/base_destroy_service_shared_examples.rb @@ -14,8 +14,6 @@ it 'returns insufficient permissions error' do result = execute_service - binding.pry - expect(result).to be_error expect(result.errors).to contain_exactly('You have insufficient permissions') end -- GitLab From 2696899a1333a7be7ce740adf8f6801be001485c Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 23 Oct 2025 17:47:24 +1300 Subject: [PATCH 4/5] Fix problems after rebasing --- config/authz/permissions/ai_catalog_item/delete.yml | 4 ++++ .../authz/permissions/hard_delete_ai_catalog_item/force.yml | 4 ++++ ee/app/policies/ai/catalog/item_policy.rb | 6 ++++-- 3 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 config/authz/permissions/ai_catalog_item/delete.yml create mode 100644 config/authz/permissions/hard_delete_ai_catalog_item/force.yml diff --git a/config/authz/permissions/ai_catalog_item/delete.yml b/config/authz/permissions/ai_catalog_item/delete.yml new file mode 100644 index 00000000000000..062b850ab23073 --- /dev/null +++ b/config/authz/permissions/ai_catalog_item/delete.yml @@ -0,0 +1,4 @@ +--- +name: delete_ai_catalog_item +description: Delete an AI Catalog item +feature_category: workflow_catalog diff --git a/config/authz/permissions/hard_delete_ai_catalog_item/force.yml b/config/authz/permissions/hard_delete_ai_catalog_item/force.yml new file mode 100644 index 00000000000000..f5e6cb6d999552 --- /dev/null +++ b/config/authz/permissions/hard_delete_ai_catalog_item/force.yml @@ -0,0 +1,4 @@ +--- +name: force_hard_delete_ai_catalog_item +description: Force hard delete an AI Catalog item +feature_category: workflow_catalog diff --git a/ee/app/policies/ai/catalog/item_policy.rb b/ee/app/policies/ai/catalog/item_policy.rb index e86871219bd1bd..efff7627f2b948 100644 --- a/ee/app/policies/ai/catalog/item_policy.rb +++ b/ee/app/policies/ai/catalog/item_policy.rb @@ -76,14 +76,16 @@ class ItemPolicy < ::BasePolicy prevent :delete_ai_catalog_item end - rule { flow & ~flows_enabled }.policy do + rule { flow & ~flows_enabled & ~admin }.policy do prevent :read_ai_catalog_item prevent :admin_ai_catalog_item + prevent :delete_ai_catalog_item end - rule { third_party_flow & ~third_party_flows_enabled }.policy do + rule { third_party_flow & ~third_party_flows_enabled & ~admin }.policy do prevent :read_ai_catalog_item prevent :admin_ai_catalog_item + prevent :delete_ai_catalog_item end end end -- GitLab From 00a66e47f376895b2944c9a84301e6cc0d02adc5 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Fri, 24 Oct 2025 12:13:16 +1300 Subject: [PATCH 5/5] Add reviewer feedback --- ee/app/graphql/mutations/ai/catalog/agent/delete.rb | 2 +- ee/app/graphql/mutations/ai/catalog/flow/delete.rb | 2 +- ee/app/graphql/mutations/ai/catalog/third_party_flow/delete.rb | 2 +- ee/spec/graphql/mutations/ai/catalog/agent/delete_spec.rb | 2 +- ee/spec/graphql/mutations/ai/catalog/flow/delete_spec.rb | 2 +- .../mutations/ai/catalog/third_party_flow/delete_spec.rb | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/ee/app/graphql/mutations/ai/catalog/agent/delete.rb b/ee/app/graphql/mutations/ai/catalog/agent/delete.rb index ec40660b59114a..d930731f6d30f5 100644 --- a/ee/app/graphql/mutations/ai/catalog/agent/delete.rb +++ b/ee/app/graphql/mutations/ai/catalog/agent/delete.rb @@ -20,7 +20,7 @@ class Delete < BaseMutation description: 'When true, the flow will always be hard deleted and never soft deleted. ' \ 'Can only be used by instance admins' - authorize :admin_ai_catalog_item + authorize :delete_ai_catalog_item def resolve(args) id = args.delete(:id) diff --git a/ee/app/graphql/mutations/ai/catalog/flow/delete.rb b/ee/app/graphql/mutations/ai/catalog/flow/delete.rb index 1aa2a4a00852c2..8c65c32dff635b 100644 --- a/ee/app/graphql/mutations/ai/catalog/flow/delete.rb +++ b/ee/app/graphql/mutations/ai/catalog/flow/delete.rb @@ -20,7 +20,7 @@ class Delete < BaseMutation description: 'When true, the flow will always be hard deleted and never soft deleted. ' \ 'Can only be used by instance admins' - authorize :admin_ai_catalog_item + authorize :delete_ai_catalog_item def resolve(args) id = args.delete(:id) diff --git a/ee/app/graphql/mutations/ai/catalog/third_party_flow/delete.rb b/ee/app/graphql/mutations/ai/catalog/third_party_flow/delete.rb index c1e883e1070f66..c97b8f4c67288a 100644 --- a/ee/app/graphql/mutations/ai/catalog/third_party_flow/delete.rb +++ b/ee/app/graphql/mutations/ai/catalog/third_party_flow/delete.rb @@ -20,7 +20,7 @@ class Delete < BaseMutation description: 'When true, the Third Party Flow will always be hard deleted and never soft deleted. ' \ 'Can only be used by instance admins' - authorize :admin_ai_catalog_item + authorize :delete_ai_catalog_item def resolve(args) id = args.delete(:id) diff --git a/ee/spec/graphql/mutations/ai/catalog/agent/delete_spec.rb b/ee/spec/graphql/mutations/ai/catalog/agent/delete_spec.rb index 8cd311dcd1a46a..87218fb491c33f 100644 --- a/ee/spec/graphql/mutations/ai/catalog/agent/delete_spec.rb +++ b/ee/spec/graphql/mutations/ai/catalog/agent/delete_spec.rb @@ -9,7 +9,7 @@ it { is_expected.to have_graphql_name('AiCatalogAgentDelete') } - it { expect(described_class).to require_graphql_authorizations(:admin_ai_catalog_item) } + it { expect(described_class).to require_graphql_authorizations(:delete_ai_catalog_item) } it { is_expected.to have_graphql_fields(:success, :errors, :client_mutation_id) } diff --git a/ee/spec/graphql/mutations/ai/catalog/flow/delete_spec.rb b/ee/spec/graphql/mutations/ai/catalog/flow/delete_spec.rb index cfbf1bd1f500ca..339c83be2f9e33 100644 --- a/ee/spec/graphql/mutations/ai/catalog/flow/delete_spec.rb +++ b/ee/spec/graphql/mutations/ai/catalog/flow/delete_spec.rb @@ -9,7 +9,7 @@ it { is_expected.to have_graphql_name('AiCatalogFlowDelete') } - it { expect(described_class).to require_graphql_authorizations(:admin_ai_catalog_item) } + it { expect(described_class).to require_graphql_authorizations(:delete_ai_catalog_item) } it { is_expected.to have_graphql_fields(:success, :errors, :client_mutation_id) } diff --git a/ee/spec/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb b/ee/spec/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb index f203641fa5721b..6a7c4760b66cfb 100644 --- a/ee/spec/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb +++ b/ee/spec/graphql/mutations/ai/catalog/third_party_flow/delete_spec.rb @@ -9,7 +9,7 @@ it { is_expected.to have_graphql_name('AiCatalogThirdPartyFlowDelete') } - it { expect(described_class).to require_graphql_authorizations(:admin_ai_catalog_item) } + it { expect(described_class).to require_graphql_authorizations(:delete_ai_catalog_item) } it { is_expected.to have_graphql_fields(:success, :errors, :client_mutation_id) } -- GitLab