Commit 3d933082 authored by Patricio Cano's avatar Patricio Cano

Refactored authentication code to make it a bit clearer, added test for wrong SSH key.

parent de24075e
...@@ -4,7 +4,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController ...@@ -4,7 +4,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
include ActionController::HttpAuthentication::Basic include ActionController::HttpAuthentication::Basic
include KerberosSpnegoHelper include KerberosSpnegoHelper
attr_reader :user, :actor attr_reader :actor
# Git clients will not know what authenticity token to send along # Git clients will not know what authenticity token to send along
skip_before_action :verify_authenticity_token skip_before_action :verify_authenticity_token
...@@ -22,9 +22,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController ...@@ -22,9 +22,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
if allow_basic_auth? && basic_auth_provided? if allow_basic_auth? && basic_auth_provided?
login, password = user_name_and_password(request) login, password = user_name_and_password(request)
handle_basic_authentication(login, password) if handle_basic_authentication(login, password)
if ci? || actor
return # Allow access return # Allow access
end end
elsif allow_kerberos_spnego_auth? && spnego_provided? elsif allow_kerberos_spnego_auth? && spnego_provided?
...@@ -107,7 +105,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController ...@@ -107,7 +105,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
end end
def ci? def ci?
@ci.present? @ci
end end
def user def user
...@@ -119,23 +117,36 @@ class Projects::GitHttpClientController < Projects::ApplicationController ...@@ -119,23 +117,36 @@ class Projects::GitHttpClientController < Projects::ApplicationController
case auth_result.type case auth_result.type
when :ci when :ci
@ci = true if download_request? if download_request?
@ci = true
else
return false
end
when :oauth when :oauth
@actor = auth_result.actor if download_request? if download_request?
@actor = auth_result.actor
else
return false
end
when :lfs_deploy_token when :lfs_deploy_token
if download_request? if download_request?
@lfs_deploy_key = true @lfs_deploy_key = true
@actor = auth_result.actor @actor = auth_result.actor
else
return false
end end
when :lfs_token, :personal_token, :gitlab_or_ldap when :lfs_token, :personal_token, :gitlab_or_ldap
@actor = auth_result.actor @actor = auth_result.actor
else else
# Not allowed # Not allowed
return false
end end
true
end end
def lfs_deploy_key? def lfs_deploy_key?
@lfs_deploy_key.present? && actor && actor.projects.include?(project) @lfs_deploy_key && actor && actor.projects.include?(project)
end end
def verify_workhorse_api! def verify_workhorse_api!
......
module Gitlab module Gitlab
module Auth module Auth
Result = Struct.new(:actor, :type) Result = Struct.new(:actor, :type) do
def success?
actor.present? || type == :ci
end
end
class MissingPersonalTokenError < StandardError; end class MissingPersonalTokenError < StandardError; end
...@@ -8,7 +12,16 @@ module Gitlab ...@@ -8,7 +12,16 @@ module Gitlab
def find_for_git_client(login, password, project:, ip:) def find_for_git_client(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?
populate_result(login, password, project, ip) result =
ci_request_check(login, password, project) ||
user_with_password_for_git(login, password) ||
oauth_access_token_check(login, password) ||
lfs_token_check(login, password) ||
personal_access_token_check(login, password)
rate_limit!(ip, success: result && result.success?, login: login)
result || Result.new
end end
def find_with_user_password(login, password) def find_with_user_password(login, password)
...@@ -49,24 +62,6 @@ module Gitlab ...@@ -49,24 +62,6 @@ module Gitlab
private private
def populate_result(login, password, project, ip)
result =
ci_request_check(login, password, project) ||
user_with_password_for_git(login, password) ||
oauth_access_token_check(login, password) ||
lfs_token_check(login, password) ||
personal_access_token_check(login, password)
if result && result.type != :ci
result.type = nil unless result.actor
end
success = result ? result.actor.present? || result.type == :ci : false
rate_limit!(ip, success: success, login: login)
result || Result.new
end
def valid_ci_request?(login, password, project) def valid_ci_request?(login, password, project)
matched_login = /(?<service>^[a-zA-Z]*-ci)-token$/.match(login) matched_login = /(?<service>^[a-zA-Z]*-ci)-token$/.match(login)
...@@ -110,7 +105,7 @@ module Gitlab ...@@ -110,7 +105,7 @@ module Gitlab
if login && password if login && password
user = User.find_by_personal_access_token(password) user = User.find_by_personal_access_token(password)
validation = User.by_login(login) validation = User.by_login(login)
Result.new(user, :personal_token) if user == validation Result.new(user, :personal_token) if user.present? && user == validation
end end
end end
...@@ -124,9 +119,11 @@ module Gitlab ...@@ -124,9 +119,11 @@ module Gitlab
User.by_login(login) User.by_login(login)
end end
token_handler = Gitlab::LfsToken.new(actor) if actor
token_handler = Gitlab::LfsToken.new(actor)
Result.new(actor, token_handler.type) if actor && Devise.secure_compare(token_handler.value, password) Result.new(actor, token_handler.type) if Devise.secure_compare(token_handler.value, password)
end
end end
end end
end end
......
...@@ -13,7 +13,7 @@ module Gitlab ...@@ -13,7 +13,7 @@ module Gitlab
when Key when Key
actor.user actor.user
else else
# raise 'Bad Actor'
end end
end end
......
...@@ -55,7 +55,7 @@ describe Gitlab::Auth, lib: true do ...@@ -55,7 +55,7 @@ describe Gitlab::Auth, lib: true do
login = 'foo' login = 'foo'
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: nil, login: login)
expect(gl_auth.find_for_git_client(login, 'bar', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new) expect(gl_auth.find_for_git_client(login, 'bar', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new)
end end
end end
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::LfsToken, lib: true do describe Gitlab::LfsToken, lib: true do
describe '#set_token and #get_value' do describe '#generate and #value' do
shared_examples 'an LFS token generator' do shared_examples 'an LFS token generator' do
it 'returns a randomly generated token' do it 'returns a randomly generated token' do
token = handler.generate token = handler.generate
......
...@@ -107,7 +107,7 @@ describe API::API, api: true do ...@@ -107,7 +107,7 @@ describe API::API, api: true do
context 'user key' do context 'user key' do
it 'returns the correct information about the key' do it 'returns the correct information about the key' do
lfs_auth(key, project) lfs_auth(key.id, project)
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['username']).to eq(user.username) expect(json_response['username']).to eq(user.username)
...@@ -115,13 +115,19 @@ describe API::API, api: true do ...@@ -115,13 +115,19 @@ describe API::API, api: true do
expect(json_response['repository_http_path']).to eq(project.http_url_to_repo) expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
end end
it 'returns a 404 when the wrong key is provided' do
lfs_auth(nil, project)
expect(response).to have_http_status(404)
end
end end
context 'deploy key' do context 'deploy key' do
let(:key) { create(:deploy_key) } let(:key) { create(:deploy_key) }
it 'returns the correct information about the key' do it 'returns the correct information about the key' do
lfs_auth(key, project) lfs_auth(key.id, project)
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['username']).to eq("lfs+deploy-key-#{key.id}") expect(json_response['username']).to eq("lfs+deploy-key-#{key.id}")
...@@ -421,10 +427,10 @@ describe API::API, api: true do ...@@ -421,10 +427,10 @@ describe API::API, api: true do
) )
end end
def lfs_auth(key, project) def lfs_auth(key_id, project)
post( post(
api("/internal/lfs_authenticate"), api("/internal/lfs_authenticate"),
key_id: key.id, key_id: key_id,
secret_token: secret_token, secret_token: secret_token,
project: project.path_with_namespace project: project.path_with_namespace
) )
......
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