diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index 297d1cda50752785caaa83e74cfc6a2a2e7e2e93..2a6c8a38f5029b5bfc56a81a95f7053157503eb8 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -20,25 +20,20 @@ class Trigger < Ci::ApplicationRecord has_many :pipelines, class_name: 'Ci::Pipeline' - validates :token, presence: true, uniqueness: true + validates :token_encrypted, presence: true, uniqueness: true validates :owner, presence: true validates :project, presence: true validate :expires_at_before_instance_max_expiry_date, on: :create - before_validation :set_default_values + before_validation :ensure_token + ignore_column :token, remove_with: '18.7', remove_after: '2025-11-30' ignore_column :encrypted_token, remove_with: '18.4', remove_after: '2025-09-30' ignore_column :encrypted_token_iv, remove_with: '18.4', remove_after: '2025-09-30' - # rubocop:disable Gitlab/TokenWithoutPrefix -- we are doing this ourselves here since ensure_token - # does not work as expected - add_authentication_token_field(:token, - encrypted: -> { - Feature.enabled?(:encrypted_trigger_token_lookup, :instance) ? :required : :migrating - } - ) - # rubocop:enable Gitlab/TokenWithoutPrefix + add_authentication_token_field(:token, encrypted: :required, format_with_prefix: :token_prefix) + scope :with_last_used, -> do ci_pipelines = Ci::Pipeline.arel_table last_used_pipelines = @@ -54,12 +49,8 @@ class Trigger < Ci::ApplicationRecord scope :with_token, ->(tokens) { tokens = Array.wrap(tokens).reject(&:blank?) - if Feature.enabled?(:encrypted_trigger_token_lookup, :instance) - encrypted_tokens = tokens.map { |token| Ci::Trigger.encode(token) } - where(token_encrypted: encrypted_tokens) - else - where(token: tokens) - end + encrypted_tokens = tokens.map { |token| Ci::Trigger.encode(token) } + where(token_encrypted: encrypted_tokens) } scope :ready_for_deletion, -> { @@ -71,15 +62,9 @@ def self.prefix_for_trigger_token end def token=(token_value) - super self.set_token(token_value) end - def set_default_values - self.set_token(self.attributes['token']) if self.attributes['token'].present? - self.set_token("#{self.class.prefix_for_trigger_token}#{SecureRandom.hex(20)}") if self.token.blank? - end - def last_used # The instance should be preloaded by `.with_last_used` for performance reason return attributes['last_used'] if attributes.has_key?('last_used') @@ -101,6 +86,10 @@ def can_access_project? protected + def token_prefix + Ci::Trigger.prefix_for_trigger_token + end + def expires_at_before_instance_max_expiry_date return unless expires_at diff --git a/config/feature_flags/gitlab_com_derisk/encrypted_trigger_token_lookup.yml b/config/feature_flags/gitlab_com_derisk/encrypted_trigger_token_lookup.yml deleted file mode 100644 index 0fd1e6efb45ff2758862d5f1ae54a8bde9ec7321..0000000000000000000000000000000000000000 --- a/config/feature_flags/gitlab_com_derisk/encrypted_trigger_token_lookup.yml +++ /dev/null @@ -1,10 +0,0 @@ ---- -name: encrypted_trigger_token_lookup -description: -feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/work_items/554239 -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/197268 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/558025 -milestone: '18.3' -group: group::pipeline execution -type: gitlab_com_derisk -default_enabled: false diff --git a/db/post_migrate/20250709140240_queue_migrate_ci_triggers_token_to_token_encrypted.rb b/db/post_migrate/20250709140240_queue_migrate_ci_triggers_token_to_token_encrypted.rb deleted file mode 100644 index 8d67ffb76c4424172a429c99bfc600fa54bf20a3..0000000000000000000000000000000000000000 --- a/db/post_migrate/20250709140240_queue_migrate_ci_triggers_token_to_token_encrypted.rb +++ /dev/null @@ -1,29 +0,0 @@ -# frozen_string_literal: true - -class QueueMigrateCiTriggersTokenToTokenEncrypted < Gitlab::Database::Migration[2.3] - milestone '18.3' - - restrict_gitlab_migration gitlab_schema: :gitlab_ci - - MIGRATION = "MigrateCiTriggersTokenToTokenEncrypted" - DELAY_INTERVAL = 2.minutes - BATCH_SIZE = 1_000 - MAX_BATCH_SIZE = 2_000 - SUB_BATCH_SIZE = 100 - - def up - queue_batched_background_migration( - MIGRATION, - :ci_triggers, - :id, - job_interval: DELAY_INTERVAL, - batch_size: BATCH_SIZE, - sub_batch_size: SUB_BATCH_SIZE, - max_batch_size: MAX_BATCH_SIZE - ) - end - - def down - delete_batched_background_migration(MIGRATION, :ci_triggers, :id, []) - end -end diff --git a/db/post_migrate/20251022131103_remove_encrypted_token_from_ci_trigger.rb b/db/post_migrate/20251022131103_remove_encrypted_token_from_ci_trigger.rb new file mode 100644 index 0000000000000000000000000000000000000000..47d29549dc2d28eba89fe47d8969666e00307028 --- /dev/null +++ b/db/post_migrate/20251022131103_remove_encrypted_token_from_ci_trigger.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class RemoveEncryptedTokenFromCiTrigger < Gitlab::Database::Migration[2.3] + milestone '18.4' + disable_ddl_transaction! + + def up + remove_column :ci_triggers, :encrypted_token, if_exists: true + remove_column :ci_triggers, :encrypted_token_iv, if_exists: true + end + + def down + add_column :ci_triggers, :encrypted_token, :binary, if_not_exists: true + add_column :ci_triggers, :encrypted_token_iv, :binary, if_not_exists: true + end +end diff --git a/db/post_migrate/20251022142745_remove_token_from_ci_trigger.rb b/db/post_migrate/20251022142745_remove_token_from_ci_trigger.rb new file mode 100644 index 0000000000000000000000000000000000000000..2f76e5931d90456c53578c5982912a7f6ff7a0e3 --- /dev/null +++ b/db/post_migrate/20251022142745_remove_token_from_ci_trigger.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class RemoveTokenFromCiTrigger < Gitlab::Database::Migration[2.3] + milestone '18.4' + disable_ddl_transaction! + + def up + remove_column :ci_triggers, :token, if_exists: true + end + + def down + add_column :ci_triggers, :token, :string, if_not_exists: true + add_concurrent_index :ci_triggers, :token, unique: true, + name: 'index_ci_triggers_on_token' + end +end diff --git a/db/schema_migrations/20250709140240 b/db/schema_migrations/20250709140240 deleted file mode 100644 index bbb59d965dd44912271b656b3669c8ce6e1a6d8f..0000000000000000000000000000000000000000 --- a/db/schema_migrations/20250709140240 +++ /dev/null @@ -1 +0,0 @@ -bc3057d224b0e67b556c50d1c63ce8a811cc9696d8aeec52c2d65d6bb184cd98 \ No newline at end of file diff --git a/db/schema_migrations/20251022131103 b/db/schema_migrations/20251022131103 new file mode 100644 index 0000000000000000000000000000000000000000..f3f24ef594add643770bb72f4f7c4aaf127a651a --- /dev/null +++ b/db/schema_migrations/20251022131103 @@ -0,0 +1 @@ +08faada1fa0194099bf292a75257b08463cae76fab4fd897f8a161390f102494 \ No newline at end of file diff --git a/db/schema_migrations/20251022142745 b/db/schema_migrations/20251022142745 new file mode 100644 index 0000000000000000000000000000000000000000..ef5bac63eb23d29eca7a1c4aa1efc15f9dba4db5 --- /dev/null +++ b/db/schema_migrations/20251022142745 @@ -0,0 +1 @@ +1d9778e66a4bd379aa739fc8600f5e03b4e0231f72941823cdffb3bfd5a274eb \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 64d8af58f2481beba53dfe077aa42827bfa206ed..f16a76d7e1de5465e440cb0bea91a4128e78f22d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -14379,14 +14379,11 @@ ALTER SEQUENCE ci_subscriptions_projects_id_seq OWNED BY ci_subscriptions_projec CREATE TABLE ci_triggers ( id bigint NOT NULL, - token character varying, created_at timestamp without time zone, updated_at timestamp without time zone, project_id bigint, owner_id bigint NOT NULL, description character varying, - encrypted_token bytea, - encrypted_token_iv bytea, expires_at timestamp with time zone, token_encrypted text, CONSTRAINT check_4905e4c2cb CHECK ((char_length(token_encrypted) <= 255)), @@ -39319,8 +39316,6 @@ CREATE INDEX index_ci_triggers_on_owner_id ON ci_triggers USING btree (owner_id) CREATE INDEX index_ci_triggers_on_project_id_and_id ON ci_triggers USING btree (project_id, id); -CREATE UNIQUE INDEX index_ci_triggers_on_token ON ci_triggers USING btree (token); - CREATE UNIQUE INDEX index_ci_triggers_on_token_encrypted ON ci_triggers USING btree (token_encrypted); CREATE INDEX index_ci_unit_test_failures_on_build_id ON ci_unit_test_failures USING btree (build_id); diff --git a/lib/gitlab/background_migration/migrate_ci_triggers_token_to_token_encrypted.rb b/lib/gitlab/background_migration/migrate_ci_triggers_token_to_token_encrypted.rb deleted file mode 100644 index dc2de1c192338a37d391fb3056f88fae8291ee70..0000000000000000000000000000000000000000 --- a/lib/gitlab/background_migration/migrate_ci_triggers_token_to_token_encrypted.rb +++ /dev/null @@ -1,39 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module BackgroundMigration - class MigrateCiTriggersTokenToTokenEncrypted < BatchedMigrationJob - feature_category :continuous_integration - operation_name :update - class CiTrigger < ::Ci::ApplicationRecord - include TokenAuthenticatable - - TRIGGER_TOKEN_PREFIX = 'glptt-' - - add_authentication_token_field :token, encrypted: :migrating, format_with_prefix: :compute_token_prefix - self.table_name = 'ci_triggers' - - def token=(new_token) - set_token(new_token) - end - - def compute_token_prefix - TRIGGER_TOKEN_PREFIX - end - end - - def perform - each_sub_batch( - batching_scope: ->(relation) { - relation.where(token_encrypted: nil) - }) do |sub_batch| - sub_batch.each do |trigger| - Gitlab::BackgroundMigration::MigrateCiTriggersTokenToTokenEncrypted::CiTrigger - .find(trigger.id) - .update!(token: trigger.token) - end - end - end - end - end -end diff --git a/spec/lib/gitlab/background_migration/migrate_ci_triggers_token_to_token_encrypted_spec.rb b/spec/lib/gitlab/background_migration/migrate_ci_triggers_token_to_token_encrypted_spec.rb deleted file mode 100644 index 7e50bf5dca91189a7d4d247f09ddd200a3c44156..0000000000000000000000000000000000000000 --- a/spec/lib/gitlab/background_migration/migrate_ci_triggers_token_to_token_encrypted_spec.rb +++ /dev/null @@ -1,49 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::BackgroundMigration::MigrateCiTriggersTokenToTokenEncrypted, feature_category: :continuous_integration, migration: :gitlab_ci do - let(:connection) { Ci::ApplicationRecord.connection } - let(:ci_triggers) do - table(:ci_triggers, database: :ci) - end - - let!(:trigger1) { ci_triggers.create!(token: 'foo', owner_id: 1, project_id: 1) } - let!(:trigger2) { ci_triggers.create!(token: 'bar', owner_id: 1, project_id: 1) } - let!(:trigger3) { ci_triggers.create!(token: 'baz', owner_id: 1, project_id: 1) } - - subject(:perform_migration) do - described_class.new( - start_id: trigger1.id, - end_id: trigger3.id, - batch_table: 'ci_triggers', - batch_column: 'id', - sub_batch_size: 1, - pause_ms: 10, - connection: connection - ).perform - end - - it 'migrates token to token_encrypted field' do - expect(trigger1.reload[:token_encrypted]).to be_nil - expect(trigger2.reload[:token_encrypted]).to be_nil - expect(trigger3.reload[:token_encrypted]).to be_nil - - expect(Gitlab::BackgroundMigration::MigrateCiTriggersTokenToTokenEncrypted::CiTrigger.new.compute_token_prefix) - .to eq('glptt-') - perform_migration - - expect(trigger1.reload[:token_encrypted]).to eq(Ci::Trigger.encode(trigger1.reload.token)) - expect(trigger2.reload[:token_encrypted]).to eq(Ci::Trigger.encode(trigger2.reload.token)) - expect(trigger3.reload[:token_encrypted]).to eq(Ci::Trigger.encode(trigger3.reload.token)) - - encrypted_token1 = trigger1.reload.token - expect(encrypted_token1).to eq('foo') - - encrypted_token2 = trigger2.reload.token - expect(encrypted_token2).to eq('bar') - - encrypted_token3 = trigger3.reload.token - expect(encrypted_token3).to eq('baz') - end -end diff --git a/spec/migrations/20250709140240_queue_migrate_ci_triggers_token_to_token_encrypted_spec.rb b/spec/migrations/20250709140240_queue_migrate_ci_triggers_token_to_token_encrypted_spec.rb deleted file mode 100644 index c6c1c9e03a7ce0228754a3d40caffa0f8cc40e20..0000000000000000000000000000000000000000 --- a/spec/migrations/20250709140240_queue_migrate_ci_triggers_token_to_token_encrypted_spec.rb +++ /dev/null @@ -1,57 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' -require_migration! - -RSpec.describe QueueMigrateCiTriggersTokenToTokenEncrypted, migration: :gitlab_ci, feature_category: :continuous_integration do - let(:batched_migrations) { table(:batched_background_migrations) } - - let!(:migration) { described_class::MIGRATION } - - describe '#up' do - context 'with migration present' do - let!(:ci_trigger_token_encryption_migration) do - batched_migrations.create!( - job_class_name: 'QueueMigrateCiTriggersTokenToTokenEncrypted', - table_name: :ci_triggers, - column_name: :id, - job_arguments: [], - interval: 2.minutes, - min_value: 1, - max_value: 2, - batch_size: 1000, - sub_batch_size: 100, - gitlab_schema: :gitlab_ci, - status: 3 # finished - ) - end - - context 'when migration finished successfully' do - it 'does not raise exception' do - expect { migrate! }.not_to raise_error - end - - it 'schedules background jobs for each batch of ci_triggers' do - migrate! - - expect(migration).to have_scheduled_batched_migration( - gitlab_schema: :gitlab_ci, - table_name: :ci_triggers, - column_name: :id, - batch_size: described_class::BATCH_SIZE, - sub_batch_size: described_class::SUB_BATCH_SIZE - ) - end - end - end - end - - describe '#down' do - it 'deletes all batched migration records' do - migrate! - schema_migrate_down! - - expect(migration).not_to have_scheduled_batched_migration - end - end -end diff --git a/spec/models/ci/trigger_spec.rb b/spec/models/ci/trigger_spec.rb index 6e654c81f0c479f6d806fee8fd063ca5cda99676..ae6cddcbe7ce69a1753c81b745cb956d180ffa00 100644 --- a/spec/models/ci/trigger_spec.rb +++ b/spec/models/ci/trigger_spec.rb @@ -101,7 +101,7 @@ end describe '.with_token' do - context 'when ff lookup for encrypted token is enabled' do + context 'when looking for ci trigger by token' do let_it_be(:project) { create(:project) } let_it_be(:trigger_1) { create(:ci_trigger, project: project) } let_it_be(:trigger_2) { create(:ci_trigger, project: project) } @@ -131,41 +131,6 @@ expect(result).to be_empty end end - - context 'when ff lookup for encrypted token is disabled' do - let_it_be(:project) { create(:project) } - let_it_be(:trigger_1) { create(:ci_trigger, project: project) } - let_it_be(:trigger_2) { create(:ci_trigger, project: project) } - let_it_be(:trigger_3) { create(:ci_trigger, project: project) } - - before do - stub_feature_flags(encrypted_trigger_token_lookup: false) - end - - it 'returns the trigger for a valid token' do - result = described_class.with_token(trigger_1.token) - - expect(result).to contain_exactly(trigger_1) - end - - it 'returns the triggers for multiple valid tokens' do - result = described_class.with_token([trigger_1.token, trigger_2.token]) - - expect(result).to contain_exactly(trigger_1, trigger_2) - end - - it 'ignores blank tokens' do - result = described_class.with_token([nil, '', ' ']) - - expect(result).to be_empty - end - - it 'does not return triggers for invalid token' do - result = described_class.with_token('nonexistent-token') - - expect(result).to be_empty - end - end end end