Commit be09bcf0 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
include ActionController::HttpAuthentication::Basic
include KerberosSpnegoHelper
attr_reader :user, :actor
attr_reader :actor
# Git clients will not know what authenticity token to send along
skip_before_action :verify_authenticity_token
......@@ -22,9 +22,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
if allow_basic_auth? && basic_auth_provided?
login, password = user_name_and_password(request)
handle_basic_authentication(login, password)
if ci? || actor
if handle_basic_authentication(login, password)
return # Allow access
end
elsif allow_kerberos_spnego_auth? && spnego_provided?
......@@ -107,7 +105,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
end
def ci?
@ci.present?
@ci
end
def user
......@@ -119,9 +117,17 @@ class Projects::GitHttpClientController < Projects::ApplicationController
case auth_result.type
when :ci
@ci = true if download_request?
if download_request?
@ci = true
else
return false
end
when :oauth
@actor = auth_result.actor if download_request?
if download_request?
@actor = auth_result.actor
else
return false
end
when :lfs_deploy_token
if download_request?
@lfs_deploy_key = true
......@@ -131,11 +137,14 @@ class Projects::GitHttpClientController < Projects::ApplicationController
@actor = auth_result.actor
else
# Not allowed
return false
end
true
end
def lfs_deploy_key?
@lfs_deploy_key.present? && actor && actor.projects.include?(project)
@lfs_deploy_key && actor && actor.projects.include?(project)
end
def verify_workhorse_api!
......
module Gitlab
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
......@@ -8,7 +12,16 @@ module Gitlab
def find_for_git_client(login, password, project:, ip:)
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
def find_with_user_password(login, password)
......@@ -49,24 +62,6 @@ module Gitlab
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)
matched_login = /(?<service>^[a-zA-Z]*-ci)-token$/.match(login)
......@@ -110,7 +105,7 @@ module Gitlab
if login && password
user = User.find_by_personal_access_token(password)
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
......@@ -124,9 +119,11 @@ module Gitlab
User.by_login(login)
end
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
......
......@@ -13,7 +13,7 @@ module Gitlab
when Key
actor.user
else
#
raise 'Bad Actor'
end
end
......
......@@ -55,7 +55,7 @@ describe Gitlab::Auth, lib: true do
login = 'foo'
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)
end
end
......
require 'spec_helper'
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
it 'returns a randomly generated token' do
token = handler.generate
......
......@@ -107,7 +107,7 @@ describe API::API, api: true do
context 'user 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(json_response['username']).to eq(user.username)
......@@ -115,13 +115,19 @@ describe API::API, api: true do
expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
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
context 'deploy key' do
let(:key) { create(:deploy_key) }
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(json_response['username']).to eq("lfs+deploy-key-#{key.id}")
......@@ -421,10 +427,10 @@ describe API::API, api: true do
)
end
def lfs_auth(key, project)
def lfs_auth(key_id, project)
post(
api("/internal/lfs_authenticate"),
key_id: key.id,
key_id: key_id,
secret_token: secret_token,
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