Merge branch '235756-ssh-public-keys-no-longer-public-accessible' into 'master'

Make SSH keys publicly accessible

Closes #235756

See merge request gitlab-org/gitlab!42288

app/controllers/profiles/keys_controller.rb

 # frozen_string_literal: true
 
 class Profiles::KeysController < Profiles::ApplicationController
+  skip_before_action :authenticate_user!, only: [:get_keys]
+
   def index
     @keys = current_user.keys.order_id_desc
     @key = Key.new
@@ -31,6 +33,25 @@ class Profiles::KeysController < Profiles::ApplicationController
     end
   end
 
+  # Get all keys of a user(params[:username]) in a text format
+  # Helpful for sysadmins to put in respective servers
+  def get_keys
+    if params[:username].present?
+      begin
+        user = UserFinder.new(params[:username]).find_by_username
+        if user.present?
+          render plain: user.all_ssh_keys.join("\n")
+        else
+          render_404
+        end
+      rescue => e
+        render html: e.message
+      end
+    else
+      render_404
+    end
+  end
+
   private
 
   def key_params

app/controllers/users_controller.rb

@@ -37,12 +37,6 @@ class UsersController < ApplicationController
     end
   end
 
-  # Get all keys of a user(params[:username]) in a text format
-  # Helpful for sysadmins to put in respective servers
-  def ssh_keys
-    render plain: user.all_ssh_keys.join("\n")
-  end
-
   def activity
     respond_to do |format|
       format.html { render 'show' }

changelogs/unreleased/235756-ssh-public-keys-no-longer-public-accessible.yml

+---
+title: Make SSH keys publicly accessible
+merge_request: 42288
+author:
+type: fixed

config/routes/user.rb

@@ -55,7 +55,7 @@ end
 
 constraints(::Constraints::UserUrlConstrainer.new) do
   # Get all keys of user
-  get ':username.keys', controller: :users, action: :ssh_keys, constraints: { username: Gitlab::PathRegex.root_namespace_route_regex }
+  get ':username.keys' => 'profiles/keys#get_keys', constraints: { username: Gitlab::PathRegex.root_namespace_route_regex }
 
   scope(path: ':username',
         as: :user,

spec/controllers/profiles/keys_controller_spec.rb

@@ -20,4 +20,108 @@ RSpec.describe Profiles::KeysController do
       expect(Key.last.expires_at).to be_like_time(expires_at)
     end
   end
+
+  describe "#get_keys" do
+    describe "non existent user" do
+      it "does not generally work" do
+        get :get_keys, params: { username: 'not-existent' }
+
+        expect(response).not_to be_successful
+      end
+    end
+
+    describe "user with no keys" do
+      it "does generally work" do
+        get :get_keys, params: { username: user.username }
+
+        expect(response).to be_successful
+      end
+
+      it "renders all keys separated with a new line" do
+        get :get_keys, params: { username: user.username }
+
+        expect(response.body).to eq("")
+      end
+      it "responds with text/plain content type" do
+        get :get_keys, params: { username: user.username }
+        expect(response.content_type).to eq("text/plain")
+      end
+    end
+
+    describe "user with keys" do
+      let!(:key) { create(:key, user: user) }
+      let!(:another_key) { create(:another_key, user: user) }
+      let!(:deploy_key) { create(:deploy_key, user: user) }
+
+      describe "while signed in" do
+        before do
+          sign_in(user)
+        end
+
+        it "does generally work" do
+          get :get_keys, params: { username: user.username }
+
+          expect(response).to be_successful
+        end
+
+        it "renders all non deploy keys separated with a new line" do
+          get :get_keys, params: { username: user.username }
+
+          expect(response.body).not_to eq('')
+          expect(response.body).to eq(user.all_ssh_keys.join("\n"))
+
+          expect(response.body).to include(key.key.sub(' dummy@gitlab.com', ''))
+          expect(response.body).to include(another_key.key.sub(' dummy@gitlab.com', ''))
+
+          expect(response.body).not_to include(deploy_key.key)
+        end
+
+        it "does not render the comment of the key" do
+          get :get_keys, params: { username: user.username }
+          expect(response.body).not_to match(/dummy@gitlab.com/)
+        end
+
+        it "responds with text/plain content type" do
+          get :get_keys, params: { username: user.username }
+
+          expect(response.content_type).to eq("text/plain")
+        end
+      end
+
+      describe 'when logged out' do
+        before do
+          sign_out(user)
+        end
+
+        it "still does generally work" do
+          get :get_keys, params: { username: user.username }
+
+          expect(response).to be_successful
+        end
+
+        it "renders all non deploy keys separated with a new line" do
+          get :get_keys, params: { username: user.username }
+
+          expect(response.body).not_to eq('')
+          expect(response.body).to eq(user.all_ssh_keys.join("\n"))
+
+          expect(response.body).to include(key.key.sub(' dummy@gitlab.com', ''))
+          expect(response.body).to include(another_key.key.sub(' dummy@gitlab.com', ''))
+
+          expect(response.body).not_to include(deploy_key.key)
+        end
+
+        it "does not render the comment of the key" do
+          get :get_keys, params: { username: user.username }
+          expect(response.body).not_to match(/dummy@gitlab.com/)
+        end
+
+        it "responds with text/plain content type" do
+          get :get_keys, params: { username: user.username }
+
+          expect(response.content_type).to eq("text/plain")
+        end
+      end
+    end
+  end
 end

spec/controllers/users_controller_spec.rb

@@ -114,71 +114,6 @@ RSpec.describe UsersController do
     end
   end
 
-  describe "#ssh_keys" do
-    describe "non existent user" do
-      it "does not generally work" do
-        get :ssh_keys, params: { username: 'not-existent' }
-
-        expect(response).not_to be_successful
-      end
-    end
-
-    describe "user with no keys" do
-      it "does generally work" do
-        get :ssh_keys, params: { username: user.username }
-
-        expect(response).to be_successful
-      end
-
-      it "renders all keys separated with a new line" do
-        get :ssh_keys, params: { username: user.username }
-
-        expect(response.body).to eq("")
-      end
-
-      it "responds with text/plain content type" do
-        get :ssh_keys, params: { username: user.username }
-        expect(response.content_type).to eq("text/plain")
-      end
-    end
-
-    describe "user with keys" do
-      let!(:key) { create(:key, user: user) }
-      let!(:another_key) { create(:another_key, user: user) }
-      let!(:deploy_key) { create(:deploy_key, user: user) }
-
-      it "does generally work" do
-        get :ssh_keys, params: { username: user.username }
-
-        expect(response).to be_successful
-      end
-
-      it "renders all non deploy keys separated with a new line" do
-        get :ssh_keys, params: { username: user.username }
-
-        expect(response.body).not_to eq('')
-        expect(response.body).to eq(user.all_ssh_keys.join("\n"))
-
-        expect(response.body).to include(key.key.sub(' dummy@gitlab.com', ''))
-        expect(response.body).to include(another_key.key.sub(' dummy@gitlab.com', ''))
-
-        expect(response.body).not_to include(deploy_key.key)
-      end
-
-      it "does not render the comment of the key" do
-        get :ssh_keys, params: { username: user.username }
-
-        expect(response.body).not_to match(/dummy@gitlab.com/)
-      end
-
-      it "responds with text/plain content type" do
-        get :ssh_keys, params: { username: user.username }
-
-        expect(response.content_type).to eq("text/plain")
-      end
-    end
-  end
-
   describe 'GET #calendar' do
     context 'for user' do
       let(:project) { create(:project) }

spec/routing/routing_spec.rb

@@ -32,13 +32,6 @@ RSpec.describe UsersController, "routing" do
     expect(get("/users/User/snippets")).to route_to('users#snippets', username: 'User')
   end
 
-  # get all the ssh-keys of a user
-  it "to #get_keys" do
-    allow_any_instance_of(::Constraints::UserUrlConstrainer).to receive(:matches?).and_return(true)
-
-    expect(get("/User.keys")).to route_to('users#ssh_keys', username: 'User')
-  end
-
   it "to #calendar" do
     expect(get("/users/User/calendar")).to route_to('users#calendar', username: 'User')
   end
@@ -193,6 +186,12 @@ RSpec.describe Profiles::KeysController, "routing" do
   it "to #destroy" do
     expect(delete("/profile/keys/1")).to route_to('profiles/keys#destroy', id: '1')
   end
+
+  it "to #get_keys" do
+    allow_any_instance_of(::Constraints::UserUrlConstrainer).to receive(:matches?).and_return(true)
+
+    expect(get("/foo.keys")).to route_to('profiles/keys#get_keys', username: 'foo')
+  end
 end
 
 #   emails GET    /emails(.:format)        emails#index