From 7085ebbc369f4bf6d7f1899e3c0f966dd28b9211 Mon Sep 17 00:00:00 2001 From: Rutger Wessels Date: Mon, 13 Jan 2025 16:41:21 +0100 Subject: [PATCH 01/12] Test branch for testing Internal Users without Organizaiton --- app/models/user.rb | 7 +++++++ lib/users/internal.rb | 5 ----- spec/lib/users/internal_spec.rb | 13 ------------- 3 files changed, 7 insertions(+), 18 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index ccd4c95a205bc5..1aa9f9669ba74a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2913,6 +2913,13 @@ def prefix_for_feed_token FEED_TOKEN_PREFIX end + def create_default_organization_user + return unless organizations.blank? + return unless user_type == 'human' # This method is candidate for removal. + + Organizations::OrganizationUser.create_default_organization_record_for(id, user_is_admin: admin?) + end + # method overridden in EE def audit_lock_access(reason: nil); end diff --git a/lib/users/internal.rb b/lib/users/internal.rb index cc38f6517cd62c..377bd05fa0acfe 100644 --- a/lib/users/internal.rb +++ b/lib/users/internal.rb @@ -174,11 +174,6 @@ def create_unique_internal(scope, username, email_pattern, &creation_block) &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 diff --git a/spec/lib/users/internal_spec.rb b/spec/lib/users/internal_spec.rb index 0e8ab182a3f0c5..28b3118830b1f0 100644 --- a/spec/lib/users/internal_spec.rb +++ b/spec/lib/users/internal_spec.rb @@ -12,19 +12,6 @@ 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) - - 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) - - expect(bot_user.organizations.first).to eq(organization) - end - it 'does not create a new user if it already exists' do described_class.public_send(bot_type) -- GitLab From 14bd1062c8220e951498ded3f4d22c2875dcd554 Mon Sep 17 00:00:00 2001 From: Rutger Wessels Date: Tue, 14 Jan 2025 11:27:40 +0100 Subject: [PATCH 02/12] Add User.instance_user? method --- app/models/concerns/has_user_type.rb | 8 ++++++++ spec/models/concerns/has_user_type_spec.rb | 24 ++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/app/models/concerns/has_user_type.rb b/app/models/concerns/has_user_type.rb index 2aba5d156e1be8..89b9a09528092b 100644 --- a/app/models/concerns/has_user_type.rb +++ b/app/models/concerns/has_user_type.rb @@ -72,6 +72,14 @@ def internal? INTERNAL_USER_TYPES.include?(user_type) end + # rubocop:disable Gitlab/AvoidGitlabInstanceChecks -- This is not related to a specific feature + def instance_user? + return !Gitlab.com? if user_type == 'service_account' + + INTERNAL_USER_TYPES.include?(user_type) + end + # rubocop:enable Gitlab/AvoidGitlabInstanceChecks + def redacted_name(viewing_user) return self.name unless self.project_bot? diff --git a/spec/models/concerns/has_user_type_spec.rb b/spec/models/concerns/has_user_type_spec.rb index fc619485d0a48b..5908ecbf4235b3 100644 --- a/spec/models/concerns/has_user_type_spec.rb +++ b/spec/models/concerns/has_user_type_spec.rb @@ -9,6 +9,7 @@ let(:bots) { User::BOT_USER_TYPES.map { |type| public_send(type) } } let(:non_internal) { User::NON_INTERNAL_USER_TYPES.map { |type| public_send(type) } } let(:everyone) { User::USER_TYPES.keys.map { |type| public_send(type) } } + let(:internal) { User::INTERNAL_USER_TYPES.map { |type| public_send(type) } } specify 'types consistency checks', :aggregate_failures do expect(described_class::USER_TYPES.keys) @@ -204,5 +205,28 @@ end end end + + describe 'instance_user?' do + context 'when on SaaS', :saas do + it 'is true for all internal user types and false for others' do + expect(internal).to all(be_instance_user) + + (everyone - internal).each do |user| + expect(user.instance_user?).to be(false) + end + end + end + + context 'when on Self Managed' do + it 'is true for service_account and all internal user types and false for others' do + expect(internal).to all(be_instance_user) + expect(service_account.instance_user?).to be(true) + + (everyone - internal - [service_account]).each do |user| + expect(user.instance_user?).to be(false) + end + end + end + end end end -- GitLab From 39d1f80cd615b8c39e4b628b6d05073fc9efe6f1 Mon Sep 17 00:00:00 2001 From: Rutger Wessels Date: Wed, 15 Jan 2025 10:41:17 +0100 Subject: [PATCH 03/12] Remove test --- app/models/user.rb | 2 +- ee/spec/lib/ee/users/internal_spec.rb | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 1aa9f9669ba74a..d64a12e5a1dbcb 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2915,7 +2915,7 @@ def prefix_for_feed_token def create_default_organization_user return unless organizations.blank? - return unless user_type == 'human' # This method is candidate for removal. + return if instance_user? # No organization for instance users Organizations::OrganizationUser.create_default_organization_record_for(id, user_is_admin: admin?) end diff --git a/ee/spec/lib/ee/users/internal_spec.rb b/ee/spec/lib/ee/users/internal_spec.rb index f5fade809e8d6e..dcd30c2a632b44 100644 --- a/ee/spec/lib/ee/users/internal_spec.rb +++ b/ee/spec/lib/ee/users/internal_spec.rb @@ -10,12 +10,6 @@ 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) - - expect(bot_user.namespace.route).to be_present - end - it 'does not create a new user if it already exists' do described_class.public_send(bot_type) -- GitLab From 28a74cedc40d96b0adf3af753c9f9e606c519752 Mon Sep 17 00:00:00 2001 From: Rutger Wessels Date: Wed, 15 Jan 2025 14:43:48 +0100 Subject: [PATCH 04/12] Don't rely on routable for initial user lookup --- app/controllers/admin/users_controller.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 623fd917afff20..e3dee2b82a393b 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -350,7 +350,15 @@ def hard_delete? end def user - @user ||= find_routable!(User, params[:id], request.fullpath) + @user ||= begin + user = User.by_username(params[:id]).first + + if user.username == params[:id] && can?(current_user, :read_user, user) + user + else + find_routable!(User, params[:id], request.fullpath) + end + end end def build_canonical_path(user) -- GitLab From 5e56d5a13e44c8caa36c2e308f82a6ca211070ea Mon Sep 17 00:00:00 2001 From: agius Date: Thu, 16 Jan 2025 11:28:40 -0800 Subject: [PATCH 05/12] Fix certain specs --- app/controllers/admin/users_controller.rb | 10 +--------- ee/app/models/ee/note.rb | 6 ++++++ .../admin/admin_users_impersonation_tokens_spec.rb | 2 +- .../delete_orphaned_groups_spec.rb | 2 +- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index e3dee2b82a393b..623fd917afff20 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -350,15 +350,7 @@ def hard_delete? end def user - @user ||= begin - user = User.by_username(params[:id]).first - - if user.username == params[:id] && can?(current_user, :read_user, user) - user - else - find_routable!(User, params[:id], request.fullpath) - end - end + @user ||= find_routable!(User, params[:id], request.fullpath) end def build_canonical_path(user) diff --git a/ee/app/models/ee/note.rb b/ee/app/models/ee/note.rb index 009c2745573284..aacaeb946ae610 100644 --- a/ee/app/models/ee/note.rb +++ b/ee/app/models/ee/note.rb @@ -157,6 +157,12 @@ def duo_bot_mentioned? # We don't want the bot to talk to itself return false if authored_by_duo_bot? + # We only care about threads that Duo Code Review created + return false unless discussion.first_note.author == duo_code_review_bot + + # FIXME do not merge this to production + return true if note.match?(/@GitLabDuo/) + mentioned_users.include?(duo_code_review_bot) end diff --git a/spec/features/admin/admin_users_impersonation_tokens_spec.rb b/spec/features/admin/admin_users_impersonation_tokens_spec.rb index 6b058ef31040e0..e82d98b1f072ca 100644 --- a/spec/features/admin/admin_users_impersonation_tokens_spec.rb +++ b/spec/features/admin/admin_users_impersonation_tokens_spec.rb @@ -8,7 +8,7 @@ let(:organization) { create(:organization) } let(:admin) { create(:admin, organizations: [organization]) } - let!(:user) { create(:user, organizations: [organization]) } + let!(:user) { create(:user, :with_namespace, organizations: [organization]) } before do sign_in(admin) diff --git a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb index 4d34cb55602792..fdc2c91d9269de 100644 --- a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb +++ b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb @@ -55,7 +55,7 @@ it 'enqueues ::GroupDestroyWorker for each group whose parent\'s do not exist and destroys them', :sidekiq_inline do parent.destroy! - expect { background_migration }.to change { namespaces.count }.from(10).to(6) + expect { background_migration }.to change { namespaces.count }.from(9).to(5) .and change { namespaces.where(id: orphaned_groups.pluck(:id)).count }.from(4).to(0) end end -- GitLab From 4a818cb0566b6377ec09ccd3505fd474f368bf11 Mon Sep 17 00:00:00 2001 From: Rutger Wessels Date: Wed, 15 Jan 2025 15:03:53 +0100 Subject: [PATCH 06/12] Remove unneeded organization creation --- spec/lib/users/internal_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/lib/users/internal_spec.rb b/spec/lib/users/internal_spec.rb index 28b3118830b1f0..86ae3415cd5966 100644 --- a/spec/lib/users/internal_spec.rb +++ b/spec/lib/users/internal_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' RSpec.describe Users::Internal, feature_category: :user_profile do - 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 -- GitLab From 5ebdcc03b3672752ff2c3c02258f111113f617fe Mon Sep 17 00:00:00 2001 From: Rutger Wessels Date: Wed, 22 Jan 2025 14:58:57 +0100 Subject: [PATCH 07/12] ensure duo_code_review_bot is an instance scoped bot user --- app/models/concerns/has_user_type.rb | 3 ++- spec/models/concerns/has_user_type_spec.rb | 14 +++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/app/models/concerns/has_user_type.rb b/app/models/concerns/has_user_type.rb index 89b9a09528092b..ea3ba1a1a9abae 100644 --- a/app/models/concerns/has_user_type.rb +++ b/app/models/concerns/has_user_type.rb @@ -47,6 +47,7 @@ module HasUserType # Changes to these types might have billing implications, https://docs.gitlab.com/ee/subscriptions/gitlab_com/#billable-users NON_INTERNAL_USER_TYPES = %w[human project_bot service_user service_account].freeze INTERNAL_USER_TYPES = (USER_TYPES.keys - NON_INTERNAL_USER_TYPES).freeze + INSTANCE_USER_TYPES = (INTERNAL_USER_TYPES - [:duo_code_review_bot]).freeze included do enum user_type: USER_TYPES @@ -76,7 +77,7 @@ def internal? def instance_user? return !Gitlab.com? if user_type == 'service_account' - INTERNAL_USER_TYPES.include?(user_type) + INSTANCE_USER_TYPES.include?(user_type) end # rubocop:enable Gitlab/AvoidGitlabInstanceChecks diff --git a/spec/models/concerns/has_user_type_spec.rb b/spec/models/concerns/has_user_type_spec.rb index 5908ecbf4235b3..a8a90ea514b956 100644 --- a/spec/models/concerns/has_user_type_spec.rb +++ b/spec/models/concerns/has_user_type_spec.rb @@ -9,7 +9,7 @@ let(:bots) { User::BOT_USER_TYPES.map { |type| public_send(type) } } let(:non_internal) { User::NON_INTERNAL_USER_TYPES.map { |type| public_send(type) } } let(:everyone) { User::USER_TYPES.keys.map { |type| public_send(type) } } - let(:internal) { User::INTERNAL_USER_TYPES.map { |type| public_send(type) } } + let(:instance) { User::INSTANCE_USER_TYPES.map { |type| public_send(type) } } specify 'types consistency checks', :aggregate_failures do expect(described_class::USER_TYPES.keys) @@ -208,21 +208,21 @@ describe 'instance_user?' do context 'when on SaaS', :saas do - it 'is true for all internal user types and false for others' do - expect(internal).to all(be_instance_user) + it 'is true for all instance user types and false for others' do + expect(instance).to all(be_instance_user) - (everyone - internal).each do |user| + (everyone - instance).each do |user| expect(user.instance_user?).to be(false) end end end context 'when on Self Managed' do - it 'is true for service_account and all internal user types and false for others' do - expect(internal).to all(be_instance_user) + it 'is true for service_account and all instance user types and false for others' do + expect(instance).to all(be_instance_user) expect(service_account.instance_user?).to be(true) - (everyone - internal - [service_account]).each do |user| + (everyone - instance - [service_account]).each do |user| expect(user.instance_user?).to be(false) end end -- GitLab From 0fab4dae7403bc5d159e61b7722df792b9c379af Mon Sep 17 00:00:00 2001 From: Rutger Wessels Date: Fri, 24 Jan 2025 16:45:16 +0100 Subject: [PATCH 08/12] Support users without personal namespace in UserReferenceFilter --- ee/app/models/ee/note.rb | 3 --- ee/app/models/ee/user.rb | 14 ++++++++++++-- .../filter/references/user_reference_filter.rb | 10 ++++++---- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/ee/app/models/ee/note.rb b/ee/app/models/ee/note.rb index aacaeb946ae610..65a30d8c5a0f21 100644 --- a/ee/app/models/ee/note.rb +++ b/ee/app/models/ee/note.rb @@ -160,9 +160,6 @@ def duo_bot_mentioned? # We only care about threads that Duo Code Review created return false unless discussion.first_note.author == duo_code_review_bot - # FIXME do not merge this to production - return true if note.match?(/@GitLabDuo/) - mentioned_users.include?(duo_code_review_bot) end diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index a766e1da29a000..f717baab8dd45f 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -44,9 +44,19 @@ module User after_create :associate_with_enterprise_group after_update :email_changed_hook, if: :saved_change_to_email? - delegate :shared_runners_minutes_limit, :shared_runners_minutes_limit=, - :extra_shared_runners_minutes_limit, :extra_shared_runners_minutes_limit=, + delegate :shared_runners_minutes_limit=, + :extra_shared_runners_minutes_limit=, to: :namespace + + # PoC branch: this address http 500 errors with users API + def shared_runners_minutes_limit + namespace&.shared_runners_minutes_limit + end + + def extra_shared_runners_minutes_limit + namespace&.extra_shared_runners_minutes_limit + end + delegate :provisioned_by_group, :provisioned_by_group=, :provisioned_by_group_id, :provisioned_by_group_id=, :onboarding_status_step_url, :onboarding_status_step_url=, diff --git a/lib/banzai/filter/references/user_reference_filter.rb b/lib/banzai/filter/references/user_reference_filter.rb index bce181b6a882d1..2dc10ae58134c5 100644 --- a/lib/banzai/filter/references/user_reference_filter.rb +++ b/lib/banzai/filter/references/user_reference_filter.rb @@ -51,6 +51,8 @@ def object_link_filter(text, pattern, link_content: nil, link_reference: false) cached_call(:banzai_url_for_object, match, path: [User, username.downcase]) do if namespace = namespaces[username.downcase] link_to_namespace(namespace, link_content: link_content) || match + elsif user = User.by_username(username).first + link_to_user(user, link_content: link_content) else match end @@ -118,12 +120,12 @@ def link_to_group(group, namespace, link_content: nil) link_tag(url, data, content, namespace.full_name) end - def link_to_user(user, namespace, link_content: nil) + def link_to_user(user, link_content: nil) url = urls.user_url(user, only_path: context[:only_path]) - data = data_attribute(user: namespace.owner_id) - content = link_content || (User.reference_prefix + user) + data = data_attribute(user: user.id) + content = link_content || (User.reference_prefix + user.username) - link_tag(url, data, content, namespace.owner_name) + link_tag(url, data, content, user.username) end def link_tag(url, data, link_content, title) -- GitLab From c47232bda99fd063741376901e271e5843b19ce5 Mon Sep 17 00:00:00 2001 From: Rutger Wessels Date: Fri, 24 Jan 2025 16:51:45 +0100 Subject: [PATCH 09/12] Fix UserNamespace links --- lib/banzai/filter/references/user_reference_filter.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/banzai/filter/references/user_reference_filter.rb b/lib/banzai/filter/references/user_reference_filter.rb index 2dc10ae58134c5..8dc59bf6c029e3 100644 --- a/lib/banzai/filter/references/user_reference_filter.rb +++ b/lib/banzai/filter/references/user_reference_filter.rb @@ -108,7 +108,7 @@ def link_to_namespace(namespace, link_content: nil) if namespace.is_a?(Group) link_to_group(namespace.full_path, namespace, link_content: link_content) else - link_to_user(namespace.path, namespace, link_content: link_content) + link_to_user(namespace.owner, link_content: link_content) end end -- GitLab From 1259677296f224ce85e39e96467567e9c02891cd Mon Sep 17 00:00:00 2001 From: agius Date: Wed, 5 Feb 2025 17:16:31 -0800 Subject: [PATCH 10/12] Fix N+1 query in UserReferenceFilter --- lib/banzai/filter/references/user_reference_filter.rb | 6 +++++- .../banzai/filter/references/user_reference_filter_spec.rb | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/banzai/filter/references/user_reference_filter.rb b/lib/banzai/filter/references/user_reference_filter.rb index 8dc59bf6c029e3..518e7a28dff3c5 100644 --- a/lib/banzai/filter/references/user_reference_filter.rb +++ b/lib/banzai/filter/references/user_reference_filter.rb @@ -51,7 +51,7 @@ def object_link_filter(text, pattern, link_content: nil, link_reference: false) cached_call(:banzai_url_for_object, match, path: [User, username.downcase]) do if namespace = namespaces[username.downcase] link_to_namespace(namespace, link_content: link_content) || match - elsif user = User.by_username(username).first + elsif user = users[username.downcase] link_to_user(user, link_content: link_content) else match @@ -73,6 +73,10 @@ def namespaces .transform_keys(&:downcase) end + def users + @users ||= User.by_username(usernames).index_by(&:username).transform_keys(&:downcase) + end + # Returns all usernames referenced in the current document. def usernames refs = Set.new diff --git a/spec/lib/banzai/filter/references/user_reference_filter_spec.rb b/spec/lib/banzai/filter/references/user_reference_filter_spec.rb index d37993cb316b76..c0f7d4c5ce0229 100644 --- a/spec/lib/banzai/filter/references/user_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/references/user_reference_filter_spec.rb @@ -230,7 +230,7 @@ def get_reference(user) let(:reference3) { group.to_reference } it 'does not have N+1 per multiple user references', :use_sql_query_cache do - markdown = reference.to_s + markdown = "#{reference} @qwertyuiopzx" reference_filter(markdown) # warm up control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do -- GitLab From daa42abc662e7eca997e6909acce6f46c364c8df Mon Sep 17 00:00:00 2001 From: agius Date: Wed, 5 Feb 2025 19:42:25 -0800 Subject: [PATCH 11/12] Add username lookup to User find_routeable Also fix specs --- app/models/user.rb | 2 +- ee/app/models/ee/note.rb | 3 --- ee/spec/services/ee/merge_requests/update_service_spec.rb | 2 +- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index d64a12e5a1dbcb..769c0db34d054d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1062,7 +1062,7 @@ def find_by_ssh_key_id(key_id) def find_by_full_path(path, follow_redirects: false) namespace = Namespace.user_namespaces.find_by_full_path(path, follow_redirects: follow_redirects) - namespace&.owner + namespace&.owner || find_by_username(path) end def reference_prefix diff --git a/ee/app/models/ee/note.rb b/ee/app/models/ee/note.rb index 65a30d8c5a0f21..009c2745573284 100644 --- a/ee/app/models/ee/note.rb +++ b/ee/app/models/ee/note.rb @@ -157,9 +157,6 @@ def duo_bot_mentioned? # We don't want the bot to talk to itself return false if authored_by_duo_bot? - # We only care about threads that Duo Code Review created - return false unless discussion.first_note.author == duo_code_review_bot - mentioned_users.include?(duo_code_review_bot) end diff --git a/ee/spec/services/ee/merge_requests/update_service_spec.rb b/ee/spec/services/ee/merge_requests/update_service_spec.rb index 053dbf106e44d5..1c468b6b9e7031 100644 --- a/ee/spec/services/ee/merge_requests/update_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/update_service_spec.rb @@ -599,7 +599,7 @@ def update_merge_request(opts) old_associations: { approval_rules: [], labels: [], - mentioned_users: [], + mentioned_users: [user2], assignees: [user3], closing_issues_ids: [], reviewers: [], -- GitLab From 0c032e4cb2376ab6e9b383952804aa3d60e1d568 Mon Sep 17 00:00:00 2001 From: agius Date: Thu, 6 Feb 2025 10:59:59 -0800 Subject: [PATCH 12/12] re-remove User#create_default_organization_user method --- app/models/user.rb | 7 ------- 1 file changed, 7 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 769c0db34d054d..ea2a2d7fc57b46 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2913,13 +2913,6 @@ def prefix_for_feed_token FEED_TOKEN_PREFIX end - def create_default_organization_user - return unless organizations.blank? - return if instance_user? # No organization for instance users - - Organizations::OrganizationUser.create_default_organization_record_for(id, user_is_admin: admin?) - end - # method overridden in EE def audit_lock_access(reason: nil); end -- GitLab