Merge branch '220185-mask-key-comments-when-exposing-ssh-deploy-keys-via-the-api' into 'master'

Resolve "Mask key comments when exposing SSH/Deploy Keys via the API"

See merge request gitlab-org/gitlab!34255

changelogs/unreleased/220185-mask-key-comments-when-exposing-ssh-deploy-keys-via-the-api.yml

+---
+title: Mask key comments when exposing SSH/Deploy Keys via the API
+merge_request: 34255
+author:
+type: added

lib/api/deploy_keys.rb

@@ -25,7 +25,8 @@ module API
     get "deploy_keys" do
       authenticated_as_admin!
 
-      present paginate(DeployKey.all), with: Entities::SSHKey
+      deploy_keys = DeployKey.all.preload_users
+      present paginate(deploy_keys), with: Entities::SSHKey
     end
 
     params do
@@ -42,7 +43,7 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       get ":id/deploy_keys" do
-        keys = user_project.deploy_keys_projects.preload(:deploy_key)
+        keys = user_project.deploy_keys_projects.preload(deploy_key: [:user])
 
         present paginate(keys), with: Entities::DeployKeysProject
       end

lib/api/entities/ssh_key.rb

@@ -3,7 +3,8 @@
 module API
   module Entities
     class SSHKey < Grape::Entity
-      expose :id, :title, :key, :created_at, :expires_at
+      expose :id, :title, :created_at, :expires_at
+      expose :publishable_key, as: :key
     end
   end
 end

lib/api/users.rb

@@ -285,7 +285,8 @@ module API
         user = find_user(params[:user_id])
         not_found!('User') unless user && can?(current_user, :read_user, user)
 
-        present paginate(user.keys), with: Entities::SSHKey
+        keys = user.keys.preload_users
+        present paginate(keys), with: Entities::SSHKey
       end
 
       desc 'Delete an existing SSH key from a specified user. Available only for admins.' do
@@ -697,7 +698,9 @@ module API
         use :pagination
       end
       get "keys" do
-        present paginate(current_user.keys), with: Entities::SSHKey
+        keys = current_user.keys.preload_users
+
+        present paginate(keys), with: Entities::SSHKey
       end
 
       desc 'Get a single key owned by currently authenticated user' do

spec/requests/api/deploy_keys_spec.rb

@@ -8,7 +8,7 @@ describe API::DeployKeys do
   let(:admin)       { create(:admin) }
   let(:project)     { create(:project, creator_id: user.id) }
   let(:project2)    { create(:project, creator_id: user.id) }
-  let(:deploy_key)  { create(:deploy_key, public: true) }
+  let(:deploy_key)  { create(:deploy_key, public: true, user: user) }
 
   let!(:deploy_keys_project) do
     create(:deploy_keys_project, project: project, deploy_key: deploy_key)
@@ -40,6 +40,32 @@ describe API::DeployKeys do
         expect(json_response).to be_an Array
         expect(json_response.first['id']).to eq(deploy_keys_project.deploy_key.id)
       end
+
+      it 'returns all deploy keys with comments replaced with'\
+         'a simple identifier of username + hostname' do
+        get api('/deploy_keys', admin)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+
+        keys = json_response.map { |key_detail| key_detail['key'] }
+        expect(keys).to all(include("#{user.name} (#{Gitlab.config.gitlab.host}"))
+      end
+
+      context 'N+1 queries' do
+        before do
+          get api('/deploy_keys', admin)
+        end
+
+        it 'avoids N+1 queries', :request_store do
+          control_count = ActiveRecord::QueryRecorder.new { get api('/deploy_keys', admin) }.count
+
+          create_list(:deploy_key, 2, public: true, user: create(:user))
+
+          expect { get api('/deploy_keys', admin) }.not_to exceed_query_limit(control_count)
+        end
+      end
     end
   end
 
@@ -56,6 +82,25 @@ describe API::DeployKeys do
       expect(json_response).to be_an Array
       expect(json_response.first['title']).to eq(deploy_key.title)
     end
+
+    context 'N+1 queries' do
+      before do
+        get api("/projects/#{project.id}/deploy_keys", admin)
+      end
+
+      it 'avoids N+1 queries', :request_store do
+        control_count = ActiveRecord::QueryRecorder.new do
+          get api("/projects/#{project.id}/deploy_keys", admin)
+        end.count
+
+        deploy_key = create(:deploy_key, user: create(:user))
+        create(:deploy_keys_project, project: project, deploy_key: deploy_key)
+
+        expect do
+          get api("/projects/#{project.id}/deploy_keys", admin)
+        end.not_to exceed_query_limit(control_count)
+      end
+    end
   end
 
   describe 'GET /projects/:id/deploy_keys/:key_id' do
@@ -66,6 +111,13 @@ describe API::DeployKeys do
       expect(json_response['title']).to eq(deploy_key.title)
     end
 
+    it 'exposes key comment as a simple identifier of username + hostname' do
+      get api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin)
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response['key']).to include("#{deploy_key.user_name} (#{Gitlab.config.gitlab.host})")
+    end
+
     it 'returns 404 Not Found with invalid ID' do
       get api("/projects/#{project.id}/deploy_keys/404", admin)
 

spec/requests/api/internal/base_spec.rb

@@ -218,7 +218,15 @@ describe API::Internal::Base do
         get(api('/internal/authorized_keys'), params: { fingerprint: key.fingerprint, secret_token: secret_token })
 
         expect(response).to have_gitlab_http_status(:ok)
-        expect(json_response["key"]).to eq(key.key)
+        expect(json_response['id']).to eq(key.id)
+        expect(json_response['key'].split[1]).to eq(key.key.split[1])
+      end
+
+      it 'exposes the comment of the key as a simple identifier of username + hostname' do
+        get(api('/internal/authorized_keys'), params: { fingerprint: key.fingerprint, secret_token: secret_token })
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['key']).to include("#{key.user_name} (#{Gitlab.config.gitlab.host})")
       end
     end
 
@@ -239,11 +247,21 @@ describe API::Internal::Base do
     end
 
     context "sending the key" do
-      it "finds the key" do
-        get(api('/internal/authorized_keys'), params: { key: key.key.split[1], secret_token: secret_token })
+      context "using an existing key" do
+        it "finds the key" do
+          get(api('/internal/authorized_keys'), params: { key: key.key.split[1], secret_token: secret_token })
 
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(json_response["key"]).to eq(key.key)
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response['id']).to eq(key.id)
+          expect(json_response['key'].split[1]).to eq(key.key.split[1])
+        end
+
+        it 'exposes the comment of the key as a simple identifier of username + hostname' do
+          get(api('/internal/authorized_keys'), params: { fingerprint: key.fingerprint, secret_token: secret_token })
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response['key']).to include("#{key.user_name} (#{Gitlab.config.gitlab.host})")
+        end
       end
 
       it "returns 404 with a partial key" do

spec/requests/api/users_spec.rb

@@ -1276,6 +1276,36 @@ describe API::Users, :do_not_mock_admin_mode do
       expect(json_response).to be_an Array
       expect(json_response.first['title']).to eq(key.title)
     end
+
+    it 'returns array of ssh keys with comments replaced with'\
+      'a simple identifier of username + hostname' do
+      get api("/users/#{user.id}/keys")
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(response).to include_pagination_headers
+      expect(json_response).to be_an Array
+
+      keys = json_response.map { |key_detail| key_detail['key'] }
+      expect(keys).to all(include("#{user.name} (#{Gitlab.config.gitlab.host}"))
+    end
+
+    context 'N+1 queries' do
+      before do
+        get api("/users/#{user.id}/keys")
+      end
+
+      it 'avoids N+1 queries', :request_store do
+        control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do
+          get api("/users/#{user.id}/keys")
+        end.count
+
+        create_list(:key, 2, user: user)
+
+        expect do
+          get api("/users/#{user.id}/keys")
+        end.not_to exceed_all_query_limit(control_count)
+      end
+    end
   end
 
   describe 'GET /user/:user_id/keys' do
@@ -1751,6 +1781,36 @@ describe API::Users, :do_not_mock_admin_mode do
         expect(json_response.first["title"]).to eq(key.title)
       end
 
+      it 'returns array of ssh keys with comments replaced with'\
+        'a simple identifier of username + hostname' do
+        get api("/user/keys", user)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+
+        keys = json_response.map { |key_detail| key_detail['key'] }
+        expect(keys).to all(include("#{user.name} (#{Gitlab.config.gitlab.host}"))
+      end
+
+      context 'N+1 queries' do
+        before do
+          get api("/user/keys", user)
+        end
+
+        it 'avoids N+1 queries', :request_store do
+          control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do
+            get api("/user/keys", user)
+          end.count
+
+          create_list(:key, 2, user: user)
+
+          expect do
+            get api("/user/keys", user)
+          end.not_to exceed_all_query_limit(control_count)
+        end
+      end
+
       context "scopes" do
         let(:path) { "/user/keys" }
         let(:api_call) { method(:api) }
@@ -1769,6 +1829,13 @@ describe API::Users, :do_not_mock_admin_mode do
       expect(json_response["title"]).to eq(key.title)
     end
 
+    it 'exposes SSH key comment as a simple identifier of username + hostname' do
+      get api("/user/keys/#{key.id}", user)
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response['key']).to include("#{key.user_name} (#{Gitlab.config.gitlab.host})")
+    end
+
     it "returns 404 Not Found within invalid ID" do
       get api("/user/keys/#{non_existing_record_id}", user)