Add revoke button to managed groups credentials inventory

- Updated revoke_button_available? restriction to check for group
feature flag
- Added new routes for managed groups revoke
- Updated the revoke service to support no token being passed
- Updated the inventory actions to properly render 404
- Added tests

app/finders/personal_access_tokens_finder.rb

@@ -14,6 +14,7 @@ class PersonalAccessTokensFinder
     tokens = PersonalAccessToken.all
     tokens = by_current_user(tokens)
     tokens = by_user(tokens)
+    tokens = by_users(tokens)
     tokens = by_impersonation(tokens)
     tokens = by_state(tokens)
 
@@ -37,6 +38,12 @@ class PersonalAccessTokensFinder
     tokens.for_user(@params[:user])
   end
 
+  def by_users(tokens)
+    return tokens unless @params[:users]
+
+    tokens.for_users(@params[:users])
+  end
+
   def sort(tokens)
     available_sort_orders = PersonalAccessToken.simple_sorts.keys
 

app/models/personal_access_token.rb

@@ -26,6 +26,7 @@ class PersonalAccessToken < ApplicationRecord
   scope :revoked, -> { where(revoked: true) }
   scope :not_revoked, -> { where(revoked: [false, nil]) }
   scope :for_user, -> (user) { where(user: user) }
+  scope :for_users, -> (users) { where(user: users) }
   scope :preload_users, -> { preload(:user) }
   scope :order_expires_at_asc, -> { reorder(expires_at: :asc) }
   scope :order_expires_at_desc, -> { reorder(expires_at: :desc) }

ee/app/controllers/admin/credentials_controller.rb

@@ -48,4 +48,9 @@ class Admin::CredentialsController < Admin::ApplicationController
   def users
     nil
   end
+
+  override :revocable
+  def revocable
+    nil
+  end
 end

ee/app/controllers/concerns/credentials_inventory_actions.rb

@@ -30,13 +30,16 @@ module CredentialsInventoryActions
   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
-    if service.success?
-      flash[:notice] = service.message
+    personal_access_token = personal_access_token_finder.find_by_id(params[:id])
+    return render_404 unless personal_access_token
+
+    result = revoke_service(personal_access_token).execute
+
+    if result.success?
+      flash[:notice] = result.message
       notify_deleted_or_revoked_credential(personal_access_token)
     else
-      flash[:alert] = service.message
+      flash[:alert] = result.message
     end
 
     redirect_to credentials_inventory_path(page: params[:page])
@@ -46,7 +49,7 @@ module CredentialsInventoryActions
 
   def filter_credentials
     if show_personal_access_tokens?
-      ::PersonalAccessTokensFinder.new({ user: users, impersonation: false, sort: 'id_desc' }).execute
+      ::PersonalAccessTokensFinder.new({ users: users, impersonation: false, sort: 'id_desc' }).execute
     elsif show_ssh_keys?
       ::KeysFinder.new({ users: users, key_type: 'ssh' }).execute
     end
@@ -67,7 +70,27 @@ module CredentialsInventoryActions
     end
   end
 
+  def personal_access_token_finder
+    if revocable.instance_of?(Group)
+      ::PersonalAccessTokensFinder.new({ impersonation: false, users: users })
+    else
+      ::PersonalAccessTokensFinder.new({ impersonation: false }, current_user)
+    end
+  end
+
+  def revoke_service(token)
+    if revocable.instance_of?(Group)
+      ::PersonalAccessTokens::RevokeService.new(current_user, token: token, group: revocable)
+    else
+      ::PersonalAccessTokens::RevokeService.new(current_user, token: token)
+    end
+  end
+
   def users
     raise NotImplementedError, "#{self.class} does not implement #{__method__}"
   end
+
+  def revocable
+    raise NotImplementedError, "#{self.class} does not implement #{__method__}"
+  end
 end

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, :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, :destroy]
+  before_action :validate_group_level_credentials_inventory_available!, only: [:index, :revoke, :destroy]
 
   feature_category :compliance_management
 
@@ -34,8 +34,23 @@ class Groups::Security::CredentialsController < Groups::ApplicationController
     user_path(user)
   end
 
+  override :personal_access_token_revoke_path
+  def personal_access_token_revoke_path(token)
+    revoke_group_security_credential_path(group, token)
+  end
+
+  override :revoke_button_available?
+  def revoke_button_available?
+    ::Feature.enabled?(:revoke_managed_users_token, group)
+  end
+
   override :users
   def users
     group.managed_users
   end
+
+  override :revocable
+  def revocable
+    group
+  end
 end

ee/app/services/ee/personal_access_tokens/revoke_service.rb

@@ -21,14 +21,17 @@ module EE
       end
 
       def managed_user_revocation_allowed?
+        return unless token.present?
         return unless ::Feature.enabled?(:revoke_managed_users_token, group)
 
-        token.user.group_managed_account? &&
-          token.user.managing_group == group &&
+        token.user&.group_managed_account? &&
+          token.user&.managing_group == group &&
           can?(current_user, :admin_group_credentials_inventory, group)
       end
 
       def log_audit_event(token, response)
+        return unless token.present?
+
         audit_event_service(token, response).for_user(full_path: token.user.username, entity_id: token.user.id).security_event
       end
 

ee/config/routes/group.rb

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

ee/spec/controllers/admin/credentials_controller_spec.rb

@@ -102,6 +102,32 @@ RSpec.describe Admin::CredentialsController do
   end
 
   describe 'PUT #revoke' do
+    shared_examples_for 'responds with 404' do
+      it do
+        put :revoke, params: { id: token_id }
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    shared_examples_for 'displays the flash success message' do
+      it do
+        put :revoke, params: { id: token_id }
+
+        expect(response).to redirect_to(admin_credentials_path)
+        expect(flash[:notice]).to start_with 'Revoked personal access token '
+      end
+    end
+
+    shared_examples_for 'displays the flash error message' do
+      it do
+        put :revoke, params: { id: token_id }
+
+        expect(response).to redirect_to(admin_credentials_path)
+        expect(flash[:alert]).to eql 'Not permitted to revoke'
+      end
+    end
+
     context 'admin user' do
       before do
         sign_in(admin)
@@ -113,11 +139,9 @@ RSpec.describe Admin::CredentialsController do
         end
 
         context 'non-existent personal access token specified' do
-          it 'returns 404' do
-            put :revoke, params: { id: 999999999999999999999999999999999 }
+          let(:token_id) { 999999999999999999999999999999999 }
 
-            expect(response).to have_gitlab_http_status(:not_found)
-          end
+          it_behaves_like 'responds with 404'
         end
 
         describe 'with an existing personal access token' do
@@ -127,43 +151,27 @@ RSpec.describe Admin::CredentialsController do
               allow(Ability).to receive(:allowed?).with(admin, :revoke_token, personal_access_token) { false }
             end
 
-            it 'returns the flash error message' do
-              put :revoke, params: { id: personal_access_token.id }
+            let(:token_id) { personal_access_token.id }
 
-              expect(response).to redirect_to(admin_credentials_path)
-              expect(flash[:alert]).to eql 'Not permitted to revoke'
-            end
+            it_behaves_like 'displays the flash error message'
           end
 
           context 'personal access token is already revoked' do
-            let_it_be(:personal_access_token) { create(:personal_access_token, revoked: true, user: user) }
-
-            it 'returns the flash success message' do
-              put :revoke, params: { id: personal_access_token.id }
+            let_it_be(:token_id) { create(:personal_access_token, revoked: true, user: user).id }
 
-              expect(response).to redirect_to(admin_credentials_path)
-              expect(flash[:notice]).to eql 'Revoked personal access token %{personal_access_token_name}!' % { personal_access_token_name: personal_access_token.name }
-            end
+            it_behaves_like 'displays the flash success message'
           end
 
           context 'personal access token is already expired' do
-            let_it_be(:personal_access_token) { create(:personal_access_token, expires_at: 5.days.ago, user: user) }
+            let_it_be(:token_id) { create(:personal_access_token, expires_at: 5.days.ago, user: user).id }
 
-            it 'returns the flash success message' do
-              put :revoke, params: { id: personal_access_token.id }
-
-              expect(response).to redirect_to(admin_credentials_path)
-              expect(flash[:notice]).to eql 'Revoked personal access token %{personal_access_token_name}!' % { personal_access_token_name: personal_access_token.name }
-            end
+            it_behaves_like 'displays the flash success message'
           end
 
           context 'personal access token is not revoked or expired' do
-            it 'returns the flash success message' do
-              put :revoke, params: { id: personal_access_token.id }
+            let(:token_id) { personal_access_token.id }
 
-              expect(response).to redirect_to(admin_credentials_path)
-              expect(flash[:notice]).to eql 'Revoked personal access token %{personal_access_token_name}!' % { personal_access_token_name: personal_access_token.name }
-            end
+            it_behaves_like 'displays the flash success message'
 
             it 'informs the token owner' do
               expect(CredentialsInventoryMailer).to receive_message_chain(:personal_access_token_revoked_email, :deliver_later)
@@ -179,11 +187,9 @@ RSpec.describe Admin::CredentialsController do
           stub_licensed_features(credentials_inventory: false)
         end
 
-        it 'returns 404' do
-          put :revoke, params: { id: personal_access_token.id }
+        let(:token_id) { personal_access_token.id }
 
-          expect(response).to have_gitlab_http_status(:not_found)
-        end
+        it_behaves_like 'responds with 404'
       end
     end
 
@@ -192,11 +198,9 @@ RSpec.describe Admin::CredentialsController do
         sign_in(user)
       end
 
-      it 'returns 404' do
-        put :revoke, params: { id: personal_access_token.id }
+      let(:token_id) { personal_access_token.id }
 
-        expect(response).to have_gitlab_http_status(:not_found)
-      end
+      it_behaves_like 'responds with 404'
     end
   end
 end

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

@@ -5,21 +5,24 @@ require 'spec_helper'
 RSpec.describe Groups::Security::CredentialsController do
   let_it_be(:group_with_managed_accounts) { create(:group_with_managed_accounts, :private) }
   let_it_be(:managed_users) { create_list(:user, 2, :group_managed, managing_group: group_with_managed_accounts) }
+  let_it_be(:owner) { managed_users.first }
+  let_it_be(:maintainer) { managed_users.last }
+  let_it_be(:group_id) { group_with_managed_accounts.to_param }
+  let_it_be(:personal_access_token) { create(:personal_access_token, user: maintainer) }
 
   before do
     allow_next_instance_of(Gitlab::Auth::GroupSaml::SsoEnforcer) do |sso_enforcer|
       allow(sso_enforcer).to receive(:active_session?).and_return(true)
     end
 
-    owner = managed_users.first
     group_with_managed_accounts.add_owner(owner)
+    group_with_managed_accounts.add_maintainer(maintainer)
 
     sign_in(owner)
   end
 
   describe 'GET #index' do
     let(:filter) {}
-    let(:group_id) { group_with_managed_accounts.to_param }
 
     subject { get :index, params: { group_id: group_id.to_param, filter: filter } }
 
@@ -98,10 +101,6 @@ RSpec.describe Groups::Security::CredentialsController do
 
           context 'for a user without access to view credentials inventory' do
             before do
-              maintainer = managed_users.last
-
-              group_with_managed_accounts.add_maintainer(maintainer)
-
               sign_in(maintainer)
             end
 
@@ -115,7 +114,7 @@ RSpec.describe Groups::Security::CredentialsController do
       end
 
       context 'for a group that does not enforce group managed accounts' do
-        let(:group_id) { create(:group).to_param }
+        let_it_be(:group_id) { create(:group).id }
 
         it 'responds with 404' do
           subject
@@ -143,4 +142,168 @@ RSpec.describe Groups::Security::CredentialsController do
 
     it_behaves_like 'credentials inventory controller 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 }
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    shared_examples_for 'displays the flash success message' do
+      it do
+        put :revoke, params: { 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 '
+      end
+    end
+
+    shared_examples_for 'displays the flash error message' do
+      it do
+        put :revoke, params: { 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'
+      end
+    end
+
+    context 'when `credentials_inventory` feature is enabled' do
+      before do
+        stub_licensed_features(credentials_inventory: true, group_saml: true)
+        stub_feature_flags(revoke_managed_users_token: true)
+      end
+
+      context 'for a group that enforces group managed accounts' do
+        context 'when `revoke_managed_users_token` feature is enabled' do
+          before_all do
+            stub_feature_flags(revoke_managed_users_token: true)
+          end
+
+          context 'for a user with access to view credentials inventory' do
+            context 'non-existent personal access token specified' do
+              let(:token_id) { 999999999999999999999999999999999 }
+
+              it_behaves_like 'responds with 404'
+            end
+
+            describe 'with an existing personal access token' do
+              context 'personal access token is already revoked' do
+                let_it_be(:token_id) { create(:personal_access_token, revoked: true, user: maintainer).id }
+
+                it_behaves_like 'displays the flash success message'
+              end
+
+              context 'personal access token is already expired' do
+                let_it_be(:token_id) { create(:personal_access_token, expires_at: 5.days.ago, user: maintainer).id }
+
+                it_behaves_like 'displays the flash success message'
+              end
+
+              context 'does not have permissions to revoke the credential' do
+                let_it_be(:token_id) { create(:personal_access_token, user: create(:user)).id }
+
+                it_behaves_like 'responds with 404'
+              end
+
+              context 'personal access token is already revoked' do
+                let_it_be(:token_id) { create(:personal_access_token, revoked: true, user: maintainer).id }
+
+                it_behaves_like 'displays the flash success message'
+              end
+
+              context 'personal access token is already expired' do
+                let_it_be(:token_id) { create(:personal_access_token, expires_at: 5.days.ago, user: maintainer).id }
+
+                it_behaves_like 'displays the flash success message'
+              end
+
+              context 'personal access token is not revoked or expired' do
+                let_it_be(:token_id) { personal_access_token.id }
+
+                it_behaves_like 'displays the flash success message'
+
+                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 }
+                end
+
+                context 'when credentials_inventory_revocation_emails flag is disabled' do
+                  before do
+                    stub_feature_flags(credentials_inventory_revocation_emails: false)
+                  end
+
+                  it 'does not inform the token owner' do
+                    expect do
+                      put :revoke, params: { group_id: group_id.to_param, id: personal_access_token.id }
+                    end.not_to change { ActionMailer::Base.deliveries.size }
+                  end
+                end
+              end
+            end
+          end
+
+          context 'for a user without access to view credentials inventory' do
+            let_it_be(:token_id) { create(:personal_access_token, user: owner).id }
+
+            before do
+              sign_in(maintainer)
+            end
+
+            it_behaves_like 'responds with 404'
+          end
+        end
+
+        context 'when `revoke_managed_users_token` feature is disabled' do
+          before_all do
+            stub_feature_flags(revoke_managed_users_token: false)
+          end
+
+          context 'for a user with access to view credentials inventory' do
+            context 'non-existent personal access token specified' do
+              let(:token_id) { 999999999999999999999999999999999 }
+
+              it_behaves_like 'responds with 404'
+            end
+
+            context 'valid personal access token specified' do
+              let_it_be(:token_id) { create(:personal_access_token, user: create(:user)).id }
+
+              it_behaves_like 'responds with 404'
+            end
+          end
+
+          context 'for a user without access to view credentials inventory' do
+            let_it_be(:token_id) { create(:personal_access_token, user: owner).id }
+
+            before do
+              sign_in(maintainer)
+            end
+
+            it_behaves_like 'responds with 404'
+          end
+        end
+      end
+
+      context 'for a group that does not enforce group managed accounts' do
+        let_it_be(:token_id) { personal_access_token.id }
+        let_it_be(:group_id) { create(:group).id }
+
+        it_behaves_like 'responds with 404'
+      end
+    end
+
+    context 'when `credentials_inventory` feature is disabled' do
+      let_it_be(:token_id) { create(:personal_access_token, user: owner).id }
+
+      before do
+        stub_licensed_features(credentials_inventory: false)
+      end
+
+      it_behaves_like 'responds with 404'
+    end
+  end
 end

ee/spec/features/groups/groups_security_credentials_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe 'Groups::Security::Credentials' do
   include Spec::Support::Helpers::Features::ResponsiveTableHelpers
 
   let_it_be(:group_with_managed_accounts) { create(:group_with_managed_accounts, :private) }
-  let_it_be(:managed_user) { create(:user, :group_managed, managing_group: group_with_managed_accounts, name: 'David') }
+  let_it_be(:managed_user) { create(:user, :group_managed, managing_group: group_with_managed_accounts, name: 'abc') }
   let(:group_id) { group_with_managed_accounts.to_param }
 
   before do
@@ -44,13 +44,13 @@ RSpec.describe 'Groups::Security::Credentials' do
       context 'by Personal Access Tokens' do
         let(:credentials_path) { group_security_credentials_path(group_id: group_id, filter: 'personal_access_tokens') }
 
-        it_behaves_like 'credentials inventory personal access tokens', group_managed_account: true
+        it_behaves_like 'credentials inventory personal access tokens'
       end
 
       context 'by SSH Keys' do
         let(:credentials_path) { group_security_credentials_path(group_id: group_id, filter: 'ssh_keys') }
 
-        it_behaves_like 'credentials inventory SSH keys', group_managed_account: true
+        it_behaves_like 'credentials inventory SSH keys'
       end
     end
   end

ee/spec/services/ee/personal_access_tokens/revoke_service_spec.rb

@@ -34,6 +34,12 @@ RSpec.describe EE::PersonalAccessTokens::RevokeService do
         let_it_be(:token) { create(:personal_access_token, user: managed_user) }
 
         it_behaves_like 'a successfully revoked token'
+
+        context 'and an empty token is given' do
+          let_it_be(:token) { nil }
+
+          it { expect(subject.success?).to be false }
+        end
       end
 
       context 'when feature flag is disabled' do

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(: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 }

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

@@ -2,8 +2,8 @@
 
 require 'spec_helper'
 
-RSpec.shared_examples_for 'credentials inventory personal access tokens' do |group_managed_account: false|
-  let_it_be(:user) { group_managed_account ? managed_user : create(:user, name: 'David') }
+RSpec.shared_examples_for 'credentials inventory personal access tokens' do
+  let_it_be(:user) { defined?(managed_user) ? managed_user : create(:user, name: 'abc') }
 
   context 'when a personal access token is active' do
     before_all do
@@ -19,15 +19,12 @@ RSpec.shared_examples_for 'credentials inventory personal access tokens' do |gro
     end
 
     it 'shows the details', :aggregate_failures do
-      expect(first_row.text).to include('David')
+      expect(first_row.text).to include('abc')
       expect(first_row.text).to include('api')
       expect(first_row.text).to include('2019-12-10')
       expect(first_row.text).to include('Never')
       expect(first_row.text).not_to include('2020-06-22')
-
-      unless group_managed_account
-        expect(first_row).to have_selector('a.btn', text: 'Revoke')
-      end
+      expect(first_row).to have_selector('a.btn', text: 'Revoke')
     end
   end
 
@@ -71,16 +68,13 @@ RSpec.shared_examples_for 'credentials inventory personal access tokens' do |gro
 
     it 'shows the details with a revoked date', :aggregate_failures do
       expect(first_row.text).to include('2020-06-22')
-
-      unless group_managed_account
-        expect(first_row).not_to have_selector('a.btn', text: 'Revoke')
-      end
+      expect(first_row).not_to have_selector('a.btn', text: 'Revoke')
     end
   end
 end
 
-RSpec.shared_examples_for 'credentials inventory SSH keys' do |group_managed_account: false|
-  let_it_be(:user) { group_managed_account ? managed_user : create(:user, name: 'David') }
+RSpec.shared_examples_for 'credentials inventory SSH keys' do
+  let_it_be(:user) { defined?(managed_user) ? managed_user : create(:user, name: 'abc') }
 
   context 'when a SSH key is active' do
     before_all do
@@ -96,7 +90,7 @@ RSpec.shared_examples_for 'credentials inventory SSH keys' do |group_managed_acc
     end
 
     it 'shows the details', :aggregate_failures do
-      expect(first_row.text).to include('David')
+      expect(first_row.text).to include('abc')
       expect(first_row.text).to include('2019-12-09')
       expect(first_row.text).to include('2019-12-10')
       expect(first_row.text).to include('Never')

spec/finders/personal_access_tokens_finder_spec.rb

@@ -62,6 +62,27 @@ RSpec.describe PersonalAccessTokensFinder do
           revoked_impersonation_token, expired_impersonation_token)
       end
 
+      describe 'with users' do
+        let(:user2) { create(:user) }
+
+        before do
+          create(:personal_access_token, user: user2)
+          create(:personal_access_token, :expired, user: user2)
+          create(:personal_access_token, :revoked, user: user2)
+          create(:personal_access_token, :impersonation, user: user2)
+          create(:personal_access_token, :expired, :impersonation, user: user2)
+          create(:personal_access_token, :revoked, :impersonation, user: user2)
+
+          params[:users] = [user]
+        end
+
+        it {
+          is_expected.to contain_exactly(active_personal_access_token, active_impersonation_token,
+                                         revoked_personal_access_token, expired_personal_access_token,
+                                         revoked_impersonation_token, expired_impersonation_token)
+        }
+      end
+
       describe 'with sort order' do
         before do
           params[:sort] = 'id_asc'

spec/models/personal_access_token_spec.rb

@@ -31,6 +31,18 @@ RSpec.describe PersonalAccessToken do
         expect(described_class.for_user(user_1)).to contain_exactly(token_of_user_1)
       end
     end
+
+    describe '.for_users' do
+      it 'returns personal access tokens for the specified users only' do
+        user_1 = create(:user)
+        user_2 = create(:user)
+        token_of_user_1 = create(:personal_access_token, user: user_1)
+        token_of_user_2 = create(:personal_access_token, user: user_2)
+        create_list(:personal_access_token, 3)
+
+        expect(described_class.for_users([user_1, user_2])).to contain_exactly(token_of_user_1, token_of_user_2)
+      end
+    end
   end
 
   describe ".active?" do