Commit 3ffa494f authored by Jacob Vosmaer's avatar Jacob Vosmaer

Changes after more review from Rémy

parent fea591e5
...@@ -41,15 +41,17 @@ class Projects::GitHttpController < Projects::ApplicationController ...@@ -41,15 +41,17 @@ class Projects::GitHttpController < Projects::ApplicationController
return if project && project.public? && upload_pack? return if project && project.public? && upload_pack?
authenticate_or_request_with_http_basic do |login, password| 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 @ci = true
elsif (type == :oauth) && !upload_pack? elsif auth_result.type == :oauth && !upload_pack?
@user = nil # Not allowed
else else
@user = user @user = auth_result.user
end end
ci? || user
end end
end end
...@@ -73,7 +75,7 @@ class Projects::GitHttpController < Projects::ApplicationController ...@@ -73,7 +75,7 @@ class Projects::GitHttpController < Projects::ApplicationController
def project_id_with_suffix def project_id_with_suffix
id = params[:project_id] || '' id = params[:project_id] || ''
%w{.wiki.git .git}.each do |suffix| %w[.wiki.git .git].each do |suffix|
if id.end_with?(suffix) if id.end_with?(suffix)
# Be careful to only remove the suffix from the end of 'id'. # Be careful to only remove the suffix from the end of 'id'.
# Accidentally removing it from the middle is how security # Accidentally removing it from the middle is how security
...@@ -109,7 +111,7 @@ class Projects::GitHttpController < Projects::ApplicationController ...@@ -109,7 +111,7 @@ class Projects::GitHttpController < Projects::ApplicationController
if action_name == 'info_refs' if action_name == 'info_refs'
params[:service] params[:service]
else else
action_name.gsub('_', '-') action_name.dasherize
end end
end end
......
module Gitlab module Gitlab
class Auth class Auth
Result = Struct.new(:user, :type)
class << self class << self
def find(login, password, project:, ip:) def find(login, password, project:, ip:)
raise "Must provide an IP for rate limiting" if ip.nil? raise "Must provide an IP for rate limiting" if ip.nil?
user = nil result = Result.new
type = nil
if valid_ci_request?(login, password, project) if valid_ci_request?(login, password, project)
type = :ci result.type = :ci
elsif user = find_in_gitlab_or_ldap(login, password) elsif result.user = find_in_gitlab_or_ldap(login, password)
type = :master_or_ldap result.type = :gitlab_or_ldap
elsif user = oauth_access_token_check(login, password) elsif result.user = oauth_access_token_check(login, password)
type = :oauth result.type = :oauth
end end
rate_limit!(ip, success: !!user || (type == :ci), login: login) rate_limit!(ip, success: !!result.user || (result.type == :ci), login: login)
[user, type] result
end end
def find_in_gitlab_or_ldap(login, password) def find_in_gitlab_or_ldap(login, password)
...@@ -67,7 +68,7 @@ module Gitlab ...@@ -67,7 +68,7 @@ module Gitlab
# from Rack::Attack for that IP. A client may attempt to authenticate # from Rack::Attack for that IP. A client may attempt to authenticate
# with a username and blank password first, and only after it receives # with a username and blank password first, and only after it receives
# a 401 error does it present a password. Resetting the count prevents # 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 # Otherwise, we let Rack::Attack know there was a failed authentication
# attempt from this IP. This information is stored in the Rails cache # attempt from this IP. This information is stored in the Rails cache
...@@ -78,15 +79,14 @@ module Gitlab ...@@ -78,15 +79,14 @@ module Gitlab
return unless config.enabled return unless config.enabled
if success if success
# A successful login will reset the auth failure count from this IP
Rack::Attack::Allow2Ban.reset(ip, config) Rack::Attack::Allow2Ban.reset(ip, config)
else else
banned = Rack::Attack::Allow2Ban.filter(ip, config) do 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) if config.ip_whitelist.include?(ip)
# Don't increment the ban counter for this IP
false false
else else
# Increment the ban counter for this IP
true true
end end
end end
......
...@@ -11,7 +11,7 @@ describe Gitlab::Auth, lib: true do ...@@ -11,7 +11,7 @@ describe Gitlab::Auth, lib: true do
ip = 'ip' ip = 'ip'
expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'gitlab-ci-token') 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 end
it 'recognizes master passwords' do it 'recognizes master passwords' do
...@@ -19,7 +19,7 @@ describe Gitlab::Auth, lib: true do ...@@ -19,7 +19,7 @@ describe Gitlab::Auth, lib: true do
ip = 'ip' ip = 'ip'
expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username) 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 end
it 'recognizes OAuth tokens' do it 'recognizes OAuth tokens' do
...@@ -29,7 +29,7 @@ describe Gitlab::Auth, lib: true do ...@@ -29,7 +29,7 @@ describe Gitlab::Auth, lib: true do
ip = 'ip' ip = 'ip'
expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'oauth2') 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 end
it 'returns double nil for invalid credentials' do it 'returns double nil for invalid credentials' do
...@@ -37,7 +37,7 @@ describe Gitlab::Auth, lib: true do ...@@ -37,7 +37,7 @@ describe Gitlab::Auth, lib: true do
ip = 'ip' ip = 'ip'
expect(gl_auth).to receive(:rate_limit!).with(ip, success: false, login: login) expect(gl_auth).to receive(:rate_limit!).with(ip, success: 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
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment