Merge branch '223218-re-introduce-ssh-key-comments-via-the-api' into 'master'

Do not mask key comments for DeployKeys

Closes #223218

See merge request gitlab-org/gitlab!35014

changelogs/unreleased/223218-re-introduce-ssh-key-comments-via-the-api.yml

+---
+title: Do not mask key comments for DeployKeys
+merge_request: 35014
+author:
+type: fixed

lib/api/deploy_keys.rb

@@ -25,8 +25,7 @@ module API
     get "deploy_keys" do
       authenticated_as_admin!
 
-      deploy_keys = DeployKey.all.preload_users
-      present paginate(deploy_keys), with: Entities::SSHKey
+      present paginate(DeployKey.all), with: Entities::DeployKey
     end
 
     params do
@@ -43,7 +42,7 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       get ":id/deploy_keys" do
-        keys = user_project.deploy_keys_projects.preload(deploy_key: [:user])
+        keys = user_project.deploy_keys_projects.preload(:deploy_key)
 
         present paginate(keys), with: Entities::DeployKeysProject
       end
@@ -105,7 +104,7 @@ module API
       # rubocop: enable CodeReuse/ActiveRecord
 
       desc 'Update an existing deploy key for a project' do
-        success Entities::SSHKey
+        success Entities::DeployKey
       end
       params do
         requires :key_id, type: Integer, desc: 'The ID of the deploy key'
@@ -140,7 +139,7 @@ module API
 
       desc 'Enable a deploy key for a project' do
         detail 'This feature was added in GitLab 8.11'
-        success Entities::SSHKey
+        success Entities::DeployKey
       end
       params do
         requires :key_id, type: Integer, desc: 'The ID of the deploy key'
@@ -150,7 +149,7 @@ module API
                                                       current_user, declared_params).execute
 
         if key
-          present key, with: Entities::SSHKey
+          present key, with: Entities::DeployKey
         else
           not_found!('Deploy Key')
         end

lib/api/entities/deploy_key.rb

+# frozen_string_literal: true
+
+module API
+  module Entities
+    class DeployKey < Entities::SSHKey
+      expose :key
+    end
+  end
+end

lib/api/entities/deploy_key_with_user.rb

@@ -2,7 +2,8 @@
 
 module API
   module Entities
-    class DeployKeyWithUser < Entities::SSHKeyWithUser
+    class DeployKeyWithUser < Entities::DeployKey
+      expose :user, using: Entities::UserPublic
       expose :deploy_keys_projects
     end
   end

lib/api/entities/deploy_keys_project.rb

@@ -3,7 +3,7 @@
 module API
   module Entities
     class DeployKeysProject < Grape::Entity
-      expose :deploy_key, merge: true, using: Entities::SSHKey
+      expose :deploy_key, merge: true, using: Entities::DeployKey
       expose :can_push
     end
   end

spec/lib/api/entities/deploy_key_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Entities::DeployKey do
+  describe '#as_json' do
+    subject { entity.as_json }
+
+    let(:deploy_key) { create(:deploy_key, public: true) }
+    let(:entity) { described_class.new(deploy_key) }
+
+    it 'includes basic fields', :aggregate_failures do
+      is_expected.to include(
+        id: deploy_key.id,
+        title: deploy_key.title,
+        created_at: deploy_key.created_at,
+        expires_at: deploy_key.expires_at,
+        key: deploy_key.key
+      )
+    end
+  end
+end

spec/lib/api/entities/deploy_keys_project_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Entities::DeployKeysProject do
+  describe '#as_json' do
+    subject { entity.as_json }
+
+    let(:deploy_keys_project) { create(:deploy_keys_project, :write_access) }
+    let(:entity) { described_class.new(deploy_keys_project) }
+
+    it 'includes basic fields', :aggregate_failures do
+      deploy_key = deploy_keys_project.deploy_key
+
+      is_expected.to include(
+        id: deploy_key.id,
+        title: deploy_key.title,
+        created_at: deploy_key.created_at,
+        expires_at: deploy_key.expires_at,
+        key: deploy_key.key,
+        can_push: deploy_keys_project.can_push
+      )
+    end
+  end
+end

spec/lib/api/entities/ssh_key_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Entities::SSHKey do
+  describe '#as_json' do
+    subject { entity.as_json }
+
+    let(:key) { create(:key, user: create(:user)) }
+    let(:entity) { described_class.new(key) }
+
+    it 'includes basic fields', :aggregate_failures do
+      is_expected.to include(
+        id: key.id,
+        title: key.title,
+        created_at: key.created_at,
+        expires_at: key.expires_at,
+        key: key.publishable_key
+      )
+    end
+  end
+end

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, user: user) }
+  let(:deploy_key)  { create(:deploy_key, public: true) }
 
   let!(:deploy_keys_project) do
     create(:deploy_keys_project, project: project, deploy_key: deploy_key)
@@ -40,32 +40,6 @@ 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
 
@@ -82,25 +56,6 @@ 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
@@ -111,13 +66,6 @@ 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)