diff --git a/app/models/concerns/has_user_type.rb b/app/models/concerns/has_user_type.rb index 2aba5d156e1be89e0975ee790f9fbe86c7918879..ea3ba1a1a9abae9df959fc368d0586020e6ddc17 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 @@ -72,6 +73,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' + + INSTANCE_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/app/models/user.rb b/app/models/user.rb index ccd4c95a205bc5e15f6e8648caff1d77af97d6bf..ea2a2d7fc57b461a27a668907f389d2a3a5f2038 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/user.rb b/ee/app/models/ee/user.rb index a766e1da29a0001cd2a26b503b73ef7dac379cef..f717baab8dd45fc8affd7c00346ff297a152cc8b 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/ee/spec/lib/ee/users/internal_spec.rb b/ee/spec/lib/ee/users/internal_spec.rb index f5fade809e8d6e12c5fe1669c0fafc07cc8c173d..dcd30c2a632b4453a5b228a443c80654708f29f3 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) 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 053dbf106e44d51158c02254e2a94a8ef53fa5c5..1c468b6b9e70312f90eabb2a0a43e0f1866c4f77 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: [], diff --git a/lib/banzai/filter/references/user_reference_filter.rb b/lib/banzai/filter/references/user_reference_filter.rb index bce181b6a882d10490ffacb37dbd8c45fce4b6a2..518e7a28dff3c5fe5d5ca829b38624b041d62851 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 = users[username.downcase] + link_to_user(user, link_content: link_content) else match end @@ -71,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 @@ -106,7 +112,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 @@ -118,12 +124,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) diff --git a/lib/users/internal.rb b/lib/users/internal.rb index cc38f6517cd62c8194a1ee79d32290a495a09235..377bd05fa0acfe1ccdac1e7cfb2122e1968893ec 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/features/admin/admin_users_impersonation_tokens_spec.rb b/spec/features/admin/admin_users_impersonation_tokens_spec.rb index 6b058ef31040e0a0c005bb21b2a8d9d2f6ca0d0b..e82d98b1f072cacfdce18ad32ba8bb447187f7e8 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/banzai/filter/references/user_reference_filter_spec.rb b/spec/lib/banzai/filter/references/user_reference_filter_spec.rb index d37993cb316b76d3f84e1146bd3b2ddbe34df2b6..c0f7d4c5ce022958643b1d5593d28fe00f1106d0 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 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 4d34cb55602792a32ce3add06ddbae0617c4b7a2..fdc2c91d9269deb3f4e27640230deb643bfb44fb 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 diff --git a/spec/lib/users/internal_spec.rb b/spec/lib/users/internal_spec.rb index 0e8ab182a3f0c5522a3e8edaaf1d72a75d5b3970..86ae3415cd5966baf28a87f9c7ab8a830715f06b 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 @@ -12,19 +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 - 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) diff --git a/spec/models/concerns/has_user_type_spec.rb b/spec/models/concerns/has_user_type_spec.rb index fc619485d0a48b81956dabc9c8dab630cbac472b..a8a90ea514b95695b4504b79fc7617393b8caa4c 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(:instance) { User::INSTANCE_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 instance user types and false for others' do + expect(instance).to all(be_instance_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 instance user types and false for others' do + expect(instance).to all(be_instance_user) + expect(service_account.instance_user?).to be(true) + + (everyone - instance - [service_account]).each do |user| + expect(user.instance_user?).to be(false) + end + end + end + end end end