From f7c9cf3b58b8812d288d8046e72e102d9c420b60 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Thu, 9 Jan 2025 09:49:30 -0600 Subject: [PATCH 1/7] Adding GraphQL mutations to trigger Allowlist Autopopulation processes GraphQL mutations have been added that can trigger Autopopulation and Clear Autopopulated allowlist entries for CI Job Tokens Changelog: added MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/177465 --- .../job_token_scope/autopopulate_allowlist.rb | 39 +++++++++++++++ .../clear_allowlist_autopopulations.rb | 36 ++++++++++++++ app/graphql/types/mutation_type.rb | 2 + doc/api/graphql/reference/index.md | 48 +++++++++++++++++++ 4 files changed, 125 insertions(+) create mode 100644 app/graphql/mutations/ci/job_token_scope/autopopulate_allowlist.rb create mode 100644 app/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations.rb diff --git a/app/graphql/mutations/ci/job_token_scope/autopopulate_allowlist.rb b/app/graphql/mutations/ci/job_token_scope/autopopulate_allowlist.rb new file mode 100644 index 00000000000000..d3674dfc421730 --- /dev/null +++ b/app/graphql/mutations/ci/job_token_scope/autopopulate_allowlist.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module Mutations + module Ci + module JobTokenScope + class AutopopulateAllowlist < BaseMutation + graphql_name 'CiJobTokenScopeAutopopulateAllowlist' + + include FindsProject + + authorize :admin_project + + argument :project_path, GraphQL::Types::ID, + required: true, + description: 'Project in which to autopopulate the allowlist.' + + field :status, + GraphQL::Types::String, + null: false, + description: "Status of the autopopulation process." + + def resolve(project_path:) + authorized_find!(project_path) + + # ApplicationRecord.transaction do + # Ci::JobToken::ClearAutopopulatedAllowlistService.new(project.id).execute + # Ci::JobToken::AutopopulateAllowlistService.new(project.id).execute + # end + + { status: "complete", errors: [] } + + rescue StandardError => error + + { status: "error", errors: [error] } + end + end + end + end +end diff --git a/app/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations.rb b/app/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations.rb new file mode 100644 index 00000000000000..fac3115dc494d5 --- /dev/null +++ b/app/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module Mutations + module Ci + module JobTokenScope + class ClearAllowlistAutopopulations < BaseMutation + graphql_name 'CiJobTokenScopeClearAllowlistAutopopulations' + + include FindsProject + + authorize :admin_project + + argument :project_path, GraphQL::Types::ID, + required: true, + description: 'Project in which to autopopulate the allowlist.' + + field :status, + GraphQL::Types::String, + null: false, + description: "Status of the autopopulation process." + + def resolve(project_path:) + authorized_find!(project_path) + + # Ci::JobToken::AutopopulateAllowlistService.new(project.id).execute + + { status: "complete", errors: [] } + + rescue StandardError => error + + { status: "error", errors: [error] } + end + end + end + end +end diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 7549ca5e147a4e..e67ed8727235a6 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -180,6 +180,8 @@ class MutationType < BaseObject mount_mutation Mutations::Ci::JobTokenScope::RemoveGroup mount_mutation Mutations::Ci::JobTokenScope::RemoveProject mount_mutation Mutations::Ci::JobTokenScope::UpdateJobTokenPolicies, experiment: { milestone: '17.6' } + mount_mutation Mutations::Ci::JobTokenScope::AutopopulateAllowlist, experiment: { milestone: '17.8' } + mount_mutation Mutations::Ci::JobTokenScope::ClearAllowlistAutopopulations, experiment: { milestone: '17.8' } mount_mutation Mutations::Ci::Pipeline::Cancel mount_mutation Mutations::Ci::Pipeline::Create mount_mutation Mutations::Ci::Pipeline::Destroy diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index a3db407a3f814e..0522e9295643fe 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -3212,6 +3212,52 @@ Input type: `CiJobTokenScopeAddProjectInput` | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +### `Mutation.ciJobTokenScopeAutopopulateAllowlist` + +DETAILS: +**Introduced** in GitLab 17.8. +**Status**: Experiment. + +Input type: `CiJobTokenScopeAutopopulateAllowlistInput` + +#### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `projectPath` | [`ID!`](#id) | Project in which to autopopulate the allowlist. | + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +| `status` | [`String!`](#string) | Status of the autopopulation process. | + +### `Mutation.ciJobTokenScopeClearAllowlistAutopopulations` + +DETAILS: +**Introduced** in GitLab 17.8. +**Status**: Experiment. + +Input type: `CiJobTokenScopeClearAllowlistAutopopulationsInput` + +#### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `projectPath` | [`ID!`](#id) | Project in which to autopopulate the allowlist. | + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +| `status` | [`String!`](#string) | Status of the autopopulation process. | + ### `Mutation.ciJobTokenScopeRemoveGroup` Input type: `CiJobTokenScopeRemoveGroupInput` @@ -41436,6 +41482,8 @@ State of a Sentry error. | `GITHUB_SERVICE` | GithubService type. | | `GITLAB_SLACK_APPLICATION_SERVICE` | GitlabSlackApplicationService type. | | `GIT_GUARDIAN_SERVICE` | GitGuardianService type. | +| `GOOGLE_CLOUD_PLATFORM_ARTIFACT_REGISTRY_SERVICE` | GoogleCloudPlatformArtifactRegistryService type. | +| `GOOGLE_CLOUD_PLATFORM_WORKLOAD_IDENTITY_FEDERATION_SERVICE` | GoogleCloudPlatformWorkloadIdentityFederationService type. | | `GOOGLE_PLAY_SERVICE` | GooglePlayService type. | | `HANGOUTS_CHAT_SERVICE` | HangoutsChatService type. | | `HARBOR_SERVICE` | HarborService type. | -- GitLab From 408d347b5115261f452a180e844203e3dfa633a4 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Wed, 15 Jan 2025 12:34:26 -0600 Subject: [PATCH 2/7] Connecting GraphQL mutation to service classes --- .../job_token_scope/autopopulate_allowlist.rb | 18 +++-- .../clear_allowlist_autopopulations.rb | 4 +- app/graphql/types/mutation_type.rb | 4 +- .../autopopulate_allowlist_service.rb | 4 ++ doc/api/graphql/reference/index.md | 6 +- .../autopopulate_allowlist_spec.rb | 67 +++++++++++++++++++ .../autopopulate_allowlist_service_spec.rb | 37 +++++++--- 7 files changed, 111 insertions(+), 29 deletions(-) create mode 100644 spec/graphql/mutations/ci/job_token_scope/autopopulate_allowlist_spec.rb diff --git a/app/graphql/mutations/ci/job_token_scope/autopopulate_allowlist.rb b/app/graphql/mutations/ci/job_token_scope/autopopulate_allowlist.rb index d3674dfc421730..bed4ea12db2bc2 100644 --- a/app/graphql/mutations/ci/job_token_scope/autopopulate_allowlist.rb +++ b/app/graphql/mutations/ci/job_token_scope/autopopulate_allowlist.rb @@ -20,18 +20,16 @@ class AutopopulateAllowlist < BaseMutation description: "Status of the autopopulation process." def resolve(project_path:) - authorized_find!(project_path) + project = authorized_find!(project_path) - # ApplicationRecord.transaction do - # Ci::JobToken::ClearAutopopulatedAllowlistService.new(project.id).execute - # Ci::JobToken::AutopopulateAllowlistService.new(project.id).execute - # end + ::Ci::JobToken::ClearAutopopulatedAllowlistService.new(project, current_user).execute + result = ::Ci::JobToken::AutopopulateAllowlistService.new(project, current_user).execute - { status: "complete", errors: [] } - - rescue StandardError => error - - { status: "error", errors: [error] } + if result.success? + { status: "complete", errors: [] } + else + { status: "error", errors: [result.message] } + end end end end diff --git a/app/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations.rb b/app/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations.rb index fac3115dc494d5..976ef92779ff2e 100644 --- a/app/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations.rb +++ b/app/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations.rb @@ -20,9 +20,9 @@ class ClearAllowlistAutopopulations < BaseMutation description: "Status of the autopopulation process." def resolve(project_path:) - authorized_find!(project_path) + project = authorized_find!(project_path) - # Ci::JobToken::AutopopulateAllowlistService.new(project.id).execute + ::Ci::JobToken::AutopopulateAllowlistService.new(project, current_user).execute { status: "complete", errors: [] } diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index e67ed8727235a6..81cc7dd220257b 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -180,8 +180,8 @@ class MutationType < BaseObject mount_mutation Mutations::Ci::JobTokenScope::RemoveGroup mount_mutation Mutations::Ci::JobTokenScope::RemoveProject mount_mutation Mutations::Ci::JobTokenScope::UpdateJobTokenPolicies, experiment: { milestone: '17.6' } - mount_mutation Mutations::Ci::JobTokenScope::AutopopulateAllowlist, experiment: { milestone: '17.8' } - mount_mutation Mutations::Ci::JobTokenScope::ClearAllowlistAutopopulations, experiment: { milestone: '17.8' } + mount_mutation Mutations::Ci::JobTokenScope::AutopopulateAllowlist, experiment: { milestone: '17.9' } + mount_mutation Mutations::Ci::JobTokenScope::ClearAllowlistAutopopulations, experiment: { milestone: '17.9' } mount_mutation Mutations::Ci::Pipeline::Cancel mount_mutation Mutations::Ci::Pipeline::Create mount_mutation Mutations::Ci::Pipeline::Destroy diff --git a/app/services/ci/job_token/autopopulate_allowlist_service.rb b/app/services/ci/job_token/autopopulate_allowlist_service.rb index f11e6d4921b5eb..f8b6e6b9958ed2 100644 --- a/app/services/ci/job_token/autopopulate_allowlist_service.rb +++ b/app/services/ci/job_token/autopopulate_allowlist_service.rb @@ -24,6 +24,10 @@ def execute allowlist.bulk_add_groups!(groups, user: @user, autopopulated: true) allowlist.bulk_add_projects!(projects, user: @user, autopopulated: true) end + + ServiceResponse.success + rescue StandardError => e + ServiceResponse.error(message: e.message) end private diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 0522e9295643fe..47cbb3dc6333c5 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -3215,7 +3215,7 @@ Input type: `CiJobTokenScopeAddProjectInput` ### `Mutation.ciJobTokenScopeAutopopulateAllowlist` DETAILS: -**Introduced** in GitLab 17.8. +**Introduced** in GitLab 17.9. **Status**: Experiment. Input type: `CiJobTokenScopeAutopopulateAllowlistInput` @@ -3238,7 +3238,7 @@ Input type: `CiJobTokenScopeAutopopulateAllowlistInput` ### `Mutation.ciJobTokenScopeClearAllowlistAutopopulations` DETAILS: -**Introduced** in GitLab 17.8. +**Introduced** in GitLab 17.9. **Status**: Experiment. Input type: `CiJobTokenScopeClearAllowlistAutopopulationsInput` @@ -41482,8 +41482,6 @@ State of a Sentry error. | `GITHUB_SERVICE` | GithubService type. | | `GITLAB_SLACK_APPLICATION_SERVICE` | GitlabSlackApplicationService type. | | `GIT_GUARDIAN_SERVICE` | GitGuardianService type. | -| `GOOGLE_CLOUD_PLATFORM_ARTIFACT_REGISTRY_SERVICE` | GoogleCloudPlatformArtifactRegistryService type. | -| `GOOGLE_CLOUD_PLATFORM_WORKLOAD_IDENTITY_FEDERATION_SERVICE` | GoogleCloudPlatformWorkloadIdentityFederationService type. | | `GOOGLE_PLAY_SERVICE` | GooglePlayService type. | | `HANGOUTS_CHAT_SERVICE` | HangoutsChatService type. | | `HARBOR_SERVICE` | HarborService type. | diff --git a/spec/graphql/mutations/ci/job_token_scope/autopopulate_allowlist_spec.rb b/spec/graphql/mutations/ci/job_token_scope/autopopulate_allowlist_spec.rb new file mode 100644 index 00000000000000..b7fffc593b5e51 --- /dev/null +++ b/spec/graphql/mutations/ci/job_token_scope/autopopulate_allowlist_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Mutations::Ci::JobTokenScope::AutopopulateAllowlist, feature_category: :continuous_integration do + include GraphqlHelpers + + let(:mutation) do + described_class.new(object: nil, context: query_context, field: nil) + end + + describe '#resolve' do + let_it_be(:project) { create(:project) } + let_it_be(:origin_project) { create(:project) } + let(:mutation_args) { { project_path: project.full_path } } + + subject(:resolver) do + mutation.resolve(**mutation_args) + end + + context 'when user is not logged in' do + let(:current_user) { nil } + + it 'raises error' do + expect { resolver }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end + end + + context 'when user is logged in' do + let_it_be(:current_user) { create(:user) } + + context 'when user does not have permissions to admin the project' do + it 'raises error' do + expect { resolver }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end + end + + context 'when user has permissions to admin the project' do + before_all do + project.add_maintainer(current_user) + + create(:ci_job_token_authorization, origin_project: origin_project, accessed_project: project, + last_authorized_at: 1.day.ago) + end + + it 'adds target project to the inbound job token scope by default' do + expect do + expect(resolver).to include(errors: be_empty) + end.to change { Ci::JobToken::ProjectScopeLink.count }.by(1) + end + + context 'when the service returns an error' do + let(:service) { instance_double(::Ci::JobToken::AutopopulateAllowlistService) } + + it 'returns an error response' do + expect(::Ci::JobToken::AutopopulateAllowlistService).to receive(:new).with(project, + current_user).and_return(service) + expect(service).to receive(:execute) + .and_return(ServiceResponse.error(message: 'The error message')) + + expect(resolver.fetch(:errors)).to include("The error message") + end + end + end + end + end +end diff --git a/spec/services/ci/job_token/autopopulate_allowlist_service_spec.rb b/spec/services/ci/job_token/autopopulate_allowlist_service_spec.rb index e449711fa01438..759d62b5ed69d8 100644 --- a/spec/services/ci/job_token/autopopulate_allowlist_service_spec.rb +++ b/spec/services/ci/job_token/autopopulate_allowlist_service_spec.rb @@ -57,12 +57,17 @@ let(:service) { described_class.new(accessed_project, developer) } it 'raises an access denied error' do - expect { service.execute }.to raise_error(Gitlab::Access::AccessDeniedError) + result = service.execute + + expect(result).to be_error + expect(result.message).to eq('Gitlab::Access::AccessDeniedError') end end it 'creates the expected group and project links for the given limit' do - service.execute + result = service.execute + + expect(result).to be_success expect(Ci::JobToken::GroupScopeLink.autopopulated.pluck(:target_group_id)).to match_array([ns2.id, ns4.id]) expect(Ci::JobToken::ProjectScopeLink.autopopulated.pluck(:target_project_id)).to match_array([pns1.project.id, @@ -73,8 +78,9 @@ let(:compaction_limit) { 3 } it 'creates the expected group and project links' do - service.execute + result = service.execute + expect(result).to be_success expect(Ci::JobToken::GroupScopeLink.autopopulated.pluck(:target_group_id)).to match_array([ns1.id]) expect(Ci::JobToken::ProjectScopeLink.autopopulated.pluck(:target_project_id)).to match_array([pns8.project.id]) end @@ -84,9 +90,10 @@ let(:compaction_limit) { 1 } it 'logs a CompactionLimitCannotBeAchievedError error' do - expect do - service.execute - end.to raise_error(Gitlab::Utils::TraversalIdCompactor::CompactionLimitCannotBeAchievedError) + result = service.execute + + expect(result).to be_error + expect(result.message).to eq('Gitlab::Utils::TraversalIdCompactor::CompactionLimitCannotBeAchievedError') expect(Ci::JobToken::GroupScopeLink.count).to be(0) expect(Ci::JobToken::ProjectScopeLink.count).to be(0) @@ -102,7 +109,10 @@ original_response << [1, 2, 3] end - expect { service.execute }.to raise_error(Gitlab::Utils::TraversalIdCompactor::UnexpectedCompactionEntry) + result = service.execute + + expect(result).to be_error + expect(result.message).to eq('Gitlab::Utils::TraversalIdCompactor::UnexpectedCompactionEntry') end it 'logs a RedundantCompactionEntry error' do @@ -111,7 +121,10 @@ original_response << original_response.last.first(2) end - expect { service.execute }.to raise_error(Gitlab::Utils::TraversalIdCompactor::RedundantCompactionEntry) + result = service.execute + + expect(result).to be_error + expect(result.message).to eq('Gitlab::Utils::TraversalIdCompactor::RedundantCompactionEntry') end end @@ -137,9 +150,11 @@ let(:compaction_limit) { 2 } it 'raises when the limit cannot be achieved' do - expect do - service.execute - end.to raise_error(Gitlab::Utils::TraversalIdCompactor::CompactionLimitCannotBeAchievedError) + result = service.execute + + expect(result).to be_error + expect(result.message).to eq('Gitlab::Utils::TraversalIdCompactor::CompactionLimitCannotBeAchievedError') + expect(Ci::JobToken::GroupScopeLink.count).to be(0) expect(Ci::JobToken::ProjectScopeLink.count).to be(0) end -- GitLab From f377fefda3fa2740038bbf40388eca8f6554b463 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Wed, 15 Jan 2025 15:38:12 -0600 Subject: [PATCH 3/7] Updated interface for clear autopopulations API --- .../clear_allowlist_autopopulations.rb | 12 +-- .../clear_autopopulated_allowlist_service.rb | 4 + .../clear_allowlist_autopopulations_spec.rb | 87 +++++++++++++++++++ 3 files changed, 97 insertions(+), 6 deletions(-) create mode 100644 spec/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations_spec.rb diff --git a/app/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations.rb b/app/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations.rb index 976ef92779ff2e..d083c20172b16c 100644 --- a/app/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations.rb +++ b/app/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations.rb @@ -22,13 +22,13 @@ class ClearAllowlistAutopopulations < BaseMutation def resolve(project_path:) project = authorized_find!(project_path) - ::Ci::JobToken::AutopopulateAllowlistService.new(project, current_user).execute + result = ::Ci::JobToken::ClearAutopopulatedAllowlistService.new(project, current_user).execute - { status: "complete", errors: [] } - - rescue StandardError => error - - { status: "error", errors: [error] } + if result.success? + { status: "complete", errors: [] } + else + { status: "error", errors: [result.message] } + end end end end diff --git a/app/services/ci/job_token/clear_autopopulated_allowlist_service.rb b/app/services/ci/job_token/clear_autopopulated_allowlist_service.rb index 24e1761bc28573..21cdd2ee93c0b3 100644 --- a/app/services/ci/job_token/clear_autopopulated_allowlist_service.rb +++ b/app/services/ci/job_token/clear_autopopulated_allowlist_service.rb @@ -17,6 +17,10 @@ def execute allowlist.project_links.autopopulated.delete_all allowlist.group_links.autopopulated.delete_all end + + ServiceResponse.success + rescue StandardError => e + ServiceResponse.error(message: e.message) end private diff --git a/spec/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations_spec.rb b/spec/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations_spec.rb new file mode 100644 index 00000000000000..670d6e785388ac --- /dev/null +++ b/spec/graphql/mutations/ci/job_token_scope/clear_allowlist_autopopulations_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Mutations::Ci::JobTokenScope::ClearAllowlistAutopopulations, feature_category: :continuous_integration do + include GraphqlHelpers + + let(:mutation) do + described_class.new(object: nil, context: query_context, field: nil) + end + + describe '#resolve' do + let_it_be(:project) { create(:project) } + let(:mutation_args) { { project_path: project.full_path } } + + subject(:resolver) do + mutation.resolve(**mutation_args) + end + + context 'when user is not logged in' do + let(:current_user) { nil } + + it 'raises error' do + expect { resolver }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end + end + + context 'when user is logged in' do + let_it_be(:current_user) { create(:user) } + + context 'when user does not have permissions to admin the project' do + it 'raises error' do + expect { resolver }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end + end + + context 'when user has permissions to admin the project' do + before_all do + project.add_maintainer(current_user) + end + + context 'when group scope links have been autopopulated' do + before do + create(:ci_job_token_group_scope_link, source_project: project, autopopulated: true) + create(:ci_job_token_group_scope_link, source_project: project, autopopulated: false) + end + + it 'removes the autopopulated group links' do + expect do + expect(resolver).to include(errors: be_empty) + end.to change { Ci::JobToken::GroupScopeLink.autopopulated.count }.by(-1) + .and not_change { Ci::JobToken::ProjectScopeLink.autopopulated.count } + end + end + + context 'when project scope links have been autopopulated' do + before do + create(:ci_job_token_project_scope_link, source_project: project, direction: :inbound, + autopopulated: true) + create(:ci_job_token_project_scope_link, source_project: project, direction: :inbound, + autopopulated: false) + end + + it 'removes the autopopulated project links' do + expect do + expect(resolver).to include(errors: be_empty) + end.to change { Ci::JobToken::ProjectScopeLink.autopopulated.count }.by(-1) + .and not_change { Ci::JobToken::GroupScopeLink.autopopulated.count } + end + end + + context 'when the service returns an error' do + let(:service) { instance_double(::Ci::JobToken::ClearAutopopulatedAllowlistService) } + + it 'returns an error response' do + expect(::Ci::JobToken::ClearAutopopulatedAllowlistService).to receive(:new).with(project, + current_user).and_return(service) + expect(service).to receive(:execute) + .and_return(ServiceResponse.error(message: 'The error message')) + + expect(resolver.fetch(:errors)).to include("The error message") + end + end + end + end + end +end -- GitLab From ee828885fed6775148daa2a7da17687e453fd4b5 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Wed, 15 Jan 2025 16:11:35 -0600 Subject: [PATCH 4/7] Spec and testing fixes --- app/models/ci/job_token/allowlist.rb | 1 + .../job_token/clear_autopopulated_allowlist_service_spec.rb | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/ci/job_token/allowlist.rb b/app/models/ci/job_token/allowlist.rb index 80a640b7919647..15498a3ac70d98 100644 --- a/app/models/ci/job_token/allowlist.rb +++ b/app/models/ci/job_token/allowlist.rb @@ -81,6 +81,7 @@ def bulk_add_projects!(target_projects, user:, autopopulated: false, policies: [ autopopulated: autopopulated, added_by: user, job_token_policies: job_token_policies, + direction: @direction, created_at: now ) end diff --git a/spec/services/ci/job_token/clear_autopopulated_allowlist_service_spec.rb b/spec/services/ci/job_token/clear_autopopulated_allowlist_service_spec.rb index 245ac053c890be..547b723350b284 100644 --- a/spec/services/ci/job_token/clear_autopopulated_allowlist_service_spec.rb +++ b/spec/services/ci/job_token/clear_autopopulated_allowlist_service_spec.rb @@ -19,7 +19,10 @@ let(:service) { described_class.new(accessed_project, developer) } it 'raises an access denied error' do - expect { service.execute }.to raise_error(Gitlab::Access::AccessDeniedError) + result = service.execute + + expect(result).to be_error + expect(result.message).to eq('Gitlab::Access::AccessDeniedError') end end -- GitLab From a0bfaa0f3e5a94fee0861dc3f106823fd0e03a3b Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Wed, 22 Jan 2025 15:39:52 -0600 Subject: [PATCH 5/7] Adding eror handling for Clear service --- .../job_token_scope/autopopulate_allowlist.rb | 4 ++-- .../autopopulate_allowlist_spec.rb | 19 ++++++++++++++++--- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/app/graphql/mutations/ci/job_token_scope/autopopulate_allowlist.rb b/app/graphql/mutations/ci/job_token_scope/autopopulate_allowlist.rb index bed4ea12db2bc2..202d153e9d018a 100644 --- a/app/graphql/mutations/ci/job_token_scope/autopopulate_allowlist.rb +++ b/app/graphql/mutations/ci/job_token_scope/autopopulate_allowlist.rb @@ -22,8 +22,8 @@ class AutopopulateAllowlist < BaseMutation def resolve(project_path:) project = authorized_find!(project_path) - ::Ci::JobToken::ClearAutopopulatedAllowlistService.new(project, current_user).execute - result = ::Ci::JobToken::AutopopulateAllowlistService.new(project, current_user).execute + result = ::Ci::JobToken::ClearAutopopulatedAllowlistService.new(project, current_user).execute + result = ::Ci::JobToken::AutopopulateAllowlistService.new(project, current_user).execute if result.success? if result.success? { status: "complete", errors: [] } diff --git a/spec/graphql/mutations/ci/job_token_scope/autopopulate_allowlist_spec.rb b/spec/graphql/mutations/ci/job_token_scope/autopopulate_allowlist_spec.rb index b7fffc593b5e51..424d5c8b3fdba3 100644 --- a/spec/graphql/mutations/ci/job_token_scope/autopopulate_allowlist_spec.rb +++ b/spec/graphql/mutations/ci/job_token_scope/autopopulate_allowlist_spec.rb @@ -49,16 +49,29 @@ end.to change { Ci::JobToken::ProjectScopeLink.count }.by(1) end - context 'when the service returns an error' do + context 'when the clear service returns an error' do + let(:service) { instance_double(::Ci::JobToken::ClearAutopopulatedAllowlistService) } + + it 'returns an error response' do + expect(::Ci::JobToken::ClearAutopopulatedAllowlistService).to receive(:new).with(project, + current_user).and_return(service) + expect(service).to receive(:execute) + .and_return(ServiceResponse.error(message: 'Clear service error message')) + + expect(resolver.fetch(:errors)).to include("Clear service error message") + end + end + + context 'when the autopopulate service returns an error' do let(:service) { instance_double(::Ci::JobToken::AutopopulateAllowlistService) } it 'returns an error response' do expect(::Ci::JobToken::AutopopulateAllowlistService).to receive(:new).with(project, current_user).and_return(service) expect(service).to receive(:execute) - .and_return(ServiceResponse.error(message: 'The error message')) + .and_return(ServiceResponse.error(message: 'Autopopulates service error message')) - expect(resolver.fetch(:errors)).to include("The error message") + expect(resolver.fetch(:errors)).to include("Autopopulates service error message") end end end -- GitLab From 8a33eb638fe69ad02d7c5e6fc1f7c7598e560240 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Thu, 23 Jan 2025 17:27:22 -0600 Subject: [PATCH 6/7] Adding error tracking to autopopulate and clear services --- app/services/ci/job_token/autopopulate_allowlist_service.rb | 1 + .../ci/job_token/clear_autopopulated_allowlist_service.rb | 1 + .../ci/job_token/autopopulate_allowlist_service_spec.rb | 4 ++++ .../job_token/clear_autopopulated_allowlist_service_spec.rb | 3 +++ 4 files changed, 9 insertions(+) diff --git a/app/services/ci/job_token/autopopulate_allowlist_service.rb b/app/services/ci/job_token/autopopulate_allowlist_service.rb index f8b6e6b9958ed2..8d8033a39951a0 100644 --- a/app/services/ci/job_token/autopopulate_allowlist_service.rb +++ b/app/services/ci/job_token/autopopulate_allowlist_service.rb @@ -27,6 +27,7 @@ def execute ServiceResponse.success rescue StandardError => e + Gitlab::ErrorTracking.log_exception(e, { project_id: @project.id, user_id: @user.id }) ServiceResponse.error(message: e.message) end diff --git a/app/services/ci/job_token/clear_autopopulated_allowlist_service.rb b/app/services/ci/job_token/clear_autopopulated_allowlist_service.rb index 21cdd2ee93c0b3..3001765de4461f 100644 --- a/app/services/ci/job_token/clear_autopopulated_allowlist_service.rb +++ b/app/services/ci/job_token/clear_autopopulated_allowlist_service.rb @@ -20,6 +20,7 @@ def execute ServiceResponse.success rescue StandardError => e + Gitlab::ErrorTracking.log_exception(e, { project_id: @project.id, user_id: @user.id }) ServiceResponse.error(message: e.message) end diff --git a/spec/services/ci/job_token/autopopulate_allowlist_service_spec.rb b/spec/services/ci/job_token/autopopulate_allowlist_service_spec.rb index 759d62b5ed69d8..937dbb430b6dcd 100644 --- a/spec/services/ci/job_token/autopopulate_allowlist_service_spec.rb +++ b/spec/services/ci/job_token/autopopulate_allowlist_service_spec.rb @@ -150,6 +150,10 @@ let(:compaction_limit) { 2 } it 'raises when the limit cannot be achieved' do + expect(Gitlab::ErrorTracking).to receive(:log_exception).with( + kind_of(Gitlab::Utils::TraversalIdCompactor::CompactionLimitCannotBeAchievedError), + { project_id: accessed_project.id, user_id: maintainer.id } + ) result = service.execute expect(result).to be_error diff --git a/spec/services/ci/job_token/clear_autopopulated_allowlist_service_spec.rb b/spec/services/ci/job_token/clear_autopopulated_allowlist_service_spec.rb index 547b723350b284..68a5f25f3e2033 100644 --- a/spec/services/ci/job_token/clear_autopopulated_allowlist_service_spec.rb +++ b/spec/services/ci/job_token/clear_autopopulated_allowlist_service_spec.rb @@ -19,6 +19,9 @@ let(:service) { described_class.new(accessed_project, developer) } it 'raises an access denied error' do + expect(Gitlab::ErrorTracking).to receive(:log_exception).with(kind_of(Gitlab::Access::AccessDeniedError), + { project_id: accessed_project.id, user_id: developer.id }) + result = service.execute expect(result).to be_error -- GitLab From 815c57907c125e591f4335e257983ee2dfe80e12 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Fri, 24 Jan 2025 07:33:57 -0600 Subject: [PATCH 7/7] Specify exceptions to rescue --- .../ci/job_token/autopopulate_allowlist_service.rb | 6 +++++- .../ci/job_token/clear_autopopulated_allowlist_service.rb | 3 --- .../ci/job_token/autopopulate_allowlist_service_spec.rb | 5 +---- .../clear_autopopulated_allowlist_service_spec.rb | 8 +------- 4 files changed, 7 insertions(+), 15 deletions(-) diff --git a/app/services/ci/job_token/autopopulate_allowlist_service.rb b/app/services/ci/job_token/autopopulate_allowlist_service.rb index 8d8033a39951a0..d23a8b95a486ba 100644 --- a/app/services/ci/job_token/autopopulate_allowlist_service.rb +++ b/app/services/ci/job_token/autopopulate_allowlist_service.rb @@ -26,7 +26,11 @@ def execute end ServiceResponse.success - rescue StandardError => e + rescue Gitlab::Utils::TraversalIdCompactor::CompactionLimitCannotBeAchievedError, + Gitlab::Utils::TraversalIdCompactor::RedundantCompactionEntry, + Gitlab::Utils::TraversalIdCompactor::UnexpectedCompactionEntry, + Ci::JobToken::AuthorizationsCompactor::UnexpectedCompactionEntry, + Ci::JobToken::AuthorizationsCompactor::RedundantCompactionEntry => e Gitlab::ErrorTracking.log_exception(e, { project_id: @project.id, user_id: @user.id }) ServiceResponse.error(message: e.message) end diff --git a/app/services/ci/job_token/clear_autopopulated_allowlist_service.rb b/app/services/ci/job_token/clear_autopopulated_allowlist_service.rb index 3001765de4461f..1ba8c2c2f5fa9e 100644 --- a/app/services/ci/job_token/clear_autopopulated_allowlist_service.rb +++ b/app/services/ci/job_token/clear_autopopulated_allowlist_service.rb @@ -19,9 +19,6 @@ def execute end ServiceResponse.success - rescue StandardError => e - Gitlab::ErrorTracking.log_exception(e, { project_id: @project.id, user_id: @user.id }) - ServiceResponse.error(message: e.message) end private diff --git a/spec/services/ci/job_token/autopopulate_allowlist_service_spec.rb b/spec/services/ci/job_token/autopopulate_allowlist_service_spec.rb index 937dbb430b6dcd..ce607aba21061f 100644 --- a/spec/services/ci/job_token/autopopulate_allowlist_service_spec.rb +++ b/spec/services/ci/job_token/autopopulate_allowlist_service_spec.rb @@ -57,10 +57,7 @@ let(:service) { described_class.new(accessed_project, developer) } it 'raises an access denied error' do - result = service.execute - - expect(result).to be_error - expect(result.message).to eq('Gitlab::Access::AccessDeniedError') + expect { service.execute }.to raise_error(Gitlab::Access::AccessDeniedError) end end diff --git a/spec/services/ci/job_token/clear_autopopulated_allowlist_service_spec.rb b/spec/services/ci/job_token/clear_autopopulated_allowlist_service_spec.rb index 68a5f25f3e2033..245ac053c890be 100644 --- a/spec/services/ci/job_token/clear_autopopulated_allowlist_service_spec.rb +++ b/spec/services/ci/job_token/clear_autopopulated_allowlist_service_spec.rb @@ -19,13 +19,7 @@ let(:service) { described_class.new(accessed_project, developer) } it 'raises an access denied error' do - expect(Gitlab::ErrorTracking).to receive(:log_exception).with(kind_of(Gitlab::Access::AccessDeniedError), - { project_id: accessed_project.id, user_id: developer.id }) - - result = service.execute - - expect(result).to be_error - expect(result.message).to eq('Gitlab::Access::AccessDeniedError') + expect { service.execute }.to raise_error(Gitlab::Access::AccessDeniedError) end end -- GitLab