Merge branch 'rollout-pat-revoke-gma' into 'master'

PAT revoke for managed account FF cleanup [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!56427

changelogs/unreleased/rollout-pat-revoke-gma.yml

+---
+title: Personal access token revoke for managed accounts (feature flag removed)
+merge_request: 56427
+author:
+type: added

doc/user/group/saml_sso/group_managed_accounts.md

@@ -84,6 +84,16 @@ To access the Credentials inventory of a group, navigate to **{shield}** **Secur
 
 This feature is similar to the [Credentials inventory for self-managed instances](../../admin_area/credentials_inventory.md).
 
+### Revoke a group-managed account's personal access token
+
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/214811) in GitLab 13.5.
+> - [Feature flag removed](https://gitlab.com/gitlab-org/gitlab/-/issues/267184) in GitLab 13.10.
+
+Group owners can revoke the personal access tokens of accounts in their group. To do so, select
+the Personal Access Tokens tab, and select Revoke.
+
+When a personal access token is revoked, the group-managed account user is notified by email.
+
 ## Limiting lifetime of personal access tokens of users in Group-managed accounts **(ULTIMATE)**
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/118893) in GitLab 12.10.

ee/app/controllers/admin/credentials_controller.rb

@@ -6,7 +6,7 @@ class Admin::CredentialsController < Admin::ApplicationController
   include Analytics::UniqueVisitsHelper
 
   helper_method :credentials_inventory_path, :user_detail_path, :personal_access_token_revoke_path,
-                :revoke_button_available?, :ssh_key_delete_path, :gpg_keys_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]
@@ -50,11 +50,6 @@ class Admin::CredentialsController < Admin::ApplicationController
     revoke_admin_credential_path(token)
   end
 
-  override :revoke_button_available?
-  def revoke_button_available?
-    true
-  end
-
   override :gpg_keys_available?
   def gpg_keys_available?
     Feature.enabled?(:credential_inventory_gpg_keys, default_enabled: :yaml)

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

@@ -8,7 +8,7 @@ class Groups::Security::CredentialsController < Groups::ApplicationController
   include Groups::SecurityFeaturesHelper
 
   helper_method :credentials_inventory_path, :user_detail_path, :personal_access_token_revoke_path,
-                :revoke_button_available?, :ssh_key_delete_path
+                :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]
@@ -45,11 +45,6 @@ class Groups::Security::CredentialsController < Groups::ApplicationController
     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

ee/app/helpers/credentials_inventory_helper.rb

@@ -21,10 +21,6 @@ module CredentialsInventoryHelper
     License.feature_available?(:credentials_inventory)
   end
 
-  def revoke_button_available?
-    false
-  end
-
   def gpg_keys_available?
     false
   end

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

@@ -22,7 +22,6 @@ module EE
 
       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 &&

ee/app/views/shared/credentials_inventory/personal_access_tokens/_personal_access_token.html.haml

@@ -27,5 +27,5 @@
       - if personal_access_token.revoked?
         -# We're inferring the revoked date from the last updated_at, see https://gitlab.com/gitlab-org/gitlab/-/issues/218046#note_362875952
         = personal_access_token.updated_at.to_date
-      - elsif revoke_button_available? && personal_access_token.active?
+      - elsif personal_access_token.active?
         = link_to _('Revoke'), personal_access_token_revoke_path(personal_access_token), method: :put, data: { confirm: _('Are you sure you want to revoke this personal access token? This action cannot be undone.') }, class: 'btn btn-danger btn-danger-secondary btn-md btn-secondary gl-button'

ee/config/feature_flags/development/revoke_managed_users_token.yml

----
-name: revoke_managed_users_token
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/44783
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/267184
-milestone: '13.5'
-type: development
-group: group::compliance
-default_enabled: false

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

@@ -173,118 +173,81 @@ RSpec.describe Groups::Security::CredentialsController do
     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)
+        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 'for a user with access to view credentials inventory' do
-            context 'non-existent personal access token specified' do
-              let(:token_id) { 999999999999999999999999999999999 }
+          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 'responds with 404'
+              it_behaves_like 'displays the flash success message'
             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 }
 
-              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
 
-                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 }
 
-              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
 
-                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 }
 
-              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
 
-                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 }
 
-              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
 
-                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 }
 
-              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_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)
 
-                it 'informs the token owner' do
-                  expect(CredentialsInventoryMailer).to receive_message_chain(:personal_access_token_revoked_email, :deliver_later)
+                put revoke_group_security_credential_path(group_id: group_id.to_param, id: personal_access_token.id)
+              end
 
-                  put revoke_group_security_credential_path(group_id: group_id.to_param, id: personal_access_token.id)
+              context 'when credentials_inventory_revocation_emails flag is disabled' do
+                before do
+                  stub_feature_flags(credentials_inventory_revocation_emails: false)
                 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_group_security_credential_path(group_id: group_id.to_param, id: personal_access_token.id)
-                    end.not_to change { ActionMailer::Base.deliveries.size }
-                  end
+                it 'does not inform the token owner' do
+                  expect do
+                    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
             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 }
+        context 'for a user without access to view credentials inventory' do
+          let_it_be(:token_id) { create(:personal_access_token, user: owner).id }
 
-              it_behaves_like 'responds with 404'
-            end
+          before do
+            sign_in(maintainer)
           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
+          it_behaves_like 'responds with 404'
         end
       end
 

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

@@ -42,17 +42,6 @@ RSpec.describe EE::PersonalAccessTokens::RevokeService do
         end
       end
 
-      context 'when feature flag is disabled' do
-        let_it_be(:current_user) { group_owner }
-        let_it_be(:token) { create(:personal_access_token, user: managed_user) }
-
-        before do
-          stub_feature_flags(revoke_managed_users_token: false)
-        end
-
-        it_behaves_like 'an unsuccessfully revoked token'
-      end
-
       context 'when current user is a group owner of a different managed group' do
         let_it_be(:group) { create(:group_with_managed_accounts) }
         let_it_be(:group_owner2) { create(:user) }

ee/spec/views/shared/credentials_inventory/personal_access_tokens/_personal_access_token.html.haml_spec.rb

@@ -12,7 +12,6 @@ RSpec.describe('shared/credentials_inventory/personal_access_tokens/_personal_ac
 
     allow(view).to receive(:user_detail_path).and_return('abcd')
     allow(view).to receive(:personal_access_token_revoke_path).and_return('revoke')
-    allow(view).to receive(:revoke_button_available?).and_return(false)
 
     render 'shared/credentials_inventory/personal_access_tokens/personal_access_token', personal_access_token: personal_access_token
   end
@@ -36,61 +35,31 @@ RSpec.describe('shared/credentials_inventory/personal_access_tokens/_personal_ac
   context 'revoked date' do
     let_it_be(:updated_at_date) { 10.days.ago }
 
-    context 'without revoke button available' do
-      context 'when revoked is set' do
-        let_it_be(:personal_access_token) { build_stubbed(:personal_access_token, user: user, updated_at: updated_at_date, revoked: true)}
+    before do
+      render 'shared/credentials_inventory/personal_access_tokens/personal_access_token', personal_access_token: personal_access_token
+    end
 
-        it 'shows the revoked on date' do
-          expect(rendered).to have_text(updated_at_date.to_date.to_s)
-        end
+    context 'when revoked is set' do
+      let_it_be(:personal_access_token) { build_stubbed(:personal_access_token, user: user, updated_at: updated_at_date, revoked: true)}
 
-        it 'does not show the revoke button' do
-          expect(rendered).not_to have_css('a.btn', text: 'Revoke')
-        end
+      it 'shows the revoked on date' do
+        expect(rendered).to have_text(updated_at_date.to_date.to_s)
       end
 
-      context 'when revoked is not set' do
-        let_it_be(:personal_access_token) { build_stubbed(:personal_access_token, user: user, updated_at: updated_at_date)}
-
-        it 'does not show the revoked on date' do
-          expect(rendered).not_to have_text(updated_at_date.to_date.to_s)
-        end
-
-        it 'does not show the revoke button' do
-          expect(rendered).not_to have_css('a.btn', text: 'Revoke')
-        end
+      it 'does not show the revoke button' do
+        expect(rendered).not_to have_css('a.btn', text: 'Revoke')
       end
     end
 
-    context 'with revoke button available' do
-      before do
-        allow(view).to receive(:revoke_button_available?).and_return(true)
+    context 'when revoked is not set' do
+      let_it_be(:personal_access_token) { build_stubbed(:personal_access_token, user: user, updated_at: updated_at_date)}
 
-        render 'shared/credentials_inventory/personal_access_tokens/personal_access_token', personal_access_token: personal_access_token
+      it 'does not show the revoked on date' do
+        expect(rendered).not_to have_text(updated_at_date.to_date.to_s)
       end
 
-      context 'when revoked is set' do
-        let_it_be(:personal_access_token) { build_stubbed(:personal_access_token, user: user, updated_at: updated_at_date, revoked: true)}
-
-        it 'shows the revoked on date' do
-          expect(rendered).to have_text(updated_at_date.to_date.to_s)
-        end
-
-        it 'does not show the revoke button' do
-          expect(rendered).not_to have_css('a.btn', text: 'Revoke')
-        end
-      end
-
-      context 'when revoked is not set' do
-        let_it_be(:personal_access_token) { build_stubbed(:personal_access_token, user: user, updated_at: updated_at_date)}
-
-        it 'does not show the revoked on date' do
-          expect(rendered).not_to have_text(updated_at_date.to_date.to_s)
-        end
-
-        it 'shows the revoke button' do
-          expect(rendered).to have_css('a.btn', text: 'Revoke')
-        end
+      it 'shows the revoke button' do
+        expect(rendered).to have_css('a.btn', text: 'Revoke')
       end
     end
   end