From c7abf584ecab96b610e1621285468bad4e28e598 Mon Sep 17 00:00:00 2001 From: Jessie Young Date: Thu, 11 May 2023 11:02:03 -0700 Subject: [PATCH 1/2] Set default PAT expiration to 365 days from now * 16.0 breaking change * This is to ensure that no new PATs are being created with a nil expiration. * Next step is to run a batched background migration to backfill the expiration date for all existing PATs. * Once background migration is done, we can add a validation to ensure that PATs are always created with an expiration no more than 365 days in the future. * This is being done behind a feature flag to minimize risk. * https://gitlab.com/gitlab-org/gitlab/-/issues/369123 Changelog: removed --- app/models/personal_access_token.rb | 24 ++++++ .../resource_access_tokens/create_service.rb | 10 ++- .../development/default_pat_expiration.yml | 7 ++ .../16_0/16-0-non-expiring-access-tokens.yml | 19 +++++ doc/update/removals.md | 11 +++ .../models/ee/personal_access_token_spec.rb | 84 ++++++++++++++++--- .../credentials_inventory_shared_examples.rb | 55 ++++++++---- .../_expiry_date.html.haml_spec.rb | 23 ++++- locale/gitlab.pot | 3 + .../entities/personal_access_token_spec.rb | 2 +- spec/models/personal_access_token_spec.rb | 69 ++++++++++++++- spec/requests/api/internal/base_spec.rb | 64 ++++++++++---- .../api/resource_access_tokens_spec.rb | 32 +++++-- .../access_token_entity_base_spec.rb | 2 +- .../create_service_spec.rb | 52 ++++++++++-- 15 files changed, 390 insertions(+), 67 deletions(-) create mode 100644 config/feature_flags/development/default_pat_expiration.yml create mode 100644 data/removals/16_0/16-0-non-expiring-access-tokens.yml diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index 3ebb2126f4dfbc..75afff6a2fafcf 100644 --- a/app/models/personal_access_token.rb +++ b/app/models/personal_access_token.rb @@ -15,6 +15,7 @@ class PersonalAccessToken < ApplicationRecord # PATs are 20 characters + optional configurable settings prefix (0..20) TOKEN_LENGTH_RANGE = (20..40).freeze + MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS = 365 serialize :scopes, Array # rubocop:disable Cop/ActiveRecordSerialize @@ -48,6 +49,7 @@ class PersonalAccessToken < ApplicationRecord validates :scopes, presence: true validate :validate_scopes + validate :expires_at_before_instance_max_expiry_date, on: :create def revoke! update!(revoked: true) @@ -57,6 +59,19 @@ def active? !revoked? && !expired? end + # fall back to default value until background migration has updated all + # existing PATs and we can add a validation + # https://gitlab.com/gitlab-org/gitlab/-/issues/369123 + def expires_at=(value) + datetime = if Feature.enabled?(:default_pat_expiration) + value.presence || MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now + else + value + end + + super(datetime) + end + override :simple_sorts def self.simple_sorts super.merge( @@ -108,6 +123,15 @@ def add_admin_mode_scope def prefix_from_application_current_settings self.class.token_prefix end + + def expires_at_before_instance_max_expiry_date + return unless Feature.enabled?(:default_pat_expiration) + return unless expires_at + + if expires_at > MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now + errors.add(:expires_at, _('must expire in 365 days')) + end + end end PersonalAccessToken.prepend_mod_with('PersonalAccessToken') diff --git a/app/services/resource_access_tokens/create_service.rb b/app/services/resource_access_tokens/create_service.rb index cfa43f5d9c823e..553315f08f9652 100644 --- a/app/services/resource_access_tokens/create_service.rb +++ b/app/services/resource_access_tokens/create_service.rb @@ -100,7 +100,15 @@ def default_scopes end def create_membership(resource, user, access_level) - resource.add_member(user, access_level, expires_at: params[:expires_at]) + resource.add_member(user, access_level, expires_at: default_pat_expiration) + end + + def default_pat_expiration + if Feature.enabled?(:default_pat_expiration) + params[:expires_at].presence || PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now + else + params[:expires_at] + end end def log_event(token) diff --git a/config/feature_flags/development/default_pat_expiration.yml b/config/feature_flags/development/default_pat_expiration.yml new file mode 100644 index 00000000000000..b48d6a027235af --- /dev/null +++ b/config/feature_flags/development/default_pat_expiration.yml @@ -0,0 +1,7 @@ +name: default_pat_expiration +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/120213 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/410440 +milestone: '16.0' +type: development +group: group::authentication and authorization +default_enabled: true diff --git a/data/removals/16_0/16-0-non-expiring-access-tokens.yml b/data/removals/16_0/16-0-non-expiring-access-tokens.yml new file mode 100644 index 00000000000000..5ef7df3a13172a --- /dev/null +++ b/data/removals/16_0/16-0-non-expiring-access-tokens.yml @@ -0,0 +1,19 @@ +- title: "Non-expiring access tokens no longer supported" + announcement_milestone: "15.4" # (required) The milestone when this feature was deprecated. + removal_milestone: "16.0" # (required) The milestone when this feature is being removed. + breaking_change: true # (required) Change to false if this is not a breaking change. + reporter: jessieay # (required) GitLab username of the person reporting the removal + stage: Manage # (required) String value of the stage that the feature was created in. e.g., Growth + issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/369123 + body: | # (required) Do not modify this line, instead modify the lines below. + Currently, you can create access tokens that have no expiration date. These access tokens are valid indefinitely, which presents a security risk if the access token is + divulged. Because expiring access tokens are better, from GitLab 15.4 we [populate a default expiration date](https://gitlab.com/gitlab-org/gitlab/-/issues/348660). + + In GitLab 16.0, any personal, project, or group access token that does not have an expiration date will automatically have an expiration date set at 365 days later than the current date. +# +# OPTIONAL FIELDS +# + tiers: # (optional - may be required in the future) An array of tiers that the feature is available in currently. e.g., [Free, Silver, Gold, Core, Premium, Ultimate] + documentation_url: # (optional) This is a link to the current documentation page + image_url: # (optional) This is a link to a thumbnail image depicting the feature + video_url: # (optional) Use the youtube thumbnail URL with the structure of https://img.youtube.com/vi/UNIQUEID/hqdefault.jpg diff --git a/doc/update/removals.md b/doc/update/removals.md index d0a6952745a8c2..0b6e7ccdadf6cb 100644 --- a/doc/update/removals.md +++ b/doc/update/removals.md @@ -190,6 +190,17 @@ The [**Maximum number of active pipelines per project** limit](https://docs.gitl - [**Pipelines rate limits**](https://docs.gitlab.com/ee/user/admin_area/settings/rate_limit_on_pipelines_creation.html). - [**Total number of jobs in currently active pipelines**](https://docs.gitlab.com/ee/user/admin_area/settings/continuous_integration.html#set-cicd-limits). +### Non-expiring access tokens no longer supported + +WARNING: +This is a [breaking change](https://docs.gitlab.com/ee/development/deprecation_guidelines/). +Review the details carefully before upgrading. + +Currently, you can create access tokens that have no expiration date. These access tokens are valid indefinitely, which presents a security risk if the access token is +divulged. Because expiring access tokens are better, from GitLab 15.4 we [populate a default expiration date](https://gitlab.com/gitlab-org/gitlab/-/issues/348660). + +In GitLab 16.0, any personal, project, or group access token that does not have an expiration date will automatically have an expiration date set at 365 days later than the current date. + ### Non-standard default Redis ports are no longer supported WARNING: diff --git a/ee/spec/models/ee/personal_access_token_spec.rb b/ee/spec/models/ee/personal_access_token_spec.rb index eed2e2e73bdd9c..49dd9eaf44fbd4 100644 --- a/ee/spec/models/ee/personal_access_token_spec.rb +++ b/ee/spec/models/ee/personal_access_token_spec.rb @@ -2,11 +2,16 @@ require 'spec_helper' -RSpec.describe PersonalAccessToken do +RSpec.describe PersonalAccessToken, feature_category: :system_access do 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_it_be(:long_expiry_token) { create(:personal_access_token, expires_at: '999999-12-31'.to_date) } + let_it_be(:long_expiry_token) do + create( + :personal_access_token, + expires_at: described_class::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now + ) + end let!(:pat) { create(:personal_access_token, expires_at: expiration_date) } @@ -31,7 +36,12 @@ describe 'with_no_expires_at' do subject { described_class.with_expires_at_after(2.days.from_now) } - let(:expiration_date) { nil } + let!(:pat) { create(:personal_access_token) } + + before do + stub_feature_flags(default_pat_expiration: false) + pat.update_attribute(:expires_at, nil) + end it 'includes the tokens with nil expires_at' do expect(described_class.with_no_expires_at).to contain_exactly(pat) @@ -103,11 +113,26 @@ 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 + context 'when default_pat_expiration feature flag is true' do + it "is invalid" 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") + expect(personal_access_token).not_to be_valid + expect(personal_access_token.errors[:expires_at].first).to eq("is invalid") + end + end + + context 'when default_pat_expiration feature flag is false' do + before do + stub_feature_flags(default_pat_expiration: false) + 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 end @@ -146,13 +171,48 @@ end end - context 'when the instance does not enforce a PAT expiry setting' do - before do - stub_ee_application_setting(max_personal_access_token_lifetime: nil) + context 'when default_pat_expiration feature flag is true' do + context 'when the instance does not enforce a PAT expiry setting' do + before do + stub_ee_application_setting(max_personal_access_token_lifetime: nil) + end + + it_behaves_like 'PAT expiry rules are not enforced' do + let(:max_expiration_date) { instance_level_max_expiration_date } + end + + it 'defaults to PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS' do + freeze_time do + personal_access_token.expires_at = nil + + expect(personal_access_token).to be_valid + expect(personal_access_token.expires_at).to eq( + PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now.to_date + ) + end + end end + end - it_behaves_like 'PAT expiry rules are not enforced' do - let(:max_expiration_date) { instance_level_max_expiration_date } + context 'when default_pat_expiration feature flag is false' do + context 'when the instance does not enforce a PAT expiry setting' do + before do + stub_ee_application_setting(max_personal_access_token_lifetime: nil) + stub_feature_flags(default_pat_expiration: false) + end + + it_behaves_like 'PAT expiry rules are not enforced' do + let(:max_expiration_date) { instance_level_max_expiration_date } + end + + it 'does not have a default' do + freeze_time do + personal_access_token.expires_at = nil + + expect(personal_access_token).to be_valid + expect(personal_access_token.expires_at).to eq(nil) + end + end end end end diff --git a/ee/spec/support/shared_examples/features/credentials_inventory_shared_examples.rb b/ee/spec/support/shared_examples/features/credentials_inventory_shared_examples.rb index ec05c646d72437..7ca429ca4a243d 100644 --- a/ee/spec/support/shared_examples/features/credentials_inventory_shared_examples.rb +++ b/ee/spec/support/shared_examples/features/credentials_inventory_shared_examples.rb @@ -6,25 +6,46 @@ let_it_be(:user) { defined?(managed_user) ? managed_user : create(:user, name: 'abc') } context 'when a personal access token is active' do - before_all do - create(:personal_access_token, - user: user, - created_at: '2019-12-10', - updated_at: '2020-06-22', - expires_at: nil) - end + context 'when default_pat_expiration feature flag is true' do + before do + create(:personal_access_token, + user: user, + created_at: '2019-12-10', + updated_at: '2020-06-22', + expires_at: nil) + visit credentials_path + end - before do - visit credentials_path + it 'shows the details', :aggregate_failures do + expect(first_row.text).to include('abc') + expect(first_row.text).to include('api') + expect(first_row.text).to include('2019-12-10') + expect(first_row.text).to include(PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS + .days.from_now.strftime("%Y-%m-%d")) + expect(first_row.text).not_to include('2020-06-22') + expect(first_row).to have_selector('a.btn', text: 'Revoke') + end end - it 'shows the details', :aggregate_failures do - expect(first_row.text).to include('abc') - expect(first_row.text).to include('api') - expect(first_row.text).to include('2019-12-10') - expect(first_row.text).to include('Never') - expect(first_row.text).not_to include('2020-06-22') - expect(first_row).to have_selector('a.btn', text: 'Revoke') + context 'when default_pat_expiration feature flag is false' do + before do + stub_feature_flags(default_pat_expiration: false) + create(:personal_access_token, + user: user, + created_at: '2019-12-10', + updated_at: '2020-06-22', + expires_at: nil) + visit credentials_path + end + + it 'shows the details', :aggregate_failures do + expect(first_row.text).to include('abc') + expect(first_row.text).to include('api') + expect(first_row.text).to include('2019-12-10') + expect(first_row.text).to include('Never') + expect(first_row.text).not_to include('2020-06-22') + expect(first_row).to have_selector('a.btn', text: 'Revoke') + end end end @@ -59,7 +80,7 @@ user: user, created_at: '2019-12-10', updated_at: '2020-06-22', - expires_at: nil) + expires_at: Time.zone.now) end before do diff --git a/ee/spec/views/shared/credentials_inventory/_expiry_date.html.haml_spec.rb b/ee/spec/views/shared/credentials_inventory/_expiry_date.html.haml_spec.rb index 0a7ef92d6527bf..7df14ce62de81c 100644 --- a/ee/spec/views/shared/credentials_inventory/_expiry_date.html.haml_spec.rb +++ b/ee/spec/views/shared/credentials_inventory/_expiry_date.html.haml_spec.rb @@ -20,8 +20,27 @@ context 'when an expirable credential is used' do let_it_be(:credential) { create(:personal_access_token, user: user, expires_at: nil) } - it 'shows "Never" when not expirable' do - expect(rendered).to have_text('Never') + context 'when default_pat_expiration feature flag is true' do + it 'shows default expiry date in case expires_at is nil' do + expect(rendered).to have_text( + PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS + .days + .from_now.strftime("%Y-%m-%d") + ) + end + end + + context 'when default_pat_expiration feature flag is false' do + before do + stub_feature_flags(default_pat_expiration: false) + credential.expires_at = nil + end + + it 'shows "Never" when not expirable' do + render 'shared/credentials_inventory/expiry_date', credential: credential + + expect(rendered).to have_text("Never") + end end context 'and is not expired' do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 669e97cf0e7f65..5bcd1233b755f4 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -53917,6 +53917,9 @@ msgstr "" msgid "must contain only a discord user ID." msgstr "" +msgid "must expire in 365 days" +msgstr "" + msgid "must have a repository" msgstr "" diff --git a/spec/lib/api/entities/personal_access_token_spec.rb b/spec/lib/api/entities/personal_access_token_spec.rb index fd3c53a21b474e..7f79cc80573215 100644 --- a/spec/lib/api/entities/personal_access_token_spec.rb +++ b/spec/lib/api/entities/personal_access_token_spec.rb @@ -19,7 +19,7 @@ user_id: user.id, last_used_at: nil, active: true, - expires_at: nil + expires_at: token.expires_at.iso8601 }) end end diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb index 45b8de509fcef7..bd6a7c156c4121 100644 --- a/spec/models/personal_access_token_spec.rb +++ b/spec/models/personal_access_token_spec.rb @@ -267,6 +267,41 @@ expect(personal_access_token).not_to be_valid expect(personal_access_token.errors[:scopes].first).to eq "can only contain available scopes" end + + context 'validates expires_at' do + let(:max_expiration_date) { described_class::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now } + + context 'when default_pat_expiration feature flag is true' do + context 'when expires_in is less than MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS days' do + it 'is valid' do + personal_access_token.expires_at = max_expiration_date - 1.day + + expect(personal_access_token).to be_valid + end + end + + context 'when expires_in is more than MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS days' do + it 'is invalid' 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('must expire in 365 days') + end + end + end + + context 'when default_pat_expiration feature flag is false' do + before do + stub_feature_flags(default_pat_expiration: false) + end + + it 'allows any expires_at value' do + personal_access_token.expires_at = max_expiration_date + 1.day + + expect(personal_access_token).to be_valid + end + end + end end describe 'scopes' do @@ -289,7 +324,7 @@ let_it_be(:revoked_token) { create(:personal_access_token, revoked: true) } let_it_be(:valid_token_and_notified) { create(:personal_access_token, expires_at: 2.days.from_now, expire_notification_delivered: true) } let_it_be(:valid_token) { create(:personal_access_token, expires_at: 2.days.from_now) } - let_it_be(:long_expiry_token) { create(:personal_access_token, expires_at: '999999-12-31'.to_date) } + let_it_be(:long_expiry_token) { create(:personal_access_token, expires_at: described_class::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now) } context 'in one day' do it "doesn't have any tokens" do @@ -427,4 +462,36 @@ end end end + + describe '#expires_at=' do + let(:personal_access_token) { described_class.new } + + context 'when default_pat_expiration feature flag is true' do + context 'expires_at set to empty value' do + [nil, ""].each do |expires_in_value| + it 'defaults to PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS' do + personal_access_token.expires_at = expires_in_value + + freeze_time do + expect(personal_access_token.expires_at).to eq( + PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now.to_date + ) + end + end + end + end + end + + context 'when default_pat_expiration feature flag is false' do + before do + stub_feature_flags(default_pat_expiration: false) + end + + it 'does not set a default' do + personal_access_token.expires_at = nil + + expect(personal_access_token.expires_at).to eq(nil) + end + end + end end diff --git a/spec/requests/api/internal/base_spec.rb b/spec/requests/api/internal/base_spec.rb index dff41c4c4773b7..6600366d2d21b4 100644 --- a/spec/requests/api/internal/base_spec.rb +++ b/spec/requests/api/internal/base_spec.rb @@ -10,6 +10,9 @@ let_it_be(:project, reload: true) { create(:project, :repository, :wiki_repo) } let_it_be(:personal_snippet) { create(:personal_snippet, :repository, author: user) } let_it_be(:project_snippet) { create(:project_snippet, :repository, author: user, project: project) } + let_it_be(:max_pat_access_token_lifetime) do + PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now.to_date.freeze + end let(:key) { create(:key, user: user) } let(:secret_token) { Gitlab::Shell.secret_token } @@ -194,39 +197,66 @@ def perform_request(headers: gitlab_shell_internal_api_request_header) expect(json_response['message']).to match(/\AInvalid scope: 'badscope'. Valid scopes are: /) end - it 'returns a token without expiry when the expires_at parameter is missing' do + it 'returns a token with expiry when it receives a valid expires_at parameter' do token_size = (PersonalAccessToken.token_prefix || '').size + 20 post api('/internal/personal_access_token'), params: { key_id: key.id, name: 'newtoken', - scopes: %w(read_api read_repository) + scopes: %w(read_api read_repository), + expires_at: max_pat_access_token_lifetime }, headers: gitlab_shell_internal_api_request_header expect(json_response['success']).to be_truthy expect(json_response['token']).to match(/\A\S{#{token_size}}\z/) expect(json_response['scopes']).to match_array(%w(read_api read_repository)) - expect(json_response['expires_at']).to be_nil + expect(json_response['expires_at']).to eq(max_pat_access_token_lifetime.iso8601) end - it 'returns a token with expiry when it receives a valid expires_at parameter' do - token_size = (PersonalAccessToken.token_prefix || '').size + 20 + context 'when default_pat_expiration feature flag is true' do + it 'returns token with expiry as PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS' do + freeze_time do + token_size = (PersonalAccessToken.token_prefix || '').size + 20 - post api('/internal/personal_access_token'), - params: { - key_id: key.id, - name: 'newtoken', - scopes: %w(read_api read_repository), - expires_at: '9001-11-17' - }, - headers: gitlab_shell_internal_api_request_header + post api('/internal/personal_access_token'), + params: { + key_id: key.id, + name: 'newtoken', + scopes: %w(read_api read_repository) + }, + headers: gitlab_shell_internal_api_request_header - expect(json_response['success']).to be_truthy - expect(json_response['token']).to match(/\A\S{#{token_size}}\z/) - expect(json_response['scopes']).to match_array(%w(read_api read_repository)) - expect(json_response['expires_at']).to eq('9001-11-17') + expect(json_response['success']).to be_truthy + expect(json_response['token']).to match(/\A\S{#{token_size}}\z/) + expect(json_response['scopes']).to match_array(%w(read_api read_repository)) + expect(json_response['expires_at']).to eq(max_pat_access_token_lifetime.iso8601) + end + end + end + + context 'when default_pat_expiration feature flag is false' do + before do + stub_feature_flags(default_pat_expiration: false) + end + + it 'uses nil expiration value' do + token_size = (PersonalAccessToken.token_prefix || '').size + 20 + + post api('/internal/personal_access_token'), + params: { + key_id: key.id, + name: 'newtoken', + scopes: %w(read_api read_repository) + }, + headers: gitlab_shell_internal_api_request_header + + expect(json_response['success']).to be_truthy + expect(json_response['token']).to match(/\A\S{#{token_size}}\z/) + expect(json_response['scopes']).to match_array(%w(read_api read_repository)) + expect(json_response['expires_at']).to be_nil + end end end diff --git a/spec/requests/api/resource_access_tokens_spec.rb b/spec/requests/api/resource_access_tokens_spec.rb index a13aa48a497aa5..ce05fa2b383185 100644 --- a/spec/requests/api/resource_access_tokens_spec.rb +++ b/spec/requests/api/resource_access_tokens_spec.rb @@ -336,13 +336,33 @@ context "when 'expires_at' is not set" do let(:expires_at) { nil } - it "creates a #{source_type} access token with the params", :aggregate_failures do - create_token + context 'when default_pat_expiration feature flag is true' do + it "creates a #{source_type} access token with the default expires_at value", :aggregate_failures do + freeze_time do + create_token + expires_at = PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now + + expect(response).to have_gitlab_http_status(:created) + expect(json_response["name"]).to eq("test") + expect(json_response["scopes"]).to eq(["api"]) + expect(json_response["expires_at"]).to eq(expires_at.to_date.iso8601) + end + end + end - expect(response).to have_gitlab_http_status(:created) - expect(json_response["name"]).to eq("test") - expect(json_response["scopes"]).to eq(["api"]) - expect(json_response["expires_at"]).to eq(nil) + context 'when default_pat_expiration feature flag is false' do + before do + stub_feature_flags(default_pat_expiration: false) + end + + it "creates a #{source_type} access token with the params", :aggregate_failures do + create_token + + expect(response).to have_gitlab_http_status(:created) + expect(json_response["name"]).to eq("test") + expect(json_response["scopes"]).to eq(["api"]) + expect(json_response["expires_at"]).to eq(nil) + end end end diff --git a/spec/serializers/access_token_entity_base_spec.rb b/spec/serializers/access_token_entity_base_spec.rb index e14a07a346ae48..8a92a53d0c1cb4 100644 --- a/spec/serializers/access_token_entity_base_spec.rb +++ b/spec/serializers/access_token_entity_base_spec.rb @@ -16,7 +16,7 @@ revoked: false, created_at: token.created_at, scopes: token.scopes, - expires_at: nil, + expires_at: token.expires_at.iso8601, expired: false, expires_soon: false )) diff --git a/spec/services/resource_access_tokens/create_service_spec.rb b/spec/services/resource_access_tokens/create_service_spec.rb index 464fc18f1f07f5..59d582f038a142 100644 --- a/spec/services/resource_access_tokens/create_service_spec.rb +++ b/spec/services/resource_access_tokens/create_service_spec.rb @@ -9,6 +9,9 @@ let_it_be(:project) { create(:project, :private) } let_it_be(:group) { create(:group, :private) } let_it_be(:params) { {} } + let_it_be(:max_pat_access_token_lifetime) do + PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now.to_date.freeze + end before do stub_config_setting(host: 'example.com') @@ -185,20 +188,51 @@ context 'expires_at' do context 'when no expiration value is passed' do - it 'uses nil expiration value' do - response = subject - access_token = response.payload[:access_token] + context 'when default_pat_expiration feature flag is true' do + it 'defaults to PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS' do + freeze_time do + response = subject + access_token = response.payload[:access_token] + + expect(access_token.expires_at).to eq( + max_pat_access_token_lifetime.to_date + ) + end + end - expect(access_token.expires_at).to eq(nil) + context 'expiry of the project bot member' do + it 'project bot membership does not expire' do + response = subject + access_token = response.payload[:access_token] + project_bot = access_token.user + + expect(resource.members.find_by(user_id: project_bot.id).expires_at).to eq( + max_pat_access_token_lifetime.to_date + ) + end + end end - context 'expiry of the project bot member' do - it 'project bot membership does not expire' do + context 'when default_pat_expiration feature flag is false' do + before do + stub_feature_flags(default_pat_expiration: false) + end + + it 'uses nil expiration value' do response = subject access_token = response.payload[:access_token] - project_bot = access_token.user - expect(resource.members.find_by(user_id: project_bot.id).expires_at).to eq(nil) + expect(access_token.expires_at).to eq(nil) + end + + context 'expiry of the project bot member' do + it 'project bot membership expires' do + response = subject + access_token = response.payload[:access_token] + project_bot = access_token.user + + expect(resource.members.find_by(user_id: project_bot.id).expires_at).to eq(nil) + end end end end @@ -219,7 +253,7 @@ access_token = response.payload[:access_token] project_bot = access_token.user - expect(resource.members.find_by(user_id: project_bot.id).expires_at).to eq(params[:expires_at]) + expect(resource.members.find_by(user_id: project_bot.id).expires_at).to eq(access_token.expires_at) end end end -- GitLab From 347d8c4305a1a4b86b890013c404ed7184f4db14 Mon Sep 17 00:00:00 2001 From: Jessie Young Date: Thu, 11 May 2023 11:04:01 -0700 Subject: [PATCH 2/2] Freeze time --- spec/requests/api/internal/base_spec.rb | 28 +++++++++++++------------ 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/spec/requests/api/internal/base_spec.rb b/spec/requests/api/internal/base_spec.rb index 6600366d2d21b4..6414b1efe6a7cb 100644 --- a/spec/requests/api/internal/base_spec.rb +++ b/spec/requests/api/internal/base_spec.rb @@ -198,21 +198,23 @@ def perform_request(headers: gitlab_shell_internal_api_request_header) end it 'returns a token with expiry when it receives a valid expires_at parameter' do - token_size = (PersonalAccessToken.token_prefix || '').size + 20 + freeze_time do + token_size = (PersonalAccessToken.token_prefix || '').size + 20 - post api('/internal/personal_access_token'), - params: { - key_id: key.id, - name: 'newtoken', - scopes: %w(read_api read_repository), - expires_at: max_pat_access_token_lifetime - }, - headers: gitlab_shell_internal_api_request_header + post api('/internal/personal_access_token'), + params: { + key_id: key.id, + name: 'newtoken', + scopes: %w(read_api read_repository), + expires_at: max_pat_access_token_lifetime + }, + headers: gitlab_shell_internal_api_request_header - expect(json_response['success']).to be_truthy - expect(json_response['token']).to match(/\A\S{#{token_size}}\z/) - expect(json_response['scopes']).to match_array(%w(read_api read_repository)) - expect(json_response['expires_at']).to eq(max_pat_access_token_lifetime.iso8601) + expect(json_response['success']).to be_truthy + expect(json_response['token']).to match(/\A\S{#{token_size}}\z/) + expect(json_response['scopes']).to match_array(%w(read_api read_repository)) + expect(json_response['expires_at']).to eq(max_pat_access_token_lifetime.iso8601) + end end context 'when default_pat_expiration feature flag is true' do -- GitLab