Merge branch '225248-add-delete-ssh-to-credential-inventory' into 'master'

Add Delete buttons to the SSH tab of the credential inventory

See merge request gitlab-org/gitlab!41592

app/assets/javascripts/pages/admin/credentials/index.js

+import initConfirmModal from '~/confirm_modal';
+
+initConfirmModal();

app/assets/javascripts/pages/groups/security/credentials/index.js

+import initConfirmModal from '~/confirm_modal';
+
+initConfirmModal();

app/finders/keys_finder.rb

 # frozen_string_literal: true
 class KeysFinder
+  delegate :find, :find_by_id, to: :execute
+
   InvalidFingerprint = Class.new(StandardError)
   GitLabAccessDeniedError = Class.new(StandardError)
 

app/helpers/profiles_helper.rb

@@ -29,19 +29,4 @@ module ProfilesHelper
   def user_profile?
     params[:controller] == 'users'
   end
-
-  def ssh_key_delete_modal_data(key, is_admin)
-    {
-        path: path_to_key(key, is_admin),
-        method: 'delete',
-        qa_selector: 'delete_ssh_key_button',
-        modal_attributes: {
-            'data-qa-selector': 'ssh_key_delete_modal',
-            title: _('Are you sure you want to delete this SSH key?'),
-            message: _('This action cannot be undone, and will permanently delete the %{key} SSH key') % { key: key.title },
-            okVariant: 'danger',
-            okTitle: _('Delete')
-        }
-    }
-  end
 end

app/helpers/ssh_keys_helper.rb

+# frozen_string_literal: true
+
+module SshKeysHelper
+  def ssh_key_delete_modal_data(key, path)
+    {
+        path: path,
+        method: 'delete',
+        qa_selector: 'delete_ssh_key_button',
+        modal_attributes: {
+            'data-qa-selector': 'ssh_key_delete_modal',
+            title: _('Are you sure you want to delete this SSH key?'),
+            message: _('This action cannot be undone, and will permanently delete the %{key} SSH key') % { key: key.title },
+            okVariant: 'danger',
+            okTitle: _('Delete')
+        }
+    }
+  end
+end

app/views/profiles/keys/_key.html.haml

@@ -27,6 +27,6 @@
         = s_('Profiles|Created%{time_ago}'.html_safe) % { time_ago: time_ago_with_tooltip(key.created_at, html_class: 'gl-ml-2')}
         - if key.can_delete?
           .gl-ml-3
-            = button_to '#', class: "btn btn-default gl-button btn-default-tertiary js-confirm-modal-button", data: ssh_key_delete_modal_data(key, is_admin) do
+            = button_to '#', class: "btn btn-default gl-button btn-default-tertiary js-confirm-modal-button", data: ssh_key_delete_modal_data(key, path_to_key(key, is_admin)) do
               %span.sr-only= _('Delete')
               = sprite_icon('remove')

app/views/profiles/keys/_key_details.html.haml

@@ -38,4 +38,4 @@
   .col-md-12
     .float-right
       - if @key.can_delete?
-        = button_to _('Delete'), '#', class: "btn btn-danger gl-button delete-key js-confirm-modal-button", data: ssh_key_delete_modal_data(@key, is_admin)
+        = button_to _('Delete'), '#', class: "btn btn-danger gl-button delete-key js-confirm-modal-button", data: ssh_key_delete_modal_data(@key, path_to_key(@key, is_admin))

ee/app/controllers/admin/credentials_controller.rb

@@ -5,9 +5,9 @@ class Admin::CredentialsController < Admin::ApplicationController
   include CredentialsInventoryActions
   include Analytics::UniqueVisitsHelper
 
-  helper_method :credentials_inventory_path, :user_detail_path, :personal_access_token_revoke_path, :revoke_button_available?
+  helper_method :credentials_inventory_path, :user_detail_path, :personal_access_token_revoke_path, :revoke_button_available?, :ssh_key_delete_path
 
-  before_action :check_license_credentials_inventory_available!, only: [:index, :revoke]
+  before_action :check_license_credentials_inventory_available!, only: [:index, :revoke, :destroy]
 
   track_unique_visits :index, target_id: 'i_compliance_credential_inventory'
 
@@ -29,6 +29,11 @@ class Admin::CredentialsController < Admin::ApplicationController
     admin_user_path(user)
   end
 
+  override :ssh_key_delete_path
+  def ssh_key_delete_path(key)
+    admin_credential_path(key)
+  end
+
   override :personal_access_token_revoke_path
   def personal_access_token_revoke_path(token)
     revoke_admin_credential_path(token)

ee/app/controllers/concerns/credentials_inventory_actions.rb

@@ -14,6 +14,22 @@ module CredentialsInventoryActions
     end
   end
 
+  def destroy
+    key = KeysFinder.new({ users: users, key_type: 'ssh' }).find_by_id(params[:id])
+
+    alert = if key.present?
+              if Keys::DestroyService.new(current_user).execute(key)
+                _('User key was successfully removed.')
+              else
+                _('Failed to remove user key.')
+              end
+            else
+              _('Cannot find user key.')
+            end
+
+    redirect_to credentials_inventory_path(filter: 'ssh_keys'), status: :found, notice: alert
+  end
+
   def revoke
     personal_access_token = PersonalAccessTokensFinder.new({ user: users, impersonation: false }, current_user).find(params[:id])
     service = PersonalAccessTokens::RevokeService.new(current_user, token: personal_access_token).execute

ee/app/controllers/groups/security/credentials_controller.rb

@@ -7,9 +7,9 @@ class Groups::Security::CredentialsController < Groups::ApplicationController
   include CredentialsInventoryActions
   include Groups::SecurityFeaturesHelper
 
-  helper_method :credentials_inventory_path, :user_detail_path
+  helper_method :credentials_inventory_path, :user_detail_path, :ssh_key_delete_path
 
-  before_action :validate_group_level_credentials_inventory_available!, only: [:index]
+  before_action :validate_group_level_credentials_inventory_available!, only: [:index, :destroy]
 
   feature_category :compliance_management
 
@@ -24,6 +24,11 @@ class Groups::Security::CredentialsController < Groups::ApplicationController
     group_security_credentials_path(args)
   end
 
+  override :ssh_key_delete_path
+  def ssh_key_delete_path(key)
+    group_security_credential_path(@group, key)
+  end
+
   override :user_detail_path
   def user_detail_path(user)
     user_path(user)

ee/app/helpers/credentials_inventory_helper.rb

@@ -25,6 +25,10 @@ module CredentialsInventoryHelper
     raise NotImplementedError, "#{self.class} does not implement #{__method__}"
   end
 
+  def ssh_key_delete_path(key)
+    raise NotImplementedError, "#{self.class} does not implement #{__method__}"
+  end
+
   def user_detail_path(user)
     raise NotImplementedError, "#{self.class} does not implement #{__method__}"
   end

ee/app/views/shared/credentials_inventory/_ssh_keys.html.haml

@@ -4,5 +4,6 @@
     .table-section.section-15{ role: 'rowheader' }= _('Created On')
     .table-section.section-15{ role: 'rowheader' }= _('Last Accessed On')
     .table-section.section-15{ role: 'rowheader' }= _('Expiration')
+    .table-section.section-5{ role: 'rowheader' }
 
   = render partial: 'shared/credentials_inventory/ssh_keys/ssh_key', collection: credentials

ee/app/views/shared/credentials_inventory/ssh_keys/_ssh_key.html.haml

@@ -19,3 +19,7 @@
       = _('Expiration')
     .table-mobile-content.gl-w-full
       = render 'shared/credentials_inventory/expiry_date', credential: ssh_key
+  .table-section.section-5
+    .table-mobile-header{ role: 'rowheader' }
+    .table-mobile-content
+      = button_to _('Delete'), '#', class: "btn btn-danger btn-danger-secondary btn-md btn-secondary gl-button js-confirm-modal-button", data: ssh_key_delete_modal_data(ssh_key, ssh_key_delete_path(ssh_key))

ee/changelogs/unreleased/225248-add-delete-ssh-to-credential-inventory.yml

+---
+title: Added a delete SSH key button to the credentials inventory pages
+merge_request: 41592
+author:
+type: added

ee/config/routes/admin.rb

@@ -19,7 +19,7 @@ namespace :admin do
   resource :email, only: [:show, :create]
   resources :audit_logs, controller: 'audit_logs', only: [:index]
   resources :audit_log_reports, only: [:index], constraints: { format: :csv }
-  resources :credentials, only: [:index] do
+  resources :credentials, only: [:index, :destroy] do
     member do
       put :revoke
     end

ee/config/routes/group.rb

@@ -139,7 +139,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
       resources :vulnerabilities, only: [:index]
       resource :compliance_dashboard, only: [:show]
       resource :discover, only: [:show], controller: :discover
-      resources :credentials, only: [:index]
+      resources :credentials, only: [:index, :destroy]
       resources :merge_commit_reports, only: [:index], constraints: { format: :csv }
     end
 

ee/spec/controllers/admin/credentials_controller_spec.rb

@@ -95,6 +95,12 @@ RSpec.describe Admin::CredentialsController do
     end
   end
 
+  describe 'POST #destroy' do
+    let(:credentials_path) { admin_credentials_path(filter: 'ssh_keys') }
+
+    it_behaves_like 'credentials inventory controller delete SSH key'
+  end
+
   describe 'PUT #revoke' do
     context 'admin user' do
       before do

ee/spec/controllers/groups/security/credentials_controller_spec.rb

@@ -137,4 +137,10 @@ RSpec.describe Groups::Security::CredentialsController do
       end
     end
   end
+
+  describe 'POST #destroy' do
+    let(:credentials_path) { group_security_credentials_path(filter: 'ssh_keys') }
+
+    it_behaves_like 'credentials inventory controller delete SSH key', group_managed_account: true
+  end
 end

ee/spec/support/shared_examples/controllers/credentials_inventory_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples_for 'credentials inventory controller delete SSH key' do |group_managed_account: false|
+  let_it_be(:user) { group_managed_account ? managed_users.last : create(:user, name: 'David') }
+  let_it_be(:ssh_key) { create(:personal_key, user: user) }
+
+  let(:ssh_key_id) { ssh_key.id }
+  let(:params) { group_managed_account ? { group_id: group_with_managed_accounts.to_param, id: ssh_key_id } : { id: ssh_key_id } }
+
+  subject { post :destroy, params: params }
+
+  context 'admin user' do
+    before do
+      unless group_managed_account
+        sign_in(admin)
+      end
+    end
+
+    context 'when `credentials_inventory` feature is enabled' do
+      before do
+        if group_managed_account
+          stub_licensed_features(credentials_inventory: true, group_saml: true)
+        else
+          stub_licensed_features(credentials_inventory: true)
+        end
+      end
+
+      context 'and the ssh_key exists' do
+        context 'and it removes the key' do
+          it 'renders a success message' do
+            subject
+
+            expect(response).to redirect_to(credentials_path)
+            expect(flash[:notice]).to eql 'User key was successfully removed.'
+          end
+        end
+
+        context 'and it fails to remove the key' do
+          before do
+            allow_next_instance_of(Keys::DestroyService) do |service|
+              allow(service).to receive(:execute).and_return(false)
+            end
+          end
+
+          it 'renders a failure message' do
+            subject
+
+            expect(response).to redirect_to(credentials_path)
+            expect(flash[:notice]).to eql 'Failed to remove user key.'
+          end
+        end
+      end
+
+      context 'and the ssh_key does not exist' do
+        let(:ssh_key_id) { 999999999999999999999999999999999 }
+
+        it 'renders a not found message' do
+          subject
+
+          expect(response).to redirect_to(credentials_path)
+          expect(flash[:notice]).to eql 'Cannot find user key.'
+        end
+      end
+    end
+
+    context 'when `credentials_inventory` feature is disabled' do
+      before do
+        stub_licensed_features(credentials_inventory: false)
+      end
+
+      it 'returns 404' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+  end
+
+  context 'non-admin user' do
+    before do
+      sign_in(user)
+    end
+
+    it 'returns 404' do
+      subject
+
+      expect(response).to have_gitlab_http_status(:not_found)
+    end
+  end
+end

ee/spec/support/shared_examples/features/credentials_inventory_shared_examples.rb

@@ -101,6 +101,23 @@ RSpec.shared_examples_for 'credentials inventory SSH keys' do |group_managed_acc
       expect(first_row.text).to include('2019-12-10')
       expect(first_row.text).to include('Never')
     end
+
+    it 'shows the delete button' do
+      expect(first_row).to have_selector('.js-confirm-modal-button[value="Delete"]')
+    end
+
+    context 'and the user clicks the delete button', :js do
+      it 'deletes the key' do
+        click_button('Delete')
+
+        page.within('.modal') do
+          page.click_button('Delete')
+        end
+
+        expect(page).to have_content('User key was successfully removed.')
+        expect(page).to have_content('No credentials found')
+      end
+    end
   end
 
   context 'when a SSH key has an expiry' do

ee/spec/views/shared/credentials_inventory/ssh_keys/_ssh_key.html.haml_spec.rb

@@ -6,8 +6,10 @@ RSpec.describe('shared/credentials_inventory/ssh_keys/_ssh_key.html.haml') do
   let_it_be(:user) { create(:user) }
   let_it_be(:expiry_date) { 20.days.since }
   let_it_be(:ssh_key) { create(:personal_key, user: user, expires_at: expiry_date)}
+  let_it_be(:ssh_key_delete_path) { |key| 'ssh_key_delete_path' }
 
   before do
+    allow(view).to receive(:ssh_key_delete_path).and_return('key_path')
     allow(view).to receive(:user_detail_path).and_return('abcd')
     render 'shared/credentials_inventory/ssh_keys/ssh_key', ssh_key: ssh_key
   end
@@ -24,6 +26,10 @@ RSpec.describe('shared/credentials_inventory/ssh_keys/_ssh_key.html.haml') do
     expect(rendered).to have_text(ssh_key.expires_at.to_date.to_s)
   end
 
+  it 'shows the delete button' do
+    expect(rendered).to have_css('.js-confirm-modal-button[value="Delete"]')
+  end
+
   context 'last accessed date' do
     context 'when set' do
       let_it_be(:last_used_date) { 10.days.ago }

locale/gitlab.pot

@@ -4686,6 +4686,9 @@ msgstr ""
 msgid "Cannot enable shared runners because parent group does not allow it"
 msgstr ""
 
+msgid "Cannot find user key."
+msgstr ""
+
 msgid "Cannot have multiple Jira imports running at the same time"
 msgstr ""
 

qa/qa/page/profile/ssh_keys.rb

@@ -11,7 +11,7 @@ module QA
           element :add_key_button
         end
 
-        view 'app/helpers/profiles_helper.rb' do
+        view 'app/helpers/ssh_keys_helper.rb' do
           element :delete_ssh_key_button
           element :ssh_key_delete_modal
         end