From c56e1911e5656e64fc0dac2a1048f844a88e8647 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Tue, 19 Aug 2025 15:31:00 +0300 Subject: [PATCH 01/15] Ensure that when creating a personal access token for human user we set `group_id` and `user_type` for the new token. --- .../personal_access_tokens/create_service.rb | 6 +++++ .../personal_access_tokens/create_service.rb | 6 +++++ .../create_service_spec.rb | 25 +++++++++++++++++++ .../create_service_spec.rb | 1 + 4 files changed, 38 insertions(+) diff --git a/app/services/personal_access_tokens/create_service.rb b/app/services/personal_access_tokens/create_service.rb index be7d9b931e5d9b..c31caaf864e4b1 100644 --- a/app/services/personal_access_tokens/create_service.rb +++ b/app/services/personal_access_tokens/create_service.rb @@ -35,6 +35,8 @@ def execute def personal_access_token_params { name: params[:name], + user_type: target_user.user_type, + group_id: group_id, impersonation: params[:impersonation] || false, scopes: params[:scopes], expires_at: pat_expiration, @@ -43,6 +45,10 @@ def personal_access_token_params } end + def group_id + # overridden in EE + end + def pat_expiration return params[:expires_at] if params[:expires_at].present? diff --git a/ee/app/services/ee/personal_access_tokens/create_service.rb b/ee/app/services/ee/personal_access_tokens/create_service.rb index ffa759762fe479..8bb255207aadf9 100644 --- a/ee/app/services/ee/personal_access_tokens/create_service.rb +++ b/ee/app/services/ee/personal_access_tokens/create_service.rb @@ -13,6 +13,12 @@ def execute private + def group_id + return target_user.enterprise_group_id if target_user.enterprise_user? + + super + end + def send_audit_event(response) message = if response.success? "Created personal access token with id #{response.payload[:personal_access_token].id}" diff --git a/ee/spec/services/ee/personal_access_tokens/create_service_spec.rb b/ee/spec/services/ee/personal_access_tokens/create_service_spec.rb index cbc1b7b297c345..8809fdb2e394fa 100644 --- a/ee/spec/services/ee/personal_access_tokens/create_service_spec.rb +++ b/ee/spec/services/ee/personal_access_tokens/create_service_spec.rb @@ -66,6 +66,7 @@ subject(:create_token) { service.execute } let(:target_user) { create(:user) } + let(:current_user) { target_user } let(:organization) { create(:organization) } let(:service) do described_class.new(current_user: current_user, target_user: target_user, @@ -291,5 +292,29 @@ it_behaves_like 'an unsuccessfully created token' end end + + context 'for group_id' do + let(:params) { valid_params } + + context 'when the user is an enterprise user' do + let_it_be(:target_user) { create(:enterprise_user) } + + it "creates personal access token record with group_id set to the user's enterprise_group_id" do + expect(target_user.enterprise_group_id).not_to be_nil + + expect(create_token.success?).to be true + expect(token.group_id).to eq(target_user.enterprise_group_id) + end + end + + context 'when the user is a regular user' do + let_it_be(:target_user) { create(:user) } + + it "creates personal access token record with group_id set to nil" do + expect(create_token.success?).to be true + expect(token.group_id).to be_nil + end + end + end end end diff --git a/spec/services/personal_access_tokens/create_service_spec.rb b/spec/services/personal_access_tokens/create_service_spec.rb index 014b610e05c1d8..f2caf3bfbe408c 100644 --- a/spec/services/personal_access_tokens/create_service_spec.rb +++ b/spec/services/personal_access_tokens/create_service_spec.rb @@ -13,6 +13,7 @@ expect(token.expires_at).to eq(params[:expires_at]) expect(token.organization).to eq(organization) expect(token.user).to eq(user) + expect(token.user_type).to eq(user.user_type) end it 'logs the event' do -- GitLab From 953e4f282cd2f1951f2aa9759cf3b77587a4ed78 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Tue, 19 Aug 2025 16:12:29 +0300 Subject: [PATCH 02/15] Ensure that when rotating a personal access token for human user we set `group_id` and `user_type` for the new token. --- .../personal_access_tokens/rotate_service.rb | 2 ++ .../rotate_service_spec.rb | 21 +++++++++++++++++++ .../rotate_service_spec.rb | 12 +++++++++++ 3 files changed, 35 insertions(+) diff --git a/app/services/personal_access_tokens/rotate_service.rb b/app/services/personal_access_tokens/rotate_service.rb index 54597dc14d2596..adbcfa3ba956fc 100644 --- a/app/services/personal_access_tokens/rotate_service.rb +++ b/app/services/personal_access_tokens/rotate_service.rb @@ -110,6 +110,8 @@ def error_response(message) def create_token_params { name: token.name, + user_type: token.user_type, + group_id: token.group_id, description: token.description, previous_personal_access_token_id: token.id, impersonation: token.impersonation, diff --git a/ee/spec/services/ee/personal_access_tokens/rotate_service_spec.rb b/ee/spec/services/ee/personal_access_tokens/rotate_service_spec.rb index 0337d9a4765d00..c9f92bf3887c5f 100644 --- a/ee/spec/services/ee/personal_access_tokens/rotate_service_spec.rb +++ b/ee/spec/services/ee/personal_access_tokens/rotate_service_spec.rb @@ -88,4 +88,25 @@ expect(new_token.user).to eq(token.user) end end + + context "for enterprise_user's token" do + let_it_be(:current_user) { create(:enterprise_user) } + let_it_be(:token, reload: true) do + create(:personal_access_token, user: current_user, group: current_user.enterprise_group) + end + + it "rotates user's own token" do + expect(response).to be_success + + new_token = response.payload[:personal_access_token] + + expect(new_token.token).not_to eq(token.token) + expect(new_token.user).to eq(token.user) + expect(new_token.user_type).to eq(token.user_type) + expect(new_token.group_id).to eq(token.group_id) + expect(new_token.user.namespace).to eq(token.user.namespace) + expect(new_token.organization).to eq(token.organization) + expect(new_token.description).to eq(token.description) + end + end end diff --git a/spec/services/personal_access_tokens/rotate_service_spec.rb b/spec/services/personal_access_tokens/rotate_service_spec.rb index d6684c98fdd664..dfd40d810f2b73 100644 --- a/spec/services/personal_access_tokens/rotate_service_spec.rb +++ b/spec/services/personal_access_tokens/rotate_service_spec.rb @@ -22,6 +22,8 @@ expect(new_token.token).not_to eq(token.token) expect(new_token.expires_at).to eq(Time.zone.today + 1.week) expect(new_token.user).to eq(token.user) + expect(new_token.user_type).to eq(token.user_type) + expect(new_token.group_id).to eq(token.group_id) expect(new_token.user.namespace).to eq(token.user.namespace) expect(new_token.organization).to eq(token.organization) expect(new_token.description).to eq(token.description) @@ -154,5 +156,15 @@ end end end + + context "for group service account's token" do + let_it_be(:group) { create(:group) } + let_it_be(:current_user) { create(:user, :service_account, provisioned_by_group: group) } + let_it_be(:token, reload: true) do + create(:personal_access_token, user: current_user, group: group, expires_at: Time.zone.today + 30.days) + end + + it_behaves_like "rotates token successfully" + end end end -- GitLab From 3fcb32f0f89b0159632137ac6aaca9f1f6bfc5ff Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Wed, 20 Aug 2025 13:56:32 +0300 Subject: [PATCH 03/15] Ensure that `Groups::EnterpriseUsers::AssociateService` that claims a user, sets `group_id` to the group ID for the user's personal_access_tokens. --- .../enterprise_users/associate_service.rb | 6 ++- .../associate_service_spec.rb | 38 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/ee/app/services/groups/enterprise_users/associate_service.rb b/ee/app/services/groups/enterprise_users/associate_service.rb index 071b9b565b644f..f159b74e2aaa51 100644 --- a/ee/app/services/groups/enterprise_users/associate_service.rb +++ b/ee/app/services/groups/enterprise_users/associate_service.rb @@ -21,7 +21,11 @@ def execute # Allows the raising of persistent failure and enables it to be retried when called from inside sidekiq. # see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130735#note_1550114699 - @user.update!(user_attributes) + User.transaction do + @user.update!(user_attributes) + + @user.personal_access_tokens.update_all(group_id: @user.enterprise_group_id) + end Notify.user_associated_with_enterprise_group_email(user.id).deliver_later diff --git a/ee/spec/services/groups/enterprise_users/associate_service_spec.rb b/ee/spec/services/groups/enterprise_users/associate_service_spec.rb index a97864ba7d7c10..d1d84c9587043c 100644 --- a/ee/spec/services/groups/enterprise_users/associate_service_spec.rb +++ b/ee/spec/services/groups/enterprise_users/associate_service_spec.rb @@ -22,6 +22,12 @@ let(:user) { create(:user, created_at: user_created_at) } + let(:user_personal_access_token1) { create(:personal_access_token, user: user) } + let(:user_personal_access_token2) { create(:personal_access_token, :expired, user: user) } + let(:user_personal_access_token3) { create(:personal_access_token, :revoked, user: user) } + + let(:another_user_personal_access_token) { create(:personal_access_token) } + subject(:service) { described_class.new(group: group, user: user) } describe '#execute' do @@ -56,6 +62,22 @@ expect(user.user_detail.enterprise_group_associated_at).to eq(Time.current) end + it 'sets group_id for user.personal_access_tokens', :aggregate_failures do + expect(user_personal_access_token1.group_id).to be_nil + expect(user_personal_access_token2.group_id).to be_nil + expect(user_personal_access_token3.group_id).to be_nil + + expect(another_user_personal_access_token.group_id).to be_nil + + service.execute + + expect(user_personal_access_token1.reload.group_id).to eq(group.id) + expect(user_personal_access_token2.reload.group_id).to eq(group.id) + expect(user_personal_access_token3.reload.group_id).to eq(group.id) + + expect(another_user_personal_access_token.reload.group_id).to be_nil + end + it 'enqueues user_associated_with_enterprise_group_email email for later delivery to the user' do expect do service.execute @@ -164,6 +186,22 @@ expect(user.user_detail.enterprise_group_associated_at).to eq(previous_enterprise_group_associated_at) end + it 'does not update group_id for user.personal_access_tokens', :aggregate_failures do + expect(user_personal_access_token1.group_id).to be_nil + expect(user_personal_access_token2.group_id).to be_nil + expect(user_personal_access_token3.group_id).to be_nil + + expect(another_user_personal_access_token.group_id).to be_nil + + service.execute + + expect(user_personal_access_token1.reload.group_id).to be_nil + expect(user_personal_access_token2.reload.group_id).to be_nil + expect(user_personal_access_token3.reload.group_id).to be_nil + + expect(another_user_personal_access_token.reload.group_id).to be_nil + end + it 'does not enqueue any email for later delivery' do expect do service.execute -- GitLab From 3b93ea0618a9824c6cca6d8cfacddbdf4441d4dc Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Wed, 20 Aug 2025 16:57:47 +0300 Subject: [PATCH 04/15] Ensure that `Groups::EnterpriseUsers::DisassociateService` that disassociates a user from the group, sets `group_id` to NULL for the user's personal_access_tokens. --- .../enterprise_users/disassociate_service.rb | 8 ++- .../disassociate_service_spec.rb | 60 +++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/ee/app/services/groups/enterprise_users/disassociate_service.rb b/ee/app/services/groups/enterprise_users/disassociate_service.rb index 4399bf8171ccab..f9b5bdff72e3e9 100644 --- a/ee/app/services/groups/enterprise_users/disassociate_service.rb +++ b/ee/app/services/groups/enterprise_users/disassociate_service.rb @@ -17,7 +17,13 @@ def execute return error('The user matches the "Enterprise User" definition for the group') end - @user.user_detail.update!(enterprise_group_id: nil, enterprise_group_associated_at: nil) + # Allows the raising of persistent failure and enables it to be retried when called from inside sidekiq. + # see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130735#note_1550114699 + User.transaction do + @user.user_detail.update!(enterprise_group_id: nil, enterprise_group_associated_at: nil) + + @user.personal_access_tokens.update_all(group_id: nil) + end log_info(message: 'Disassociated the user from the enterprise group') diff --git a/ee/spec/services/groups/enterprise_users/disassociate_service_spec.rb b/ee/spec/services/groups/enterprise_users/disassociate_service_spec.rb index 876c9745089e01..7b6df934f0c343 100644 --- a/ee/spec/services/groups/enterprise_users/disassociate_service_spec.rb +++ b/ee/spec/services/groups/enterprise_users/disassociate_service_spec.rb @@ -5,6 +5,22 @@ RSpec.describe Groups::EnterpriseUsers::DisassociateService, :saas, feature_category: :user_management do subject(:service) { described_class.new(user: user) } + let(:user_personal_access_token1) do + create(:personal_access_token, user: user, group_id: user.enterprise_group_id) + end + + let(:user_personal_access_token2) do + create(:personal_access_token, :expired, user: user, group_id: user.enterprise_group_id) + end + + let(:user_personal_access_token3) do + create(:personal_access_token, :revoked, user: user, group_id: user.enterprise_group_id) + end + + let(:another_user_personal_access_token) do + create(:personal_access_token, group_id: create(:group).id) + end + describe '#execute' do shared_examples 'disassociates the user from the enterprise group' do it 'returns a successful response', :aggregate_failures do @@ -35,6 +51,22 @@ expect(user.user_detail.enterprise_group_associated_at).to eq(nil) end + it 'sets group_id for user.personal_access_tokens to nil', :aggregate_failures do + expect(user_personal_access_token1.group_id).not_to be_nil + expect(user_personal_access_token2.group_id).not_to be_nil + expect(user_personal_access_token3.group_id).not_to be_nil + + expect(another_user_personal_access_token.group_id).not_to be_nil + + service.execute + + expect(user_personal_access_token1.reload.group_id).to be_nil + expect(user_personal_access_token2.reload.group_id).to be_nil + expect(user_personal_access_token3.reload.group_id).to be_nil + + expect(another_user_personal_access_token.reload.group_id).not_to be_nil + end + it 'logs message with info level about disassociating the user from the enterprise group' do allow(Gitlab::AppLogger).to receive(:info) @@ -92,6 +124,34 @@ expect(user.user_detail.enterprise_group_associated_at).to eq(previous_enterprise_group_associated_at) end + it 'does not update group_id for user.personal_access_tokens to nil', :aggregate_failures do + if user.enterprise_user? + expect(user_personal_access_token1.group_id).not_to be_nil + expect(user_personal_access_token2.group_id).not_to be_nil + expect(user_personal_access_token3.group_id).not_to be_nil + else + expect(user_personal_access_token1.group_id).to be_nil + expect(user_personal_access_token2.group_id).to be_nil + expect(user_personal_access_token3.group_id).to be_nil + end + + expect(another_user_personal_access_token.group_id).not_to be_nil + + service.execute + + if user.enterprise_user? + expect(user_personal_access_token1.reload.group_id).not_to be_nil + expect(user_personal_access_token2.reload.group_id).not_to be_nil + expect(user_personal_access_token3.reload.group_id).not_to be_nil + else + expect(user_personal_access_token1.group_id).to be_nil + expect(user_personal_access_token2.group_id).to be_nil + expect(user_personal_access_token3.group_id).to be_nil + end + + expect(another_user_personal_access_token.reload.group_id).not_to be_nil + end + it 'does not log any message with info level' do expect(Gitlab::AppLogger).not_to receive(:info).with( message: 'Disassociated the user from the enterprise group' -- GitLab From 3e33527677a6dc03d098a1882317015531f0c99d Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Thu, 21 Aug 2025 15:49:09 +0300 Subject: [PATCH 05/15] Add DB migration to set `group_id` based on `user_details.enterprise_group_id` and `user_type` based on `users.user_type` for existing human users' personal_access_tokens. --- ...for_human_users_personal_access_tokens.yml | 9 + ..._for_human_users_personal_access_tokens.rb | 27 ++ db/schema_migrations/20250821112501 | 1 + ..._for_human_users_personal_access_tokens.rb | 29 ++ ...human_users_personal_access_tokens_spec.rb | 295 ++++++++++++++++++ ...human_users_personal_access_tokens_spec.rb | 27 ++ 6 files changed, 388 insertions(+) create mode 100644 db/docs/batched_background_migrations/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.yml create mode 100644 db/post_migrate/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb create mode 100644 db/schema_migrations/20250821112501 create mode 100644 lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb create mode 100644 spec/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb create mode 100644 spec/migrations/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb diff --git a/db/docs/batched_background_migrations/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.yml b/db/docs/batched_background_migrations/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.yml new file mode 100644 index 00000000000000..674bd20139986c --- /dev/null +++ b/db/docs/batched_background_migrations/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.yml @@ -0,0 +1,9 @@ +--- +migration_job_name: BackfillGroupIdAndUserTypeForHumanUsersPersonalAccessTokens +description: Backfills `group_id` based on `user_details.enterprise_group_id` and + `user_type` based on `users.user_type` for existing human users' personal_access_tokens. +feature_category: system_access +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/201921 +milestone: '18.4' +queued_migration_version: 20250821112501 +finalized_by: # version of the migration that finalized this BBM diff --git a/db/post_migrate/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb b/db/post_migrate/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb new file mode 100644 index 00000000000000..79029b8073ec24 --- /dev/null +++ b/db/post_migrate/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class QueueBackfillGroupIdAndUserTypeForHumanUsersPersonalAccessTokens < Gitlab::Database::Migration[2.3] + milestone '18.4' + + restrict_gitlab_migration gitlab_schema: :gitlab_main + + MIGRATION = "BackfillGroupIdAndUserTypeForHumanUsersPersonalAccessTokens" + DELAY_INTERVAL = 2.minutes + BATCH_SIZE = 5000 + SUB_BATCH_SIZE = 100 + + def up + queue_batched_background_migration( + MIGRATION, + :users, + :id, + job_interval: DELAY_INTERVAL, + batch_size: BATCH_SIZE, + sub_batch_size: SUB_BATCH_SIZE + ) + end + + def down + delete_batched_background_migration(MIGRATION, :users, :id, []) + end +end diff --git a/db/schema_migrations/20250821112501 b/db/schema_migrations/20250821112501 new file mode 100644 index 00000000000000..14979a8dd02eb2 --- /dev/null +++ b/db/schema_migrations/20250821112501 @@ -0,0 +1 @@ +f233834dd1240799da46750f0157107c68fcd3f6af9a44fe03db6c6a15f79a09 \ No newline at end of file diff --git a/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb b/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb new file mode 100644 index 00000000000000..daa79fba3ccd7f --- /dev/null +++ b/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + class BackfillGroupIdAndUserTypeForHumanUsersPersonalAccessTokens < BatchedMigrationJob + operation_name :backfill_group_id_and_user_type_for_human_users_personal_access_tokens + scope_to ->(relation) { relation.where(user_type: 0) } # rubocop: disable Database/AvoidScopeTo -- `user_type` is an indexed column + feature_category :system_access + + def perform + each_sub_batch do |sub_batch| + connection.execute( + <<~SQL + UPDATE personal_access_tokens + SET user_type=users.user_type, group_id=user_details.enterprise_group_id + FROM + users + LEFT JOIN user_details ON user_details.user_id=users.id + WHERE + personal_access_tokens.user_id=users.id + AND personal_access_tokens.user_type IS NULL + AND users.id IN (#{sub_batch.select(:id).to_sql}) + SQL + ) + end + end + end + end +end diff --git a/spec/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb b/spec/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb new file mode 100644 index 00000000000000..66bbc327c1be43 --- /dev/null +++ b/spec/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb @@ -0,0 +1,295 @@ +# frozen_string_literal: true + +require 'spec_helper' + +# rubocop:disable RSpec/MultipleMemoizedHelpers -- We need this many for this background migration +RSpec.describe( + Gitlab::BackgroundMigration::BackfillGroupIdAndUserTypeForHumanUsersPersonalAccessTokens, + feature_category: :system_access +) do + subject(:migration) do + described_class.new( + batch_table: :users, + batch_column: :id, + sub_batch_size: 100, + pause_ms: 100, + connection: ApplicationRecord.connection + ) + end + + let(:organizations) { table(:organizations) } + let(:namespaces) { table(:namespaces) } + let(:users) { table(:users) } + let(:user_details) { table(:user_details) } + let(:personal_access_tokens) { table(:personal_access_tokens) } + + let!(:organization) { organizations.create!(name: 'organization', path: 'organization') } + + let(:group1) { namespaces.create!(name: 'group1', path: 'group1', type: 'Group1', organization_id: organization.id) } + let(:group2) { namespaces.create!(name: 'group2', path: 'group2', type: 'Group2', organization_id: organization.id) } + + let!(:human_user) do + users.create!( + username: 'human_user', + email: 'human_user@example.com', + user_type: 0, + projects_limit: 10, + organization_id: organization.id + ) + end + + let!(:human_user_details) do + user_details.create!( + user_id: human_user.id + ) + end + + let!(:human_user_personal_access_token1) do + personal_access_tokens.create!( + name: 'human_user_personal_access_token1', + user_id: human_user.id, + organization_id: human_user.organization_id + ) + end + + let!(:human_user_personal_access_token2) do + personal_access_tokens.create!( + name: 'human_user_personal_access_token2', + user_id: human_user.id, + organization_id: human_user.organization_id + ) + end + + let!(:human_user_without_user_details) do + users.create!( + username: 'human_user_without_user_details', + email: 'human_user_without_user_details@example.com', + user_type: 0, + projects_limit: 10, + organization_id: organization.id + ) + end + + let!(:human_user_without_user_details_personal_access_token1) do + personal_access_tokens.create!( + name: 'human_user_without_user_details_personal_access_token1', + user_id: human_user_without_user_details.id, + organization_id: human_user_without_user_details.organization_id + ) + end + + let!(:human_user_without_user_details_personal_access_token2) do + personal_access_tokens.create!( + name: 'human_user_without_user_details_personal_access_token2', + user_id: human_user_without_user_details.id, + organization_id: human_user_without_user_details.organization_id + ) + end + + let!(:enterprise_user1) do + users.create!( + username: 'enterprise_user1', + email: 'enterprise_user1@example.com', + user_type: 0, + projects_limit: 10, + organization_id: organization.id + ) + end + + let!(:enterprise_user1_details) do + user_details.create!( + user_id: enterprise_user1.id, + enterprise_group_id: group1.id + ) + end + + let!(:enterprise_user1_personal_access_token1) do + personal_access_tokens.create!( + name: 'enterprise_user1_personal_access_token1', + user_id: enterprise_user1.id, + organization_id: enterprise_user1.organization_id + ) + end + + let!(:enterprise_user1_personal_access_token2) do + personal_access_tokens.create!( + name: 'enterprise_user1_personal_access_token2', + user_id: enterprise_user1.id, + organization_id: enterprise_user1.organization_id + ) + end + + let!(:enterprise_user2) do + users.create!( + username: 'enterprise_user2', + email: 'enterprise_user2@example.com', + user_type: 0, + projects_limit: 10, + organization_id: organization.id + ) + end + + let!(:enterprise_user2_details) do + user_details.create!( + user_id: enterprise_user2.id, + enterprise_group_id: group2.id + ) + end + + let!(:enterprise_user2_personal_access_token1) do + personal_access_tokens.create!( + name: 'enterprise_user2_personal_access_token1', + user_id: enterprise_user2.id, + organization_id: enterprise_user2.organization_id + ) + end + + let!(:enterprise_user2_personal_access_token2) do + personal_access_tokens.create!( + name: 'enterprise_user2_personal_access_token2', + user_id: enterprise_user2.id, + organization_id: enterprise_user2.organization_id + ) + end + + let!(:project_bot_user) do + users.create!( + username: 'project_bot_user', + email: 'project_bot_user@example.com', + user_type: 6, + projects_limit: 10, + organization_id: organization.id + ) + end + + let!(:project_bot_user_details) do + user_details.create!( + user_id: project_bot_user.id + ) + end + + let!(:project_bot_user_personal_access_token1) do + personal_access_tokens.create!( + name: 'project_bot_user_personal_access_token1', + user_id: project_bot_user.id, + organization_id: project_bot_user.organization_id + ) + end + + let!(:project_bot_user_personal_access_token2) do + personal_access_tokens.create!( + name: 'project_bot_user_personal_access_token2', + user_id: project_bot_user.id, + organization_id: project_bot_user.organization_id + ) + end + + let!(:service_account_user) do + users.create!( + username: 'service_account_user', + email: 'service_account_user@example.com', + user_type: 13, + projects_limit: 10, + organization_id: organization.id + ) + end + + let!(:service_account_user_details) do + user_details.create!( + user_id: service_account_user.id + ) + end + + let!(:service_account_user_personal_access_token1) do + personal_access_tokens.create!( + name: 'service_account_user_personal_access_token1', + user_id: service_account_user.id, + organization_id: service_account_user.organization_id + ) + end + + let!(:service_account_user_personal_access_token2) do + personal_access_tokens.create!( + name: 'service_account_user_personal_access_token2', + user_id: service_account_user.id, + organization_id: service_account_user.organization_id + ) + end + + it "backfills group_id and user_type for human users' personal_access_tokens", :aggregate_failures do + expect(human_user_personal_access_token1.group_id).to be_nil + expect(human_user_personal_access_token1.user_type).to be_nil + + expect(human_user_personal_access_token2.group_id).to be_nil + expect(human_user_personal_access_token2.user_type).to be_nil + + expect(human_user_without_user_details_personal_access_token1.group_id).to be_nil + expect(human_user_without_user_details_personal_access_token1.user_type).to be_nil + + expect(human_user_without_user_details_personal_access_token2.group_id).to be_nil + expect(human_user_without_user_details_personal_access_token2.user_type).to be_nil + + expect(enterprise_user1_personal_access_token1.group_id).to be_nil + expect(enterprise_user1_personal_access_token1.user_type).to be_nil + + expect(enterprise_user1_personal_access_token2.group_id).to be_nil + expect(enterprise_user1_personal_access_token2.user_type).to be_nil + + expect(enterprise_user2_personal_access_token1.group_id).to be_nil + expect(enterprise_user2_personal_access_token1.user_type).to be_nil + + expect(enterprise_user2_personal_access_token2.group_id).to be_nil + expect(enterprise_user2_personal_access_token2.user_type).to be_nil + + expect(project_bot_user_personal_access_token1.group_id).to be_nil + expect(project_bot_user_personal_access_token1.user_type).to be_nil + + expect(project_bot_user_personal_access_token2.group_id).to be_nil + expect(project_bot_user_personal_access_token2.user_type).to be_nil + + expect(service_account_user_personal_access_token1.group_id).to be_nil + expect(service_account_user_personal_access_token1.user_type).to be_nil + + expect(service_account_user_personal_access_token2.group_id).to be_nil + expect(service_account_user_personal_access_token2.user_type).to be_nil + + migration.perform + + expect(human_user_personal_access_token1.reload.group_id).to be_nil + expect(human_user_personal_access_token1.reload.user_type).to eq(0) + + expect(human_user_personal_access_token2.reload.group_id).to be_nil + expect(human_user_personal_access_token2.reload.user_type).to eq(0) + + expect(human_user_without_user_details_personal_access_token1.reload.group_id).to be_nil + expect(human_user_without_user_details_personal_access_token1.reload.user_type).to eq(0) + + expect(human_user_without_user_details_personal_access_token2.reload.group_id).to be_nil + expect(human_user_without_user_details_personal_access_token2.reload.user_type).to eq(0) + + expect(enterprise_user1_personal_access_token1.reload.group_id).to eq(group1.id) + expect(enterprise_user1_personal_access_token1.reload.user_type).to eq(0) + + expect(enterprise_user1_personal_access_token2.reload.group_id).to eq(group1.id) + expect(enterprise_user1_personal_access_token2.reload.user_type).to eq(0) + + expect(enterprise_user2_personal_access_token1.reload.group_id).to eq(group2.id) + expect(enterprise_user2_personal_access_token1.reload.user_type).to eq(0) + + expect(enterprise_user2_personal_access_token2.reload.group_id).to eq(group2.id) + expect(enterprise_user2_personal_access_token2.reload.user_type).to eq(0) + + expect(project_bot_user_personal_access_token1.reload.group_id).to be_nil + expect(project_bot_user_personal_access_token1.reload.user_type).to be_nil + + expect(project_bot_user_personal_access_token2.reload.group_id).to be_nil + expect(project_bot_user_personal_access_token2.reload.user_type).to be_nil + + expect(service_account_user_personal_access_token1.reload.group_id).to be_nil + expect(service_account_user_personal_access_token1.reload.user_type).to be_nil + + expect(service_account_user_personal_access_token2.reload.group_id).to be_nil + expect(service_account_user_personal_access_token2.reload.user_type).to be_nil + end +end +# rubocop:enable RSpec/MultipleMemoizedHelpers diff --git a/spec/migrations/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb b/spec/migrations/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb new file mode 100644 index 00000000000000..f38092c7a4c586 --- /dev/null +++ b/spec/migrations/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe QueueBackfillGroupIdAndUserTypeForHumanUsersPersonalAccessTokens, migration: :gitlab_main, feature_category: :system_access do + let!(:batched_migration) { described_class::MIGRATION } + + it 'schedules a new batched migration' do + reversible_migration do |migration| + migration.before -> { + expect(batched_migration).not_to have_scheduled_batched_migration + } + + migration.after -> { + expect(batched_migration).to have_scheduled_batched_migration( + gitlab_schema: :gitlab_main, + table_name: :users, + column_name: :id, + interval: described_class::DELAY_INTERVAL, + batch_size: described_class::BATCH_SIZE, + sub_batch_size: described_class::SUB_BATCH_SIZE + ) + } + end + end +end -- GitLab From 69e7272050c19f0bb66287f32989bdb7c00b1617 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Mon, 25 Aug 2025 15:25:31 +0300 Subject: [PATCH 06/15] Batch the user's personal access tokens updates in Groups::EnterpriseUsers::AssociateService --- .../enterprise_users/associate_service.rb | 17 ++++++++- .../associate_service_spec.rb | 35 ++++++++++++------- 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/ee/app/services/groups/enterprise_users/associate_service.rb b/ee/app/services/groups/enterprise_users/associate_service.rb index f159b74e2aaa51..3371b834141af9 100644 --- a/ee/app/services/groups/enterprise_users/associate_service.rb +++ b/ee/app/services/groups/enterprise_users/associate_service.rb @@ -24,7 +24,22 @@ def execute User.transaction do @user.update!(user_attributes) - @user.personal_access_tokens.update_all(group_id: @user.enterprise_group_id) + loop do + # rubocop:disable CodeReuse/ActiveRecord -- required to batch the update + update_count = @user.personal_access_tokens + .where(group_id: nil) + .or( + # For the use case where an organization migrates their enterprise users + # from one group to another group. + @user.personal_access_tokens + .where.not(group_id: @user.enterprise_group_id) + ) + .limit(100) + .update_all(group_id: @user.enterprise_group_id) + # rubocop:enable CodeReuse/ActiveRecord -- required to batch the update + + break if update_count == 0 + end end Notify.user_associated_with_enterprise_group_email(user.id).deliver_later diff --git a/ee/spec/services/groups/enterprise_users/associate_service_spec.rb b/ee/spec/services/groups/enterprise_users/associate_service_spec.rb index d1d84c9587043c..6d45b6b201a00b 100644 --- a/ee/spec/services/groups/enterprise_users/associate_service_spec.rb +++ b/ee/spec/services/groups/enterprise_users/associate_service_spec.rb @@ -22,9 +22,15 @@ let(:user) { create(:user, created_at: user_created_at) } - let(:user_personal_access_token1) { create(:personal_access_token, user: user) } - let(:user_personal_access_token2) { create(:personal_access_token, :expired, user: user) } - let(:user_personal_access_token3) { create(:personal_access_token, :revoked, user: user) } + let(:user_personal_access_token1) { create(:personal_access_token, user: user, group_id: user.enterprise_group_id) } + + let(:user_personal_access_token2) do + create(:personal_access_token, :expired, user: user, group_id: user.enterprise_group_id) + end + + let(:user_personal_access_token3) do + create(:personal_access_token, :revoked, user: user, group_id: user.enterprise_group_id) + end let(:another_user_personal_access_token) { create(:personal_access_token) } @@ -63,9 +69,12 @@ end it 'sets group_id for user.personal_access_tokens', :aggregate_failures do - expect(user_personal_access_token1.group_id).to be_nil - expect(user_personal_access_token2.group_id).to be_nil - expect(user_personal_access_token3.group_id).to be_nil + previous_enterprise_group_id = user.user_detail.enterprise_group_id + expect(previous_enterprise_group_id).not_to eq(group.id) + + expect(user_personal_access_token1.group_id).to eq(previous_enterprise_group_id) + expect(user_personal_access_token2.group_id).to eq(previous_enterprise_group_id) + expect(user_personal_access_token3.group_id).to eq(previous_enterprise_group_id) expect(another_user_personal_access_token.group_id).to be_nil @@ -187,17 +196,19 @@ end it 'does not update group_id for user.personal_access_tokens', :aggregate_failures do - expect(user_personal_access_token1.group_id).to be_nil - expect(user_personal_access_token2.group_id).to be_nil - expect(user_personal_access_token3.group_id).to be_nil + previous_enterprise_group_id = user.user_detail.enterprise_group_id + + expect(user_personal_access_token1.group_id).to eq(previous_enterprise_group_id) + expect(user_personal_access_token2.group_id).to eq(previous_enterprise_group_id) + expect(user_personal_access_token3.group_id).to eq(previous_enterprise_group_id) expect(another_user_personal_access_token.group_id).to be_nil service.execute - expect(user_personal_access_token1.reload.group_id).to be_nil - expect(user_personal_access_token2.reload.group_id).to be_nil - expect(user_personal_access_token3.reload.group_id).to be_nil + expect(user_personal_access_token1.reload.group_id).to eq(previous_enterprise_group_id) + expect(user_personal_access_token2.reload.group_id).to eq(previous_enterprise_group_id) + expect(user_personal_access_token3.reload.group_id).to eq(previous_enterprise_group_id) expect(another_user_personal_access_token.reload.group_id).to be_nil end -- GitLab From 916e8ab2c86687fb5e176b42952458d228027fe8 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Tue, 26 Aug 2025 12:08:10 +0300 Subject: [PATCH 07/15] Batch the user's personal access tokens updates in Groups::EnterpriseUsers::DisassociateService --- .../enterprise_users/disassociate_service.rb | 11 +++++- .../disassociate_service_spec.rb | 35 ++++++++----------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/ee/app/services/groups/enterprise_users/disassociate_service.rb b/ee/app/services/groups/enterprise_users/disassociate_service.rb index f9b5bdff72e3e9..fc1bc07a4b8864 100644 --- a/ee/app/services/groups/enterprise_users/disassociate_service.rb +++ b/ee/app/services/groups/enterprise_users/disassociate_service.rb @@ -22,7 +22,16 @@ def execute User.transaction do @user.user_detail.update!(enterprise_group_id: nil, enterprise_group_associated_at: nil) - @user.personal_access_tokens.update_all(group_id: nil) + loop do + # rubocop:disable CodeReuse/ActiveRecord -- required to batch the update + update_count = @user.personal_access_tokens + .where.not(group_id: nil) + .limit(100) + .update_all(group_id: nil) + # rubocop:enable CodeReuse/ActiveRecord -- required to batch the update + + break if update_count == 0 + end end log_info(message: 'Disassociated the user from the enterprise group') diff --git a/ee/spec/services/groups/enterprise_users/disassociate_service_spec.rb b/ee/spec/services/groups/enterprise_users/disassociate_service_spec.rb index 7b6df934f0c343..8e81173d1cb453 100644 --- a/ee/spec/services/groups/enterprise_users/disassociate_service_spec.rb +++ b/ee/spec/services/groups/enterprise_users/disassociate_service_spec.rb @@ -52,9 +52,12 @@ end it 'sets group_id for user.personal_access_tokens to nil', :aggregate_failures do - expect(user_personal_access_token1.group_id).not_to be_nil - expect(user_personal_access_token2.group_id).not_to be_nil - expect(user_personal_access_token3.group_id).not_to be_nil + previous_enterprise_group_id = user.user_detail.enterprise_group_id + expect(previous_enterprise_group_id).not_to be_nil + + expect(user_personal_access_token1.group_id).to eq(previous_enterprise_group_id) + expect(user_personal_access_token2.group_id).to eq(previous_enterprise_group_id) + expect(user_personal_access_token3.group_id).to eq(previous_enterprise_group_id) expect(another_user_personal_access_token.group_id).not_to be_nil @@ -125,29 +128,19 @@ end it 'does not update group_id for user.personal_access_tokens to nil', :aggregate_failures do - if user.enterprise_user? - expect(user_personal_access_token1.group_id).not_to be_nil - expect(user_personal_access_token2.group_id).not_to be_nil - expect(user_personal_access_token3.group_id).not_to be_nil - else - expect(user_personal_access_token1.group_id).to be_nil - expect(user_personal_access_token2.group_id).to be_nil - expect(user_personal_access_token3.group_id).to be_nil - end + previous_enterprise_group_id = user.user_detail.enterprise_group_id + + expect(user_personal_access_token1.group_id).to eq(previous_enterprise_group_id) + expect(user_personal_access_token2.group_id).to eq(previous_enterprise_group_id) + expect(user_personal_access_token3.group_id).to eq(previous_enterprise_group_id) expect(another_user_personal_access_token.group_id).not_to be_nil service.execute - if user.enterprise_user? - expect(user_personal_access_token1.reload.group_id).not_to be_nil - expect(user_personal_access_token2.reload.group_id).not_to be_nil - expect(user_personal_access_token3.reload.group_id).not_to be_nil - else - expect(user_personal_access_token1.group_id).to be_nil - expect(user_personal_access_token2.group_id).to be_nil - expect(user_personal_access_token3.group_id).to be_nil - end + expect(user_personal_access_token1.reload.group_id).to eq(previous_enterprise_group_id) + expect(user_personal_access_token2.reload.group_id).to eq(previous_enterprise_group_id) + expect(user_personal_access_token3.reload.group_id).to eq(previous_enterprise_group_id) expect(another_user_personal_access_token.reload.group_id).not_to be_nil end -- GitLab From ab05a3146e982f50fe7e9872118bc0d817f21d9e Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Tue, 26 Aug 2025 20:26:51 +0300 Subject: [PATCH 08/15] Batch the user's personal access tokens updates in Gitlab::BackgroundMigration::BackfillGroupIdAndUserTypeForHumanUsersPersonalAccessTokens --- ..._for_human_users_personal_access_tokens.rb | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb b/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb index daa79fba3ccd7f..96815e298d0ae7 100644 --- a/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb +++ b/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb @@ -9,19 +9,28 @@ class BackfillGroupIdAndUserTypeForHumanUsersPersonalAccessTokens < BatchedMigra def perform each_sub_batch do |sub_batch| - connection.execute( - <<~SQL - UPDATE personal_access_tokens - SET user_type=users.user_type, group_id=user_details.enterprise_group_id - FROM - users - LEFT JOIN user_details ON user_details.user_id=users.id - WHERE - personal_access_tokens.user_id=users.id - AND personal_access_tokens.user_type IS NULL - AND users.id IN (#{sub_batch.select(:id).to_sql}) - SQL - ) + loop do + result = connection.execute( + <<~SQL + WITH personal_access_tokens_cte AS MATERIALIZED ( + SELECT personal_access_tokens.id AS id, users.user_type AS user_type, user_details.enterprise_group_id AS enterprise_group_id + FROM users + LEFT JOIN user_details ON user_details.user_id=users.id + INNER JOIN personal_access_tokens ON personal_access_tokens.user_id=users.id + WHERE + users.id IN (#{sub_batch.select(:id).to_sql}) + AND personal_access_tokens.user_type IS NULL + LIMIT 100 + ) + UPDATE personal_access_tokens + SET user_type=personal_access_tokens_cte.user_type, group_id=personal_access_tokens_cte.enterprise_group_id + FROM personal_access_tokens_cte + WHERE personal_access_tokens.id=personal_access_tokens_cte.id + SQL + ) + + break if result.cmd_tuples == 0 + end end end end -- GitLab From c260ebbc0b18474abab48358cbbab5e19ea88a78 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Wed, 27 Aug 2025 13:27:25 +0300 Subject: [PATCH 09/15] Remove transaction in Groups::EnterpriseUsers::AssociateService and Groups::EnterpriseUsers::DisassociateService --- .../enterprise_users/associate_service.rb | 41 ++++++++++--------- .../enterprise_users/disassociate_service.rb | 31 +++++++------- 2 files changed, 38 insertions(+), 34 deletions(-) diff --git a/ee/app/services/groups/enterprise_users/associate_service.rb b/ee/app/services/groups/enterprise_users/associate_service.rb index 3371b834141af9..d26e8ff171a781 100644 --- a/ee/app/services/groups/enterprise_users/associate_service.rb +++ b/ee/app/services/groups/enterprise_users/associate_service.rb @@ -21,26 +21,8 @@ def execute # Allows the raising of persistent failure and enables it to be retried when called from inside sidekiq. # see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130735#note_1550114699 - User.transaction do - @user.update!(user_attributes) - - loop do - # rubocop:disable CodeReuse/ActiveRecord -- required to batch the update - update_count = @user.personal_access_tokens - .where(group_id: nil) - .or( - # For the use case where an organization migrates their enterprise users - # from one group to another group. - @user.personal_access_tokens - .where.not(group_id: @user.enterprise_group_id) - ) - .limit(100) - .update_all(group_id: @user.enterprise_group_id) - # rubocop:enable CodeReuse/ActiveRecord -- required to batch the update - - break if update_count == 0 - end - end + associate_user_personal_access_tokens_with_group + @user.update!(user_attributes) Notify.user_associated_with_enterprise_group_email(user.id).deliver_later @@ -51,6 +33,25 @@ def execute private + def associate_user_personal_access_tokens_with_group + loop do + # rubocop:disable CodeReuse/ActiveRecord -- required to batch the update + update_count = user.personal_access_tokens + .where(group_id: nil) + .or( + # For the use case where an organization migrates their enterprise users + # from one group to another group. + user.personal_access_tokens + .where.not(group_id: group.id) + ) + .limit(100) + .update_all(group_id: group.id) + # rubocop:enable CodeReuse/ActiveRecord -- required to batch the update + + break if update_count == 0 + end + end + def user_attributes enterprise_user_attributes.merge(::Onboarding::FinishService.new(user).onboarding_attributes) end diff --git a/ee/app/services/groups/enterprise_users/disassociate_service.rb b/ee/app/services/groups/enterprise_users/disassociate_service.rb index fc1bc07a4b8864..b9a45c3ae4f151 100644 --- a/ee/app/services/groups/enterprise_users/disassociate_service.rb +++ b/ee/app/services/groups/enterprise_users/disassociate_service.rb @@ -19,25 +19,28 @@ def execute # Allows the raising of persistent failure and enables it to be retried when called from inside sidekiq. # see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130735#note_1550114699 - User.transaction do - @user.user_detail.update!(enterprise_group_id: nil, enterprise_group_associated_at: nil) - - loop do - # rubocop:disable CodeReuse/ActiveRecord -- required to batch the update - update_count = @user.personal_access_tokens - .where.not(group_id: nil) - .limit(100) - .update_all(group_id: nil) - # rubocop:enable CodeReuse/ActiveRecord -- required to batch the update - - break if update_count == 0 - end - end + disassociate_user_personal_access_tokens_from_group + @user.user_detail.update!(enterprise_group_id: nil, enterprise_group_associated_at: nil) log_info(message: 'Disassociated the user from the enterprise group') success end + + private + + def disassociate_user_personal_access_tokens_from_group + loop do + # rubocop:disable CodeReuse/ActiveRecord -- required to batch the update + update_count = user.personal_access_tokens + .where.not(group_id: nil) + .limit(100) + .update_all(group_id: nil) + # rubocop:enable CodeReuse/ActiveRecord -- required to batch the update + + break if update_count == 0 + end + end end end end -- GitLab From 9d685b188b4581763cc660c4a4a673c4edbf46ca Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Wed, 27 Aug 2025 14:33:15 +0300 Subject: [PATCH 10/15] Rename personal_access_tokens_cte.enterprise_group_id to personal_access_tokens_cte.group_id --- ...id_and_user_type_for_human_users_personal_access_tokens.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb b/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb index 96815e298d0ae7..1cba87e135e944 100644 --- a/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb +++ b/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb @@ -13,7 +13,7 @@ def perform result = connection.execute( <<~SQL WITH personal_access_tokens_cte AS MATERIALIZED ( - SELECT personal_access_tokens.id AS id, users.user_type AS user_type, user_details.enterprise_group_id AS enterprise_group_id + SELECT personal_access_tokens.id AS id, users.user_type AS user_type, user_details.enterprise_group_id AS group_id FROM users LEFT JOIN user_details ON user_details.user_id=users.id INNER JOIN personal_access_tokens ON personal_access_tokens.user_id=users.id @@ -23,7 +23,7 @@ def perform LIMIT 100 ) UPDATE personal_access_tokens - SET user_type=personal_access_tokens_cte.user_type, group_id=personal_access_tokens_cte.enterprise_group_id + SET user_type=personal_access_tokens_cte.user_type, group_id=personal_access_tokens_cte.group_id FROM personal_access_tokens_cte WHERE personal_access_tokens.id=personal_access_tokens_cte.id SQL -- GitLab From 4d862b231bb2b3300900fc1e615b2843d1973b3c Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Wed, 27 Aug 2025 15:07:29 +0300 Subject: [PATCH 11/15] Fix identation in ee/app/services/groups/enterprise_users/associate_service.rb --- ee/app/services/groups/enterprise_users/associate_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/services/groups/enterprise_users/associate_service.rb b/ee/app/services/groups/enterprise_users/associate_service.rb index d26e8ff171a781..818136702ec4ac 100644 --- a/ee/app/services/groups/enterprise_users/associate_service.rb +++ b/ee/app/services/groups/enterprise_users/associate_service.rb @@ -42,7 +42,7 @@ def associate_user_personal_access_tokens_with_group # For the use case where an organization migrates their enterprise users # from one group to another group. user.personal_access_tokens - .where.not(group_id: group.id) + .where.not(group_id: group.id) ) .limit(100) .update_all(group_id: group.id) -- GitLab From 87c99f286684d8c1823d043c821a33021b4a38a9 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Thu, 28 Aug 2025 12:55:51 +0300 Subject: [PATCH 12/15] Refactor updating personal_access_tokens to use index_personal_access_tokens_on_user_id_and_id --- .../enterprise_users/associate_service.rb | 17 ++--------------- .../enterprise_users/disassociate_service.rb | 11 ++--------- 2 files changed, 4 insertions(+), 24 deletions(-) diff --git a/ee/app/services/groups/enterprise_users/associate_service.rb b/ee/app/services/groups/enterprise_users/associate_service.rb index 818136702ec4ac..f4a4fcfd07eadf 100644 --- a/ee/app/services/groups/enterprise_users/associate_service.rb +++ b/ee/app/services/groups/enterprise_users/associate_service.rb @@ -34,21 +34,8 @@ def execute private def associate_user_personal_access_tokens_with_group - loop do - # rubocop:disable CodeReuse/ActiveRecord -- required to batch the update - update_count = user.personal_access_tokens - .where(group_id: nil) - .or( - # For the use case where an organization migrates their enterprise users - # from one group to another group. - user.personal_access_tokens - .where.not(group_id: group.id) - ) - .limit(100) - .update_all(group_id: group.id) - # rubocop:enable CodeReuse/ActiveRecord -- required to batch the update - - break if update_count == 0 + user.personal_access_tokens.each_batch(of: 100) do |personal_access_tokens_batch| + personal_access_tokens_batch.update_all(group_id: group.id) end end diff --git a/ee/app/services/groups/enterprise_users/disassociate_service.rb b/ee/app/services/groups/enterprise_users/disassociate_service.rb index b9a45c3ae4f151..dd4cb2d18d76e3 100644 --- a/ee/app/services/groups/enterprise_users/disassociate_service.rb +++ b/ee/app/services/groups/enterprise_users/disassociate_service.rb @@ -30,15 +30,8 @@ def execute private def disassociate_user_personal_access_tokens_from_group - loop do - # rubocop:disable CodeReuse/ActiveRecord -- required to batch the update - update_count = user.personal_access_tokens - .where.not(group_id: nil) - .limit(100) - .update_all(group_id: nil) - # rubocop:enable CodeReuse/ActiveRecord -- required to batch the update - - break if update_count == 0 + user.personal_access_tokens.each_batch(of: 100) do |personal_access_tokens_batch| + personal_access_tokens_batch.update_all(group_id: nil) end end end -- GitLab From a93da96cb76c1b88b6c06bb53b249362e35cd7cd Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Thu, 28 Aug 2025 14:33:06 +0300 Subject: [PATCH 13/15] Update BackfillGroupIdAndUserTypeForHumanUsersPersonalAccessTokens to iterate over personal_access_tokens --- ...for_human_users_personal_access_tokens.yml | 2 +- ...for_human_users_personal_access_tokens.rb} | 6 +-- db/schema_migrations/20250821112501 | 1 - db/schema_migrations/20250828111740 | 1 + ..._for_human_users_personal_access_tokens.rb | 37 +++++++------------ ...human_users_personal_access_tokens_spec.rb | 2 +- ...uman_users_personal_access_tokens_spec.rb} | 2 +- 7 files changed, 21 insertions(+), 30 deletions(-) rename db/post_migrate/{20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb => 20250828111740_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb} (80%) delete mode 100644 db/schema_migrations/20250821112501 create mode 100644 db/schema_migrations/20250828111740 rename spec/migrations/{20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb => 20250828111740_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb} (94%) diff --git a/db/docs/batched_background_migrations/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.yml b/db/docs/batched_background_migrations/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.yml index 674bd20139986c..678f37f78987fb 100644 --- a/db/docs/batched_background_migrations/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.yml +++ b/db/docs/batched_background_migrations/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.yml @@ -5,5 +5,5 @@ description: Backfills `group_id` based on `user_details.enterprise_group_id` an feature_category: system_access introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/201921 milestone: '18.4' -queued_migration_version: 20250821112501 +queued_migration_version: 20250828111740 finalized_by: # version of the migration that finalized this BBM diff --git a/db/post_migrate/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb b/db/post_migrate/20250828111740_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb similarity index 80% rename from db/post_migrate/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb rename to db/post_migrate/20250828111740_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb index 79029b8073ec24..ce4569008966da 100644 --- a/db/post_migrate/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb +++ b/db/post_migrate/20250828111740_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb @@ -7,13 +7,13 @@ class QueueBackfillGroupIdAndUserTypeForHumanUsersPersonalAccessTokens < Gitlab: MIGRATION = "BackfillGroupIdAndUserTypeForHumanUsersPersonalAccessTokens" DELAY_INTERVAL = 2.minutes - BATCH_SIZE = 5000 + BATCH_SIZE = 1000 SUB_BATCH_SIZE = 100 def up queue_batched_background_migration( MIGRATION, - :users, + :personal_access_tokens, :id, job_interval: DELAY_INTERVAL, batch_size: BATCH_SIZE, @@ -22,6 +22,6 @@ def up end def down - delete_batched_background_migration(MIGRATION, :users, :id, []) + delete_batched_background_migration(MIGRATION, :personal_access_tokens, :id, []) end end diff --git a/db/schema_migrations/20250821112501 b/db/schema_migrations/20250821112501 deleted file mode 100644 index 14979a8dd02eb2..00000000000000 --- a/db/schema_migrations/20250821112501 +++ /dev/null @@ -1 +0,0 @@ -f233834dd1240799da46750f0157107c68fcd3f6af9a44fe03db6c6a15f79a09 \ No newline at end of file diff --git a/db/schema_migrations/20250828111740 b/db/schema_migrations/20250828111740 new file mode 100644 index 00000000000000..a3dd67f58acba5 --- /dev/null +++ b/db/schema_migrations/20250828111740 @@ -0,0 +1 @@ +9eedf1bfcb5c6c72cee92fb23f45a1073fdc5f0e096e7289d5cfa782245a280e \ No newline at end of file diff --git a/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb b/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb index 1cba87e135e944..b56547a0a678e9 100644 --- a/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb +++ b/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb @@ -4,33 +4,24 @@ module Gitlab module BackgroundMigration class BackfillGroupIdAndUserTypeForHumanUsersPersonalAccessTokens < BatchedMigrationJob operation_name :backfill_group_id_and_user_type_for_human_users_personal_access_tokens - scope_to ->(relation) { relation.where(user_type: 0) } # rubocop: disable Database/AvoidScopeTo -- `user_type` is an indexed column feature_category :system_access def perform each_sub_batch do |sub_batch| - loop do - result = connection.execute( - <<~SQL - WITH personal_access_tokens_cte AS MATERIALIZED ( - SELECT personal_access_tokens.id AS id, users.user_type AS user_type, user_details.enterprise_group_id AS group_id - FROM users - LEFT JOIN user_details ON user_details.user_id=users.id - INNER JOIN personal_access_tokens ON personal_access_tokens.user_id=users.id - WHERE - users.id IN (#{sub_batch.select(:id).to_sql}) - AND personal_access_tokens.user_type IS NULL - LIMIT 100 - ) - UPDATE personal_access_tokens - SET user_type=personal_access_tokens_cte.user_type, group_id=personal_access_tokens_cte.group_id - FROM personal_access_tokens_cte - WHERE personal_access_tokens.id=personal_access_tokens_cte.id - SQL - ) - - break if result.cmd_tuples == 0 - end + connection.execute( + <<~SQL + UPDATE personal_access_tokens + SET user_type=users.user_type, group_id=user_details.enterprise_group_id + FROM + users + LEFT JOIN user_details ON user_details.user_id=users.id + WHERE + personal_access_tokens.id IN (#{sub_batch.select(:id).to_sql}) + AND personal_access_tokens.user_type IS NULL + AND personal_access_tokens.user_id=users.id + AND users.user_type = 0 + SQL + ) end end end diff --git a/spec/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb b/spec/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb index 66bbc327c1be43..a54068a4b560f3 100644 --- a/spec/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb +++ b/spec/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb @@ -9,7 +9,7 @@ ) do subject(:migration) do described_class.new( - batch_table: :users, + batch_table: :personal_access_tokens, batch_column: :id, sub_batch_size: 100, pause_ms: 100, diff --git a/spec/migrations/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb b/spec/migrations/20250828111740_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb similarity index 94% rename from spec/migrations/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb rename to spec/migrations/20250828111740_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb index f38092c7a4c586..9b5eea599f4492 100644 --- a/spec/migrations/20250821112501_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb +++ b/spec/migrations/20250828111740_queue_backfill_group_id_and_user_type_for_human_users_personal_access_tokens_spec.rb @@ -15,7 +15,7 @@ migration.after -> { expect(batched_migration).to have_scheduled_batched_migration( gitlab_schema: :gitlab_main, - table_name: :users, + table_name: :personal_access_tokens, column_name: :id, interval: described_class::DELAY_INTERVAL, batch_size: described_class::BATCH_SIZE, -- GitLab From b20436f3b67dd9cc235a43ef1205d38704389dbb Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Tue, 2 Sep 2025 12:34:09 +0300 Subject: [PATCH 14/15] Explicitly apply limit(sub_batch_size) in BackfillGroupIdAndUserTypeForHumanUsersPersonalAccessTokens --- ...p_id_and_user_type_for_human_users_personal_access_tokens.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb b/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb index b56547a0a678e9..db1c8aa931a8ad 100644 --- a/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb +++ b/lib/gitlab/background_migration/backfill_group_id_and_user_type_for_human_users_personal_access_tokens.rb @@ -16,7 +16,7 @@ def perform users LEFT JOIN user_details ON user_details.user_id=users.id WHERE - personal_access_tokens.id IN (#{sub_batch.select(:id).to_sql}) + personal_access_tokens.id IN (#{sub_batch.select(:id).limit(sub_batch_size).to_sql}) AND personal_access_tokens.user_type IS NULL AND personal_access_tokens.user_id=users.id AND users.user_type = 0 -- GitLab From 0284d1a088f13d213d5c42dc614d6ca092879759 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Fri, 5 Sep 2025 12:22:27 +0300 Subject: [PATCH 15/15] Move test related group service account's token to EE --- .../rotate_service_spec.rb | 22 +++++++++++++++++++ .../rotate_service_spec.rb | 10 --------- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/ee/spec/services/ee/personal_access_tokens/rotate_service_spec.rb b/ee/spec/services/ee/personal_access_tokens/rotate_service_spec.rb index c9f92bf3887c5f..5ec3c0facaa8f9 100644 --- a/ee/spec/services/ee/personal_access_tokens/rotate_service_spec.rb +++ b/ee/spec/services/ee/personal_access_tokens/rotate_service_spec.rb @@ -109,4 +109,26 @@ expect(new_token.description).to eq(token.description) end end + + context "for group service account's token" do + let_it_be(:group) { create(:group) } + let_it_be(:current_user) { create(:user, :service_account, provisioned_by_group: group) } + let_it_be(:token, reload: true) do + create(:personal_access_token, user: current_user, group: group) + end + + it "rotates user's own token" do + expect(response).to be_success + + new_token = response.payload[:personal_access_token] + + expect(new_token.token).not_to eq(token.token) + expect(new_token.user).to eq(token.user) + expect(new_token.user_type).to eq(token.user_type) + expect(new_token.group_id).to eq(token.group_id) + expect(new_token.user.namespace).to eq(token.user.namespace) + expect(new_token.organization).to eq(token.organization) + expect(new_token.description).to eq(token.description) + end + end end diff --git a/spec/services/personal_access_tokens/rotate_service_spec.rb b/spec/services/personal_access_tokens/rotate_service_spec.rb index dfd40d810f2b73..61a9d927dd3829 100644 --- a/spec/services/personal_access_tokens/rotate_service_spec.rb +++ b/spec/services/personal_access_tokens/rotate_service_spec.rb @@ -156,15 +156,5 @@ end end end - - context "for group service account's token" do - let_it_be(:group) { create(:group) } - let_it_be(:current_user) { create(:user, :service_account, provisioned_by_group: group) } - let_it_be(:token, reload: true) do - create(:personal_access_token, user: current_user, group: group, expires_at: Time.zone.today + 30.days) - end - - it_behaves_like "rotates token successfully" - end end end -- GitLab