From d28becab7945b89ee460b5ff2e9b611fe18de7be Mon Sep 17 00:00:00 2001 From: drew Date: Tue, 31 Jan 2023 13:22:39 -0500 Subject: [PATCH 1/2] Add directionality field to JobToken::RemoveProject GraphQL mutation --- .../ci/job_token_scope/remove_project.rb | 9 ++++- app/models/ci/job_token/project_scope_link.rb | 5 ++- .../job_token_scope/remove_project_service.rb | 6 ++- doc/api/graphql/reference/index.md | 1 + .../ci/job_token_scope/remove_project_spec.rb | 38 ++++++++++++++++--- .../ci/job_token_scope/remove_project_spec.rb | 3 +- 6 files changed, 50 insertions(+), 12 deletions(-) diff --git a/app/graphql/mutations/ci/job_token_scope/remove_project.rb b/app/graphql/mutations/ci/job_token_scope/remove_project.rb index f503b4f2f7aab2..e5f40bbf87e67f 100644 --- a/app/graphql/mutations/ci/job_token_scope/remove_project.rb +++ b/app/graphql/mutations/ci/job_token_scope/remove_project.rb @@ -18,18 +18,23 @@ class RemoveProject < BaseMutation required: true, description: 'Project to be removed from the CI job token scope.' + argument :direction, + ::Types::Ci::JobTokenScope::DirectionEnum, + required: false, + description: 'Direction of access, which defaults to outbound.' + field :ci_job_token_scope, Types::Ci::JobTokenScopeType, null: true, description: "CI job token's scope of access." - def resolve(project_path:, target_project_path:) + def resolve(project_path:, target_project_path:, direction: :outbound) project = authorized_find!(project_path) target_project = Project.find_by_full_path(target_project_path) result = ::Ci::JobTokenScope::RemoveProjectService .new(project, current_user) - .execute(target_project) + .execute(target_project, direction: direction) if result.success? { diff --git a/app/models/ci/job_token/project_scope_link.rb b/app/models/ci/job_token/project_scope_link.rb index 4fb577f58bc6d6..81c1c66be5cd11 100644 --- a/app/models/ci/job_token/project_scope_link.rb +++ b/app/models/ci/job_token/project_scope_link.rb @@ -13,8 +13,9 @@ class ProjectScopeLink < Ci::ApplicationRecord belongs_to :target_project, class_name: 'Project' belongs_to :added_by, class_name: 'User' - scope :with_source, ->(project) { where(source_project: project) } - scope :with_target, ->(project) { where(target_project: project) } + scope :direction, ->(direction) { where(direction: direction) } + scope :with_source, ->(project) { where(source_project: project) } + scope :with_target, ->(project) { where(target_project: project) } validates :source_project, presence: true validates :target_project, presence: true diff --git a/app/services/ci/job_token_scope/remove_project_service.rb b/app/services/ci/job_token_scope/remove_project_service.rb index 15644e529d9987..e9060706abca12 100644 --- a/app/services/ci/job_token_scope/remove_project_service.rb +++ b/app/services/ci/job_token_scope/remove_project_service.rb @@ -5,14 +5,16 @@ module JobTokenScope class RemoveProjectService < ::BaseService include EditScopeValidations - def execute(target_project) + def execute(target_project, direction: :outbound) validate_edit!(project, target_project, current_user) if project == target_project return ServiceResponse.error(message: "Source project cannot be removed from the job token scope") end - link = ::Ci::JobToken::ProjectScopeLink.for_source_and_target(project, target_project) + link = ::Ci::JobToken::ProjectScopeLink + .direction(direction) + .for_source_and_target(project, target_project) unless link return ServiceResponse.error(message: "Target project is not in the job token scope") diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index d9e07d45355c32..3e1c91040c9f3c 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -1204,6 +1204,7 @@ Input type: `CiJobTokenScopeRemoveProjectInput` | Name | Type | Description | | ---- | ---- | ----------- | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `direction` | [`CiJobTokenScopeDirection`](#cijobtokenscopedirection) | Direction of access, which defaults to outbound. | | `projectPath` | [`ID!`](#id) | Project that the CI job token scope belongs to. | | `targetProjectPath` | [`ID!`](#id) | Project to be removed from the CI job token scope. | diff --git a/spec/graphql/mutations/ci/job_token_scope/remove_project_spec.rb b/spec/graphql/mutations/ci/job_token_scope/remove_project_spec.rb index 4a0501bddbddb4..203e3f50f4b832 100644 --- a/spec/graphql/mutations/ci/job_token_scope/remove_project_spec.rb +++ b/spec/graphql/mutations/ci/job_token_scope/remove_project_spec.rb @@ -17,6 +17,7 @@ end let(:target_project_path) { target_project.full_path } + let(:links_relation) { Ci::JobToken::ProjectScopeLink.with_source(project).with_target(target_project) } subject do mutation.resolve(project_path: project.full_path, target_project_path: target_project_path) @@ -45,10 +46,37 @@ target_project.add_guest(current_user) end - it 'removes target project from the job token scope' do - expect do - expect(subject).to include(ci_job_token_scope: be_present, errors: be_empty) - end.to change { Ci::JobToken::ProjectScopeLink.count }.by(-1) + context 'with no direction specified' do + it 'defaults to removing an outbound link to the target project' do + expect do + expect(subject).to include(ci_job_token_scope: be_present, errors: be_empty) + end.to change { Ci::JobToken::ProjectScopeLink.count }.by(-1) + + expect(links_relation.outbound.reload).to be_empty + end + end + + context 'with an inbound direction specified' do + let!(:inbound_link) do + create(:ci_job_token_project_scope_link, + source_project: project, + target_project: target_project, + direction: 'inbound' + ) + end + + subject do + mutation.resolve(project_path: project.full_path, target_project_path: target_project_path, direction: 'inbound') + end + + it 'removes the inbound scope without removing the outbound scope' do + expect do + expect(subject).to include(ci_job_token_scope: be_present, errors: be_empty) + end.to change { Ci::JobToken::ProjectScopeLink.count }.by(-1) + + expect(links_relation.inbound.reload).to be_empty + expect(links_relation.outbound.reload).not_to be_empty + end end context 'when the service returns an error' do @@ -56,7 +84,7 @@ it 'returns an error response' do expect(::Ci::JobTokenScope::RemoveProjectService).to receive(:new).with(project, current_user).and_return(service) - expect(service).to receive(:execute).with(target_project).and_return(ServiceResponse.error(message: 'The error message')) + expect(service).to receive(:execute).with(target_project, direction: :outbound).and_return(ServiceResponse.error(message: 'The error message')) expect(subject.fetch(:ci_job_token_scope)).to be_nil expect(subject.fetch(:errors)).to include("The error message") diff --git a/spec/requests/api/graphql/mutations/ci/job_token_scope/remove_project_spec.rb b/spec/requests/api/graphql/mutations/ci/job_token_scope/remove_project_spec.rb index 61d5c56ae8a2c3..805f300072d52a 100644 --- a/spec/requests/api/graphql/mutations/ci/job_token_scope/remove_project_spec.rb +++ b/spec/requests/api/graphql/mutations/ci/job_token_scope/remove_project_spec.rb @@ -22,6 +22,7 @@ let(:variables) do { + direction: 'OUTBOUND', project_path: project.full_path, target_project_path: target_project.full_path } @@ -67,7 +68,7 @@ target_project.add_guest(current_user) end - it 'removes the target project from the job token scope' do + it 'removes the target project from the job token outbound scope' do expect do post_graphql_mutation(mutation, current_user: current_user) expect(response).to have_gitlab_http_status(:success) -- GitLab From c2a031e07df95e13fd024dc73c1b8a546d8dfc71 Mon Sep 17 00:00:00 2001 From: Allison Browne Date: Tue, 7 Feb 2023 09:57:46 -0500 Subject: [PATCH 2/2] Changes from code review feedback --- app/models/ci/job_token/project_scope_link.rb | 2 +- .../job_token_scope/remove_project_service.rb | 2 +- .../ci/job_token_scope/remove_project_spec.rb | 24 +++++++------------ 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/app/models/ci/job_token/project_scope_link.rb b/app/models/ci/job_token/project_scope_link.rb index 81c1c66be5cd11..774d85e3d3c0d7 100644 --- a/app/models/ci/job_token/project_scope_link.rb +++ b/app/models/ci/job_token/project_scope_link.rb @@ -13,7 +13,7 @@ class ProjectScopeLink < Ci::ApplicationRecord belongs_to :target_project, class_name: 'Project' belongs_to :added_by, class_name: 'User' - scope :direction, ->(direction) { where(direction: direction) } + scope :with_access_direction, ->(direction) { where(direction: direction) } scope :with_source, ->(project) { where(source_project: project) } scope :with_target, ->(project) { where(target_project: project) } diff --git a/app/services/ci/job_token_scope/remove_project_service.rb b/app/services/ci/job_token_scope/remove_project_service.rb index e9060706abca12..d21eff2b6193eb 100644 --- a/app/services/ci/job_token_scope/remove_project_service.rb +++ b/app/services/ci/job_token_scope/remove_project_service.rb @@ -13,7 +13,7 @@ def execute(target_project, direction: :outbound) end link = ::Ci::JobToken::ProjectScopeLink - .direction(direction) + .with_access_direction(direction) .for_source_and_target(project, target_project) unless link diff --git a/spec/graphql/mutations/ci/job_token_scope/remove_project_spec.rb b/spec/graphql/mutations/ci/job_token_scope/remove_project_spec.rb index 203e3f50f4b832..7fb45e93474491 100644 --- a/spec/graphql/mutations/ci/job_token_scope/remove_project_spec.rb +++ b/spec/graphql/mutations/ci/job_token_scope/remove_project_spec.rb @@ -56,31 +56,25 @@ end end - context 'with an inbound direction specified' do - let!(:inbound_link) do - create(:ci_job_token_project_scope_link, - source_project: project, - target_project: target_project, - direction: 'inbound' - ) - end + context 'with direction specified' do + let(:service) { instance_double('Ci::JobTokenScope::RemoveProjectService') } subject do mutation.resolve(project_path: project.full_path, target_project_path: target_project_path, direction: 'inbound') end - it 'removes the inbound scope without removing the outbound scope' do - expect do - expect(subject).to include(ci_job_token_scope: be_present, errors: be_empty) - end.to change { Ci::JobToken::ProjectScopeLink.count }.by(-1) + it 'executes project removal for the correct direction' do + expect(::Ci::JobTokenScope::RemoveProjectService) + .to receive(:new).with(project, current_user).and_return(service) + expect(service).to receive(:execute).with(target_project, direction: 'inbound') + .and_return(instance_double('ServiceResponse', "success?": true)) - expect(links_relation.inbound.reload).to be_empty - expect(links_relation.outbound.reload).not_to be_empty + subject end end context 'when the service returns an error' do - let(:service) { double(:service) } + let(:service) { instance_double('Ci::JobTokenScope::RemoveProjectService') } it 'returns an error response' do expect(::Ci::JobTokenScope::RemoveProjectService).to receive(:new).with(project, current_user).and_return(service) -- GitLab