Refactored authentication code to make it a bit clearer, added test for wrong SSH key.lfs-support-for-ssh

6 files changed, 52 insertions, 38 deletions

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index f5a07608bf8..f74a7000373 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, :actor
+  attr_reader :actor
 
   # Git clients will not know what authenticity token to send along
   skip_before_action :verify_authenticity_token
@@ -22,9 +22,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     if allow_basic_auth? && basic_auth_provided?
       login, password = user_name_and_password(request)
 
-      handle_basic_authentication(login, password)
-
-      if ci? || actor
+      if handle_basic_authentication(login, password)
         return # Allow access
       end
     elsif allow_kerberos_spnego_auth? && spnego_provided?
@@ -107,7 +105,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   end
 
   def ci?
-    @ci.present?
+    @ci
   end
 
   def user
@@ -119,23 +117,36 @@ class Projects::GitHttpClientController < Projects::ApplicationController
 
     case auth_result.type
     when :ci
-      @ci = true if download_request?
+      if download_request?
+        @ci = true
+      else
+        return false
+      end
     when :oauth
-      @actor = auth_result.actor if download_request?
+      if download_request?
+        @actor = auth_result.actor
+      else
+        return false
+      end
     when :lfs_deploy_token
       if download_request?
         @lfs_deploy_key = true
         @actor = auth_result.actor
+      else
+        return false
       end
     when :lfs_token, :personal_token, :gitlab_or_ldap
       @actor = auth_result.actor
     else
       # Not allowed
+      return false
     end
+
+    true
   end
 
   def lfs_deploy_key?
-    @lfs_deploy_key.present? && actor && actor.projects.include?(project)
+    @lfs_deploy_key && actor && actor.projects.include?(project)
   end
 
   def verify_workhorse_api!
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 391b8f2f5de..6be9bf7de44 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -1,6 +1,10 @@
 module Gitlab
   module Auth
-    Result = Struct.new(:actor, :type)
+    Result = Struct.new(:actor, :type) do
+      def success?
+        actor.present? || type == :ci
+      end
+    end
 
     class MissingPersonalTokenError < StandardError; end
 
@@ -8,7 +12,16 @@ module Gitlab
       def find_for_git_client(login, password, project:, ip:)
         raise "Must provide an IP for rate limiting" if ip.nil?
 
-        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)
+
+        rate_limit!(ip, success: result && result.success?, login: login)
+
+        result || Result.new
       end
 
       def find_with_user_password(login, password)
@@ -49,24 +62,6 @@ 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)
 
@@ -110,7 +105,7 @@ module Gitlab
         if login && password
           user = User.find_by_personal_access_token(password)
           validation = User.by_login(login)
-          Result.new(user, :personal_token) if user == validation
+          Result.new(user, :personal_token) if user.present? && user == validation
         end
       end
 
@@ -124,9 +119,11 @@ module Gitlab
             User.by_login(login)
           end
 
-        token_handler = Gitlab::LfsToken.new(actor)
+        if actor
+          token_handler = Gitlab::LfsToken.new(actor)
 
-        Result.new(actor, token_handler.type) if actor && Devise.secure_compare(token_handler.value, password)
+          Result.new(actor, token_handler.type) if Devise.secure_compare(token_handler.value, password)
+        end
       end
     end
   end
diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb
index 224e4516074..f492754b1c8 100644
--- a/lib/gitlab/lfs_token.rb
+++ b/lib/gitlab/lfs_token.rb
@@ -13,7 +13,7 @@ module Gitlab
         when Key
           actor.user
         else
-          #
+          raise 'Bad Actor'
         end
     end
 
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 56f349f5d92..13c5a7156f5 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -55,7 +55,7 @@ describe Gitlab::Auth, lib: true do
       login = 'foo'
       ip = 'ip'
 
-      expect(gl_auth).to receive(:rate_limit!).with(ip, success: false, login: login)
+      expect(gl_auth).to receive(:rate_limit!).with(ip, success: nil, login: login)
       expect(gl_auth.find_for_git_client(login, 'bar', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new)
     end
   end
diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb
index 184f235c1b2..9f04f67e0a8 100644
--- a/spec/lib/gitlab/lfs_token_spec.rb
+++ b/spec/lib/gitlab/lfs_token_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe Gitlab::LfsToken, lib: true do
-  describe '#set_token and #get_value' do
+  describe '#generate and #value' do
     shared_examples 'an LFS token generator' do
       it 'returns a randomly generated token' do
         token = handler.generate
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 2e1e6a11b53..46e8e6f1169 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -107,7 +107,7 @@ describe API::API, api: true  do
 
     context 'user key' do
       it 'returns the correct information about the key' do
-        lfs_auth(key, project)
+        lfs_auth(key.id, project)
 
         expect(response).to have_http_status(200)
         expect(json_response['username']).to eq(user.username)
@@ -115,13 +115,19 @@ describe API::API, api: true  do
 
         expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
       end
+
+      it 'returns a 404 when the wrong key is provided' do
+        lfs_auth(nil, project)
+
+        expect(response).to have_http_status(404)
+      end
     end
 
     context 'deploy key' do
       let(:key) { create(:deploy_key) }
 
       it 'returns the correct information about the key' do
-        lfs_auth(key, project)
+        lfs_auth(key.id, project)
 
         expect(response).to have_http_status(200)
         expect(json_response['username']).to eq("lfs+deploy-key-#{key.id}")
@@ -421,10 +427,10 @@ describe API::API, api: true  do
     )
   end
 
-  def lfs_auth(key, project)
+  def lfs_auth(key_id, project)
     post(
       api("/internal/lfs_authenticate"),
-      key_id: key.id,
+      key_id: key_id,
       secret_token: secret_token,
       project: project.path_with_namespace
     )