From 5bcd7faae4a82428640ce561ab638d660f931118 Mon Sep 17 00:00:00 2001 From: "j.seto" Date: Fri, 17 May 2024 15:32:42 -0400 Subject: [PATCH 1/2] Update exclusions graphql endpoints to handle groups Contributes to: https://gitlab.com/gitlab-org/gitlab/-/issues/454372 --- .../integrations/exclusions/create.rb | 35 ++++++--- .../integrations/exclusions/delete.rb | 11 ++- .../types/integrations/exclusion_type.rb | 2 + doc/api/graphql/reference/index.md | 7 +- .../integrations/exclusions/create_spec.rb | 74 +++++++++++++++---- .../integrations/exclusions/delete_spec.rb | 31 ++++++++ 6 files changed, 134 insertions(+), 26 deletions(-) diff --git a/app/graphql/mutations/integrations/exclusions/create.rb b/app/graphql/mutations/integrations/exclusions/create.rb index 488fa149f74948..45aa627605bcd1 100644 --- a/app/graphql/mutations/integrations/exclusions/create.rb +++ b/app/graphql/mutations/integrations/exclusions/create.rb @@ -7,6 +7,7 @@ class Create < BaseMutation graphql_name 'IntegrationExclusionCreate' include ResolvesIds MAX_PROJECT_IDS = ::Integrations::Exclusions::CreateService::MAX_PROJECTS + MAX_GROUP_IDS = ::Integrations::Exclusions::CreateService::MAX_GROUPS field :exclusions, [::Types::Integrations::ExclusionType], null: true, @@ -19,23 +20,25 @@ class Create < BaseMutation argument :project_ids, [::Types::GlobalIDType[::Project]], - required: true, + required: false, + validates: { length: { maximum: MAX_PROJECT_IDS } }, description: "IDs of projects to exclude up to a maximum of #{MAX_PROJECT_IDS}." + argument :group_ids, + [::Types::GlobalIDType[::Group]], + required: false, + validates: { length: { maximum: MAX_GROUP_IDS } }, + description: "IDs of groups to exclude up to a maximum of #{MAX_GROUP_IDS}." + authorize :admin_all_resources - def resolve(integration_name:, project_ids:) + def resolve(integration_name:, project_ids: [], group_ids: []) authorize!(:global) - if project_ids.length > MAX_PROJECT_IDS - raise Gitlab::Graphql::Errors::ArgumentError, "Count of projectIds should be less than #{MAX_PROJECT_IDS}" - end - - projects = Project.id_in(resolve_ids(project_ids)).with_group.limit(MAX_PROJECT_IDS) - result = ::Integrations::Exclusions::CreateService.new( current_user: current_user, - projects: projects, + projects: projects(project_ids), + groups: groups(group_ids), integration_name: integration_name ).execute @@ -44,6 +47,20 @@ def resolve(integration_name:, project_ids:) errors: result.errors } end + + private + + def groups(group_ids) + return [] unless group_ids.present? + + Group.id_in(resolve_ids(group_ids)).limit(MAX_GROUP_IDS) + end + + def projects(project_ids) + return [] unless project_ids.present? + + Project.id_in(resolve_ids(project_ids)).with_group.limit(MAX_PROJECT_IDS) + end end end end diff --git a/app/graphql/mutations/integrations/exclusions/delete.rb b/app/graphql/mutations/integrations/exclusions/delete.rb index 13152746527498..8e843989e04a80 100644 --- a/app/graphql/mutations/integrations/exclusions/delete.rb +++ b/app/graphql/mutations/integrations/exclusions/delete.rb @@ -18,19 +18,26 @@ class Delete < BaseMutation argument :project_ids, [::Types::GlobalIDType[::Project]], - required: true, + required: false, description: 'IDs of excluded projects.' + argument :group_ids, + [::Types::GlobalIDType[::Group]], + required: false, + description: 'IDs of excluded groups.' + authorize :admin_all_resources - def resolve(integration_name:, project_ids:) + def resolve(integration_name:, project_ids: [], group_ids: []) authorize!(:global) projects = Project.id_in(resolve_ids(project_ids)) + groups = Group.id_in(resolve_ids(group_ids)) result = ::Integrations::Exclusions::DestroyService.new( current_user: current_user, projects: projects, + groups: groups, integration_name: integration_name ).execute diff --git a/app/graphql/types/integrations/exclusion_type.rb b/app/graphql/types/integrations/exclusion_type.rb index 0cd07f74238453..17c5b3a7405707 100644 --- a/app/graphql/types/integrations/exclusion_type.rb +++ b/app/graphql/types/integrations/exclusion_type.rb @@ -7,6 +7,8 @@ class ExclusionType < BaseObject description 'An integration to override the level settings of instance specific integrations.' authorize :admin_all_resources + field :group, ::Types::GroupType, + description: 'Group that has been excluded from the instance specific integration.' field :project, ::Types::ProjectType, description: 'Project that has been excluded from the instance specific integration.' end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 2f67a127b67b5e..bed361545e8965 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -5579,8 +5579,9 @@ Input type: `IntegrationExclusionCreateInput` | Name | Type | Description | | ---- | ---- | ----------- | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `groupIds` | [`[GroupID!]`](#groupid) | IDs of groups to exclude up to a maximum of 100. | | `integrationName` | [`IntegrationType!`](#integrationtype) | Type of integration to exclude. | -| `projectIds` | [`[ProjectID!]!`](#projectid) | IDs of projects to exclude up to a maximum of 100. | +| `projectIds` | [`[ProjectID!]`](#projectid) | IDs of projects to exclude up to a maximum of 100. | #### Fields @@ -5603,8 +5604,9 @@ Input type: `IntegrationExclusionDeleteInput` | Name | Type | Description | | ---- | ---- | ----------- | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `groupIds` | [`[GroupID!]`](#groupid) | IDs of excluded groups. | | `integrationName` | [`IntegrationType!`](#integrationtype) | Type of integration. | -| `projectIds` | [`[ProjectID!]!`](#projectid) | IDs of excluded projects. | +| `projectIds` | [`[ProjectID!]`](#projectid) | IDs of excluded projects. | #### Fields @@ -23585,6 +23587,7 @@ An integration to override the level settings of instance specific integrations. | Name | Type | Description | | ---- | ---- | ----------- | +| `group` | [`Group`](#group) | Group that has been excluded from the instance specific integration. | | `project` | [`Project`](#project) | Project that has been excluded from the instance specific integration. | ### `IssuableResourceLink` diff --git a/spec/requests/api/graphql/mutations/integrations/exclusions/create_spec.rb b/spec/requests/api/graphql/mutations/integrations/exclusions/create_spec.rb index 153a5ef51e5d13..bd6842b0a49fb0 100644 --- a/spec/requests/api/graphql/mutations/integrations/exclusions/create_spec.rb +++ b/spec/requests/api/graphql/mutations/integrations/exclusions/create_spec.rb @@ -6,19 +6,38 @@ include GraphqlHelpers let_it_be(:project) { create(:project) } - let_it_be(:project2) { create(:project) } + let_it_be(:project2) { create(:project, :in_subgroup) } let_it_be(:admin_user) { create(:admin) } let_it_be(:user) { create(:user) } let(:current_user) { admin_user } - let(:mutation) { graphql_mutation(:integration_exclusion_create, args) } + let(:mutation) { graphql_mutation(:integration_exclusion_create, args, fields) } let(:args) do { 'integrationName' => 'BEYOND_IDENTITY', - 'projectIds' => project_ids + 'projectIds' => project_ids, + 'groupIds' => group_ids } end + let(:fields) do + <<~FIELDS + exclusions{ + project{ + id + name + avatarUrl + } + group{ + id + name + avatarUrl + } + } + FIELDS + end + let(:project_ids) { [project.to_global_id.to_s] } + let(:group_ids) { [] } subject(:resolve_mutation) { post_graphql_mutation(mutation, current_user: current_user) } @@ -55,21 +74,50 @@ expect(existing_exclusion).not_to be_active expect(existing_exclusion.inherit_from_id).to be_nil end + + context 'when creating exclusions for groups' do + let(:args) do + { + 'integrationName' => 'BEYOND_IDENTITY', + 'projectIds' => project_ids, + 'groupIds' => [project2.group.to_global_id.to_s] + } + end + + it 'updates existing integrations and creates integrations for projects', :sidekiq_inline do + expect { resolve_mutation }.to change { Integration.count }.from(2).to(4) + expect(graphql_data['integrationExclusionCreate']['exclusions'].length).to eq(2) + expect(graphql_data['integrationExclusionCreate']['exclusions']).to include(a_hash_including({ + 'project' => nil, 'group' => a_hash_including({ 'id' => project2.group.to_global_id.to_s }) + })) + expect(graphql_data['integrationExclusionCreate']['exclusions']).to include(a_hash_including({ + 'project' => a_hash_including({ 'id' => project.to_global_id.to_s }), 'group' => nil + })) + expect(existing_exclusion).not_to be_active + expect(existing_exclusion.inherit_from_id).to be_present + end + end end - context 'and the number of project ids exceeds the maximum' do - let(:stubbed_limit) { 1 } - let(:project_ids) { [project, project2].map { |p| p.to_global_id.to_s } } + describe 'validations' do + context 'when there are too many project ids in the request' do + let(:project_ids) { (1..101).map { |id| "gid://gitlab/Project/#{id}" } } - before do - stub_const("#{described_class.name}::MAX_PROJECT_IDS", stubbed_limit) + it 'responds with an error without changing exclusions' do + expect(Integrations::Exclusions::CreateService).not_to receive(:new) + resolve_mutation + expect(graphql_errors).to include(a_hash_including('message' => "projectIds is too long (maximum is 100)")) + end end - it 'responds with an error' do - resolve_mutation - expect(graphql_errors.first['message']).to eq( - "Count of projectIds should be less than #{stubbed_limit}" - ) + context 'when there are too many group ids in the request' do + let(:group_ids) { (1..101).map { |id| "gid://gitlab/Group/#{id}" } } + + it 'responds with an error without changing exclusions' do + expect(Integrations::Exclusions::CreateService).not_to receive(:new) + resolve_mutation + expect(graphql_errors).to include(a_hash_including('message' => "groupIds is too long (maximum is 100)")) + end end end end diff --git a/spec/requests/api/graphql/mutations/integrations/exclusions/delete_spec.rb b/spec/requests/api/graphql/mutations/integrations/exclusions/delete_spec.rb index 2b1e41ff56b499..f84daa3ac2530c 100644 --- a/spec/requests/api/graphql/mutations/integrations/exclusions/delete_spec.rb +++ b/spec/requests/api/graphql/mutations/integrations/exclusions/delete_spec.rb @@ -6,6 +6,7 @@ include GraphqlHelpers let_it_be(:project) { create(:project) } + let_it_be(:project2) { create(:project, :in_subgroup) } let_it_be(:admin_user) { create(:admin) } let_it_be(:user) { create(:user) } let(:current_user) { admin_user } @@ -49,6 +50,36 @@ context 'and the integration is active for the instance' do let!(:instance_integration) { create(:beyond_identity_integration) } + context 'and there is a group exclusion', :sidekiq_inline do + let!(:group_exclusion) do + create(:beyond_identity_integration, group: project2.root_namespace, active: false, inherit_from_id: nil, + instance: false) + end + + let(:args) do + { + 'integrationName' => 'BEYOND_IDENTITY', + 'projectIds' => project_ids, + 'groupIds' => [project2.root_namespace.to_global_id.to_s] + } + end + + it 'enables the integration for the specified project' do + resolve_mutation + + existing_exclusion.reload + expect(existing_exclusion).to be_activated + expect(existing_exclusion.inherit_from_id).to eq(instance_integration.id) + expect(project2.beyond_identity_integration).to be_activated + expect(project2.beyond_identity_integration.inherit_from_id).to eq(instance_integration.id) + exclusion_response = graphql_data['integrationExclusionDelete']['exclusions'] + expect(exclusion_response).to include(a_hash_including({ 'group' => nil, +'project' => a_hash_including({ 'id' => project.to_global_id.to_s }) })) + expect(exclusion_response).to include(a_hash_including({ 'project' => nil, +'group' => a_hash_including({ 'id' => project2.root_namespace.to_global_id.to_s }) })) + end + end + it 'enables the integration for the specified project' do resolve_mutation -- GitLab From 9e3305b1a19e830c83901cb9047a5158cc9aed65 Mon Sep 17 00:00:00 2001 From: "j.seto" Date: Thu, 27 Jun 2024 20:13:36 -0400 Subject: [PATCH 2/2] Refactor to avoid calling resolve_ids without ids --- .../integrations/exclusions/delete.rb | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/app/graphql/mutations/integrations/exclusions/delete.rb b/app/graphql/mutations/integrations/exclusions/delete.rb index 8e843989e04a80..0a8591e9270bc9 100644 --- a/app/graphql/mutations/integrations/exclusions/delete.rb +++ b/app/graphql/mutations/integrations/exclusions/delete.rb @@ -31,13 +31,10 @@ class Delete < BaseMutation def resolve(integration_name:, project_ids: [], group_ids: []) authorize!(:global) - projects = Project.id_in(resolve_ids(project_ids)) - groups = Group.id_in(resolve_ids(group_ids)) - result = ::Integrations::Exclusions::DestroyService.new( current_user: current_user, - projects: projects, - groups: groups, + projects: projects(project_ids), + groups: groups(group_ids), integration_name: integration_name ).execute @@ -51,6 +48,20 @@ def resolve(integration_name:, project_ids: [], group_ids: []) errors: result.errors } end + + private + + def projects(project_ids) + return [] unless project_ids.present? + + Project.id_in(resolve_ids(project_ids)) + end + + def groups(group_ids) + return [] unless group_ids.present? + + Group.id_in(resolve_ids(group_ids)) + end end end end -- GitLab