From a89eb28a3cb89c54a054437c3fdc9b3e8ff2dbc4 Mon Sep 17 00:00:00 2001 From: Allison Browne Date: Thu, 27 Oct 2022 08:21:42 -0400 Subject: [PATCH] Make existing scope use outbound As we introduce the inbound scope direction we need to ensure that the existing queries look at the outboud scope for determining which projects are in scope in the existing job token scope feature --- app/models/ci/job_token/inbound_scope.rb | 37 +++++++ app/models/ci/job_token/outbound_scope.rb | 33 ++++++ app/models/ci/job_token/project_scope_link.rb | 3 + app/models/ci/job_token/scope.rb | 68 ++++++++---- app/models/project.rb | 6 ++ .../models/ci/job_token/inbound_scope_spec.rb | 101 ++++++++++++++++++ .../ci/job_token/outbound_scope_spec.rb | 68 ++++++++++++ spec/models/project_spec.rb | 14 +++ .../ci/project_ci_cd_settings_update_spec.rb | 18 ++-- 9 files changed, 321 insertions(+), 27 deletions(-) create mode 100644 app/models/ci/job_token/inbound_scope.rb create mode 100644 app/models/ci/job_token/outbound_scope.rb create mode 100644 spec/models/ci/job_token/inbound_scope_spec.rb create mode 100644 spec/models/ci/job_token/outbound_scope_spec.rb diff --git a/app/models/ci/job_token/inbound_scope.rb b/app/models/ci/job_token/inbound_scope.rb new file mode 100644 index 00000000000000..6fe0461ff3e697 --- /dev/null +++ b/app/models/ci/job_token/inbound_scope.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +# This model represents the surface where a CI_JOB_TOKEN can be used. +# +# When a project is added to the allow list for the inbound scope that +# project is allowed to access the source project via the CI_JOB_TOKEN. +# +# If the scope is disabled all projects can access the source project + +module Ci + module JobToken + class InboundScope + attr_reader :source_project + + def initialize(source_project) + @source_project = source_project + end + + def includes?(target_project) + # All projects are allowed unless this is enabled. + return true unless source_project.ci_inbound_job_token_scope_enabled? + + scope.self_referential?(target_project) || scope.added?(target_project, direction: :inbound) + end + + def all_projects + scope.all_inbound_projects + end + + private + + def scope + Ci::JobToken::Scope.new(source_project) + end + end + end +end diff --git a/app/models/ci/job_token/outbound_scope.rb b/app/models/ci/job_token/outbound_scope.rb new file mode 100644 index 00000000000000..b2dd9de2b61696 --- /dev/null +++ b/app/models/ci/job_token/outbound_scope.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +# By default the projects in the scope +# only includes the source project. + +module Ci + module JobToken + class OutboundScope + attr_reader :source_project + + def initialize(source_project) + @source_project = source_project + end + + def includes?(target_project) + # default scope is outbound. All projects are allowed if disabled. + return true unless source_project.ci_outbound_job_token_scope_enabled? + + scope.self_referential?(target_project) || scope.added?(target_project, direction: :outbound) + end + + def all_projects + scope.all_outbound_projects + end + + private + + def scope + Ci::JobToken::Scope.new(source_project) + end + end + end +end diff --git a/app/models/ci/job_token/project_scope_link.rb b/app/models/ci/job_token/project_scope_link.rb index 3fdf07123e6cfb..2a111247775ffd 100644 --- a/app/models/ci/job_token/project_scope_link.rb +++ b/app/models/ci/job_token/project_scope_link.rb @@ -12,6 +12,9 @@ class ProjectScopeLink < Ci::ApplicationRecord belongs_to :target_project, class_name: 'Project' belongs_to :added_by, class_name: 'User' + scope :added_project, ->(source_project, target_project) do + where(source_project: source_project, target_project: target_project) + end scope :from_project, ->(project) { where(source_project: project) } scope :to_project, ->(project) { where(target_project: project) } diff --git a/app/models/ci/job_token/scope.rb b/app/models/ci/job_token/scope.rb index 1aa49b95201aa6..8d84397dc82aa6 100644 --- a/app/models/ci/job_token/scope.rb +++ b/app/models/ci/job_token/scope.rb @@ -1,26 +1,33 @@ # frozen_string_literal: true +# TODO(issue 346298): Use Ci::JobToken::OutboundScope or Ci::JobToken::InboundScope +# This class will be private to those 2 classes + # This model represents the surface where a CI_JOB_TOKEN can be used. -# A Scope is initialized with the project that the job token belongs to, -# and indicates what are all the other projects that the token could access. # -# By default a job token can only access its own project, which is the same -# project that defines the scope. -# By adding ScopeLinks to the scope we can allow other projects to be accessed -# by the job token. This works as an allowlist of projects for a job token. +# A Directional Scope is initialized with a source project. +# Projects can be added to the scope by adding ScopeLinks to +# create an allowlist of projects. +# +# Projects in the outbound allowlist can be accessed via the token +# in the source project. This is the default direction. +# +# Projects in the inbound allowlist can use there token to access +# the source project. +# +# CI_JOB_TOKEN should be considered untrusted without these features enabled. # -# If a project is not included in the scope we should not allow the job user -# to access it since operations using CI_JOB_TOKEN should be considered untrusted. module Ci module JobToken class Scope attr_reader :source_project - def initialize(project) - @source_project = project + def initialize(source_project) + @source_project = source_project end + # To deprecate in favor of module Ci::JobToken::OutboundScope def includes?(target_project) # if the setting is disabled any project is considered to be in scope. return true unless source_project.ci_outbound_job_token_scope_enabled? @@ -29,21 +36,42 @@ def includes?(target_project) Ci::JobToken::ProjectScopeLink.from_project(source_project).to_project(target_project).exists? end - def all_projects - Project.from_union(target_projects, remove_duplicates: false) + def all_inbound_projects + all_projects(direction: :inbound) end - private + def all_outbound_projects + all_projects(direction: :outbound) + end - def target_project_ids - Ci::JobToken::ProjectScopeLink.from_project(source_project).pluck(:target_project_id) + # default is outbound projects + def all_projects(direction: :outbound) + Project.from_union( + [ + Project.id_in(source_project), + Project.id_in(target_project_ids(direction)) + ] + ) end - def target_projects - [ - Project.id_in(source_project), - Project.id_in(target_project_ids) - ] + def self_referential?(target_project) + target_project.id == source_project.id + end + + def added?(target_project, direction: :outbound) + Ci::JobToken::ProjectScopeLink + .added_project(source_project, target_project) + .where(direction: direction) + .exists? + end + + private + + def target_project_ids(direction) + Ci::JobToken::ProjectScopeLink + .from_project(source_project) + .where(direction: direction) + .pluck(:target_project_id) end end end diff --git a/app/models/project.rb b/app/models/project.rb index a9144fa7c2afc8..b5b28cca89e192 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2908,6 +2908,12 @@ def ci_outbound_job_token_scope_enabled? ci_cd_settings.job_token_scope_enabled? end + def ci_inbound_job_token_scope_active? + return false unless Feature.enabled?(:ci_inbound_job_token_scope, self) + + ci_inbound_job_token_scope_enabled? + end + def ci_inbound_job_token_scope_enabled? return false unless ci_cd_settings diff --git a/spec/models/ci/job_token/inbound_scope_spec.rb b/spec/models/ci/job_token/inbound_scope_spec.rb new file mode 100644 index 00000000000000..48d1b4b6d94c7e --- /dev/null +++ b/spec/models/ci/job_token/inbound_scope_spec.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::JobToken::InboundScope do + let_it_be(:source_project) { create(:project, ci_inbound_job_token_scope_enabled: true).tap(&:save!) } + + let(:scope) { described_class.new(source_project) } + + shared_context 'with scoped projects' do + let!(:inbound_scoped_project) { create_scoped_project(source_project, direction: :inbound) } + let!(:outbound_scoped_project) { create_scoped_project(source_project, direction: :outbound) } + let!(:unscoped_project1) { create(:project) } + let!(:unscoped_project2) { create(:project) } + + let!(:link_out_of_scope) { create(:ci_job_token_project_scope_link, target_project: unscoped_project1) } + end + + describe '#all_projects' do + subject(:all_projects) { scope.all_projects } + + context 'when no projects are added to the scope' do + it 'returns the project defining the scope' do + expect(all_projects).to contain_exactly(source_project) + end + end + + context 'when projects are added to the scope' do + include_context 'with scoped projects' + + it 'returns all projects that can be accessed from a given scope' do + expect(subject).to contain_exactly(source_project, inbound_scoped_project) + end + end + end + + describe '#includes?' do + subject { scope.includes?(includes_project) } + + context 'without projects' do + context 'when param is the source project' do + let(:includes_project) { source_project } + + it { is_expected.to be_truthy } + end + end + + context 'with scoped projects' do + include_context 'with scoped projects' + + context 'when param is a project in scope' do + let(:includes_project) { inbound_scoped_project } + + it { is_expected.to be_truthy } + end + + context 'when param is a project in the outbound scope' do + let(:includes_project) { outbound_scoped_project } + + it { is_expected.to be_falsey } + end + + context 'when param is a project linked to a different project' do + let(:includes_project) { unscoped_project1 } + + it { is_expected.to be_falsey } + end + + context 'when param is a project unlinked to any project' do + let(:includes_project) { unscoped_project2 } + + it { is_expected.to be_falsey } + end + + context 'when project scope setting is disabled' do + let(:includes_project) { unscoped_project1 } + + before do + source_project.ci_inbound_job_token_scope_enabled = false + end + + it 'considers any project to be part of the scope' do + expect(subject).to be_truthy + end + end + end + end + + private + + def create_scoped_project(source_project, direction: 0) + create(:project).tap do |scoped_project| + create( + :ci_job_token_project_scope_link, + source_project: source_project, + target_project: scoped_project, + direction: direction + ) + end + end +end diff --git a/spec/models/ci/job_token/outbound_scope_spec.rb b/spec/models/ci/job_token/outbound_scope_spec.rb new file mode 100644 index 00000000000000..9e67c454f93a51 --- /dev/null +++ b/spec/models/ci/job_token/outbound_scope_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::JobToken::OutboundScope do + let_it_be(:project) { create(:project, ci_outbound_job_token_scope_enabled: true).tap(&:save!) } + + let(:scope) { described_class.new(project) } + + describe '#all_projects' do + subject(:all_projects) { scope.all_projects } + + context 'when no projects are added to the scope' do + it 'returns the project defining the scope' do + expect(all_projects).to contain_exactly(project) + end + end + + context 'when other projects are added to the scope' do + let_it_be(:scoped_project) { create(:project) } + let_it_be(:unscoped_project) { create(:project) } + + let!(:link_in_scope) do + create(:ci_job_token_project_scope_link, source_project: project, target_project: scoped_project) + end + + let!(:link_out_of_scope) { create(:ci_job_token_project_scope_link, target_project: unscoped_project) } + + it 'returns all projects that can be accessed from a given scope' do + expect(subject).to contain_exactly(project, scoped_project) + end + end + end + + describe '#includes?' do + subject { scope.includes?(target_project) } + + context 'when param is the project defining the scope' do + let(:target_project) { project } + + it { is_expected.to be_truthy } + end + + context 'when param is a project in scope' do + let(:target_link) { create(:ci_job_token_project_scope_link, source_project: project) } + let(:target_project) { target_link.target_project } + + it { is_expected.to be_truthy } + end + + context 'when param is a project in another scope' do + let(:scope_link) { create(:ci_job_token_project_scope_link) } + let(:target_project) { scope_link.target_project } + + it { is_expected.to be_falsey } + + context 'when project scope setting is disabled' do + before do + project.ci_outbound_job_token_scope_enabled = false + end + + it 'considers any project to be part of the scope' do + expect(subject).to be_truthy + end + end + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 2fffbe578de63a..f18ccb692628f3 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -941,6 +941,20 @@ end end + describe '#inbound_job_token_scope_active?' do + let_it_be(:project) { FactoryBot.create(:project, ci_inbound_job_token_scope_enabled: true) } + + specify { expect(project.ci_inbound_job_token_scope_active?).to eq true } + + context 'when ci_inbound_inbound_job_token_scope feature is disabled' do + before do + stub_feature_flags(ci_inbound_job_token_scope: false) + end + + specify { expect(project.ci_inbound_job_token_scope_active?).to eq false } + end + end + describe '#restrict_user_defined_variables?' do it_behaves_like 'a ci_cd_settings predicate method' do let(:delegated_method) { :restrict_user_defined_variables? } diff --git a/spec/requests/api/graphql/mutations/ci/project_ci_cd_settings_update_spec.rb b/spec/requests/api/graphql/mutations/ci/project_ci_cd_settings_update_spec.rb index c808cf5ede95de..4f9741c1b030e6 100644 --- a/spec/requests/api/graphql/mutations/ci/project_ci_cd_settings_update_spec.rb +++ b/spec/requests/api/graphql/mutations/ci/project_ci_cd_settings_update_spec.rb @@ -90,15 +90,19 @@ expect(project.ci_inbound_job_token_scope_enabled).to eq(false) end - it 'does not update inbound_job_token_scope_enabled if not specified' do - variables.except!(:inbound_job_token_scope_enabled) + context 'when not included' do + before do + variables.except!(:inbound_job_token_scope_enabled) + end - post_graphql_mutation(mutation, current_user: user) + it 'does not update inbound_job_token_scope_enabled' do + post_graphql_mutation(mutation, current_user: user) - project.reload + project.reload - expect(response).to have_gitlab_http_status(:success) - expect(project.ci_inbound_job_token_scope_enabled).to eq(true) + expect(response).to have_gitlab_http_status(:success) + expect(project.ci_inbound_job_token_scope_enabled).to eq(true) + end end context 'when ci_inbound_job_token_scope disabled' do @@ -106,7 +110,7 @@ stub_feature_flags(ci_inbound_job_token_scope: false) end - it 'does not update inbound_job_token_scope_enabled' do + it 'does not update inbound_job_token_scope_enabled if not specified' do post_graphql_mutation(mutation, current_user: user) project.reload -- GitLab