Merge branch '10io-return-empty-string-for-md5-maven-upload-requests' into 'master'

Fix maven packages md5 uploads

See merge request gitlab-org/gitlab!45271

changelogs/unreleased/10io-return-empty-string-for-md5-maven-upload-requests.yml

+---
+title: Fix the maven md5 upload endpoint
+merge_request: 45271
+author:
+type: fixed

lib/api/maven_packages.rb

@@ -32,10 +32,10 @@ module API
       end
 
       def verify_package_file(package_file, uploaded_file)
-        stored_sha1 = Digest::SHA256.hexdigest(package_file.file_sha1)
-        expected_sha1 = uploaded_file.sha256
+        stored_sha256 = Digest::SHA256.hexdigest(package_file.file_sha1)
+        expected_sha256 = uploaded_file.sha256
 
-        if stored_sha1 == expected_sha1
+        if stored_sha256 == expected_sha256
           no_content!
         else
           conflict!
@@ -231,7 +231,7 @@ module API
 
           verify_package_file(package_file, params[:file])
         when 'md5'
-          nil
+          ''
         else
           track_package_event('push_package', :maven) if jar_file?(format)
 

spec/features/file_uploads/maven_package_spec.rb

@@ -25,5 +25,31 @@ RSpec.describe 'Upload a maven package', :api, :js do
     it { expect(subject.code).to eq(200) }
   end
 
+  RSpec.shared_examples 'for a maven sha1' do
+    let(:dummy_package) { double(Packages::Package) }
+    let(:api_path) { "/projects/#{project.id}/packages/maven/com/example/my-app/1.0/my-app-1.0-20180724.124855-1.jar.sha1" }
+
+    before do
+      # The sha verification done by the maven api is between:
+      # - the sha256 set by workhorse
+      # - the sha256 of the sha1 of the uploaded package file
+      # We're going to send `file` for the sha1 and stub the sha1 of the package file so that
+      # both sha256 being the same
+      expect(::Packages::PackageFileFinder).to receive(:new).and_return(double(execute!: dummy_package))
+      expect(dummy_package).to receive(:file_sha1).and_return(File.read(file.path))
+    end
+
+    it { expect(subject.code).to eq(204) }
+  end
+
+  RSpec.shared_examples 'for a maven md5' do
+    let(:api_path) { "/projects/#{project.id}/packages/maven/com/example/my-app/1.0/my-app-1.0-20180724.124855-1.jar.md5" }
+    let(:file) { StringIO.new('dummy_package') }
+
+    it { expect(subject.code).to eq(200) }
+  end
+
   it_behaves_like 'handling file uploads', 'for a maven package'
+  it_behaves_like 'handling file uploads', 'for a maven sha1'
+  it_behaves_like 'handling file uploads', 'for a maven md5'
 end

spec/requests/api/maven_packages_spec.rb

@@ -562,7 +562,7 @@ RSpec.describe API::MavenPackages do
             allow(uploaded_file).to receive(:size).and_return(project.actual_limits.maven_max_file_size + 1)
           end
 
-          upload_file_with_token(params)
+          upload_file_with_token(params: params)
 
           expect(response).to have_gitlab_http_status(:bad_request)
         end
@@ -577,19 +577,19 @@ RSpec.describe API::MavenPackages do
       context 'without workhorse header' do
         let(:workhorse_header) { {} }
 
-        subject { upload_file_with_token(params) }
+        subject { upload_file_with_token(params: params) }
 
         it_behaves_like 'package workhorse uploads'
       end
 
       context 'event tracking' do
-        subject { upload_file_with_token(params) }
+        subject { upload_file_with_token(params: params) }
 
         it_behaves_like 'a package tracking event', described_class.name, 'push_package'
       end
 
       it 'creates package and stores package file' do
-        expect { upload_file_with_token(params) }.to change { project.packages.count }.by(1)
+        expect { upload_file_with_token(params: params) }.to change { project.packages.count }.by(1)
           .and change { Packages::Maven::Metadatum.count }.by(1)
           .and change { Packages::PackageFile.count }.by(1)
 
@@ -598,7 +598,7 @@ RSpec.describe API::MavenPackages do
       end
 
       it 'allows upload with running job token' do
-        upload_file(params.merge(job_token: job.token))
+        upload_file(params: params.merge(job_token: job.token))
 
         expect(response).to have_gitlab_http_status(:ok)
         expect(project.reload.packages.last.build_info.pipeline).to eq job.pipeline
@@ -606,13 +606,13 @@ RSpec.describe API::MavenPackages do
 
       it 'rejects upload without running job token' do
         job.update!(status: :failed)
-        upload_file(params.merge(job_token: job.token))
+        upload_file(params: params.merge(job_token: job.token))
 
         expect(response).to have_gitlab_http_status(:unauthorized)
       end
 
       it 'allows upload with deploy token' do
-        upload_file(params, headers_with_deploy_token)
+        upload_file(params: params, request_headers: headers_with_deploy_token)
 
         expect(response).to have_gitlab_http_status(:ok)
       end
@@ -626,7 +626,10 @@ RSpec.describe API::MavenPackages do
         # We force the id of the deploy token and the user to be the same
         unauthorized_deploy_token.update!(id: another_user.id)
 
-        upload_file(params, headers.merge(Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => unauthorized_deploy_token.token))
+        upload_file(
+          params: params,
+          request_headers: headers.merge(Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => unauthorized_deploy_token.token)
+        )
 
         expect(response).to have_gitlab_http_status(:forbidden)
       end
@@ -635,16 +638,43 @@ RSpec.describe API::MavenPackages do
         let(:version) { '$%123' }
 
         it 'rejects request' do
-          expect { upload_file_with_token(params) }.not_to change { project.packages.count }
+          expect { upload_file_with_token(params: params) }.not_to change { project.packages.count }
 
           expect(response).to have_gitlab_http_status(:bad_request)
           expect(json_response['message']).to include('Validation failed')
         end
       end
+
+      context 'for sha1 file' do
+        let(:dummy_package) { double(Packages::Package) }
+
+        it 'checks the sha1' do
+          # The sha verification done by the maven api is between:
+          # - the sha256 set by workhorse helpers
+          # - the sha256 of the sha1 of the uploaded package file
+          # We're going to send `file_upload` for the sha1 and stub the sha1 of the package file so that
+          # both sha256 being the same
+          expect(::Packages::PackageFileFinder).to receive(:new).and_return(double(execute!: dummy_package))
+          expect(dummy_package).to receive(:file_sha1).and_return(File.read(file_upload.path))
+
+          upload_file_with_token(params: params, file_extension: 'jar.sha1')
+
+          expect(response).to have_gitlab_http_status(:no_content)
+        end
+      end
+
+      context 'for md5 file' do
+        it 'returns an empty body' do
+          upload_file_with_token(params: params, file_extension: 'jar.md5')
+
+          expect(response.body).to eq('')
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
     end
 
-    def upload_file(params = {}, request_headers = headers)
-      url = "/projects/#{project.id}/packages/maven/com/example/my-app/#{version}/my-app-1.0-20180724.124855-1.jar"
+    def upload_file(params: {}, request_headers: headers, file_extension: 'jar')
+      url = "/projects/#{project.id}/packages/maven/com/example/my-app/#{version}/my-app-1.0-20180724.124855-1.#{file_extension}"
       workhorse_finalize(
         api(url),
         method: :put,
@@ -655,8 +685,8 @@ RSpec.describe API::MavenPackages do
       )
     end
 
-    def upload_file_with_token(params = {}, request_headers = headers_with_token)
-      upload_file(params, request_headers)
+    def upload_file_with_token(params: {}, request_headers: headers_with_token, file_extension: 'jar')
+      upload_file(params: params, request_headers: request_headers, file_extension: file_extension)
     end
   end
 end