Merge remote-tracking branch 'origin/master' into ash.mckenzie/srp-refactor

6 files changed, 87 insertions, 61 deletions

diff --git a/spec/action/api_2fa_recovery.rb_spec.rb b/spec/action/api_2fa_recovery.rb_spec.rb
index ab09ed2..bfdadd6 100644
--- a/spec/action/api_2fa_recovery.rb_spec.rb
+++ b/spec/action/api_2fa_recovery.rb_spec.rb
@@ -3,18 +3,18 @@ require_relative '../../lib/action/api_2fa_recovery'
 
 describe Action::API2FARecovery do
   let(:key_id) { '1' }
-  let(:key) { Actor::Key.new(key_id) }
+  let(:actor) { Actor::Key.new(key_id) }
   let(:username) { 'testuser' }
   let(:discover_payload) { { 'username' => username } }
   let(:api) { double(GitlabNet) }
 
   before do
     allow(GitlabNet).to receive(:new).and_return(api)
-    allow(api).to receive(:discover).with(key_id).and_return(discover_payload)
+    allow(api).to receive(:discover).with(actor).and_return(discover_payload)
   end
 
   subject do
-    described_class.new(key)
+    described_class.new(actor)
   end
 
   describe '#execute' do
@@ -46,7 +46,7 @@ describe Action::API2FARecovery do
 
       before do
         expect(subject).to receive(:continue?).and_return(true)
-        expect(api).to receive(:two_factor_recovery_codes).with(key_id).and_return(response)
+        expect(api).to receive(:two_factor_recovery_codes).with(subject).and_return(response)
       end
 
       context 'with a unsuccessful response' do
diff --git a/spec/action/git_lfs_authenticate_spec.rb b/spec/action/git_lfs_authenticate_spec.rb
index 20740db..3c84d0c 100644
--- a/spec/action/git_lfs_authenticate_spec.rb
+++ b/spec/action/git_lfs_authenticate_spec.rb
@@ -4,24 +4,24 @@ require_relative '../../lib/action/git_lfs_authenticate'
 describe Action::GitLFSAuthenticate do
   let(:key_id) { '1' }
   let(:repo_name) { 'gitlab-ci.git' }
-  let(:key) { Actor::Key.new(key_id) }
+  let(:actor) { Actor::Key.new(key_id) }
   let(:username) { 'testuser' }
   let(:discover_payload) { { 'username' => username } }
   let(:api) { double(GitlabNet) }
 
   before do
     allow(GitlabNet).to receive(:new).and_return(api)
-    allow(api).to receive(:discover).with(key_id).and_return(discover_payload)
+    allow(api).to receive(:discover).with(actor).and_return(discover_payload)
   end
 
   subject do
-    described_class.new(key, repo_name)
+    described_class.new(actor, repo_name)
   end
 
   describe '#execute' do
     context 'when response from API is not a success' do
       before do
-        expect(api).to receive(:lfs_authenticate).with(key_id, repo_name).and_return(nil)
+        expect(api).to receive(:lfs_authenticate).with(subject, repo_name).and_return(nil)
       end
 
       it 'returns nil' do
@@ -36,7 +36,7 @@ describe Action::GitLFSAuthenticate do
       let(:gitlab_lfs_authentication) { GitlabLfsAuthentication.new(username, lfs_token, repository_http_path) }
 
       before do
-        expect(api).to receive(:lfs_authenticate).with(key_id, repo_name).and_return(gitlab_lfs_authentication)
+        expect(api).to receive(:lfs_authenticate).with(subject, repo_name).and_return(gitlab_lfs_authentication)
       end
 
       it 'puts payload to stdout' do
diff --git a/spec/actor/key_spec.rb b/spec/actor/key_spec.rb
index 3c13f76..e61a083 100644
--- a/spec/actor/key_spec.rb
+++ b/spec/actor/key_spec.rb
@@ -11,7 +11,7 @@ describe Actor::Key do
 
   before do
     allow(GitlabNet).to receive(:new).and_return(api)
-    allow(api).to receive(:discover).with(key_id).and_return(discover_payload)
+    allow(api).to receive(:discover).with(subject).and_return(discover_payload)
   end
 
   describe '.from' do
diff --git a/spec/gitlab_keys_spec.rb b/spec/gitlab_keys_spec.rb
index 0baeed7..3cd6798 100644
--- a/spec/gitlab_keys_spec.rb
+++ b/spec/gitlab_keys_spec.rb
@@ -8,13 +8,25 @@ describe GitlabKeys do
   end
 
   describe '.command' do
+    it 'the internal "command" utility function' do
+      command = "#{ROOT_PATH}/bin/gitlab-shell does-not-validate"
+      expect(described_class.command('does-not-validate')).to eq(command)
+    end
+
+    it 'does not raise a KeyError on invalid input' do
+      command = "#{ROOT_PATH}/bin/gitlab-shell foo\nbar\nbaz\n"
+      expect(described_class.command("foo\nbar\nbaz\n")).to eq(command)
+    end
+  end
+
+  describe '.command_key' do
     it 'returns the "command" part of the key line' do
       command = "#{ROOT_PATH}/bin/gitlab-shell key-123"
-      expect(described_class.command('key-123')).to eq(command)
+      expect(described_class.command_key('key-123')).to eq(command)
     end
 
     it 'raises KeyError on invalid input' do
-      expect do described_class.command("\nssh-rsa AAA") end.to raise_error(described_class::KeyError)
+      expect { described_class.command_key("\nssh-rsa AAA") }.to raise_error(described_class::KeyError)
     end
   end
 
@@ -30,7 +42,23 @@ describe GitlabKeys do
     end
 
     it 'raises KeyError on invalid input' do
-      expect do described_class.key_line('key-741', "ssh-rsa AAA\nssh-rsa AAA") end.to raise_error(described_class::KeyError)
+      expect { described_class.key_line('key-741', "ssh-rsa AAA\nssh-rsa AAA") }.to raise_error(described_class::KeyError)
+    end
+  end
+
+  describe '.principal_line' do
+    let(:line) { %(command="#{ROOT_PATH}/bin/gitlab-shell username-someuser",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty sshUsers) }
+
+    it 'returns the key line' do
+      expect(described_class.principal_line('username-someuser', 'sshUsers')).to eq(line)
+    end
+
+    it 'silently removes a trailing newline' do
+      expect(described_class.principal_line('username-someuser', "sshUsers\n")).to eq(line)
+    end
+
+    it 'raises KeyError on invalid input' do
+      expect { described_class.principal_line('username-someuser', "sshUsers\nloginUsers") }.to raise_error(described_class::KeyError)
     end
   end
 
diff --git a/spec/gitlab_net_spec.rb b/spec/gitlab_net_spec.rb
index 2dd70af..a02744f 100644
--- a/spec/gitlab_net_spec.rb
+++ b/spec/gitlab_net_spec.rb
@@ -8,11 +8,9 @@ describe GitlabNet, vcr: true do
   let(:internal_api_endpoint) { 'http://localhost:3000/api/v4/internal' }
   let(:project) { 'gitlab-org/gitlab-test.git' }
 
-  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(:actor1) { Actor.new_from('key-1') }
+  let(:bad_actor1) { Actor.new_from('key-777') }
+  let(:actor2) { Actor.new_from('user-1') }
 
   let(:secret) { "0a3938d9d95d807e94d937af3a4fbbea\n" }
 
@@ -46,7 +44,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_id1)
+        user = gitlab_net.discover(actor1)
         expect(user['name']).to eql 'Administrator'
         expect(user['username']).to eql 'root'
       end
@@ -55,14 +53,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_id1)
+        gitlab_net.discover(actor1)
       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_id1)).to be_nil
+        expect(gitlab_net.discover(actor1)).to be_nil
       end
     end
   end
@@ -71,7 +69,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_id1, project)
+          lfs_access = gitlab_net.lfs_authenticate(actor1, 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
@@ -161,7 +159,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: key1.identifier, changes: changes }
+      { gl_repository: gl_repository, identifier: actor1.identifier, changes: changes }
     end
     let(:merge_request_urls) do
       [{
@@ -171,7 +169,7 @@ describe GitlabNet, vcr: true do
       }]
     end
 
-    subject { gitlab_net.post_receive(gl_repository, key1, changes) }
+    subject { gitlab_net.post_receive(gl_repository, actor1, changes) }
 
     it 'sends the correct parameters' do
       allow_any_instance_of(Net::HTTP::Post).to receive(:set_form_data).with(hash_including(params))
@@ -230,7 +228,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(key1_str)
+        result = gitlab_net.two_factor_recovery_codes(actor1)
         expect(result['success']).to be_truthy
         expect(result['recovery_codes']).to eq(['f67c514de60c4953','41278385fc00c1e0'])
       end
@@ -238,7 +236,7 @@ describe GitlabNet, vcr: true do
 
     it 'returns false when recovery codes cannot be generated' do
       VCR.use_cassette('two-factor-recovery-codes-fail') do
-        result = gitlab_net.two_factor_recovery_codes('key-777')
+        result = gitlab_net.two_factor_recovery_codes(bad_actor1)
         expect(result['success']).to be_falsey
         expect(result['message']).to eq('Could not find the given key')
       end
@@ -272,7 +270,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, key1, changes, 'ssh')
+              gitlab_net.check_access('git-receive-pack', nil, project, actor1, changes, 'ssh')
             end.to raise_error(UnknownError, 'API is not accessible: An internal server error occurred')
           end
         end
@@ -282,7 +280,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, key1, changes, 'ssh')
+              gitlab_net.check_access('git-receive-pack', nil, project, actor1, changes, 'ssh')
             end.to raise_error(UnknownError, 'API is not accessible')
           end
         end
@@ -292,7 +290,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, key1, changes, 'ssh')
+          action = gitlab_net.check_access('git-receive-pack', nil, project, actor1, changes, 'ssh')
           expect(action).to be_instance_of(Action::Gitaly)
         end
       end
@@ -300,13 +298,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, key1, changes, 'ssh')
+          gitlab_net.check_access('git-receive-pack', nil, project, actor1, 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, key1, changes, 'ssh')
+          action = gitlab_net.check_access('git-upload-pack', nil, project, actor1, changes, 'ssh')
           expect(action).to be_instance_of(Action::Gitaly)
         end
       end
@@ -316,13 +314,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, key1, changes, 'http')
+            gitlab_net.check_access('git-upload-pack', nil, project, actor1, 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, key1, changes, 'http')
+            gitlab_net.check_access('git-upload-pack', nil, project, actor1, changes, 'http')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
       end
@@ -330,13 +328,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, key1, changes, 'ssh')
+            gitlab_net.check_access('git-receive-pack', nil, project, actor1, 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, key1, changes, 'ssh')
+            gitlab_net.check_access('git-receive-pack', nil, project, actor1, changes, 'ssh')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
       end
@@ -346,13 +344,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, key1, changes, 'http')
+            gitlab_net.check_access('git-upload-pack', nil, project, actor1, 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, key1, changes, 'http')
+            gitlab_net.check_access('git-upload-pack', nil, project, actor1, changes, 'http')
           end.to raise_error(AccessDeniedError, 'Pulling over HTTP is not allowed.')
         end
       end
@@ -360,13 +358,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, key1, changes, 'http')
+            gitlab_net.check_access('git-receive-pack', nil, project, actor1, 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, key1, changes, 'http')
+            gitlab_net.check_access('git-receive-pack', nil, project, actor1, changes, 'http')
           end.to raise_error(AccessDeniedError, 'Pushing over HTTP is not allowed.')
         end
       end
@@ -376,13 +374,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, user1, changes, 'ssh')
+            gitlab_net.check_access('git-receive-pack', nil, project, actor2, 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, user1, changes, 'ssh')
+            gitlab_net.check_access('git-receive-pack', nil, project, actor2, changes, 'ssh')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
       end
@@ -390,13 +388,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, user1, changes, 'ssh')
+            gitlab_net.check_access('git-upload-pack', nil, project, actor2, 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, user1, changes, 'ssh')
+            gitlab_net.check_access('git-upload-pack', nil, project, actor2, changes, 'ssh')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
       end
@@ -404,13 +402,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, user1, changes, 'ssh')
+            gitlab_net.check_access('git-upload-pack', nil, project, actor2, 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, user1, changes, 'ssh')
+            gitlab_net.check_access('git-upload-pack', nil, project, actor2, changes, 'ssh')
           end.to raise_error(AccessDeniedError, 'Git access over SSH is not allowed')
         end
       end
@@ -419,7 +417,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, key1, changes, 'ssh')
+        gitlab_net.check_access('git-upload-pack', nil, project, actor1, changes, 'ssh')
       }.to raise_error(GitlabNet::ApiUnreachableError)
     end
   end
diff --git a/spec/gitlab_shell_spec.rb b/spec/gitlab_shell_spec.rb
index 456dfcf..201bc62 100644
--- a/spec/gitlab_shell_spec.rb
+++ b/spec/gitlab_shell_spec.rb
@@ -10,44 +10,44 @@ describe GitlabShell do
 
   after { FileUtils.rm_rf(tmp_repos_path) }
 
-  subject { described_class.new(key_id) }
+  subject { described_class.new(who) }
 
-  let(:key_id) { '1' }
-  let(:key) { Actor::Key.new(key_id) }
+  let(:who) { 'key-1' }
+  let(:audit_usernames) { true }
+  let(:actor) { Actor.new_from(who, audit_usernames: audit_usernames) }
   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,
+                          actor,
                           gl_repository,
                           gl_username,
                           repo_path,
                           { 'repository' => { 'relative_path' => repo_name, 'storage_name' => 'default' } , 'address' => 'unix:gitaly.socket' })
                       }
-  let(:api_2fa_recovery_action) { Action::API2FARecovery.new(key_id) }
-  let(:git_lfs_authenticate_action) { Action::GitLFSAuthenticate.new(key_id, repo_name) }
+  let(:api_2fa_recovery_action) { Action::API2FARecovery.new(actor) }
+  let(:git_lfs_authenticate_action) { Action::GitLFSAuthenticate.new(actor, 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(Actor).to receive(:new_from).with(who, audit_usernames: audit_usernames).and_return(actor)
 
     allow(GitlabNet).to receive(:new).and_return(api)
-    allow(api).to receive(:discover).with(key_id).and_return('username' => gl_username)
+    allow(api).to receive(:discover).with(actor).and_return('username' => gl_username)
   end
 
   describe '#exec' do
     context "when we don't have a valid user" do
       before do
-        allow(api).to receive(:discover).with(key_id).and_return(nil)
+        allow(api).to receive(:discover).with(actor).and_return(nil)
       end
 
       it 'prints Welcome.. and returns true' do
@@ -114,7 +114,7 @@ describe GitlabShell do
       let(:git_access) { '2fa_recovery_codes' }
 
       before do
-        expect(Action::API2FARecovery).to receive(:new).with(key).and_return(api_2fa_recovery_action)
+        expect(Action::API2FARecovery).to receive(:new).with(actor).and_return(api_2fa_recovery_action)
       end
 
       it 'returns true' do
@@ -125,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, '_any').and_raise(AccessDeniedError, 'Sorry, access denied')
+        expect(api).to receive(:check_access).with('git-upload-pack', nil, repo_name, actor, '_any').and_raise(AccessDeniedError, 'Sorry, access denied')
       end
 
       it 'prints a message to stderr and returns false' do
@@ -136,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, '_any').and_raise(GitlabNet::ApiUnreachableError)
+        expect(api).to receive(:check_access).with('git-upload-pack', nil, repo_name, actor, '_any').and_raise(GitlabNet::ApiUnreachableError)
       end
 
       it 'prints a message to stderr and returns false' do
@@ -147,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, '_any').and_return(gitaly_action)
+        expect(api).to receive(:check_access).with(git_access, nil, repo_name, actor, '_any').and_return(gitaly_action)
       end
 
       context 'when origin_cmd is git-upload-pack' do
@@ -180,7 +180,7 @@ describe GitlabShell do
         let(:lfs_access) { double(GitlabLfsAuthentication, authentication_payload: fake_payload)}
 
         before do
-          expect(Action::GitLFSAuthenticate).to receive(:new).with(key, repo_name).and_return(git_lfs_authenticate_action)
+          expect(Action::GitLFSAuthenticate).to receive(:new).with(actor, repo_name).and_return(git_lfs_authenticate_action)
         end
 
         context 'upload' do