diff --git a/app/assets/javascripts/editor/schema/ci.json b/app/assets/javascripts/editor/schema/ci.json index b5b0fd9721a45a8ffcd4adf558b18bb431d8bd13..fd3623162b8f94d722ea0667ff0bc59aad047dad 100644 --- a/app/assets/javascripts/editor/schema/ci.json +++ b/app/assets/javascripts/editor/schema/ci.json @@ -239,6 +239,16 @@ "always" ] }, + "access": { + "markdownDescription": "Configure who can access the artifacts. [Learn More](https://docs.gitlab.com/ee/ci/yaml/#artifactsaccess).", + "default": "all", + "type": "string", + "enum": [ + "none", + "developer", + "all" + ] + }, "expire_in": { "type": "string", "markdownDescription": "How long artifacts should be kept. They are saved 30 days by default. Artifacts that have expired are removed periodically via cron job. Supports a wide variety of formats, e.g. '1 week', '3 mins 4 sec', '2 hrs 20 min', '2h20min', '6 mos 1 day', '47 yrs 6 mos and 4d', '3 weeks and 2 days'. [Learn More](https://docs.gitlab.com/ee/ci/yaml/#artifactsexpire_in).", diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 7bda535c72d61c229d4edd6e976b5d0b4e090931..38c2227ae33bc0ef789346273f14d7bd9d8df894 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -726,12 +726,24 @@ def artifacts_public? job_artifacts_archive.public_access? end - def artifact_is_public_in_config? + def artifacts_no_access? + return false if job_artifacts_archive.nil? # To backward compatibility return false if no artifacts found + + job_artifacts_archive.none_access? + end + + def artifact_access_setting_in_config artifacts_public = options.dig(:artifacts, :public) + artifacts_access = options.dig(:artifacts, :access) + + raise ArgumentError, 'artifacts:public and artifacts:access are mutually exclusive' if !artifacts_public.nil? && !artifacts_access.nil? - return true if artifacts_public.nil? # Default artifacts:public to true + return :public if artifacts_public == true || artifacts_access == 'all' + return :private if artifacts_public == false || artifacts_access == 'developer' + return :none if artifacts_access == 'none' - artifacts_public + # default behaviour + :public end def artifacts_metadata_entry(path, **options) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 5a08924fc4778c1026a8ab04f2743d0453d5a02d..ca7f539d386c0330eefc9419f8daa53d4c7a7bc7 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -19,7 +19,7 @@ class JobArtifact < Ci::ApplicationRecord self.primary_key = :id self.sequence_name = :ci_job_artifacts_id_seq - enum accessibility: { public: 0, private: 1 }, _suffix: true + enum accessibility: { public: 0, private: 1, none: 2 }, _suffix: true PLAN_LIMIT_PREFIX = 'ci_max_artifact_size_' @@ -227,6 +227,10 @@ def public_access? public_accessibility? end + def none_access? + none_accessibility? + end + private def store_file_in_transaction! diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index 71ea42e1f235cf18f3250b4c49ac54ce11559938..dcdd7d6668637ad2f5ea60ca032a1616d94d5538 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -40,6 +40,10 @@ class BuildPolicy < CommitStatusPolicy @subject.artifacts_public? end + condition(:artifacts_none, scope: :subject) do + @subject.artifacts_no_access? + end + condition(:terminal, scope: :subject) do @subject.has_terminal? end @@ -116,7 +120,7 @@ class BuildPolicy < CommitStatusPolicy prevent :create_build_service_proxy end - rule { can_read_project_build }.enable :read_job_artifacts + rule { can_read_project_build & ~artifacts_none }.enable :read_job_artifacts rule { ~artifacts_public & ~project_developer }.prevent :read_job_artifacts end end diff --git a/app/policies/ci/job_artifact_policy.rb b/app/policies/ci/job_artifact_policy.rb index 61c935af8badc42f1d7b88427d88c81c40086099..8f0b9847d8ffcdcebc5ac136020de7d7eb96b70a 100644 --- a/app/policies/ci/job_artifact_policy.rb +++ b/app/policies/ci/job_artifact_policy.rb @@ -8,6 +8,10 @@ class JobArtifactPolicy < BasePolicy @subject.public_access? end + condition(:none_access, scope: :subject) do + @subject.none_access? + end + condition(:can_read_project_build, scope: :subject) do can?(:read_build, @subject.job.project) end @@ -16,7 +20,7 @@ class JobArtifactPolicy < BasePolicy can?(:developer_access, @subject.job.project) end - rule { can_read_project_build }.enable :read_job_artifacts + rule { can_read_project_build & ~none_access }.enable :read_job_artifacts rule { ~public_access & ~has_access_to_project }.prevent :read_job_artifacts end end diff --git a/app/services/ci/job_artifacts/create_service.rb b/app/services/ci/job_artifacts/create_service.rb index 0791fff8545a9965fdd6285c0b294a0744c814d1..82f2c22adadc3979d1e054608efe8a4297a08ba3 100644 --- a/app/services/ci/job_artifacts/create_service.rb +++ b/app/services/ci/job_artifacts/create_service.rb @@ -130,7 +130,7 @@ def accessibility(params) return accessibility if accessibility.present? - job.artifact_is_public_in_config? ? :public : :private + job.artifact_access_setting_in_config end def parse_artifact(artifact) diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md index 7bedef67625cea26f5119f391c9630decb545747..ee8d482010b75b2dc7824197bf3b9116d4d7d939 100644 --- a/doc/ci/yaml/index.md +++ b/doc/ci/yaml/index.md @@ -1424,6 +1424,10 @@ job: > - [Updated](https://gitlab.com/gitlab-org/gitlab/-/issues/322454) in GitLab 15.10. Artifacts created with `artifacts:public` before 15.10 are not guaranteed to remain private after this update. > - [Generally available](https://gitlab.com/gitlab-org/gitlab/-/issues/294503) in GitLab 16.7. Feature flag `non_public_artifacts` removed. +NOTE: +`artifacts:public` is now superceded by [`artifacts:access`](#artifactsaccess) which +has more options. + Use `artifacts:public` to determine whether the job artifacts should be publicly available. @@ -1448,6 +1452,31 @@ job: public: false ``` +### `artifacts:access` + +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/145206) in GitLab 16.10. + +Use `artifacts:access` to determine who can access the job artifacts. + +You cannot use [`artifacts:public`](#artifactspublic) and `artifacts:access` in the same job. + +**Keyword type**: Job keyword. You can use it only as part of a job. + +**Possible inputs**: + +- `all` (default): Artifacts in public pipelines are available for download by anyone, including anonymous, + guest, and reporter users. +- `developer`: Artifacts are only available for download by users with the Developer role or higher. +- `none`: Artifacts are not available for download by anyone. + +**Example of `artifacts:access`**: + +```yaml +job: + artifacts: + access: 'developer' +``` + #### `artifacts:reports` Use [`artifacts:reports`](artifacts_reports.md) to collect artifacts generated by diff --git a/ee/spec/policies/ci/job_artifact_policy_spec.rb b/ee/spec/policies/ci/job_artifact_policy_spec.rb index 6fbf9e376c60dd5aec7ad5fd3825c6e719569f96..0b1a713853b8d007028556661f0eb4bc352db7b3 100644 --- a/ee/spec/policies/ci/job_artifact_policy_spec.rb +++ b/ee/spec/policies/ci/job_artifact_policy_spec.rb @@ -29,6 +29,14 @@ expect(policy).to be_allowed :read_job_artifacts end end + + context 'when job artifact access is set to none' do + let(:job_artifact) { create(:ci_job_artifact, :none, project: project) } + + it 'disallows read_job_artifacts' do + expect(policy).to be_disallowed :read_job_artifacts + end + end end describe 'for user with access to the project' do @@ -51,6 +59,14 @@ expect(policy).to be_allowed :read_job_artifacts end end + + context 'when job artifact access is set to none' do + let(:job_artifact) { create(:ci_job_artifact, :none, project: project) } + + it 'disallows read_job_artifacts' do + expect(policy).to be_disallowed :read_job_artifacts + end + end end describe 'for auditor user' do @@ -75,6 +91,14 @@ expect(policy).to be_allowed :read_job_artifacts end end + + context 'when job artifact access is set to none' do + let(:job_artifact) { create(:ci_job_artifact, :none, project: project) } + + it 'disallows read_job_artifacts' do + expect(policy).to be_disallowed :read_job_artifacts + end + end end describe 'for reporter user' do @@ -97,6 +121,14 @@ expect(policy).to be_allowed :read_job_artifacts end end + + context 'when job artifact access is set to none' do + let(:job_artifact) { create(:ci_job_artifact, :none, project: project) } + + it 'allows read_job_artifacts' do + expect(policy).to be_disallowed :read_job_artifacts + end + end end describe 'for guest user' do @@ -118,6 +150,14 @@ expect(policy).to be_allowed :read_job_artifacts end end + + context 'when job artifact access is set to none' do + let(:job_artifact) { create(:ci_job_artifact, :none, project: project) } + + it 'disallows read_job_artifacts' do + expect(policy).to be_disallowed :read_job_artifacts + end + end end end end diff --git a/lib/gitlab/ci/config/entry/artifacts.rb b/lib/gitlab/ci/config/entry/artifacts.rb index 3fd07811daffe5dadcd2ce3d528b993839d09079..688ce89501128c68bbcc5e5d48b1910ba0cced62 100644 --- a/lib/gitlab/ci/config/entry/artifacts.rb +++ b/lib/gitlab/ci/config/entry/artifacts.rb @@ -13,7 +13,8 @@ class Artifacts < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Attributable ALLOWED_WHEN = %w[on_success on_failure always].freeze - ALLOWED_KEYS = %i[name untracked paths reports when expire_in expose_as exclude public].freeze + ALLOWED_ACCESS = %w[none developer all].freeze + ALLOWED_KEYS = %i[name untracked paths reports when expire_in expose_as exclude public access].freeze EXPOSE_AS_REGEX = /\A\w[-\w ]*\z/ EXPOSE_AS_ERROR_MESSAGE = "can contain only letters, digits, '-', '_' and spaces" @@ -26,6 +27,8 @@ class Artifacts < ::Gitlab::Config::Entry::Node validates :config, allowed_keys: ALLOWED_KEYS validates :paths, presence: true, if: :expose_as_present? + validates :config, mutually_exclusive_keys: %i[access public] + with_options allow_nil: true do validates :name, type: String validates :public, boolean: true @@ -44,6 +47,10 @@ class Artifacts < ::Gitlab::Config::Entry::Node message: "should be one of: #{ALLOWED_WHEN.join(', ')}" } validates :expire_in, duration: { parser: ::Gitlab::Ci::Build::DurationParser } + validates :access, type: String, inclusion: { + in: ALLOWED_ACCESS, + message: "should be one of: #{ALLOWED_ACCESS.join(', ')}" + } end end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 0556b42aa97e21fd3fc6f132e35286c6165b527d..99a65ed13d2ce96f1fd97a152e79319413bbaca1 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -330,6 +330,14 @@ end end + trait :no_access_artifacts do + after(:create) do |build, evaluator| + create(:ci_job_artifact, :archive, :none, job: build, expire_at: build.artifacts_expire_at) + create(:ci_job_artifact, :metadata, :none, job: build, expire_at: build.artifacts_expire_at) + build.reload + end + end + trait :report_results do after(:build) do |build| build.report_results << build(:ci_build_report_result) @@ -597,6 +605,34 @@ end end + trait :with_developer_access_artifacts do + options do + { + artifacts: { access: 'developer' } + } + end + end + + # invalid case for access setting + trait :with_access_and_public_setting do + options do + { + artifacts: { + public: true, + access: 'all' + } + } + end + end + + trait :with_none_access_artifacts do + options do + { + artifacts: { access: 'none' } + } + end + end + trait :with_public_artifacts_config do options do { @@ -605,6 +641,14 @@ end end + trait :with_all_access_artifacts do + options do + { + artifacts: { access: 'all' } + } + end + end + trait :non_playable do status { 'created' } self.when { 'manual' } diff --git a/spec/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb index 292138d76dbcc439ea7d3814535c843327ebc5d7..26004ea929d1528934984734eb1d5285deaa7b71 100644 --- a/spec/factories/ci/job_artifacts.rb +++ b/spec/factories/ci/job_artifacts.rb @@ -182,6 +182,10 @@ accessibility { 'public' } end + trait :none do + accessibility { 'none' } + end + trait :accessibility do file_type { :accessibility } file_format { :raw } diff --git a/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/artifacts.yml b/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/artifacts.yml index ba4b0db908de2f5eeabd6d916b3a959812c40680..5b80511cbec4f61b838cfd2ad4a79283cbc77d01 100644 --- a/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/artifacts.yml +++ b/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/artifacts.yml @@ -67,3 +67,12 @@ artifacts-when-array: artifacts-when-boolean: artifacts: when: true + +artifacts-access-all: + artifacts: + public: true + accesss: all + +artifacts-access-invalid-value: + artifacts: + access: random diff --git a/spec/frontend/editor/schema/ci/yaml_tests/positive_tests/artifacts.yml b/spec/frontend/editor/schema/ci/yaml_tests/positive_tests/artifacts.yml index 70761a09b582b0edb9d6f6582e3b8d3589d0e900..f37de22551d880ecea4d990ee3c54ad2d5f34a5c 100644 --- a/spec/frontend/editor/schema/ci/yaml_tests/positive_tests/artifacts.yml +++ b/spec/frontend/editor/schema/ci/yaml_tests/positive_tests/artifacts.yml @@ -51,3 +51,15 @@ artifacts-no-when: artifacts: paths: - binaries/ + +artifacts-access-developer: + artifacts: + access: developer + +artifacts-access-all: + artifacts: + access: all + +artifacts-access-none: + artifacts: + access: none diff --git a/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb b/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb index 6264e0c8e33f211dd10953a1ba9da6ccceccd6b8..ffd5e902a509a30e97d77a6ca51efc589573f183 100644 --- a/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb @@ -142,6 +142,35 @@ end end + context 'when access keyword is used' do + context 'when used with public' do + let(:config) { { paths: %w[results.txt], public: true, access: 'all' } } + + it 'reports error' do + expect(entry.errors) + .to include "artifacts config these keys cannot be used together: public, access" + end + end + + context 'when value is not one from expected list' do + let(:config) { { paths: %w[results.txt], access: 'random' } } + + it 'reports error' do + expect(entry.errors) + .to include 'artifacts access should be one of: none, developer, all' + end + end + + context 'when value is correct' do + let(:config) { { paths: %w[results.txt], access: 'developer' } } + + it 'correctly parses the configuration' do + expect(entry).to be_valid + expect(entry.value).to eq(config) + end + end + end + context 'when the `when` keyword is not a string' do context 'when it is an array' do let(:config) { { paths: %w[results.txt], when: ['always'] } } diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 78820c0f36977f4ab4876bb0c3f9c9e464a18936..d7d06bee0d96b55b9684bf5405b0329142d08f78 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1055,31 +1055,57 @@ end end - describe '#artifact_is_public_in_config?' do - subject { build.artifact_is_public_in_config? } + describe '#artifact_access_setting_in_config' do + subject { build.artifact_access_setting_in_config } context 'artifacts with defaults' do let(:build) { create(:ci_build, :artifacts, pipeline: pipeline) } - it { is_expected.to be_truthy } + it { is_expected.to eq(:public) } end context 'non public artifacts' do let(:build) { create(:ci_build, :with_private_artifacts_config, pipeline: pipeline) } - it { is_expected.to be_falsey } + it { is_expected.to eq(:private) } + end + + context 'non public artifacts via access' do + let(:build) { create(:ci_build, :with_developer_access_artifacts, pipeline: pipeline) } + + it { is_expected.to eq(:private) } + end + + context 'non public artifacts via access as none' do + let(:build) { create(:ci_build, :with_none_access_artifacts, pipeline: pipeline) } + + it { is_expected.to eq(:none) } end context 'public artifacts' do let(:build) { create(:ci_build, :with_public_artifacts_config, pipeline: pipeline) } - it { is_expected.to be_truthy } + it { is_expected.to eq(:public) } + end + + context 'public artifacts via access' do + let(:build) { create(:ci_build, :with_all_access_artifacts, pipeline: pipeline) } + + it { is_expected.to eq(:public) } end context 'no artifacts' do let(:build) { create(:ci_build, pipeline: pipeline) } - it { is_expected.to be_truthy } + it { is_expected.to eq(:public) } + end + + context 'when public and access are used together' do + let(:build) { create(:ci_build, :with_access_and_public_setting, pipeline: pipeline) } + + it 'raises ArgumentError' do + expect { subject }.to raise_error(ArgumentError, 'artifacts:public and artifacts:access are mutually exclusive') + end end end diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb index a4214e1a93ee5a32f15192a1d2711e4d122a3ee4..76269868dcf7c9a5e2b434e62da298146b087326 100644 --- a/spec/models/ci/job_artifact_spec.rb +++ b/spec/models/ci/job_artifact_spec.rb @@ -179,6 +179,22 @@ end end + describe 'none_access?' do + subject { artifact.none_access? } + + context 'when job artifact created by default' do + let!(:artifact) { create(:ci_job_artifact) } + + it { is_expected.to be_falsey } + end + + context 'when job artifact created as none access' do + let!(:artifact) { create(:ci_job_artifact, :none) } + + it { is_expected.to be_truthy } + end + end + describe '.file_types_for_report' do it 'returns the report file types for the report type' do expect(described_class.file_types_for_report(:test)).to match_array(%w[junit]) diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb index ad568e60d5ce0abc11247a0818816416fe6c4d37..3a928db67f74851789cec0188df92912fa598662 100644 --- a/spec/policies/ci/build_policy_spec.rb +++ b/spec/policies/ci/build_policy_spec.rb @@ -19,6 +19,53 @@ end end + describe 'artifacts access config with access keyword' do + let_it_be(:project) { create(:project, :public) } + let_it_be(:user) { create(:user) } + + include_context 'public pipelines disabled' + + before_all do + project.add_developer(user) + end + + context 'when job artifact access is set to all' do + let(:build) { create(:ci_build, :artifacts, pipeline: pipeline) } + + it 'allows read_job_artifacts to project members' do + expect(policy).to be_allowed :read_job_artifacts + end + end + + context 'when job artifact is private to developers' do + let(:build) { create(:ci_build, :private_artifacts, pipeline: pipeline) } + + it 'allows read_job_artifacts to project members' do + expect(policy).to be_allowed :read_job_artifacts + end + + context 'when user is anon' do + let(:user2) { create(:user) } + + let(:policy2) do + described_class.new(user2, build) + end + + it 'disallows read_job_artifacts to anon user' do + expect(policy2).to be_disallowed :read_job_artifacts + end + end + end + + context 'when job artifact access is set to none' do + let(:build) { create(:ci_build, :no_access_artifacts, pipeline: pipeline) } + + it 'disallows read_job_artifacts to project members' do + expect(policy).to be_disallowed :read_job_artifacts + end + end + end + describe '#rules' do context 'when user does not have access to the project' do let(:project) { create(:project, :private) } diff --git a/spec/services/ci/job_artifacts/create_service_spec.rb b/spec/services/ci/job_artifacts/create_service_spec.rb index 8f7afcc0c74b6bb4c395e7108777ae91d13d25fa..39bba6a2dd8f3142bded34b166038338040a4110 100644 --- a/spec/services/ci/job_artifacts/create_service_spec.rb +++ b/spec/services/ci/job_artifacts/create_service_spec.rb @@ -136,6 +136,22 @@ subject(:execute) { service.execute(artifacts_file, params, metadata_file: metadata_file) } + context 'when artifacts have none access setting' do + let(:artifacts_file) do + file_to_upload('spec/fixtures/ci_build_artifacts.zip', sha256: artifacts_sha256) + end + + let(:access_job) { create(:ci_build, :no_access_artifacts, project: project) } + let(:access_service) { described_class.new(access_job) } + + it 'sets accessibility to none' do + access_service.execute(artifacts_file, params, metadata_file: metadata_file) + + expect(access_job.job_artifacts).not_to be_empty + expect(access_job.job_artifacts).to all be_none_accessibility + end + end + shared_examples_for 'handling accessibility' do shared_examples 'public accessibility' do it 'sets accessibility to public level' do