diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 71939f070cb..fcbe0b350a3 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -146,6 +146,12 @@ module Ci .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 @@ module Ci 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 c80d50ea131..d34c85ed17b 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -133,6 +133,12 @@ module Ci 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 5e5f51d776f..9796970e4fd 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -68,8 +68,8 @@ module Ci 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 7e69e1fdd88..65f2a70672b 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -37,6 +37,10 @@ module Ci @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 @@ module Ci 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 @@ module Ci 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 917c416ce33..2432a6a0e4d 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 50efa9ea15d..e53fa7873ac 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 8b4411776bc..cee6eb1189f 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 00000000000..db7912f0c38 --- /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 00000000000..e2a2fd49df7 --- /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 6fe25471289..030efe14d8b 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -275,6 +275,10 @@ module API 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 1faa28d6f07..28737f61f61 100644 --- a/lib/api/job_artifacts.rb +++ b/lib/api/job_artifacts.rb @@ -32,6 +32,7 @@ module API 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 @@ module API 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 @@ module API 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 @@ module API 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 206dbaea272..6118ff49928 100644 --- a/lib/gitlab/ci/config/entry/artifacts.rb +++ b/lib/gitlab/ci/config/entry/artifacts.rb @@ -12,7 +12,7 @@ module Gitlab 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 @@ module Gitlab 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 c3d6e9d7569..24abad66530 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -486,6 +486,14 @@ FactoryBot.define do 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 028dcd3e1e6..0e6d5b6c311 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 @@ RSpec.describe Gitlab::Ci::Config::Entry::Artifacts do 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 @@ RSpec.describe Gitlab::Ci::Config::Entry::Artifacts do 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 9f412d64d56..7e52b897dbc 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -717,6 +717,22 @@ RSpec.describe Ci::Build do 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 f8521818845..0ffc5acbe1f 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -260,6 +260,36 @@ RSpec.describe API::Jobs do 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 @@ RSpec.describe API::Jobs do 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 @@ RSpec.describe API::Jobs do 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 5d29452e91c..1adcf236b98 100644 --- a/spec/serializers/build_details_entity_spec.rb +++ b/spec/serializers/build_details_entity_spec.rb @@ -225,5 +225,36 @@ RSpec.describe BuildDetailsEntity do 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 1357836cb89..d3474f333b9 100644 --- a/spec/serializers/pipeline_details_entity_spec.rb +++ b/spec/serializers/pipeline_details_entity_spec.rb @@ -183,5 +183,26 @@ RSpec.describe PipelineDetailsEntity do 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