Merge branch '282429-add-gpg-keys-list-view-to-admin-credentials-inventory' into 'master'

Add GPG keys list view to Admin Credentials Inventory

See merge request gitlab-org/gitlab!54429

ee/app/controllers/admin/credentials_controller.rb

@@ -5,7 +5,8 @@ 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?, :ssh_key_delete_path
+  helper_method :credentials_inventory_path, :user_detail_path, :personal_access_token_revoke_path,
+                :revoke_button_available?, :ssh_key_delete_path, :gpg_keys_available?
 
   before_action :check_license_credentials_inventory_available!, only: [:index, :revoke, :destroy]
   before_action :check_gpg_keys_list_enabled!, only: [:index]
@@ -54,6 +55,11 @@ class Admin::CredentialsController < Admin::ApplicationController
     true
   end
 
+  override :gpg_keys_available?
+  def gpg_keys_available?
+    Feature.enabled?(:credential_inventory_gpg_keys)
+  end
+
   override :users
   def users
     nil

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

@@ -7,9 +7,11 @@ class Groups::Security::CredentialsController < Groups::ApplicationController
   include CredentialsInventoryActions
   include Groups::SecurityFeaturesHelper
 
-  helper_method :credentials_inventory_path, :user_detail_path, :personal_access_token_revoke_path, :revoke_button_available?, :ssh_key_delete_path
+  helper_method :credentials_inventory_path, :user_detail_path, :personal_access_token_revoke_path,
+                :revoke_button_available?, :ssh_key_delete_path
 
   before_action :validate_group_level_credentials_inventory_available!, only: [:index, :revoke, :destroy]
+  before_action :check_gpg_keys_list_enabled!, only: [:index]
 
   feature_category :compliance_management
 
@@ -19,6 +21,10 @@ class Groups::Security::CredentialsController < Groups::ApplicationController
     render_404 unless group_level_credentials_inventory_available?(group)
   end
 
+  def check_gpg_keys_list_enabled!
+    render_404 if show_gpg_keys?
+  end
+
   override :credentials_inventory_path
   def credentials_inventory_path(args)
     group_security_credentials_path(args)

ee/app/helpers/credentials_inventory_helper.rb

@@ -25,6 +25,10 @@ module CredentialsInventoryHelper
     false
   end
 
+  def gpg_keys_available?
+    false
+  end
+
   def credentials_inventory_path(args)
     raise NotImplementedError, "#{self.class} does not implement #{__method__}"
   end

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

+.table-holder
+  .thead-white.gl-white-space-nowrap.gl-responsive-table-row.table-row-header{ role: 'row' }
+    .table-section.section-40{ role: 'rowheader' }= _('Owner')
+    .table-section.section-20{ role: 'rowheader' }= _('ID')
+    .table-section.section-15{ role: 'rowheader' }= _('Status')
+
+  = render partial: 'shared/credentials_inventory/gpg_keys/gpg_key', collection: credentials

ee/app/views/shared/credentials_inventory/gpg_keys/_gpg_key.html.haml

+- verified = gpg_key.verified?
+- verified_css_classes = %w(badge gl-badge)
+- verified_css_classes << (verified ? 'badge-success': 'badge-danger')
+- verified_text = verified ? _('Verified') : _('Unverified')
+
+.gl-responsive-table-row{ role: 'row', data: { qa_selector: 'credentials_gpg_key_row_content' } }
+  .table-section.section-40
+    .table-mobile-header{ role: 'rowheader' }
+      = _('Owner')
+    .table-mobile-content
+      = render 'shared/credentials_inventory/users/user_detail', user: gpg_key.user
+  .table-section.section-20
+    .table-mobile-header{ role: 'rowheader' }
+      = _('ID')
+    .table-mobile-content
+      %code
+        = gpg_key.primary_keyid
+  .table-section.section-15
+    .table-mobile-header{ role: 'rowheader' }
+      = _('Status')
+    .table-mobile-content
+      %div{ class: verified_css_classes }
+        = verified_text

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

@@ -12,6 +12,10 @@
     = nav_link(html_options: { class: active_when(show_ssh_keys?) }) do
       = link_to credentials_inventory_path(filter: 'ssh_keys') do
         = s_('CredentialsInventory|SSH Keys')
+    - if gpg_keys_available?
+      = nav_link(html_options: { class: active_when(show_gpg_keys?) }) do
+        = link_to credentials_inventory_path(filter: 'gpg_keys') do
+          = s_('CredentialsInventory|GPG Keys')
 
 - if @credentials.empty?
   .nothing-here-block.border-top-0
@@ -21,5 +25,7 @@
     = render 'shared/credentials_inventory/personal_access_tokens', credentials: @credentials
   - elsif show_ssh_keys?
     = render 'shared/credentials_inventory/ssh_keys', credentials: @credentials
+  - elsif show_gpg_keys?
+    = render 'shared/credentials_inventory/gpg_keys', credentials: @credentials
 
 = paginate_without_count @credentials

ee/spec/features/admin/admin_credentials_inventory_spec.rb

@@ -38,9 +38,28 @@ RSpec.describe 'Admin::CredentialsInventory' do
       end
 
       context 'tabs' do
-        it 'contains the relevant filter tabs' do
-          expect(page).to have_link('Personal Access Tokens', href: admin_credentials_path(filter: 'personal_access_tokens'))
-          expect(page).to have_link('SSH Keys', href: admin_credentials_path(filter: 'ssh_keys'))
+        context 'when GPG keys feature is enabled' do
+          before_all do
+            stub_feature_flags(credential_inventory_gpg_keys: true)
+          end
+
+          it 'contains the relevant filter tabs' do
+            expect(page).to have_link('Personal Access Tokens', href: admin_credentials_path(filter: 'personal_access_tokens'))
+            expect(page).to have_link('SSH Keys', href: admin_credentials_path(filter: 'ssh_keys'))
+            expect(page).to have_link('GPG Keys', href: admin_credentials_path(filter: 'gpg_keys'))
+          end
+        end
+
+        context 'when GPG keys feature is disabled' do
+          before_all do
+            stub_feature_flags(credential_inventory_gpg_keys: false)
+          end
+
+          it 'contains the relevant filter tabs' do
+            expect(page).to have_link('Personal Access Tokens', href: admin_credentials_path(filter: 'personal_access_tokens'))
+            expect(page).to have_link('SSH Keys', href: admin_credentials_path(filter: 'ssh_keys'))
+            expect(page).not_to have_link('GPG Keys', href: admin_credentials_path(filter: 'gpg_keys'))
+          end
         end
       end
     end
@@ -57,6 +76,46 @@ RSpec.describe 'Admin::CredentialsInventory' do
 
         it_behaves_like 'credentials inventory SSH keys'
       end
+
+      context 'by GPG Keys' do
+        let(:credentials_path) { admin_credentials_path(filter: 'gpg_keys') }
+
+        context 'when a GPG key is verified' do
+          let_it_be(:user) { create(:user, name: 'User Name', email: GpgHelpers::User1.emails.first) }
+
+          before_all do
+            create(:gpg_key, user: user, key: GpgHelpers::User1.public_key)
+          end
+
+          before do
+            visit credentials_path
+          end
+
+          it 'shows the details', :aggregate_failures do
+            expect(first_row.text).to include('User Name')
+            expect(first_row.text).to include(GpgHelpers::User1.primary_keyid)
+            expect(first_row.text).to include('Verified')
+          end
+        end
+
+        context 'when a GPG key is unverified' do
+          let_it_be(:user) { create(:user, name: 'User Name', email: 'random@example.com') }
+
+          before_all do
+            create(:another_gpg_key, user: user)
+          end
+
+          before do
+            visit credentials_path
+          end
+
+          it 'shows the details', :aggregate_failures do
+            expect(first_row.text).to include('User Name')
+            expect(first_row.text).to include(GpgHelpers::User1.primary_keyid2)
+            expect(first_row.text).to include('Unverified')
+          end
+        end
+      end
     end
   end
 end

ee/spec/features/groups/groups_security_credentials_spec.rb

@@ -36,6 +36,7 @@ RSpec.describe 'Groups::Security::Credentials' do
         it 'contains the relevant filter tabs' do
           expect(page).to have_link('Personal Access Tokens', href: group_security_credentials_path(group_id: group_id, filter: 'personal_access_tokens'))
           expect(page).to have_link('SSH Keys', href: group_security_credentials_path(group_id: group_id, filter: 'ssh_keys'))
+          expect(page).not_to have_link('GPG Keys', href: group_security_credentials_path(group_id: group_id, filter: 'gpg_keys'))
         end
       end
     end
@@ -52,6 +53,16 @@ RSpec.describe 'Groups::Security::Credentials' do
 
         it_behaves_like 'credentials inventory SSH keys'
       end
+
+      context 'by GPG Keys' do
+        before do
+          visit group_security_credentials_path(group_id: group_id, filter: 'gpg_keys')
+        end
+
+        it 'returns a 404 not found response' do
+          expect(page.status_code).to eq(404)
+        end
+      end
     end
   end
 

ee/spec/models/ee/gpg_key_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe GpgKey do
   let_it_be(:gpg_key) { create(:gpg_key) }
   let_it_be(:gpg_key_2) { create(:another_gpg_key) }
-  let(:user) { gpg_key.user }
+  let_it_be(:user) { gpg_key.user }
 
   describe '.for_user' do
     subject { GpgKey.for_user(user) }

ee/spec/controllers/admin/credentials_controller_spec.rb → ee/spec/requests/admin/credentials_controller_spec.rb

@@ -2,7 +2,9 @@
 
 require 'spec_helper'
 
-RSpec.describe Admin::CredentialsController do
+RSpec.describe Admin::CredentialsController, type: :request do
+  include AdminModeHelper
+
   let_it_be(:admin) { create(:admin) }
   let_it_be(:user) { create(:user) }
   let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
@@ -10,7 +12,8 @@ RSpec.describe Admin::CredentialsController do
   describe 'GET #index' do
     context 'admin user' do
       before do
-        sign_in(admin)
+        login_as(admin)
+        enable_admin_mode!(admin)
       end
 
       context 'when `credentials_inventory` feature is enabled' do
@@ -19,49 +22,45 @@ RSpec.describe Admin::CredentialsController do
         end
 
         it 'responds with 200' do
-          get :index
+          get admin_credentials_path
 
           expect(response).to have_gitlab_http_status(:ok)
         end
 
-        it_behaves_like 'tracking unique visits', :index do
-          let(:target_id) { 'i_compliance_credential_inventory' }
-        end
-
         describe 'filtering by type of credential' do
           let_it_be(:personal_access_tokens) { create_list(:personal_access_token, 2, user: user) }
 
           shared_examples_for 'filtering by `personal_access_tokens`' do
             specify do
-              get :index, params: params
+              get admin_credentials_path(filter: filter)
 
               expect(assigns(:credentials)).to match_array(user.personal_access_tokens)
             end
           end
 
           context 'no credential type specified' do
-            let(:params) { {} }
+            let(:filter) { nil }
 
             it_behaves_like 'filtering by `personal_access_tokens`'
           end
 
           context 'non-existent credential type specified' do
-            let(:params) { { filter: 'non_existent_credential_type' } }
+            let(:filter) { 'non_existent_credential_type' }
 
             it_behaves_like 'filtering by `personal_access_tokens`'
           end
 
           context 'credential type specified as `personal_access_tokens`' do
-            let(:params) { { filter: 'personal_access_tokens' } }
+            let(:filter) { 'personal_access_tokens' }
 
             it_behaves_like 'filtering by `personal_access_tokens`'
           end
 
           context 'credential type specified as `ssh_keys`' do
             it 'filters by ssh keys' do
-              ssh_keys =  create_list(:personal_key, 2, user: user)
+              ssh_keys = create_list(:personal_key, 2, user: user)
 
-              get :index, params: { filter: 'ssh_keys' }
+              get admin_credentials_path(filter: 'ssh_keys')
 
               expect(assigns(:credentials)).to match_array(ssh_keys)
             end
@@ -71,7 +70,7 @@ RSpec.describe Admin::CredentialsController do
             it 'filters by gpg keys' do
               gpg_key = create(:gpg_key)
 
-              get :index, params: { filter: 'gpg_keys' }
+              get admin_credentials_path(filter: 'gpg_keys')
 
               expect(assigns(:credentials)).to match_array([gpg_key])
             end
@@ -82,11 +81,32 @@ RSpec.describe Admin::CredentialsController do
               end
 
               it 'responds with not found' do
-                get :index, params: { filter: 'gpg_keys' }
+                get admin_credentials_path(filter: 'gpg_keys')
 
                 expect(response).to have_gitlab_http_status(:not_found)
               end
             end
+
+            # There is currently N+1 issue when checking verified_emails per user:
+            # https://gitlab.com/gitlab-org/gitlab/-/issues/322773
+            # Therefore we are currently adding + 1 to the control until this is resolved
+            it 'avoids N+1 queries' do
+              control_user = create(:user, name: 'User Name', email: GpgHelpers::User1.emails.first)
+              new_user = create(:user, name: 'User Name', email: GpgHelpers::User2.emails.first)
+
+              create(:gpg_key, user: control_user, key: GpgHelpers::User1.public_key)
+              create(:gpg_key, user: control_user, key: GpgHelpers::User1.public_key2)
+
+              control = ActiveRecord::QueryRecorder.new(skip_cached: false) do
+                get admin_credentials_path(filter: 'gpg_keys')
+              end.count
+
+              create(:gpg_key, user: new_user, key: GpgHelpers::User2.public_key)
+
+              expect do
+                get admin_credentials_path(filter: 'gpg_keys')
+              end.not_to exceed_query_limit(control + 1)
+            end
           end
         end
       end
@@ -97,7 +117,7 @@ RSpec.describe Admin::CredentialsController do
         end
 
         it 'returns 404' do
-          get :index
+          get admin_credentials_path
 
           expect(response).to have_gitlab_http_status(:not_found)
         end
@@ -106,27 +126,27 @@ RSpec.describe Admin::CredentialsController do
 
     context 'non-admin user' do
       before do
-        sign_in(user)
+        login_as(user)
       end
 
       it 'returns 404' do
-        get :index
+        get admin_credentials_path
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
   end
 
-  describe 'POST #destroy' do
+  describe 'DELETE #destroy' do
     let(:credentials_path) { admin_credentials_path(filter: 'ssh_keys') }
 
-    it_behaves_like 'credentials inventory controller delete SSH key'
+    it_behaves_like 'credentials inventory delete SSH key'
   end
 
   describe 'PUT #revoke' do
     shared_examples_for 'responds with 404' do
       it do
-        put :revoke, params: { id: token_id }
+        put revoke_admin_credential_path(id: token_id)
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
@@ -134,7 +154,7 @@ RSpec.describe Admin::CredentialsController do
 
     shared_examples_for 'displays the flash success message' do
       it do
-        put :revoke, params: { id: token_id }
+        put revoke_admin_credential_path(id: token_id)
 
         expect(response).to redirect_to(admin_credentials_path)
         expect(flash[:notice]).to start_with 'Revoked personal access token '
@@ -143,7 +163,7 @@ RSpec.describe Admin::CredentialsController do
 
     shared_examples_for 'displays the flash error message' do
       it do
-        put :revoke, params: { id: token_id }
+        put revoke_admin_credential_path(id: token_id)
 
         expect(response).to redirect_to(admin_credentials_path)
         expect(flash[:alert]).to eql 'Not permitted to revoke'
@@ -152,7 +172,8 @@ RSpec.describe Admin::CredentialsController do
 
     context 'admin user' do
       before do
-        sign_in(admin)
+        login_as(admin)
+        enable_admin_mode!(admin)
       end
 
       context 'when `credentials_inventory` feature is enabled' do
@@ -198,7 +219,7 @@ RSpec.describe Admin::CredentialsController do
             it 'informs the token owner' do
               expect(CredentialsInventoryMailer).to receive_message_chain(:personal_access_token_revoked_email, :deliver_later)
 
-              put :revoke, params: { id: personal_access_token.id }
+              put revoke_admin_credential_path(id: personal_access_token.id)
             end
           end
         end
@@ -217,7 +238,7 @@ RSpec.describe Admin::CredentialsController do
 
     context 'non-admin user' do
       before do
-        sign_in(user)
+        login_as(user)
       end
 
       let(:token_id) { personal_access_token.id }
@@ -226,3 +247,19 @@ RSpec.describe Admin::CredentialsController do
     end
   end
 end
+
+RSpec.describe Admin::CredentialsController, type: :controller do
+  include AdminModeHelper
+
+  let_it_be(:admin) { create(:admin) }
+
+  before do
+    stub_licensed_features(credentials_inventory: true)
+    sign_in(admin)
+    enable_admin_mode!(admin)
+  end
+
+  it_behaves_like 'tracking unique visits', :index do
+    let(:target_id) { 'i_compliance_credential_inventory' }
+  end
+end

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

@@ -18,13 +18,13 @@ RSpec.describe Groups::Security::CredentialsController do
     group_with_managed_accounts.add_owner(owner)
     group_with_managed_accounts.add_maintainer(maintainer)
 
-    sign_in(owner)
+    login_as(owner)
   end
 
   describe 'GET #index' do
     let(:filter) {}
 
-    subject { get :index, params: { group_id: group_id.to_param, filter: filter } }
+    subject { get group_security_credentials_path(group_id: group_id.to_param, filter: filter) }
 
     context 'when `credentials_inventory` feature is enabled' do
       before do
@@ -137,16 +137,16 @@ RSpec.describe Groups::Security::CredentialsController do
     end
   end
 
-  describe 'POST #destroy' do
+  describe 'DELETE #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
+    it_behaves_like 'credentials inventory delete SSH key', group_managed_account: true
   end
 
   describe 'PUT #revoke' do
     shared_examples_for 'responds with 404' do
       it do
-        put :revoke, params: { group_id: group_id.to_param, id: token_id }
+        put revoke_group_security_credential_path(group_id: group_id.to_param, id: token_id)
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
@@ -154,7 +154,7 @@ RSpec.describe Groups::Security::CredentialsController do
 
     shared_examples_for 'displays the flash success message' do
       it do
-        put :revoke, params: { group_id: group_id.to_param, id: token_id }
+        put revoke_group_security_credential_path(group_id: group_id.to_param, id: token_id)
 
         expect(response).to redirect_to(group_security_credentials_path)
         expect(flash[:notice]).to start_with 'Revoked personal access token '
@@ -163,7 +163,7 @@ RSpec.describe Groups::Security::CredentialsController do
 
     shared_examples_for 'displays the flash error message' do
       it do
-        put :revoke, params: { group_id: group_id.to_param, id: token_id }
+        put revoke_group_security_credential_path(group_id: group_id.to_param, id: token_id)
 
         expect(response).to redirect_to(group_security_credentials_path)
         expect(flash[:alert]).to eql 'Not permitted to revoke'
@@ -228,7 +228,7 @@ RSpec.describe Groups::Security::CredentialsController do
                 it 'informs the token owner' do
                   expect(CredentialsInventoryMailer).to receive_message_chain(:personal_access_token_revoked_email, :deliver_later)
 
-                  put :revoke, params: { group_id: group_id.to_param, id: personal_access_token.id }
+                  put revoke_group_security_credential_path(group_id: group_id.to_param, id: personal_access_token.id)
                 end
 
                 context 'when credentials_inventory_revocation_emails flag is disabled' do
@@ -238,7 +238,7 @@ RSpec.describe Groups::Security::CredentialsController do
 
                   it 'does not inform the token owner' do
                     expect do
-                      put :revoke, params: { group_id: group_id.to_param, id: personal_access_token.id }
+                      put revoke_group_security_credential_path(group_id: group_id.to_param, id: personal_access_token.id)
                     end.not_to change { ActionMailer::Base.deliveries.size }
                   end
                 end
@@ -292,7 +292,11 @@ RSpec.describe Groups::Security::CredentialsController do
         let_it_be(:token_id) { personal_access_token.id }
         let_it_be(:group_id) { create(:group).id }
 
-        it_behaves_like 'responds with 404'
+        it 'responds with 404' do
+          expect do
+            put revoke_group_security_credential_path(group_id: group_id.to_param, id: token_id)
+          end.to raise_error(ActionController::RoutingError)
+        end
       end
     end
 

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

 # frozen_string_literal: true
 
-RSpec.shared_examples_for 'credentials inventory controller delete SSH key' do |group_managed_account: false|
+RSpec.shared_examples_for 'credentials inventory delete SSH key' do |group_managed_account: false|
+  include AdminModeHelper
+
   let_it_be(:user) { group_managed_account ? managed_users.last : create(:user, name: 'abc') }
   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 }
+  if group_managed_account
+    subject { delete group_security_credential_path(group_id: group_with_managed_accounts.to_param, id: ssh_key_id) }
+  else
+    subject { delete admin_credential_path(id: ssh_key_id) }
+  end
 
   context 'admin user' do
     before do
       unless group_managed_account
         sign_in(admin)
+        enable_admin_mode!(admin)
       end
     end
 

ee/spec/views/shared/credentials_inventory/gpg_keys/_gpg_key.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe('shared/credentials_inventory/gpg_keys/_gpg_key.html.haml') do
+  let_it_be(:user) { create(:user, email: GpgHelpers::User1.emails.first) }
+  let_it_be(:gpg_key) { create(:gpg_key, user: user, key: GpgHelpers::User1.public_key) }
+
+  before do
+    allow(view).to receive(:user_detail_path).and_return('abcd')
+    render 'shared/credentials_inventory/gpg_keys/gpg_key', gpg_key: gpg_key
+  end
+
+  it 'shows the users name' do
+    expect(rendered).to have_text(user.name)
+  end
+
+  it 'shows the ID' do
+    expect(rendered).to have_text(gpg_key.primary_keyid)
+  end
+
+  context 'shows the status' do
+    it 'when the key is verified it shows the verified badge', :aggregate_failures do
+      expect(rendered).to have_css('.badge-success')
+      expect(rendered).to have_text('Verified')
+    end
+
+    context 'when the key is not verified' do
+      let_it_be(:user) { create(:user, email: 'random@example.com') }
+      let_it_be(:gpg_key) { create(:another_gpg_key, user: user) }
+
+      it 'shows the unverified badge', :aggregate_failures do
+        expect(rendered).to have_css('.badge-danger')
+        expect(rendered).to have_text('Unverified')
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -8884,6 +8884,9 @@ msgstr ""
 msgid "Credentials"
 msgstr ""
 
+msgid "CredentialsInventory|GPG Keys"
+msgstr ""
+
 msgid "CredentialsInventory|No credentials found"
 msgstr ""
 

spec/support/helpers/gpg_helpers.rb

@@ -279,6 +279,10 @@ module GpgHelpers
       KEY
     end
 
+    def primary_keyid2
+      fingerprint2[-16..-1]
+    end
+
     def fingerprint2
       'C447A6F6BFD9CEF8FB371785571625A930241179'
     end