From c5a7a70d10cc59e940f85ce21bc25e392ab68978 Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Wed, 10 Aug 2016 19:03:32 -0500
Subject: [PATCH 1/8] Allow Git over HTTP access using Personal Access Tokens

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

diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index db1704af75e..926774837d0 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -14,6 +14,8 @@ module Gitlab
           result.type = :gitlab_or_ldap
         elsif result.user = oauth_access_token_check(login, password)
           result.type = :oauth
+        elsif result.user = personal_access_token_check(login, password)
+          result.type = :personal_token
         end
 
         rate_limit!(ip, success: !!result.user || (result.type == :ci), login: login)
@@ -82,6 +84,13 @@ module Gitlab
           token && token.accessible? && User.find_by(id: token.resource_owner_id)
         end
       end
+
+      def personal_access_token_check(login, password)
+        if login && password
+          user = User.find_by_personal_access_token(password)
+          user if user && user.username == login
+        end
+      end
     end
   end
 end

From 8bb1931ef2d25ee5dfc5352a3932b948656ddf94 Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Wed, 10 Aug 2016 19:04:25 -0500
Subject: [PATCH 2/8] Deny Git over HTTP access to users that have 2FA enabled,
 unless they use a Personal Access Token.

---
 .../personal_access_tokens/index.html.haml    |  4 ++
 spec/requests/git_http_spec.rb                | 41 +++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/app/views/profiles/personal_access_tokens/index.html.haml b/app/views/profiles/personal_access_tokens/index.html.haml
index 71ac367830d..03ee682b343 100644
--- a/app/views/profiles/personal_access_tokens/index.html.haml
+++ b/app/views/profiles/personal_access_tokens/index.html.haml
@@ -7,6 +7,10 @@
       = page_title
     %p
       You can generate a personal access token for each application you use that needs access to the GitLab API.
+    %p
+      You can also use personal access tokens to authenticate against Git over HTTP. Use them specially when you
+      have 2FA enabled.
+
   .col-lg-9
 
     - if flash[:personal_access_token]
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 8537c252b58..37c2586bbe2 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -198,6 +198,47 @@ describe 'Git HTTP requests', lib: true do
               end
             end
 
+            context 'when user has 2FA enabled' do
+              before do
+                @user = create(:user, :two_factor)
+                project.team << [@user, :master]
+              end
+
+              context 'when username and password are provided' do
+                it 'rejects the clone attempt' do
+                  download("#{project.path_with_namespace}.git", user: @user.username, password: @user.password) do |response|
+                    expect(response).to have_http_status(401)
+                    expect(response.body).to include('You have 2FA enabled, please use a personal access token for Git over HTTP')
+                  end
+                end
+
+                it 'rejects the push attempt' do
+                  upload("#{project.path_with_namespace}.git", user: @user.username, password: @user.password) do |response|
+                    expect(response).to have_http_status(401)
+                    expect(response.body).to include('You have 2FA enabled, please use a personal access token for Git over HTTP')
+                  end
+                end
+              end
+
+              context 'when username and personal access token are provided' do
+                before do
+                  @token = create(:personal_access_token, user: @user)
+                end
+
+                it 'allows clones' do
+                  download("#{project.path_with_namespace}.git", user: @user.username, password: @token.token) do |response|
+                    expect(response).to have_http_status(200)
+                  end
+                end
+
+                it 'allows pushes' do
+                  upload("#{project.path_with_namespace}.git", user: @user.username, password: @token.token) do |response|
+                    expect(response).to have_http_status(200)
+                  end
+                end
+              end
+            end
+
             context "when blank password attempts follow a valid login" do
               def attempt_login(include_password)
                 password = include_password ? user.password : ""

From 641d061e3c94cc7ef740a2bd25c936ee7e78bceb Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Wed, 10 Aug 2016 19:20:00 -0500
Subject: [PATCH 3/8] Added CHANGELOG item

---
 CHANGELOG | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG b/CHANGELOG
index fc9291eefd5..4111ed32866 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -54,6 +54,7 @@ v 8.11.0 (unreleased)
   - Upgrade Grape from 0.13.0 to 0.15.0. !4601
   - Trigram indexes for the "ci_runners" table have been removed to speed up UPDATE queries
   - Fix devise deprecation warnings.
+  - Check for 2FA when using Git over HTTP and only allow PersonalAccessTokens as password in that case !5764
   - Update version_sorter and use new interface for faster tag sorting
   - Optimize checking if a user has read access to a list of issues !5370
   - Store all DB secrets in secrets.yml, under descriptive names !5274

From 0f37721b6002bc5c7f2202479e7900a9bae4ce19 Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Thu, 11 Aug 2016 11:42:34 -0500
Subject: [PATCH 4/8] 2FA check is now done in the main GitHTTPClientController

---
 app/controllers/projects/git_http_client_controller.rb | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 7c21bd181dc..e48403ca817 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -29,6 +29,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
         # Not allowed
       else
         @user = auth_result.user
+        check_2fa(auth_result.type)
       end
 
       if ci? || user
@@ -91,6 +92,12 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     [nil, nil]
   end
 
+  def check_2fa(auth_type)
+    if user && user.two_factor_enabled? && auth_type == :gitlab_or_ldap
+      render plain: "HTTP Basic: Access denied\nYou have 2FA enabled, please use a personal access token for Git over HTTP\n", status: 401
+    end
+  end
+
   def repository
     _, suffix = project_id_with_suffix
     if suffix == '.wiki.git'

From f971026ad3aabcd682c42db6d35e35cb64121f40 Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Fri, 12 Aug 2016 11:37:10 -0500
Subject: [PATCH 5/8] Added better information about the personal tokens

---
 app/controllers/projects/git_http_client_controller.rb    | 5 ++++-
 app/views/profiles/personal_access_tokens/index.html.haml | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index e48403ca817..abe47f80858 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -94,7 +94,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController
 
   def check_2fa(auth_type)
     if user && user.two_factor_enabled? && auth_type == :gitlab_or_ldap
-      render plain: "HTTP Basic: Access denied\nYou have 2FA enabled, please use a personal access token for Git over HTTP\n", status: 401
+      render plain: "HTTP Basic: Access denied\n"\
+                      "You have 2FA enabled, please use a personal access token for Git over HTTP.\n"\
+                      "You can generate one at #{profile_personal_access_tokens_url}",
+             status: 401
     end
   end
 
diff --git a/app/views/profiles/personal_access_tokens/index.html.haml b/app/views/profiles/personal_access_tokens/index.html.haml
index 03ee682b343..d2964c065d1 100644
--- a/app/views/profiles/personal_access_tokens/index.html.haml
+++ b/app/views/profiles/personal_access_tokens/index.html.haml
@@ -9,7 +9,7 @@
       You can generate a personal access token for each application you use that needs access to the GitLab API.
     %p
       You can also use personal access tokens to authenticate against Git over HTTP. Use them specially when you
-      have 2FA enabled.
+      have Two-Factor Authentication (2FA) enabled.
 
   .col-lg-9
 

From 5f5d8a8e09bbd2fcdfd02c68145a8c1086fe5e7c Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Mon, 15 Aug 2016 15:47:29 -0500
Subject: [PATCH 6/8] Moved 2FA check to `auth.rb` and cleaned up the flow
 `authenticate_user`

---
 .../projects/git_http_client_controller.rb       | 16 ++++++++--------
 lib/gitlab/auth.rb                               | 10 ++++++++--
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index abe47f80858..59395abf401 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -27,9 +27,11 @@ class Projects::GitHttpClientController < Projects::ApplicationController
         @ci = true
       elsif auth_result.type == :oauth && !download_request?
         # Not allowed
+      elsif auth_result.type == :missing_personal_token
+        render_missing_personal_token
+        return # Render above denied access, nothing left to do
       else
         @user = auth_result.user
-        check_2fa(auth_result.type)
       end
 
       if ci? || user
@@ -92,13 +94,11 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     [nil, nil]
   end
 
-  def check_2fa(auth_type)
-    if user && user.two_factor_enabled? && auth_type == :gitlab_or_ldap
-      render plain: "HTTP Basic: Access denied\n"\
-                      "You have 2FA enabled, please use a personal access token for Git over HTTP.\n"\
-                      "You can generate one at #{profile_personal_access_tokens_url}",
-             status: 401
-    end
+  def render_missing_personal_token
+    render plain: "HTTP Basic: Access denied\n"\
+                  "You have 2FA enabled, please use a personal access token for Git over HTTP.\n"\
+                  "You can generate one at #{profile_personal_access_tokens_url}",
+           status: 401
   end
 
   def repository
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 926774837d0..538e001ec35 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -11,14 +11,20 @@ module Gitlab
         if valid_ci_request?(login, password, project)
           result.type = :ci
         elsif result.user = find_with_user_password(login, password)
-          result.type = :gitlab_or_ldap
+          if result.user.two_factor_enabled?
+            result.user = nil
+            result.type = :missing_personal_token
+          else
+            result.type = :gitlab_or_ldap
+          end
         elsif result.user = oauth_access_token_check(login, password)
           result.type = :oauth
         elsif result.user = personal_access_token_check(login, password)
           result.type = :personal_token
         end
 
-        rate_limit!(ip, success: !!result.user || (result.type == :ci), login: login)
+        success = result.user.present? || [:ci, :missing_personal_token].include?(result.type)
+        rate_limit!(ip, success: success, login: login)
         result
       end
 

From 2f86860a6ded54bb48f03bae1de9a88113c75173 Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Wed, 17 Aug 2016 17:21:18 -0500
Subject: [PATCH 7/8] Refactor `find_for_git_client` method to not use
 assignment in conditionals and syntax fixes.

---
 .../projects/git_http_client_controller.rb    |  4 +-
 .../personal_access_tokens/index.html.haml    |  4 +-
 lib/gitlab/auth.rb                            | 38 ++++++++++++-------
 spec/requests/git_http_spec.rb                | 18 ++++-----
 4 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 59395abf401..a5b4031c30f 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -95,8 +95,8 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   end
 
   def render_missing_personal_token
-    render plain: "HTTP Basic: Access denied\n"\
-                  "You have 2FA enabled, please use a personal access token for Git over HTTP.\n"\
+    render plain: "HTTP Basic: Access denied\n" \
+                  "You have 2FA enabled, please use a personal access token for Git over HTTP.\n" \
                   "You can generate one at #{profile_personal_access_tokens_url}",
            status: 401
   end
diff --git a/app/views/profiles/personal_access_tokens/index.html.haml b/app/views/profiles/personal_access_tokens/index.html.haml
index d2964c065d1..05a2ea67aa2 100644
--- a/app/views/profiles/personal_access_tokens/index.html.haml
+++ b/app/views/profiles/personal_access_tokens/index.html.haml
@@ -8,8 +8,8 @@
     %p
       You can generate a personal access token for each application you use that needs access to the GitLab API.
     %p
-      You can also use personal access tokens to authenticate against Git over HTTP. Use them specially when you
-      have Two-Factor Authentication (2FA) enabled.
+      You can also use personal access tokens to authenticate against Git over HTTP.
+      They are the only accepted password when you have Two-Factor Authentication (2FA) enabled.
 
   .col-lg-9
 
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 538e001ec35..e60ce21388e 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -10,17 +10,8 @@ module Gitlab
 
         if valid_ci_request?(login, password, project)
           result.type = :ci
-        elsif result.user = find_with_user_password(login, password)
-          if result.user.two_factor_enabled?
-            result.user = nil
-            result.type = :missing_personal_token
-          else
-            result.type = :gitlab_or_ldap
-          end
-        elsif result.user = oauth_access_token_check(login, password)
-          result.type = :oauth
-        elsif result.user = personal_access_token_check(login, password)
-          result.type = :personal_token
+        else
+          result.user, result.type = populate_result(login, password)
         end
 
         success = result.user.present? || [:ci, :missing_personal_token].include?(result.type)
@@ -87,16 +78,37 @@ module Gitlab
       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)
+          if token && token.accessible?
+            user = User.find_by(id: token.resource_owner_id)
+            return user, :oauth
+          end
         end
       end
 
       def personal_access_token_check(login, password)
         if login && password
           user = User.find_by_personal_access_token(password)
-          user if user && user.username == login
+          validation = User.by_login(login)
+          return user, :personal_token if user == validation
         end
       end
+
+      def user_with_password_for_git(login, password)
+        user = find_with_user_password(login, password)
+        return user, :gitlab_or_ldap if user
+      end
+
+      def populate_result(login, password)
+        user, type =
+          user_with_password_for_git(login, password) || oauth_access_token_check(login, password) || personal_access_token_check(login, password)
+
+        if user && user.two_factor_enabled? && type == :gitlab_or_ldap
+          user = nil
+          type = :missing_personal_token
+        end
+
+        [user, type]
+      end
     end
   end
 end
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 37c2586bbe2..afaf4b7cefb 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -199,21 +199,23 @@ describe 'Git HTTP requests', lib: true do
             end
 
             context 'when user has 2FA enabled' do
+              let(:user) { create(:user, :two_factor) }
+              let(:access_token) { create(:personal_access_token, user: user) }
+
               before do
-                @user = create(:user, :two_factor)
-                project.team << [@user, :master]
+                project.team << [user, :master]
               end
 
               context 'when username and password are provided' do
                 it 'rejects the clone attempt' do
-                  download("#{project.path_with_namespace}.git", user: @user.username, password: @user.password) do |response|
+                  download("#{project.path_with_namespace}.git", user: user.username, password: user.password) do |response|
                     expect(response).to have_http_status(401)
                     expect(response.body).to include('You have 2FA enabled, please use a personal access token for Git over HTTP')
                   end
                 end
 
                 it 'rejects the push attempt' do
-                  upload("#{project.path_with_namespace}.git", user: @user.username, password: @user.password) do |response|
+                  upload("#{project.path_with_namespace}.git", user: user.username, password: user.password) do |response|
                     expect(response).to have_http_status(401)
                     expect(response.body).to include('You have 2FA enabled, please use a personal access token for Git over HTTP')
                   end
@@ -221,18 +223,14 @@ describe 'Git HTTP requests', lib: true do
               end
 
               context 'when username and personal access token are provided' do
-                before do
-                  @token = create(:personal_access_token, user: @user)
-                end
-
                 it 'allows clones' do
-                  download("#{project.path_with_namespace}.git", user: @user.username, password: @token.token) do |response|
+                  download("#{project.path_with_namespace}.git", user: user.username, password: access_token.token) do |response|
                     expect(response).to have_http_status(200)
                   end
                 end
 
                 it 'allows pushes' do
-                  upload("#{project.path_with_namespace}.git", user: @user.username, password: @token.token) do |response|
+                  upload("#{project.path_with_namespace}.git", user: user.username, password: access_token.token) do |response|
                     expect(response).to have_http_status(200)
                   end
                 end

From de5f2380293f9c8ccbb9a1c83a309589f42b77b8 Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Wed, 17 Aug 2016 17:59:25 -0500
Subject: [PATCH 8/8] Refactor `find_for_git_client` and its related methods.

---
 lib/gitlab/auth.rb | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index e60ce21388e..91f0270818a 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -11,7 +11,7 @@ module Gitlab
         if valid_ci_request?(login, password, project)
           result.type = :ci
         else
-          result.user, result.type = populate_result(login, password)
+          result = populate_result(login, password)
         end
 
         success = result.user.present? || [:ci, :missing_personal_token].include?(result.type)
@@ -75,12 +75,34 @@ module Gitlab
         end
       end
 
+      def populate_result(login, password)
+        result =
+          user_with_password_for_git(login, password) ||
+          oauth_access_token_check(login, password) ||
+          personal_access_token_check(login, password)
+
+        if result
+          result.type = nil unless result.user
+
+          if result.user && result.user.two_factor_enabled? && result.type == :gitlab_or_ldap
+            result.type = :missing_personal_token
+          end
+        end
+
+        result || Result.new
+      end
+
+      def user_with_password_for_git(login, password)
+        user = find_with_user_password(login, password)
+        Result.new(user, :gitlab_or_ldap) if user
+      end
+
       def oauth_access_token_check(login, password)
         if login == "oauth2" && password.present?
           token = Doorkeeper::AccessToken.by_token(password)
           if token && token.accessible?
             user = User.find_by(id: token.resource_owner_id)
-            return user, :oauth
+            Result.new(user, :oauth)
           end
         end
       end
@@ -89,26 +111,9 @@ module Gitlab
         if login && password
           user = User.find_by_personal_access_token(password)
           validation = User.by_login(login)
-          return user, :personal_token if user == validation
+          Result.new(user, :personal_token) if user == validation
         end
       end
-
-      def user_with_password_for_git(login, password)
-        user = find_with_user_password(login, password)
-        return user, :gitlab_or_ldap if user
-      end
-
-      def populate_result(login, password)
-        user, type =
-          user_with_password_for_git(login, password) || oauth_access_token_check(login, password) || personal_access_token_check(login, password)
-
-        if user && user.two_factor_enabled? && type == :gitlab_or_ldap
-          user = nil
-          type = :missing_personal_token
-        end
-
-        [user, type]
-      end
     end
   end
 end