Merge branch 'mattkasa-downloadable-artifacts' into 'master'

Add artifacts:public boolean

See merge request gitlab-org/gitlab!49775

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(

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
 

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
 

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
 

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

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

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)

changelogs/unreleased/mattkasa-downloadable-artifacts.yml

+---
+title: Add artifacts:public boolean
+merge_request: 49775
+author:
+type: added

config/feature_flags/development/non_public_artifacts.yml

+---
+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

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

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

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: {

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' }

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 } }
 

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? }
 

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 }

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

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