Further refactoring of authentication code, and code style fixes.

3 files changed, 48 insertions, 47 deletions

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 4be580e759e..f5a07608bf8 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -4,7 +4,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   include ActionController::HttpAuthentication::Basic
   include KerberosSpnegoHelper
 
-  attr_reader :user
+  attr_reader :user, :actor
 
   # Git clients will not know what authenticity token to send along
   skip_before_action :verify_authenticity_token
@@ -24,13 +24,13 @@ class Projects::GitHttpClientController < Projects::ApplicationController
 
       handle_basic_authentication(login, password)
 
-      if ci? || user
+      if ci? || actor
         return # Allow access
       end
     elsif allow_kerberos_spnego_auth? && spnego_provided?
-      @user = find_kerberos_user
+      @actor = find_kerberos_user
 
-      if user
+      if actor
         send_final_spnego_response
         return # Allow access
       end
@@ -110,6 +110,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     @ci.present?
   end
 
+  def user
+    @actor
+  end
+
   def handle_basic_authentication(login, password)
     auth_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip)
 
@@ -117,21 +121,21 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     when :ci
       @ci = true if download_request?
     when :oauth
-      @user = auth_result.user if download_request?
+      @actor = auth_result.actor if download_request?
     when :lfs_deploy_token
       if download_request?
         @lfs_deploy_key = true
-        @user = auth_result.user
+        @actor = auth_result.actor
       end
     when :lfs_token, :personal_token, :gitlab_or_ldap
-      @user = auth_result.user
+      @actor = auth_result.actor
     else
       # Not allowed
     end
   end
 
   def lfs_deploy_key?
-    @lfs_deploy_key.present? && (user && user.projects.include?(project))
+    @lfs_deploy_key.present? && actor && actor.projects.include?(project)
   end
 
   def verify_workhorse_api!
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index f4e6ebb7bc7..391b8f2f5de 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -1,6 +1,6 @@
 module Gitlab
   module Auth
-    Result = Struct.new(:user, :type)
+    Result = Struct.new(:actor, :type)
 
     class MissingPersonalTokenError < StandardError; end
 
@@ -49,6 +49,24 @@ module Gitlab
 
       private
 
+      def populate_result(login, password, project, ip)
+        result =
+          ci_request_check(login, password, project) ||
+          user_with_password_for_git(login, password) ||
+          oauth_access_token_check(login, password) ||
+          lfs_token_check(login, password) ||
+          personal_access_token_check(login, password)
+
+        if result && result.type != :ci
+          result.type = nil unless result.actor
+        end
+
+        success = result ? result.actor.present? || result.type == :ci : false
+        rate_limit!(ip, success: success, login: login)
+
+        result || Result.new
+      end
+
       def valid_ci_request?(login, password, project)
         matched_login = /(?<service>^[a-zA-Z]*-ci)-token$/.match(login)
 
@@ -67,31 +85,14 @@ module Gitlab
         end
       end
 
-      def populate_result(login, password, project, ip)
-        result = Result.new(nil, :ci) if valid_ci_request?(login, password, project)
-
-        result ||=
-          user_with_password_for_git(login, password) ||
-          oauth_access_token_check(login, password) ||
-          lfs_token_check(login, password) ||
-          personal_access_token_check(login, password)
-
-        if result && result.type != :ci
-          result.type = nil unless result.user
-
-          if result.user && result.type == :gitlab_or_ldap && result.user.two_factor_enabled?
-            raise Gitlab::Auth::MissingPersonalTokenError
-          end
-        end
-
-        success = result ? result.user.present? || [:ci].include?(result.type) : false
-        rate_limit!(ip, success: success, login: login)
-
-        result || Result.new
+      def ci_request_check(login, password, project)
+        Result.new(nil, :ci) if valid_ci_request?(login, password, project)
       end
 
       def user_with_password_for_git(login, password)
         user = find_with_user_password(login, password)
+        raise Gitlab::Auth::MissingPersonalTokenError if user && user.two_factor_enabled?
+
         Result.new(user, :gitlab_or_ldap) if user
       end
 
@@ -114,11 +115,11 @@ module Gitlab
       end
 
       def lfs_token_check(login, password)
+        deploy_key_matches = login.match(/\Alfs\+deploy-key-(\d+)\z/)
+
         actor =
-          if login =~ /\Alfs\+deploy-key-\d+\Z/
-            /\d+\Z/.match(login) do |id|
-              DeployKey.find(id[0])
-            end
+          if deploy_key_matches
+            DeployKey.find(deploy_key_matches[1])
           else
             User.by_login(login)
           end
diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb
index edf4dffc4c0..224e4516074 100644
--- a/lib/gitlab/lfs_token.rb
+++ b/lib/gitlab/lfs_token.rb
@@ -6,7 +6,15 @@ module Gitlab
     EXPIRY_TIME = 1800
 
     def initialize(actor)
-      set_actor(actor)
+      @actor =
+        case actor
+        when DeployKey, User
+          actor
+        when Key
+          actor.user
+        else
+          #
+        end
     end
 
     def generate
@@ -38,17 +46,5 @@ module Gitlab
     def redis_key
       "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}" if actor
     end
-
-    def set_actor(actor)
-      @actor =
-        case actor
-        when DeployKey, User
-          actor
-        when Key
-          actor.user
-        else
-          #
-        end
-    end
   end
 end