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 544362beb055264e3830751e2d72e3d240ffc940..ceec89019516af82b03fbc45772de5fe5503f53c 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 0000000000000000000000000000000000000000..ca0abc70b64c9d8adc949c2f9ab40404245165fc --- /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 87f6a280791fe45088ab5c94498662f3c6f1e6f3..ed152d1f930228cb5926ec5d804b090f3e141c43 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 bde3255b89961f822f11beafebcb7326c4e4f84a..13b292c7c72c27b92b35dcf46337ade707e5052d 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 e51e4457f20850620d5b52c19adbe21971f09a63..789b9181a0f2ad1313a64d65b9cbbcba88e80580 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 fa8e05a7bc6e71ca947c29b7b091045ed13d1793..42295414dbeab945d75e655a2be32e99e508bd13 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 3a6c888e2cfde50570da0474977f957abdb3c122..7d26ee57c6e23a9896ee3a655536bd7cf7a251f7 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 34688fc99df4a20e4cc19a0d1289471b7c93472d..ec4b7476248961d86015c4b23259e3aa70036f92 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 7db9e7db3120b332a317a3d19c805f58852e2a5c..fbf0bd680063577006d1e31905c82112db59df71 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 0000000000000000000000000000000000000000..671b14854bdb578a922d8ecc8fdd09c3686e2ebb --- /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 0000000000000000000000000000000000000000..8c48fae5fa1d4446d2692e0124918d70ceb3a87a --- /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 a81d8bf81357505fe0199e85f3463341d2810948..1c84b4b0b9f3438b72f84661ded120b1e6652ceb 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 0000000000000000000000000000000000000000..84304d3c4b837596d13bd61b8fb2f83dee528a1f --- /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 8bcb9b867cd027312f95dc4e445e4b322ffbb314..ea776776fa374681facac2197bdf962109f8c534 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 1bc0b612c6f181124a911e1d6a90d838fcc22b0e..eaab1b6ac51d4fa9640dfd1d3d8f7dfa245a3104 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 fd6a33111bb9027e664abc245dbc5c4e7457d9ca..89120cd21e7f9e3a011beb62a6ee3d8364417bc5 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 fd603418b9880454186b3567a9a7e333d32595c2..608652f3e766914ad42ccdfdddc83715a17bf511 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 ""