Merge branch 'secure-file-project-deletion' into 'master'

Remove Secure Files when a project is destroyed

See merge request gitlab-org/gitlab!82443

app/controllers/projects/ci/secure_files_controller.rb

 # frozen_string_literal: true
 
 class Projects::Ci::SecureFilesController < Projects::ApplicationController
-  before_action :check_can_collaborate!
+  before_action :authorize_read_secure_files!
 
   feature_category :pipeline_authoring
 
   def show
   end
-
-  private
-
-  def check_can_collaborate!
-    render_404 unless can_collaborate_with_project?(project)
-  end
 end

app/policies/project_policy.rb

@@ -413,6 +413,7 @@ class ProjectPolicy < BasePolicy
     enable :admin_feature_flag
     enable :admin_feature_flags_user_lists
     enable :update_escalation_status
+    enable :read_secure_files
   end
 
   rule { can?(:developer_access) & user_confirmed? }.policy do
@@ -462,6 +463,7 @@ class ProjectPolicy < BasePolicy
     enable :register_project_runners
     enable :update_runners_registration_token
     enable :admin_project_google_cloud
+    enable :admin_secure_files
   end
 
   rule { public_project & metrics_dashboard_allowed }.policy do

app/services/ci/destroy_secure_file_service.rb

+# frozen_string_literal: true
+
+module Ci
+  class DestroySecureFileService < BaseService
+    def execute(secure_file)
+      raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_secure_files, secure_file.project)
+
+      secure_file.destroy!
+    end
+  end
+end

app/services/projects/destroy_service.rb

@@ -200,6 +200,10 @@ module Projects
         ::Ci::DestroyPipelineService.new(project, current_user).execute(pipeline)
       end
 
+      project.secure_files.find_each(batch_size: BATCH_SIZE) do |secure_file| # rubocop: disable CodeReuse/ActiveRecord
+        ::Ci::DestroySecureFileService.new(project, current_user).execute(secure_file)
+      end
+
       deleted_count = ::CommitStatus.for_project(project).delete_all
 
       Gitlab::AppLogger.info(

lib/api/ci/secure_files.rb

@@ -7,8 +7,8 @@ module API
 
       before do
         authenticate!
-        authorize! :admin_build, user_project
         feature_flag_enabled?
+        authorize! :read_secure_files, user_project
       end
 
       feature_category :pipeline_authoring
@@ -52,13 +52,17 @@ module API
           body secure_file.file.read
         end
 
+        resource do
+          before do
+            authorize! :admin_secure_files, user_project
+          end
+
           desc 'Upload a Secure File'
           params do
             requires :name, type: String, desc: 'The name of the file'
             requires :file, types: [Rack::Multipart::UploadedFile, ::API::Validations::Types::WorkhorseFile], desc: 'The secure file to be uploaded'
             optional :permissions, type: String, desc: 'The file permissions', default: 'read_only', values: %w[read_only read_write execute]
           end
-
           route_setting :authentication, basic_auth_personal_access_token: true, job_token_allowed: true
           post ':id/secure_files' do
             secure_file = user_project.secure_files.new(
@@ -82,11 +86,12 @@ module API
           delete ':id/secure_files/:secure_file_id' do
             secure_file = user_project.secure_files.find(params[:secure_file_id])
 
-          secure_file.destroy!
+            ::Ci::DestroySecureFileService.new(user_project, current_user).execute(secure_file)
 
             no_content!
           end
         end
+      end
 
       helpers do
         def feature_flag_enabled?

spec/requests/api/ci/secure_files_spec.rb

@@ -8,49 +8,72 @@ RSpec.describe API::Ci::SecureFiles do
     stub_feature_flags(ci_secure_files: true)
   end
 
-  let_it_be(:user) { create(:user) }
-  let_it_be(:user2) { create(:user) }
-  let_it_be(:project) { create(:project, creator_id: user.id) }
-  let_it_be(:maintainer) { create(:project_member, :maintainer, user: user, project: project) }
-  let_it_be(:developer) { create(:project_member, :developer, user: user2, project: project) }
+  let_it_be(:maintainer) { create(:user) }
+  let_it_be(:developer) { create(:user) }
+  let_it_be(:guest) { create(:user) }
+  let_it_be(:anonymous) { create(:user) }
+  let_it_be(:project) { create(:project, creator_id: maintainer.id) }
   let_it_be(:secure_file) { create(:ci_secure_file, project: project) }
 
+  before_all do
+    project.add_maintainer(maintainer)
+    project.add_developer(developer)
+    project.add_guest(guest)
+  end
+
   describe 'GET /projects/:id/secure_files' do
     context 'feature flag' do
       it 'returns a 503 when the feature flag is disabled' do
         stub_feature_flags(ci_secure_files: false)
 
-        get api("/projects/#{project.id}/secure_files", user)
+        get api("/projects/#{project.id}/secure_files", maintainer)
 
         expect(response).to have_gitlab_http_status(:service_unavailable)
       end
 
       it 'returns a 200 when the feature flag is enabled' do
-        get api("/projects/#{project.id}/secure_files", user)
+        get api("/projects/#{project.id}/secure_files", maintainer)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response).to be_a(Array)
+      end
+    end
+
+    context 'authenticated user with admin permissions' do
+      it 'returns project secure files' do
+        get api("/projects/#{project.id}/secure_files", maintainer)
 
         expect(response).to have_gitlab_http_status(:ok)
         expect(json_response).to be_a(Array)
       end
     end
 
-    context 'authorized user with proper permissions' do
+    context 'authenticated user with read permissions' do
       it 'returns project secure files' do
-        get api("/projects/#{project.id}/secure_files", user)
+        get api("/projects/#{project.id}/secure_files", developer)
 
         expect(response).to have_gitlab_http_status(:ok)
         expect(json_response).to be_a(Array)
       end
     end
 
-    context 'authorized user with invalid permissions' do
+    context 'authenticated user with guest permissions' do
       it 'does not return project secure files' do
-        get api("/projects/#{project.id}/secure_files", user2)
+        get api("/projects/#{project.id}/secure_files", guest)
 
         expect(response).to have_gitlab_http_status(:forbidden)
       end
     end
 
-    context 'unauthorized user' do
+    context 'authenticated user with no permissions' do
+      it 'does not return project secure files' do
+        get api("/projects/#{project.id}/secure_files", anonymous)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'unauthenticated user' do
       it 'does not return project secure files' do
         get api("/projects/#{project.id}/secure_files")
 
@@ -60,9 +83,9 @@ RSpec.describe API::Ci::SecureFiles do
   end
 
   describe 'GET /projects/:id/secure_files/:secure_file_id' do
-    context 'authorized user with proper permissions' do
+    context 'authenticated user with admin permissions' do
       it 'returns project secure file details' do
-        get api("/projects/#{project.id}/secure_files/#{secure_file.id}", user)
+        get api("/projects/#{project.id}/secure_files/#{secure_file.id}", maintainer)
 
         expect(response).to have_gitlab_http_status(:ok)
         expect(json_response['name']).to eq(secure_file.name)
@@ -70,21 +93,31 @@ RSpec.describe API::Ci::SecureFiles do
       end
 
       it 'responds with 404 Not Found if requesting non-existing secure file' do
-        get api("/projects/#{project.id}/secure_files/99999", user)
+        get api("/projects/#{project.id}/secure_files/#{non_existing_record_id}", maintainer)
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
-    context 'authorized user with invalid permissions' do
+    context 'authenticated user with read permissions' do
+      it 'returns project secure file details' do
+        get api("/projects/#{project.id}/secure_files/#{secure_file.id}", developer)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['name']).to eq(secure_file.name)
+        expect(json_response['permissions']).to eq(secure_file.permissions)
+      end
+    end
+
+    context 'authenticated user with no permissions' do
       it 'does not return project secure file details' do
-        get api("/projects/#{project.id}/secure_files/#{secure_file.id}", user2)
+        get api("/projects/#{project.id}/secure_files/#{secure_file.id}", anonymous)
 
-        expect(response).to have_gitlab_http_status(:forbidden)
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
-    context 'unauthorized user' do
+    context 'unauthenticated user' do
       it 'does not return project secure file details' do
         get api("/projects/#{project.id}/secure_files/#{secure_file.id}")
 
@@ -94,34 +127,47 @@ RSpec.describe API::Ci::SecureFiles do
   end
 
   describe 'GET /projects/:id/secure_files/:secure_file_id/download' do
-    context 'authorized user with proper permissions' do
+    context 'authenticated user with admin permissions' do
       it 'returns a secure file' do
         sample_file = fixture_file('ci_secure_files/upload-keystore.jks')
         secure_file.file = CarrierWaveStringFile.new(sample_file)
         secure_file.save!
 
-        get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", user)
+        get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", maintainer)
 
         expect(response).to have_gitlab_http_status(:ok)
         expect(Base64.encode64(response.body)).to eq(Base64.encode64(sample_file))
       end
 
       it 'responds with 404 Not Found if requesting non-existing secure file' do
-        get api("/projects/#{project.id}/secure_files/99999/download", user)
+        get api("/projects/#{project.id}/secure_files/#{non_existing_record_id}/download", maintainer)
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
-    context 'authorized user with invalid permissions' do
+    context 'authenticated user with read permissions' do
+      it 'returns a secure file' do
+        sample_file = fixture_file('ci_secure_files/upload-keystore.jks')
+        secure_file.file = CarrierWaveStringFile.new(sample_file)
+        secure_file.save!
+
+        get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", developer)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(Base64.encode64(response.body)).to eq(Base64.encode64(sample_file))
+      end
+    end
+
+    context 'authenticated user with no permissions' do
       it 'does not return project secure file details' do
-        get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", user2)
+        get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", anonymous)
 
-        expect(response).to have_gitlab_http_status(:forbidden)
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
-    context 'unauthorized user' do
+    context 'unauthenticated user' do
       it 'does not return project secure file details' do
         get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download")
 
@@ -131,7 +177,7 @@ RSpec.describe API::Ci::SecureFiles do
   end
 
   describe 'POST /projects/:id/secure_files' do
-    context 'authorized user with proper permissions' do
+    context 'authenticated user with admin permissions' do
       it 'creates a secure file' do
         params = {
           file: fixture_file_upload('spec/fixtures/ci_secure_files/upload-keystore.jks'),
@@ -140,7 +186,7 @@ RSpec.describe API::Ci::SecureFiles do
         }
 
         expect do
-          post api("/projects/#{project.id}/secure_files", user), params: params
+          post api("/projects/#{project.id}/secure_files", maintainer), params: params
         end.to change {project.secure_files.count}.by(1)
 
         expect(response).to have_gitlab_http_status(:created)
@@ -164,7 +210,7 @@ RSpec.describe API::Ci::SecureFiles do
         }
 
         expect do
-          post api("/projects/#{project.id}/secure_files", user), params: params
+          post api("/projects/#{project.id}/secure_files", maintainer), params: params
         end.to change {project.secure_files.count}.by(1)
 
         expect(json_response['permissions']).to eq('read_only')
@@ -177,11 +223,11 @@ RSpec.describe API::Ci::SecureFiles do
           permissions: 'read_write'
         }
 
-        post api("/projects/#{project.id}/secure_files", user), params: post_params
+        post api("/projects/#{project.id}/secure_files", maintainer), params: post_params
 
         secure_file_id = json_response['id']
 
-        get api("/projects/#{project.id}/secure_files/#{secure_file_id}/download", user)
+        get api("/projects/#{project.id}/secure_files/#{secure_file_id}/download", maintainer)
 
         expect(Base64.encode64(response.body)).to eq(Base64.encode64(fixture_file_upload('spec/fixtures/ci_secure_files/upload-keystore.jks').read))
       end
@@ -189,7 +235,9 @@ RSpec.describe API::Ci::SecureFiles do
       it 'returns an error when the file checksum fails to validate' do
         secure_file.update!(checksum: 'foo')
 
-        get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", user)
+        expect do
+          get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", maintainer)
+        end.not_to change { project.secure_files.count }
 
         expect(response.code).to eq("500")
       end
@@ -199,7 +247,9 @@ RSpec.describe API::Ci::SecureFiles do
           name: 'upload-keystore.jks'
         }
 
-        post api("/projects/#{project.id}/secure_files", user), params: post_params
+        expect do
+          post api("/projects/#{project.id}/secure_files", maintainer), params: post_params
+        end.not_to change { project.secure_files.count }
 
         expect(response).to have_gitlab_http_status(:bad_request)
         expect(json_response['error']).to eq('file is missing')
@@ -210,7 +260,9 @@ RSpec.describe API::Ci::SecureFiles do
           file: fixture_file_upload('spec/fixtures/ci_secure_files/upload-keystore.jks')
         }
 
-        post api("/projects/#{project.id}/secure_files", user), params: post_params
+        expect do
+          post api("/projects/#{project.id}/secure_files", maintainer), params: post_params
+        end.not_to change { project.secure_files.count }
 
         expect(response).to have_gitlab_http_status(:bad_request)
         expect(json_response['error']).to eq('name is missing')
@@ -223,7 +275,9 @@ RSpec.describe API::Ci::SecureFiles do
           permissions: 'foo'
         }
 
-        post api("/projects/#{project.id}/secure_files", user), params: post_params
+        expect do
+          post api("/projects/#{project.id}/secure_files", maintainer), params: post_params
+        end.not_to change { project.secure_files.count }
 
         expect(response).to have_gitlab_http_status(:bad_request)
         expect(json_response['error']).to eq('permissions does not have a valid value')
@@ -241,7 +295,9 @@ RSpec.describe API::Ci::SecureFiles do
           name: 'upload-keystore.jks'
         }
 
-        post api("/projects/#{project.id}/secure_files", user), params: post_params
+        expect do
+          post api("/projects/#{project.id}/secure_files", maintainer), params: post_params
+        end.not_to change { project.secure_files.count }
 
         expect(response).to have_gitlab_http_status(:bad_request)
       end
@@ -256,23 +312,39 @@ RSpec.describe API::Ci::SecureFiles do
           name: 'upload-keystore.jks'
         }
 
-        post api("/projects/#{project.id}/secure_files", user), params: post_params
+        expect do
+          post api("/projects/#{project.id}/secure_files", maintainer), params: post_params
+        end.not_to change { project.secure_files.count }
 
         expect(response).to have_gitlab_http_status(:payload_too_large)
       end
     end
 
-    context 'authorized user with invalid permissions' do
+    context 'authenticated user with read permissions' do
       it 'does not create a secure file' do
-        post api("/projects/#{project.id}/secure_files", user2)
+        expect do
+          post api("/projects/#{project.id}/secure_files", developer)
+        end.not_to change { project.secure_files.count }
 
         expect(response).to have_gitlab_http_status(:forbidden)
       end
     end
 
-    context 'unauthorized user' do
+    context 'authenticated user with no permissions' do
       it 'does not create a secure file' do
+        expect do
+          post api("/projects/#{project.id}/secure_files", anonymous)
+        end.not_to change { project.secure_files.count }
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'unauthenticated user' do
+      it 'does not create a secure file' do
+        expect do
           post api("/projects/#{project.id}/secure_files")
+        end.not_to change { project.secure_files.count }
 
         expect(response).to have_gitlab_http_status(:unauthorized)
       end
@@ -280,33 +352,49 @@ RSpec.describe API::Ci::SecureFiles do
   end
 
   describe 'DELETE /projects/:id/secure_files/:secure_file_id' do
-    context 'authorized user with proper permissions' do
+    context 'authenticated user with admin permissions' do
       it 'deletes the secure file' do
         expect do
-          delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", user)
+          delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", maintainer)
 
           expect(response).to have_gitlab_http_status(:no_content)
-        end.to change {project.secure_files.count}.by(-1)
+        end.to change { project.secure_files.count }
       end
 
       it 'responds with 404 Not Found if requesting non-existing secure_file' do
-        delete api("/projects/#{project.id}/secure_files/99999", user)
+        expect do
+          delete api("/projects/#{project.id}/secure_files/#{non_existing_record_id}", maintainer)
+        end.not_to change { project.secure_files.count }
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
-    context 'authorized user with invalid permissions' do
+    context 'authenticated user with read permissions' do
       it 'does not delete the secure_file' do
-        delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", user2)
+        expect do
+          delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", developer)
+        end.not_to change { project.secure_files.count }
 
         expect(response).to have_gitlab_http_status(:forbidden)
       end
     end
 
-    context 'unauthorized user' do
+    context 'authenticated user with no permissions' do
       it 'does not delete the secure_file' do
+        expect do
+          delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", anonymous)
+        end.not_to change { project.secure_files.count }
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'unauthenticated user' do
+      it 'does not delete the secure_file' do
+        expect do
           delete api("/projects/#{project.id}/secure_files/#{secure_file.id}")
+        end.not_to change { project.secure_files.count }
 
         expect(response).to have_gitlab_http_status(:unauthorized)
       end

spec/services/ci/destroy_secure_file_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Ci::DestroySecureFileService do
+  let_it_be(:maintainer_user) { create(:user) }
+  let_it_be(:developer_user) { create(:user) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:secure_file) { create(:ci_secure_file, project: project) }
+  let_it_be(:project_member) { create(:project_member, :maintainer, user: maintainer_user, project: project) }
+  let_it_be(:project_member2) { create(:project_member, :developer, user: developer_user, project: project) }
+
+  subject { described_class.new(project, user).execute(secure_file) }
+
+  context 'user is a maintainer' do
+    let(:user) { maintainer_user }
+
+    it 'destroys the secure file' do
+      subject
+
+      expect { secure_file.reload }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
+
+  context 'user is a developer' do
+    let(:user) { developer_user }
+
+    it 'raises an exception' do
+      expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
+    end
+  end
+end

spec/services/projects/destroy_service_spec.rb

@@ -43,6 +43,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi
       let!(:report_result) { create(:ci_build_report_result, build: build) }
       let!(:pending_state) { create(:ci_build_pending_state, build: build) }
       let!(:pipeline_artifact) { create(:ci_pipeline_artifact, pipeline: pipeline) }
+      let!(:secure_file) { create(:ci_secure_file, project: project) }
 
       it 'deletes build and pipeline related records' do
         expect { destroy_project(project, user, {}) }
@@ -56,6 +57,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi
           .and change { Ci::BuildReportResult.count }.by(-1)
           .and change { Ci::BuildRunnerSession.count }.by(-1)
           .and change { Ci::Pipeline.count }.by(-1)
+          .and change { Ci::SecureFile.count }.by(-1)
       end
 
       it 'avoids N+1 queries' do