From 19a5e7c95e91baca58836ad3ae189190c9ba4ca2 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Wed, 23 Mar 2016 14:04:09 +0100
Subject: [PATCH 01/39] Test Grack::Auth via a request spec

---
 .../git_http_spec.rb}                         | 167 ++++++++----------
 1 file changed, 74 insertions(+), 93 deletions(-)
 rename spec/{lib/gitlab/backend/grack_auth_spec.rb => requests/git_http_spec.rb} (57%)

diff --git a/spec/lib/gitlab/backend/grack_auth_spec.rb b/spec/requests/git_http_spec.rb
similarity index 57%
rename from spec/lib/gitlab/backend/grack_auth_spec.rb
rename to spec/requests/git_http_spec.rb
index cd26dca0998..7e274b4209b 100644
--- a/spec/lib/gitlab/backend/grack_auth_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -1,83 +1,60 @@
 require "spec_helper"
 
-describe Grack::Auth, lib: true do
+describe 'Git HTTP requests', lib: true do
   let(:user)    { create(:user) }
   let(:project) { create(:project) }
 
-  let(:app)   { lambda { |env| [200, {}, "Success!"] } }
-  let!(:auth) { Grack::Auth.new(app) }
-  let(:env) do
-    {
-      'rack.input'     => '',
-      'REQUEST_METHOD' => 'GET',
-      'QUERY_STRING'   => 'service=git-upload-pack'
-    }
-  end
-  let(:status) { auth.call(env).first }
-
   describe "#call" do
     context "when the project doesn't exist" do
-      before do
-        env["PATH_INFO"] = "doesnt/exist.git"
-      end
-
       context "when no authentication is provided" do
         it "responds with status 401" do
-          expect(status).to eq(401)
+          clone_get '/doesnt/exist.git/info/refs'
+
+          expect(response.status).to eq(401)
         end
       end
 
       context "when username and password are provided" do
         context "when authentication fails" do
-          before do
-            env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, "nope")
-          end
-
           it "responds with status 401" do
-            expect(status).to eq(401)
+            clone_get '/doesnt/exist.git/info/refs', user: user.username, password: "nope"
+
+            expect(response.status).to eq(401)
           end
         end
 
         context "when authentication succeeds" do
-          before do
-            env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, user.password)
-          end
-
           it "responds with status 404" do
-            expect(status).to eq(404)
+            clone_get '/doesnt/exist.git/info/refs', user: user.username, password: user.password
+
+            expect(response.status).to eq(404)
           end
         end
       end
     end
 
     context "when the Wiki for a project exists" do
-      before do
-        @wiki = ProjectWiki.new(project)
-        env["PATH_INFO"] = "#{@wiki.repository.path_with_namespace}.git/info/refs"
-        project.update_attribute(:visibility_level, Project::PUBLIC)
-      end
-
       it "responds with the right project" do
-        response = auth.call(env)
-        json_body = ActiveSupport::JSON.decode(response[2][0])
+        wiki = ProjectWiki.new(project)
+        project.update_attribute(:visibility_level, Project::PUBLIC)
 
-        expect(response.first).to eq(200)
-        expect(json_body['RepoPath']).to include(@wiki.repository.path_with_namespace)
+        clone_get "/#{wiki.repository.path_with_namespace}.git/info/refs"
+        json_body = ActiveSupport::JSON.decode(response.body)
+
+        expect(response.status).to eq(200)
+        expect(json_body['RepoPath']).to include(wiki.repository.path_with_namespace)
       end
     end
 
     context "when the project exists" do
-      before do
-        env["PATH_INFO"] = project.path_with_namespace + ".git"
-      end
+      let(:path) { clone_path(project) }
 
       context "when the project is public" do
-        before do
-          project.update_attribute(:visibility_level, Project::PUBLIC)
-        end
-
         it "responds with status 200" do
-          expect(status).to eq(200)
+          project.update_attribute(:visibility_level, Project::PUBLIC)
+          clone_get path
+
+          expect(response.status).to eq(200)
         end
       end
 
@@ -88,85 +65,74 @@ describe Grack::Auth, lib: true do
 
         context "when no authentication is provided" do
           it "responds with status 401" do
-            expect(status).to eq(401)
+            clone_get path
+
+            expect(response.status).to eq(401)
           end
         end
 
         context "when username and password are provided" do
           context "when authentication fails" do
-            before do
-              env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, "nope")
-            end
-
             it "responds with status 401" do
-              expect(status).to eq(401)
+              clone_get path, user: user.username, password: 'nope'
+
+              expect(response.status).to eq(401)
             end
 
             context "when the user is IP banned" do
-              before do
+              it "responds with status 401" do
                 expect(Rack::Attack::Allow2Ban).to receive(:filter).and_return(true)
                 allow_any_instance_of(Rack::Request).to receive(:ip).and_return('1.2.3.4')
-              end
 
-              it "responds with status 401" do
-                expect(status).to eq(401)
+                clone_get path, user: user.username, password: 'nope'
+
+                expect(response.status).to eq(401)
               end
             end
           end
 
           context "when authentication succeeds" do
-            before do
-              env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, user.password)
-            end
-
             context "when the user has access to the project" do
               before do
                 project.team << [user, :master]
               end
 
               context "when the user is blocked" do
-                before do
+                it "responds with status 404" do
                   user.block
                   project.team << [user, :master]
-                end
 
-                it "responds with status 404" do
-                  expect(status).to eq(404)
+                  clone_get path, user: user.username, password: user.password
+
+                  expect(response.status).to eq(404)
                 end
               end
 
               context "when the user isn't blocked" do
-                before do
-                  expect(Rack::Attack::Allow2Ban).to receive(:reset)
-                end
-
                 it "responds with status 200" do
-                  expect(status).to eq(200)
+                  expect(Rack::Attack::Allow2Ban).to receive(:reset)
+
+                  clone_get path, user: user.username, password: user.password
+
+                  expect(response.status).to eq(200)
                 end
               end
 
               context "when blank password attempts follow a valid login" do
-                let(:options) { Gitlab.config.rack_attack.git_basic_auth }
-                let(:maxretry) { options[:maxretry] - 1 }
-                let(:ip) { '1.2.3.4' }
-
-                before do
-                  allow_any_instance_of(Rack::Request).to receive(:ip).and_return(ip)
-                  Rack::Attack::Allow2Ban.reset(ip, options)
-                end
-
-                after do
-                  Rack::Attack::Allow2Ban.reset(ip, options)
-                end
-
                 def attempt_login(include_password)
                   password = include_password ? user.password : ""
-                  env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, password)
-                  Grack::Auth.new(app)
-                  auth.call(env).first
+                  clone_get path, user: user.username, password: password
+                  response.status
                 end
 
                 it "repeated attempts followed by successful attempt" do
+                  options = Gitlab.config.rack_attack.git_basic_auth
+                  maxretry = options[:maxretry] - 1
+                  ip = '1.2.3.4'
+
+                  allow_any_instance_of(Rack::Request).to receive(:ip).and_return(ip)
+                  Rack::Attack::Allow2Ban.reset(ip, options)
+
                   maxretry.times.each do
                     expect(attempt_login(false)).to eq(401)
                   end
@@ -177,33 +143,48 @@ describe Grack::Auth, lib: true do
                   maxretry.times.each do
                     expect(attempt_login(false)).to eq(401)
                   end
+
+                  Rack::Attack::Allow2Ban.reset(ip, options)
                 end
               end
             end
 
             context "when the user doesn't have access to the project" do
               it "responds with status 404" do
-                expect(status).to eq(404)
+                clone_get path, user: user.username, password: user.password
+
+                expect(response.status).to eq(404)
               end
             end
           end
         end
 
         context "when a gitlab ci token is provided" do
-          let(:token) { "123" }
-          let(:project) { FactoryGirl.create :empty_project }
-
-          before do
+          it "responds with status 200" do
+            token = "123"
+            project = FactoryGirl.create :empty_project
             project.update_attributes(runners_token: token, builds_enabled: true)
 
-            env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials("gitlab-ci-token", token)
-          end
+            clone_get clone_path(project), user: 'gitlab-ci-token', password: token
 
-          it "responds with status 200" do
-            expect(status).to eq(200)
+            expect(response.status).to eq(200)
           end
         end
       end
     end
   end
+
+  def clone_get(url, user: nil, password: nil)
+    if user && password
+      env = { 'HTTP_AUTHORIZATION' => ActionController::HttpAuthentication::Basic.encode_credentials(user, password) }
+    else
+      env = {}
+    end
+
+    get url, { 'service' => 'git-upload-pack' }, env
+  end
+
+  def clone_path(project)
+    "/#{project.path_with_namespace}.git/info/refs"
+  end
 end

From 55f5a68f092cc64ae4782c0d7fbbf1d3d1ce6284 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Wed, 23 Mar 2016 18:34:16 +0100
Subject: [PATCH 02/39] Get Grack::Auth tests to pass

---
 .../projects/application_controller.rb        |  22 ++-
 .../projects/git_http_controller.rb           | 167 ++++++++++++++++++
 config/routes.rb                              |  10 +-
 3 files changed, 191 insertions(+), 8 deletions(-)
 create mode 100644 app/controllers/projects/git_http_controller.rb

diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index 657ee94cfd7..5f5dc1adadf 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -10,9 +10,6 @@ class Projects::ApplicationController < ApplicationController
 
   def project
     unless @project
-      namespace = params[:namespace_id]
-      id = params[:project_id] || params[:id]
-
       # Redirect from
       #   localhost/group/project.git
       # to
@@ -23,8 +20,7 @@ class Projects::ApplicationController < ApplicationController
         return
       end
 
-      project_path = "#{namespace}/#{id}"
-      @project = Project.find_with_namespace(project_path)
+      @project = find_project
 
       if @project && can?(current_user, :read_project, @project)
         if @project.path_with_namespace != project_path
@@ -44,6 +40,22 @@ class Projects::ApplicationController < ApplicationController
     @project
   end
 
+  def id
+    params[:project_id] || params[:id]
+  end
+  
+  def namespace
+    params[:namespace_id]
+  end
+  
+  def project_path
+    "#{namespace}/#{id}"
+  end
+  
+  def find_project
+    Project.find_with_namespace(project_path)
+  end
+
   def repository
     @repository ||= project.repository
   end
diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
new file mode 100644
index 00000000000..129e87dbf13
--- /dev/null
+++ b/app/controllers/projects/git_http_controller.rb
@@ -0,0 +1,167 @@
+class Projects::GitHttpController < Projects::ApplicationController
+  skip_before_action :repository
+  before_action :authenticate_user
+  before_action :project_found?
+    
+  def git_rpc
+    if upload_pack? && upload_pack_allowed?
+      render_ok and return
+    end
+    
+    render_not_found
+  end
+  
+  %i{info_refs git_receive_pack git_upload_pack}.each do |method|
+    alias_method method, :git_rpc
+  end
+
+  private
+
+  def authenticate_user
+    return if project && project.public? && upload_pack?
+
+    authenticate_or_request_with_http_basic do |login, password|
+      return @ci = true if ci_request?(login, password)
+
+      @user = Gitlab::Auth.new.find(login, password)
+      @user ||= oauth_access_token_check(login, password)
+      rate_limit_ip!(login, @user)
+    end
+  end
+
+  def project_found?
+    render_not_found if project.nil?
+  end
+
+  def ci_request?(login, password)
+    matched_login = /(?<s>^[a-zA-Z]*-ci)-token$/.match(login)
+
+    if project && matched_login.present? && upload_pack?
+      underscored_service = matched_login['s'].underscore
+
+      if underscored_service == 'gitlab_ci'
+        return project && project.valid_build_token?(password)
+      elsif Service.available_services_names.include?(underscored_service)
+        service_method = "#{underscored_service}_service"
+        service = project.send(service_method)
+
+        return service && service.activated? && service.valid_token?(password)
+      end
+    end
+
+    false
+  end
+
+  def oauth_access_token_check(login, password)
+    if login == "oauth2" && upload_pack? && password.present?
+      token = Doorkeeper::AccessToken.by_token(password)
+      token && token.accessible? && User.find_by(id: token.resource_owner_id)
+    end
+  end
+  
+  def rate_limit_ip!(login, user)
+    # If the user authenticated successfully, we reset the auth failure count
+    # from Rack::Attack for that IP. A client may attempt to authenticate
+    # with a username and blank password first, and only after it receives
+    # a 401 error does it present a password. Resetting the count prevents
+    # false positives from occurring.
+    #
+    # Otherwise, we let Rack::Attack know there was a failed authentication
+    # attempt from this IP. This information is stored in the Rails cache
+    # (Redis) and will be used by the Rack::Attack middleware to decide
+    # whether to block requests from this IP.
+
+    config = Gitlab.config.rack_attack.git_basic_auth
+    return user unless config.enabled
+
+    if user
+      # A successful login will reset the auth failure count from this IP
+      Rack::Attack::Allow2Ban.reset(request.ip, config)
+    else
+      banned = Rack::Attack::Allow2Ban.filter(request.ip, config) do
+        # Unless the IP is whitelisted, return true so that Allow2Ban
+        # increments the counter (stored in Rails.cache) for the IP
+        if config.ip_whitelist.include?(request.ip)
+          false
+        else
+          true
+        end
+      end
+
+      if banned
+        Rails.logger.info "IP #{request.ip} failed to login " \
+          "as #{login} but has been temporarily banned from Git auth"
+      end
+    end
+    
+    user
+  end
+
+  def project
+    return @project if defined?(@project)
+    @project = find_project
+  end
+
+  def id
+    id = params[:project_id]
+    return if id.nil?
+    
+    if id.end_with?('.wiki.git')
+      id.slice(0, id.length - 9)
+    elsif id.end_with?('.git')
+      id.slice(0, id.length - 4)
+    end
+  end
+
+  def repo_path
+    @repo_path ||= begin
+      if params[:project_id].end_with?('.wiki.git')
+        project.wiki.wiki.path
+      else
+        repository.path_to_repo
+      end
+    end
+  end
+
+  def upload_pack?
+    if action_name == 'info_refs'
+      params[:service] == 'git-upload-pack'
+    else
+      action_name == 'git_upload_pack'
+    end
+  end
+
+  def render_ok
+    render json: {
+      'GL_ID' => Gitlab::ShellEnv.gl_id(@user),
+      'RepoPath' => repo_path,
+    }
+  end
+  
+  def render_not_found
+    render text: 'Not Found', status: :not_found
+  end
+
+  def ci?
+    !!@ci
+  end
+  
+  def user
+    @user
+  end
+  
+  def upload_pack_allowed?
+    if !Gitlab.config.gitlab_shell.upload_pack
+      false
+    elsif ci?
+      true
+    elsif user
+      Gitlab::GitAccess.new(user, project).download_access_check.allowed?
+    elsif project.public?
+      # Allow clone/fetch for public projects
+      true
+    else
+      false
+    end
+  end
+end
diff --git a/config/routes.rb b/config/routes.rb
index 4a3c23b7c1c..47ab1a89b8d 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -59,9 +59,6 @@ Rails.application.routes.draw do
     mount Sidekiq::Web, at: '/admin/sidekiq', as: :sidekiq
   end
 
-  # Enable Grack support
-  mount Grack::AuthSpawner, at: '/', constraints: lambda { |request| /[-\/\w\.]+\.git\//.match(request.path_info) }, via: [:get, :post, :put]
-
   # Help
   get 'help'                  => 'help#index'
   get 'help/:category/:file'  => 'help#show', as: :help_page, constraints: { category: /.*/, file: /[^\/\.]+/ }
@@ -426,6 +423,13 @@ Rails.application.routes.draw do
       end
 
       scope module: :projects do
+        # Git HTTP clients ('git clone' etc.)
+        scope constraints: { format: /(git|wiki\.git)/ } do
+          get '/info/refs', to: 'git_http#info_refs', only: :get
+          get '/git-upload-pack', to: 'git_http#git_upload_pack', only: :post
+          get '/git-receive-pack', to: 'git_http#git_receive_pack', only: :post
+        end
+
         # Blob routes:
         get '/new/*id', to: 'blob#new', constraints: { id: /.+/ }, as: 'new_blob'
         post '/create/*id', to: 'blob#create', constraints: { id: /.+/ }, as: 'create_blob'

From 8f3e86d72c16294c8bcec8c9a3af86ec99d66ee8 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Thu, 24 Mar 2016 14:53:20 +0100
Subject: [PATCH 03/39] Keep Grack::Auth in the routes for LFS only

---
 config/routes.rb | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/config/routes.rb b/config/routes.rb
index 47ab1a89b8d..021eab89c2a 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -59,6 +59,9 @@ Rails.application.routes.draw do
     mount Sidekiq::Web, at: '/admin/sidekiq', as: :sidekiq
   end
 
+  # Enable Grack support (for LFS only)
+  mount Grack::AuthSpawner, at: '/', constraints: lambda { |request| /[-\/\w\.]+\.git\/(info\/lfs|gitlab-lfs)/.match(request.path_info) }, via: [:get, :post, :put]
+
   # Help
   get 'help'                  => 'help#index'
   get 'help/:category/:file'  => 'help#show', as: :help_page, constraints: { category: /.*/, file: /[^\/\.]+/ }

From 31bc876b7b34fa1785be022e9cffdc601f2192d7 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Thu, 24 Mar 2016 16:14:09 +0100
Subject: [PATCH 04/39] Test both GET and POST for git-upload-pack

---
 config/routes.rb               |   4 +-
 spec/requests/git_http_spec.rb | 112 ++++++++++++++++++++-------------
 2 files changed, 69 insertions(+), 47 deletions(-)

diff --git a/config/routes.rb b/config/routes.rb
index 021eab89c2a..eace7516e91 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -429,8 +429,8 @@ Rails.application.routes.draw do
         # Git HTTP clients ('git clone' etc.)
         scope constraints: { format: /(git|wiki\.git)/ } do
           get '/info/refs', to: 'git_http#info_refs', only: :get
-          get '/git-upload-pack', to: 'git_http#git_upload_pack', only: :post
-          get '/git-receive-pack', to: 'git_http#git_receive_pack', only: :post
+          post '/git-upload-pack', to: 'git_http#git_upload_pack', only: :post
+          post '/git-receive-pack', to: 'git_http#git_receive_pack', only: :post
         end
 
         # Blob routes:
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 7e274b4209b..ef0b83fd475 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -8,26 +8,26 @@ describe 'Git HTTP requests', lib: true do
     context "when the project doesn't exist" do
       context "when no authentication is provided" do
         it "responds with status 401" do
-          clone_get '/doesnt/exist.git/info/refs'
-
-          expect(response.status).to eq(401)
+          download('doesnt/exist.git') do |response|
+            expect(response.status).to eq(401)
+          end
         end
       end
 
       context "when username and password are provided" do
         context "when authentication fails" do
           it "responds with status 401" do
-            clone_get '/doesnt/exist.git/info/refs', user: user.username, password: "nope"
-
-            expect(response.status).to eq(401)
+            download('doesnt/exist.git', user: user.username, password: "nope") do |response|
+              expect(response.status).to eq(401)
+            end
           end
         end
 
         context "when authentication succeeds" do
           it "responds with status 404" do
-            clone_get '/doesnt/exist.git/info/refs', user: user.username, password: user.password
-
-            expect(response.status).to eq(404)
+            download('/doesnt/exist.git', user: user.username, password: user.password) do |response|
+              expect(response.status).to eq(404)
+            end
           end
         end
       end
@@ -38,23 +38,25 @@ describe 'Git HTTP requests', lib: true do
         wiki = ProjectWiki.new(project)
         project.update_attribute(:visibility_level, Project::PUBLIC)
 
-        clone_get "/#{wiki.repository.path_with_namespace}.git/info/refs"
-        json_body = ActiveSupport::JSON.decode(response.body)
-
-        expect(response.status).to eq(200)
-        expect(json_body['RepoPath']).to include(wiki.repository.path_with_namespace)
+        download("/#{wiki.repository.path_with_namespace}.git") do |response|
+          json_body = ActiveSupport::JSON.decode(response.body)
+  
+          expect(response.status).to eq(200)
+          expect(json_body['RepoPath']).to include(wiki.repository.path_with_namespace)
+        end
       end
     end
 
     context "when the project exists" do
-      let(:path) { clone_path(project) }
+      let(:path) { "#{project.path_with_namespace}.git" }
+      let(:env) { {} }
 
       context "when the project is public" do
         it "responds with status 200" do
           project.update_attribute(:visibility_level, Project::PUBLIC)
-          clone_get path
-
-          expect(response.status).to eq(200)
+          download(path, env) do |response|
+            expect(response.status).to eq(200)
+          end
         end
       end
 
@@ -65,33 +67,37 @@ describe 'Git HTTP requests', lib: true do
 
         context "when no authentication is provided" do
           it "responds with status 401" do
-            clone_get path
-
-            expect(response.status).to eq(401)
+            download(path, env) do |response|
+              expect(response.status).to eq(401)
+            end
           end
         end
 
         context "when username and password are provided" do
+          let(:env) { { user: user.username, password: 'nope' } }
+          
           context "when authentication fails" do
             it "responds with status 401" do
-              clone_get path, user: user.username, password: 'nope'
-
-              expect(response.status).to eq(401)
+              download(path, env) do |response|
+                expect(response.status).to eq(401)
+              end
             end
 
             context "when the user is IP banned" do
               it "responds with status 401" do
                 expect(Rack::Attack::Allow2Ban).to receive(:filter).and_return(true)
                 allow_any_instance_of(Rack::Request).to receive(:ip).and_return('1.2.3.4')
-
-                clone_get path, user: user.username, password: 'nope'
-
+                
+                clone_get(path, env)
+              
                 expect(response.status).to eq(401)
               end
             end
           end
 
           context "when authentication succeeds" do
+            let(:env) { { user: user.username, password: user.password } }
+            
             context "when the user has access to the project" do
               before do
                 project.team << [user, :master]
@@ -102,18 +108,18 @@ describe 'Git HTTP requests', lib: true do
                   user.block
                   project.team << [user, :master]
 
-                  clone_get path, user: user.username, password: user.password
-
-                  expect(response.status).to eq(404)
+                  download(path, env) do |response|
+                    expect(response.status).to eq(404)
+                  end
                 end
               end
 
               context "when the user isn't blocked" do
                 it "responds with status 200" do
                   expect(Rack::Attack::Allow2Ban).to receive(:reset)
-
-                  clone_get path, user: user.username, password: user.password
-
+                  
+                  clone_get(path, env)
+                  
                   expect(response.status).to eq(200)
                 end
               end
@@ -151,9 +157,9 @@ describe 'Git HTTP requests', lib: true do
 
             context "when the user doesn't have access to the project" do
               it "responds with status 404" do
-                clone_get path, user: user.username, password: user.password
-
-                expect(response.status).to eq(404)
+                download(path, user: user.username, password: user.password) do |response|
+                  expect(response.status).to eq(404)
+                end
               end
             end
           end
@@ -165,7 +171,7 @@ describe 'Git HTTP requests', lib: true do
             project = FactoryGirl.create :empty_project
             project.update_attributes(runners_token: token, builds_enabled: true)
 
-            clone_get clone_path(project), user: 'gitlab-ci-token', password: token
+            clone_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: token
 
             expect(response.status).to eq(200)
           end
@@ -174,17 +180,33 @@ describe 'Git HTTP requests', lib: true do
     end
   end
 
-  def clone_get(url, user: nil, password: nil)
-    if user && password
-      env = { 'HTTP_AUTHORIZATION' => ActionController::HttpAuthentication::Basic.encode_credentials(user, password) }
-    else
-      env = {}
-    end
-
-    get url, { 'service' => 'git-upload-pack' }, env
+  def clone_get(project, options={})
+    get "/#{project}/info/refs", { service: 'git-upload-pack' }, auth_env(*options.values_at(:user, :password))
+  end
+  
+  def clone_post(project, options={})
+    post "/#{project}/git-upload-pack", {}, auth_env(*options.values_at(:user, :password))
   end
 
   def clone_path(project)
     "/#{project.path_with_namespace}.git/info/refs"
   end
+
+  def download(project, user: nil, password: nil)
+    args = [project, {user: user, password: password}]
+
+    clone_get *args
+    yield response
+    
+    clone_post *args
+    yield response
+  end
+  
+  def auth_env(user, password)
+    if user && password
+      { 'HTTP_AUTHORIZATION' => ActionController::HttpAuthentication::Basic.encode_credentials(user, password) }
+    else
+      {}
+    end
+  end
 end

From 0f8fe93c26f00eac14cbc33e9ed2e2260b7014cc Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Thu, 24 Mar 2016 16:21:19 +0100
Subject: [PATCH 05/39] Whitespace, remove unused method

---
 spec/requests/git_http_spec.rb | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index ef0b83fd475..1e3f3f3e617 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -40,7 +40,7 @@ describe 'Git HTTP requests', lib: true do
 
         download("/#{wiki.repository.path_with_namespace}.git") do |response|
           json_body = ActiveSupport::JSON.decode(response.body)
-  
+
           expect(response.status).to eq(200)
           expect(json_body['RepoPath']).to include(wiki.repository.path_with_namespace)
         end
@@ -75,7 +75,7 @@ describe 'Git HTTP requests', lib: true do
 
         context "when username and password are provided" do
           let(:env) { { user: user.username, password: 'nope' } }
-          
+
           context "when authentication fails" do
             it "responds with status 401" do
               download(path, env) do |response|
@@ -87,9 +87,9 @@ describe 'Git HTTP requests', lib: true do
               it "responds with status 401" do
                 expect(Rack::Attack::Allow2Ban).to receive(:filter).and_return(true)
                 allow_any_instance_of(Rack::Request).to receive(:ip).and_return('1.2.3.4')
-                
+
                 clone_get(path, env)
-              
+
                 expect(response.status).to eq(401)
               end
             end
@@ -97,7 +97,7 @@ describe 'Git HTTP requests', lib: true do
 
           context "when authentication succeeds" do
             let(:env) { { user: user.username, password: user.password } }
-            
+
             context "when the user has access to the project" do
               before do
                 project.team << [user, :master]
@@ -117,9 +117,9 @@ describe 'Git HTTP requests', lib: true do
               context "when the user isn't blocked" do
                 it "responds with status 200" do
                   expect(Rack::Attack::Allow2Ban).to receive(:reset)
-                  
+
                   clone_get(path, env)
-                  
+
                   expect(response.status).to eq(200)
                 end
               end
@@ -183,25 +183,21 @@ describe 'Git HTTP requests', lib: true do
   def clone_get(project, options={})
     get "/#{project}/info/refs", { service: 'git-upload-pack' }, auth_env(*options.values_at(:user, :password))
   end
-  
+
   def clone_post(project, options={})
     post "/#{project}/git-upload-pack", {}, auth_env(*options.values_at(:user, :password))
   end
 
-  def clone_path(project)
-    "/#{project.path_with_namespace}.git/info/refs"
-  end
-
   def download(project, user: nil, password: nil)
     args = [project, {user: user, password: password}]
 
     clone_get *args
     yield response
-    
+
     clone_post *args
     yield response
   end
-  
+
   def auth_env(user, password)
     if user && password
       { 'HTTP_AUTHORIZATION' => ActionController::HttpAuthentication::Basic.encode_credentials(user, password) }

From 1433068ad951045a3440d58b86e9489001ff3774 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Thu, 24 Mar 2016 16:28:18 +0100
Subject: [PATCH 06/39] Remove useles only:

---
 config/routes.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/config/routes.rb b/config/routes.rb
index eace7516e91..40c149abda2 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -428,9 +428,9 @@ Rails.application.routes.draw do
       scope module: :projects do
         # Git HTTP clients ('git clone' etc.)
         scope constraints: { format: /(git|wiki\.git)/ } do
-          get '/info/refs', to: 'git_http#info_refs', only: :get
-          post '/git-upload-pack', to: 'git_http#git_upload_pack', only: :post
-          post '/git-receive-pack', to: 'git_http#git_receive_pack', only: :post
+          get '/info/refs', to: 'git_http#info_refs'
+          post '/git-upload-pack', to: 'git_http#git_upload_pack'
+          post '/git-receive-pack', to: 'git_http#git_receive_pack'
         end
 
         # Blob routes:

From aae577f92141f3ec973b4dd362452502274147f5 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Thu, 24 Mar 2016 17:34:56 +0100
Subject: [PATCH 07/39] Add test for gitlab_shell.upload_pack config setting

---
 spec/requests/git_http_spec.rb | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 1e3f3f3e617..3a6a9b7a70d 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -52,12 +52,25 @@ describe 'Git HTTP requests', lib: true do
       let(:env) { {} }
 
       context "when the project is public" do
-        it "responds with status 200" do
+        before do
           project.update_attribute(:visibility_level, Project::PUBLIC)
+        end
+        
+        it "responds with status 200" do
           download(path, env) do |response|
             expect(response.status).to eq(200)
           end
         end
+        
+        context 'but git-upload-pack is disabled' do
+          it "responds with status 404" do
+            allow(Gitlab.config.gitlab_shell).to receive(:upload_pack).and_return(false)
+
+            download(path, env) do |response|
+              expect(response.status).to eq(404)
+            end
+          end
+        end
       end
 
       context "when the project is private" do

From ccf5b21f28d41e10de450e31d6e8855d1ee2f81e Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Thu, 24 Mar 2016 17:38:30 +0100
Subject: [PATCH 08/39] Remove useless "describe"

---
 spec/requests/git_http_spec.rb | 351 ++++++++++++++++-----------------
 1 file changed, 174 insertions(+), 177 deletions(-)

diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 3a6a9b7a70d..a26b986aeb0 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -4,195 +4,192 @@ describe 'Git HTTP requests', lib: true do
   let(:user)    { create(:user) }
   let(:project) { create(:project) }
 
-  describe "#call" do
-    context "when the project doesn't exist" do
-      context "when no authentication is provided" do
+  context "when the project doesn't exist" do
+    context "when no authentication is provided" do
+      it "responds with status 401" do
+        download('doesnt/exist.git') do |response|
+          expect(response.status).to eq(401)
+        end
+      end
+    end
+
+    context "when username and password are provided" do
+      context "when authentication fails" do
         it "responds with status 401" do
-          download('doesnt/exist.git') do |response|
+          download('doesnt/exist.git', user: user.username, password: "nope") do |response|
             expect(response.status).to eq(401)
           end
         end
       end
 
-      context "when username and password are provided" do
-        context "when authentication fails" do
-          it "responds with status 401" do
-            download('doesnt/exist.git', user: user.username, password: "nope") do |response|
-              expect(response.status).to eq(401)
-            end
-          end
-        end
-
-        context "when authentication succeeds" do
-          it "responds with status 404" do
-            download('/doesnt/exist.git', user: user.username, password: user.password) do |response|
-              expect(response.status).to eq(404)
-            end
-          end
-        end
-      end
-    end
-
-    context "when the Wiki for a project exists" do
-      it "responds with the right project" do
-        wiki = ProjectWiki.new(project)
-        project.update_attribute(:visibility_level, Project::PUBLIC)
-
-        download("/#{wiki.repository.path_with_namespace}.git") do |response|
-          json_body = ActiveSupport::JSON.decode(response.body)
-
-          expect(response.status).to eq(200)
-          expect(json_body['RepoPath']).to include(wiki.repository.path_with_namespace)
-        end
-      end
-    end
-
-    context "when the project exists" do
-      let(:path) { "#{project.path_with_namespace}.git" }
-      let(:env) { {} }
-
-      context "when the project is public" do
-        before do
-          project.update_attribute(:visibility_level, Project::PUBLIC)
-        end
-        
-        it "responds with status 200" do
-          download(path, env) do |response|
-            expect(response.status).to eq(200)
-          end
-        end
-        
-        context 'but git-upload-pack is disabled' do
-          it "responds with status 404" do
-            allow(Gitlab.config.gitlab_shell).to receive(:upload_pack).and_return(false)
-
-            download(path, env) do |response|
-              expect(response.status).to eq(404)
-            end
-          end
-        end
-      end
-
-      context "when the project is private" do
-        before do
-          project.update_attribute(:visibility_level, Project::PRIVATE)
-        end
-
-        context "when no authentication is provided" do
-          it "responds with status 401" do
-            download(path, env) do |response|
-              expect(response.status).to eq(401)
-            end
-          end
-        end
-
-        context "when username and password are provided" do
-          let(:env) { { user: user.username, password: 'nope' } }
-
-          context "when authentication fails" do
-            it "responds with status 401" do
-              download(path, env) do |response|
-                expect(response.status).to eq(401)
-              end
-            end
-
-            context "when the user is IP banned" do
-              it "responds with status 401" do
-                expect(Rack::Attack::Allow2Ban).to receive(:filter).and_return(true)
-                allow_any_instance_of(Rack::Request).to receive(:ip).and_return('1.2.3.4')
-
-                clone_get(path, env)
-
-                expect(response.status).to eq(401)
-              end
-            end
-          end
-
-          context "when authentication succeeds" do
-            let(:env) { { user: user.username, password: user.password } }
-
-            context "when the user has access to the project" do
-              before do
-                project.team << [user, :master]
-              end
-
-              context "when the user is blocked" do
-                it "responds with status 404" do
-                  user.block
-                  project.team << [user, :master]
-
-                  download(path, env) do |response|
-                    expect(response.status).to eq(404)
-                  end
-                end
-              end
-
-              context "when the user isn't blocked" do
-                it "responds with status 200" do
-                  expect(Rack::Attack::Allow2Ban).to receive(:reset)
-
-                  clone_get(path, env)
-
-                  expect(response.status).to eq(200)
-                end
-              end
-
-              context "when blank password attempts follow a valid login" do
-                def attempt_login(include_password)
-                  password = include_password ? user.password : ""
-                  clone_get path, user: user.username, password: password
-                  response.status
-                end
-
-                it "repeated attempts followed by successful attempt" do
-                  options = Gitlab.config.rack_attack.git_basic_auth
-                  maxretry = options[:maxretry] - 1
-                  ip = '1.2.3.4'
-
-                  allow_any_instance_of(Rack::Request).to receive(:ip).and_return(ip)
-                  Rack::Attack::Allow2Ban.reset(ip, options)
-
-                  maxretry.times.each do
-                    expect(attempt_login(false)).to eq(401)
-                  end
-
-                  expect(attempt_login(true)).to eq(200)
-                  expect(Rack::Attack::Allow2Ban.banned?(ip)).to be_falsey
-
-                  maxretry.times.each do
-                    expect(attempt_login(false)).to eq(401)
-                  end
-
-                  Rack::Attack::Allow2Ban.reset(ip, options)
-                end
-              end
-            end
-
-            context "when the user doesn't have access to the project" do
-              it "responds with status 404" do
-                download(path, user: user.username, password: user.password) do |response|
-                  expect(response.status).to eq(404)
-                end
-              end
-            end
-          end
-        end
-
-        context "when a gitlab ci token is provided" do
-          it "responds with status 200" do
-            token = "123"
-            project = FactoryGirl.create :empty_project
-            project.update_attributes(runners_token: token, builds_enabled: true)
-
-            clone_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: token
-
-            expect(response.status).to eq(200)
+      context "when authentication succeeds" do
+        it "responds with status 404" do
+          download('/doesnt/exist.git', user: user.username, password: user.password) do |response|
+            expect(response.status).to eq(404)
           end
         end
       end
     end
   end
 
+  context "when the Wiki for a project exists" do
+    it "responds with the right project" do
+      wiki = ProjectWiki.new(project)
+      project.update_attribute(:visibility_level, Project::PUBLIC)
+
+      download("/#{wiki.repository.path_with_namespace}.git") do |response|
+        json_body = ActiveSupport::JSON.decode(response.body)
+
+        expect(response.status).to eq(200)
+        expect(json_body['RepoPath']).to include(wiki.repository.path_with_namespace)
+      end
+    end
+  end
+
+  context "when the project exists" do
+    let(:path) { "#{project.path_with_namespace}.git" }
+    let(:env) { {} }
+
+    context "when the project is public" do
+      before do
+        project.update_attribute(:visibility_level, Project::PUBLIC)
+      end
+      
+      it "responds with status 200" do
+        download(path, env) do |response|
+          expect(response.status).to eq(200)
+        end
+      end
+      
+      context 'but git-upload-pack is disabled' do
+        it "responds with status 404" do
+          allow(Gitlab.config.gitlab_shell).to receive(:upload_pack).and_return(false)
+
+          download(path, env) do |response|
+            expect(response.status).to eq(404)
+          end
+        end
+      end
+    end
+
+    context "when the project is private" do
+      before do
+        project.update_attribute(:visibility_level, Project::PRIVATE)
+      end
+
+      context "when no authentication is provided" do
+        it "responds with status 401" do
+          download(path, env) do |response|
+            expect(response.status).to eq(401)
+          end
+        end
+      end
+
+      context "when username and password are provided" do
+        let(:env) { { user: user.username, password: 'nope' } }
+
+        context "when authentication fails" do
+          it "responds with status 401" do
+            download(path, env) do |response|
+              expect(response.status).to eq(401)
+            end
+          end
+
+          context "when the user is IP banned" do
+            it "responds with status 401" do
+              expect(Rack::Attack::Allow2Ban).to receive(:filter).and_return(true)
+              allow_any_instance_of(Rack::Request).to receive(:ip).and_return('1.2.3.4')
+
+              clone_get(path, env)
+
+              expect(response.status).to eq(401)
+            end
+          end
+        end
+
+        context "when authentication succeeds" do
+          let(:env) { { user: user.username, password: user.password } }
+
+          context "when the user has access to the project" do
+            before do
+              project.team << [user, :master]
+            end
+
+            context "when the user is blocked" do
+              it "responds with status 404" do
+                user.block
+                project.team << [user, :master]
+
+                download(path, env) do |response|
+                  expect(response.status).to eq(404)
+                end
+              end
+            end
+
+            context "when the user isn't blocked" do
+              it "responds with status 200" do
+                expect(Rack::Attack::Allow2Ban).to receive(:reset)
+
+                clone_get(path, env)
+
+                expect(response.status).to eq(200)
+              end
+            end
+
+            context "when blank password attempts follow a valid login" do
+              def attempt_login(include_password)
+                password = include_password ? user.password : ""
+                clone_get path, user: user.username, password: password
+                response.status
+              end
+
+              it "repeated attempts followed by successful attempt" do
+                options = Gitlab.config.rack_attack.git_basic_auth
+                maxretry = options[:maxretry] - 1
+                ip = '1.2.3.4'
+
+                allow_any_instance_of(Rack::Request).to receive(:ip).and_return(ip)
+                Rack::Attack::Allow2Ban.reset(ip, options)
+
+                maxretry.times.each do
+                  expect(attempt_login(false)).to eq(401)
+                end
+
+                expect(attempt_login(true)).to eq(200)
+                expect(Rack::Attack::Allow2Ban.banned?(ip)).to be_falsey
+
+                maxretry.times.each do
+                  expect(attempt_login(false)).to eq(401)
+                end
+
+                Rack::Attack::Allow2Ban.reset(ip, options)
+              end
+            end
+          end
+
+          context "when the user doesn't have access to the project" do
+            it "responds with status 404" do
+              download(path, user: user.username, password: user.password) do |response|
+                expect(response.status).to eq(404)
+              end
+            end
+          end
+        end
+      end
+
+      context "when a gitlab ci token is provided" do
+        it "responds with status 200" do
+          token = "123"
+          project = FactoryGirl.create :empty_project
+          project.update_attributes(runners_token: token, builds_enabled: true)
+
+          clone_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: token
+
+          expect(response.status).to eq(200)
+        end
+      end
+    end
+  end
   def clone_get(project, options={})
     get "/#{project}/info/refs", { service: 'git-upload-pack' }, auth_env(*options.values_at(:user, :password))
   end

From 57145483fc41cc73b7b41005ebac90779f817b5e Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Thu, 24 Mar 2016 17:44:10 +0100
Subject: [PATCH 09/39] Spec Www-Authenticate

---
 spec/requests/git_http_spec.rb | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index a26b986aeb0..967e0ab6e74 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -4,6 +4,12 @@ describe 'Git HTTP requests', lib: true do
   let(:user)    { create(:user) }
   let(:project) { create(:project) }
 
+  it "gives WWW-Authenticate hints" do
+    clone_get('doesnt/exist.git')
+
+    expect(response.header['WWW-Authenticate']).to start_with('Basic ')
+  end
+
   context "when the project doesn't exist" do
     context "when no authentication is provided" do
       it "responds with status 401" do

From 5f3708418ab71c47c6fffe63b1fac03c0e7c889f Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Thu, 24 Mar 2016 17:44:13 +0100
Subject: [PATCH 10/39] Whitespace!

---
 spec/requests/git_http_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 967e0ab6e74..c1aad48ad04 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -60,13 +60,13 @@ describe 'Git HTTP requests', lib: true do
       before do
         project.update_attribute(:visibility_level, Project::PUBLIC)
       end
-      
+
       it "responds with status 200" do
         download(path, env) do |response|
           expect(response.status).to eq(200)
         end
       end
-      
+
       context 'but git-upload-pack is disabled' do
         it "responds with status 404" do
           allow(Gitlab.config.gitlab_shell).to receive(:upload_pack).and_return(false)

From 5fe06d7365f5552904add8027309d6216954793e Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Thu, 24 Mar 2016 18:58:29 +0100
Subject: [PATCH 11/39] Add some upload specs

---
 .../projects/git_http_controller.rb           | 40 +++++++++---
 spec/requests/git_http_spec.rb                | 63 ++++++++++++++++++-
 2 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 129e87dbf13..a26ab736115 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -5,10 +5,12 @@ class Projects::GitHttpController < Projects::ApplicationController
     
   def git_rpc
     if upload_pack? && upload_pack_allowed?
-      render_ok and return
+      render_ok
+    elsif receive_pack? && receive_pack_allowed?
+      render_ok
+    else
+      render_not_found
     end
-    
-    render_not_found
   end
   
   %i{info_refs git_receive_pack git_upload_pack}.each do |method|
@@ -30,7 +32,7 @@ class Projects::GitHttpController < Projects::ApplicationController
   end
 
   def project_found?
-    render_not_found if project.nil?
+    render_not_found if project.blank?
   end
 
   def ci_request?(login, password)
@@ -124,13 +126,21 @@ class Projects::GitHttpController < Projects::ApplicationController
   end
 
   def upload_pack?
-    if action_name == 'info_refs'
-      params[:service] == 'git-upload-pack'
-    else
-      action_name == 'git_upload_pack'
-    end
+    rpc == 'git-upload-pack'
   end
 
+  def receive_pack?
+    rpc == 'git-receive-pack'
+  end
+
+  def rpc
+    if action_name == 'info_refs'
+      params[:service]
+    else
+      action_name.gsub('_', '-')
+    end
+  end
+    
   def render_ok
     render json: {
       'GL_ID' => Gitlab::ShellEnv.gl_id(@user),
@@ -164,4 +174,16 @@ class Projects::GitHttpController < Projects::ApplicationController
       false
     end
   end
+
+  def receive_pack_allowed?
+    if !Gitlab.config.gitlab_shell.receive_pack
+      false
+    elsif user
+      # Skip user authorization on upload request.
+      # It will be done by the pre-receive hook in the repository.
+      true
+    else
+      false
+    end
+  end
 end
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index c1aad48ad04..1fa14cadc0f 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -61,12 +61,38 @@ describe 'Git HTTP requests', lib: true do
         project.update_attribute(:visibility_level, Project::PUBLIC)
       end
 
-      it "responds with status 200" do
+      it "downloads get status 200" do
         download(path, env) do |response|
           expect(response.status).to eq(200)
         end
       end
 
+      it "uploads get status 401" do
+        upload(path, env) do |response|
+          expect(response.status).to eq(401)
+        end
+      end
+      
+      context "with correct credentials" do
+        let(:env) { { user: user.username, password: user.password } }
+
+        it "uploads get status 200 (because Git hooks do the real check)" do
+          upload(path, env) do |response|
+            expect(response.status).to eq(200)
+          end
+        end
+        
+        context 'but git-receive-pack is disabled' do
+          it "responds with status 404" do
+            allow(Gitlab.config.gitlab_shell).to receive(:receive_pack).and_return(false)
+  
+            upload(path, env) do |response|
+              expect(response.status).to eq(404)
+            end
+          end
+        end
+      end
+
       context 'but git-upload-pack is disabled' do
         it "responds with status 404" do
           allow(Gitlab.config.gitlab_shell).to receive(:upload_pack).and_return(false)
@@ -133,13 +159,19 @@ describe 'Git HTTP requests', lib: true do
             end
 
             context "when the user isn't blocked" do
-              it "responds with status 200" do
+              it "downloads status 200" do
                 expect(Rack::Attack::Allow2Ban).to receive(:reset)
 
                 clone_get(path, env)
 
                 expect(response.status).to eq(200)
               end
+              
+              it "uploads get status 200" do
+                upload(path, env) do |response|
+                  expect(response.status).to eq(200)
+                end      
+              end
             end
 
             context "when blank password attempts follow a valid login" do
@@ -174,11 +206,17 @@ describe 'Git HTTP requests', lib: true do
           end
 
           context "when the user doesn't have access to the project" do
-            it "responds with status 404" do
+            it "downloads get status 404" do
               download(path, user: user.username, password: user.password) do |response|
                 expect(response.status).to eq(404)
               end
             end
+            
+            it "uploads get status 200 (because Git hooks do the real check)" do
+              upload(path, user: user.username, password: user.password) do |response|
+                expect(response.status).to eq(200)
+              end
+            end
           end
         end
       end
@@ -196,6 +234,7 @@ describe 'Git HTTP requests', lib: true do
       end
     end
   end
+
   def clone_get(project, options={})
     get "/#{project}/info/refs", { service: 'git-upload-pack' }, auth_env(*options.values_at(:user, :password))
   end
@@ -204,6 +243,14 @@ describe 'Git HTTP requests', lib: true do
     post "/#{project}/git-upload-pack", {}, auth_env(*options.values_at(:user, :password))
   end
 
+  def push_get(project, options={})
+    get "/#{project}/info/refs", { service: 'git-receive-pack' }, auth_env(*options.values_at(:user, :password))
+  end
+
+  def push_post(project, options={})
+    post "/#{project}/git-receive-pack", {}, auth_env(*options.values_at(:user, :password))
+  end
+
   def download(project, user: nil, password: nil)
     args = [project, {user: user, password: password}]
 
@@ -214,6 +261,16 @@ describe 'Git HTTP requests', lib: true do
     yield response
   end
 
+  def upload(project, user: nil, password: nil)
+    args = [project, {user: user, password: password}]
+
+    push_get *args
+    yield response
+
+    push_post *args
+    yield response
+  end
+
   def auth_env(user, password)
     if user && password
       { 'HTTP_AUTHORIZATION' => ActionController::HttpAuthentication::Basic.encode_credentials(user, password) }

From ac4d3dc5ccba32e026250ab48fe7f29bcf4ddd97 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Wed, 6 Apr 2016 17:23:16 +0200
Subject: [PATCH 12/39] Rubocop

---
 spec/requests/git_http_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 1fa14cadc0f..5d41d973083 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -252,7 +252,7 @@ describe 'Git HTTP requests', lib: true do
   end
 
   def download(project, user: nil, password: nil)
-    args = [project, {user: user, password: password}]
+    args = [project, { user: user, password: password }]
 
     clone_get *args
     yield response
@@ -262,7 +262,7 @@ describe 'Git HTTP requests', lib: true do
   end
 
   def upload(project, user: nil, password: nil)
-    args = [project, {user: user, password: password}]
+    args = [project, { user: user, password: password }]
 
     push_get *args
     yield response

From 6cc6d9730a234c2cc27869f9b9388ab61de9c460 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Wed, 6 Apr 2016 17:27:52 +0200
Subject: [PATCH 13/39] Delete dead code

---
 lib/gitlab/backend/grack_auth.rb | 53 +-------------------------------
 1 file changed, 1 insertion(+), 52 deletions(-)

diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb
index cdcaae8094c..e2363b91265 100644
--- a/lib/gitlab/backend/grack_auth.rb
+++ b/lib/gitlab/backend/grack_auth.rb
@@ -36,10 +36,7 @@ module Grack
       lfs_response = Gitlab::Lfs::Router.new(project, @user, @request).try_call
       return lfs_response unless lfs_response.nil?
 
-      if project && authorized_request?
-        # Tell gitlab-workhorse the request is OK, and what the GL_ID is
-        render_grack_auth_ok
-      elsif @user.nil? && !@ci
+      if @user.nil? && !@ci
         unauthorized
       else
         render_not_found
@@ -141,36 +138,6 @@ module Grack
       user
     end
 
-    def authorized_request?
-      return true if @ci
-
-      case git_cmd
-      when *Gitlab::GitAccess::DOWNLOAD_COMMANDS
-        if !Gitlab.config.gitlab_shell.upload_pack
-          false
-        elsif user
-          Gitlab::GitAccess.new(user, project).download_access_check.allowed?
-        elsif project.public?
-          # Allow clone/fetch for public projects
-          true
-        else
-          false
-        end
-      when *Gitlab::GitAccess::PUSH_COMMANDS
-        if !Gitlab.config.gitlab_shell.receive_pack
-          false
-        elsif user
-          # Skip user authorization on upload request.
-          # It will be done by the pre-receive hook in the repository.
-          true
-        else
-          false
-        end
-      else
-        false
-      end
-    end
-
     def git_cmd
       if @request.get?
         @request.params['service']
@@ -197,24 +164,6 @@ module Grack
       end
     end
 
-    def render_grack_auth_ok
-      repo_path =
-        if @request.path_info =~ /^([\w\.\/-]+)\.wiki\.git/
-          ProjectWiki.new(project).repository.path_to_repo
-        else
-          project.repository.path_to_repo
-        end
-
-      [
-        200,
-        { "Content-Type" => "application/json" },
-        [JSON.dump({
-          'GL_ID' => Gitlab::ShellEnv.gl_id(@user),
-          'RepoPath' => repo_path,
-        })]
-      ]
-    end
-
     def render_not_found
       [404, { "Content-Type" => "text/plain" }, ["Not Found"]]
     end

From 91226c200151461b21e85cc8c85a103c93d6a17f Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Wed, 6 Apr 2016 17:52:12 +0200
Subject: [PATCH 14/39] Move workhorse protocol code into lib

---
 app/controllers/projects/git_http_controller.rb | 13 +++++--------
 lib/gitlab/workhorse.rb                         |  7 +++++++
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index a26ab736115..6dd7a683b0e 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -115,12 +115,12 @@ class Projects::GitHttpController < Projects::ApplicationController
     end
   end
 
-  def repo_path
-    @repo_path ||= begin
+  def repository
+    @repository ||= begin
       if params[:project_id].end_with?('.wiki.git')
-        project.wiki.wiki.path
+        project.wiki.repository
       else
-        repository.path_to_repo
+        project.repository
       end
     end
   end
@@ -142,10 +142,7 @@ class Projects::GitHttpController < Projects::ApplicationController
   end
     
   def render_ok
-    render json: {
-      'GL_ID' => Gitlab::ShellEnv.gl_id(@user),
-      'RepoPath' => repo_path,
-    }
+    render json: Gitlab::Workhorse.git_http_ok(repository, user)
   end
   
   def render_not_found
diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb
index c3ddd4c2680..5b2982e4994 100644
--- a/lib/gitlab/workhorse.rb
+++ b/lib/gitlab/workhorse.rb
@@ -6,6 +6,13 @@ module Gitlab
     SEND_DATA_HEADER = 'Gitlab-Workhorse-Send-Data'
 
     class << self
+      def git_http_ok(repository, user)
+        {
+          'GL_ID' => Gitlab::ShellEnv.gl_id(user),
+          'RepoPath' => repository.path_to_repo,
+        }
+      end
+
       def send_git_blob(repository, blob)
         params = {
           'RepoPath' => repository.path_to_repo,

From ccb29955c9d7de69d99fe91425d6246cc723def4 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Wed, 6 Apr 2016 18:58:19 +0200
Subject: [PATCH 15/39] More tests, better descriptions

---
 spec/requests/git_http_spec.rb | 41 +++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 5d41d973083..8b217684911 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -12,7 +12,7 @@ describe 'Git HTTP requests', lib: true do
 
   context "when the project doesn't exist" do
     context "when no authentication is provided" do
-      it "responds with status 401" do
+      it "responds with status 401 (no project existence information leak)" do
         download('doesnt/exist.git') do |response|
           expect(response.status).to eq(401)
         end
@@ -72,7 +72,7 @@ describe 'Git HTTP requests', lib: true do
           expect(response.status).to eq(401)
         end
       end
-      
+
       context "with correct credentials" do
         let(:env) { { user: user.username, password: user.password } }
 
@@ -81,11 +81,11 @@ describe 'Git HTTP requests', lib: true do
             expect(response.status).to eq(200)
           end
         end
-        
+
         context 'but git-receive-pack is disabled' do
           it "responds with status 404" do
             allow(Gitlab.config.gitlab_shell).to receive(:receive_pack).and_return(false)
-  
+
             upload(path, env) do |response|
               expect(response.status).to eq(404)
             end
@@ -110,11 +110,17 @@ describe 'Git HTTP requests', lib: true do
       end
 
       context "when no authentication is provided" do
-        it "responds with status 401" do
+        it "responds with status 401 to downloads" do
           download(path, env) do |response|
             expect(response.status).to eq(401)
           end
         end
+
+        it "responds with status 401 to uploads" do
+          upload(path, env) do |response|
+            expect(response.status).to eq(401)
+          end
+        end
       end
 
       context "when username and password are provided" do
@@ -159,18 +165,18 @@ describe 'Git HTTP requests', lib: true do
             end
 
             context "when the user isn't blocked" do
-              it "downloads status 200" do
+              it "downloads get status 200" do
                 expect(Rack::Attack::Allow2Ban).to receive(:reset)
 
                 clone_get(path, env)
 
                 expect(response.status).to eq(200)
               end
-              
+
               it "uploads get status 200" do
                 upload(path, env) do |response|
                   expect(response.status).to eq(200)
-                end      
+                end
               end
             end
 
@@ -211,7 +217,7 @@ describe 'Git HTTP requests', lib: true do
                 expect(response.status).to eq(404)
               end
             end
-            
+
             it "uploads get status 200 (because Git hooks do the real check)" do
               upload(path, user: user.username, password: user.password) do |response|
                 expect(response.status).to eq(200)
@@ -222,15 +228,24 @@ describe 'Git HTTP requests', lib: true do
       end
 
       context "when a gitlab ci token is provided" do
-        it "responds with status 200" do
-          token = "123"
-          project = FactoryGirl.create :empty_project
-          project.update_attributes(runners_token: token, builds_enabled: true)
+        let(:token) { 123 }
+        let(:project) { FactoryGirl.create :empty_project }
 
+        before do
+          project.update_attributes(runners_token: token, builds_enabled: true)
+        end
+
+        it "downloads get status 200" do
           clone_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: token
 
           expect(response.status).to eq(200)
         end
+
+        it "uploads get status 401 (no project existence information leak)" do
+          push_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: token
+
+          expect(response.status).to eq(401)
+        end
       end
     end
   end

From ab9dfa8fd681ac558cf988aa2cdb5bd69feea757 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Wed, 6 Apr 2016 19:25:47 +0200
Subject: [PATCH 16/39] Clarify intentions

---
 app/controllers/projects/git_http_controller.rb | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 6dd7a683b0e..11e17510cb9 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -108,11 +108,14 @@ class Projects::GitHttpController < Projects::ApplicationController
     id = params[:project_id]
     return if id.nil?
     
-    if id.end_with?('.wiki.git')
-      id.slice(0, id.length - 9)
-    elsif id.end_with?('.git')
-      id.slice(0, id.length - 4)
+    %w{.wiki.git .git}.each do |suffix|
+      # Be careful to only remove the suffix from the end of 'id'.
+      # Accidentally removing it from the middle is how security
+      # vulnerabilities happen!
+      return id.slice(0, id.length - suffix.length) if id.end_with?(suffix)
     end
+
+    nil
   end
 
   def repository

From e7cea8cd75aa23ad4eb9705ddb0871775d65309b Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Fri, 15 Apr 2016 11:22:08 +0200
Subject: [PATCH 17/39] Avoid path helper name clash

---
 app/controllers/projects/application_controller.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index 275e94d39ed..817727d7868 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -23,8 +23,8 @@ class Projects::ApplicationController < ApplicationController
       @project = find_project
 
       if @project && can?(current_user, :read_project, @project)
-        if @project.path_with_namespace != project_path
-          redirect_to request.original_url.gsub(project_path, @project.path_with_namespace)
+        if @project.path_with_namespace != path_with_namespace
+          redirect_to request.original_url.gsub(path_with_namespace, @project.path_with_namespace)
         end
       else
         @project = nil
@@ -48,12 +48,12 @@ class Projects::ApplicationController < ApplicationController
     params[:namespace_id]
   end
   
-  def project_path
+  def path_with_namespace
     "#{namespace}/#{id}"
   end
   
   def find_project
-    Project.find_with_namespace(project_path)
+    Project.find_with_namespace(path_with_namespace)
   end
 
   def repository

From d3541da4ceaa0f5e2051edd2aa59d4275f93f0f8 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Fri, 15 Apr 2016 12:40:43 +0200
Subject: [PATCH 18/39] Comment and whitespace

---
 .../projects/git_http_controller.rb           | 29 +++++++++++++------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 11e17510cb9..13af17083bd 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -2,7 +2,18 @@ class Projects::GitHttpController < Projects::ApplicationController
   skip_before_action :repository
   before_action :authenticate_user
   before_action :project_found?
-    
+
+  # We support two actions (git push and git pull) which use four
+  # different HTTP requests:
+  #
+  # - GET /foo/bar.git/info/refs?service=git-upload-pack (pull)
+  # - GET /foo/bar.git/info/refs?service=git-receive-pack (push)
+  # - POST /foo/bar.git/git-upload-pack (pull)
+  # - POST /foo/bar.git/git-receive-pack" (push)
+  #
+  # The Rails routes divide these four requests over three methods:
+  # info_refs, git_upload_pack, and git_receive_pack.
+
   def git_rpc
     if upload_pack? && upload_pack_allowed?
       render_ok
@@ -12,7 +23,7 @@ class Projects::GitHttpController < Projects::ApplicationController
       render_not_found
     end
   end
-  
+
   %i{info_refs git_receive_pack git_upload_pack}.each do |method|
     alias_method method, :git_rpc
   end
@@ -60,7 +71,7 @@ class Projects::GitHttpController < Projects::ApplicationController
       token && token.accessible? && User.find_by(id: token.resource_owner_id)
     end
   end
-  
+
   def rate_limit_ip!(login, user)
     # If the user authenticated successfully, we reset the auth failure count
     # from Rack::Attack for that IP. A client may attempt to authenticate
@@ -95,7 +106,7 @@ class Projects::GitHttpController < Projects::ApplicationController
           "as #{login} but has been temporarily banned from Git auth"
       end
     end
-    
+
     user
   end
 
@@ -107,7 +118,7 @@ class Projects::GitHttpController < Projects::ApplicationController
   def id
     id = params[:project_id]
     return if id.nil?
-    
+
     %w{.wiki.git .git}.each do |suffix|
       # Be careful to only remove the suffix from the end of 'id'.
       # Accidentally removing it from the middle is how security
@@ -143,11 +154,11 @@ class Projects::GitHttpController < Projects::ApplicationController
       action_name.gsub('_', '-')
     end
   end
-    
+
   def render_ok
     render json: Gitlab::Workhorse.git_http_ok(repository, user)
   end
-  
+
   def render_not_found
     render text: 'Not Found', status: :not_found
   end
@@ -155,11 +166,11 @@ class Projects::GitHttpController < Projects::ApplicationController
   def ci?
     !!@ci
   end
-  
+
   def user
     @user
   end
-  
+
   def upload_pack_allowed?
     if !Gitlab.config.gitlab_shell.upload_pack
       false

From 9add3fbb3346460934d5990ede1b3216c03e62ee Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Fri, 22 Apr 2016 13:24:53 +0200
Subject: [PATCH 19/39] =?UTF-8?q?Some=20changes=20after=20review=20from=20?=
 =?UTF-8?q?R=C3=A9my=20and=20Valery?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../projects/git_http_controller.rb           | 59 ++++++++++---------
 1 file changed, 32 insertions(+), 27 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 13af17083bd..cd8dd610bcd 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -1,20 +1,11 @@
 class Projects::GitHttpController < Projects::ApplicationController
   skip_before_action :repository
   before_action :authenticate_user
-  before_action :project_found?
+  before_action :ensure_project_found?
 
-  # We support two actions (git push and git pull) which use four
-  # different HTTP requests:
-  #
-  # - GET /foo/bar.git/info/refs?service=git-upload-pack (pull)
-  # - GET /foo/bar.git/info/refs?service=git-receive-pack (push)
-  # - POST /foo/bar.git/git-upload-pack (pull)
-  # - POST /foo/bar.git/git-receive-pack" (push)
-  #
-  # The Rails routes divide these four requests over three methods:
-  # info_refs, git_upload_pack, and git_receive_pack.
-
-  def git_rpc
+  # GET /foo/bar.git/info/refs?service=git-upload-pack (git pull)
+  # GET /foo/bar.git/info/refs?service=git-receive-pack (git push)
+  def info_refs
     if upload_pack? && upload_pack_allowed?
       render_ok
     elsif receive_pack? && receive_pack_allowed?
@@ -24,8 +15,22 @@ class Projects::GitHttpController < Projects::ApplicationController
     end
   end
 
-  %i{info_refs git_receive_pack git_upload_pack}.each do |method|
-    alias_method method, :git_rpc
+  # POST /foo/bar.git/git-upload-pack (git pull)
+  def git_upload_pack
+    if upload_pack? && upload_pack_allowed?
+      render_ok
+    else
+      render_not_found
+    end
+  end
+
+  # POST /foo/bar.git/git-receive-pack" (git push)
+  def git_receive_pack
+    if receive_pack? && receive_pack_allowed?
+      render_ok
+    else
+      render_not_found
+    end
   end
 
   private
@@ -34,7 +39,7 @@ class Projects::GitHttpController < Projects::ApplicationController
     return if project && project.public? && upload_pack?
 
     authenticate_or_request_with_http_basic do |login, password|
-      return @ci = true if ci_request?(login, password)
+      return @ci = true if valid_ci_request?(login, password)
 
       @user = Gitlab::Auth.new.find(login, password)
       @user ||= oauth_access_token_check(login, password)
@@ -42,19 +47,21 @@ class Projects::GitHttpController < Projects::ApplicationController
     end
   end
 
-  def project_found?
+  def ensure_project_found?
     render_not_found if project.blank?
   end
 
-  def ci_request?(login, password)
-    matched_login = /(?<s>^[a-zA-Z]*-ci)-token$/.match(login)
+  def valid_ci_request?(login, password)
+    matched_login = /(?<service>^[a-zA-Z]*-ci)-token$/.match(login)
 
     if project && matched_login.present? && upload_pack?
-      underscored_service = matched_login['s'].underscore
+      underscored_service = matched_login['service'].underscore
 
       if underscored_service == 'gitlab_ci'
         return project && project.valid_build_token?(password)
       elsif Service.available_services_names.include?(underscored_service)
+        # We treat underscored_service as a trusted input because it is included
+        # in the Service.available_services_names whitelist.
         service_method = "#{underscored_service}_service"
         service = project.send(service_method)
 
@@ -126,6 +133,7 @@ class Projects::GitHttpController < Projects::ApplicationController
       return id.slice(0, id.length - suffix.length) if id.end_with?(suffix)
     end
 
+    # No valid id was found.
     nil
   end
 
@@ -140,14 +148,14 @@ class Projects::GitHttpController < Projects::ApplicationController
   end
 
   def upload_pack?
-    rpc == 'git-upload-pack'
+    git_command == 'git-upload-pack'
   end
 
   def receive_pack?
-    rpc == 'git-receive-pack'
+    git_command == 'git-receive-pack'
   end
 
-  def rpc
+  def git_command
     if action_name == 'info_refs'
       params[:service]
     else
@@ -178,11 +186,8 @@ class Projects::GitHttpController < Projects::ApplicationController
       true
     elsif user
       Gitlab::GitAccess.new(user, project).download_access_check.allowed?
-    elsif project.public?
-      # Allow clone/fetch for public projects
-      true
     else
-      false
+      project.public?
     end
   end
 

From c161065e781a2c6d7a3b22954259809ffd7c5b26 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Fri, 22 Apr 2016 13:58:40 +0200
Subject: [PATCH 20/39] Don't mess up our parent controller

---
 .../projects/application_controller.rb        | 26 ++++-----------
 .../projects/git_http_controller.rb           | 32 ++++++++++++-------
 2 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index 817727d7868..74150ad606b 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -10,6 +10,9 @@ class Projects::ApplicationController < ApplicationController
 
   def project
     unless @project
+      namespace = params[:namespace_id]
+      id = params[:project_id] || params[:id]
+
       # Redirect from
       #   localhost/group/project.git
       # to
@@ -20,11 +23,12 @@ class Projects::ApplicationController < ApplicationController
         return
       end
 
-      @project = find_project
+      project_path = "#{namespace}/#{id}"
+      @project = Project.find_with_namespace(project_path)
 
       if @project && can?(current_user, :read_project, @project)
-        if @project.path_with_namespace != path_with_namespace
-          redirect_to request.original_url.gsub(path_with_namespace, @project.path_with_namespace)
+        if @project.path_with_namespace != project_path
+          redirect_to request.original_url.gsub(project_path, @project.path_with_namespace)
         end
       else
         @project = nil
@@ -40,22 +44,6 @@ class Projects::ApplicationController < ApplicationController
     @project
   end
 
-  def id
-    params[:project_id] || params[:id]
-  end
-  
-  def namespace
-    params[:namespace_id]
-  end
-  
-  def path_with_namespace
-    "#{namespace}/#{id}"
-  end
-  
-  def find_project
-    Project.find_with_namespace(path_with_namespace)
-  end
-
   def repository
     @repository ||= project.repository
   end
diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index cd8dd610bcd..e38552218ec 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -119,27 +119,37 @@ class Projects::GitHttpController < Projects::ApplicationController
 
   def project
     return @project if defined?(@project)
-    @project = find_project
+
+    project_id, _ = project_id_with_suffix
+    if project_id.blank?
+      @project = nil
+    else
+      @project = Project.find_with_namespace("#{params[:namespace_id]}/#{project_id}")
+    end
   end
 
-  def id
-    id = params[:project_id]
-    return if id.nil?
+  # This method returns two values so that we can parse
+  # params[:project_id] (untrusted input!) in exactly one place.
+  def project_id_with_suffix
+    id = params[:project_id] || ''
 
     %w{.wiki.git .git}.each do |suffix|
-      # Be careful to only remove the suffix from the end of 'id'.
-      # Accidentally removing it from the middle is how security
-      # vulnerabilities happen!
-      return id.slice(0, id.length - suffix.length) if id.end_with?(suffix)
+      if id.end_with?(suffix)
+        # Be careful to only remove the suffix from the end of 'id'.
+        # Accidentally removing it from the middle is how security
+        # vulnerabilities happen!
+        return [id.slice(0, id.length - suffix.length), suffix]
+      end
     end
 
-    # No valid id was found.
-    nil
+    # Something is wrong with params[:project_id]; do not pass it on.
+    [nil, nil]
   end
 
   def repository
     @repository ||= begin
-      if params[:project_id].end_with?('.wiki.git')
+      _, suffix = project_id_with_suffix
+      if suffix == '.wiki.git'
         project.wiki.repository
       else
         project.repository

From b64cbaccbe297c82b5af0dac94b491f86b17ddd3 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Fri, 22 Apr 2016 14:04:36 +0200
Subject: [PATCH 21/39] Remove trivial 'let'

---
 spec/requests/git_http_spec.rb | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 8b217684911..20c7357cba5 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -54,7 +54,6 @@ describe 'Git HTTP requests', lib: true do
 
   context "when the project exists" do
     let(:path) { "#{project.path_with_namespace}.git" }
-    let(:env) { {} }
 
     context "when the project is public" do
       before do
@@ -62,13 +61,13 @@ describe 'Git HTTP requests', lib: true do
       end
 
       it "downloads get status 200" do
-        download(path, env) do |response|
+        download(path, {}) do |response|
           expect(response.status).to eq(200)
         end
       end
 
       it "uploads get status 401" do
-        upload(path, env) do |response|
+        upload(path, {}) do |response|
           expect(response.status).to eq(401)
         end
       end
@@ -97,7 +96,7 @@ describe 'Git HTTP requests', lib: true do
         it "responds with status 404" do
           allow(Gitlab.config.gitlab_shell).to receive(:upload_pack).and_return(false)
 
-          download(path, env) do |response|
+          download(path, {}) do |response|
             expect(response.status).to eq(404)
           end
         end
@@ -111,13 +110,13 @@ describe 'Git HTTP requests', lib: true do
 
       context "when no authentication is provided" do
         it "responds with status 401 to downloads" do
-          download(path, env) do |response|
+          download(path, {}) do |response|
             expect(response.status).to eq(401)
           end
         end
 
         it "responds with status 401 to uploads" do
-          upload(path, env) do |response|
+          upload(path, {}) do |response|
             expect(response.status).to eq(401)
           end
         end

From d698d3e846c83f49cd363291dd811220c338c8e9 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 25 Apr 2016 18:05:05 +0200
Subject: [PATCH 22/39] =?UTF-8?q?More=20changes=20suggested=20by=20R=C3=A9?=
 =?UTF-8?q?my?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../projects/git_http_controller.rb           | 38 +++++++++----------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index e38552218ec..fafd9e445b5 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -1,7 +1,9 @@
 class Projects::GitHttpController < Projects::ApplicationController
+  attr_reader :user
+
   skip_before_action :repository
   before_action :authenticate_user
-  before_action :ensure_project_found?
+  before_action :ensure_project_found!
 
   # GET /foo/bar.git/info/refs?service=git-upload-pack (git pull)
   # GET /foo/bar.git/info/refs?service=git-receive-pack (git push)
@@ -47,29 +49,29 @@ class Projects::GitHttpController < Projects::ApplicationController
     end
   end
 
-  def ensure_project_found?
+  def ensure_project_found!
     render_not_found if project.blank?
   end
 
   def valid_ci_request?(login, password)
     matched_login = /(?<service>^[a-zA-Z]*-ci)-token$/.match(login)
 
-    if project && matched_login.present? && upload_pack?
-      underscored_service = matched_login['service'].underscore
-
-      if underscored_service == 'gitlab_ci'
-        return project && project.valid_build_token?(password)
-      elsif Service.available_services_names.include?(underscored_service)
-        # We treat underscored_service as a trusted input because it is included
-        # in the Service.available_services_names whitelist.
-        service_method = "#{underscored_service}_service"
-        service = project.send(service_method)
-
-        return service && service.activated? && service.valid_token?(password)
-      end
+    unless project && matched_login.present? && upload_pack?
+      return false
     end
 
-    false
+    underscored_service = matched_login['service'].underscore
+
+    if underscored_service == 'gitlab_ci'
+      project && project.valid_build_token?(password)
+    elsif Service.available_services_names.include?(underscored_service)
+      # We treat underscored_service as a trusted input because it is included
+      # in the Service.available_services_names whitelist.
+      service_method = "#{underscored_service}_service"
+      service = project.send(service_method)
+
+      service && service.activated? && service.valid_token?(password)
+    end
   end
 
   def oauth_access_token_check(login, password)
@@ -185,10 +187,6 @@ class Projects::GitHttpController < Projects::ApplicationController
     !!@ci
   end
 
-  def user
-    @user
-  end
-
   def upload_pack_allowed?
     if !Gitlab.config.gitlab_shell.upload_pack
       false

From 9ef50db6279d722caed1ab1e4576275428e6a94f Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Fri, 29 Apr 2016 18:56:53 +0200
Subject: [PATCH 23/39] Specify that oauth cannot push code

---
 spec/requests/git_http_spec.rb | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 20c7357cba5..14d126480a3 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -179,6 +179,25 @@ describe 'Git HTTP requests', lib: true do
               end
             end
 
+            context "when an oauth token is provided" do
+              before do
+                application = Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user)
+                @token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id)
+              end
+
+              it "downloads get status 200" do
+                clone_get "#{project.path_with_namespace}.git", user: 'oauth2', password: @token.token
+
+                expect(response.status).to eq(200)
+              end
+
+              it "uploads get status 401 (no project existence information leak)" do
+                push_get "#{project.path_with_namespace}.git", user: 'oauth2', password: @token.token
+
+                expect(response.status).to eq(401)
+              end
+            end
+
             context "when blank password attempts follow a valid login" do
               def attempt_login(include_password)
                 password = include_password ? user.password : ""

From b1ffc9f0fee16251899e5a2efbc78c4781ef4902 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Fri, 29 Apr 2016 18:58:55 +0200
Subject: [PATCH 24/39] Make CI/Oauth/rate limiting reusable

---
 .../projects/git_http_controller.rb           |  78 ++-----------
 config/initializers/doorkeeper.rb             |   2 +-
 lib/api/session.rb                            |   8 +-
 lib/gitlab/auth.rb                            | 103 ++++++++++++++++--
 lib/gitlab/backend/grack_auth.rb              |   2 +-
 spec/lib/gitlab/auth_spec.rb                  |  56 ++++++++--
 6 files changed, 156 insertions(+), 93 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index fafd9e445b5..16a85d6f62b 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -41,11 +41,15 @@ class Projects::GitHttpController < Projects::ApplicationController
     return if project && project.public? && upload_pack?
 
     authenticate_or_request_with_http_basic do |login, password|
-      return @ci = true if valid_ci_request?(login, password)
+      user, type = Gitlab::Auth.find(login, password, project: project, ip: request.ip)
 
-      @user = Gitlab::Auth.new.find(login, password)
-      @user ||= oauth_access_token_check(login, password)
-      rate_limit_ip!(login, @user)
+      if (type == :ci) && upload_pack?
+        @ci = true
+      elsif (type == :oauth) && !upload_pack?
+        @user = nil
+      else
+        @user = user
+      end
     end
   end
 
@@ -53,72 +57,6 @@ class Projects::GitHttpController < Projects::ApplicationController
     render_not_found if project.blank?
   end
 
-  def valid_ci_request?(login, password)
-    matched_login = /(?<service>^[a-zA-Z]*-ci)-token$/.match(login)
-
-    unless project && matched_login.present? && upload_pack?
-      return false
-    end
-
-    underscored_service = matched_login['service'].underscore
-
-    if underscored_service == 'gitlab_ci'
-      project && project.valid_build_token?(password)
-    elsif Service.available_services_names.include?(underscored_service)
-      # We treat underscored_service as a trusted input because it is included
-      # in the Service.available_services_names whitelist.
-      service_method = "#{underscored_service}_service"
-      service = project.send(service_method)
-
-      service && service.activated? && service.valid_token?(password)
-    end
-  end
-
-  def oauth_access_token_check(login, password)
-    if login == "oauth2" && upload_pack? && password.present?
-      token = Doorkeeper::AccessToken.by_token(password)
-      token && token.accessible? && User.find_by(id: token.resource_owner_id)
-    end
-  end
-
-  def rate_limit_ip!(login, user)
-    # If the user authenticated successfully, we reset the auth failure count
-    # from Rack::Attack for that IP. A client may attempt to authenticate
-    # with a username and blank password first, and only after it receives
-    # a 401 error does it present a password. Resetting the count prevents
-    # false positives from occurring.
-    #
-    # Otherwise, we let Rack::Attack know there was a failed authentication
-    # attempt from this IP. This information is stored in the Rails cache
-    # (Redis) and will be used by the Rack::Attack middleware to decide
-    # whether to block requests from this IP.
-
-    config = Gitlab.config.rack_attack.git_basic_auth
-    return user unless config.enabled
-
-    if user
-      # A successful login will reset the auth failure count from this IP
-      Rack::Attack::Allow2Ban.reset(request.ip, config)
-    else
-      banned = Rack::Attack::Allow2Ban.filter(request.ip, config) do
-        # Unless the IP is whitelisted, return true so that Allow2Ban
-        # increments the counter (stored in Rails.cache) for the IP
-        if config.ip_whitelist.include?(request.ip)
-          false
-        else
-          true
-        end
-      end
-
-      if banned
-        Rails.logger.info "IP #{request.ip} failed to login " \
-          "as #{login} but has been temporarily banned from Git auth"
-      end
-    end
-
-    user
-  end
-
   def project
     return @project if defined?(@project)
 
diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb
index 66ac88e9f4a..0c694e0d37a 100644
--- a/config/initializers/doorkeeper.rb
+++ b/config/initializers/doorkeeper.rb
@@ -12,7 +12,7 @@ Doorkeeper.configure do
   end
 
   resource_owner_from_credentials do |routes|
-    Gitlab::Auth.new.find(params[:username], params[:password])
+    Gitlab::Auth.find_by_master_or_ldap(params[:username], params[:password])
   end
 
   # If you want to restrict access to the web interface for adding oauth authorized applications, you need to declare the block below.
diff --git a/lib/api/session.rb b/lib/api/session.rb
index cc646895914..e308ccc3004 100644
--- a/lib/api/session.rb
+++ b/lib/api/session.rb
@@ -11,8 +11,12 @@ module API
     # Example Request:
     #  POST /session
     post "/session" do
-      auth = Gitlab::Auth.new
-      user = auth.find(params[:email] || params[:login], params[:password])
+      user, _ = Gitlab::Auth.find(
+        params[:email] || params[:login],
+        params[:password],
+        project: nil,
+        ip: request.ip
+      )
 
       return unauthorized! unless user
       present user, with: Entities::UserLogin
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 30509528b8b..32e903905ad 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -1,17 +1,100 @@
 module Gitlab
   class Auth
-    def find(login, password)
-      user = User.by_login(login)
+    class << self
+      def find(login, password, project:, ip:)
+        raise "Must provide an IP for rate limiting" if ip.nil?
 
-      # If no user is found, or it's an LDAP server, try LDAP.
-      #   LDAP users are only authenticated via LDAP
-      if user.nil? || user.ldap_user?
-        # Second chance - try LDAP authentication
-        return nil unless Gitlab::LDAP::Config.enabled?
+        user, type = nil, nil
 
-        Gitlab::LDAP::Authentication.login(login, password)
-      else
-        user if user.valid_password?(password)
+        if valid_ci_request?(login, password, project)
+          type = :ci
+        elsif user = find_by_master_or_ldap(login, password)
+          type = :master_or_ldap
+        elsif user = oauth_access_token_check(login, password)
+          type = :oauth
+        end
+
+        rate_limit!(ip, success: !!user || (type == :ci), login: login)
+        [user, type]
+      end
+
+      def find_by_master_or_ldap(login, password)
+        user = User.by_login(login)
+
+        # If no user is found, or it's an LDAP server, try LDAP.
+        #   LDAP users are only authenticated via LDAP
+        if user.nil? || user.ldap_user?
+          # Second chance - try LDAP authentication
+          return nil unless Gitlab::LDAP::Config.enabled?
+
+          Gitlab::LDAP::Authentication.login(login, password)
+        else
+          user if user.valid_password?(password)
+        end
+      end
+
+      private
+
+      def valid_ci_request?(login, password, project)
+        matched_login = /(?<service>^[a-zA-Z]*-ci)-token$/.match(login)
+
+        return false unless project && matched_login.present?
+
+        underscored_service = matched_login['service'].underscore
+
+        if underscored_service == 'gitlab_ci'
+          project && project.valid_build_token?(password)
+        elsif Service.available_services_names.include?(underscored_service)
+          # We treat underscored_service as a trusted input because it is included
+          # in the Service.available_services_names whitelist.
+          service_method = "#{underscored_service}_service"
+          service = project.send(service_method)
+
+          service && service.activated? && service.valid_token?(password)
+        end
+      end
+
+      def oauth_access_token_check(login, password)
+        if login == "oauth2" && password.present?
+          token = Doorkeeper::AccessToken.by_token(password)
+          token && token.accessible? && User.find_by(id: token.resource_owner_id)
+        end
+      end
+
+      def rate_limit!(ip, success:, login:)
+        # If the user authenticated successfully, we reset the auth failure count
+        # from Rack::Attack for that IP. A client may attempt to authenticate
+        # with a username and blank password first, and only after it receives
+        # a 401 error does it present a password. Resetting the count prevents
+        # false positives from occurring.
+        #
+        # Otherwise, we let Rack::Attack know there was a failed authentication
+        # attempt from this IP. This information is stored in the Rails cache
+        # (Redis) and will be used by the Rack::Attack middleware to decide
+        # whether to block requests from this IP.
+
+        config = Gitlab.config.rack_attack.git_basic_auth
+        return unless config.enabled
+
+        if success
+          # A successful login will reset the auth failure count from this IP
+          Rack::Attack::Allow2Ban.reset(ip, config)
+        else
+          banned = Rack::Attack::Allow2Ban.filter(ip, config) do
+            # Unless the IP is whitelisted, return true so that Allow2Ban
+            # increments the counter (stored in Rails.cache) for the IP
+            if config.ip_whitelist.include?(ip)
+              false
+            else
+              true
+            end
+          end
+
+          if banned
+            Rails.logger.info "IP #{ip} failed to login " \
+              "as #{login} but has been temporarily banned from Git auth"
+          end
+        end
       end
     end
   end
diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb
index e2363b91265..b263a27d4d3 100644
--- a/lib/gitlab/backend/grack_auth.rb
+++ b/lib/gitlab/backend/grack_auth.rb
@@ -95,7 +95,7 @@ module Grack
     end
 
     def authenticate_user(login, password)
-      user = Gitlab::Auth.new.find(login, password)
+      user, _ = Gitlab::Auth.new.find_by_master_or_ldap(login, password)
 
       unless user
         user = oauth_access_token_check(login, password)
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index aad291c03cd..2c2f7ed0665 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -1,9 +1,47 @@
 require 'spec_helper'
 
 describe Gitlab::Auth, lib: true do
-  let(:gl_auth) { Gitlab::Auth.new }
+  let(:gl_auth) { described_class }
 
-  describe :find do
+  describe 'find' do
+    it 'recognizes CI' do
+      token = '123'
+      project = create(:empty_project)
+      project.update_attributes(runners_token: token, builds_enabled: true)
+      ip = 'ip'
+
+      expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'gitlab-ci-token')
+      expect(gl_auth.find('gitlab-ci-token', token, project: project, ip: ip)).to eq([nil, :ci])
+    end
+
+    it 'recognizes master passwords' do
+      user = create(:user, password: 'password')
+      ip = 'ip'
+
+      expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username)
+      expect(gl_auth.find(user.username, 'password', project: nil, ip: ip)).to eq([user, :master_or_ldap])
+    end
+
+    it 'recognizes OAuth tokens' do
+      user = create(:user)
+      application = Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user)
+      token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id)
+      ip = 'ip'
+
+      expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'oauth2')
+      expect(gl_auth.find("oauth2", token.token, project: nil, ip: ip)).to eq([user, :oauth])
+    end
+
+    it 'returns double nil for invalid credentials' do
+      login = 'foo'
+      ip = 'ip'
+
+      expect(gl_auth).to receive(:rate_limit!).with(ip, success: false, login: login)
+      expect(gl_auth.find(login, 'bar', project: nil, ip: ip)).to eq ([nil, nil])
+    end
+  end
+
+  describe 'find_by_master_or_ldap' do
     let!(:user) do
       create(:user,
         username: username,
@@ -14,25 +52,25 @@ describe Gitlab::Auth, lib: true do
     let(:password) { 'my-secret' }
 
     it "should find user by valid login/password" do
-      expect( gl_auth.find(username, password) ).to eql user
+      expect( gl_auth.find_by_master_or_ldap(username, password) ).to eql user
     end
 
     it 'should find user by valid email/password with case-insensitive email' do
-      expect(gl_auth.find(user.email.upcase, password)).to eql user
+      expect(gl_auth.find_by_master_or_ldap(user.email.upcase, password)).to eql user
     end
 
     it 'should find user by valid username/password with case-insensitive username' do
-      expect(gl_auth.find(username.upcase, password)).to eql user
+      expect(gl_auth.find_by_master_or_ldap(username.upcase, password)).to eql user
     end
 
     it "should not find user with invalid password" do
       password = 'wrong'
-      expect( gl_auth.find(username, password) ).not_to eql user
+      expect( gl_auth.find_by_master_or_ldap(username, password) ).not_to eql user
     end
 
     it "should not find user with invalid login" do
       user = 'wrong'
-      expect( gl_auth.find(username, password) ).not_to eql user
+      expect( gl_auth.find_by_master_or_ldap(username, password) ).not_to eql user
     end
 
     context "with ldap enabled" do
@@ -43,13 +81,13 @@ describe Gitlab::Auth, lib: true do
       it "tries to autheticate with db before ldap" do
         expect(Gitlab::LDAP::Authentication).not_to receive(:login)
 
-        gl_auth.find(username, password)
+        gl_auth.find_by_master_or_ldap(username, password)
       end
 
       it "uses ldap as fallback to for authentication" do
         expect(Gitlab::LDAP::Authentication).to receive(:login)
 
-        gl_auth.find('ldap_user', 'password')
+        gl_auth.find_by_master_or_ldap('ldap_user', 'password')
       end
     end
   end

From d1f5019511a1dc630e97f99bdb1f6b9fe6b02bba Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 2 May 2016 13:19:39 +0200
Subject: [PATCH 25/39] Use correct auth finder

---
 lib/api/session.rb               | 7 +------
 lib/gitlab/backend/grack_auth.rb | 2 +-
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/lib/api/session.rb b/lib/api/session.rb
index e308ccc3004..1156aab8cc2 100644
--- a/lib/api/session.rb
+++ b/lib/api/session.rb
@@ -11,12 +11,7 @@ module API
     # Example Request:
     #  POST /session
     post "/session" do
-      user, _ = Gitlab::Auth.find(
-        params[:email] || params[:login],
-        params[:password],
-        project: nil,
-        ip: request.ip
-      )
+      user = Gitlab::Auth.find_by_master_or_ldap(params[:email] || params[:login], params[:password])
 
       return unauthorized! unless user
       present user, with: Entities::UserLogin
diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb
index b263a27d4d3..3462c2dcfbc 100644
--- a/lib/gitlab/backend/grack_auth.rb
+++ b/lib/gitlab/backend/grack_auth.rb
@@ -95,7 +95,7 @@ module Grack
     end
 
     def authenticate_user(login, password)
-      user, _ = Gitlab::Auth.new.find_by_master_or_ldap(login, password)
+      user = Gitlab::Auth.new.find_by_master_or_ldap(login, password)
 
       unless user
         user = oauth_access_token_check(login, password)

From 9ce099429972726da22253407d98ae8aa1ef167b Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 2 May 2016 13:21:59 +0200
Subject: [PATCH 26/39] Rubocop and whitespace

---
 lib/gitlab/workhorse.rb      | 4 ++--
 spec/lib/gitlab/auth_spec.rb | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb
index 5b2982e4994..f9ceee142d7 100644
--- a/lib/gitlab/workhorse.rb
+++ b/lib/gitlab/workhorse.rb
@@ -36,9 +36,9 @@ module Gitlab
           "git-archive:#{encode(params)}",
         ]
       end
-      
+
       protected
-      
+
       def encode(hash)
         Base64.urlsafe_encode64(JSON.dump(hash))
       end
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 2c2f7ed0665..16083f90bb4 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -37,7 +37,7 @@ describe Gitlab::Auth, lib: true do
       ip = 'ip'
 
       expect(gl_auth).to receive(:rate_limit!).with(ip, success: false, login: login)
-      expect(gl_auth.find(login, 'bar', project: nil, ip: ip)).to eq ([nil, nil])
+      expect(gl_auth.find(login, 'bar', project: nil, ip: ip)).to eq([nil, nil])
     end
   end
 

From 3dc276b367fe88c3c1026371d275d6078611f625 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Tue, 3 May 2016 11:46:14 +0200
Subject: [PATCH 27/39] Remove parallel assignment

---
 lib/gitlab/auth.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 32e903905ad..0479006f993 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -4,7 +4,8 @@ module Gitlab
       def find(login, password, project:, ip:)
         raise "Must provide an IP for rate limiting" if ip.nil?
 
-        user, type = nil, nil
+        user = nil
+        type = nil
 
         if valid_ci_request?(login, password, project)
           type = :ci

From fea591e5c5796235d28eeec4d27759f87fa9d8e2 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Thu, 2 Jun 2016 13:42:18 +0200
Subject: [PATCH 28/39] Rename finder to find_in_gitlab_or_ldap

---
 config/initializers/doorkeeper.rb |  2 +-
 lib/api/session.rb                |  2 +-
 lib/gitlab/auth.rb                |  4 ++--
 lib/gitlab/backend/grack_auth.rb  |  2 +-
 spec/lib/gitlab/auth_spec.rb      | 16 ++++++++--------
 5 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb
index aae2ee3193d..8dc8e270afc 100644
--- a/config/initializers/doorkeeper.rb
+++ b/config/initializers/doorkeeper.rb
@@ -12,7 +12,7 @@ Doorkeeper.configure do
   end
 
   resource_owner_from_credentials do |routes|
-    Gitlab::Auth.find_by_master_or_ldap(params[:username], params[:password])
+    Gitlab::Auth.find_in_gitlab_or_ldap(params[:username], params[:password])
   end
 
   # If you want to restrict access to the web interface for adding oauth authorized applications, you need to declare the block below.
diff --git a/lib/api/session.rb b/lib/api/session.rb
index 1156aab8cc2..56e69b2366f 100644
--- a/lib/api/session.rb
+++ b/lib/api/session.rb
@@ -11,7 +11,7 @@ module API
     # Example Request:
     #  POST /session
     post "/session" do
-      user = Gitlab::Auth.find_by_master_or_ldap(params[:email] || params[:login], params[:password])
+      user = Gitlab::Auth.find_in_gitlab_or_ldap(params[:email] || params[:login], params[:password])
 
       return unauthorized! unless user
       present user, with: Entities::UserLogin
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 0479006f993..d156fa2978d 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -9,7 +9,7 @@ module Gitlab
 
         if valid_ci_request?(login, password, project)
           type = :ci
-        elsif user = find_by_master_or_ldap(login, password)
+        elsif user = find_in_gitlab_or_ldap(login, password)
           type = :master_or_ldap
         elsif user = oauth_access_token_check(login, password)
           type = :oauth
@@ -19,7 +19,7 @@ module Gitlab
         [user, type]
       end
 
-      def find_by_master_or_ldap(login, password)
+      def find_in_gitlab_or_ldap(login, password)
         user = User.by_login(login)
 
         # If no user is found, or it's an LDAP server, try LDAP.
diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb
index 3462c2dcfbc..492ffb138a3 100644
--- a/lib/gitlab/backend/grack_auth.rb
+++ b/lib/gitlab/backend/grack_auth.rb
@@ -95,7 +95,7 @@ module Grack
     end
 
     def authenticate_user(login, password)
-      user = Gitlab::Auth.new.find_by_master_or_ldap(login, password)
+      user = Gitlab::Auth.new.find_in_gitlab_or_ldap(login, password)
 
       unless user
         user = oauth_access_token_check(login, password)
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 16083f90bb4..3c41c4b0681 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -41,7 +41,7 @@ describe Gitlab::Auth, lib: true do
     end
   end
 
-  describe 'find_by_master_or_ldap' do
+  describe 'find_in_gitlab_or_ldap' do
     let!(:user) do
       create(:user,
         username: username,
@@ -52,25 +52,25 @@ describe Gitlab::Auth, lib: true do
     let(:password) { 'my-secret' }
 
     it "should find user by valid login/password" do
-      expect( gl_auth.find_by_master_or_ldap(username, password) ).to eql user
+      expect( gl_auth.find_in_gitlab_or_ldap(username, password) ).to eql user
     end
 
     it 'should find user by valid email/password with case-insensitive email' do
-      expect(gl_auth.find_by_master_or_ldap(user.email.upcase, password)).to eql user
+      expect(gl_auth.find_in_gitlab_or_ldap(user.email.upcase, password)).to eql user
     end
 
     it 'should find user by valid username/password with case-insensitive username' do
-      expect(gl_auth.find_by_master_or_ldap(username.upcase, password)).to eql user
+      expect(gl_auth.find_in_gitlab_or_ldap(username.upcase, password)).to eql user
     end
 
     it "should not find user with invalid password" do
       password = 'wrong'
-      expect( gl_auth.find_by_master_or_ldap(username, password) ).not_to eql user
+      expect( gl_auth.find_in_gitlab_or_ldap(username, password) ).not_to eql user
     end
 
     it "should not find user with invalid login" do
       user = 'wrong'
-      expect( gl_auth.find_by_master_or_ldap(username, password) ).not_to eql user
+      expect( gl_auth.find_in_gitlab_or_ldap(username, password) ).not_to eql user
     end
 
     context "with ldap enabled" do
@@ -81,13 +81,13 @@ describe Gitlab::Auth, lib: true do
       it "tries to autheticate with db before ldap" do
         expect(Gitlab::LDAP::Authentication).not_to receive(:login)
 
-        gl_auth.find_by_master_or_ldap(username, password)
+        gl_auth.find_in_gitlab_or_ldap(username, password)
       end
 
       it "uses ldap as fallback to for authentication" do
         expect(Gitlab::LDAP::Authentication).to receive(:login)
 
-        gl_auth.find_by_master_or_ldap('ldap_user', 'password')
+        gl_auth.find_in_gitlab_or_ldap('ldap_user', 'password')
       end
     end
   end

From 3ffa494ffe06105d6e36a46df52e8a842be0ab69 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Fri, 3 Jun 2016 14:57:34 +0200
Subject: [PATCH 29/39] =?UTF-8?q?Changes=20after=20more=20review=20from=20?=
 =?UTF-8?q?R=C3=A9my?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../projects/git_http_controller.rb           | 16 +++++++-----
 lib/gitlab/auth.rb                            | 26 +++++++++----------
 spec/lib/gitlab/auth_spec.rb                  |  8 +++---
 3 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 16a85d6f62b..5dfa10d218e 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -41,15 +41,17 @@ class Projects::GitHttpController < Projects::ApplicationController
     return if project && project.public? && upload_pack?
 
     authenticate_or_request_with_http_basic do |login, password|
-      user, type = Gitlab::Auth.find(login, password, project: project, ip: request.ip)
+      auth_result = Gitlab::Auth.find(login, password, project: project, ip: request.ip)
 
-      if (type == :ci) && upload_pack?
+      if auth_result.type == :ci && upload_pack?
         @ci = true
-      elsif (type == :oauth) && !upload_pack?
-        @user = nil
+      elsif auth_result.type == :oauth && !upload_pack?
+        # Not allowed
       else
-        @user = user
+        @user = auth_result.user
       end
+
+      ci? || user
     end
   end
 
@@ -73,7 +75,7 @@ class Projects::GitHttpController < Projects::ApplicationController
   def project_id_with_suffix
     id = params[:project_id] || ''
 
-    %w{.wiki.git .git}.each do |suffix|
+    %w[.wiki.git .git].each do |suffix|
       if id.end_with?(suffix)
         # Be careful to only remove the suffix from the end of 'id'.
         # Accidentally removing it from the middle is how security
@@ -109,7 +111,7 @@ class Projects::GitHttpController < Projects::ApplicationController
     if action_name == 'info_refs'
       params[:service]
     else
-      action_name.gsub('_', '-')
+      action_name.dasherize
     end
   end
 
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index d156fa2978d..672642ebfbe 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -1,22 +1,23 @@
 module Gitlab
   class Auth
+    Result = Struct.new(:user, :type)
+
     class << self
       def find(login, password, project:, ip:)
         raise "Must provide an IP for rate limiting" if ip.nil?
 
-        user = nil
-        type = nil
+        result = Result.new
 
         if valid_ci_request?(login, password, project)
-          type = :ci
-        elsif user = find_in_gitlab_or_ldap(login, password)
-          type = :master_or_ldap
-        elsif user = oauth_access_token_check(login, password)
-          type = :oauth
+          result.type = :ci
+        elsif result.user = find_in_gitlab_or_ldap(login, password)
+          result.type = :gitlab_or_ldap
+        elsif result.user = oauth_access_token_check(login, password)
+          result.type = :oauth
         end
 
-        rate_limit!(ip, success: !!user || (type == :ci), login: login)
-        [user, type]
+        rate_limit!(ip, success: !!result.user || (result.type == :ci), login: login)
+        result
       end
 
       def find_in_gitlab_or_ldap(login, password)
@@ -67,7 +68,7 @@ module Gitlab
         # from Rack::Attack for that IP. A client may attempt to authenticate
         # with a username and blank password first, and only after it receives
         # a 401 error does it present a password. Resetting the count prevents
-        # false positives from occurring.
+        # false positives.
         #
         # Otherwise, we let Rack::Attack know there was a failed authentication
         # attempt from this IP. This information is stored in the Rails cache
@@ -78,15 +79,14 @@ module Gitlab
         return unless config.enabled
 
         if success
-          # A successful login will reset the auth failure count from this IP
           Rack::Attack::Allow2Ban.reset(ip, config)
         else
           banned = Rack::Attack::Allow2Ban.filter(ip, config) do
-            # Unless the IP is whitelisted, return true so that Allow2Ban
-            # increments the counter (stored in Rails.cache) for the IP
             if config.ip_whitelist.include?(ip)
+              # Don't increment the ban counter for this IP
               false
             else
+              # Increment the ban counter for this IP
               true
             end
           end
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 3c41c4b0681..a814ad2a4e7 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -11,7 +11,7 @@ describe Gitlab::Auth, lib: true do
       ip = 'ip'
 
       expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'gitlab-ci-token')
-      expect(gl_auth.find('gitlab-ci-token', token, project: project, ip: ip)).to eq([nil, :ci])
+      expect(gl_auth.find('gitlab-ci-token', token, project: project, ip: ip)).to eq(Gitlab::Auth::Result.new(nil, :ci))
     end
 
     it 'recognizes master passwords' do
@@ -19,7 +19,7 @@ describe Gitlab::Auth, lib: true do
       ip = 'ip'
 
       expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username)
-      expect(gl_auth.find(user.username, 'password', project: nil, ip: ip)).to eq([user, :master_or_ldap])
+      expect(gl_auth.find(user.username, 'password', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :gitlab_or_ldap))
     end
 
     it 'recognizes OAuth tokens' do
@@ -29,7 +29,7 @@ describe Gitlab::Auth, lib: true do
       ip = 'ip'
 
       expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'oauth2')
-      expect(gl_auth.find("oauth2", token.token, project: nil, ip: ip)).to eq([user, :oauth])
+      expect(gl_auth.find("oauth2", token.token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :oauth))
     end
 
     it 'returns double nil for invalid credentials' do
@@ -37,7 +37,7 @@ describe Gitlab::Auth, lib: true do
       ip = 'ip'
 
       expect(gl_auth).to receive(:rate_limit!).with(ip, success: false, login: login)
-      expect(gl_auth.find(login, 'bar', project: nil, ip: ip)).to eq([nil, nil])
+      expect(gl_auth.find(login, 'bar', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new)
     end
   end
 

From 1564074648afc12fc788a7b5e2eb896dc74f62ef Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Fri, 3 Jun 2016 15:28:35 +0200
Subject: [PATCH 30/39] =?UTF-8?q?Refactor=20=5Fallowed=3F=20methods=20as?=
 =?UTF-8?q?=20R=C3=A9my=20asked?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../projects/git_http_controller.rb           | 24 +++++++------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 5dfa10d218e..bf7ba7a5829 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -128,26 +128,20 @@ class Projects::GitHttpController < Projects::ApplicationController
   end
 
   def upload_pack_allowed?
-    if !Gitlab.config.gitlab_shell.upload_pack
-      false
-    elsif ci?
-      true
-    elsif user
+    return false unless Gitlab.config.gitlab_shell.upload_pack
+
+    if user
       Gitlab::GitAccess.new(user, project).download_access_check.allowed?
     else
-      project.public?
+      ci? || project.public?
     end
   end
 
   def receive_pack_allowed?
-    if !Gitlab.config.gitlab_shell.receive_pack
-      false
-    elsif user
-      # Skip user authorization on upload request.
-      # It will be done by the pre-receive hook in the repository.
-      true
-    else
-      false
-    end
+    return false unless Gitlab.config.gitlab_shell.receive_pack
+
+    # Skip user authorization on upload request.
+    # It will be done by the pre-receive hook in the repository.
+    user.present?
   end
 end

From 50a357d7e8fcc7c7ec25eff01495c39f7f90ffd8 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Fri, 3 Jun 2016 15:49:52 +0200
Subject: [PATCH 31/39] Use #present?

---
 app/controllers/projects/git_http_controller.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index bf7ba7a5829..e53158c4121 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -124,7 +124,7 @@ class Projects::GitHttpController < Projects::ApplicationController
   end
 
   def ci?
-    !!@ci
+    @ci.present?
   end
 
   def upload_pack_allowed?

From 46d5760c76bc3ba8222698d12cab5bc4d01c822b Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Fri, 3 Jun 2016 16:04:59 +0200
Subject: [PATCH 32/39] Fewer silly instance variables

---
 .../projects/git_http_controller.rb           | 20 +++++++++----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index e53158c4121..380139a9c30 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -88,17 +88,6 @@ class Projects::GitHttpController < Projects::ApplicationController
     [nil, nil]
   end
 
-  def repository
-    @repository ||= begin
-      _, suffix = project_id_with_suffix
-      if suffix == '.wiki.git'
-        project.wiki.repository
-      else
-        project.repository
-      end
-    end
-  end
-
   def upload_pack?
     git_command == 'git-upload-pack'
   end
@@ -119,6 +108,15 @@ class Projects::GitHttpController < Projects::ApplicationController
     render json: Gitlab::Workhorse.git_http_ok(repository, user)
   end
 
+  def repository
+    _, suffix = project_id_with_suffix
+    if suffix == '.wiki.git'
+      project.wiki.repository
+    else
+      project.repository
+    end
+  end
+
   def render_not_found
     render text: 'Not Found', status: :not_found
   end

From fa35aea3ddf1093db26f8b7fec78175a5f88af7a Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Fri, 3 Jun 2016 17:07:40 +0200
Subject: [PATCH 33/39] Refactor Gitlab::Auth rate limiting

---
 lib/gitlab/auth.rb              | 36 +++++++++-------------------
 lib/gitlab/auth/rate_limiter.rb | 42 +++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 25 deletions(-)
 create mode 100644 lib/gitlab/auth/rate_limiter.rb

diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 672642ebfbe..dd6ba84c973 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -1,5 +1,5 @@
 module Gitlab
-  class Auth
+  module Auth
     Result = Struct.new(:user, :type)
 
     class << self
@@ -64,34 +64,20 @@ module Gitlab
       end
 
       def rate_limit!(ip, success:, login:)
-        # If the user authenticated successfully, we reset the auth failure count
-        # from Rack::Attack for that IP. A client may attempt to authenticate
-        # with a username and blank password first, and only after it receives
-        # a 401 error does it present a password. Resetting the count prevents
-        # false positives.
-        #
-        # Otherwise, we let Rack::Attack know there was a failed authentication
-        # attempt from this IP. This information is stored in the Rails cache
-        # (Redis) and will be used by the Rack::Attack middleware to decide
-        # whether to block requests from this IP.
-
-        config = Gitlab.config.rack_attack.git_basic_auth
-        return unless config.enabled
+        rate_limiter = IpRateLimiter.new(ip)
+        return unless rate_limiter.enabled?
 
         if success
-          Rack::Attack::Allow2Ban.reset(ip, config)
+          # Repeated login 'failures' are normal behavior for some Git clients so
+          # it is important to reset the ban counter once the client has proven
+          # they are not a 'bad guy'.
+          rate_limiter.reset!
         else
-          banned = Rack::Attack::Allow2Ban.filter(ip, config) do
-            if config.ip_whitelist.include?(ip)
-              # Don't increment the ban counter for this IP
-              false
-            else
-              # Increment the ban counter for this IP
-              true
-            end
-          end
+          # Register a login failure so that Rack::Attack can block the next
+          # request from this IP if needed.
+          rate_limiter.register_fail!(ip, config)
 
-          if banned
+          if rate_limiter.banned?
             Rails.logger.info "IP #{ip} failed to login " \
               "as #{login} but has been temporarily banned from Git auth"
           end
diff --git a/lib/gitlab/auth/rate_limiter.rb b/lib/gitlab/auth/rate_limiter.rb
new file mode 100644
index 00000000000..4be9f6d0efe
--- /dev/null
+++ b/lib/gitlab/auth/rate_limiter.rb
@@ -0,0 +1,42 @@
+module Gitlab
+  module Auth
+    class IpRateLimiter
+      attr_reader :ip
+
+      def initialize(ip)
+        @ip = ip
+        @banned = false
+      end
+
+      def enabled?
+        config.enabled
+      end
+      
+      def reset!
+        Rack::Attack::Allow2Ban.reset(ip, config)
+      end
+      
+      def register_fail!
+        # Allow2Ban.filter will return false if this IP has not failed too often yet
+        @banned = Rack::Attack::Allow2Ban.filter(ip, config) do
+          # If we return false here, the failure for this IP is ignored by Allow2Ban
+          ignore_failure?
+        end
+      end
+      
+      def banned?
+        @banned
+      end
+      
+      private
+      
+      def config
+        Gitlab.config.rack_attack.git_basic_auth
+      end
+      
+      def ignore_failure?
+        config.ip_whitelist.exclude?(ip)
+      end
+    end
+  end
+end

From 1fab583266af0904dfc29facfe4551e37c06342a Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Fri, 3 Jun 2016 17:08:44 +0200
Subject: [PATCH 34/39] Remove instances of Auth.new

---
 app/controllers/jwt_controller.rb | 2 +-
 lib/gitlab/backend/grack_auth.rb  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/controllers/jwt_controller.rb b/app/controllers/jwt_controller.rb
index cee3b6c43e7..c05a55633b5 100644
--- a/app/controllers/jwt_controller.rb
+++ b/app/controllers/jwt_controller.rb
@@ -45,7 +45,7 @@ class JwtController < ApplicationController
     # TODO: this is a copy and paste from grack_auth,
     # it should be refactored in the future
 
-    user = Gitlab::Auth.new.find(login, password)
+    user = Gitlab::Auth.find_in_gitlab_or_ldap(login, password)
 
     # If the user authenticated successfully, we reset the auth failure count
     # from Rack::Attack for that IP. A client may attempt to authenticate
diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb
index 492ffb138a3..9e09d2e118d 100644
--- a/lib/gitlab/backend/grack_auth.rb
+++ b/lib/gitlab/backend/grack_auth.rb
@@ -95,7 +95,7 @@ module Grack
     end
 
     def authenticate_user(login, password)
-      user = Gitlab::Auth.new.find_in_gitlab_or_ldap(login, password)
+      user = Gitlab::Auth.find_in_gitlab_or_ldap(login, password)
 
       unless user
         user = oauth_access_token_check(login, password)

From 03bec6b0e943a3a047fd8f6185f71a976c02506c Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Fri, 3 Jun 2016 17:14:13 +0200
Subject: [PATCH 35/39] Argh mixed up all the negatives

---
 lib/gitlab/auth/rate_limiter.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/gitlab/auth/rate_limiter.rb b/lib/gitlab/auth/rate_limiter.rb
index 4be9f6d0efe..1089bc9f89e 100644
--- a/lib/gitlab/auth/rate_limiter.rb
+++ b/lib/gitlab/auth/rate_limiter.rb
@@ -20,7 +20,7 @@ module Gitlab
         # Allow2Ban.filter will return false if this IP has not failed too often yet
         @banned = Rack::Attack::Allow2Ban.filter(ip, config) do
           # If we return false here, the failure for this IP is ignored by Allow2Ban
-          ignore_failure?
+          ip_can_be_banned?
         end
       end
       
@@ -34,7 +34,7 @@ module Gitlab
         Gitlab.config.rack_attack.git_basic_auth
       end
       
-      def ignore_failure?
+      def ip_can_be_banned?
         config.ip_whitelist.exclude?(ip)
       end
     end

From 3f3b036defe4f22656f945f341c1d7da06d5543c Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Fri, 3 Jun 2016 17:23:34 +0200
Subject: [PATCH 36/39] Use public_send

---
 lib/gitlab/auth.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index dd6ba84c973..bd129d7216a 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -49,8 +49,7 @@ module Gitlab
         elsif Service.available_services_names.include?(underscored_service)
           # We treat underscored_service as a trusted input because it is included
           # in the Service.available_services_names whitelist.
-          service_method = "#{underscored_service}_service"
-          service = project.send(service_method)
+          service = project.public_send("#{underscored_service}_service")
 
           service && service.activated? && service.valid_token?(password)
         end

From 07f49626d01ddffcd127e937c528b74b8248043b Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Mon, 6 Jun 2016 17:40:26 +0200
Subject: [PATCH 37/39] Fix tests

---
 lib/gitlab/auth.rb                            | 42 +++++++++----------
 .../{rate_limiter.rb => ip_rate_limiter.rb}   |  0
 spec/requests/jwt_controller_spec.rb          |  2 +-
 3 files changed, 22 insertions(+), 22 deletions(-)
 rename lib/gitlab/auth/{rate_limiter.rb => ip_rate_limiter.rb} (100%)

diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index bd129d7216a..076e2af7d38 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -35,6 +35,27 @@ module Gitlab
         end
       end
 
+      def rate_limit!(ip, success:, login:)
+        rate_limiter = Gitlab::Auth::IpRateLimiter.new(ip)
+        return unless rate_limiter.enabled?
+
+        if success
+          # Repeated login 'failures' are normal behavior for some Git clients so
+          # it is important to reset the ban counter once the client has proven
+          # they are not a 'bad guy'.
+          rate_limiter.reset!
+        else
+          # Register a login failure so that Rack::Attack can block the next
+          # request from this IP if needed.
+          rate_limiter.register_fail!
+
+          if rate_limiter.banned?
+            Rails.logger.info "IP #{ip} failed to login " \
+              "as #{login} but has been temporarily banned from Git auth"
+          end
+        end
+      end
+
       private
 
       def valid_ci_request?(login, password, project)
@@ -61,27 +82,6 @@ module Gitlab
           token && token.accessible? && User.find_by(id: token.resource_owner_id)
         end
       end
-
-      def rate_limit!(ip, success:, login:)
-        rate_limiter = IpRateLimiter.new(ip)
-        return unless rate_limiter.enabled?
-
-        if success
-          # Repeated login 'failures' are normal behavior for some Git clients so
-          # it is important to reset the ban counter once the client has proven
-          # they are not a 'bad guy'.
-          rate_limiter.reset!
-        else
-          # Register a login failure so that Rack::Attack can block the next
-          # request from this IP if needed.
-          rate_limiter.register_fail!(ip, config)
-
-          if rate_limiter.banned?
-            Rails.logger.info "IP #{ip} failed to login " \
-              "as #{login} but has been temporarily banned from Git auth"
-          end
-        end
-      end
     end
   end
 end
diff --git a/lib/gitlab/auth/rate_limiter.rb b/lib/gitlab/auth/ip_rate_limiter.rb
similarity index 100%
rename from lib/gitlab/auth/rate_limiter.rb
rename to lib/gitlab/auth/ip_rate_limiter.rb
diff --git a/spec/requests/jwt_controller_spec.rb b/spec/requests/jwt_controller_spec.rb
index d006ff195cf..c995993a853 100644
--- a/spec/requests/jwt_controller_spec.rb
+++ b/spec/requests/jwt_controller_spec.rb
@@ -44,7 +44,7 @@ describe JwtController do
       let(:user) { create(:user) }
       let(:headers) { { authorization: credentials('user', 'password') } }
 
-      before { expect_any_instance_of(Gitlab::Auth).to receive(:find).with('user', 'password').and_return(user) }
+      before { expect(Gitlab::Auth).to receive(:find_in_gitlab_or_ldap).with('user', 'password').and_return(user) }
 
       subject! { get '/jwt/auth', parameters, headers }
 

From ff7c4e588ab4f7a397963d43becbe00d1bb584a1 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Mon, 6 Jun 2016 17:40:30 +0200
Subject: [PATCH 38/39] Remove code duplication in JwtController

---
 app/controllers/jwt_controller.rb | 40 +------------------------------
 1 file changed, 1 insertion(+), 39 deletions(-)

diff --git a/app/controllers/jwt_controller.rb b/app/controllers/jwt_controller.rb
index c05a55633b5..131a16dad9b 100644
--- a/app/controllers/jwt_controller.rb
+++ b/app/controllers/jwt_controller.rb
@@ -42,46 +42,8 @@ class JwtController < ApplicationController
   end
 
   def authenticate_user(login, password)
-    # TODO: this is a copy and paste from grack_auth,
-    # it should be refactored in the future
-
     user = Gitlab::Auth.find_in_gitlab_or_ldap(login, password)
-
-    # If the user authenticated successfully, we reset the auth failure count
-    # from Rack::Attack for that IP. A client may attempt to authenticate
-    # with a username and blank password first, and only after it receives
-    # a 401 error does it present a password. Resetting the count prevents
-    # false positives from occurring.
-    #
-    # Otherwise, we let Rack::Attack know there was a failed authentication
-    # attempt from this IP. This information is stored in the Rails cache
-    # (Redis) and will be used by the Rack::Attack middleware to decide
-    # whether to block requests from this IP.
-    config = Gitlab.config.rack_attack.git_basic_auth
-
-    if config.enabled
-      if user
-        # A successful login will reset the auth failure count from this IP
-        Rack::Attack::Allow2Ban.reset(request.ip, config)
-      else
-        banned = Rack::Attack::Allow2Ban.filter(request.ip, config) do
-          # Unless the IP is whitelisted, return true so that Allow2Ban
-          # increments the counter (stored in Rails.cache) for the IP
-          if config.ip_whitelist.include?(request.ip)
-            false
-          else
-            true
-          end
-        end
-
-        if banned
-          Rails.logger.info "IP #{request.ip} failed to login " \
-              "as #{login} but has been temporarily banned from Git auth"
-          return
-        end
-      end
-    end
-
+    Gitlab::Auth.rate_limit!(request.ip, success: user.present?, login: login)
     user
   end
 end

From df62cbd917f85f85d2e3371da2eccf724d5d94e0 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Wed, 8 Jun 2016 11:42:25 +0200
Subject: [PATCH 39/39] Add parentheses

---
 spec/requests/git_http_spec.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 14d126480a3..594a60a4340 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -287,20 +287,20 @@ describe 'Git HTTP requests', lib: true do
   def download(project, user: nil, password: nil)
     args = [project, { user: user, password: password }]
 
-    clone_get *args
+    clone_get(*args)
     yield response
 
-    clone_post *args
+    clone_post(*args)
     yield response
   end
 
   def upload(project, user: nil, password: nil)
     args = [project, { user: user, password: password }]
 
-    push_get *args
+    push_get(*args)
     yield response
 
-    push_post *args
+    push_post(*args)
     yield response
   end