diff --git a/app/finders/user_finder.rb b/app/finders/user_finder.rb
index 1dd1a27437ed2e15e55ac71ea2c700798b235433..556be4c4338cb9e9bc75c42d725772fd6d50f551 100644
--- a/app/finders/user_finder.rb
+++ b/app/finders/user_finder.rb
@@ -52,12 +52,6 @@ def find_by_id_or_username!
end
end
- def find_by_ssh_key_id
- return unless input_is_id?
-
- User.find_by_ssh_key_id(@username_or_id)
- end
-
def input_is_id?
@username_or_id.is_a?(Numeric) || @username_or_id =~ /^\d+$/
end
diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb
index dfac777e4a1dbf49055213de410585edeaa023ed..b03eb5ad440f293c03da7cf7d935a9844436f2ab 100644
--- a/lib/api/helpers/internal_helpers.rb
+++ b/lib/api/helpers/internal_helpers.rb
@@ -7,6 +7,10 @@ module InternalHelpers
delegate :wiki?, to: :repo_type
+ def actor
+ @actor ||= Support::GitAccessActor.from_params(params)
+ end
+
def repo_type
set_project unless defined?(@repo_type) # rubocop:disable Gitlab/ModuleWithInstanceVariables
@repo_type # rubocop:disable Gitlab/ModuleWithInstanceVariables
diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb
index c70f2f3e2c838707bc69b23548cf8f94ee44d53f..50142b8641eeda8be4a19728fe843f125bdd2063 100644
--- a/lib/api/internal/base.rb
+++ b/lib/api/internal/base.rb
@@ -7,7 +7,6 @@ class Base < Grape::API
before { authenticate_by_gitlab_shell_token! }
helpers ::API::Helpers::InternalHelpers
- helpers ::Gitlab::Identifier
UNKNOWN_CHECK_RESULT_ERROR = 'Unknown check result'.freeze
@@ -35,7 +34,6 @@ def check_allowed(params)
env = parse_env
Gitlab::Git::HookEnv.set(gl_repository, env) if project
- actor = Support::GitAccessActor.from_params(params)
actor.update_last_used_at!
access_checker = access_checker_for(actor, params[:protocol])
@@ -103,36 +101,30 @@ def check_allowed(params)
check_allowed(params)
end
- # rubocop: disable CodeReuse/ActiveRecord
post "/lfs_authenticate" do
status 200
- if params[:key_id]
- actor = Key.find(params[:key_id])
- actor.update_last_used_at
- elsif params[:user_id]
- actor = User.find_by(id: params[:user_id])
- raise ActiveRecord::RecordNotFound.new("No such user id!") unless actor
- else
- raise ActiveRecord::RecordNotFound.new("No key_id or user_id passed!")
+ unless actor.key_or_user
+ raise ActiveRecord::RecordNotFound.new('User not found!')
end
+ actor.update_last_used_at!
+
Gitlab::LfsToken
- .new(actor)
+ .new(actor.key_or_user)
.authentication_payload(lfs_authentication_url(project))
end
- # rubocop: enable CodeReuse/ActiveRecord
#
# Get a ssh key using the fingerprint
#
# rubocop: disable CodeReuse/ActiveRecord
- get "/authorized_keys" do
+ get '/authorized_keys' do
fingerprint = params.fetch(:fingerprint) do
Gitlab::InsecureKeyFingerprint.new(params.fetch(:key)).fingerprint
end
key = Key.find_by(fingerprint: fingerprint)
- not_found!("Key") if key.nil?
+ not_found!('Key') if key.nil?
present key, with: Entities::SSHKey
end
# rubocop: enable CodeReuse/ActiveRecord
@@ -141,16 +133,10 @@ def check_allowed(params)
# Discover user by ssh key, user id or username
#
get '/discover' do
- if params[:key_id]
- user = UserFinder.new(params[:key_id]).find_by_ssh_key_id
- elsif params[:username]
- user = UserFinder.new(params[:username]).find_by_username
- end
-
- present user, with: Entities::UserSafe
+ present actor.user, with: Entities::UserSafe
end
- get "/check" do
+ get '/check' do
{
api_version: API.version,
gitlab_version: Gitlab::VERSION,
@@ -158,35 +144,26 @@ def check_allowed(params)
redis: redis_ping
}
end
-
- # rubocop: disable CodeReuse/ActiveRecord
post '/two_factor_recovery_codes' do
status 200
- if params[:key_id]
- key = Key.find_by(id: params[:key_id])
+ actor.update_last_used_at!
+ user = actor.user
- if key
- key.update_last_used_at
- else
- break { 'success' => false, 'message' => 'Could not find the given key' }
+ if params[:key_id]
+ unless actor.key
+ break { success: false, message: 'Could not find the given key' }
end
- if key.is_a?(DeployKey)
+ if actor.key.is_a?(DeployKey)
break { success: false, message: 'Deploy keys cannot be used to retrieve recovery codes' }
end
- user = key.user
-
unless user
break { success: false, message: 'Could not find a user for the given key' }
end
- elsif params[:user_id]
- user = User.find_by(id: params[:user_id])
-
- unless user
- break { success: false, message: 'Could not find the given user' }
- end
+ elsif params[:user_id] && user.nil?
+ break { success: false, message: 'Could not find the given user' }
end
unless user.two_factor_enabled?
@@ -201,7 +178,6 @@ def check_allowed(params)
{ success: true, recovery_codes: codes }
end
- # rubocop: enable CodeReuse/ActiveRecord
post '/pre_receive' do
status 200
@@ -211,7 +187,7 @@ def check_allowed(params)
{ reference_counter_increased: reference_counter_increased }
end
- post "/notify_post_receive" do
+ post '/notify_post_receive' do
status 200
# TODO: Re-enable when Gitaly is processing the post-receive notification
@@ -229,8 +205,7 @@ def check_allowed(params)
status 200
response = Gitlab::InternalPostReceive::Response.new
- user = identify(params[:identifier])
- project = Gitlab::GlRepository.parse(params[:gl_repository]).first
+ user = actor.user
push_options = Gitlab::PushOptions.new(params[:push_options])
response.reference_counter_decreased = Gitlab::ReferenceCounter.new(params[:gl_repository]).decrease
diff --git a/lib/api/support/git_access_actor.rb b/lib/api/support/git_access_actor.rb
index 2e0962c6295526e155d3f160667a5a7dceb8413f..cb9bf4472ebb78da9c722069df75d54503477ad4 100644
--- a/lib/api/support/git_access_actor.rb
+++ b/lib/api/support/git_access_actor.rb
@@ -3,7 +3,9 @@
module API
module Support
class GitAccessActor
- attr_reader :user
+ extend ::Gitlab::Identifier
+
+ attr_reader :user, :key
def initialize(user: nil, key: nil)
@user = user
@@ -19,6 +21,10 @@ def self.from_params(params)
new(user: UserFinder.new(params[:user_id]).find_by_id)
elsif params[:username]
new(user: UserFinder.new(params[:username]).find_by_username)
+ elsif params[:identifier]
+ new(user: identify(params[:identifier]))
+ else
+ new
end
end
@@ -33,10 +39,6 @@ def username
def update_last_used_at!
key&.update_last_used_at
end
-
- private
-
- attr_reader :key
end
end
end
diff --git a/spec/finders/user_finder_spec.rb b/spec/finders/user_finder_spec.rb
index c20d7850d682bae7fbc52ac3d21202a76c670bcd..b89b422aa2c99832d5dca9702e19fc7781f17f2f 100644
--- a/spec/finders/user_finder_spec.rb
+++ b/spec/finders/user_finder_spec.rb
@@ -176,26 +176,4 @@
end
end
end
-
- describe '#find_by_ssh_key_id' do
- let_it_be(:ssh_key) { create(:key, user: user) }
-
- it 'returns the user when passing the ssh key id' do
- found = described_class.new(ssh_key.id).find_by_ssh_key_id
-
- expect(found).to eq(user)
- end
-
- it 'returns the user when passing the ssh key id (string)' do
- found = described_class.new(ssh_key.id.to_s).find_by_ssh_key_id
-
- expect(found).to eq(user)
- end
-
- it 'returns nil when the id does not exist' do
- found = described_class.new(-1).find_by_ssh_key_id
-
- expect(found).to be_nil
- end
- end
end
diff --git a/spec/lib/api/support/git_access_actor_spec.rb b/spec/lib/api/support/git_access_actor_spec.rb
index 63f5966faea46646da4ac6436469a13b2af7172a..69637947c7930f73b1067ac38aedb4b98e6303f3 100644
--- a/spec/lib/api/support/git_access_actor_spec.rb
+++ b/spec/lib/api/support/git_access_actor_spec.rb
@@ -9,17 +9,26 @@
subject { described_class.new(user: user, key: key) }
describe '.from_params' do
+ let(:key) { create(:key) }
+
context 'with params that are valid' do
it 'returns an instance of API::Support::GitAccessActor' do
- params = { key_id: create(:key).id }
+ params = { key_id: key.id }
expect(described_class.from_params(params)).to be_instance_of(described_class)
end
end
context 'with params that are invalid' do
- it 'returns nil' do
- expect(described_class.from_params({})).to be_nil
+ it "returns an instance of #{described_class}" do
+ expect(described_class.from_params({})).to be_instance_of(described_class)
+ end
+ end
+
+ context 'when passing an identifier used gitaly' do
+ it 'finds the user based on an identifier' do
+ expect(described_class).to receive(:identify).and_call_original
+ expect(described_class.from_params(identifier: "key-#{key.id}").user).to eq(key.user)
end
end
end
diff --git a/spec/requests/api/internal/base_spec.rb b/spec/requests/api/internal/base_spec.rb
index fd3d1ebee205e3279324e0f449c48d72175a1f68..7292e7a6a4efa7b1b4d2b58e42770b6eaa55a68c 100644
--- a/spec/requests/api/internal/base_spec.rb
+++ b/spec/requests/api/internal/base_spec.rb
@@ -193,7 +193,15 @@
end
it 'responds successfully when a user is not found' do
- get(api("/internal/discover"), params: { username: 'noone', secret_token: secret_token })
+ get(api('/internal/discover'), params: { username: 'noone', secret_token: secret_token })
+
+ expect(response).to have_gitlab_http_status(200)
+
+ expect(response.body).to eq('null')
+ end
+
+ it 'response successfully when passing invalid params' do
+ get(api('/internal/discover'), params: { nothing: 'to find a user', secret_token: secret_token })
expect(response).to have_gitlab_http_status(200)
@@ -819,7 +827,6 @@
before do
project.add_developer(user)
- allow(described_class).to receive(:identify).and_return(user)
allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(user)
end