From 8c63f42a4e41c60469ad17852687994b37b859a0 Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Wed, 5 Mar 2025 17:41:03 +0100 Subject: [PATCH 1/8] Move partition_id to token payload Changelog: changed --- app/models/ci/runner.rb | 23 ++++---- .../token_field/generator/routable_token.rb | 2 +- .../generator/routable_token_spec.rb | 2 +- spec/models/ci/runner_spec.rb | 52 ++++++++++++++----- 4 files changed, 50 insertions(+), 29 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index a940a5f03db1dd..3730f6c5029780 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -35,7 +35,8 @@ class Runner < Ci::ApplicationRecord o: ->(token_owner_record) { token_owner_record.owner.organization_id }, g: ->(token_owner_record) { token_owner_record.group_type? ? token_owner_record.sharding_key_id : nil }, p: ->(token_owner_record) { token_owner_record.project_type? ? token_owner_record.sharding_key_id : nil }, - u: ->(token_owner_record) { token_owner_record.creator_id } + u: ->(token_owner_record) { token_owner_record.creator_id }, + t: ->(token_owner_record) { token_owner_record.partition_id } } } @@ -446,11 +447,8 @@ def only_for?(project) def short_sha return unless token - # We want to show the first characters of the hash, so we need to bypass any fixed components of the token, - # such as CREATED_RUNNER_TOKEN_PREFIX or partition_id_prefix_in_16_bit_encode - partition_prefix = partition_id_prefix_in_16_bit_encode + # We want to show the first characters of the hash, so we need to bypass CREATED_RUNNER_TOKEN_PREFIX start_index = authenticated_user_registration_type? ? CREATED_RUNNER_TOKEN_PREFIX.length : 0 - start_index += partition_prefix.length if token[start_index..].start_with?(partition_prefix) token[start_index..start_index + RUNNER_SHORT_SHA_LENGTH - 1] end @@ -568,6 +566,10 @@ def dedicated_gitlab_hosted? false end + def partition_id + self.class.runner_types[runner_type] + end + private scope :with_upgrade_status, ->(upgrade_status) do @@ -667,17 +669,10 @@ def no_allowed_plan_ids errors.add(:runner, 'cannot have allowed plans assigned') unless allowed_plan_ids.empty? end - def partition_id_prefix_in_16_bit_encode - # Prefix with t1 / t2 / t3 (`t` as in runner type, to allow us to easily detect how a token got prefixed). - # This is needed in order to ensure that tokens have unique values across partitions - # in the new ci_runners partitioned table. - "t#{self.class.runner_types[runner_type].to_s(16)}_" - end - def prefix_for_new_and_legacy_runner - return partition_id_prefix_in_16_bit_encode if registration_token_registration_type? + return '' if registration_token_registration_type? - "#{CREATED_RUNNER_TOKEN_PREFIX}#{partition_id_prefix_in_16_bit_encode}" + CREATED_RUNNER_TOKEN_PREFIX end end end diff --git a/lib/authn/token_field/generator/routable_token.rb b/lib/authn/token_field/generator/routable_token.rb index 15d68a9d7f7c09..9c0e83ab7f40a0 100644 --- a/lib/authn/token_field/generator/routable_token.rb +++ b/lib/authn/token_field/generator/routable_token.rb @@ -7,7 +7,7 @@ class RoutableToken RANDOM_BYTES_LENGTH = 16 BASE64_PAYLOAD_LENGTH_HOLDER_BYTES = 2 CRC_BYTES = 7 - VALID_ROUTING_KEYS = %i[c g o p u].freeze + VALID_ROUTING_KEYS = %i[c g o p u t].freeze REQUIRED_ROUTING_KEYS = %i[o].freeze MAXIMUM_SIZE_OF_ROUTING_PAYLOAD = 159 DEFAULT_ROUTING_PAYLOAD_HASH = diff --git a/spec/lib/authn/token_field/generator/routable_token_spec.rb b/spec/lib/authn/token_field/generator/routable_token_spec.rb index 872dcb3200cc7b..57b04777154e78 100644 --- a/spec/lib/authn/token_field/generator/routable_token_spec.rb +++ b/spec/lib/authn/token_field/generator/routable_token_spec.rb @@ -41,7 +41,7 @@ it 'raises an exception' do expect { generator }.to raise_error(described_class::InvalidRoutingKeys, - "Invalid routing keys: :q, :k. Valid routing keys are: :c, :g, :o, :p, :u.") + "Invalid routing keys: :q, :k. Valid routing keys are: :c, :g, :o, :p, :u, :t.") end end end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 7200e49bc46011..1fc5dd36fb204e 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -1812,7 +1812,6 @@ def does_db_update specify { expect(runner.token).not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } - it { is_expected.not_to start_with('t1_') } it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } end @@ -1821,7 +1820,6 @@ def does_db_update specify { expect(runner.token).to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } - it { is_expected.not_to start_with('t1_') } it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } end end @@ -1858,12 +1856,18 @@ def does_db_update end shared_examples 'an encrypted routable token for resource' do |prefix| + let(:routing_payload) do + { + c: 1, + **(resource.is_a?(Group) ? { g: resource.id.to_s(36) } : { p: resource.id.to_s(36) }), + o: resource.organization_id.to_s(36), + u: creator.id.to_s(36), + t: described_class.runner_types[runner_type].to_s(36) + }.sort.to_h + end + let(:expected_routing_payload) do - if resource.is_a?(Group) - "c:1\ng:#{resource.id.to_s(36)}\no:#{resource.organization_id.to_s(36)}\nu:#{creator.id.to_s(36)}" - else - "c:1\no:#{resource.organization_id.to_s(36)}\np:#{resource.id.to_s(36)}\nu:#{creator.id.to_s(36)}" - end + routing_payload.map { |pairs| pairs.join(':') }.join("\n") end context 'when :routable_runner_token feature flag is enabled for the resource' do @@ -1924,15 +1928,15 @@ def does_db_update let(:registration_type) { :registration_token } context 'when runner is instance type' do - it_behaves_like 'an instance runner encrypted token', 't1_' + it_behaves_like 'an instance runner encrypted token' end context 'when runner is group type' do - it_behaves_like 'a group runner encrypted token', 't2_' + it_behaves_like 'a group runner encrypted token' end context 'when runner is project type' do - it_behaves_like 'a project runner encrypted token', 't3_' + it_behaves_like 'a project runner encrypted token' end end @@ -1940,15 +1944,15 @@ def does_db_update let(:registration_type) { :authenticated_user } context 'when runner is instance type' do - it_behaves_like 'an instance runner encrypted token', 'glrt-t1_' + it_behaves_like 'an instance runner encrypted token', described_class::CREATED_RUNNER_TOKEN_PREFIX end context 'when runner is group type' do - it_behaves_like 'a group runner encrypted token', 'glrt-t2_' + it_behaves_like 'a group runner encrypted token', described_class::CREATED_RUNNER_TOKEN_PREFIX end context 'when runner is project type' do - it_behaves_like 'a project runner encrypted token', 'glrt-t3_' + it_behaves_like 'a project runner encrypted token', described_class::CREATED_RUNNER_TOKEN_PREFIX end end end @@ -2347,6 +2351,28 @@ def does_db_update specify { expect(is_hosted).to be false } end + describe '#partition_id' do + subject(:partition) { runner.partition_id } + + context 'with an instance runner' do + let(:runner) { build(:ci_runner, :instance) } + + it { is_expected.to be(1) } + end + + context 'with a group runner' do + let(:runner) { build(:ci_runner, :group, groups: [group]) } + + it { is_expected.to be(2) } + end + + context 'with a project runner' do + let(:runner) { build(:ci_runner, :project, projects: [project]) } + + it { is_expected.to be(3) } + end + end + describe 'status scopes', :freeze_time do before_all do freeze_time # Freeze time before `let_it_be` runs, so that runner statuses are frozen during execution -- GitLab From 4c62080d8db3e434f6318f99d741504f9acb3ff5 Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Tue, 11 Mar 2025 13:46:30 +0100 Subject: [PATCH 2/8] Adds prefix for registration type runner --- app/models/ci/runner.rb | 8 +++++--- spec/models/ci/runner_spec.rb | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 3730f6c5029780..2c0405350ebda6 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -63,6 +63,7 @@ class Runner < Ci::ApplicationRecord # Prefix assigned to runners created from the UI, instead of registered via the command line CREATED_RUNNER_TOKEN_PREFIX = 'glrt-' + REGISTRATION_RUNNER_TOKEN_PREFIX = 'glrrt-' RUNNER_SHORT_SHA_LENGTH = 8 @@ -447,8 +448,9 @@ def only_for?(project) def short_sha return unless token - # We want to show the first characters of the hash, so we need to bypass CREATED_RUNNER_TOKEN_PREFIX - start_index = authenticated_user_registration_type? ? CREATED_RUNNER_TOKEN_PREFIX.length : 0 + # We want to show the first characters of the hash, so we need to bypass any fixed components of the token, + # such as CREATED_RUNNER_TOKEN_PREFIX or REGISTRATION_RUNNER_TOKEN_PREFIX + start_index = authenticated_user_registration_type? ? CREATED_RUNNER_TOKEN_PREFIX.length : REGISTRATION_RUNNER_TOKEN_PREFIX.length token[start_index..start_index + RUNNER_SHORT_SHA_LENGTH - 1] end @@ -670,7 +672,7 @@ def no_allowed_plan_ids end def prefix_for_new_and_legacy_runner - return '' if registration_token_registration_type? + return REGISTRATION_RUNNER_TOKEN_PREFIX if registration_token_registration_type? CREATED_RUNNER_TOKEN_PREFIX end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 1fc5dd36fb204e..d7bf83e5920260 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -1812,6 +1812,7 @@ def does_db_update specify { expect(runner.token).not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } + it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } end @@ -1820,6 +1821,7 @@ def does_db_update specify { expect(runner.token).to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } + it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } end end @@ -1928,15 +1930,15 @@ def does_db_update let(:registration_type) { :registration_token } context 'when runner is instance type' do - it_behaves_like 'an instance runner encrypted token' + it_behaves_like 'an instance runner encrypted token', described_class::REGISTRATION_RUNNER_TOKEN_PREFIX end context 'when runner is group type' do - it_behaves_like 'a group runner encrypted token' + it_behaves_like 'a group runner encrypted token', described_class::REGISTRATION_RUNNER_TOKEN_PREFIX end context 'when runner is project type' do - it_behaves_like 'a project runner encrypted token' + it_behaves_like 'a project runner encrypted token', described_class::REGISTRATION_RUNNER_TOKEN_PREFIX end end -- GitLab From 1ef3389b2187f8e3adce36e5b1aa4c3c4ff4a29b Mon Sep 17 00:00:00 2001 From: Bojan Marjanovic Date: Thu, 20 Mar 2025 18:25:35 +0100 Subject: [PATCH 3/8] Update the REGISTRATION_RUNNER_TOKEN_PREFIX --- app/models/ci/runner.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 2c0405350ebda6..6ffce64b82dec5 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -63,7 +63,7 @@ class Runner < Ci::ApplicationRecord # Prefix assigned to runners created from the UI, instead of registered via the command line CREATED_RUNNER_TOKEN_PREFIX = 'glrt-' - REGISTRATION_RUNNER_TOKEN_PREFIX = 'glrrt-' + REGISTRATION_RUNNER_TOKEN_PREFIX = 'glrtr-' RUNNER_SHORT_SHA_LENGTH = 8 -- GitLab From d71945dff66fceecdeaf2f3f80aec787cc339a06 Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Fri, 21 Mar 2025 11:43:47 +0100 Subject: [PATCH 4/8] Updates short_sha method --- app/models/ci/runner.rb | 21 ++++++++++++++++++--- spec/models/ci/runner_spec.rb | 2 ++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 6ffce64b82dec5..484f31e9df7681 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -448,9 +448,17 @@ def only_for?(project) def short_sha return unless token - # We want to show the first characters of the hash, so we need to bypass any fixed components of the token, - # such as CREATED_RUNNER_TOKEN_PREFIX or REGISTRATION_RUNNER_TOKEN_PREFIX - start_index = authenticated_user_registration_type? ? CREATED_RUNNER_TOKEN_PREFIX.length : REGISTRATION_RUNNER_TOKEN_PREFIX.length + # We want to show the first characters of the hash, so we need to bypass any fixed components of the token, such + # as CREATED_RUNNER_TOKEN_PREFIX, REGISTRATION_RUNNER_TOKEN_PREFIX or legacy_partition_id_prefix_in_16_bit_encode + legacy_partition_prefix = legacy_partition_id_prefix_in_16_bit_encode + start_index = authenticated_user_registration_type? ? CREATED_RUNNER_TOKEN_PREFIX.length : 0 + + if token[start_index..].start_with?(legacy_partition_prefix) + start_index += legacy_partition_prefix.length + else + start_index = REGISTRATION_RUNNER_TOKEN_PREFIX.length + end + token[start_index..start_index + RUNNER_SHORT_SHA_LENGTH - 1] end @@ -671,6 +679,13 @@ def no_allowed_plan_ids errors.add(:runner, 'cannot have allowed plans assigned') unless allowed_plan_ids.empty? end + def legacy_partition_id_prefix_in_16_bit_encode + # Prefix with t1 / t2 / t3 (`t` as in runner type, to allow us to easily detect how a token got prefixed). + # This is needed in order to ensure that tokens have unique values across partitions + # in the new ci_runners partitioned table. + "t#{partition_id.to_s(16)}_" + end + def prefix_for_new_and_legacy_runner return REGISTRATION_RUNNER_TOKEN_PREFIX if registration_token_registration_type? diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index d7bf83e5920260..8c00c6acb1dec4 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -1812,6 +1812,7 @@ def does_db_update specify { expect(runner.token).not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } + it { is_expected.not_to start_with('t1_') } it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } end @@ -1821,6 +1822,7 @@ def does_db_update specify { expect(runner.token).to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } + it { is_expected.not_to start_with('t1_') } it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } end -- GitLab From cacb666f9db52c0823d4ef12a3346667ff9129e7 Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Mon, 24 Mar 2025 14:15:00 +0100 Subject: [PATCH 5/8] Adds additional specs for legacy prefix --- spec/models/ci/runner_spec.rb | 44 ++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 8c00c6acb1dec4..427b5e048b81fa 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -1808,19 +1808,35 @@ def does_db_update subject(:short_sha) { runner.short_sha } context 'when registered via command-line' do - let_it_be(:runner) { create(:ci_runner) } + let(:runner) { create(:ci_runner) } + specify { expect(runner.token).to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } specify { expect(runner.token).not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } it { is_expected.not_to start_with('t1_') } it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } + + context 'with legacy token prefix' do + let(:legacy_partition_prefix) { 't1_' } + + before do + stub_const("#{described_class}::REGISTRATION_RUNNER_TOKEN_PREFIX", legacy_partition_prefix) + end + + specify { expect(runner.token).to start_with(legacy_partition_prefix) } + it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } + it { is_expected.not_to start_with(legacy_partition_prefix) } + it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } + it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } + end end context 'when creating new runner via UI' do let_it_be(:runner) { create(:ci_runner, registration_type: :authenticated_user) } specify { expect(runner.token).to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } + specify { expect(runner.token).not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } it { is_expected.not_to start_with('t1_') } it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } @@ -1942,6 +1958,32 @@ def does_db_update context 'when runner is project type' do it_behaves_like 'a project runner encrypted token', described_class::REGISTRATION_RUNNER_TOKEN_PREFIX end + + context 'with legacy token prefix' do + context 'when runner is instance type' do + before do + stub_const("#{described_class}::REGISTRATION_RUNNER_TOKEN_PREFIX", 't1_') + end + + it_behaves_like 'an instance runner encrypted token', 't1_' + end + + context 'when runner is group type' do + before do + stub_const("#{described_class}::REGISTRATION_RUNNER_TOKEN_PREFIX", 't2_') + end + + it_behaves_like 'a group runner encrypted token', 't2_' + end + + context 'when runner is project type' do + before do + stub_const("#{described_class}::REGISTRATION_RUNNER_TOKEN_PREFIX", 't3_') + end + + it_behaves_like 'a project runner encrypted token', 't3_' + end + end end context 'when runner is created via UI' do -- GitLab From 4d2368df2257a0aa37555569e2d0479e9a561db2 Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Mon, 24 Mar 2025 15:43:44 +0100 Subject: [PATCH 6/8] Remove unnecessary specs --- spec/models/ci/runner_spec.rb | 48 +++++++---------------------------- 1 file changed, 9 insertions(+), 39 deletions(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 427b5e048b81fa..0e3e2adc55928d 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -1808,7 +1808,7 @@ def does_db_update subject(:short_sha) { runner.short_sha } context 'when registered via command-line' do - let(:runner) { create(:ci_runner) } + let_it_be(:runner) { create(:ci_runner) } specify { expect(runner.token).to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } specify { expect(runner.token).not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } @@ -1816,20 +1816,16 @@ def does_db_update it { is_expected.not_to start_with('t1_') } it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } + end - context 'with legacy token prefix' do - let(:legacy_partition_prefix) { 't1_' } + context 'with legacy token prefix' do + let_it_be(:runner) { create(:ci_runner, token: 't1_deadbeaf') } - before do - stub_const("#{described_class}::REGISTRATION_RUNNER_TOKEN_PREFIX", legacy_partition_prefix) - end - - specify { expect(runner.token).to start_with(legacy_partition_prefix) } - it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } - it { is_expected.not_to start_with(legacy_partition_prefix) } - it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } - it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } - end + specify { expect(runner.token).to start_with('t1_') } + it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } + it { is_expected.not_to start_with('t1_') } + it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } + it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } end context 'when creating new runner via UI' do @@ -1958,32 +1954,6 @@ def does_db_update context 'when runner is project type' do it_behaves_like 'a project runner encrypted token', described_class::REGISTRATION_RUNNER_TOKEN_PREFIX end - - context 'with legacy token prefix' do - context 'when runner is instance type' do - before do - stub_const("#{described_class}::REGISTRATION_RUNNER_TOKEN_PREFIX", 't1_') - end - - it_behaves_like 'an instance runner encrypted token', 't1_' - end - - context 'when runner is group type' do - before do - stub_const("#{described_class}::REGISTRATION_RUNNER_TOKEN_PREFIX", 't2_') - end - - it_behaves_like 'a group runner encrypted token', 't2_' - end - - context 'when runner is project type' do - before do - stub_const("#{described_class}::REGISTRATION_RUNNER_TOKEN_PREFIX", 't3_') - end - - it_behaves_like 'a project runner encrypted token', 't3_' - end - end end context 'when runner is created via UI' do -- GitLab From 47f39767c9efa1b6dbee4e62d29e4abfd01e3344 Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Mon, 24 Mar 2025 18:29:25 +0100 Subject: [PATCH 7/8] Apply reviewer's feedback --- app/models/ci/runner.rb | 16 +++++++++------- spec/models/ci/runner_spec.rb | 28 +++++++++++++++++++++------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 484f31e9df7681..04e0979c54a57f 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -451,13 +451,15 @@ def short_sha # We want to show the first characters of the hash, so we need to bypass any fixed components of the token, such # as CREATED_RUNNER_TOKEN_PREFIX, REGISTRATION_RUNNER_TOKEN_PREFIX or legacy_partition_id_prefix_in_16_bit_encode legacy_partition_prefix = legacy_partition_id_prefix_in_16_bit_encode - start_index = authenticated_user_registration_type? ? CREATED_RUNNER_TOKEN_PREFIX.length : 0 - - if token[start_index..].start_with?(legacy_partition_prefix) - start_index += legacy_partition_prefix.length - else - start_index = REGISTRATION_RUNNER_TOKEN_PREFIX.length - end + start_index = if authenticated_user_registration_type? + CREATED_RUNNER_TOKEN_PREFIX.length + elsif token.start_with?(REGISTRATION_RUNNER_TOKEN_PREFIX) + REGISTRATION_RUNNER_TOKEN_PREFIX.length + else + 0 + end + + start_index += legacy_partition_prefix.length if token[start_index..].start_with?(legacy_partition_prefix) token[start_index..start_index + RUNNER_SHORT_SHA_LENGTH - 1] end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 0e3e2adc55928d..63d7e04c1b12e8 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -1818,14 +1818,28 @@ def does_db_update it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } end - context 'with legacy token prefix' do - let_it_be(:runner) { create(:ci_runner, token: 't1_deadbeaf') } + context 'when legacy token' do + context 'with legacy partition prefix' do + let_it_be(:runner) { create(:ci_runner, token: 't1_deadbeaf') } - specify { expect(runner.token).to start_with('t1_') } - it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } - it { is_expected.not_to start_with('t1_') } - it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } - it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } + specify { expect(runner.token).to start_with('t1_') } + it { is_expected.to eq('deadbeaf') } + end + + context 'with runner token prefix and legacy partition prefix' do + let_it_be(:runner) { create(:ci_runner, registration_type: :authenticated_user, token: 'glrt-t1_deadbeaf') } + + specify { expect(runner.token).to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } + it { is_expected.to eq('deadbeaf') } + end + + context 'without prefix' do + let_it_be(:runner) { create(:ci_runner, token: 'deadbeaf') } + + specify { expect(runner.token).not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } + specify { expect(runner.token).not_to start_with('t1_') } + it { is_expected.to eq('deadbeaf') } + end end context 'when creating new runner via UI' do -- GitLab From 21c2e37b8f74441927205474e55899deff4ee99c Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Tue, 25 Mar 2025 14:07:25 +0100 Subject: [PATCH 8/8] Removes redundant tests --- spec/models/ci/runner_spec.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 63d7e04c1b12e8..92f152fd0b368a 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -1813,7 +1813,6 @@ def does_db_update specify { expect(runner.token).to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } specify { expect(runner.token).not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } - it { is_expected.not_to start_with('t1_') } it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } end @@ -1822,22 +1821,18 @@ def does_db_update context 'with legacy partition prefix' do let_it_be(:runner) { create(:ci_runner, token: 't1_deadbeaf') } - specify { expect(runner.token).to start_with('t1_') } it { is_expected.to eq('deadbeaf') } end context 'with runner token prefix and legacy partition prefix' do let_it_be(:runner) { create(:ci_runner, registration_type: :authenticated_user, token: 'glrt-t1_deadbeaf') } - specify { expect(runner.token).to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } it { is_expected.to eq('deadbeaf') } end context 'without prefix' do let_it_be(:runner) { create(:ci_runner, token: 'deadbeaf') } - specify { expect(runner.token).not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } - specify { expect(runner.token).not_to start_with('t1_') } it { is_expected.to eq('deadbeaf') } end end @@ -1848,7 +1843,6 @@ def does_db_update specify { expect(runner.token).to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } specify { expect(runner.token).not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } it { is_expected.to match(/[0-9a-zA-Z_-]{8}/) } - it { is_expected.not_to start_with('t1_') } it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) } it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) } end -- GitLab