2FA checks for Git over HTTP

What does this MR do?

This MR allows the use of PersonalAccessTokens to access Git over HTTP and makes that the only allowed method if the user has 2FA enabled. If a user with 2FA enabled tries to access Git over HTTP using his username and password the request will be denied and the user will be presented with the following message:

remote: HTTP Basic: Access denied
remote: You have 2FA enabled, please use a personal access token for Git over HTTP.
remote: You can generate one at http://localhost:3000/profile/personal_access_tokens
fatal: Authentication failed for 'http://localhost:3000/documentcloud/underscore.git/'

What are the relevant issue numbers?

Fixes #13568 (closed)

Added 1 commit:

• 8e8aa664 - Added CHANGELOG item

Added 1 commit:

• bf7b0b54 - Refactor 2FA check to fix Cyclomatic Complexity

Added 48 commits:

• bf7b0b54...5a33bc98 - 44 commits from branch master
• 6243e128 - Allow Git over HTTP access using Personal Access Tokens
• bf001117 - Deny Git over HTTP access to users that have 2FA enabled, unless they use a Personal Access Token.
• 12669c01 - Added CHANGELOG item
• 9e1ff5fe - 2FA check is now done in the main GitHTTPClientController

Reassigned to @dbalexandre

@dbalexandre could I bother with another review?

@patricio Sure, added to TODOS :)

I think that will be more clear, if we handle this scenario in the case statement. Wdyt?

if auth_result.type == :ci && upload_pack?
  @ci = true
elsif auth_result.type == :oauth && !upload_pack?
  # Not allowed
elsif auth_result.type == :gitlab_or_ldap && auth_result.user.two_factor_enabled?
  render plain: "HTTP Basic: Access denied\nYou have 2FA enabled, please use a personal access token for Git over HTTP\n", status: 401
else
  @user = auth_result.user
end

@dbalexandre I tried something similar, but rubocop complained about cyclomatic complexity, so I moved it to it's own method. I'll try it like you say and see what rubocop says.

@dbalexandre rubocop's cyclomatic and percieved complexity warnings are triggered if I write like you mentioned. I'll leave it like it is.

@patricio what about this:

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index abe47f8..2529a8e 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 9267748..538e001 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
 

@jacobvosmaer-gitlab yeah, it does make more sense to check for the user 2FA status in auth.rb than in the controller. Thanks for the suggestion and the snippet. I'll update the code.

@patricio Looks good Just one remark.

Reassigned to @patricio

@patricio it'd be nice if you could add a link in the warning to the settings page that allows you to generate personal access tokens.

Other than that, looks really nice!

@connorshea that makes sense. I'll add it.

Added 1 commit:

• d5cc0e30 - Added better information about the personal tokens

Reassigned to @rymai

@rymai can you help me with an endboss review?

@patricio I'd love to, but I'm in holiday! Maybe @jacobvosmaer-gitlab can help since he refactored app/controllers/projects/git_http_client_controller.rb? :)

Reassigned to @jacobvosmaer-gitlab

@rymai I'm sorry, I didn't know. Thanks for replying during your holiday, and have a great time!

Added 96 commits:

• d5cc0e30...640e485c - 90 commits from branch master
• 98c161b5 - Allow Git over HTTP access using Personal Access Tokens
• 4998e4d9 - Deny Git over HTTP access to users that have 2FA enabled, unless they use a Personal Access Token.
• 394a6dc1 - Added CHANGELOG item
• 1db36ebb - 2FA check is now done in the main GitHTTPClientController
• d9633706 - Added better information about the personal tokens
• c9e0f1ab - Moved 2FA check to auth.rb and cleaned up the flow authenticate_user

@patricio LGTM. Not sure what is up with those test failures. Pass it on to an actual endboss? I am just an opinionated team member. :)

@jacobvosmaer-gitlab thanks for the review. I appreciate your input. I'll assign to an endboss and see what's up with the specs.