Fix two regressions in SSH certificate support

Fix two regressions in my 2e8b670 ("Add support for SSH certificate
authentication", 2018-06-14) merged in gitlab-org/gitlab-shell!207.

This fixes the issue noted in gitlab-org/gitlab-shell#145 where the
command-line contains things other than the key/user/username, and
also a regression where SSH certificates are being used, and the
username presented in the key is unknown to GitLab.

In that case, we should log the user in as "Anonymous" (on an instance
that allows public access), but because of how the error checking
around api.discover() was implemented we ended up erroring out
instead.

4 files changed, 172 insertions, 2 deletions

diff --git a/bin/gitlab-shell b/bin/gitlab-shell
index 1016570..ae751d7 100755
--- a/bin/gitlab-shell
+++ b/bin/gitlab-shell
@@ -17,7 +17,11 @@ require_relative '../lib/gitlab_init'
 #
 require File.join(ROOT_PATH, 'lib', 'gitlab_shell')
 
-if GitlabShell.new(ARGV.join).exec(original_cmd)
+# We must match e.g. "key-12345" anywhere on the command-line. See
+# https://gitlab.com/gitlab-org/gitlab-shell/issues/145
+who = /\b(?:(?:key|user)-[0-9]+|username-\S+)\b/.match(ARGV.join).to_s;
+
+if GitlabShell.new(who).exec(original_cmd)
   exit 0
 else
   exit 1
diff --git a/lib/gitlab_shell.rb b/lib/gitlab_shell.rb
index 78fdfe8..c6b28ce 100644
--- a/lib/gitlab_shell.rb
+++ b/lib/gitlab_shell.rb
@@ -208,7 +208,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
     begin
       if defined?(@who)
         @user = api.discover(@who)
-        @gl_id = "user-#{@user['id']}"
+        @gl_id = "user-#{@user['id']}" if @user
       else
         @user = api.discover(@gl_id)
       end
diff --git a/spec/gitlab_shell_authorized_keys_check_spec.rb b/spec/gitlab_shell_authorized_keys_check_spec.rb
index 30237e0..baaa560 100644
--- a/spec/gitlab_shell_authorized_keys_check_spec.rb
+++ b/spec/gitlab_shell_authorized_keys_check_spec.rb
@@ -5,6 +5,9 @@ describe 'bin/gitlab-shell-authorized-keys-check' do
     ROOT_PATH
   end
 
+  # All this test boilerplate is mostly copy/pasted between
+  # gitlab_shell_gitlab_shell_spec.rb and
+  # gitlab_shell_authorized_keys_check_spec.rb
   def tmp_root_path
     @tmp_root_path ||= File.realpath(Dir.mktmpdir)
   end
diff --git a/spec/gitlab_shell_gitlab_shell_spec.rb b/spec/gitlab_shell_gitlab_shell_spec.rb
new file mode 100644
index 0000000..76e2afd
--- /dev/null
+++ b/spec/gitlab_shell_gitlab_shell_spec.rb
@@ -0,0 +1,163 @@
+require_relative 'spec_helper'
+
+describe 'bin/gitlab-shell' do
+  def original_root_path
+    ROOT_PATH
+  end
+
+  # All this test boilerplate is mostly copy/pasted between
+  # gitlab_shell_gitlab_shell_spec.rb and
+  # gitlab_shell_authorized_keys_check_spec.rb
+  def tmp_root_path
+    @tmp_root_path ||= File.realpath(Dir.mktmpdir)
+  end
+
+  def config_path
+    File.join(tmp_root_path, 'config.yml')
+  end
+
+  def tmp_socket_path
+    # This has to be a relative path shorter than 100 bytes due to
+    # limitations in how Unix sockets work.
+    'tmp/gitlab-shell-socket'
+  end
+
+  before(:all) do
+    FileUtils.mkdir_p(File.dirname(tmp_socket_path))
+    FileUtils.touch(File.join(tmp_root_path, '.gitlab_shell_secret'))
+
+    @server = HTTPUNIXServer.new(BindAddress: tmp_socket_path)
+    @server.mount_proc('/api/v4/internal/discover') do |req, res|
+      if req.query['key_id'] == '100' ||
+         req.query['user_id'] == '10' ||
+         req.query['username'] == 'someuser'
+        res.status = 200
+        res.content_type = 'application/json'
+        res.body = '{"id":1, "name": "Some User", "username": "someuser"}'
+      else
+        res.status = 500
+      end
+    end
+
+    @webrick_thread = Thread.new { @server.start }
+
+    sleep(0.1) while @webrick_thread.alive? && @server.status != :Running
+    raise "Couldn't start stub GitlabNet server" unless @server.status == :Running
+
+    File.open(config_path, 'w') do |f|
+      f.write("---\ngitlab_url: http+unix://#{CGI.escape(tmp_socket_path)}\n")
+    end
+
+    copy_dirs = ['bin', 'lib']
+    FileUtils.rm_rf(copy_dirs.map { |d| File.join(tmp_root_path, d) })
+    FileUtils.cp_r(copy_dirs, tmp_root_path)
+  end
+
+  after(:all) do
+    @server.shutdown if @server
+    @webrick_thread.join if @webrick_thread
+    FileUtils.rm_rf(tmp_root_path)
+  end
+
+  let(:gitlab_shell_path) { File.join(tmp_root_path, 'bin', 'gitlab-shell') }
+
+  # Basic valid input
+  it 'succeeds and prints username when a valid known key id is given' do
+    output, status = run!(["key-100"])
+
+    expect(output).to eq("Welcome to GitLab, @someuser!\n")
+    expect(status).to be_success
+  end
+
+  it 'succeeds and prints username when a valid known user id is given' do
+    output, status = run!(["user-10"])
+
+    expect(output).to eq("Welcome to GitLab, @someuser!\n")
+    expect(status).to be_success
+  end
+
+  it 'succeeds and prints username when a valid known username is given' do
+    output, status = run!(["username-someuser"])
+
+    expect(output).to eq("Welcome to GitLab, @someuser!\n")
+    expect(status).to be_success
+  end
+
+  # Valid but unknown input
+  it 'succeeds and prints Anonymous when a valid unknown key id is given' do
+    output, status = run!(["key-12345"])
+
+    expect(output).to eq("Welcome to GitLab, Anonymous!\n")
+    expect(status).to be_success
+  end
+
+  it 'succeeds and prints Anonymous when a valid unknown user id is given' do
+    output, status = run!(["user-12345"])
+
+    expect(output).to eq("Welcome to GitLab, Anonymous!\n")
+    expect(status).to be_success
+  end
+
+  it 'succeeds and prints Anonymous when a valid unknown username is given' do
+    output, status = run!(["username-unknown"])
+
+    expect(output).to eq("Welcome to GitLab, Anonymous!\n")
+    expect(status).to be_success
+  end
+
+  # Invalid input. TODO: capture stderr & compare
+  it 'gets an ArgumentError on invalid input (empty)' do
+    output, status = run!([])
+
+    expect(output).to eq("")
+    expect(status).not_to be_success
+  end
+
+  it 'gets an ArgumentError on invalid input (unknown)' do
+    output, status = run!(["whatever"])
+
+    expect(output).to eq("")
+    expect(status).not_to be_success
+  end
+
+  it 'gets an ArgumentError on invalid input (multiple unknown)' do
+    output, status = run!(["this", "is", "all", "invalid"])
+
+    expect(output).to eq("")
+    expect(status).not_to be_success
+  end
+
+  # Not so basic valid input
+  # (https://gitlab.com/gitlab-org/gitlab-shell/issues/145)
+  it 'succeeds and prints username when a valid known key id is given in the middle of other input' do
+    output, status = run!(["-c/usr/share/webapps/gitlab-shell/bin/gitlab-shell key-100"])
+
+    expect(output).to eq("Welcome to GitLab, @someuser!\n")
+    expect(status).to be_success
+  end
+
+  it 'succeeds and prints username when a valid known user id is given in the middle of other input' do
+    output, status = run!(["-c/usr/share/webapps/gitlab-shell/bin/gitlab-shell user-10"])
+
+    expect(output).to eq("Welcome to GitLab, @someuser!\n")
+    expect(status).to be_success
+  end
+
+  it 'succeeds and prints username when a valid known username is given in the middle of other input' do
+    output, status = run!(["-c/usr/share/webapps/gitlab-shell/bin/gitlab-shell username-someuser"])
+
+    expect(output).to eq("Welcome to GitLab, @someuser!\n")
+    expect(status).to be_success
+  end
+
+  def run!(args)
+    cmd = [
+      gitlab_shell_path,
+      args
+    ].flatten.compact
+
+    output = IO.popen({'SSH_CONNECTION' => 'fake'}, cmd, &:read)
+
+    [output, $?]
+  end
+end