diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 516f25db15b..0990e2a1fba 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -6,8 +6,17 @@ module API helpers ::API::Helpers::InternalHelpers helpers ::Gitlab::Identifier + UNKNOWN_CHECK_RESULT_ERROR = 'Unknown check result'.freeze + + helpers do + def response_with_status(code: 200, success: true, message: nil, **extra_options) + status code + { status: success, message: message }.merge(extra_options).compact + end + end + namespace 'internal' do - # Check if git command is allowed to project + # Check if git command is allowed for project # # Params: # key_id - ssh key id for Git over SSH @@ -18,8 +27,6 @@ module API # action - git action (git-upload-pack or git-receive-pack) # changes - changes as "oldrev newrev ref", see Gitlab::ChangesList post "/allowed" do - status 200 - # Stores some Git-specific env thread-safely env = parse_env Gitlab::Git::HookEnv.set(gl_repository, env) if project @@ -49,27 +56,37 @@ module API namespace_path: namespace_path, project_path: project_path, redirected_path: redirected_path) - begin - access_checker.check(params[:action], params[:changes]) - @project ||= access_checker.project - rescue Gitlab::GitAccess::UnauthorizedError, Gitlab::GitAccess::NotFoundError => e - break { status: false, message: e.message } - end + check_result = begin + result = access_checker.check(params[:action], params[:changes]) + @project ||= access_checker.project + result + rescue Gitlab::GitAccess::UnauthorizedError => e + break response_with_status(code: 401, success: false, message: e.message) + rescue Gitlab::GitAccess::NotFoundError => e + break response_with_status(code: 404, success: false, message: e.message) + end log_user_activity(actor) - { - status: true, - gl_repository: gl_repository, - gl_id: Gitlab::GlId.gl_id(user), - gl_username: user&.username, + case check_result + when ::Gitlab::GitAccessResult::Success + payload = { + gl_repository: gl_repository, + gl_id: Gitlab::GlId.gl_id(user), + gl_username: user&.username, - # This repository_path is a bogus value but gitlab-shell still requires - # its presence. https://gitlab.com/gitlab-org/gitlab-shell/issues/135 - repository_path: '/', + # This repository_path is a bogus value but gitlab-shell still requires + # its presence. https://gitlab.com/gitlab-org/gitlab-shell/issues/135 + repository_path: '/', - gitaly: gitaly_payload(params[:action]) - } + gitaly: gitaly_payload(params[:action]) + } + response_with_status(**payload) + when ::Gitlab::GitAccessResult::CustomAction + response_with_status(code: 300, message: check_result.message, payload: check_result.payload) + else + response_with_status(code: 500, success: false, message: UNKNOWN_CHECK_RESULT_ERROR) + end end post "/lfs_authenticate" do diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index d74bcb8a459..93720500711 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -51,7 +51,7 @@ module Gitlab check_command_disabled!(cmd) check_command_existence!(cmd) - custom_action = check_custom_action!(cmd) + custom_action = check_custom_action(cmd) return custom_action if custom_action check_db_accessibility!(cmd) @@ -96,8 +96,8 @@ module Gitlab private - def check_custom_action!(cmd) - false + def check_custom_action(cmd) + nil end def check_valid_actor! diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 85c93f35c20..6890f46c724 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -381,7 +381,7 @@ describe API::Internal do it do pull(key, project) - expect(response).to have_gitlab_http_status(200) + expect(response).to have_gitlab_http_status(401) expect(json_response["status"]).to be_falsey expect(user.reload.last_activity_on).to be_nil end @@ -391,13 +391,61 @@ describe API::Internal do it do push(key, project) - expect(response).to have_gitlab_http_status(200) + expect(response).to have_gitlab_http_status(401) expect(json_response["status"]).to be_falsey expect(user.reload.last_activity_on).to be_nil end end end + context "custom action" do + let(:access_checker) { double(Gitlab::GitAccess) } + let(:message) { 'CustomActionError message' } + let(:payload) do + { + 'action' => 'geo_proxy_to_primary', + 'data' => { + 'api_endpoints' => %w{geo/proxy_git_push_ssh/info_refs geo/proxy_git_push_ssh/push}, + 'gl_username' => 'testuser', + 'primary_repo' => 'http://localhost:3000/testuser/repo.git' + } + } + end + + let(:custom_action_result) { Gitlab::GitAccessResult::CustomAction.new(payload, message) } + + before do + project.add_guest(user) + expect(Gitlab::GitAccess).to receive(:new).with( + key, + project, + 'ssh', + { + authentication_abilities: [:read_project, :download_code, :push_code], + namespace_path: project.namespace.name, + project_path: project.path, + redirected_path: nil + } + ).and_return(access_checker) + expect(access_checker).to receive(:check).with( + 'git-receive-pack', + 'd14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master' + ).and_return(custom_action_result) + end + + context "git push" do + it do + push(key, project) + + expect(response).to have_gitlab_http_status(300) + expect(json_response['status']).to be_truthy + expect(json_response['message']).to eql(message) + expect(json_response['payload']).to eql(payload) + expect(user.reload.last_activity_on).to be_nil + end + end + end + context "blocked user" do let(:personal_project) { create(:project, namespace: user.namespace) } @@ -409,7 +457,7 @@ describe API::Internal do it do pull(key, personal_project) - expect(response).to have_gitlab_http_status(200) + expect(response).to have_gitlab_http_status(401) expect(json_response["status"]).to be_falsey expect(user.reload.last_activity_on).to be_nil end @@ -419,7 +467,7 @@ describe API::Internal do it do push(key, personal_project) - expect(response).to have_gitlab_http_status(200) + expect(response).to have_gitlab_http_status(401) expect(json_response["status"]).to be_falsey expect(user.reload.last_activity_on).to be_nil end @@ -445,7 +493,7 @@ describe API::Internal do it do push(key, project) - expect(response).to have_gitlab_http_status(200) + expect(response).to have_gitlab_http_status(401) expect(json_response["status"]).to be_falsey end end @@ -477,7 +525,7 @@ describe API::Internal do it do archive(key, project) - expect(response).to have_gitlab_http_status(200) + expect(response).to have_gitlab_http_status(404) expect(json_response["status"]).to be_falsey end end @@ -489,7 +537,7 @@ describe API::Internal do pull(key, project) - expect(response).to have_gitlab_http_status(200) + expect(response).to have_gitlab_http_status(404) expect(json_response["status"]).to be_falsey end end @@ -498,7 +546,7 @@ describe API::Internal do it do pull(OpenStruct.new(id: 0), project) - expect(response).to have_gitlab_http_status(200) + expect(response).to have_gitlab_http_status(404) expect(json_response["status"]).to be_falsey end end @@ -511,7 +559,7 @@ describe API::Internal do it 'rejects the SSH push' do push(key, project) - expect(response.status).to eq(200) + expect(response.status).to eq(401) expect(json_response['status']).to be_falsey expect(json_response['message']).to eq 'Git access over SSH is not allowed' end @@ -519,7 +567,7 @@ describe API::Internal do it 'rejects the SSH pull' do pull(key, project) - expect(response.status).to eq(200) + expect(response.status).to eq(401) expect(json_response['status']).to be_falsey expect(json_response['message']).to eq 'Git access over SSH is not allowed' end @@ -533,7 +581,7 @@ describe API::Internal do it 'rejects the HTTP push' do push(key, project, 'http') - expect(response.status).to eq(200) + expect(response.status).to eq(401) expect(json_response['status']).to be_falsey expect(json_response['message']).to eq 'Git access over HTTP is not allowed' end @@ -541,7 +589,7 @@ describe API::Internal do it 'rejects the HTTP pull' do pull(key, project, 'http') - expect(response.status).to eq(200) + expect(response.status).to eq(401) expect(json_response['status']).to be_falsey expect(json_response['message']).to eq 'Git access over HTTP is not allowed' end @@ -571,14 +619,14 @@ describe API::Internal do it 'rejects the push' do push(key, project) - expect(response).to have_gitlab_http_status(200) + expect(response).to have_gitlab_http_status(404) expect(json_response['status']).to be_falsy end it 'rejects the SSH pull' do pull(key, project) - expect(response).to have_gitlab_http_status(200) + expect(response).to have_gitlab_http_status(404) expect(json_response['status']).to be_falsy end end