Simplify checking of allowed abilities in git_http_client_controller

4 files changed, 60 insertions, 59 deletions

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 3cc915ecc2a..632dac6aac9 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -4,7 +4,11 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   include ActionController::HttpAuthentication::Basic
   include KerberosSpnegoHelper
 
-  attr_reader :actor, :authentication_abilities
+  attr_reader :authentication_result
+
+  delegate :actor, :authentication_abilities, to: :authentication_result, allow_nil: true
+
+  alias_method :user, :actor
 
   # Git clients will not know what authenticity token to send along
   skip_before_action :verify_authenticity_token
@@ -26,9 +30,12 @@ class Projects::GitHttpClientController < Projects::ApplicationController
         return # Allow access
       end
     elsif allow_kerberos_spnego_auth? && spnego_provided?
-      @actor = find_kerberos_user
+      user = find_kerberos_user
+
+      if user
+        @authentication_result = Gitlab::Auth::Result.new(
+          user, nil, :kerberos, Gitlab::Auth.full_authentication_abilities)
 
-      if actor
         send_final_spnego_response
         return # Allow access
       end
@@ -104,56 +111,40 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     render plain: 'Not Found', status: :not_found
   end
 
-  def ci?
-    @ci
-  end
+  def handle_basic_authentication(login, password)
+    @authentication_result = Gitlab::Auth.find_for_git_client(
+      login, password, project: project, ip: request.ip)
 
-  def user
-    @actor
-  end
+    return false unless @authentication_result.success?
 
-  def handle_basic_authentication(login, password)
-    auth_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip)
-
-    case auth_result.type
-    when :ci
-      if auth_result.project == project && download_request?
-        @ci = true
-      else
-        return false
-      end
-    when :oauth
-      if download_request?
-        @actor = auth_result.actor
-        @authentication_abilities = auth_result.authentication_abilities
-      else
-        return false
-      end
-    when :lfs_deploy_token
-      if download_request?
-        @lfs_deploy_key = true
-        @actor = auth_result.actor
-        @authentication_abilities = auth_result.authentication_abilities
-      else
-        return false
-      end
-    when :lfs_token, :personal_token, :gitlab_or_ldap, :build
-      @actor = auth_result.actor
-      @authentication_abilities = auth_result.authentication_abilities
+    if download_request?
+      authentication_has_download_access?
     else
-      # Not allowed
-      return false
+      authentication_has_upload_access?
     end
+  end
+
+  def authentication_has_download_access?
+    has_authentication_ability?(:download_code) || has_authentication_ability?(:build_download_code)
+  end
+
+  def authentication_has_upload_access?
+    has_authentication_ability?(:push_code)
+  end
 
-    true
+  def ci?
+    authentication_result && authentication_result.ci? &&
+      authentication_result.project && authentication_result.project == project
   end
 
   def lfs_deploy_key?
-    @lfs_deploy_key && actor && actor.projects.include?(project)
+    authentication_result && authentication_result.lfs_deploy_token? &&
+      actor && actor.projects.include?(project)
   end
 
   def has_authentication_ability?(capability)
-    @authentication_abilities.include?(capability)
+    authentication_abilities &&
+      authentication_abilities.include?(capability)
   end
 
   def verify_workhorse_api!
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 3d7cc176e07..f9ae5e4543f 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -1,6 +1,14 @@
 module Gitlab
   module Auth
     Result = Struct.new(:actor, :project, :type, :authentication_abilities) do
+      def ci?
+        type == :ci
+      end
+
+      def lfs_deploy_token?
+        type == :lfs_deploy_token
+      end
+
       def success?
         actor.present? || type == :ci
       end
@@ -143,6 +151,8 @@ module Gitlab
         end
       end
 
+      public
+
       def build_authentication_abilities
         [
           :read_project,
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 0311755dd06..f828e898740 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -346,7 +346,7 @@ describe 'Git HTTP requests', lib: true do
             it 'uploads get status 403' do
               push_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token
 
-              expect(response).to have_http_status(403)
+              expect(response).to have_http_status(401)
             end
           end
 
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index cad8b914668..09e4e265dd1 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -310,8 +310,8 @@ describe 'Git LFS API and storage' do
             let(:build) { create(:ci_build, :running, pipeline: pipeline) }
 
             it_behaves_like 'can download LFS only from own projects' do
-              # We render 401, to prevent data leakage about existence of the project
-              let(:other_project_status) { 401 }
+              # We render 404, to prevent data leakage about existence of the project
+              let(:other_project_status) { 404 }
             end
           end
         end
@@ -545,8 +545,8 @@ describe 'Git LFS API and storage' do
           let(:build) { create(:ci_build, :running, pipeline: pipeline) }
 
           it_behaves_like 'can download LFS only from own projects' do
-            # We render 401, to prevent data leakage about existence of the project
-            let(:other_project_status) { 401 }
+            # We render 404, to prevent data leakage about existence of the project
+            let(:other_project_status) { 404 }
           end
         end
       end
@@ -706,8 +706,8 @@ describe 'Git LFS API and storage' do
             context 'tries to push to own project' do
               let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
 
-              it 'responds with 403' do
-                expect(response).to have_http_status(403)
+              it 'responds with 401' do
+                expect(response).to have_http_status(401)
               end
             end
 
@@ -716,8 +716,8 @@ describe 'Git LFS API and storage' do
               let(:pipeline) { create(:ci_empty_pipeline, project: other_project) }
               let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
 
-              it 'responds with 403' do
-                expect(response).to have_http_status(403)
+              it 'responds with 401' do
+                expect(response).to have_http_status(401)
               end
             end
           end
@@ -925,8 +925,8 @@ describe 'Git LFS API and storage' do
               put_authorize
             end
 
-            it 'responds with 403' do
-              expect(response).to have_http_status(403)
+            it 'responds with 401' do
+              expect(response).to have_http_status(401)
             end
           end
 
@@ -939,8 +939,8 @@ describe 'Git LFS API and storage' do
               put_authorize
             end
 
-            it 'responds with 404' do
-              expect(response).to have_http_status(404)
+            it 'responds with 401' do
+              expect(response).to have_http_status(401)
             end
           end
         end
@@ -1025,8 +1025,8 @@ describe 'Git LFS API and storage' do
           context 'tries to push to own project' do
             let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
 
-            it 'responds with 403' do
-              expect(response).to have_http_status(403)
+            it 'responds with 401' do
+              expect(response).to have_http_status(401)
             end
           end
 
@@ -1035,8 +1035,8 @@ describe 'Git LFS API and storage' do
             let(:pipeline) { create(:ci_empty_pipeline, project: other_project) }
             let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
 
-            it 'responds with 403' do
-              expect(response).to have_http_status(403)
+            it 'responds with 401' do
+              expect(response).to have_http_status(401)
             end
           end
         end