diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 71939f070cb630f74c94b53b7ba5ed9811a5a878..fcbe0b350a3f6315d6c1a230a10fb32d022605c4 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -146,6 +146,12 @@ def persisted_environment .includes(:metadata, :job_artifacts_metadata) end + scope :with_project_and_metadata, -> do + if Feature.enabled?(:non_public_artifacts, type: :development) + joins(:metadata).includes(:project, :metadata) + end + end + scope :with_artifacts_not_expired, -> { with_downloadable_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.current) } scope :with_expired_artifacts, -> { with_downloadable_artifacts.where('artifacts_expire_at < ?', Time.current) } scope :last_month, -> { where('created_at > ?', Date.today - 1.month) } @@ -741,6 +747,16 @@ def browsable_artifacts? artifacts_metadata? end + def artifacts_public? + return true unless Feature.enabled?(:non_public_artifacts, type: :development) + + artifacts_public = options.dig(:artifacts, :public) + + return true if artifacts_public.nil? # Default artifacts:public to true + + options.dig(:artifacts, :public) + end + def artifacts_metadata_entry(path, **options) artifacts_metadata.open do |metadata_stream| metadata = Gitlab::Ci::Build::Artifacts::Metadata.new( diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index c80d50ea131362e514be13ed4b9c6c7adf13eedb..d34c85ed17b9e3ee17ee55e2e20a512f5c3c71ca 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -133,6 +133,12 @@ class JobArtifact < ApplicationRecord scope :for_sha, ->(sha, project_id) { joins(job: :pipeline).where(ci_pipelines: { sha: sha, project_id: project_id }) } scope :for_job_name, ->(name) { joins(:job).where(ci_builds: { name: name }) } + scope :with_job, -> do + if Feature.enabled?(:non_public_artifacts, type: :development) + joins(:job).includes(:job) + end + end + scope :with_file_types, -> (file_types) do types = self.file_types.select { |file_type| file_types.include?(file_type) }.values diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 5e5f51d776fd7308d29199e5669dfe0ce0125a1d..9796970e4fd17669a2015c3c60e3ea3516496303 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -68,8 +68,8 @@ class Pipeline < ApplicationRecord has_many :variables, class_name: 'Ci::PipelineVariable' has_many :deployments, through: :builds has_many :environments, -> { distinct }, through: :deployments - has_many :latest_builds, -> { latest }, foreign_key: :commit_id, inverse_of: :pipeline, class_name: 'Ci::Build' - has_many :downloadable_artifacts, -> { not_expired.downloadable }, through: :latest_builds, source: :job_artifacts + has_many :latest_builds, -> { latest.with_project_and_metadata }, foreign_key: :commit_id, inverse_of: :pipeline, class_name: 'Ci::Build' + has_many :downloadable_artifacts, -> { not_expired.downloadable.with_job }, through: :latest_builds, source: :job_artifacts has_many :messages, class_name: 'Ci::PipelineMessage', inverse_of: :pipeline diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index 7e69e1fdd88f5c2de455bdaa0091f25207319de3..65f2a70672b889ba72bfde3ed41ffec44e388a0d 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -37,6 +37,10 @@ class BuildPolicy < CommitStatusPolicy @subject.archived? end + condition(:artifacts_public, scope: :subject) do + @subject.artifacts_public? + end + condition(:terminal, scope: :subject) do @subject.has_terminal? end @@ -57,6 +61,10 @@ class BuildPolicy < CommitStatusPolicy can?(:update_build, @subject.project) end + condition(:project_developer) do + can?(:developer_access, @subject.project) + end + rule { project_read_build }.enable :read_build_trace rule { debug_mode & ~project_update_build }.prevent :read_build_trace @@ -94,6 +102,9 @@ class BuildPolicy < CommitStatusPolicy rule { ~can?(:build_service_proxy_enabled) }.policy do prevent :create_build_service_proxy end + + rule { project_read_build }.enable :read_job_artifacts + rule { ~artifacts_public & ~project_developer }.prevent :read_job_artifacts end end diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb index 917c416ce3316c3def0240d630cbbb506bdc40b4..2432a6a0e4d80b4c9dbf00be4f670298f02804db 100644 --- a/app/serializers/build_details_entity.rb +++ b/app/serializers/build_details_entity.rb @@ -26,7 +26,7 @@ class BuildDetailsEntity < JobEntity DeploymentClusterEntity.represent(build.deployment, options) end - expose :artifact, if: -> (*) { can?(current_user, :read_build, build) } do + expose :artifact, if: -> (*) { can?(current_user, :read_job_artifacts, build) } do expose :download_path, if: -> (*) { build.locked_artifacts? || build.artifacts? } do |build| download_project_job_artifacts_path(project, build) end diff --git a/app/serializers/pipeline_details_entity.rb b/app/serializers/pipeline_details_entity.rb index 50efa9ea15d57e057e4e01fee6f0fda1083dc255..e53fa7873ac14c18488e7bf1587a4d71de4302dc 100644 --- a/app/serializers/pipeline_details_entity.rb +++ b/app/serializers/pipeline_details_entity.rb @@ -11,6 +11,10 @@ class PipelineDetailsEntity < PipelineEntity expose :artifacts do |pipeline, options| rel = pipeline.downloadable_artifacts + if Feature.enabled?(:non_public_artifacts, type: :development) + rel = rel.select { |artifact| can?(request.current_user, :read_job_artifacts, artifact.job) } + end + BuildArtifactEntity.represent(rel, options) end expose :manual_actions, using: BuildActionEntity diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index 8b4411776bcc966e3033491d7c761f987cd46815..cee6eb1189f1809e2fc8134d60b8360f67e456ba 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -98,7 +98,7 @@ %td .gl-display-flex - - if can?(current_user, :read_build, job) && job.artifacts? + - if can?(current_user, :read_job_artifacts, job) && job.artifacts? = link_to download_project_job_artifacts_path(job.project, job), rel: 'nofollow', download: '', title: _('Download artifacts'), class: 'btn btn-build gl-button btn-icon btn-svg' do = sprite_icon('download') - if can?(current_user, :update_build, job) diff --git a/changelogs/unreleased/mattkasa-downloadable-artifacts.yml b/changelogs/unreleased/mattkasa-downloadable-artifacts.yml new file mode 100644 index 0000000000000000000000000000000000000000..db7912f0c3860f41fb5b14df1a7cfbdaace0f1eb --- /dev/null +++ b/changelogs/unreleased/mattkasa-downloadable-artifacts.yml @@ -0,0 +1,5 @@ +--- +title: Add artifacts:public boolean +merge_request: 49775 +author: +type: added diff --git a/config/feature_flags/development/non_public_artifacts.yml b/config/feature_flags/development/non_public_artifacts.yml new file mode 100644 index 0000000000000000000000000000000000000000..e2a2fd49df7ea7aa972c1c612d090f2e40179a26 --- /dev/null +++ b/config/feature_flags/development/non_public_artifacts.yml @@ -0,0 +1,8 @@ +--- +name: non_public_artifacts +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49775 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/294503 +milestone: '13.8' +type: development +group: group::configure +default_enabled: false diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 6fe25471289b9ce21a674924be6719bc61b217e9..030efe14d8b8ad5b071ee9ec64f02f1f98653294 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -275,6 +275,10 @@ def authorize_read_build_trace!(build) authorize! :read_build_trace, build end + def authorize_read_job_artifacts!(build) + authorize! :read_job_artifacts, build + end + def authorize_destroy_artifacts! authorize! :destroy_artifacts, user_project end diff --git a/lib/api/job_artifacts.rb b/lib/api/job_artifacts.rb index 1faa28d6f0782edfbf8062836cc51cfb288acd78..28737f61f61f4f0c855dabf839717065bbabf28c 100644 --- a/lib/api/job_artifacts.rb +++ b/lib/api/job_artifacts.rb @@ -32,6 +32,7 @@ def authorize_download_artifacts! authorize_download_artifacts! latest_build = user_project.latest_successful_build_for_ref!(params[:job], params[:ref_name]) + authorize_read_job_artifacts!(latest_build) present_carrierwave_file!(latest_build.artifacts_file) end @@ -50,6 +51,7 @@ def authorize_download_artifacts! authorize_download_artifacts! build = user_project.latest_successful_build_for_ref!(params[:job], params[:ref_name]) + authorize_read_job_artifacts!(build) path = Gitlab::Ci::Build::Artifacts::Path .new(params[:artifact_path]) @@ -70,6 +72,7 @@ def authorize_download_artifacts! authorize_download_artifacts! build = find_build!(params[:job_id]) + authorize_read_job_artifacts!(build) present_carrierwave_file!(build.artifacts_file) end @@ -82,9 +85,11 @@ def authorize_download_artifacts! requires :artifact_path, type: String, desc: 'Artifact path' end get ':id/jobs/:job_id/artifacts/*artifact_path', format: false do - authorize_read_builds! + authorize_download_artifacts! build = find_build!(params[:job_id]) + authorize_read_job_artifacts!(build) + not_found! unless build.artifacts? path = Gitlab::Ci::Build::Artifacts::Path diff --git a/lib/gitlab/ci/config/entry/artifacts.rb b/lib/gitlab/ci/config/entry/artifacts.rb index 206dbaea27251e9f4872583d6725374f9c2f6ede..6118ff499285a86ba35efd3f3f62e8f2ee35c260 100644 --- a/lib/gitlab/ci/config/entry/artifacts.rb +++ b/lib/gitlab/ci/config/entry/artifacts.rb @@ -12,7 +12,7 @@ class Artifacts < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Validatable include ::Gitlab::Config::Entry::Attributable - ALLOWED_KEYS = %i[name untracked paths reports when expire_in expose_as exclude].freeze + ALLOWED_KEYS = %i[name untracked paths reports when expire_in expose_as exclude public].freeze EXPOSE_AS_REGEX = /\A\w[-\w ]*\z/.freeze EXPOSE_AS_ERROR_MESSAGE = "can contain only letters, digits, '-', '_' and spaces" @@ -27,6 +27,7 @@ class Artifacts < ::Gitlab::Config::Entry::Node with_options allow_nil: true do validates :name, type: String + validates :public, boolean: true validates :untracked, boolean: true validates :paths, array_of_strings: true validates :paths, array_of_strings: { diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index c3d6e9d7569b59a311c95f31c78b0f53afb1bb9a..24abad6653064bfce0270ef26e9261632992fb7a 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -486,6 +486,14 @@ end end + trait :non_public_artifacts do + options do + { + artifacts: { public: false } + } + end + end + trait :non_playable do status { 'created' } self.when { 'manual' } diff --git a/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb b/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb index 028dcd3e1e663fd95328fc89d332f3758fe7c456..0e6d5b6c3116badec3847429ee309c5e05423074 100644 --- a/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb @@ -36,6 +36,14 @@ expect(entry.value).to eq config end end + + context "when value includes 'public' keyword" do + let(:config) { { paths: %w[results.txt], public: false } } + + it 'returns general artifact and report-type artifacts configuration' do + expect(entry.value).to eq config + end + end end context 'when entry value is not correct' do @@ -67,6 +75,15 @@ end end + context "when 'public' is not a boolean" do + let(:config) { { paths: %w[results.txt], public: 'false' } } + + it 'reports error' do + expect(entry.errors) + .to include 'artifacts public should be a boolean value' + end + end + context "when 'expose_as' is not a string" do let(:config) { { paths: %w[results.txt], expose_as: 1 } } diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 9f412d64d568732071e266a01c6000f1f0dc912e..7e52b897dbc4d5e21206e1fb73651d02b81cff2b 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -717,6 +717,22 @@ end end + describe '#artifacts_public?' do + subject { build.artifacts_public? } + + context 'artifacts with defaults' do + let(:build) { create(:ci_build, :artifacts) } + + it { is_expected.to be_truthy } + end + + context 'non public artifacts' do + let(:build) { create(:ci_build, :artifacts, :non_public_artifacts) } + + it { is_expected.to be_falsey } + end + end + describe '#artifacts_expired?' do subject { build.artifacts_expired? } diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index f8521818845c68090181a62bd75e37d8043debde..0ffc5acbe1fbd7eb12653f767dbeb152109081a5 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -260,6 +260,36 @@ def go end end + context 'when project is public with artifacts that are non public' do + let(:job) { create(:ci_build, :artifacts, :non_public_artifacts, pipeline: pipeline) } + + it 'rejects access to artifacts' do + project.update_column(:visibility_level, + Gitlab::VisibilityLevel::PUBLIC) + project.update_column(:public_builds, true) + + get_artifact_file(artifact) + + expect(response).to have_gitlab_http_status(:forbidden) + end + + context 'with the non_public_artifacts feature flag disabled' do + before do + stub_feature_flags(non_public_artifacts: false) + end + + it 'allows access to artifacts' do + project.update_column(:visibility_level, + Gitlab::VisibilityLevel::PUBLIC) + project.update_column(:public_builds, true) + + get_artifact_file(artifact) + + expect(response).to have_gitlab_http_status(:ok) + end + end + end + context 'when project is public with builds access disabled' do it 'rejects access to artifacts' do project.update_column(:visibility_level, @@ -396,6 +426,33 @@ def get_artifact_file(artifact_path) end end + context 'when public project guest and artifacts are non public' do + let(:api_user) { guest } + let(:job) { create(:ci_build, :artifacts, :non_public_artifacts, pipeline: pipeline) } + + before do + project.update_column(:visibility_level, + Gitlab::VisibilityLevel::PUBLIC) + project.update_column(:public_builds, true) + get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user) + end + + it 'rejects access and hides existence of artifacts' do + expect(response).to have_gitlab_http_status(:forbidden) + end + + context 'with the non_public_artifacts feature flag disabled' do + before do + stub_feature_flags(non_public_artifacts: false) + get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user) + end + + it 'allows access to artifacts' do + expect(response).to have_gitlab_http_status(:ok) + end + end + end + it 'does not return job artifacts if not uploaded' do get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user) @@ -580,6 +637,33 @@ def get_for_ref(ref = pipeline.ref, job_name = job.name) end end + context 'when project is public with non public artifacts' do + let(:job) { create(:ci_build, :artifacts, :non_public_artifacts, pipeline: pipeline, user: api_user) } + let(:visibility_level) { Gitlab::VisibilityLevel::PUBLIC } + let(:public_builds) { true } + + it 'rejects access and hides existence of artifacts', :sidekiq_might_not_need_inline do + get_artifact_file(artifact) + + expect(response).to have_gitlab_http_status(:forbidden) + expect(json_response).to have_key('message') + expect(response.headers.to_h) + .not_to include('Gitlab-Workhorse-Send-Data' => /artifacts-entry/) + end + + context 'with the non_public_artifacts feature flag disabled' do + before do + stub_feature_flags(non_public_artifacts: false) + end + + it 'allows access to artifacts', :sidekiq_might_not_need_inline do + get_artifact_file(artifact) + + expect(response).to have_gitlab_http_status(:ok) + end + end + end + context 'when project is private' do let(:visibility_level) { Gitlab::VisibilityLevel::PRIVATE } let(:public_builds) { true } diff --git a/spec/serializers/build_details_entity_spec.rb b/spec/serializers/build_details_entity_spec.rb index 5d29452e91c50e71123d6e501d9e0d44e4de9d5a..1adcf236b989e386613e50bd85a3d5a07184b803 100644 --- a/spec/serializers/build_details_entity_spec.rb +++ b/spec/serializers/build_details_entity_spec.rb @@ -225,5 +225,36 @@ expect(subject[:artifact].keys).to include(:download_path, :browse_path, :keep_path, :expire_at, :expired, :locked) end end + + context 'when the project is public and the user is a guest' do + let(:project) { create(:project, :repository, :public) } + let(:user) { create(:project_member, :guest, project: project).user } + + context 'when the build has public archive type artifacts' do + let(:build) { create(:ci_build, :artifacts) } + + it 'exposes public artifact details' do + expect(subject[:artifact].keys).to include(:download_path, :browse_path, :locked) + end + end + + context 'when the build has non public archive type artifacts' do + let(:build) { create(:ci_build, :artifacts, :non_public_artifacts, pipeline: pipeline) } + + it 'does not expose non public artifacts' do + expect(subject.keys).not_to include(:artifact) + end + + context 'with the non_public_artifacts feature flag disabled' do + before do + stub_feature_flags(non_public_artifacts: false) + end + + it 'exposes artifact details' do + expect(subject[:artifact].keys).to include(:download_path, :browse_path, :locked) + end + end + end + end end end diff --git a/spec/serializers/pipeline_details_entity_spec.rb b/spec/serializers/pipeline_details_entity_spec.rb index 1357836cb894b75be9b4aea7f318e3133b099c8d..d3474f333b91a87c14a2d3a36737e3d71fce19b1 100644 --- a/spec/serializers/pipeline_details_entity_spec.rb +++ b/spec/serializers/pipeline_details_entity_spec.rb @@ -183,5 +183,26 @@ expect(source_jobs[child_pipeline.id][:name]).to eq('child') end end + + context 'when a pipeline belongs to a public project' do + let(:project) { create(:project, :public) } + let(:pipeline) { create(:ci_empty_pipeline, status: :success, project: project) } + + context 'that has artifacts' do + let!(:build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) } + + it 'contains information about artifacts' do + expect(subject[:details][:artifacts].length).to eq(1) + end + end + + context 'that has non public artifacts' do + let!(:build) { create(:ci_build, :success, :artifacts, :non_public_artifacts, pipeline: pipeline) } + + it 'does not contain information about artifacts' do + expect(subject[:details][:artifacts].length).to eq(0) + end + end + end end end