From d6021533f601ccb70f299195056fd7e4832b42c6 Mon Sep 17 00:00:00 2001
From: Aishwarya Subramanian <asubramanian@gitlab.com>
Date: Wed, 3 Jun 2020 14:47:29 -0500
Subject: [PATCH] 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.
---
 .../_account_and_limit.html.haml              |  1 +
 ..._pat_expiration_to_application_settings.rb |  9 +++
 db/structure.sql                              |  2 +
 .../helpers/ee/application_settings_helper.rb |  1 +
 .../helpers/personal_access_tokens_helper.rb  |  4 +
 ee/app/models/ee/application_setting.rb       |  1 +
 ee/app/models/ee/personal_access_token.rb     | 20 +++++
 ee/app/models/license.rb                      |  1 +
 .../revoke_invalid_tokens.rb                  |  1 +
 .../_enforce_pat_expiration.html.haml         |  9 +++
 .../automatic_expiry_access_token.yml         |  5 ++
 .../application_settings_controller_spec.rb   |  7 ++
 .../ee/application_settings_helper_spec.rb    | 11 +++
 .../personal_access_tokens_helper_spec.rb     | 26 ++++--
 .../models/ee/personal_access_token_spec.rb   | 56 +++++++++++++
 .../revoke_invalid_tokens_spec.rb             | 81 +++++++++++++------
 locale/gitlab.pot                             |  3 +
 17 files changed, 208 insertions(+), 30 deletions(-)
 create mode 100644 db/migrate/20200603180338_add_enforce_pat_expiration_to_application_settings.rb
 create mode 100644 ee/app/views/admin/application_settings/_enforce_pat_expiration.html.haml
 create mode 100644 ee/changelogs/unreleased/automatic_expiry_access_token.yml
 create mode 100644 ee/spec/helpers/ee/application_settings_helper_spec.rb

diff --git a/app/views/admin/application_settings/_account_and_limit.html.haml b/app/views/admin/application_settings/_account_and_limit.html.haml
index 544362beb05526..ceec89019516af 100644
--- a/app/views/admin/application_settings/_account_and_limit.html.haml
+++ b/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'
diff --git a/db/migrate/20200603180338_add_enforce_pat_expiration_to_application_settings.rb b/db/migrate/20200603180338_add_enforce_pat_expiration_to_application_settings.rb
new file mode 100644
index 00000000000000..ca0abc70b64c9d
--- /dev/null
+++ b/db/migrate/20200603180338_add_enforce_pat_expiration_to_application_settings.rb
@@ -0,0 +1,9 @@
+# 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
diff --git a/db/structure.sql b/db/structure.sql
index 87f6a280791fe4..ed152d1f930228 100644
--- a/db/structure.sql
+++ b/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
diff --git a/ee/app/helpers/ee/application_settings_helper.rb b/ee/app/helpers/ee/application_settings_helper.rb
index bde3255b89961f..13b292c7c72c27 100644
--- a/ee/app/helpers/ee/application_settings_helper.rb
+++ b/ee/app/helpers/ee/application_settings_helper.rb
@@ -42,6 +42,7 @@ def visible_attributes
         :help_text,
         :lock_memberships_to_ldap,
         :max_personal_access_token_lifetime,
+        :enforce_pat_expiration,
         :pseudonymizer_enabled,
         :repository_size_limit,
         :seat_link_enabled,
diff --git a/ee/app/helpers/personal_access_tokens_helper.rb b/ee/app/helpers/personal_access_tokens_helper.rb
index e51e4457f20850..789b9181a0f2ad 100644
--- a/ee/app/helpers/personal_access_tokens_helper.rb
+++ b/ee/app/helpers/personal_access_tokens_helper.rb
@@ -19,6 +19,10 @@ def personal_access_token_expiration_policy_licensed?
     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?
diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb
index fa8e05a7bc6e71..42295414dbeab9 100644
--- a/ee/app/models/ee/application_setting.rb
+++ b/ee/app/models/ee/application_setting.rb
@@ -107,6 +107,7 @@ def defaults
           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'],
diff --git a/ee/app/models/ee/personal_access_token.rb b/ee/app/models/ee/personal_access_token.rb
index 3a6c888e2cfde5..7d26ee57c6e23a 100644
--- a/ee/app/models/ee/personal_access_token.rb
+++ b/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 @@ def with_invalid_expires_at(max_lifetime, limit = 1_000)
           ]
         )
       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
diff --git a/ee/app/models/license.rb b/ee/app/models/license.rb
index 34688fc99df4a2..ec4b7476248961 100644
--- a/ee/app/models/license.rb
+++ b/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
diff --git a/ee/app/services/personal_access_tokens/revoke_invalid_tokens.rb b/ee/app/services/personal_access_tokens/revoke_invalid_tokens.rb
index 7db9e7db3120b3..fbf0bd68006357 100644
--- a/ee/app/services/personal_access_tokens/revoke_invalid_tokens.rb
+++ b/ee/app/services/personal_access_tokens/revoke_invalid_tokens.rb
@@ -9,6 +9,7 @@ def initialize(user, expiration_date)
 
     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
diff --git a/ee/app/views/admin/application_settings/_enforce_pat_expiration.html.haml b/ee/app/views/admin/application_settings/_enforce_pat_expiration.html.haml
new file mode 100644
index 00000000000000..671b14854bdb57
--- /dev/null
+++ b/ee/app/views/admin/application_settings/_enforce_pat_expiration.html.haml
@@ -0,0 +1,9 @@
+- 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')
diff --git a/ee/changelogs/unreleased/automatic_expiry_access_token.yml b/ee/changelogs/unreleased/automatic_expiry_access_token.yml
new file mode 100644
index 00000000000000..8c48fae5fa1d44
--- /dev/null
+++ b/ee/changelogs/unreleased/automatic_expiry_access_token.yml
@@ -0,0 +1,5 @@
+---
+title: Ability to make PAT expiration optional in self managed instances
+merge_request: 33783
+author:
+type: added
diff --git a/ee/spec/controllers/admin/application_settings_controller_spec.rb b/ee/spec/controllers/admin/application_settings_controller_spec.rb
index a81d8bf8135750..1c84b4b0b9f343 100644
--- a/ee/spec/controllers/admin/application_settings_controller_spec.rb
+++ b/ee/spec/controllers/admin/application_settings_controller_spec.rb
@@ -224,6 +224,13 @@
         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
diff --git a/ee/spec/helpers/ee/application_settings_helper_spec.rb b/ee/spec/helpers/ee/application_settings_helper_spec.rb
new file mode 100644
index 00000000000000..84304d3c4b8375
--- /dev/null
+++ b/ee/spec/helpers/ee/application_settings_helper_spec.rb
@@ -0,0 +1,11 @@
+# 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
diff --git a/ee/spec/helpers/personal_access_tokens_helper_spec.rb b/ee/spec/helpers/personal_access_tokens_helper_spec.rb
index 8bcb9b867cd027..ea776776fa3746 100644
--- a/ee/spec/helpers/personal_access_tokens_helper_spec.rb
+++ b/ee/spec/helpers/personal_access_tokens_helper_spec.rb
@@ -127,12 +127,10 @@
     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 @@
 
     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
diff --git a/ee/spec/models/ee/personal_access_token_spec.rb b/ee/spec/models/ee/personal_access_token_spec.rb
index 1bc0b612c6f181..eaab1b6ac51d4f 100644
--- a/ee/spec/models/ee/personal_access_token_spec.rb
+++ b/ee/spec/models/ee/personal_access_token_spec.rb
@@ -196,4 +196,60 @@
       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
diff --git a/ee/spec/services/personal_access_tokens/revoke_invalid_tokens_spec.rb b/ee/spec/services/personal_access_tokens/revoke_invalid_tokens_spec.rb
index fd6a33111bb902..89120cd21e7f9e 100644
--- a/ee/spec/services/personal_access_tokens/revoke_invalid_tokens_spec.rb
+++ b/ee/spec/services/personal_access_tokens/revoke_invalid_tokens_spec.rb
@@ -13,19 +13,64 @@
     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
-        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
+        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
+
+          it "revokes invalid user's tokens" do
+            service.execute
+
+            expect(pat.reload).not_to be_revoked
+            expect(invalid_pat1.reload).to be_revoked
+            expect(invalid_pat2.reload).to be_revoked
+          end
         end
 
-        it "revokes invalid user's tokens" do
-          service.execute
+        shared_examples 'does not revoke token' do
+          it_behaves_like 'user does not receive revoke notification email'
 
-          expect(pat.reload).not_to be_revoked
-          expect(invalid_pat1.reload).to be_revoked
-          expect(invalid_pat2.reload).to be_revoked
+          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
@@ -33,10 +78,7 @@
             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 @@
     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 @@
     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 @@
         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 }
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index fd603418b98804..608652f3e76691 100644
--- a/locale/gitlab.pot
+++ b/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 ""
 
-- 
GitLab