From 37cfaf27abf714252c48765b0e78f25dc4bcf641 Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Wed, 7 Aug 2019 17:51:12 +1000 Subject: Extract /internal/allowed API Actor logic out Created new API::Support::GitAccessActor class to encapsulate some of the more edge logic, making the /internal/allowed route much cleaner. --- lib/api/helpers/internal_helpers.rb | 12 +++ lib/api/internal/base.rb | 35 ++----- lib/api/support/git_access_actor.rb | 42 +++++++++ spec/lib/api/support/git_access_actor_spec.rb | 128 ++++++++++++++++++++++++++ 4 files changed, 188 insertions(+), 29 deletions(-) create mode 100644 lib/api/support/git_access_actor.rb create mode 100644 spec/lib/api/support/git_access_actor_spec.rb diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index 6b438235258..9a80671f996 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -17,6 +17,18 @@ module API @project # rubocop:disable Gitlab/ModuleWithInstanceVariables end + def access_checker_for(actor, protocol) + access_checker_klass.new(actor.key_or_user, project, protocol, + authentication_abilities: ssh_authentication_abilities, + namespace_path: namespace_path, + project_path: project_path, + redirected_path: redirected_path) + end + + def access_checker_klass + repo_type.access_checker_class + end + def ssh_authentication_abilities [ :read_project, diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb index 622032b8355..087985d34b0 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -35,36 +35,14 @@ module API # project - project full_path (not path on disk) # action - git action (git-upload-pack or git-receive-pack) # changes - changes as "oldrev newrev ref", see Gitlab::ChangesList - # rubocop: disable CodeReuse/ActiveRecord post "/allowed" do # Stores some Git-specific env thread-safely env = parse_env Gitlab::Git::HookEnv.set(gl_repository, env) if project - actor = - if params[:key_id] - Key.find_by(id: params[:key_id]) - elsif params[:user_id] - User.find_by(id: params[:user_id]) - elsif params[:username] - UserFinder.new(params[:username]).find_by_username - end - - protocol = params[:protocol] - - actor.update_last_used_at if actor.is_a?(Key) - user = - if actor.is_a?(Key) - actor.user - else - actor - end - - access_checker_klass = repo_type.access_checker_class - access_checker = access_checker_klass.new(actor, project, - protocol, authentication_abilities: ssh_authentication_abilities, - namespace_path: namespace_path, project_path: project_path, - redirected_path: redirected_path) + actor = Support::GitAccessActor.from_params(params) + actor.update_last_used_at! + access_checker = access_checker_for(actor, params[:protocol]) check_result = begin result = access_checker.check(params[:action], params[:changes]) @@ -78,15 +56,15 @@ module API break response_with_status(code: 404, success: false, message: e.message) end - log_user_activity(actor) + log_user_activity(actor.user) case check_result when ::Gitlab::GitAccessResult::Success payload = { gl_repository: gl_repository, gl_project_path: gl_project_path, - gl_id: Gitlab::GlId.gl_id(user), - gl_username: user&.username, + gl_id: Gitlab::GlId.gl_id(actor.user), + gl_username: actor.username, git_config_options: [], gitaly: gitaly_payload(params[:action]), gl_console_messages: check_result.console_messages @@ -105,7 +83,6 @@ module API response_with_status(code: 500, success: false, message: UNKNOWN_CHECK_RESULT_ERROR) end end - # rubocop: enable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord post "/lfs_authenticate" do diff --git a/lib/api/support/git_access_actor.rb b/lib/api/support/git_access_actor.rb new file mode 100644 index 00000000000..2e0962c6295 --- /dev/null +++ b/lib/api/support/git_access_actor.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +module API + module Support + class GitAccessActor + attr_reader :user + + def initialize(user: nil, key: nil) + @user = user + @key = key + + @user = key.user if !user && key + end + + def self.from_params(params) + if params[:key_id] + new(key: Key.find_by_id(params[:key_id])) + elsif params[:user_id] + new(user: UserFinder.new(params[:user_id]).find_by_id) + elsif params[:username] + new(user: UserFinder.new(params[:username]).find_by_username) + end + end + + def key_or_user + key || user + end + + def username + user&.username + end + + def update_last_used_at! + key&.update_last_used_at + end + + private + + attr_reader :key + 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 new file mode 100644 index 00000000000..63f5966faea --- /dev/null +++ b/spec/lib/api/support/git_access_actor_spec.rb @@ -0,0 +1,128 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Support::GitAccessActor do + let(:user) { nil } + let(:key) { nil } + + subject { described_class.new(user: user, key: key) } + + describe '.from_params' do + context 'with params that are valid' do + it 'returns an instance of API::Support::GitAccessActor' do + params = { key_id: create(: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 + end + end + end + + describe 'attributes' do + describe '#user' do + context 'when initialized with a User' do + let(:user) { create(:user) } + + it 'returns the User' do + expect(subject.user).to eq(user) + end + end + + context 'when initialized with a Key' do + let(:user_for_key) { create(:user) } + let(:key) { create(:key, user: user_for_key) } + + it 'returns the User associated to the Key' do + expect(subject.user).to eq(user_for_key) + end + end + end + end + + describe '#key_or_user' do + context 'when params contains a :key_id' do + it 'is an instance of Key' do + key = create(:key) + params = { key_id: key.id } + + expect(described_class.from_params(params).key_or_user).to eq(key) + end + end + + context 'when params contains a :user_id' do + it 'is an instance of User' do + user = create(:user) + params = { user_id: user.id } + + expect(described_class.from_params(params).key_or_user).to eq(user) + end + end + + context 'when params contains a :username' do + it 'is an instance of User (via UserFinder)' do + user = create(:user) + params = { username: user.username } + + expect(described_class.from_params(params).key_or_user).to eq(user) + end + end + end + + describe '#username' do + context 'when initialized with a User' do + let(:user) { create(:user) } + + it 'returns the username' do + expect(subject.username).to eq(user.username) + end + end + + context 'when initialized with a Key' do + let(:key) { create(:key, user: user_for_key) } + + context 'that has no User associated' do + let(:user_for_key) { nil } + + it 'returns nil' do + expect(subject.username).to be_nil + end + end + + context 'that has a User associated' do + let(:user_for_key) { create(:user) } + + it 'returns the username of the User associated to the Key' do + expect(subject.username).to eq(user_for_key.username) + end + end + end + end + + describe '#update_last_used_at!' do + context 'when initialized with a User' do + let(:user) { create(:user) } + + it 'does nothing' do + expect(user).not_to receive(:update_last_used_at) + + subject.update_last_used_at! + end + end + + context 'when initialized with a Key' do + let(:key) { create(:key) } + + it 'updates update_last_used_at' do + expect(key).to receive(:update_last_used_at) + + subject.update_last_used_at! + end + end + end +end -- cgit v1.2.1