Update /api/v4/allowed

- Use proper HTTP codes for /api/v4/allowed response
- CustomAction support

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,
-
-          # 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])
-        }
+        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: '/',
+
+            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

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!

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