Option to disable PAT expiry for self-managed instances

Adds an option in Admin Settings page to
toggle automatic expiry of PAT.
When enabled, token expires on the expiry date defined.
When disabled, token can continue to be used even after
expiry date.
If personal access token expiry policy is set, invalid
tokens will not be revoked in case the Automatic expiry
setting is disabled.
In a future MR, an in-app notification will be shown
to the user to update the token.

app/views/admin/application_settings/_account_and_limit.html.haml

@@ -38,6 +38,7 @@
       %span.form-text.text-muted#session_expire_delay_help_block= _('GitLab restart is required to apply changes.')
 
     = render_if_exists 'admin/application_settings/personal_access_token_expiration_policy', form: f
+    = render_if_exists 'admin/application_settings/enforce_pat_expiration', form: f
 
     .form-group
       = f.label :user_oauth_applications, _('User OAuth applications'), class: 'label-bold'

db/migrate/20200603180338_add_enforce_pat_expiration_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddEnforcePatExpirationToApplicationSettings < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :application_settings, :enforce_pat_expiration, :boolean, default: true, null: false
+  end
+end

db/structure.sql

@@ -477,6 +477,7 @@ CREATE TABLE public.application_settings (
     elasticsearch_pause_indexing boolean DEFAULT false NOT NULL,
     repository_storages_weighted jsonb DEFAULT '{}'::jsonb NOT NULL,
     max_import_size integer DEFAULT 50 NOT NULL,
+    enforce_pat_expiration boolean DEFAULT true NOT NULL,
     CONSTRAINT check_d03919528d CHECK ((char_length(container_registry_vendor) <= 255)),
     CONSTRAINT check_d820146492 CHECK ((char_length(spam_check_endpoint_url) <= 255)),
     CONSTRAINT check_e5aba18f02 CHECK ((char_length(container_registry_version) <= 255))
@@ -13958,6 +13959,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200602013900
 20200602013901
 20200603073101
+20200603180338
 20200604143628
 20200604145731
 20200604174544

ee/app/helpers/ee/application_settings_helper.rb

@@ -42,6 +42,7 @@ module EE
         :help_text,
         :lock_memberships_to_ldap,
         :max_personal_access_token_lifetime,
+        :enforce_pat_expiration,
         :pseudonymizer_enabled,
         :repository_size_limit,
         :seat_link_enabled,

ee/app/helpers/personal_access_tokens_helper.rb

@@ -19,6 +19,10 @@ module PersonalAccessTokensHelper
     License.feature_available?(:personal_access_token_expiration_policy)
   end
 
+  def enforce_pat_expiration_feature_available?
+    PersonalAccessToken.enforce_pat_expiration_feature_available?
+  end
+
   private
 
   def instance_level_personal_access_token_expiration_policy_enabled?

ee/app/models/ee/application_setting.rb

@@ -107,6 +107,7 @@ module EE
           email_additional_text: nil,
           lock_memberships_to_ldap: false,
           max_personal_access_token_lifetime: nil,
+          enforce_pat_expiration: true,
           mirror_capacity_threshold: Settings.gitlab['mirror_capacity_threshold'],
           mirror_max_capacity: Settings.gitlab['mirror_max_capacity'],
           mirror_max_delay: Settings.gitlab['mirror_max_delay'],

ee/app/models/ee/personal_access_token.rb

@@ -7,6 +7,7 @@ module EE
   # and be prepended in the `PersonalAccessToken` model
   module PersonalAccessToken
     extend ActiveSupport::Concern
+    extend ::Gitlab::Utils::Override
 
     prepended do
       include ::Gitlab::Utils::StrongMemoize
@@ -34,6 +35,25 @@ module EE
           ]
         )
       end
+
+      def expiration_enforced?
+        return true unless enforce_pat_expiration_feature_available?
+
+        ::Gitlab::CurrentSettings.enforce_pat_expiration?
+      end
+
+      def enforce_pat_expiration_feature_available?
+        License.feature_available?(:enforce_pat_expiration) &&
+          ::Feature.enabled?(:enforce_pat_expiration, default_enabled: false)
+      end
+    end
+
+    override :expired?
+    def expired?
+      return super if self.class.expiration_enforced?
+
+      # The user is notified about the expired-yet-active status of the token through an in-app banner: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/34101
+      false
     end
 
     private

ee/app/models/license.rb

@@ -121,6 +121,7 @@ class License < ApplicationRecord
     issuable_health_status
     license_scanning
     personal_access_token_expiration_policy
+    enforce_pat_expiration
     prometheus_alerts
     pseudonymizer
     report_approver_rules

ee/app/services/personal_access_tokens/revoke_invalid_tokens.rb

@@ -9,6 +9,7 @@ module PersonalAccessTokens
 
     def execute
       return unless ::Feature.enabled?(:personal_access_token_expiration_policy, default_enabled: true)
+      return unless PersonalAccessToken.expiration_enforced?
       return unless expiration_date && user_affected?
 
       notify_user

ee/app/views/admin/application_settings/_enforce_pat_expiration.html.haml

+- return unless enforce_pat_expiration_feature_available?
+
+- form = local_assigns.fetch(:form)
+
+.form-group
+  .form-check
+    = form.check_box :enforce_pat_expiration, class: 'form-check-input'
+    = form.label :enforce_pat_expiration, class: 'form-check-label' do
+      = _('Enforce personal access token expiration')

ee/changelogs/unreleased/automatic_expiry_access_token.yml

+---
+title: Ability to make PAT expiration optional in self managed instances
+merge_request: 33783
+author:
+type: added

ee/spec/controllers/admin/application_settings_controller_spec.rb

@@ -224,6 +224,13 @@ RSpec.describe Admin::ApplicationSettingsController do
         end
       end
     end
+
+    it 'updates setting to enforce personal access token expiration' do
+      put :update, params: { application_setting: { enforce_pat_expiration: false } }
+
+      expect(response).to redirect_to(general_admin_application_settings_path)
+      expect(ApplicationSetting.current.enforce_pat_expiration).to be_falsey
+    end
   end
 
   describe 'GET #seat_link_payload' do

ee/spec/helpers/ee/application_settings_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::ApplicationSettingsHelper do
+  describe '.visible_attributes' do
+    context 'personal access token parameters' do
+      it { expect(visible_attributes).to include(*%i(max_personal_access_token_lifetime enforce_pat_expiration)) }
+    end
+  end
+end

ee/spec/helpers/personal_access_tokens_helper_spec.rb

@@ -127,12 +127,10 @@ RSpec.describe PersonalAccessTokensHelper do
     end
   end
 
-  describe '#personal_access_token_expiration_policy_licensed?' do
-    subject { helper.personal_access_token_expiration_policy_licensed? }
-
-    context 'with `personal_access_token_expiration_policy` licensed' do
+  shared_examples 'feature availability' do
+    context 'when feature is licensed' do
       before do
-        stub_licensed_features(personal_access_token_expiration_policy: true)
+        stub_licensed_features(feature => true)
       end
 
       it { is_expected.to be_truthy }
@@ -140,10 +138,26 @@ RSpec.describe PersonalAccessTokensHelper do
 
     context 'with `personal_access_token_expiration_policy` not licensed' do
       before do
-        stub_licensed_features(personal_access_token_expiration_policy: false)
+        stub_licensed_features(feature => false)
       end
 
       it { is_expected.to be_falsey }
     end
   end
+
+  describe '#personal_access_token_expiration_policy_licensed?' do
+    subject { helper.personal_access_token_expiration_policy_licensed? }
+
+    let(:feature) { :personal_access_token_expiration_policy }
+
+    it_behaves_like 'feature availability'
+  end
+
+  describe '#enforce_pat_expiration_feature_available?' do
+    subject { helper.enforce_pat_expiration_feature_available? }
+
+    let(:feature) { :enforce_pat_expiration }
+
+    it_behaves_like 'feature availability'
+  end
 end

ee/spec/models/ee/personal_access_token_spec.rb

@@ -196,4 +196,60 @@ RSpec.describe PersonalAccessToken do
       expect(subject).not_to include(expired_token)
     end
   end
+
+  shared_examples 'enforcement of personal access token expiry' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:licensed, :application_setting, :result) do
+      true  | true   | true
+      true  | false  | false
+      false | true   | true
+      false | false  | true
+    end
+
+    with_them do
+      before do
+        stub_licensed_features(enforce_pat_expiration: licensed)
+        stub_application_setting(enforce_pat_expiration: application_setting)
+      end
+
+      it { expect(subject).to be result }
+    end
+  end
+
+  describe '.expiration_enforced??' do
+    subject { described_class.expiration_enforced? }
+
+    it_behaves_like 'enforcement of personal access token expiry'
+  end
+
+  describe '#expired?' do
+    let_it_be(:expired_token) { create(:personal_access_token, expires_at: 1.week.ago) }
+
+    subject { expired_token.expired? }
+
+    it_behaves_like 'enforcement of personal access token expiry'
+  end
+
+  describe '.enforce_pat_expiration_feature_available?' do
+    using RSpec::Parameterized::TableSyntax
+
+    subject { described_class.enforce_pat_expiration_feature_available? }
+
+    where(:feature_flag, :licensed, :result) do
+      true  | true   | true
+      true  | false  | false
+      false | true   | false
+      false | false  | false
+    end
+
+    with_them do
+      before do
+        stub_feature_flags(enforce_pat_expiration: feature_flag)
+        stub_licensed_features(enforce_pat_expiration: licensed)
+      end
+
+      it { expect(subject).to be result }
+    end
+  end
 end

ee/spec/services/personal_access_tokens/revoke_invalid_tokens_spec.rb

@@ -13,10 +13,20 @@ RSpec.describe PersonalAccessTokens::RevokeInvalidTokens do
     let_it_be(:invalid_pat1) { create(:personal_access_token, expires_at: nil, user: user) }
     let_it_be(:invalid_pat2) { create(:personal_access_token, expires_at: 20.days.from_now, user: user) }
 
+    shared_examples 'user does not receive revoke notification email' do
+      it 'does not send any notification to user' do
+        expect(Notify).not_to receive(:policy_revoked_personal_access_tokens_email).and_call_original
+
+        service.execute
+      end
+    end
+
     context 'with a valid user and expiration date' do
       context 'with user tokens that will be revoked' do
+        shared_examples 'revokes token' do
           it 'calls mailer to send an email notifying the user' do
             expect(Notify).to receive(:policy_revoked_personal_access_tokens_email).and_call_original
+
             service.execute
           end
 
@@ -27,16 +37,48 @@ RSpec.describe PersonalAccessTokens::RevokeInvalidTokens do
             expect(invalid_pat1.reload).to be_revoked
             expect(invalid_pat2.reload).to be_revoked
           end
+        end
+
+        shared_examples 'does not revoke token' do
+          it_behaves_like 'user does not receive revoke notification email'
+
+          it "does not revoke user's invalid tokens" do
+            service.execute
+
+            [pat, invalid_pat1, invalid_pat2].each do |token_object|
+              expect(token_object.reload).not_to be_revoked
+            end
+          end
+        end
+
+        it_behaves_like 'revokes token'
+
+        context 'enforcement of personal access token expiry' do
+          using RSpec::Parameterized::TableSyntax
+
+          where(:licensed, :application_setting, :behavior) do
+            true  | true   | 'revokes token'
+            true  | false  | 'does not revoke token'
+            false | true   | 'revokes token'
+            false | false  | 'revokes token'
+          end
+
+          with_them do
+            before do
+              stub_licensed_features(enforce_pat_expiration: licensed)
+              stub_application_setting(enforce_pat_expiration: application_setting)
+
+              it_behaves_like behavior
+            end
+          end
+        end
 
         context 'user optout for notifications' do
           before do
             allow(user).to receive(:can?).and_return(false)
           end
 
-          it "doesn't call mailer to send a notification" do
-            expect(Notify).not_to receive(:policy_revoked_personal_access_tokens_email)
-            service.execute
-          end
+          it_behaves_like 'user does not receive revoke notification email'
         end
       end
     end
@@ -44,10 +86,7 @@ RSpec.describe PersonalAccessTokens::RevokeInvalidTokens do
     context 'with no user' do
       let(:user) { nil }
 
-      it "doesn't call mailer to send an email notifying the user" do
-        expect(Notify).not_to receive(:policy_revoked_personal_access_tokens_email)
-        service.execute
-      end
+      it_behaves_like 'user does not receive revoke notification email'
 
       it "doesn't revoke user's tokens" do
         expect { service.execute }.not_to change { pat.reload.revoked }
@@ -57,10 +96,7 @@ RSpec.describe PersonalAccessTokens::RevokeInvalidTokens do
     context 'with no expiration date' do
       let(:expiration_date) { nil }
 
-      it "doesn't call mailer to send an email notifying the user" do
-        expect(Notify).not_to receive(:policy_revoked_personal_access_tokens_email)
-        service.execute
-      end
+      it_behaves_like 'user does not receive revoke notification email'
 
       it "doesn't revoke user's tokens" do
         expect { service.execute }.not_to change { pat.reload.revoked }
@@ -72,10 +108,7 @@ RSpec.describe PersonalAccessTokens::RevokeInvalidTokens do
         stub_feature_flags(personal_access_token_expiration_policy: false)
       end
 
-      it "doesn't call mailer to send an email notifying the user" do
-        expect(Notify).not_to receive(:policy_revoked_personal_access_tokens_email)
-        service.execute
-      end
+      it_behaves_like 'user does not receive revoke notification email'
 
       it "doesn't revoke user's tokens" do
         expect { service.execute }.not_to change { pat.reload.revoked }

locale/gitlab.pot

@@ -8411,6 +8411,9 @@ msgstr ""
 msgid "Enforce DNS rebinding attack protection"
 msgstr ""
 
+msgid "Enforce personal access token expiration"
+msgstr ""
+
 msgid "Ensure connectivity is available from the GitLab server to the Prometheus server"
 msgstr ""