diff --git a/ee/lib/ee/users/internal.rb b/ee/lib/ee/users/internal.rb index d5788c29ec3eddc6db6046426ce2e726a769a2f4..4ca636d18572799ecae742ecd263a3ed71977398 100644 --- a/ee/lib/ee/users/internal.rb +++ b/ee/lib/ee/users/internal.rb @@ -6,31 +6,36 @@ module Internal extend ActiveSupport::Concern class_methods do - # rubocop:disable CodeReuse/ActiveRecord - def visual_review_bot - email_pattern = "visual_review%s@#{Settings.gitlab.host}" + extend Forwardable - unique_internal(::User.where(user_type: :visual_review_bot), 'visual-review-bot', email_pattern) do |u| - u.bio = 'The Gitlab Visual Review feedback bot' - u.name = 'Gitlab Visual Review Bot' - u.confirmed_at = Time.zone.now - u.private_profile = true - end + # Delegate to an instance method of the class + def_delegators :new, :visual_review_bot, :suggested_reviewers_bot + end + + # rubocop:disable CodeReuse/ActiveRecord -- Need to instantiate a record here + def visual_review_bot + email_pattern = "visual_review%s@#{Settings.gitlab.host}" + + unique_internal(::User.where(user_type: :visual_review_bot), 'visual-review-bot', email_pattern) do |u| + u.bio = 'The Gitlab Visual Review feedback bot' + u.name = 'Gitlab Visual Review Bot' + u.confirmed_at = Time.zone.now + u.private_profile = true end + end - def suggested_reviewers_bot - email_pattern = "suggested-reviewers-bot%s@#{Settings.gitlab.host}" + def suggested_reviewers_bot + email_pattern = "suggested-reviewers-bot%s@#{Settings.gitlab.host}" - unique_internal( - ::User.where(user_type: :suggested_reviewers_bot), 'suggested-reviewers-bot', email_pattern) do |u| - u.bio = 'The GitLab suggested reviewers bot used for suggested reviewers' - u.name = 'GitLab Suggested Reviewers Bot' - u.confirmed_at = Time.zone.now - u.private_profile = true - end + unique_internal( + ::User.where(user_type: :suggested_reviewers_bot), 'suggested-reviewers-bot', email_pattern) do |u| + u.bio = 'The GitLab suggested reviewers bot used for suggested reviewers' + u.name = 'GitLab Suggested Reviewers Bot' + u.confirmed_at = Time.zone.now + u.private_profile = true end - # rubocop:enable CodeReuse/ActiveRecord end + # rubocop:enable CodeReuse/ActiveRecord end end end diff --git a/ee/spec/lib/ee/users/internal_spec.rb b/ee/spec/lib/ee/users/internal_spec.rb index f5fade809e8d6e12c5fe1669c0fafc07cc8c173d..026d76fef9fb996c5d1ac4a176576df1e7600b7a 100644 --- a/ee/spec/lib/ee/users/internal_spec.rb +++ b/ee/spec/lib/ee/users/internal_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Users::Internal, feature_category: :user_profile do + let_it_be(:first_organization) { create(:organization) } + shared_examples 'bot users' do |bot_type| it 'creates the user if it does not exist' do expect do diff --git a/lib/users/internal.rb b/lib/users/internal.rb index cc38f6517cd62c8194a1ee79d32290a495a09235..095c35bbc64c185d696d665319080d6fafc56c87 100644 --- a/lib/users/internal.rb +++ b/lib/users/internal.rb @@ -4,65 +4,15 @@ module Users class Internal class << self include Gitlab::Utils::StrongMemoize - # rubocop:disable CodeReuse/ActiveRecord - - # Return (create if necessary) the ghost user. The ghost user - # owns records previously belonging to deleted users. - def ghost - email = 'ghost%s@example.com' - unique_internal(User.where(user_type: :ghost), 'ghost', email) do |u| - u.bio = _('This is a "Ghost User", created to hold all issues authored by users that have ' \ - 'since been deleted. This user cannot be removed.') - u.name = 'Ghost User' - end - end - - def alert_bot - email_pattern = "alert%s@#{Settings.gitlab.host}" - - unique_internal(User.where(user_type: :alert_bot), 'alert-bot', email_pattern) do |u| - u.bio = 'The GitLab alert bot' - u.name = 'GitLab Alert Bot' - u.avatar = bot_avatar(image: 'alert-bot.png') - u.confirmed_at = Time.zone.now - u.private_profile = true - end - end - - def migration_bot - email_pattern = "noreply+gitlab-migration-bot%s@#{Settings.gitlab.host}" - - unique_internal(User.where(user_type: :migration_bot), 'migration-bot', email_pattern) do |u| - u.bio = 'The GitLab migration bot' - u.name = 'GitLab Migration Bot' - u.confirmed_at = Time.zone.now - u.private_profile = true - end - end - def security_bot - email_pattern = "security-bot%s@#{Settings.gitlab.host}" - - unique_internal(User.where(user_type: :security_bot), 'GitLab-Security-Bot', email_pattern) do |u| - u.bio = 'System bot that monitors detected vulnerabilities for solutions ' \ - 'and creates merge requests with the fixes.' - u.name = 'GitLab Security Bot' - u.avatar = bot_avatar(image: 'security-bot.png') - u.confirmed_at = Time.zone.now - u.private_profile = true - end - end + extend Forwardable - def support_bot - email_pattern = "support%s@#{Settings.gitlab.host}" + def_delegators :new, :bot_avatar, :ghost, :support_bot, :alert_bot, + :migration_bot, :security_bot, :automation_bot, :llm_bot, + :duo_code_review_bot, :admin_bot - unique_internal(User.where(user_type: :support_bot), 'support-bot', email_pattern) do |u| - u.bio = 'The GitLab support bot used for Service Desk' - u.name = 'GitLab Support Bot' - u.avatar = bot_avatar(image: 'support-bot.png') - u.confirmed_at = Time.zone.now - u.private_profile = true - end + def for_organization(organization) + new(organization: organization) end # Checks against this bot are now included in every issue and work item @@ -73,118 +23,194 @@ def support_bot # different nodes may have different object instances of the bot. # We only memoize the id because this is the information we check against. def support_bot_id - support_bot.id + new.support_bot.id end strong_memoize_attr :support_bot_id + end + + # rubocop:disable CodeReuse/ActiveRecord -- Need to instantiate a record here + def initialize(organization: nil) + @organization = organization + end + + # Return (create if necessary) the ghost user. The ghost user + # owns records previously belonging to deleted users. + def ghost + email = 'ghost%s@example.com' + unique_internal(User.where(user_type: :ghost), 'ghost', email) do |u| + u.bio = _('This is a "Ghost User", created to hold all issues authored by users that have ' \ + 'since been deleted. This user cannot be removed.') + u.name = 'Ghost User' + end + end + + def alert_bot + email_pattern = "alert%s@#{Settings.gitlab.host}" + + unique_internal(User.where(user_type: :alert_bot), 'alert-bot', email_pattern) do |u| + u.bio = 'The GitLab alert bot' + u.name = 'GitLab Alert Bot' + u.avatar = bot_avatar(image: 'alert-bot.png') + u.confirmed_at = Time.zone.now + u.private_profile = true + end + end + + def migration_bot + email_pattern = "noreply+gitlab-migration-bot%s@#{Settings.gitlab.host}" + + unique_internal(User.where(user_type: :migration_bot), 'migration-bot', email_pattern) do |u| + u.bio = 'The GitLab migration bot' + u.name = 'GitLab Migration Bot' + u.confirmed_at = Time.zone.now + u.private_profile = true + end + end + + def security_bot + email_pattern = "security-bot%s@#{Settings.gitlab.host}" - def automation_bot - email_pattern = "automation%s@#{Settings.gitlab.host}" + unique_internal(User.where(user_type: :security_bot), 'GitLab-Security-Bot', email_pattern) do |u| + u.bio = 'System bot that monitors detected vulnerabilities for solutions ' \ + 'and creates merge requests with the fixes.' + u.name = 'GitLab Security Bot' + u.avatar = bot_avatar(image: 'security-bot.png') + u.confirmed_at = Time.zone.now + u.private_profile = true + end + end + + def support_bot + email_pattern = "support%s@#{Settings.gitlab.host}" - unique_internal(User.where(user_type: :automation_bot), 'automation-bot', email_pattern) do |u| - u.bio = 'The GitLab automation bot used for automated workflows and tasks' - u.name = 'GitLab Automation Bot' - u.avatar = bot_avatar(image: 'support-bot.png') # todo: add an avatar for automation-bot - u.confirmed_at = Time.zone.now - u.private_profile = true - end + unique_internal(User.where(user_type: :support_bot), 'support-bot', email_pattern) do |u| + u.bio = 'The GitLab support bot used for Service Desk' + u.name = 'GitLab Support Bot' + u.avatar = bot_avatar(image: 'support-bot.png') + u.confirmed_at = Time.zone.now + u.private_profile = true end + end + + def support_bot_id + support_bot.id + end - def llm_bot - email_pattern = "llm-bot%s@#{Settings.gitlab.host}" + def automation_bot + email_pattern = "automation%s@#{Settings.gitlab.host}" - unique_internal(User.where(user_type: :llm_bot), 'GitLab-Llm-Bot', email_pattern) do |u| - u.bio = 'The Gitlab LLM bot used for fetching LLM-generated content' - u.name = 'GitLab LLM Bot' - u.avatar = bot_avatar(image: 'support-bot.png') # todo: add an avatar for llm-bot - u.confirmed_at = Time.zone.now - u.private_profile = true - end + unique_internal(User.where(user_type: :automation_bot), 'automation-bot', email_pattern) do |u| + u.bio = 'The GitLab automation bot used for automated workflows and tasks' + u.name = 'GitLab Automation Bot' + u.avatar = bot_avatar(image: 'support-bot.png') # todo: add an avatar for automation-bot + u.confirmed_at = Time.zone.now + u.private_profile = true end + end - def duo_code_review_bot - email_pattern = "gitlab-duo%s@#{Settings.gitlab.host}" + def llm_bot + email_pattern = "llm-bot%s@#{Settings.gitlab.host}" - unique_internal(User.where(user_type: :duo_code_review_bot), 'GitLabDuo', email_pattern) do |u| - u.bio = 'GitLab Duo bot for handling AI tasks' - u.name = 'GitLab Duo' - u.avatar = bot_avatar(image: 'duo-bot.png') - u.confirmed_at = Time.zone.now - u.private_profile = true - end + unique_internal(User.where(user_type: :llm_bot), 'GitLab-Llm-Bot', email_pattern) do |u| + u.bio = 'The Gitlab LLM bot used for fetching LLM-generated content' + u.name = 'GitLab LLM Bot' + u.avatar = bot_avatar(image: 'support-bot.png') # todo: add an avatar for llm-bot + u.confirmed_at = Time.zone.now + u.private_profile = true end + end + + def duo_code_review_bot + email_pattern = "gitlab-duo%s@#{Settings.gitlab.host}" - def admin_bot - email_pattern = "admin-bot%s@#{Settings.gitlab.host}" - - unique_internal(User.where(user_type: :admin_bot), 'GitLab-Admin-Bot', email_pattern) do |u| - u.bio = 'Admin bot used for tasks that require admin privileges' - u.name = 'GitLab Admin Bot' - u.avatar = bot_avatar(image: 'admin-bot.png') - u.admin = true - u.confirmed_at = Time.zone.now - u.private_profile = true - end + unique_internal(User.where(user_type: :duo_code_review_bot), 'GitLabDuo', email_pattern) do |u| + u.bio = 'GitLab Duo bot for handling AI tasks' + u.name = 'GitLab Duo' + u.avatar = bot_avatar(image: 'duo-bot.png') + u.confirmed_at = Time.zone.now + u.private_profile = true end + end - # rubocop:enable CodeReuse/ActiveRecord + def admin_bot + email_pattern = "admin-bot%s@#{Settings.gitlab.host}" - def bot_avatar(image:) - Rails.root.join('lib', 'assets', 'images', 'bot_avatars', image).open + unique_internal(User.where(user_type: :admin_bot), 'GitLab-Admin-Bot', email_pattern) do |u| + u.bio = 'Admin bot used for tasks that require admin privileges' + u.name = 'GitLab Admin Bot' + u.avatar = bot_avatar(image: 'admin-bot.png') + u.admin = true + u.confirmed_at = Time.zone.now + u.private_profile = true end + end - private + def bot_avatar(image:) + Rails.root.join('lib', 'assets', 'images', 'bot_avatars', image).open + end - # NOTE: This method is patched in spec/spec_helper.rb to allow use of exclusive lease in RSpec's - # :before_all scope to keep the specs DRY. - def unique_internal(scope, username, email_pattern, &block) - scope.first || create_unique_internal(scope, username, email_pattern, &block) + private + + # NOTE: This method is patched in spec/spec_helper.rb to allow use of exclusive lease in RSpec's + # :before_all scope to keep the specs DRY. + def unique_internal(scope, username, email_pattern, &block) + if @organization + scope = scope.joins(:organization_users).where(organization_users: { organization: @organization }) end - def create_unique_internal(scope, username, email_pattern, &creation_block) - # Since we only want a single one of these in an instance, we use an - # exclusive lease to ensure than this block is never run concurrently. - lease_key = "user:unique_internal:#{username}" - lease = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.minute.to_i) + scope.first || create_unique_internal(scope, username, email_pattern, &block) + end + def create_unique_internal(scope, username, email_pattern, &creation_block) + # Since we only want a single one of these in an instance, we use an + # exclusive lease to ensure than this block is never run concurrently. + lease_key = if @organization + "user:unique_internal:#{@organization.id}:#{username}" + else + "user:unique_internal:#{username}" + end + + lease = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.minute.to_i) + + uuid = lease.try_obtain + until uuid.present? + # Keep trying until we obtain the lease. To prevent hammering Redis too + # much we'll wait for a bit between retries. + sleep(1) uuid = lease.try_obtain - until uuid.present? - # Keep trying until we obtain the lease. To prevent hammering Redis too - # much we'll wait for a bit between retries. - sleep(1) - uuid = lease.try_obtain - end - - # Recheck if the user is already present. One might have been - # added between the time we last checked (first line of this method) - # and the time we acquired the lock. - existing_user = scope.model.uncached { scope.first } - return existing_user if existing_user.present? - - uniquify = Gitlab::Utils::Uniquify.new - - username = uniquify.string(username) { |s| Namespace.by_path(s) } - - email = uniquify.string(->(n) { Kernel.sprintf(email_pattern, n) }) do |s| - User.find_by_email(s) - end - - user = scope.build( - username: username, - email: email, - &creation_block - ) - - # https://gitlab.com/gitlab-org/gitlab/-/issues/442780 - organization = ::Organizations::Organization.first - user.assign_personal_namespace(organization) - user.organizations << organization - - Users::UpdateService.new(user, user: user).execute(validate: false) - user - ensure - Gitlab::ExclusiveLease.cancel(lease_key, uuid) end + + # Recheck if the user is already present. One might have been + # added between the time we last checked (first line of this method) + # and the time we acquired the lock. + existing_user = scope.model.uncached { scope.first } + return existing_user if existing_user.present? + + uniquify = Gitlab::Utils::Uniquify.new + + username = uniquify.string(username) { |s| Namespace.by_path(s) } + + email = uniquify.string(->(n) { Kernel.sprintf(email_pattern, n) }) do |s| + User.find_by_email(s) + end + + user = scope.build( + username: username, + email: email, + &creation_block + ) + + user_organization = @organization || Organizations::Organization.first + user.assign_personal_namespace(user_organization) + user.organizations << user_organization + + Users::UpdateService.new(user, user: user).execute(validate: false) + user + ensure + Gitlab::ExclusiveLease.cancel(lease_key, uuid) end + + # rubocop:enable CodeReuse/ActiveRecord end end diff --git a/spec/lib/users/internal_spec.rb b/spec/lib/users/internal_spec.rb index 0e8ab182a3f0c5522a3e8edaaf1d72a75d5b3970..362022869777ec4ee100ed105e365a1c6c358bd7 100644 --- a/spec/lib/users/internal_spec.rb +++ b/spec/lib/users/internal_spec.rb @@ -3,33 +3,34 @@ require 'spec_helper' RSpec.describe Users::Internal, feature_category: :user_profile do + let_it_be(:first_organization) { create(:organization) } let_it_be(:organization) { create(:organization) } shared_examples 'bot users' do |bot_type, username, email| it 'creates the user if it does not exist' do expect do - described_class.public_send(bot_type) + described_class.for_organization(organization).public_send(bot_type) end.to change { User.where(user_type: bot_type).count }.by(1) end it 'creates a route for the namespace of the created user' do - bot_user = described_class.public_send(bot_type) + bot_user = described_class.for_organization(organization).public_send(bot_type) expect(bot_user.namespace.route).to be_present expect(bot_user.namespace.organization).to eq(organization) end - it 'assigns the first found organization to the created user' do - bot_user = described_class.public_send(bot_type) + it 'assigns the organization to the created user' do + bot_user = described_class.for_organization(organization).public_send(bot_type) - expect(bot_user.organizations.first).to eq(organization) + expect(bot_user.organizations).to eq([organization]) end it 'does not create a new user if it already exists' do - described_class.public_send(bot_type) + described_class.for_organization(organization).public_send(bot_type) expect do - described_class.public_send(bot_type) + described_class.for_organization(organization).public_send(bot_type) end.not_to change { User.count } end @@ -38,7 +39,7 @@ create(:user, username: username) expect do - described_class.public_send(bot_type) + described_class.for_organization(organization).public_send(bot_type) end.to change { User.where(user_type: bot_type).count }.by(1) end end @@ -48,7 +49,7 @@ create(:user, email: email) expect do - described_class.public_send(bot_type) + described_class.for_organization(organization).public_send(bot_type) end.to change { User.where(user_type: bot_type).count }.by(1) end end @@ -58,7 +59,7 @@ create(:group, path: username) expect do - described_class.public_send(bot_type) + described_class.for_organization(organization).public_send(bot_type) end.to change { User.where(user_type: bot_type).count }.by(1) end end @@ -70,7 +71,7 @@ it 'creates the bot user' do expect do - described_class.public_send(bot_type) + described_class.for_organization(organization).public_send(bot_type) end.to change { User.where(user_type: bot_type).count }.by(1) end end @@ -78,7 +79,7 @@ shared_examples 'bot user avatars' do |bot_type, avatar_filename| it 'sets the custom avatar for the created bot' do - bot_user = described_class.public_send(bot_type) + bot_user = described_class.for_organization(organization).public_send(bot_type) expect(bot_user.avatar.url).to be_present expect(bot_user.avatar.filename).to eq(avatar_filename) @@ -104,28 +105,51 @@ it_behaves_like 'bot user avatars', :admin_bot, 'admin-bot.png' context 'when bot is the support_bot' do - subject { described_class.support_bot } + subject { described_class.for_organization(organization).support_bot } it { is_expected.to be_confirmed } end context 'when bot is the admin bot' do - subject { described_class.admin_bot } + subject { described_class.for_organization(organization).admin_bot } it { is_expected.to be_admin } it { is_expected.to be_confirmed } end describe '.support_bot_id' do - before do - # Ensure support bot user is created and memoization uses the same id - # See https://gitlab.com/gitlab-org/gitlab/-/issues/509629 - described_class.clear_memoization(:support_bot_id) - described_class.support_bot_id + context 'when organization is not used' do + before do + # Ensure support bot user is created and memoization uses the same id + # See https://gitlab.com/gitlab-org/gitlab/-/issues/509629 + described_class.clear_memoization(:support_bot_id) + described_class.support_bot_id + end + + subject(:support_bot_id) { described_class.support_bot_id } + + it "does not call instance support_bot_id method" do + expect_next_instance_of(described_class) do |instance| + expect(instance).not_to receive(:support_bot_id) + end + + expect(support_bot_id).to eq(described_class.support_bot.id) + end end - subject { described_class.support_bot_id } + context 'when organization is used' do + subject { described_class.for_organization(organization).support_bot_id } - it { is_expected.to eq(described_class.support_bot.id) } + it { is_expected.to eq(described_class.for_organization(organization).support_bot.id) } + end + end + + context 'when organization is not used' do + it 'creates user in first organization' do + bot = described_class.support_bot + + expect(bot.organizations).to eq([first_organization]) + expect(bot.namespace.organization).to eq(first_organization) + end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 58f0b40b33e6fc1fc745d17f6b60b6fa96287c90..431947ce749558e965348f8e3b206143f0c14d04 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -612,28 +612,26 @@ def call(command, redis_config) module UsersInternalAllowExclusiveLease extend ActiveSupport::Concern - class_methods do - def unique_internal(scope, username, email_pattern, &block) - # this lets skip transaction checks when Users::Internal bots are created in - # let_it_be blocks during test set-up. - # - # Users::Internal bot creation within examples are still checked since the RSPec.current_scope is :example - if ::RSpec.respond_to?(:current_scope) && ::RSpec.current_scope == :before_all - Gitlab::ExclusiveLease.skipping_transaction_check { super } - else - super - end + def unique_internal(scope, username, email_pattern, &block) + # this lets skip transaction checks when Users::Internal bots are created in + # let_it_be blocks during test set-up. + # + # Users::Internal bot creation within examples are still checked since the RSPec.current_scope is :example + if ::RSpec.respond_to?(:current_scope) && ::RSpec.current_scope == :before_all + Gitlab::ExclusiveLease.skipping_transaction_check { super } + else + super end + end - # TODO: Until https://gitlab.com/gitlab-org/gitlab/-/issues/442780 is resolved we're creating internal users in the - # first organization as a temporary workaround. Many specs lack an organization in the database, causing foreign key - # constraint violations when creating internal users. We're not seeding organizations before all specs for - # performance. - def create_unique_internal(scope, username, email_pattern, &creation_block) - Organizations::Organization.first || FactoryBot.create(:organization) + # TODO: Until https://gitlab.com/gitlab-org/gitlab/-/issues/442780 is resolved we're creating internal users in the + # first organization as a temporary workaround. Many specs lack an organization in the database, causing foreign key + # constraint violations when creating internal users. We're not seeding organizations before all specs for + # performance. + def create_unique_internal(scope, username, email_pattern, &creation_block) + Organizations::Organization.first || FactoryBot.create(:organization) - super - end + super end end