Merge branch 'ee-sh-fix-lfs-write-deploy-keys' into 'master'

[EE] Fix Error 500 when pushing LFS objects with a write deploy key

See merge request gitlab-org/gitlab-ee!3306

app/controllers/concerns/lfs_request.rb

@@ -74,9 +74,10 @@ module LfsRequest
 
   def lfs_upload_access?
     return false unless project.lfs_enabled?
+    return false unless has_authentication_ability?(:push_code)
     return false if project.above_size_limit? || objects_exceed_repo_limit?
 
-    has_authentication_ability?(:push_code) && can?(user, :push_code, project)
+    lfs_deploy_token? || can?(user, :push_code, project)
   end
 
   def lfs_deploy_token?

lib/gitlab/auth.rb

@@ -26,7 +26,7 @@ module Gitlab
         result =
           service_request_check(login, password, project) ||
           build_access_token_check(login, password) ||
-          lfs_token_check(login, password) ||
+          lfs_token_check(login, password, project) ||
           oauth_access_token_check(login, password) ||
           personal_access_token_check(password) ||
           user_with_password_for_git(login, password) ||
@@ -147,7 +147,7 @@ module Gitlab
         end.flatten.uniq
       end
 
-      def lfs_token_check(login, password)
+      def lfs_token_check(login, password, project)
         deploy_key_matches = login.match(/\Alfs\+deploy-key-(\d+)\z/)
 
         actor =
@@ -164,6 +164,8 @@ module Gitlab
         authentication_abilities =
           if token_handler.user?
             full_authentication_abilities
+          elsif token_handler.deploy_key_pushable?(project)
+            read_write_authentication_abilities
           else
             read_authentication_abilities
           end
@@ -209,10 +211,15 @@ module Gitlab
         ]
       end
 
-      def full_authentication_abilities
+      def read_write_authentication_abilities
         read_authentication_abilities + [
           :push_code,
-          :create_container_image,
+          :create_container_image
+        ]
+      end
+
+      def full_authentication_abilities
+        read_write_authentication_abilities + [
           :admin_container_image
         ]
       end

lib/gitlab/lfs_token.rb

@@ -27,6 +27,10 @@ module Gitlab
       end
     end
 
+    def deploy_key_pushable?(project)
+      actor.is_a?(DeployKey) && actor.can_push_to?(project)
+    end
+
     def user?
       actor.is_a?(User)
     end

spec/lib/gitlab/auth_spec.rb

@@ -133,6 +133,25 @@ describe Gitlab::Auth do
 
         gl_auth.find_for_git_client(user.username, token, project: nil, ip: 'ip')
       end
+
+      it 'grants deploy key write permissions' do
+        project = create(:project)
+        key = create(:deploy_key, can_push: true)
+        create(:deploy_keys_project, deploy_key: key, project: project)
+        token = Gitlab::LfsToken.new(key).token
+
+        expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: "lfs+deploy-key-#{key.id}")
+        expect(gl_auth.find_for_git_client("lfs+deploy-key-#{key.id}", token, project: project, ip: 'ip')).to eq(Gitlab::Auth::Result.new(key, nil, :lfs_deploy_token, read_write_authentication_abilities))
+      end
+
+      it 'does not grant deploy key write permissions' do
+        project = create(:project)
+        key = create(:deploy_key, can_push: true)
+        token = Gitlab::LfsToken.new(key).token
+
+        expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: "lfs+deploy-key-#{key.id}")
+        expect(gl_auth.find_for_git_client("lfs+deploy-key-#{key.id}", token, project: project, ip: 'ip')).to eq(Gitlab::Auth::Result.new(key, nil, :lfs_deploy_token, read_authentication_abilities))
+      end
     end
 
     context 'while using OAuth tokens as passwords' do
@@ -326,10 +345,15 @@ describe Gitlab::Auth do
     ]
   end
 
-  def full_authentication_abilities
+  def read_write_authentication_abilities
     read_authentication_abilities + [
       :push_code,
-      :create_container_image,
+      :create_container_image
+    ]
+  end
+
+  def full_authentication_abilities
+    read_write_authentication_abilities + [
       :admin_container_image
     ]
   end

spec/requests/lfs_http_spec.rb

@@ -671,54 +671,12 @@ describe 'Git LFS API and storage' do
         }
       end
 
-      describe 'when request is authenticated' do
-        describe 'when user has project push access' do
-          let(:authorization) { authorize_user }
-
-          let(:update_user_permissions) do
-            project.team << [user, :developer]
-          end
-
-          context 'when pushing an lfs object that already exists' do
-            let(:other_project) { create(:project) }
-            let(:update_lfs_permissions) do
-              other_project.lfs_objects << lfs_object
-            end
-
-            it 'responds with status 200' do
-              expect(response).to have_gitlab_http_status(200)
-            end
-
-            it 'responds with links the object to the project' do
-              expect(json_response['objects']).to be_kind_of(Array)
-              expect(json_response['objects'].first['oid']).to eq(sample_oid)
-              expect(json_response['objects'].first['size']).to eq(sample_size)
-              expect(lfs_object.projects.pluck(:id)).not_to include(project.id)
-              expect(lfs_object.projects.pluck(:id)).to include(other_project.id)
-              expect(json_response['objects'].first['actions']['upload']['href']).to eq("#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}")
-              expect(json_response['objects'].first['actions']['upload']['header']).to eq('Authorization' => authorization)
-            end
-          end
-
-          context 'when pushing a lfs object that does not exist' do
+      shared_examples 'pushes new LFS objects' do
         let(:sample_size) { 150.megabytes }
         let(:sample_oid) { '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897' }
 
-            let(:body) do
-              {
-                'operation' => 'upload',
-                'objects' => [
-                  { 'oid' => sample_oid,
-                    'size' => sample_size }
-                ]
-              }
-            end
-
-            it 'responds with status 200' do
-              expect(response).to have_gitlab_http_status(200)
-            end
-
         it 'responds with upload hypermedia link' do
+          expect(response).to have_gitlab_http_status(200)
           expect(json_response['objects']).to be_kind_of(Array)
           expect(json_response['objects'].first['oid']).to eq(sample_oid)
           expect(json_response['objects'].first['size']).to eq(sample_size)
@@ -726,6 +684,7 @@ describe 'Git LFS API and storage' do
           expect(json_response['objects'].first['actions']['upload']['header']).to eq('Authorization' => authorization)
         end
 
+        ## EE-specific context
         context 'and project is above the limit' do
           let(:update_lfs_permissions) do
             allow_any_instance_of(EE::Project).to receive_messages(
@@ -735,9 +694,6 @@ describe 'Git LFS API and storage' do
 
           it 'responds with status 406' do
             expect(response).to have_gitlab_http_status(406)
-              end
-
-              it 'show correct error message' do
             expect(json_response['message']).to eql('Your push has been rejected, because this repository has exceeded its size limit of 99 MB by 1 MB. Please contact your GitLab administrator for more information.')
           end
         end
@@ -752,12 +708,42 @@ describe 'Git LFS API and storage' do
           it 'responds with status 406' do
             expect(response).to have_gitlab_http_status(406)
             expect(json_response['documentation_url']).to include('/help')
+            expect(json_response['message']).to eql('Your push has been rejected, because this repository has exceeded its size limit of 300 MB by 50 MB. Please contact your GitLab administrator for more information.')
+          end
+        end
       end
 
-              it 'show correct error message' do
-                expect(json_response['message']).to eql('Your push has been rejected, because this repository has exceeded its size limit of 300 MB by 50 MB. Please contact your GitLab administrator for more information.')
+      describe 'when request is authenticated' do
+        describe 'when user has project push access' do
+          let(:authorization) { authorize_user }
+
+          let(:update_user_permissions) do
+            project.team << [user, :developer]
           end
+
+          context 'when pushing an lfs object that already exists' do
+            let(:other_project) { create(:project) }
+            let(:update_lfs_permissions) do
+              other_project.lfs_objects << lfs_object
             end
+
+            it 'responds with status 200' do
+              expect(response).to have_gitlab_http_status(200)
+            end
+
+            it 'responds with links the object to the project' do
+              expect(json_response['objects']).to be_kind_of(Array)
+              expect(json_response['objects'].first['oid']).to eq(sample_oid)
+              expect(json_response['objects'].first['size']).to eq(sample_size)
+              expect(lfs_object.projects.pluck(:id)).not_to include(project.id)
+              expect(lfs_object.projects.pluck(:id)).to include(other_project.id)
+              expect(json_response['objects'].first['actions']['upload']['href']).to eq("#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}")
+              expect(json_response['objects'].first['actions']['upload']['header']).to eq('Authorization' => authorization)
+            end
+          end
+
+          context 'when pushing a lfs object that does not exist' do
+            it_behaves_like 'pushes new LFS objects'
           end
 
           context 'when pushing one new and one existing lfs object' do
@@ -838,6 +824,17 @@ describe 'Git LFS API and storage' do
             end
           end
         end
+
+        context 'when deploy key has project push access' do
+          let(:key) { create(:deploy_key, can_push: true) }
+          let(:authorization) { authorize_deploy_key }
+
+          let(:update_user_permissions) do
+            project.deploy_keys << key
+          end
+
+          it_behaves_like 'pushes new LFS objects'
+        end
       end
 
       context 'when user is not authenticated' do