From 2f9b059dfa5ce7459eb13b9960cb1ab0602a1e7f Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 15:24:20 +0100 Subject: [PATCH 01/16] Add max_personal_access_token_lifetime to application settings * Migration * Application setting admin view * license entry for ultimate --- .../application_settings/_account_and_limit.html.haml | 3 +++ ...onal_access_token_lifetime_to_application_settings.rb | 9 +++++++++ db/schema.rb | 1 + doc/api/settings.md | 1 + ee/app/helpers/ee/application_settings_helper.rb | 1 + ee/app/models/ee/application_setting.rb | 5 +++++ ee/app/models/license.rb | 1 + .../_personal_access_token_expiration_policy.html.haml | 7 +++++++ ee/spec/models/application_setting_spec.rb | 9 +++++++++ 9 files changed, 37 insertions(+) create mode 100644 db/migrate/20190920122420_add_max_personal_access_token_lifetime_to_application_settings.rb create mode 100644 ee/app/views/admin/application_settings/_personal_access_token_expiration_policy.html.haml 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 4358365504ae15..6b95c0f40c5c7a 100644 --- a/app/views/admin/application_settings/_account_and_limit.html.haml +++ b/app/views/admin/application_settings/_account_and_limit.html.haml @@ -23,6 +23,9 @@ = f.label :session_expire_delay, _('Session duration (minutes)'), class: 'label-light' = f.number_field :session_expire_delay, class: 'form-control' %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 + .form-group = f.label :user_oauth_applications, _('User OAuth applications'), class: 'label-bold' .form-check diff --git a/db/migrate/20190920122420_add_max_personal_access_token_lifetime_to_application_settings.rb b/db/migrate/20190920122420_add_max_personal_access_token_lifetime_to_application_settings.rb new file mode 100644 index 00000000000000..5a6e810dedea96 --- /dev/null +++ b/db/migrate/20190920122420_add_max_personal_access_token_lifetime_to_application_settings.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddMaxPersonalAccessTokenLifetimeToApplicationSettings < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def change + add_column :application_settings, :max_personal_access_token_lifetime, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index c33223f8b59cc7..6d6a0d83042662 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -325,6 +325,7 @@ t.string "encrypted_asset_proxy_secret_key_iv" t.string "static_objects_external_storage_url", limit: 255 t.string "static_objects_external_storage_auth_token", limit: 255 + t.integer "max_personal_access_token_lifetime" t.boolean "throttle_protected_paths_enabled", default: false, null: false t.integer "throttle_protected_paths_requests_per_period", default: 10, null: false t.integer "throttle_protected_paths_period_in_seconds", default: 60, null: false diff --git a/doc/api/settings.md b/doc/api/settings.md index ad9ffcbf872397..185cce6353ee6d 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -269,6 +269,7 @@ are listed in the descriptions of the relevant settings. | `max_artifacts_size` | integer | no | Maximum artifacts size in MB | | `max_attachment_size` | integer | no | Limit attachment size in MB | | `max_pages_size` | integer | no | Maximum size of pages repositories in MB | +| `max_personal_access_token_lifetime` | integer | no | **(ULTIMATE ONLY)** Maximum allowable lifetime for personal access tokens in days | | `metrics_enabled` | boolean | no | (**If enabled, requires:** `metrics_host`, `metrics_method_call_threshold`, `metrics_packet_size`, `metrics_pool_size`, `metrics_port`, `metrics_sample_interval` and `metrics_timeout`) Enable influxDB metrics. | | `metrics_host` | string | required by: `metrics_enabled` | InfluxDB host. | | `metrics_method_call_threshold` | integer | required by: `metrics_enabled` | A method call is only tracked when it takes longer than the given amount of milliseconds. | diff --git a/ee/app/helpers/ee/application_settings_helper.rb b/ee/app/helpers/ee/application_settings_helper.rb index 3823da8a0dff66..4e270f7354693b 100644 --- a/ee/app/helpers/ee/application_settings_helper.rb +++ b/ee/app/helpers/ee/application_settings_helper.rb @@ -37,6 +37,7 @@ def visible_attributes :geo_node_allowed_ips, :help_text, :lock_memberships_to_ldap, + :max_personal_access_token_lifetime, :pseudonymizer_enabled, :repository_size_limit, :shared_runners_minutes, diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index bba54a8bcfc2e9..ff8aea0be81a1f 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -60,6 +60,10 @@ module ApplicationSetting validates :required_instance_ci_template, presence: true, allow_nil: true validate :check_geo_node_allowed_ips + + validates :max_personal_access_token_lifetime, + allow_blank: true, + numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 365 } end class_methods do @@ -77,6 +81,7 @@ def defaults elasticsearch_url: ENV['ELASTIC_URL'] || 'http://localhost:9200', email_additional_text: nil, lock_memberships_to_ldap: false, + max_personal_access_token_lifetime: nil, 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/license.rb b/ee/app/models/license.rb index dbf8756c9517f9..40f24df3f003e5 100644 --- a/ee/app/models/license.rb +++ b/ee/app/models/license.rb @@ -113,6 +113,7 @@ class License < ApplicationRecord insights licenses_list license_management + personal_access_token_expiration_policy pod_logs prometheus_alerts pseudonymizer diff --git a/ee/app/views/admin/application_settings/_personal_access_token_expiration_policy.html.haml b/ee/app/views/admin/application_settings/_personal_access_token_expiration_policy.html.haml new file mode 100644 index 00000000000000..0664e1b4dfb230 --- /dev/null +++ b/ee/app/views/admin/application_settings/_personal_access_token_expiration_policy.html.haml @@ -0,0 +1,7 @@ +- return unless License.feature_available?(:personal_access_token_expiration_policy) +- form = local_assigns.fetch(:form) + +.form-group + = form.label :max_personal_access_token_lifetime, _('Maximum allowable lifetime for personal access token (days)'), class: 'label-light' + = form.number_field :max_personal_access_token_lifetime, class: 'form-control input-xs' + %span.form-text.text-muted#max_personal_access_token_lifetime= _('Leave blank for no limit. Once set, existing personal access tokens may be revoked') diff --git a/ee/spec/models/application_setting_spec.rb b/ee/spec/models/application_setting_spec.rb index 5bdc506106be90..0ea531e81b7113 100644 --- a/ee/spec/models/application_setting_spec.rb +++ b/ee/spec/models/application_setting_spec.rb @@ -46,6 +46,15 @@ it { is_expected.not_to allow_value(" ").for(:required_instance_ci_template) } it { is_expected.to allow_value("template_name").for(:required_instance_ci_template) } + it { is_expected.to allow_value(1).for(:max_personal_access_token_lifetime) } + it { is_expected.to allow_value(nil).for(:max_personal_access_token_lifetime) } + it { is_expected.to allow_value(10).for(:max_personal_access_token_lifetime) } + it { is_expected.to allow_value(365).for(:max_personal_access_token_lifetime) } + it { is_expected.not_to allow_value("value").for(:max_personal_access_token_lifetime) } + it { is_expected.not_to allow_value(2.5).for(:max_personal_access_token_lifetime) } + it { is_expected.not_to allow_value(-5).for(:max_personal_access_token_lifetime) } + it { is_expected.not_to allow_value(366).for(:max_personal_access_token_lifetime) } + describe 'when additional email text is enabled' do before do stub_licensed_features(email_additional_text: true) -- GitLab From acb2390dd9fa4b18c0796b2addd60aac3e506b9c Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 15:40:24 +0100 Subject: [PATCH 02/16] Add PAT policy validations --- app/models/personal_access_token.rb | 2 + ee/app/models/ee/personal_access_token.rb | 34 ++++++++++ .../models/ee/personal_access_token_spec.rb | 64 +++++++++++++++++++ 3 files changed, 100 insertions(+) create mode 100644 ee/app/models/ee/personal_access_token.rb create mode 100644 ee/spec/models/ee/personal_access_token_spec.rb diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index 7ae431eaad77fa..770fc2b5c8fd17 100644 --- a/app/models/personal_access_token.rb +++ b/app/models/personal_access_token.rb @@ -70,3 +70,5 @@ def self.redis_shared_state_key(user_id) "gitlab:personal_access_token:#{user_id}" end end + +PersonalAccessToken.prepend_if_ee('EE::PersonalAccessToken') diff --git a/ee/app/models/ee/personal_access_token.rb b/ee/app/models/ee/personal_access_token.rb new file mode 100644 index 00000000000000..1e3a57de865000 --- /dev/null +++ b/ee/app/models/ee/personal_access_token.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module EE + # PersonalAccessToken EE mixin + # + # This module is intended to encapsulate EE-specific model logic + # and be prepended in the `PersonalAccessToken` model + module PersonalAccessToken + extend ActiveSupport::Concern + + prepended do + with_options if: :max_personal_access_token_lifetime_enabled? do + validates :expires_at, presence: true + validate :expires_at_before_max_lifetime + end + end + + private + + def max_expiration_date + @max_expiration_date ||= ::Gitlab::CurrentSettings.max_personal_access_token_lifetime&.days&.from_now + end + + def max_personal_access_token_lifetime_enabled? + max_expiration_date.present? && License.feature_available?(:personal_access_token_expiration_policy) + end + + def expires_at_before_max_lifetime + return if expires_at.blank? + + errors.add(:expires_at, :invalid) if expires_at > max_expiration_date + end + end +end diff --git a/ee/spec/models/ee/personal_access_token_spec.rb b/ee/spec/models/ee/personal_access_token_spec.rb new file mode 100644 index 00000000000000..f15a1fc64c795c --- /dev/null +++ b/ee/spec/models/ee/personal_access_token_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe PersonalAccessToken do + subject { described_class } + + describe 'validations' do + let(:personal_access_token) { build(:personal_access_token) } + + it 'allows to define expires_at' do + personal_access_token.expires_at = 1.day.from_now + + expect(personal_access_token).to be_valid + end + + it "allows to don't define expires_at" do + personal_access_token.expires_at = nil + + expect(personal_access_token).to be_valid + end + + context 'with expiration policy' do + let(:pat_expiration_policy) { 30 } + let(:max_expiration_date) { pat_expiration_policy.days.from_now } + + before do + stub_ee_application_setting(max_personal_access_token_lifetime: pat_expiration_policy) + end + + context 'when the feature is licensed' do + before do + stub_licensed_features(personal_access_token_expiration_policy: true) + end + + it 'requires to be less or equal than the max_personal_access_token_lifetime' do + personal_access_token.expires_at = max_expiration_date + 1.day + + expect(personal_access_token).not_to be_valid + expect(personal_access_token.errors[:expires_at].first).to eq('is invalid') + end + + it "can't be blank" do + personal_access_token.expires_at = nil + + expect(personal_access_token).not_to be_valid + expect(personal_access_token.errors[:expires_at].first).to eq("can't be blank") + end + end + + context 'when the feature is not available' do + before do + stub_licensed_features(personal_access_token_expiration_policy: false) + end + + it 'allows to be after the max_personal_access_token_lifetime' do + personal_access_token.expires_at = max_expiration_date + 1.day + + expect(personal_access_token).to be_valid + end + end + end + end +end -- GitLab From a0f67602b4dd701320766a179d96f9ffb234f27b Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 15:49:11 +0100 Subject: [PATCH 03/16] Add visual awareness when the policy is active to PAT creation --- app/helpers/profiles_helper.rb | 4 ++++ app/views/shared/_personal_access_tokens_form.html.haml | 3 +++ .../_callout_max_personal_access_token_lifetime.html.haml | 4 ++++ 3 files changed, 11 insertions(+) create mode 100644 ee/app/views/personal_access_tokens/_callout_max_personal_access_token_lifetime.html.haml diff --git a/app/helpers/profiles_helper.rb b/app/helpers/profiles_helper.rb index 5a42e581867a72..05d8b49bb8a72d 100644 --- a/app/helpers/profiles_helper.rb +++ b/app/helpers/profiles_helper.rb @@ -29,4 +29,8 @@ def attribute_provider_label(attribute) def user_profile? params[:controller] == 'users' end + + def personal_access_token_max_lifetime + Gitlab::CurrentSettings.max_personal_access_token_lifetime&.days&.from_now&.to_date + end end diff --git a/app/views/shared/_personal_access_tokens_form.html.haml b/app/views/shared/_personal_access_tokens_form.html.haml index ca0b473addf003..16f8a69263520f 100644 --- a/app/views/shared/_personal_access_tokens_form.html.haml +++ b/app/views/shared/_personal_access_tokens_form.html.haml @@ -18,6 +18,9 @@ .form-group.col-md-6 = f.label :expires_at, _('Expires at'), class: 'label-bold' .input-icon-wrapper + + = render_if_exists 'personal_access_tokens/callout_max_personal_access_token_lifetime' + = f.text_field :expires_at, class: "datepicker form-control", placeholder: 'YYYY-MM-DD' .form-group diff --git a/ee/app/views/personal_access_tokens/_callout_max_personal_access_token_lifetime.html.haml b/ee/app/views/personal_access_tokens/_callout_max_personal_access_token_lifetime.html.haml new file mode 100644 index 00000000000000..850e22fcd7cffc --- /dev/null +++ b/ee/app/views/personal_access_tokens/_callout_max_personal_access_token_lifetime.html.haml @@ -0,0 +1,4 @@ +- return unless Gitlab::CurrentSettings.max_personal_access_token_lifetime && License.feature_available(:personal_access_token_expiration_policy) + +.bs-callout.bs-callout-danger + = _('Maximum lifetime allowable for Personal Access Tokens is active, your expire date must be set before %{maximum_allowable_date}.') % { maximum_allowable_date: ::Gitlab::CurrentSettings.max_personal_access_token_lifetime_from_now } -- GitLab From 20b3f269d9b7a9c86c0224d4794d1c0ef6a7986d Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 15:52:42 +0100 Subject: [PATCH 04/16] Add PersonalAccessTokenPolicyWorker to change live tokens affected Includes: * Worker * PAT scopes to find affected tokens * Queue configuration --- config/sidekiq_queues.yml | 2 ++ ee/app/models/ee/personal_access_token.rb | 3 ++ ee/app/workers/all_queues.yml | 1 + .../personal_access_token_policy_worker.rb | 19 +++++++++++ .../models/ee/personal_access_token_spec.rb | 25 ++++++++++++++ ...ersonal_access_token_policy_worker_spec.rb | 33 +++++++++++++++++++ 6 files changed, 83 insertions(+) create mode 100644 ee/app/workers/personal_access_token_policy_worker.rb create mode 100644 ee/spec/workers/personal_access_token_policy_worker_spec.rb diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index b4be61d8a3d713..35a02f85257740 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -123,3 +123,5 @@ - [refresh_license_compliance_checks, 2] - [design_management_new_version, 1] - [epics, 2] + - [personal_access_token_policy, 1] + diff --git a/ee/app/models/ee/personal_access_token.rb b/ee/app/models/ee/personal_access_token.rb index 1e3a57de865000..744a903bacbba7 100644 --- a/ee/app/models/ee/personal_access_token.rb +++ b/ee/app/models/ee/personal_access_token.rb @@ -9,6 +9,9 @@ module PersonalAccessToken extend ActiveSupport::Concern prepended do + scope :with_expires_at_after, -> (max_lifetime) { where(["revoked = false AND expires_at > ?", max_lifetime]) } + scope :with_no_expires_at, -> { where(revoked: false, expires_at: nil) } + with_options if: :max_personal_access_token_lifetime_enabled? do validates :expires_at, presence: true validate :expires_at_before_max_lifetime diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 55be22a1de4f95..c3be7544f7b62d 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -69,6 +69,7 @@ - export_csv - ldap_group_sync - new_epic +- personal_access_token_policy - project_import_schedule - project_update_repository_storage - rebase diff --git a/ee/app/workers/personal_access_token_policy_worker.rb b/ee/app/workers/personal_access_token_policy_worker.rb new file mode 100644 index 00000000000000..45b0bb029a6193 --- /dev/null +++ b/ee/app/workers/personal_access_token_policy_worker.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class PersonalAccessTokenPolicyWorker + include ApplicationWorker + + feature_category :authentication_and_authorization + + def perform + lifetime = ::Gitlab::CurrentSettings.max_personal_access_token_lifetime + + return if lifetime.blank? + + expire_at = lifetime.days.from_now + + PersonalAccessToken.invalid_lifetime(expire_at).in_batches(of: 500).update_all(expires_at: expire_at) + + PersonalAccessToken.with_no_expires_at.in_batches(of: 500).update_all(expires_at: expire_at) + 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 f15a1fc64c795c..aa608c3bb65db2 100644 --- a/ee/spec/models/ee/personal_access_token_spec.rb +++ b/ee/spec/models/ee/personal_access_token_spec.rb @@ -5,6 +5,31 @@ describe PersonalAccessToken do subject { described_class } + describe 'scopes' do + let_it_be(:expired_token) { create(:personal_access_token, expires_at: 1.day.ago) } + let_it_be(:valid_token) { create(:personal_access_token, expires_at: 1.day.from_now) } + + describe 'with_expires_at_after' do + context 'token with an expire at higher than the lifetime' do + let_it_be(:pat) { create(:personal_access_token, expires_at: 3.days.from_now) } + + it 'includes the tokens with higher than the lifetime expires_at value' do + expect(described_class.with_expires_at_after(2.days.from_now)).to contain_exactly(pat) + end + end + end + + describe 'with_no_expires_at' do + context 'token with nil expires_at' do + let_it_be(:pat) { create(:personal_access_token, expires_at: nil) } + + it 'includes the tokens with nil expires_at' do + expect(described_class.with_no_expires_at).to contain_exactly(pat) + end + end + end + end + describe 'validations' do let(:personal_access_token) { build(:personal_access_token) } diff --git a/ee/spec/workers/personal_access_token_policy_worker_spec.rb b/ee/spec/workers/personal_access_token_policy_worker_spec.rb new file mode 100644 index 00000000000000..a150e00886bd26 --- /dev/null +++ b/ee/spec/workers/personal_access_token_policy_worker_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe PersonalAccessTokenPolicyWorker, type: :worker do + subject(:worker) { described_class.new } + + describe '#perform' do + let(:limit) { 7 } + let(:timelimit) { limit.days.from_now.to_date } + + before do + stub_application_setting(max_personal_access_token_lifetime: limit) + end + + context "when a token doesn't have an expiration time" do + let!(:pat) { create(:personal_access_token, expires_at: nil) } + + it 'enforces the policy on tokens' do + expect { worker.perform }.to change { PersonalAccessToken.find(pat.id).expires_at }.from(nil).to(timelimit) + end + end + + context 'when a token expires after the given time' do + let(:expire_at) { 8.days.from_now.to_date } + let!(:pat) { create(:personal_access_token, expires_at: expire_at) } + + it 'enforces the policy on tokens' do + expect { worker.perform }.to change { PersonalAccessToken.find(pat.id).expires_at }.from(expire_at).to(timelimit) + end + end + end +end -- GitLab From 068bf1d953c2fcc3a6b6abd77ad74a47ee0d44a5 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 15:53:53 +0100 Subject: [PATCH 05/16] Add after_commit hook change_personal_access_tokens_lifetime --- ee/app/models/ee/application_setting.rb | 31 +++++++++++++++ ee/spec/models/application_setting_spec.rb | 45 ++++++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index ff8aea0be81a1f..7baf66ed810ba6 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -9,6 +9,9 @@ module ApplicationSetting extend ActiveSupport::Concern prepended do + include ExclusiveLeaseGuard + + DEFAULT_LEASE_TIMEOUT = 3.hours.to_i EMAIL_ADDITIONAL_TEXT_CHARACTER_LIMIT = 10_000 INSTANCE_REVIEW_MIN_USERS = 100 @@ -64,6 +67,8 @@ module ApplicationSetting validates :max_personal_access_token_lifetime, allow_blank: true, numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 365 } + + after_commit :change_personal_access_tokens_lifetime, if: :saved_change_to_max_personal_access_token_lifetime? end class_methods do @@ -238,6 +243,32 @@ def instance_review_permitted? private + def change_personal_access_tokens_lifetime + return unless max_personal_access_token_lifetime.present? && License.feature_available?(:personal_access_token_expiration_policy) + + try_obtain_lease do + ::PersonalAccessTokenPolicyWorker.perform_in(3.hours.to_i) + end + end + + # Used by ExclusiveLeaseGuard + def lease_timeout + DEFAULT_LEASE_TIMEOUT + end + + # Used by ExclusiveLeaseGuard + def lease_key + "application_settings:#{id}" + end + + # Used by ExclusiveLeaseGuard + # Overriding value as we never release the lease + # before the timeout in order to prevent multiple + # PersonalAccessTokenPolicyWorker to start in a short span of time + def lease_release? + false + end + def mirror_max_delay_in_minutes ::Gitlab::Mirror.min_delay_upper_bound / 60 end diff --git a/ee/spec/models/application_setting_spec.rb b/ee/spec/models/application_setting_spec.rb index 0ea531e81b7113..6e74524aa30e90 100644 --- a/ee/spec/models/application_setting_spec.rb +++ b/ee/spec/models/application_setting_spec.rb @@ -425,4 +425,49 @@ def expect_is_es_licensed end end end + + describe 'changes to max_personal_access_token_lifetime', :clean_gitlab_redis_shared_state do + let(:expiration_delay) { 30 } + let(:double_expiration_date) { (2 * expiration_delay).days.from_now.to_date } + let(:maximum_expiration_date) { expiration_delay.days.from_now.to_date } + let(:token_without_expiration_date) { create(:personal_access_token, expires_at: nil) } + let(:token_with_longer_expiration_date) { create(:personal_access_token, expires_at: double_expiration_date) } + let(:token_within_the_max_expiration_date) { create(:personal_access_token, expires_at: expiration_delay.days.from_now.to_date) } + + before do + setting.max_personal_access_token_lifetime = expiration_delay + end + + context 'without personal_access_token_expiration_policy licensed' do + before do + stub_licensed_features(personal_access_token_expiration_policy: false) + end + + it "doesn't enqueue a policy job" do + expect { setting.save }.not_to change { PersonalAccessTokenPolicyWorker.jobs.count } + end + end + + context 'with personal_access_token_expiration_policy licensed' do + before do + stub_licensed_features(personal_access_token_expiration_policy: true) + end + + it 'enqueues a policy worker job' do + expect { setting.save }.to change { PersonalAccessTokenPolicyWorker.jobs.count }.from(0).to(1) + end + + context 'max_personal_access_token_lifetime changed before job is executed' do + before do + setting.save + + setting.max_personal_access_token_lifetime = 1 + end + + it "doesn't enqueue a second policy job" do + expect { setting.save }.to_not change { PersonalAccessTokenPolicyWorker.jobs.count } + end + end + end + end end -- GitLab From d2dd6bc4980779010c3f78d3b02ddb2d880e845c Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 15:54:43 +0100 Subject: [PATCH 06/16] Add changelog entry --- ee/changelogs/unreleased/3649-pat-expiry-policy.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ee/changelogs/unreleased/3649-pat-expiry-policy.yml diff --git a/ee/changelogs/unreleased/3649-pat-expiry-policy.yml b/ee/changelogs/unreleased/3649-pat-expiry-policy.yml new file mode 100644 index 00000000000000..e905108740e091 --- /dev/null +++ b/ee/changelogs/unreleased/3649-pat-expiry-policy.yml @@ -0,0 +1,5 @@ +--- +title: Add Personal access token expiry policy +merge_request: 17344 +author: +type: added -- GitLab From 037ef19919f3d11646f84a54c1af41bac7acc642 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Wed, 30 Oct 2019 17:59:23 +0100 Subject: [PATCH 07/16] Add max_personal_access_token_lifetime_from_now helper This is to reduce the amount of times that we are calling Gitlab::CurrentSettings.max_personal_access_token_lifetime.days.from_now --- app/helpers/profiles_helper.rb | 4 --- ee/app/models/ee/application_setting.rb | 4 +++ ee/app/models/ee/personal_access_token.rb | 2 +- .../personal_access_token_policy_worker.rb | 6 ++-- ee/spec/models/application_setting_spec.rb | 32 +++++++++++++++++++ 5 files changed, 39 insertions(+), 9 deletions(-) diff --git a/app/helpers/profiles_helper.rb b/app/helpers/profiles_helper.rb index 05d8b49bb8a72d..5a42e581867a72 100644 --- a/app/helpers/profiles_helper.rb +++ b/app/helpers/profiles_helper.rb @@ -29,8 +29,4 @@ def attribute_provider_label(attribute) def user_profile? params[:controller] == 'users' end - - def personal_access_token_max_lifetime - Gitlab::CurrentSettings.max_personal_access_token_lifetime&.days&.from_now&.to_date - end end diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index 7baf66ed810ba6..b51e048df615fc 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -241,6 +241,10 @@ def instance_review_permitted? users_count >= INSTANCE_REVIEW_MIN_USERS end + def max_personal_access_token_lifetime_from_now + max_personal_access_token_lifetime&.days&.from_now + end + private def change_personal_access_tokens_lifetime diff --git a/ee/app/models/ee/personal_access_token.rb b/ee/app/models/ee/personal_access_token.rb index 744a903bacbba7..b69626bc17678f 100644 --- a/ee/app/models/ee/personal_access_token.rb +++ b/ee/app/models/ee/personal_access_token.rb @@ -21,7 +21,7 @@ module PersonalAccessToken private def max_expiration_date - @max_expiration_date ||= ::Gitlab::CurrentSettings.max_personal_access_token_lifetime&.days&.from_now + @max_expiration_date ||= ::Gitlab::CurrentSettings.max_personal_access_token_lifetime_from_now end def max_personal_access_token_lifetime_enabled? diff --git a/ee/app/workers/personal_access_token_policy_worker.rb b/ee/app/workers/personal_access_token_policy_worker.rb index 45b0bb029a6193..3294952e043e54 100644 --- a/ee/app/workers/personal_access_token_policy_worker.rb +++ b/ee/app/workers/personal_access_token_policy_worker.rb @@ -6,11 +6,9 @@ class PersonalAccessTokenPolicyWorker feature_category :authentication_and_authorization def perform - lifetime = ::Gitlab::CurrentSettings.max_personal_access_token_lifetime + expire_at = ::Gitlab::CurrentSettings.max_personal_access_token_lifetime_from_now - return if lifetime.blank? - - expire_at = lifetime.days.from_now + return unless expire_at PersonalAccessToken.invalid_lifetime(expire_at).in_batches(of: 500).update_all(expires_at: expire_at) diff --git a/ee/spec/models/application_setting_spec.rb b/ee/spec/models/application_setting_spec.rb index 6e74524aa30e90..86c332395bd290 100644 --- a/ee/spec/models/application_setting_spec.rb +++ b/ee/spec/models/application_setting_spec.rb @@ -426,6 +426,38 @@ def expect_is_es_licensed end end + describe '#max_personal_access_token_lifetime_from_now' do + subject { setting.max_personal_access_token_lifetime_from_now } + + let(:days_from_now) { nil } + + before do + stub_application_setting(max_personal_access_token_lifetime: days_from_now) + end + + context 'when max_personal_access_token_lifetime is defined' do + let(:days_from_now) { 30 } + + it 'is a date time' do + expect(subject).to be_a Time + end + + it 'is in the future' do + expect(subject).to be > Time.zone.now + end + + it 'is in days_from_now' do + expect(subject.to_date - Date.today).to eq days_from_now + end + end + + context 'when max_personal_access_token_lifetime is nil' do + it 'is nil' do + expect(subject).to be_nil + end + end + end + describe 'changes to max_personal_access_token_lifetime', :clean_gitlab_redis_shared_state do let(:expiration_delay) { 30 } let(:double_expiration_date) { (2 * expiration_delay).days.from_now.to_date } -- GitLab From d19d0603b1d0594cb5eea20e5f2930eaf0514830 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Wed, 30 Oct 2019 19:05:34 +0100 Subject: [PATCH 08/16] Move out of application_settings lease for PAT policy worker The lease is now part of the new PersonalAccessTokens::UpdateLifetimeService --- ee/app/models/ee/application_setting.rb | 29 ++------------- .../update_lifetime_service.rb | 30 ++++++++++++++++ ee/spec/models/application_setting_spec.rb | 36 ++++++------------- .../update_lifetime_service_spec.rb | 25 +++++++++++++ 4 files changed, 68 insertions(+), 52 deletions(-) create mode 100644 ee/app/services/personal_access_tokens/update_lifetime_service.rb create mode 100644 ee/spec/services/personal_access_tokens/update_lifetime_service_spec.rb diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index b51e048df615fc..29a901211d0007 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -9,9 +9,6 @@ module ApplicationSetting extend ActiveSupport::Concern prepended do - include ExclusiveLeaseGuard - - DEFAULT_LEASE_TIMEOUT = 3.hours.to_i EMAIL_ADDITIONAL_TEXT_CHARACTER_LIMIT = 10_000 INSTANCE_REVIEW_MIN_USERS = 100 @@ -68,7 +65,7 @@ module ApplicationSetting allow_blank: true, numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 365 } - after_commit :change_personal_access_tokens_lifetime, if: :saved_change_to_max_personal_access_token_lifetime? + after_commit :update_personal_access_tokens_lifetime, if: :saved_change_to_max_personal_access_token_lifetime? end class_methods do @@ -247,30 +244,10 @@ def max_personal_access_token_lifetime_from_now private - def change_personal_access_tokens_lifetime + def update_personal_access_tokens_lifetime return unless max_personal_access_token_lifetime.present? && License.feature_available?(:personal_access_token_expiration_policy) - try_obtain_lease do - ::PersonalAccessTokenPolicyWorker.perform_in(3.hours.to_i) - end - end - - # Used by ExclusiveLeaseGuard - def lease_timeout - DEFAULT_LEASE_TIMEOUT - end - - # Used by ExclusiveLeaseGuard - def lease_key - "application_settings:#{id}" - end - - # Used by ExclusiveLeaseGuard - # Overriding value as we never release the lease - # before the timeout in order to prevent multiple - # PersonalAccessTokenPolicyWorker to start in a short span of time - def lease_release? - false + ::PersonalAccessTokens::UpdateLifetimeService.new.execute end def mirror_max_delay_in_minutes diff --git a/ee/app/services/personal_access_tokens/update_lifetime_service.rb b/ee/app/services/personal_access_tokens/update_lifetime_service.rb new file mode 100644 index 00000000000000..0a8960716fc8cd --- /dev/null +++ b/ee/app/services/personal_access_tokens/update_lifetime_service.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module PersonalAccessTokens + class UpdateLifetimeService + include ExclusiveLeaseGuard + + DEFAULT_LEASE_TIMEOUT = 3.hours.to_i + + def execute + try_obtain_lease do + ::PersonalAccessTokenPolicyWorker.perform_in(3.hours.to_i) + end + end + + private + + # Used by ExclusiveLeaseGuard + def lease_timeout + DEFAULT_LEASE_TIMEOUT + end + + # Used by ExclusiveLeaseGuard + # Overriding value as we never release the lease + # before the timeout in order to prevent multiple + # PersonalAccessTokenPolicyWorker to start in a short span of time + def lease_release? + false + end + end +end diff --git a/ee/spec/models/application_setting_spec.rb b/ee/spec/models/application_setting_spec.rb index 86c332395bd290..6a8b7ef57f3760 100644 --- a/ee/spec/models/application_setting_spec.rb +++ b/ee/spec/models/application_setting_spec.rb @@ -458,47 +458,31 @@ def expect_is_es_licensed end end - describe 'changes to max_personal_access_token_lifetime', :clean_gitlab_redis_shared_state do - let(:expiration_delay) { 30 } - let(:double_expiration_date) { (2 * expiration_delay).days.from_now.to_date } - let(:maximum_expiration_date) { expiration_delay.days.from_now.to_date } - let(:token_without_expiration_date) { create(:personal_access_token, expires_at: nil) } - let(:token_with_longer_expiration_date) { create(:personal_access_token, expires_at: double_expiration_date) } - let(:token_within_the_max_expiration_date) { create(:personal_access_token, expires_at: expiration_delay.days.from_now.to_date) } - - before do - setting.max_personal_access_token_lifetime = expiration_delay - end - + describe 'updates to max_personal_access_token_lifetime' do context 'without personal_access_token_expiration_policy licensed' do before do stub_licensed_features(personal_access_token_expiration_policy: false) end - it "doesn't enqueue a policy job" do - expect { setting.save }.not_to change { PersonalAccessTokenPolicyWorker.jobs.count } + it "doesn't call the update lifetime service" do + expect(::PersonalAccessTokens::UpdateLifetimeService).not_to receive(:new) + + setting.save end end context 'with personal_access_token_expiration_policy licensed' do before do + setting.max_personal_access_token_lifetime = 30 stub_licensed_features(personal_access_token_expiration_policy: true) end - it 'enqueues a policy worker job' do - expect { setting.save }.to change { PersonalAccessTokenPolicyWorker.jobs.count }.from(0).to(1) - end - - context 'max_personal_access_token_lifetime changed before job is executed' do - before do - setting.save - - setting.max_personal_access_token_lifetime = 1 + it 'executes the update lifetime service' do + expect_next_instance_of(::PersonalAccessTokens::UpdateLifetimeService) do |service| + expect(service).to receive(:execute) end - it "doesn't enqueue a second policy job" do - expect { setting.save }.to_not change { PersonalAccessTokenPolicyWorker.jobs.count } - end + setting.save end end end diff --git a/ee/spec/services/personal_access_tokens/update_lifetime_service_spec.rb b/ee/spec/services/personal_access_tokens/update_lifetime_service_spec.rb new file mode 100644 index 00000000000000..742edfb9d30d3b --- /dev/null +++ b/ee/spec/services/personal_access_tokens/update_lifetime_service_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe PersonalAccessTokens::UpdateLifetimeService do + subject { described_class.new } + + describe '#execute', :clean_gitlab_redis_shared_state do + context 'with personal_access_token_expiration_policy licensed' do + it 'enqueues a policy worker job' do + expect { subject.execute }.to change { PersonalAccessTokenPolicyWorker.jobs.count }.from(0).to(1) + end + + context 'before the lease is released job is executed' do + before do + subject.execute + end + + it "doesn't enqueue a second policy job" do + expect { subject.execute }.not_to change { PersonalAccessTokenPolicyWorker.jobs.count } + end + end + end + end +end -- GitLab From 14057be96e3eb7c8a49086f0757874bdc6b58231 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Wed, 30 Oct 2019 19:28:19 +0100 Subject: [PATCH 09/16] Use strong memoize for max_expiration_date in PAT --- ee/app/models/ee/personal_access_token.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/ee/app/models/ee/personal_access_token.rb b/ee/app/models/ee/personal_access_token.rb index b69626bc17678f..eeaed8173fa53b 100644 --- a/ee/app/models/ee/personal_access_token.rb +++ b/ee/app/models/ee/personal_access_token.rb @@ -9,7 +9,10 @@ module PersonalAccessToken extend ActiveSupport::Concern prepended do + include ::Gitlab::Utils::StrongMemoize + scope :with_expires_at_after, -> (max_lifetime) { where(["revoked = false AND expires_at > ?", max_lifetime]) } + scope :invalid_lifetime, -> (max_lifetime) { where(["revoked = false AND expires_at > ?", max_lifetime]) } scope :with_no_expires_at, -> { where(revoked: false, expires_at: nil) } with_options if: :max_personal_access_token_lifetime_enabled? do @@ -21,11 +24,13 @@ module PersonalAccessToken private def max_expiration_date - @max_expiration_date ||= ::Gitlab::CurrentSettings.max_personal_access_token_lifetime_from_now + strong_memoize(:max_expiration_date) do + ::Gitlab::CurrentSettings.max_personal_access_token_lifetime_from_now + end end def max_personal_access_token_lifetime_enabled? - max_expiration_date.present? && License.feature_available?(:personal_access_token_expiration_policy) + max_expiration_date && License.feature_available?(:personal_access_token_expiration_policy) end def expires_at_before_max_lifetime -- GitLab From 5a8283683e25b6225bed3ef480aa5f0bcec9ddd6 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Mon, 11 Nov 2019 16:43:32 +0100 Subject: [PATCH 10/16] Change Policy to revoke PAT and notify users about it Instead of changing the expiration time, we are revoking the tokens to avoid confusion and security. Include the necessary changes to use profile mailer and the templates as well --- app/mailers/emails/profile.rb | 2 + ...al_access_tokens_user_id_and_expires_at.rb | 18 +++++ db/schema.rb | 1 + ee/app/mailers/ee/emails/profile.rb | 19 +++++ ee/app/models/ee/personal_access_token.rb | 19 ++++- ee/app/models/ee/user.rb | 4 + .../revoke_invalid_tokens.rb | 45 +++++++++++ .../update_lifetime_service.rb | 2 +- ...ked_personal_access_tokens_email.html.haml | 7 ++ ...oked_personal_access_tokens_email.text.erb | 5 ++ .../personal_access_token_policy_worker.rb | 10 +-- ee/spec/mailers/ee/emails/profile_spec.rb | 35 +++++++++ .../models/ee/personal_access_token_spec.rb | 77 ++++++++++++++++--- ee/spec/models/user_spec.rb | 13 ++++ .../revoke_invalid_tokens_spec.rb | 69 +++++++++++++++++ .../update_lifetime_service_spec.rb | 30 +++++--- ...ersonal_access_token_policy_worker_spec.rb | 34 ++++++-- 17 files changed, 351 insertions(+), 39 deletions(-) create mode 100644 db/migrate/20191112105448_add_index_on_personal_access_tokens_user_id_and_expires_at.rb create mode 100644 ee/app/mailers/ee/emails/profile.rb create mode 100644 ee/app/services/personal_access_tokens/revoke_invalid_tokens.rb create mode 100644 ee/app/views/notify/policy_revoked_personal_access_tokens_email.html.haml create mode 100644 ee/app/views/notify/policy_revoked_personal_access_tokens_email.text.erb create mode 100644 ee/spec/mailers/ee/emails/profile_spec.rb create mode 100644 ee/spec/services/personal_access_tokens/revoke_invalid_tokens_spec.rb diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index 2ea1aea1f5114f..c368e6c8ac7fd5 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -34,3 +34,5 @@ def new_gpg_key_email(gpg_key_id) # rubocop: enable CodeReuse/ActiveRecord end end + +Emails::Profile.prepend_if_ee('EE::Emails::Profile') diff --git a/db/migrate/20191112105448_add_index_on_personal_access_tokens_user_id_and_expires_at.rb b/db/migrate/20191112105448_add_index_on_personal_access_tokens_user_id_and_expires_at.rb new file mode 100644 index 00000000000000..1c1dc31ff231c8 --- /dev/null +++ b/db/migrate/20191112105448_add_index_on_personal_access_tokens_user_id_and_expires_at.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class AddIndexOnPersonalAccessTokensUserIdAndExpiresAt < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + INDEX_NAME = 'index_pat_on_user_id_and_expires_at' + + disable_ddl_transaction! + + def up + add_concurrent_index :personal_access_tokens, [:user_id, :expires_at], name: INDEX_NAME, using: :btree + end + + def down + remove_concurrent_index_by_name :personal_access_tokens, INDEX_NAME + end +end diff --git a/db/schema.rb b/db/schema.rb index 6d6a0d83042662..e63db981b29809 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2909,6 +2909,7 @@ t.boolean "impersonation", default: false, null: false t.string "token_digest" t.index ["token_digest"], name: "index_personal_access_tokens_on_token_digest", unique: true + t.index ["user_id", "expires_at"], name: "index_pat_on_user_id_and_expires_at" t.index ["user_id"], name: "index_personal_access_tokens_on_user_id" end diff --git a/ee/app/mailers/ee/emails/profile.rb b/ee/app/mailers/ee/emails/profile.rb new file mode 100644 index 00000000000000..1354a4cfd339b0 --- /dev/null +++ b/ee/app/mailers/ee/emails/profile.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module EE + module Emails + module Profile + def policy_revoked_personal_access_tokens_email(user, revoked_token_names) + return unless user && revoked_token_names + + @user = user + @revoked_token_names = revoked_token_names + @target_url = profile_personal_access_tokens_url + + ::Gitlab::I18n.with_locale(@user.preferred_language) do + mail(to: user.notification_email, subject: subject(_("One or more of you personal access tokens were revoked"))) + end + end + end + end +end diff --git a/ee/app/models/ee/personal_access_token.rb b/ee/app/models/ee/personal_access_token.rb index eeaed8173fa53b..7e3a159e59ed13 100644 --- a/ee/app/models/ee/personal_access_token.rb +++ b/ee/app/models/ee/personal_access_token.rb @@ -10,10 +10,10 @@ module PersonalAccessToken prepended do include ::Gitlab::Utils::StrongMemoize + include FromUnion - scope :with_expires_at_after, -> (max_lifetime) { where(["revoked = false AND expires_at > ?", max_lifetime]) } - scope :invalid_lifetime, -> (max_lifetime) { where(["revoked = false AND expires_at > ?", max_lifetime]) } scope :with_no_expires_at, -> { where(revoked: false, expires_at: nil) } + scope :with_expires_at_after, ->(max_lifetime) { where(revoked: false).where('expires_at > ?', max_lifetime) } with_options if: :max_personal_access_token_lifetime_enabled? do validates :expires_at, presence: true @@ -21,6 +21,21 @@ module PersonalAccessToken end end + class_methods do + def pluck_names + pluck(:name) + end + + def with_invalid_expires_at(max_lifetime, limit = 1_000) + from_union( + [ + with_no_expires_at.limit(limit), + with_expires_at_after(max_lifetime).limit(limit) + ] + ) + end + end + private def max_expiration_date diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index a4aa40191720b6..acb03ad4c061a5 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -73,6 +73,10 @@ module User scope :bots, -> { where.not(bot_type: nil) } scope :humans, -> { where(bot_type: nil) } + scope :with_invalid_expires_at_tokens, ->(expiration_date) do + where(id: ::PersonalAccessToken.with_invalid_expires_at(expiration_date).select(:user_id)) + end + accepts_nested_attributes_for :namespace enum roadmap_layout: { weeks: 1, months: 4, quarters: 12 } diff --git a/ee/app/services/personal_access_tokens/revoke_invalid_tokens.rb b/ee/app/services/personal_access_tokens/revoke_invalid_tokens.rb new file mode 100644 index 00000000000000..5ae0a9e5ab42b5 --- /dev/null +++ b/ee/app/services/personal_access_tokens/revoke_invalid_tokens.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module PersonalAccessTokens + class RevokeInvalidTokens + def initialize(user, expiration_date) + @user = user + @expiration_date = expiration_date + end + + def execute + return unless user && expiration_date && affected_tokens.any? + + notify_user + + revoke_tokens! + end + + private + + attr_reader :user, :expiration_date + + def notify_user + return unless user.can?(:receive_notifications) + + mailer.policy_revoked_personal_access_tokens_email(user, affected_tokens.pluck_names).deliver_later + end + + def mailer + Notify + end + + def affected_tokens + @affected_tokens ||= user.personal_access_tokens.with_invalid_expires_at(expiration_date) + end + + def revoke_tokens! + personal_access_tokens.with_no_expires_at.update_all(revoked: true) + personal_access_tokens.with_expires_at_after(expiration_date).update_all(revoked: true) + end + + def personal_access_tokens + @personal_access_tokens ||= user.personal_access_tokens + end + end +end diff --git a/ee/app/services/personal_access_tokens/update_lifetime_service.rb b/ee/app/services/personal_access_tokens/update_lifetime_service.rb index 0a8960716fc8cd..e993a8f3d7e7fe 100644 --- a/ee/app/services/personal_access_tokens/update_lifetime_service.rb +++ b/ee/app/services/personal_access_tokens/update_lifetime_service.rb @@ -8,7 +8,7 @@ class UpdateLifetimeService def execute try_obtain_lease do - ::PersonalAccessTokenPolicyWorker.perform_in(3.hours.to_i) + ::PersonalAccessTokenPolicyWorker.perform_in(DEFAULT_LEASE_TIMEOUT) end end diff --git a/ee/app/views/notify/policy_revoked_personal_access_tokens_email.html.haml b/ee/app/views/notify/policy_revoked_personal_access_tokens_email.html.haml new file mode 100644 index 00000000000000..d2e4ea615473ad --- /dev/null +++ b/ee/app/views/notify/policy_revoked_personal_access_tokens_email.html.haml @@ -0,0 +1,7 @@ +%p + = _('Hi %{username}!') % { username: sanitize_name(@user.name) } +%p + = n_('The following personal access token: %{token_names} was revoked, because a new policy to expire personal access tokens were set.', 'The following personal access tokens: %{token_names} were revoked, because a new policy to expire personal access tokens were set.', @revoked_token_names.size) % { token_names: @revoked_token_names.to_sentence } +%p + - pat_link_start = ''.html_safe % { url: @target_url } + = _('You can create new ones at your %{pat_link_start}Personal Access Tokens%{pat_link_end} settings').html_safe % { pat_link_start: pat_link_start, pat_link_end: ''.html_safe } diff --git a/ee/app/views/notify/policy_revoked_personal_access_tokens_email.text.erb b/ee/app/views/notify/policy_revoked_personal_access_tokens_email.text.erb new file mode 100644 index 00000000000000..68ff25700e96b4 --- /dev/null +++ b/ee/app/views/notify/policy_revoked_personal_access_tokens_email.text.erb @@ -0,0 +1,5 @@ +<%= _('Hi %{username}!') % { username: sanitize_name(@user.name) } %> + +<%= n_('The following personal access token: %{token_names} was revoked, because a new policy to expire personal access tokens were set.', 'The following personal access tokens: %{token_names} were revoked, because a new policy to expire personal access tokens were set.', @revoked_token_names.size) % { token_names: @revoked_token_names.to_sentence } %> + +<%= _('You can create new ones at your Personal Access Tokens settings %{pat_link}') % { pat_link: @target_url } %> \ No newline at end of file diff --git a/ee/app/workers/personal_access_token_policy_worker.rb b/ee/app/workers/personal_access_token_policy_worker.rb index 3294952e043e54..d634f8633dfb46 100644 --- a/ee/app/workers/personal_access_token_policy_worker.rb +++ b/ee/app/workers/personal_access_token_policy_worker.rb @@ -6,12 +6,12 @@ class PersonalAccessTokenPolicyWorker feature_category :authentication_and_authorization def perform - expire_at = ::Gitlab::CurrentSettings.max_personal_access_token_lifetime_from_now + expiration_date = ::Gitlab::CurrentSettings.max_personal_access_token_lifetime_from_now - return unless expire_at + return unless expiration_date - PersonalAccessToken.invalid_lifetime(expire_at).in_batches(of: 500).update_all(expires_at: expire_at) - - PersonalAccessToken.with_no_expires_at.in_batches(of: 500).update_all(expires_at: expire_at) + User.with_invalid_expires_at_tokens(expiration_date).find_each do |user| + PersonalAccessTokens::RevokeInvalidTokens.new(user, expiration_date).execute + end end end diff --git a/ee/spec/mailers/ee/emails/profile_spec.rb b/ee/spec/mailers/ee/emails/profile_spec.rb new file mode 100644 index 00000000000000..48c4bbf3551537 --- /dev/null +++ b/ee/spec/mailers/ee/emails/profile_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'email_spec' + +describe EE::Emails::Profile do + include EmailSpec::Matchers + + describe '#policy_revoked_personal_access_tokens_email' do + let_it_be(:user) { create(:user) } + let(:token_names) { %w(name1 name2) } + + subject { Notify.policy_revoked_personal_access_tokens_email(user, token_names) } + + it 'is sent to the user' do + is_expected.to deliver_to user.email + end + + it 'has the correct subject' do + is_expected.to have_subject /^One or more of you personal access tokens were revoked$/i + end + + it 'mentions the access tokens were revoke' do + is_expected.to have_body_text /The following personal access tokens: name1 and name2 were revoked/ + end + + it 'includes a link to personal access tokens page' do + is_expected.to have_body_text /#{profile_personal_access_tokens_path}/ + end + + it 'includes the email reason' do + is_expected.to have_body_text /You're receiving this email because of your account on localhost/ + end + 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 aa608c3bb65db2..b40031beb8d601 100644 --- a/ee/spec/models/ee/personal_access_token_spec.rb +++ b/ee/spec/models/ee/personal_access_token_spec.rb @@ -3,29 +3,44 @@ require 'spec_helper' describe PersonalAccessToken do - subject { described_class } - describe 'scopes' do let_it_be(:expired_token) { create(:personal_access_token, expires_at: 1.day.ago) } let_it_be(:valid_token) { create(:personal_access_token, expires_at: 1.day.from_now) } + let!(:pat) { create(:personal_access_token, expires_at: expiration_date) } describe 'with_expires_at_after' do - context 'token with an expire at higher than the lifetime' do - let_it_be(:pat) { create(:personal_access_token, expires_at: 3.days.from_now) } + subject { described_class.with_expires_at_after(2.days.from_now) } - it 'includes the tokens with higher than the lifetime expires_at value' do - expect(described_class.with_expires_at_after(2.days.from_now)).to contain_exactly(pat) - end + let(:expiration_date) { 3.days.from_now } + + it 'includes the tokens with higher than the lifetime expires_at value' do + expect(subject).to contain_exactly(pat) + end + + it "doesn't contain expired tokens" do + expect(subject).not_to include(expired_token) + end + + it "doesn't contain tokens within the expiration time" do + expect(subject).not_to include(valid_token) end end describe 'with_no_expires_at' do - context 'token with nil expires_at' do - let_it_be(:pat) { create(:personal_access_token, expires_at: nil) } + subject { described_class.with_expires_at_after(2.days.from_now) } - it 'includes the tokens with nil expires_at' do - expect(described_class.with_no_expires_at).to contain_exactly(pat) - end + let(:expiration_date) { nil } + + it 'includes the tokens with nil expires_at' do + expect(described_class.with_no_expires_at).to contain_exactly(pat) + end + + it "doesn't contain expired tokens" do + expect(subject).not_to include(expired_token) + end + + it "doesn't contain tokens within the expiration time" do + expect(subject).not_to include(valid_token) end end end @@ -86,4 +101,42 @@ end end end + + describe '.pluck_names' do + it 'returns the names of the tokens' do + pat1 = create(:personal_access_token) + pat2 = create(:personal_access_token) + + expect(described_class.pluck_names).to contain_exactly(pat1.name, pat2.name) + end + end + + describe '.with_invalid_expires_at' do + subject { described_class.with_invalid_expires_at(2.days.from_now) } + + it 'includes the tokens with invalid expires_at' do + pat_with_no_expires_at = create(:personal_access_token, expires_at: nil) + pat_with_longer_expires_at = create(:personal_access_token, expires_at: 3.days.from_now) + + expect(subject).to contain_exactly(pat_with_no_expires_at, pat_with_longer_expires_at) + end + + it "doesn't include valid tokens" do + valid_token = create(:personal_access_token, expires_at: 1.day.from_now) + + expect(subject).not_to include(valid_token) + end + + it "doesn't include revoked tokens" do + revoked_token = create(:personal_access_token, revoked: true) + + expect(subject).not_to include(revoked_token) + end + + it "doesn't include expired tokens" do + expired_token = create(:personal_access_token, expires_at: 1.day.ago) + + expect(subject).not_to include(expired_token) + end + end end diff --git a/ee/spec/models/user_spec.rb b/ee/spec/models/user_spec.rb index 57cfe05b824f50..b14be6ebcc44f9 100644 --- a/ee/spec/models/user_spec.rb +++ b/ee/spec/models/user_spec.rb @@ -83,6 +83,19 @@ expect(described_class.bots).to match_array([bot]) end end + + describe 'with_invalid_expires_at_tokens' do + it 'only includes users with invalid tokens' do + valid_pat = create(:personal_access_token, expires_at: 7.days.from_now) + invalid_pat1 = create(:personal_access_token, expires_at: nil) + invalid_pat2 = create(:personal_access_token, expires_at: 20.days.from_now) + + users_with_invalid_tokens = described_class.with_invalid_expires_at_tokens(15.days.from_now) + + expect(users_with_invalid_tokens).to contain_exactly(invalid_pat1.user, invalid_pat2.user) + expect(users_with_invalid_tokens).not_to include valid_pat.user + end + end end describe '.find_by_smartcard_identity' do 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 new file mode 100644 index 00000000000000..f77c6b4c90c918 --- /dev/null +++ b/ee/spec/services/personal_access_tokens/revoke_invalid_tokens_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe PersonalAccessTokens::RevokeInvalidTokens do + subject(:service) { described_class.new(user, expiration_date) } + + describe '#execute' do + let(:expiration_date) { 10.days.from_now } + let_it_be(:user) { create(:user) } + let_it_be(:pat) { create(:personal_access_token, expires_at: 5.days.from_now, user: user) } + 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) } + + 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 + 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 + + 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 + end + end + end + + 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 "doesn't revoke user's tokens" do + expect { service.execute }.not_to change { pat.reload.revoked } + end + end + + 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 "doesn't revoke user's tokens" do + expect { service.execute }.not_to change { pat.reload.revoked } + end + end + end +end diff --git a/ee/spec/services/personal_access_tokens/update_lifetime_service_spec.rb b/ee/spec/services/personal_access_tokens/update_lifetime_service_spec.rb index 742edfb9d30d3b..47ebfb2089d152 100644 --- a/ee/spec/services/personal_access_tokens/update_lifetime_service_spec.rb +++ b/ee/spec/services/personal_access_tokens/update_lifetime_service_spec.rb @@ -3,22 +3,28 @@ require 'spec_helper' describe PersonalAccessTokens::UpdateLifetimeService do - subject { described_class.new } - describe '#execute', :clean_gitlab_redis_shared_state do - context 'with personal_access_token_expiration_policy licensed' do - it 'enqueues a policy worker job' do - expect { subject.execute }.to change { PersonalAccessTokenPolicyWorker.jobs.count }.from(0).to(1) + include ExclusiveLeaseHelpers + + let(:lease_key) { 'personal_access_tokens/update_lifetime_service' } + + context 'when we can obtain the lease' do + it 'schedules the worker' do + stub_exclusive_lease(lease_key, timeout: described_class::DEFAULT_LEASE_TIMEOUT) + + expect(PersonalAccessTokenPolicyWorker).to receive(:perform_in).once + + subject.execute end + end + + context "when we can't obtain the lease" do + it 'does not schedule the worker' do + stub_exclusive_lease_taken(lease_key, timeout: described_class::DEFAULT_LEASE_TIMEOUT) - context 'before the lease is released job is executed' do - before do - subject.execute - end + expect(PersonalAccessTokenPolicyWorker).not_to receive(:perform_in) - it "doesn't enqueue a second policy job" do - expect { subject.execute }.not_to change { PersonalAccessTokenPolicyWorker.jobs.count } - end + subject.execute end end end diff --git a/ee/spec/workers/personal_access_token_policy_worker_spec.rb b/ee/spec/workers/personal_access_token_policy_worker_spec.rb index a150e00886bd26..79aba6ec6f481c 100644 --- a/ee/spec/workers/personal_access_token_policy_worker_spec.rb +++ b/ee/spec/workers/personal_access_token_policy_worker_spec.rb @@ -3,30 +3,50 @@ require 'spec_helper' RSpec.describe PersonalAccessTokenPolicyWorker, type: :worker do - subject(:worker) { described_class.new } - describe '#perform' do let(:limit) { 7 } - let(:timelimit) { limit.days.from_now.to_date } + let!(:pat) { create(:personal_access_token, expires_at: expire_at) } before do stub_application_setting(max_personal_access_token_lifetime: limit) end context "when a token doesn't have an expiration time" do - let!(:pat) { create(:personal_access_token, expires_at: nil) } + let(:expire_at) { nil } it 'enforces the policy on tokens' do - expect { worker.perform }.to change { PersonalAccessToken.find(pat.id).expires_at }.from(nil).to(timelimit) + expect { subject.perform }.to change { pat.reload.revoked }.from(false).to(true) end end context 'when a token expires after the given time' do let(:expire_at) { 8.days.from_now.to_date } - let!(:pat) { create(:personal_access_token, expires_at: expire_at) } it 'enforces the policy on tokens' do - expect { worker.perform }.to change { PersonalAccessToken.find(pat.id).expires_at }.from(expire_at).to(timelimit) + expect { subject.perform }.to change { pat.reload.revoked }.from(false).to(true) + end + end + + context 'when a token is valid' do + let(:expire_at) { 5.days.from_now.to_date } + + it "doesn't revoked valid tokens" do + expect { subject.perform }.not_to change { pat.reload.revoked } + end + end + + context 'when limit is nil' do + let(:limit) { nil } + let(:expire_at) { 1.day.from_now } + + it "doesn't revoked valid tokens" do + expect { subject.perform }.not_to change { pat.reload.revoked } + end + + it "doesn't call the revoke invalid service" do + expect(PersonalAccessTokens::RevokeInvalidTokens).not_to receive(:new) + + subject.perform end end end -- GitLab From 64d67d0c480a3c688833ec0802b81aff405a69cc Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 15:59:57 +0100 Subject: [PATCH 11/16] Add locale/gitlab.pot changes --- locale/gitlab.pot | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 5b00a1629d53f9..8e459266e7c6a6 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -9107,6 +9107,9 @@ msgstr "" msgid "Helps reduce request volume for protected paths" msgstr "" +msgid "Hi %{username}!" +msgstr "" + msgid "Hide archived projects" msgstr "" @@ -10253,6 +10256,9 @@ msgstr "" msgid "Leave Admin Mode" msgstr "" +msgid "Leave blank for no limit. Once set, existing personal access tokens may be revoked" +msgstr "" + msgid "Leave edit mode? All unsaved changes will be lost." msgstr "" @@ -10743,6 +10749,9 @@ msgstr "" msgid "Max seats used" msgstr "" +msgid "Maximum allowable lifetime for personal access token (days)" +msgstr "" + msgid "Maximum artifacts size (MB)" msgstr "" @@ -10761,6 +10770,9 @@ msgstr "" msgid "Maximum job timeout has a value which could not be accepted" msgstr "" +msgid "Maximum lifetime allowable for Personal Access Tokens is active, your expire date must be set before %{maximum_allowable_date}." +msgstr "" + msgid "Maximum number of comments exceeded" msgstr "" @@ -11964,6 +11976,9 @@ msgstr[1] "" msgid "One or more groups that you don't have access to." msgstr "" +msgid "One or more of you personal access tokens were revoked" +msgstr "" + msgid "One or more of your Bitbucket projects cannot be imported into GitLab directly because they use Subversion or Mercurial for version control, rather than Git." msgstr "" @@ -17415,6 +17430,11 @@ msgstr "" msgid "The following items will be exported:" msgstr "" +msgid "The following personal access token: %{token_names} was revoked, because a new policy to expire personal access tokens were set." +msgid_plural "The following personal access tokens: %{token_names} were revoked, because a new policy to expire personal access tokens were set." +msgstr[0] "" +msgstr[1] "" + msgid "The fork relationship has been removed." msgstr "" @@ -20084,6 +20104,12 @@ msgstr "" msgid "You can create files directly in GitLab using one of the following options." msgstr "" +msgid "You can create new ones at your %{pat_link_start}Personal Access Tokens%{pat_link_end} settings" +msgstr "" + +msgid "You can create new ones at your Personal Access Tokens settings %{pat_link}" +msgstr "" + msgid "You can easily contribute to them by requesting to join these groups." msgstr "" -- GitLab From a772771ecc1f8976ecb16c4fd2381d1463901c2e Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Mon, 11 Nov 2019 21:59:25 +0100 Subject: [PATCH 12/16] Update docs with PAT policy --- .../settings/account_and_limit_settings.md | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/doc/user/admin_area/settings/account_and_limit_settings.md b/doc/user/admin_area/settings/account_and_limit_settings.md index e443127a8a0435..fa002255d2b96e 100644 --- a/doc/user/admin_area/settings/account_and_limit_settings.md +++ b/doc/user/admin_area/settings/account_and_limit_settings.md @@ -84,3 +84,35 @@ add the line below to `/etc/gitlab/gitlab.rb` before increasing the max attachme ``` nginx['client_max_body_size'] = "200m" ``` + +## Limiting lifetime of personal access tokens **(ULTIMATE ONLY)** + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/3649) in [GitLab Ultimate](https://about.gitlab.com/pricing/) 12.5. + +Users can optionally specify an expiration date for +[personal access tokens](../../profile/personal_access_tokens.md). +This expiration date is not a requirement, and can be set to any arbitrary date. + +Since personal access tokens are the only token needed for programmatic access to GitLab, +organizations with security requirements may want to enforce more protection to require +regular rotation of these tokens. + +### Setting a limit + +Only a GitLab administrator can set a limit. Leaving it empty means +there are no restrictions. + +To set a limit on how long personal access tokens are valid: + +1. Navigate to **Admin Area > Settings > General**. +1. Expand the **Account and limit** section. +1. Fill in the **Maximun allowable lifetime for personal access tokens (days)** field. +1. Click **Save changes**. + +Once a lifetime for personal access tokens is set, GitLab will: + +- Apply the lifetime for new personal access tokens, and require users to set an expiration date + and a date no later than the allowed lifetime. +- After three hours, revoke old tokens with no expiration date or with a lifetime longer than the + allowed lifetime. Three hours is given to allow administrators to change the allowed lifetime, + or remove it, before revocation takes place. -- GitLab From efe5ee9f26323606532fd34da7c68f5e1a4fbd5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Arcila=20Valenzuela?= Date: Wed, 4 Dec 2019 09:58:24 +0000 Subject: [PATCH 13/16] Apply suggestion to doc/user/admin_area/settings/account_and_limit_settings.md --- doc/user/admin_area/settings/account_and_limit_settings.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/admin_area/settings/account_and_limit_settings.md b/doc/user/admin_area/settings/account_and_limit_settings.md index fa002255d2b96e..9d82b3b4292030 100644 --- a/doc/user/admin_area/settings/account_and_limit_settings.md +++ b/doc/user/admin_area/settings/account_and_limit_settings.md @@ -87,7 +87,7 @@ nginx['client_max_body_size'] = "200m" ## Limiting lifetime of personal access tokens **(ULTIMATE ONLY)** -> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/3649) in [GitLab Ultimate](https://about.gitlab.com/pricing/) 12.5. +> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/3649) in [GitLab Ultimate](https://about.gitlab.com/pricing/) 12.6. Users can optionally specify an expiration date for [personal access tokens](../../profile/personal_access_tokens.md). -- GitLab From 58dbc1ab12ff13b11fabb67a2239c8aa5c8613c5 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Wed, 4 Dec 2019 16:07:42 +0100 Subject: [PATCH 14/16] Add personal access token helpers To avoid adding code to the views and test a bit the code around it --- .../helpers/personal_access_tokens_helper.rb | 11 ++++ ...l_access_token_expiration_policy.html.haml | 2 +- ...x_personal_access_token_lifetime.html.haml | 2 +- .../personal_access_tokens_helper_spec.rb | 55 +++++++++++++++++++ 4 files changed, 68 insertions(+), 2 deletions(-) create mode 100644 ee/app/helpers/personal_access_tokens_helper.rb create mode 100644 ee/spec/helpers/personal_access_tokens_helper_spec.rb diff --git a/ee/app/helpers/personal_access_tokens_helper.rb b/ee/app/helpers/personal_access_tokens_helper.rb new file mode 100644 index 00000000000000..949d7c06aee3a1 --- /dev/null +++ b/ee/app/helpers/personal_access_tokens_helper.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module PersonalAccessTokensHelper + def personal_access_token_expiration_policy_licensed? + License.feature_available?(:personal_access_token_expiration_policy) + end + + def personal_access_token_expiration_policy_enabled? + Gitlab::CurrentSettings.max_personal_access_token_lifetime && personal_access_token_expiration_policy_licensed? + end +end diff --git a/ee/app/views/admin/application_settings/_personal_access_token_expiration_policy.html.haml b/ee/app/views/admin/application_settings/_personal_access_token_expiration_policy.html.haml index 0664e1b4dfb230..62d3611b2f24d7 100644 --- a/ee/app/views/admin/application_settings/_personal_access_token_expiration_policy.html.haml +++ b/ee/app/views/admin/application_settings/_personal_access_token_expiration_policy.html.haml @@ -1,4 +1,4 @@ -- return unless License.feature_available?(:personal_access_token_expiration_policy) +- return unless personal_access_token_expiration_policy_licensed? - form = local_assigns.fetch(:form) .form-group diff --git a/ee/app/views/personal_access_tokens/_callout_max_personal_access_token_lifetime.html.haml b/ee/app/views/personal_access_tokens/_callout_max_personal_access_token_lifetime.html.haml index 850e22fcd7cffc..d087f299b908d1 100644 --- a/ee/app/views/personal_access_tokens/_callout_max_personal_access_token_lifetime.html.haml +++ b/ee/app/views/personal_access_tokens/_callout_max_personal_access_token_lifetime.html.haml @@ -1,4 +1,4 @@ -- return unless Gitlab::CurrentSettings.max_personal_access_token_lifetime && License.feature_available(:personal_access_token_expiration_policy) +- return unless personal_access_token_expiration_policy_enabled? .bs-callout.bs-callout-danger = _('Maximum lifetime allowable for Personal Access Tokens is active, your expire date must be set before %{maximum_allowable_date}.') % { maximum_allowable_date: ::Gitlab::CurrentSettings.max_personal_access_token_lifetime_from_now } diff --git a/ee/spec/helpers/personal_access_tokens_helper_spec.rb b/ee/spec/helpers/personal_access_tokens_helper_spec.rb new file mode 100644 index 00000000000000..b3a2d78112b48e --- /dev/null +++ b/ee/spec/helpers/personal_access_tokens_helper_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe PersonalAccessTokensHelper do + describe '#personal_access_token_expiration_policy_licensed?' do + subject { helper.personal_access_token_expiration_policy_licensed? } + + context 'when is not licensed' do + before do + stub_licensed_features(personal_access_token_expiration_policy: false) + end + + it { is_expected.to be_falsey } + end + + context 'when is licensed' do + before do + stub_licensed_features(personal_access_token_expiration_policy: true) + end + + it { is_expected.to be_truthy } + end + end + + describe '#personal_access_token_expiration_policy_enabled?' do + subject { helper.personal_access_token_expiration_policy_enabled? } + + context 'when is licensed and used' do + before do + stub_licensed_features(personal_access_token_expiration_policy: true) + stub_application_setting(max_personal_access_token_lifetime: 1) + end + + it { is_expected.to be_truthy } + end + + context 'when is not licensed' do + before do + stub_licensed_features(personal_access_token_expiration_policy: false) + end + + it { is_expected.to be_falsey } + end + + context 'when is licensed but not used' do + before do + stub_licensed_features(personal_access_token_expiration_policy: true) + stub_application_setting(max_personal_access_token_lifetime: nil) + end + + it { is_expected.to be_falsey } + end + end +end -- GitLab From 318f2c3bdf6e22d35f93cb7c52e8236799afc692 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Wed, 4 Dec 2019 16:19:49 +0100 Subject: [PATCH 15/16] Move PAT worker to its own directory This will help once we start adding more workers and also mimics the same structure that we have for services --- config/sidekiq_queues.yml | 2 +- .../update_lifetime_service.rb | 5 +++-- ee/app/workers/all_queues.yml | 3 ++- .../personal_access_token_policy_worker.rb | 17 ---------------- .../personal_access_tokens/policy_worker.rb | 20 +++++++++++++++++++ .../update_lifetime_service_spec.rb | 4 ++-- .../policy_worker_spec.rb} | 2 +- 7 files changed, 29 insertions(+), 24 deletions(-) delete mode 100644 ee/app/workers/personal_access_token_policy_worker.rb create mode 100644 ee/app/workers/personal_access_tokens/policy_worker.rb rename ee/spec/workers/{personal_access_token_policy_worker_spec.rb => personal_access_tokens/policy_worker_spec.rb} (95%) diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 35a02f85257740..063a3cc2b5b484 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -123,5 +123,5 @@ - [refresh_license_compliance_checks, 2] - [design_management_new_version, 1] - [epics, 2] - - [personal_access_token_policy, 1] + - [personal_access_tokens, 1] diff --git a/ee/app/services/personal_access_tokens/update_lifetime_service.rb b/ee/app/services/personal_access_tokens/update_lifetime_service.rb index e993a8f3d7e7fe..3a9e1e9f72214d 100644 --- a/ee/app/services/personal_access_tokens/update_lifetime_service.rb +++ b/ee/app/services/personal_access_tokens/update_lifetime_service.rb @@ -8,7 +8,7 @@ class UpdateLifetimeService def execute try_obtain_lease do - ::PersonalAccessTokenPolicyWorker.perform_in(DEFAULT_LEASE_TIMEOUT) + ::PersonalAccessTokens::PolicyWorker.perform_in(DEFAULT_LEASE_TIMEOUT) end end @@ -22,7 +22,8 @@ def lease_timeout # Used by ExclusiveLeaseGuard # Overriding value as we never release the lease # before the timeout in order to prevent multiple - # PersonalAccessTokenPolicyWorker to start in a short span of time + # PersonalAccessTokens::PolicyWorker to start in + # a short span of time def lease_release? false end diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index c3be7544f7b62d..5594f1fc2dec4f 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -58,6 +58,8 @@ - jira_connect:jira_connect_sync_branch - jira_connect:jira_connect_sync_merge_request +- personal_access_tokens:personal_access_tokens_policy + - admin_emails - create_github_webhook - design_management_new_version @@ -69,7 +71,6 @@ - export_csv - ldap_group_sync - new_epic -- personal_access_token_policy - project_import_schedule - project_update_repository_storage - rebase diff --git a/ee/app/workers/personal_access_token_policy_worker.rb b/ee/app/workers/personal_access_token_policy_worker.rb deleted file mode 100644 index d634f8633dfb46..00000000000000 --- a/ee/app/workers/personal_access_token_policy_worker.rb +++ /dev/null @@ -1,17 +0,0 @@ -# frozen_string_literal: true - -class PersonalAccessTokenPolicyWorker - include ApplicationWorker - - feature_category :authentication_and_authorization - - def perform - expiration_date = ::Gitlab::CurrentSettings.max_personal_access_token_lifetime_from_now - - return unless expiration_date - - User.with_invalid_expires_at_tokens(expiration_date).find_each do |user| - PersonalAccessTokens::RevokeInvalidTokens.new(user, expiration_date).execute - end - end -end diff --git a/ee/app/workers/personal_access_tokens/policy_worker.rb b/ee/app/workers/personal_access_tokens/policy_worker.rb new file mode 100644 index 00000000000000..69390e3eda8356 --- /dev/null +++ b/ee/app/workers/personal_access_tokens/policy_worker.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module PersonalAccessTokens + class PolicyWorker + include ApplicationWorker + + queue_namespace :personal_access_tokens + feature_category :authentication_and_authorization + + def perform + expiration_date = ::Gitlab::CurrentSettings.max_personal_access_token_lifetime_from_now + + return unless expiration_date + + User.with_invalid_expires_at_tokens(expiration_date).find_each do |user| + PersonalAccessTokens::RevokeInvalidTokens.new(user, expiration_date).execute + end + end + end +end diff --git a/ee/spec/services/personal_access_tokens/update_lifetime_service_spec.rb b/ee/spec/services/personal_access_tokens/update_lifetime_service_spec.rb index 47ebfb2089d152..e8b7a0fd40770b 100644 --- a/ee/spec/services/personal_access_tokens/update_lifetime_service_spec.rb +++ b/ee/spec/services/personal_access_tokens/update_lifetime_service_spec.rb @@ -12,7 +12,7 @@ it 'schedules the worker' do stub_exclusive_lease(lease_key, timeout: described_class::DEFAULT_LEASE_TIMEOUT) - expect(PersonalAccessTokenPolicyWorker).to receive(:perform_in).once + expect(::PersonalAccessTokens::PolicyWorker).to receive(:perform_in).once subject.execute end @@ -22,7 +22,7 @@ it 'does not schedule the worker' do stub_exclusive_lease_taken(lease_key, timeout: described_class::DEFAULT_LEASE_TIMEOUT) - expect(PersonalAccessTokenPolicyWorker).not_to receive(:perform_in) + expect(::PersonalAccessTokens::PolicyWorker).not_to receive(:perform_in) subject.execute end diff --git a/ee/spec/workers/personal_access_token_policy_worker_spec.rb b/ee/spec/workers/personal_access_tokens/policy_worker_spec.rb similarity index 95% rename from ee/spec/workers/personal_access_token_policy_worker_spec.rb rename to ee/spec/workers/personal_access_tokens/policy_worker_spec.rb index 79aba6ec6f481c..05b8bba40d5580 100644 --- a/ee/spec/workers/personal_access_token_policy_worker_spec.rb +++ b/ee/spec/workers/personal_access_tokens/policy_worker_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe PersonalAccessTokenPolicyWorker, type: :worker do +RSpec.describe PersonalAccessTokens::PolicyWorker, type: :worker do describe '#perform' do let(:limit) { 7 } let!(:pat) { create(:personal_access_token, expires_at: expire_at) } -- GitLab From 3cbcf643693db10534070c47e30ec0bc3f55eec6 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Wed, 4 Dec 2019 17:59:24 +0100 Subject: [PATCH 16/16] Add feature flag to be able to stop the emails to users --- .../revoke_invalid_tokens.rb | 7 ++++++- .../revoke_invalid_tokens_spec.rb | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) 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 5ae0a9e5ab42b5..7db9e7db3120b3 100644 --- a/ee/app/services/personal_access_tokens/revoke_invalid_tokens.rb +++ b/ee/app/services/personal_access_tokens/revoke_invalid_tokens.rb @@ -8,7 +8,8 @@ def initialize(user, expiration_date) end def execute - return unless user && expiration_date && affected_tokens.any? + return unless ::Feature.enabled?(:personal_access_token_expiration_policy, default_enabled: true) + return unless expiration_date && user_affected? notify_user @@ -19,6 +20,10 @@ def execute attr_reader :user, :expiration_date + def user_affected? + user && affected_tokens.any? + end + def notify_user return unless user.can?(:receive_notifications) 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 f77c6b4c90c918..82550951b2ea15 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 @@ -65,5 +65,20 @@ expect { service.execute }.not_to change { pat.reload.revoked } end end + + context 'when the feature flag for personal access token policy is disabled' do + before 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 "doesn't revoke user's tokens" do + expect { service.execute }.not_to change { pat.reload.revoked } + end + end end end -- GitLab