Merge branch 'automatic_expiry_access_token' into 'master'

Optional enforcement of Personal Access Token expiration

See merge request gitlab-org/gitlab!33783

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))
@@ -13959,6 +13960,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,19 +13,64 @@ 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
-        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 @@ RSpec.describe PersonalAccessTokens::RevokeInvalidTokens 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

@@ -8417,6 +8417,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 ""