diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb index f5a07608bf8dbdbb13da907ed605cd1459ff9967..4dae953b69f8646546c446aa399ba56e921fecc8 100644 --- a/app/controllers/projects/git_http_client_controller.rb +++ b/app/controllers/projects/git_http_client_controller.rb @@ -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! diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 391b8f2f5de46f3f8cb8fb60bcf58c76cbb6bdad..6be9bf7de442b145938e6fc5431db067451ac021 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -1,6 +1,10 @@ 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 = /(?^[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 - 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 diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb index 224e4516074b2950d5dcc8cef54e97e834ea8435..f492754b1c8a999e5dc17d6be4e3a015e3abade1 100644 --- a/lib/gitlab/lfs_token.rb +++ b/lib/gitlab/lfs_token.rb @@ -13,7 +13,7 @@ module Gitlab when Key actor.user else - # + raise 'Bad Actor' end end diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index 56f349f5d92255ee175c43d08cc9ae90b4dde6dc..13c5a7156f55005d5a3169fff4e09b6855d0c73c 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -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 diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb index 184f235c1b2b609be71fd62ba9e457bb90c65f70..9f04f67e0a8ddc5e6289fdb555df4f3e4a0d9bc2 100644 --- a/spec/lib/gitlab/lfs_token_spec.rb +++ b/spec/lib/gitlab/lfs_token_spec.rb @@ -1,7 +1,7 @@ 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 diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 2e1e6a11b539340910047d9d4ee97d19da9a0664..46e8e6f11697e5eed5a3e98aa1da50cddcc358b5 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -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 )