Changes after more review from Rémy

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