From 0232450c8aee08a656275caf7b990e0bcbbb1cf4 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Wed, 8 Nov 2017 16:21:39 +0000 Subject: Fix Error 500 when pushing LFS objects with a write deploy key --- app/controllers/concerns/lfs_request.rb | 3 ++- lib/gitlab/auth.rb | 15 ++++++++--- lib/gitlab/lfs_token.rb | 4 +++ spec/lib/gitlab/auth_spec.rb | 28 ++++++++++++++++++-- spec/requests/lfs_http_spec.rb | 47 ++++++++++++++++++--------------- 5 files changed, 69 insertions(+), 28 deletions(-) diff --git a/app/controllers/concerns/lfs_request.rb b/app/controllers/concerns/lfs_request.rb index 738afd612f0..6cca9f95618 100644 --- a/app/controllers/concerns/lfs_request.rb +++ b/app/controllers/concerns/lfs_request.rb @@ -74,8 +74,9 @@ module LfsRequest def lfs_upload_access? return false unless project.lfs_enabled? + return false unless has_authentication_ability?(:push_code) - has_authentication_ability?(:push_code) && can?(user, :push_code, project) + lfs_deploy_token? || can?(user, :push_code, project) end def lfs_deploy_token? diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 0ad9285c0ea..cbbc51db99e 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -25,7 +25,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) || @@ -146,7 +146,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 = @@ -163,6 +163,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 @@ -208,10 +210,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 diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb index 8e57ba831c5..ead5d566871 100644 --- a/lib/gitlab/lfs_token.rb +++ b/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 diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index 54a853c9ce3..3164d2ebf04 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/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 diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index 52e93e157f1..c597623bc4d 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -654,6 +654,20 @@ describe 'Git LFS API and storage' do } end + shared_examples 'pushes new LFS objects' do + let(:sample_size) { 150.megabytes } + let(:sample_oid) { '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897' } + + 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) + expect(json_response['objects'].first['actions']['upload']['href']).to eq("#{Gitlab.config.gitlab.url}/#{project.full_path}.git/gitlab-lfs/objects/#{sample_oid}/#{sample_size}") + expect(json_response['objects'].first['actions']['upload']['header']).to eq('Authorization' => authorization) + end + end + describe 'when request is authenticated' do describe 'when user has project push access' do let(:authorization) { authorize_user } @@ -684,27 +698,7 @@ describe 'Git LFS API and storage' do end context 'when pushing a lfs object that does not exist' do - let(:body) do - { - 'operation' => 'upload', - 'objects' => [ - { 'oid' => '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897', - 'size' => 1575078 } - ] - } - end - - it 'responds with status 200' do - expect(response).to have_gitlab_http_status(200) - end - - it 'responds with upload hypermedia link' do - expect(json_response['objects']).to be_kind_of(Array) - expect(json_response['objects'].first['oid']).to eq("91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897") - expect(json_response['objects'].first['size']).to eq(1575078) - expect(json_response['objects'].first['actions']['upload']['href']).to eq("#{Gitlab.config.gitlab.url}/#{project.full_path}.git/gitlab-lfs/objects/91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897/1575078") - expect(json_response['objects'].first['actions']['upload']['header']).to eq('Authorization' => authorization) - end + it_behaves_like 'pushes new LFS objects' end context 'when pushing one new and one existing lfs object' do @@ -785,6 +779,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 -- cgit v1.2.1