Better authentication handling, syntax fixes and better actor handling for LFS Tokens

6 files changed, 51 insertions, 45 deletions

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 4dff1ce6568..b4ec5b3fae1 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -4,8 +4,6 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   include ActionController::HttpAuthentication::Basic
   include KerberosSpnegoHelper
 
-  class MissingPersonalTokenError < StandardError; end
-
   attr_reader :user
 
   # Git clients will not know what authenticity token to send along
@@ -40,10 +38,8 @@ class Projects::GitHttpClientController < Projects::ApplicationController
 
     send_challenges
     render plain: "HTTP Basic: Access denied\n", status: 401
-
-  rescue MissingPersonalTokenError
+  rescue Gitlab::Auth::MissingPersonalTokenError
     render_missing_personal_token
-    return
   end
 
   def basic_auth_provided?
@@ -117,17 +113,20 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   def handle_authentication(login, password)
     auth_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip)
 
-    if auth_result.type == :ci && download_request?
-      @ci = true
-    elsif auth_result.type == :oauth && !download_request?
-      # Not allowed
-    elsif auth_result.type == :missing_personal_token
-      raise MissingPersonalTokenError
-    elsif auth_result.type == :lfs_deploy_token && download_request?
-      @lfs_deploy_key = true
+    case auth_result.type
+    when :ci
+      @ci = true if download_request?
+    when :oauth
+      @user = auth_result.user if download_request?
+    when :lfs_deploy_token
+      if download_request?
+        @lfs_deploy_key = true
+        @user = auth_result.user
+      end
+    when :lfs_token, :personal_token, :gitlab_or_ldap
       @user = auth_result.user
     else
-      @user = auth_result.user
+      # Not allowed
     end
   end
 
diff --git a/app/helpers/lfs_helper.rb b/app/helpers/lfs_helper.rb
index 031e7e72909..de7c9f253b2 100644
--- a/app/helpers/lfs_helper.rb
+++ b/app/helpers/lfs_helper.rb
@@ -27,7 +27,7 @@ module LfsHelper
 
     return true if project.public? || ci? || lfs_deploy_key?
 
-    (user && user.can?(:download_code, project))
+    user && user.can?(:download_code, project)
   end
 
   def lfs_upload_access?
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 1f189d81d16..f8211bdd8af 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -78,14 +78,7 @@ module API
         status 200
 
         key = Key.find(params[:key_id])
-        user = key.user
-
-        token_handler =
-          if user
-            Gitlab::LfsToken.new(user)
-          else
-            Gitlab::LfsToken.new(key)
-          end
+        token_handler = Gitlab::LfsToken.new(key)
 
         {
           username: token_handler.actor_name,
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 02b33c8c683..14e29124aac 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -2,21 +2,13 @@ module Gitlab
   module Auth
     Result = Struct.new(:user, :type)
 
+    class MissingPersonalTokenError < StandardError; end
+
     class << self
       def find_for_git_client(login, password, project:, ip:)
         raise "Must provide an IP for rate limiting" if ip.nil?
 
-        result = Result.new
-
-        if valid_ci_request?(login, password, project)
-          result.type = :ci
-        else
-          result = populate_result(login, password)
-        end
-
-        success = result.user.present? || [:ci, :missing_personal_token].include?(result.type)
-        rate_limit!(ip, success: success, login: login)
-        result
+        populate_result(login, password, project, ip)
       end
 
       def find_with_user_password(login, password)
@@ -75,21 +67,26 @@ module Gitlab
         end
       end
 
-      def populate_result(login, password)
-        result =
+      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
+        if result && result.type != :ci
           result.type = nil unless result.user
 
           if result.user && result.type == :gitlab_or_ldap && result.user.two_factor_enabled?
-            result.type = :missing_personal_token
+            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
       end
 
@@ -118,15 +115,17 @@ module Gitlab
 
       def lfs_token_check(login, password)
         actor =
-          if login.start_with?('lfs-deploy-key')
-            DeployKey.find(login.sub('lfs-deploy-key-', ''))
+          if login =~ /\Alfs-deploy-key-\d+\Z/
+            /\d+\Z/.match(login) do |id|
+              DeployKey.find(id[0])
+            end
           else
             User.by_login(login)
           end
 
         token_handler = Gitlab::LfsToken.new(actor)
 
-        Result.new(actor, token_handler.type) if actor && token_handler.value == password
+        Result.new(actor, token_handler.type) if actor && Devise.secure_compare(token_handler.value, password)
       end
     end
   end
diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb
index 8f49deb4d03..d7db8017475 100644
--- a/lib/gitlab/lfs_token.rb
+++ b/lib/gitlab/lfs_token.rb
@@ -2,15 +2,18 @@ module Gitlab
   class LfsToken
     attr_accessor :actor
 
+    TOKEN_LENGTH = 50
+    EXPIRY_TIME = 1800
+
     def initialize(actor)
-      @actor = actor
+      set_actor(actor)
     end
 
     def generate
-      token = Devise.friendly_token(50)
+      token = Devise.friendly_token(TOKEN_LENGTH)
 
       Gitlab::Redis.with do |redis|
-        redis.set(redis_key, token, ex: 600)
+        redis.set(redis_key, token, ex: EXPIRY_TIME)
       end
 
       token
@@ -35,5 +38,17 @@ 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
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index ff697286927..1ee390e0a19 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -111,7 +111,7 @@ describe API::API, api: true  do
 
         expect(response).to have_http_status(200)
         expect(json_response['username']).to eq(user.username)
-        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(user).value)
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).value)
 
         expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
       end