Use actor when we don't know if it's a Key or User

* Use gl_id when we don't know if it's a key-X or user-X
* Use Actor.new_from(gl_id) which will figure out if it's a Key or User
* Use key_str when we're referring to key-X as key_id is confusing

6 files changed, 62 insertions, 51 deletions

diff --git a/spec/action/api_2fa_recovery.rb_spec.rb b/spec/action/api_2fa_recovery.rb_spec.rb
index 1f5219a..ab09ed2 100644
--- a/spec/action/api_2fa_recovery.rb_spec.rb
+++ b/spec/action/api_2fa_recovery.rb_spec.rb
@@ -2,7 +2,7 @@ require_relative '../spec_helper'
 require_relative '../../lib/action/api_2fa_recovery'
 
 describe Action::API2FARecovery do
-  let(:key_id) { "key-#{rand(100) + 100}" }
+  let(:key_id) { '1' }
   let(:key) { Actor::Key.new(key_id) }
   let(:username) { 'testuser' }
   let(:discover_payload) { { 'username' => username } }
@@ -14,7 +14,7 @@ describe Action::API2FARecovery do
   end
 
   subject do
-    described_class.new(key_id)
+    described_class.new(key)
   end
 
   describe '#execute' do
diff --git a/spec/action/git_lfs_authenticate_spec.rb b/spec/action/git_lfs_authenticate_spec.rb
index f9a0791..20740db 100644
--- a/spec/action/git_lfs_authenticate_spec.rb
+++ b/spec/action/git_lfs_authenticate_spec.rb
@@ -2,8 +2,9 @@ require_relative '../spec_helper'
 require_relative '../../lib/action/git_lfs_authenticate'
 
 describe Action::GitLFSAuthenticate do
-  let(:key_id) { "key-#{rand(100) + 100}" }
+  let(:key_id) { '1' }
   let(:repo_name) { 'gitlab-ci.git' }
+  let(:key) { Actor::Key.new(key_id) }
   let(:username) { 'testuser' }
   let(:discover_payload) { { 'username' => username } }
   let(:api) { double(GitlabNet) }
@@ -14,7 +15,7 @@ describe Action::GitLFSAuthenticate do
   end
 
   subject do
-    described_class.new(key_id, repo_name)
+    described_class.new(key, repo_name)
   end
 
   describe '#execute' do
diff --git a/spec/action/gitaly_spec.rb b/spec/action/gitaly_spec.rb
index 9c35b49..61e4e4b 100644
--- a/spec/action/gitaly_spec.rb
+++ b/spec/action/gitaly_spec.rb
@@ -5,7 +5,9 @@ describe Action::Gitaly do
   let(:git_trace_log_file_valid) { '/tmp/git_trace_performance.log' }
   let(:git_trace_log_file_invalid) { "/bleep-bop#{git_trace_log_file_valid}" }
   let(:git_trace_log_file_relative) { "..#{git_trace_log_file_valid}" }
-  let(:key_id) { "key-#{rand(100) + 100}" }
+  let(:key_id) { '1' }
+  let(:key_str) { 'key-1' }
+  let(:key) { Actor::Key.new(key_id) }
   let(:gl_repository) { 'project-1' }
   let(:gl_username) { 'testuser' }
   let(:tmp_repos_path) { File.join(ROOT_PATH, 'tmp', 'repositories') }
@@ -34,7 +36,7 @@ describe Action::Gitaly do
   end
 
   subject do
-    described_class.new(key_id, gl_repository, gl_username, repository_path, gitaly)
+    described_class.new(key, gl_repository, gl_username, repository_path, gitaly)
   end
 
   describe '#execute' do
@@ -45,7 +47,7 @@ describe Action::Gitaly do
         'PATH' => ENV['PATH'],
         'LD_LIBRARY_PATH' => ENV['LD_LIBRARY_PATH'],
         'LANG' => ENV['LANG'],
-        'GL_ID' => key_id,
+        'GL_ID' => key_str,
         'GL_PROTOCOL' => GitlabNet::GL_PROTOCOL,
         'GL_REPOSITORY' => gl_repository,
         'GL_USERNAME' => gl_username,
@@ -63,7 +65,7 @@ describe Action::Gitaly do
       {
         'repository' => gitaly['repository'],
         'gl_repository' => gl_repository,
-        'gl_id' => key_id,
+        'gl_id' => key_str,
         'gl_username' => gl_username
       }
     end
@@ -94,7 +96,7 @@ describe Action::Gitaly do
           end
         end
 
-        context 'with an relative config.git_trace_log_file' do
+        context 'with n relative config.git_trace_log_file' do
           let(:git_trace_log_file) { git_trace_log_file_relative }
 
           it 'returns true' do
diff --git a/spec/gitlab_net_spec.rb b/spec/gitlab_net_spec.rb
index 5a65c48..2dd70af 100644
--- a/spec/gitlab_net_spec.rb
+++ b/spec/gitlab_net_spec.rb
@@ -7,8 +7,13 @@ describe GitlabNet, vcr: true do
   let(:base_api_endpoint) { 'http://localhost:3000/api/v4' }
   let(:internal_api_endpoint) { 'http://localhost:3000/api/v4/internal' }
   let(:project) { 'gitlab-org/gitlab-test.git' }
-  let(:key) { 'key-1' }
-  let(:key2) { 'key-2' }
+
+  let(:key_id1) { '1' }
+  let(:key1_str) { "key-#{key_id1}" }
+  let(:key1) { Actor::Key.new(key1_str) }
+
+  let(:user1) { Actor::User.new('user-1') }
+
   let(:secret) { "0a3938d9d95d807e94d937af3a4fbbea\n" }
 
   before do
@@ -41,7 +46,7 @@ describe GitlabNet, vcr: true do
   describe '#discover' do
     it 'should return user has based on key id' do
       VCR.use_cassette("discover-ok") do
-        user = gitlab_net.discover(key)
+        user = gitlab_net.discover(key_id1)
         expect(user['name']).to eql 'Administrator'
         expect(user['username']).to eql 'root'
       end
@@ -50,14 +55,14 @@ describe GitlabNet, vcr: true do
     it 'adds the secret_token to request' do
       VCR.use_cassette("discover-ok") do
         allow_any_instance_of(Net::HTTP::Get).to receive(:set_form_data).with(hash_including(secret_token: secret))
-        gitlab_net.discover(key)
+        gitlab_net.discover(key_id1)
       end
     end
 
     it "raises an exception if the connection fails" do
       VCR.use_cassette("discover-ok") do
         allow_any_instance_of(Net::HTTP).to receive(:request).and_raise(StandardError)
-        expect(gitlab_net.discover(key)).to be_nil
+        expect(gitlab_net.discover(key_id1)).to be_nil
       end
     end
   end
@@ -66,7 +71,7 @@ describe GitlabNet, vcr: true do
     context 'lfs authentication succeeded' do
       it 'should return the correct data' do
         VCR.use_cassette('lfs-authenticate-ok') do
-          lfs_access = gitlab_net.lfs_authenticate(key, project)
+          lfs_access = gitlab_net.lfs_authenticate(key_id1, project)
           expect(lfs_access.username).to eql 'root'
           expect(lfs_access.lfs_token).to eql 'Hyzhyde_wLUeyUQsR3tHGTG8eNocVQm4ssioTEsBSdb6KwCSzQ'
           expect(lfs_access.repository_http_path).to eql URI.join(internal_api_endpoint.sub('api/v4', ''), project).to_s
@@ -156,7 +161,7 @@ describe GitlabNet, vcr: true do
     let(:gl_repository) { "project-1" }
     let(:changes) { "123456 789012 refs/heads/test\n654321 210987 refs/tags/tag" }
     let(:params) do
-      { gl_repository: gl_repository, identifier: key, changes: changes }
+      { gl_repository: gl_repository, identifier: key1.identifier, changes: changes }
     end
     let(:merge_request_urls) do
       [{
@@ -166,12 +171,11 @@ describe GitlabNet, vcr: true do
       }]
     end
 
-    subject { gitlab_net.post_receive(gl_repository, key, changes) }
+    subject { gitlab_net.post_receive(gl_repository, key1, changes) }
 
     it 'sends the correct parameters' do
       allow_any_instance_of(Net::HTTP::Post).to receive(:set_form_data).with(hash_including(params))
 
-
       VCR.use_cassette("post-receive") do
         subject
       end
@@ -226,7 +230,7 @@ describe GitlabNet, vcr: true do
   describe '#two_factor_recovery_codes' do
     it 'returns two factor recovery codes' do
       VCR.use_cassette('two-factor-recovery-codes') do
-        result = gitlab_net.two_factor_recovery_codes(key)
+        result = gitlab_net.two_factor_recovery_codes(key1_str)
         expect(result['success']).to be_truthy
         expect(result['recovery_codes']).to eq(['f67c514de60c4953','41278385fc00c1e0'])
       end
@@ -268,7 +272,7 @@ describe GitlabNet, vcr: true do
         it 'raises an UnknownError exception' do
           VCR.use_cassette('failed-push') do
             expect do
-              gitlab_net.check_access('git-receive-pack', nil, project, key, changes, 'ssh')
+              gitlab_net.check_access('git-receive-pack', nil, project, key1, changes, 'ssh')
             end.to raise_error(UnknownError, 'API is not accessible: An internal server error occurred')
           end
         end
@@ -278,7 +282,7 @@ describe GitlabNet, vcr: true do
         it 'raises an UnknownError exception' do
           VCR.use_cassette('failed-push-unparsable') do
             expect do
-              gitlab_net.check_access('git-receive-pack', nil, project, key, changes, 'ssh')
+              gitlab_net.check_access('git-receive-pack', nil, project, key1, changes, 'ssh')
             end.to raise_error(UnknownError, 'API is not accessible')
           end
         end
@@ -288,7 +292,7 @@ describe GitlabNet, vcr: true do
     context 'ssh key with access nil, to project' do
       it 'should allow push access for host' do
         VCR.use_cassette('allowed-push') do
-          action = gitlab_net.check_access('git-receive-pack', nil, project, key, changes, 'ssh')
+          action = gitlab_net.check_access('git-receive-pack', nil, project, key1, changes, 'ssh')
           expect(action).to be_instance_of(Action::Gitaly)
         end
       end
@@ -296,13 +300,13 @@ describe GitlabNet, vcr: true do
       it 'adds the secret_token to the request' do
         VCR.use_cassette('allowed-pull') do
           allow_any_instance_of(Net::HTTP::Post).to receive(:set_form_data).with(hash_including(secret_token: secret))
-          gitlab_net.check_access('git-receive-pack', nil, project, key, changes, 'ssh')
+          gitlab_net.check_access('git-receive-pack', nil, project, key1, changes, 'ssh')
         end
       end
 
       it 'should allow pull access for host' do
         VCR.use_cassette("allowed-pull") do
-          action = gitlab_net.check_access('git-upload-pack', nil, project, key, changes, 'ssh')
+          action = gitlab_net.check_access('git-upload-pack', nil, project, key1, changes, 'ssh')
           expect(action).to be_instance_of(Action::Gitaly)
         end
       end
@@ -312,13 +316,13 @@ describe GitlabNet, vcr: true do
       it 'should deny pull access for host' do
         VCR.use_cassette('ssh-pull-disabled-old') do
           expect do
-            gitlab_net.check_access('git-upload-pack', nil, project, key, changes, 'http')
+            gitlab_net.check_access('git-upload-pack', nil, project, key1, changes, 'http')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
 
         VCR.use_cassette('ssh-pull-disabled') do
           expect do
-            gitlab_net.check_access('git-upload-pack', nil, project, key, changes, 'http')
+            gitlab_net.check_access('git-upload-pack', nil, project, key1, changes, 'http')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
       end
@@ -326,13 +330,13 @@ describe GitlabNet, vcr: true do
       it 'should deny push access for host' do
         VCR.use_cassette('ssh-push-disabled-old') do
           expect do
-            gitlab_net.check_access('git-receive-pack', nil, project, key, changes, 'ssh')
+            gitlab_net.check_access('git-receive-pack', nil, project, key1, changes, 'ssh')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
 
         VCR.use_cassette('ssh-push-disabled') do
           expect do
-            gitlab_net.check_access('git-receive-pack', nil, project, key, changes, 'ssh')
+            gitlab_net.check_access('git-receive-pack', nil, project, key1, changes, 'ssh')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
       end
@@ -342,13 +346,13 @@ describe GitlabNet, vcr: true do
       it 'should deny pull access for host' do
         VCR.use_cassette('http-pull-disabled-old') do
           expect do
-            gitlab_net.check_access('git-upload-pack', nil, project, key, changes, 'http')
+            gitlab_net.check_access('git-upload-pack', nil, project, key1, changes, 'http')
           end.to raise_error(AccessDeniedError, 'Pulling over HTTP is not allowed.')
         end
 
         VCR.use_cassette('http-pull-disabled') do
           expect do
-            gitlab_net.check_access('git-upload-pack', nil, project, key, changes, 'http')
+            gitlab_net.check_access('git-upload-pack', nil, project, key1, changes, 'http')
           end.to raise_error(AccessDeniedError, 'Pulling over HTTP is not allowed.')
         end
       end
@@ -356,13 +360,13 @@ describe GitlabNet, vcr: true do
       it 'should deny push access for host' do
         VCR.use_cassette('http-push-disabled-old') do
           expect do
-            gitlab_net.check_access('git-receive-pack', nil, project, key, changes, 'http')
+            gitlab_net.check_access('git-receive-pack', nil, project, key1, changes, 'http')
           end.to raise_error(AccessDeniedError, 'Pushing over HTTP is not allowed.')
         end
 
         VCR.use_cassette('http-push-disabled') do
           expect do
-            gitlab_net.check_access('git-receive-pack', nil, project, key, changes, 'http')
+            gitlab_net.check_access('git-receive-pack', nil, project, key1, changes, 'http')
           end.to raise_error(AccessDeniedError, 'Pushing over HTTP is not allowed.')
         end
       end
@@ -372,13 +376,13 @@ describe GitlabNet, vcr: true do
       it 'should deny pull access for host' do
         VCR.use_cassette('ssh-pull-project-denied-old') do
           expect do
-            gitlab_net.check_access('git-receive-pack', nil, project, key2, changes, 'ssh')
+            gitlab_net.check_access('git-receive-pack', nil, project, user1, changes, 'ssh')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
 
         VCR.use_cassette('ssh-pull-project-denied') do
           expect do
-            gitlab_net.check_access('git-receive-pack', nil, project, key2, changes, 'ssh')
+            gitlab_net.check_access('git-receive-pack', nil, project, user1, changes, 'ssh')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
       end
@@ -386,13 +390,13 @@ describe GitlabNet, vcr: true do
       it 'should deny push access for host' do
         VCR.use_cassette('ssh-push-project-denied-old') do
           expect do
-            gitlab_net.check_access('git-upload-pack', nil, project, key2, changes, 'ssh')
+            gitlab_net.check_access('git-upload-pack', nil, project, user1, changes, 'ssh')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
 
         VCR.use_cassette('ssh-push-project-denied') do
           expect do
-            gitlab_net.check_access('git-upload-pack', nil, project, key2, changes, 'ssh')
+            gitlab_net.check_access('git-upload-pack', nil, project, user1, changes, 'ssh')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
       end
@@ -400,13 +404,13 @@ describe GitlabNet, vcr: true do
       it 'should deny push access for host (with user)' do
         VCR.use_cassette('ssh-push-project-denied-with-user-old') do
           expect do
-            gitlab_net.check_access('git-upload-pack', nil, project, 'user-2', changes, 'ssh')
+            gitlab_net.check_access('git-upload-pack', nil, project, user1, changes, 'ssh')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
 
         VCR.use_cassette('ssh-push-project-denied-with-user') do
           expect do
-            gitlab_net.check_access('git-upload-pack', nil, project, 'user-2', changes, 'ssh')
+            gitlab_net.check_access('git-upload-pack', nil, project, user1, changes, 'ssh')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
       end
@@ -415,7 +419,7 @@ describe GitlabNet, vcr: true do
     it "raises an exception if the connection fails" do
       allow_any_instance_of(Net::HTTP).to receive(:request).and_raise(StandardError)
       expect {
-        gitlab_net.check_access('git-upload-pack', nil, project, 'user-1', changes, 'ssh')
+        gitlab_net.check_access('git-upload-pack', nil, project, key1, changes, 'ssh')
       }.to raise_error(GitlabNet::ApiUnreachableError)
     end
   end
diff --git a/spec/gitlab_post_receive_spec.rb b/spec/gitlab_post_receive_spec.rb
index 704623f..c41cceb 100644
--- a/spec/gitlab_post_receive_spec.rb
+++ b/spec/gitlab_post_receive_spec.rb
@@ -5,13 +5,13 @@ require 'gitlab_post_receive'
 describe GitlabPostReceive do
   let(:repository_path) { "/home/git/repositories" }
   let(:repo_name) { 'dzaporozhets/gitlab-ci' }
-  let(:actor) { 'key-123' }
+  let(:gl_id) { 'key-123' }
   let(:changes) { "123456 789012 refs/heads/tést\n654321 210987 refs/tags/tag" }
   let(:wrongly_encoded_changes) { changes.encode("ISO-8859-1").force_encoding("UTF-8") }
   let(:base64_changes) { Base64.encode64(wrongly_encoded_changes) }
   let(:repo_path) { File.join(repository_path, repo_name) + ".git" }
   let(:gl_repository) { "project-1" }
-  let(:gitlab_post_receive) { GitlabPostReceive.new(gl_repository, repo_path, actor, wrongly_encoded_changes) }
+  let(:gitlab_post_receive) { GitlabPostReceive.new(gl_repository, repo_path, gl_id, wrongly_encoded_changes) }
   let(:broadcast_message) { "test " * 10 + "message " * 10 }
   let(:enqueued_at) { Time.new(2016, 6, 23, 6, 59) }
   let(:new_merge_request_urls) do
diff --git a/spec/gitlab_shell_spec.rb b/spec/gitlab_shell_spec.rb
index b7c0746..456dfcf 100644
--- a/spec/gitlab_shell_spec.rb
+++ b/spec/gitlab_shell_spec.rb
@@ -12,14 +12,17 @@ describe GitlabShell do
 
   subject { described_class.new(key_id) }
 
-  let(:key_id) { "key-#{rand(100) + 100}" }
+  let(:key_id) { '1' }
+  let(:key) { Actor::Key.new(key_id) }
   let(:tmp_repos_path) { File.join(ROOT_PATH, 'tmp', 'repositories') }
   let(:repo_name) { 'gitlab-ci.git' }
   let(:repo_path) { File.join(tmp_repos_path, repo_name) }
   let(:gl_repository) { 'project-1' }
   let(:gl_username) { 'testuser' }
+  let(:audit_usernames) { true }
 
   let(:api) { double(GitlabNet) }
+  let(:config) { double(GitlabConfig) }
 
   let(:gitaly_action) { Action::Gitaly.new(
                           key_id,
@@ -32,6 +35,11 @@ describe GitlabShell do
   let(:git_lfs_authenticate_action) { Action::GitLFSAuthenticate.new(key_id, repo_name) }
 
   before do
+    allow(GitlabConfig).to receive(:new).and_return(config)
+    allow(config).to receive(:audit_usernames).and_return(audit_usernames)
+
+    allow(Actor::Key).to receive(:from).with(key_id, audit_usernames: audit_usernames).and_return(key)
+
     allow(GitlabNet).to receive(:new).and_return(api)
     allow(api).to receive(:discover).with(key_id).and_return('username' => gl_username)
   end
@@ -106,7 +114,7 @@ describe GitlabShell do
       let(:git_access) { '2fa_recovery_codes' }
 
       before do
-        expect(Action::API2FARecovery).to receive(:new).with(key_id).and_return(api_2fa_recovery_action)
+        expect(Action::API2FARecovery).to receive(:new).with(key).and_return(api_2fa_recovery_action)
       end
 
       it 'returns true' do
@@ -117,7 +125,7 @@ describe GitlabShell do
 
     context 'when access to the repo is denied' do
       before do
-        expect(api).to receive(:check_access).with('git-upload-pack', nil, repo_name, key_id, '_any').and_raise(AccessDeniedError, 'Sorry, access denied')
+        expect(api).to receive(:check_access).with('git-upload-pack', nil, repo_name, key, '_any').and_raise(AccessDeniedError, 'Sorry, access denied')
       end
 
       it 'prints a message to stderr and returns false' do
@@ -128,7 +136,7 @@ describe GitlabShell do
 
     context 'when the API is unavailable' do
       before do
-        expect(api).to receive(:check_access).with('git-upload-pack', nil, repo_name, key_id, '_any').and_raise(GitlabNet::ApiUnreachableError)
+        expect(api).to receive(:check_access).with('git-upload-pack', nil, repo_name, key, '_any').and_raise(GitlabNet::ApiUnreachableError)
       end
 
       it 'prints a message to stderr and returns false' do
@@ -139,7 +147,7 @@ describe GitlabShell do
 
     context 'when access has been verified OK' do
       before do
-        expect(api).to receive(:check_access).with(git_access, nil, repo_name, key_id, '_any').and_return(gitaly_action)
+        expect(api).to receive(:check_access).with(git_access, nil, repo_name, key, '_any').and_return(gitaly_action)
       end
 
       context 'when origin_cmd is git-upload-pack' do
@@ -169,11 +177,10 @@ describe GitlabShell do
 
       context 'when origin_cmd is git-lfs-authenticate' do
         let(:origin_cmd) { 'git-lfs-authenticate' }
-        # let(:fake_payload) { 'FAKE PAYLOAD' }
         let(:lfs_access) { double(GitlabLfsAuthentication, authentication_payload: fake_payload)}
 
         before do
-          expect(Action::GitLFSAuthenticate).to receive(:new).with(key_id, repo_name).and_return(git_lfs_authenticate_action)
+          expect(Action::GitLFSAuthenticate).to receive(:new).with(key, repo_name).and_return(git_lfs_authenticate_action)
         end
 
         context 'upload' do
@@ -181,7 +188,6 @@ describe GitlabShell do
 
           it 'returns true' do
             expect(git_lfs_authenticate_action).to receive(:execute).with('git-lfs-authenticate', %w{ git-lfs-authenticate gitlab-ci.git upload }).and_return(true)
-            # expect($stdout).to receive(:puts).with(fake_payload)
             expect(subject.exec("#{origin_cmd} #{repo_name} upload")).to be_truthy
           end
         end
@@ -191,14 +197,12 @@ describe GitlabShell do
 
           it 'returns true' do
             expect(git_lfs_authenticate_action).to receive(:execute).with('git-lfs-authenticate', %w{ git-lfs-authenticate gitlab-ci.git download }).and_return(true)
-            # expect($stdout).to receive(:puts).with(fake_payload)
             expect(subject.exec("#{origin_cmd} #{repo_name} download")).to be_truthy
           end
 
           context 'for old git-lfs clients' do
             it 'returns true' do
               expect(git_lfs_authenticate_action).to receive(:execute).with('git-lfs-authenticate', %w{ git-lfs-authenticate gitlab-ci.git download long_oid }).and_return(true)
-              # expect($stdout).to receive(:puts).with(fake_payload)
               expect(subject.exec("#{origin_cmd} #{repo_name} download long_oid")).to be_truthy
             end
           end