diff --git a/app/models/namespace.rb b/app/models/namespace.rb index b6c8630a99468d213ce58eadd12ccb4fb5455d03..22c2abe72354bb9881034e61701d529f254f921b 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -163,6 +163,7 @@ class Namespace < ApplicationRecord validate :changing_shared_runners_enabled_is_allowed, unless: -> { project_namespace? } validate :changing_allow_descendants_override_disabled_shared_runners_is_allowed, unless: -> { project_namespace? } validate :parent_organization_match + validate :no_conflict_with_organization_user_details, if: :path_changed? delegate :name, to: :owner, allow_nil: true, prefix: true delegate :avatar_url, to: :owner, allow_nil: true @@ -389,6 +390,15 @@ def username_reserved?(username) without_project_namespaces.top_level.find_by_path_or_name(username).present? end + def username_reserved_for_organization?(username, organization, excluding: []) + without_project_namespaces + .top_level + .in_organization(organization) + .where.not(id: excluding) + .find_by_path_or_name(username) + .present? + end + def self_or_ancestors_archived_setting_subquery namespace_setting_reflection = reflect_on_association(:namespace_settings) namespace_setting_table = Arel::Table.new(namespace_setting_reflection.table_name) @@ -840,6 +850,14 @@ def parent_organization_match errors.add(:organization_id, _("must match the parent organization's ID")) end + # route / path global uniqueness is handled by Routeable concern + # here we are checking only for conflicts with per-organization username aliases + def no_conflict_with_organization_user_details + return unless Organizations::OrganizationUserDetail.for_organization(organization).with_usernames(path).any? + + errors.add(:path, _('has already been taken')) + end + def cross_namespace_reference?(from) return false if from == self diff --git a/app/models/organizations/organization_user_detail.rb b/app/models/organizations/organization_user_detail.rb index 96862832a0327469f5e13fcd2392ed1d89650bbe..45aa18fae674d768b039417acb9c136d3e8cb4e5 100644 --- a/app/models/organizations/organization_user_detail.rb +++ b/app/models/organizations/organization_user_detail.rb @@ -2,10 +2,55 @@ module Organizations class OrganizationUserDetail < ApplicationRecord + include Referable + belongs_to :organization, inverse_of: :organization_user_details, optional: false belongs_to :user, inverse_of: :organization_user_details, optional: false validates :username, presence: true, uniqueness: { scope: :organization_id } validates :display_name, presence: true + + validate :no_namespace_conflicts + + scope :for_references, -> { includes(:organization, :user) } + scope :for_organization, ->(organization) { where(organization: organization) } + scope :with_usernames, ->(*usernames) { + uniq_usernames = usernames.flatten.compact.uniq + return none if uniq_usernames.blank? + + downcase_usernames = uniq_usernames.map(&:downcase) + + where("LOWER(username) IN (?)", downcase_usernames) + } + + # Referable methods should be the same as User + def reference_prefix + '@' + end + + def reference_pattern + @reference_pattern ||= + %r{ + (?#{Gitlab::PathRegex::FULL_NAMESPACE_FORMAT_REGEX}) + }x + end + + def to_reference(*) + "#{reference_prefix}#{username}" + end + + def no_namespace_conflicts + return if username.blank? + + return unless Namespace.username_reserved_for_organization?( + username, + organization, + excluding: user.namespace + ) + + errors.add(:username, _('has already been taken')) + end end end diff --git a/app/models/user.rb b/app/models/user.rb index 1e6637291d9c41eef49ee80c072d2669303cb4fa..88b65fbe2c46f7b5a99e32e012ef641ec6ab564d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -685,6 +685,10 @@ def blocked? .includes(:projects) end + scope :with_organization_user_details, -> do + includes(organization_user_details: [:organization]) + end + scope :left_join_user_detail, -> { left_joins(:user_detail) } scope :preload_user_detail, -> { preload(:user_detail) } diff --git a/app/services/concerns/users/participable_service.rb b/app/services/concerns/users/participable_service.rb index a8e7d238414fdece1fb75bed5409282ac95e0f70..76f00c52e850539883d7f94abd1ed869aee8d5f0 100644 --- a/app/services/concerns/users/participable_service.rb +++ b/app/services/concerns/users/participable_service.rb @@ -73,6 +73,8 @@ def participant_as_hash(participant) group_as_hash(participant) when User user_as_hash(participant) + when Organizations::OrganizationUserDetail + org_user_detail_as_hash(participant) else participant end @@ -99,6 +101,19 @@ def group_as_hash(group) } end + def org_user_detail_as_hash(org_user_detail) + user = org_user_detail.user + { + type: user.class.name, + username: org_user_detail.username, + name: org_user_detail.display_name, + avatar_url: user.avatar_url, + availability: lazy_user_availability(user).itself, # calling #itself to avoid returning a BatchLoader instance + original_username: user.username, + original_displayname: user.name + } + end + def group_counts groups_for_count = params[:search] ? groups : current_user.authorized_groups diff --git a/app/services/projects/participants_service.rb b/app/services/projects/participants_service.rb index 7e9dbce4e6d7a8e16750d0f8ce88d92ae621536f..cbf8df4e57ed7d24fbd44d70a685f9d471c5e328 100644 --- a/app/services/projects/participants_service.rb +++ b/app/services/projects/participants_service.rb @@ -15,8 +15,9 @@ def execute(noteable) project_members participants += groups unless relation_at_search_limit?(project_members) + participants = organization_user_details_for_participants(participants.uniq) - render_participants_as_hash(participants.uniq) + render_participants_as_hash(participants) end def project_members @@ -31,7 +32,31 @@ def all_members end def project_members_relation - project.authorized_users + project.authorized_users.with_organization_user_details + end + + private + + def organization + project.organization + end + strong_memoize_attr :organization + + # for users that have an OrganizationUserDetail for the current organization, use this instead + # of the User model for rendering username, display_name, and other details + # details should be pre-loaded to avoid N+1 queries + def organization_user_details_for_participants(participants) + return participants unless Feature.enabled?(:organization_users_internal, organization) + + participants.map do |participant| + next participant unless participant.is_a?(User) + + detail = participant.organization_user_details.to_a.find do |det| + det.organization == organization + end + + detail.presence || participant + end end end end diff --git a/config/feature_flags/beta/organization_users_internal.yml b/config/feature_flags/beta/organization_users_internal.yml new file mode 100644 index 0000000000000000000000000000000000000000..697feb9aa273fb51ac2c3a3ed0ab45b36d4e386b --- /dev/null +++ b/config/feature_flags/beta/organization_users_internal.yml @@ -0,0 +1,10 @@ +--- +name: organization_users_internal +description: +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/442780 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/190607 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/547109 +milestone: '18.1' +group: group::authentication +type: beta +default_enabled: false diff --git a/lib/banzai/filter/references/user_reference_filter.rb b/lib/banzai/filter/references/user_reference_filter.rb index bce181b6a882d10490ffacb37dbd8c45fce4b6a2..cbfdcf037e568f2bbb68133e0ade2b496cbefbc2 100644 --- a/lib/banzai/filter/references/user_reference_filter.rb +++ b/lib/banzai/filter/references/user_reference_filter.rb @@ -7,6 +7,8 @@ module References # # A special `@all` reference is also supported. class UserReferenceFilter < ReferenceFilter + include Gitlab::Utils::StrongMemoize + self.reference_type = :user self.object_class = User @@ -49,7 +51,10 @@ def object_link_filter(text, pattern, link_content: nil, link_reference: false) link_to_all(link_content: link_content) else cached_call(:banzai_url_for_object, match, path: [User, username.downcase]) do - if namespace = namespaces[username.downcase] + # order is important: per-organization usernames should be checked before global namespace + if org_user_detail = org_user_details[username.downcase] + link_to_org_user_detail(org_user_detail) + elsif namespace = namespaces[username.downcase] link_to_namespace(namespace, link_content: link_content) || match else match @@ -65,11 +70,25 @@ def object_link_filter(text, pattern, link_content: nil, link_reference: false) # The keys of this Hash are the namespace paths, the values the # corresponding Namespace objects. def namespaces - @namespaces ||= Namespace.preload(:owner, :route) - .where_full_path_in(usernames) - .index_by(&:full_path) - .transform_keys(&:downcase) + Namespace.preload(:owner, :route) + .where_full_path_in(usernames) + .index_by(&:full_path) + .transform_keys(&:downcase) end + strong_memoize_attr :namespaces + + # check for users that have an aliased name within an organization, + # for example the bot users created by Users::Internal + def org_user_details + return {} unless Feature.enabled?(:organization_users_internal, organization) + + Organizations::OrganizationUserDetail.for_references + .for_organization(organization) + .with_usernames(usernames) + .index_by(&:username) + .transform_keys(&:downcase) + end + strong_memoize_attr :org_user_details # Returns all usernames referenced in the current document. def usernames @@ -126,10 +145,26 @@ def link_to_user(user, namespace, link_content: nil) link_tag(url, data, content, namespace.owner_name) end + def link_to_org_user_detail(org_user_detail, link_content: nil) + user = org_user_detail.user + url = urls.user_url(user, only_path: context[:only_path]) + data = data_attribute(user: user.id) + content = link_content || org_user_detail.to_reference + + link_tag(url, data, content, org_user_detail.username) + end + def link_tag(url, data, link_content, title) %(#{link_content}) end + def organization + parent&.organization || + context[:author]&.organizations&.first || + Organizations::Organization.first + end + strong_memoize_attr :organization + def parent context[:project] || context[:group] end diff --git a/lib/users/internal.rb b/lib/users/internal.rb index 095c35bbc64c185d696d665319080d6fafc56c87..777fe401d283d64eedd6123319bb4faf015edd9d 100644 --- a/lib/users/internal.rb +++ b/lib/users/internal.rb @@ -28,6 +28,8 @@ def support_bot_id strong_memoize_attr :support_bot_id end + include Gitlab::Utils::StrongMemoize + # rubocop:disable CodeReuse/ActiveRecord -- Need to instantiate a record here def initialize(organization: nil) @organization = organization @@ -40,7 +42,7 @@ def ghost 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' + u.name = 'Ghost' end end @@ -154,13 +156,37 @@ def bot_avatar(image:) # 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 + if @organization && organization_users_internal_enabled? scope = scope.joins(:organization_users).where(organization_users: { organization: @organization }) end scope.first || create_unique_internal(scope, username, email_pattern, &block) end + def username_with_organization_suffix(username) + return username if @organization.nil? || @organization == first_organization + return username unless organization_users_internal_enabled? + + [username, @organization.path].join('_') + end + + def display_name_with_organization_suffix(display_name) + return display_name if @organization.nil? || @organization == first_organization + return display_name unless organization_users_internal_enabled? + + "#{display_name} (#{@organization.name})" + end + + def first_organization + Organizations::Organization.first + end + strong_memoize_attr :first_organization + + def organization_users_internal_enabled? + Feature.enabled?(:organization_users_internal, @organization) + end + strong_memoize_attr :organization_users_internal_enabled? + 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. @@ -188,22 +214,40 @@ def create_unique_internal(scope, username, email_pattern, &creation_block) uniquify = Gitlab::Utils::Uniquify.new - username = uniquify.string(username) { |s| Namespace.by_path(s) } + global_username = username_with_organization_suffix(username) + global_username = uniquify.string(global_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, + username: global_username, email: email, &creation_block ) - user_organization = @organization || Organizations::Organization.first + user_organization = if @organization && organization_users_internal_enabled? + @organization + else + Organizations::Organization.first + end + user.assign_personal_namespace(user_organization) user.organizations << user_organization + uniquify = Gitlab::Utils::Uniquify.new + organization_username = uniquify.string(username) { |s| Namespace.in_organization(user_organization).by_path(s) } + + if organization_users_internal_enabled? + org_user_details = user_organization.organization_user_details.build( + user: user, + username: organization_username, + display_name: display_name_with_organization_suffix(user.name) + ) + user.organization_user_details << org_user_details + end + Users::UpdateService.new(user, user: user).execute(validate: false) user ensure diff --git a/spec/factories/users.rb b/spec/factories/users.rb index e096221d401285db8c3ec8aa8b6181364c3cdde9..e718a645bd8bd7c44991550689fd4cc3d5a4d775 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -12,6 +12,10 @@ color_scheme_id { 1 } color_mode_id { 1 } + transient do + in_organization { create(:organization) } # rubocop: disable RSpec/FactoryBot/InlineAssociation -- required since this is a transient attribute, not a factory association + end + after(:build) do |user, evaluator| # UserWithNamespaceShim is not defined in gdk reset-data. We assume the shim is enabled in this case. assign_ns = if defined?(UserWithNamespaceShim) @@ -27,7 +31,7 @@ .order(:created_at).first || # We create an organization next even though we are building here. We need to ensure # that an organization exists so other entities can belong to the same organization - create(:organization) + evaluator.in_organization user.assign_personal_namespace(org) end @@ -38,15 +42,12 @@ end trait :with_namespace do - # rubocop: disable RSpec/FactoryBot/InlineAssociation -- We need to pass an Organization to this method - namespace { assign_personal_namespace(create(:organization)) } - # rubocop: enable RSpec/FactoryBot/InlineAssociation + namespace { assign_personal_namespace(in_organization) } end trait :with_organization do - after(:create) do |user| - organization = create(:organization) - create(:organization_user, user: user, organization: organization) + after(:create) do |user, evaluator| + create(:organization_user, user: user, organization: evaluator.in_organization) end end diff --git a/spec/features/participants_autocomplete_spec.rb b/spec/features/participants_autocomplete_spec.rb index dbeca6016175905d7b3122aa7e1c11fcbfb94682..9ae4e63c186e3db8494db2092f17ed95a89cd401 100644 --- a/spec/features/participants_autocomplete_spec.rb +++ b/spec/features/participants_autocomplete_spec.rb @@ -86,4 +86,42 @@ include_examples "open suggestions when typing @", 'commit' end + + context 'when mentioning users with OrganizationUserDetail username alias' do + let_it_be(:author) { create(:user, :with_organization) } + let_it_be(:organization) { author.organizations.first } + let_it_be(:project) { create(:project, :public, :repository, organization: organization) } + let_it_be(:admin_bot) { Users::Internal.for_organization(organization).admin_bot } + + let(:organization_user_detail) { admin_bot.organization_user_details.first } + let(:resource_name) { 'issue' } + + let!(:note) { create(:note, noteable: noteable, project: noteable.project) } + let(:noteable) { create(:issue, author: author, project: project) } + + before_all do + project.add_owner(admin_bot) + end + + it 'creates admin_bot with OrganizationUserDetail alias' do + expect(admin_bot.organization_user_details.count).to eq(1) + expect(admin_bot.username).not_to eq(organization_user_detail.username) + + visit project_issue_path(project, noteable) + fill_in 'Comment', with: "#{User.reference_prefix}#{admin_bot.username[0..2]}" + + expect(find_autocomplete_menu).to have_text(organization_user_detail.username) + expect(find_autocomplete_menu).not_to have_text(admin_bot.username) + + send_keys [:arrow_down, :enter] + + click_on 'Comment' + wait_for_requests + + expect(page).to have_link( + "#{User.reference_prefix}#{organization_user_detail.username}", + href: user_path(admin_bot) + ) + end + end end 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..527395eab9d96bb2c4bf1e92767303340cb90068 100644 --- a/spec/lib/banzai/filter/references/user_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/references/user_reference_filter_spec.rb @@ -223,6 +223,80 @@ def get_reference(user) end end + describe '#org_user_detail' do + let(:org_user_detail) { create(:organization_user_detail, organization: project.organization) } + let(:author) { create(:user, in_organization: project.organization) } + + it 'supports mentioning users aliased within organization' do + reference = org_user_detail.to_reference + doc = reference_filter("Hey #{reference}", project: project) + + expect(doc.css('a').first.attr('href')).to eq urls.user_url(org_user_detail.user) + end + + it 'supports mentioning user aliases for snippets' do + reference = org_user_detail.to_reference + doc = reference_filter("Hey #{reference}", author: author) + + expect(doc.css('a').first.attr('href')).to eq urls.user_url(org_user_detail.user) + end + + context 'with no context for filter' do + let!(:user) { create(:user, :with_organization) } + let!(:org_user_detail) { create(:organization_user_detail, user: user, organization: user.organizations.first) } + + it 'supports mentioning user aliases from default organization' do + expect(Organizations::Organization.count).to eq(1) + + reference = org_user_detail.to_reference + doc = reference_filter("Hey #{reference}") + + expect(doc.css('a').first.attr('href')).to eq urls.user_url(org_user_detail.user) + end + end + + context 'in group context' do + let(:group) { create(:group, developers: [group_member]) } + let(:group_member) { create(:user) } + let(:org_user_detail) { create(:organization_user_detail, organization: group.organization) } + let(:context) { { author: group_member, project: nil, group: group } } + + it 'supports mentioning a single user' do + reference = org_user_detail.to_reference + doc = reference_filter("Hey #{reference}", context) + + expect(doc.css('a').first.attr('href')).to eq urls.user_url(org_user_detail.user) + end + end + + context 'when organization_users_internal FF is disabled' do + before do + stub_feature_flags(organization_users_internal: false) + end + + it 'does not support mentioning users aliased within organization' do + reference = org_user_detail.to_reference + doc = reference_filter("Hey #{reference}", project: project) + + expect(doc.css('a')).to be_empty + end + + context 'in group context' do + let(:group) { create(:group, developers: [group_member]) } + let(:group_member) { create(:user) } + let(:org_user_detail) { create(:organization_user_detail, organization: group.organization) } + let(:context) { { author: group_member, project: nil, group: group } } + + it 'does not support mentioning a single user' do + reference = org_user_detail.to_reference + doc = reference_filter("Hey #{reference}", context) + + expect(doc.css('a')).to be_empty + end + end + end + end + context 'checking N+1' do let(:user2) { create(:user) } let(:group) { create(:group) } diff --git a/spec/lib/users/internal_spec.rb b/spec/lib/users/internal_spec.rb index 362022869777ec4ee100ed105e365a1c6c358bd7..f21e3f8c1deb2a163db042428d65a1064d05869f 100644 --- a/spec/lib/users/internal_spec.rb +++ b/spec/lib/users/internal_spec.rb @@ -7,6 +7,14 @@ let_it_be(:organization) { create(:organization) } shared_examples 'bot users' do |bot_type, username, email| + subject(:bot_user) { described_class.for_organization(organization).public_send(bot_type) } + + let(:bot_username) do + described_class.for_organization(organization).send( + :username_with_organization_suffix, username + ) + end + it 'creates the user if it does not exist' do expect do described_class.for_organization(organization).public_send(bot_type) @@ -14,18 +22,28 @@ end it 'creates a route for the namespace of the created user' do - 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 organization to the created user' do - bot_user = described_class.for_organization(organization).public_send(bot_type) + it 'creates a user with a global username suffixed with the organization name' do + expect(bot_user.username).to include(username) + expect(bot_user.username).to include(organization.path) + expect(bot_user.name).not_to include("(#{organization.name})") + end + it 'assigns the organization to the created user' do expect(bot_user.organizations).to eq([organization]) end + it 'creates an organization_user_detail with a per-organization username' do + expect(bot_user.organization_user_details.count).to eq(1) + + detail = bot_user.organization_user_details.first + expect(detail.username).to eq(username) + expect(detail.display_name).to include("(#{organization.name})") + end + it 'does not create a new user if it already exists' do described_class.for_organization(organization).public_send(bot_type) @@ -35,13 +53,37 @@ end context 'when a regular user exists with the bot username' do - it 'creates a user with a non-conflicting username' do - create(:user, username: username) + let!(:user) { create(:user, :with_namespace, username: bot_username) } + it 'creates a user with a non-conflicting username' do expect do - described_class.for_organization(organization).public_send(bot_type) + bot_user end.to change { User.where(user_type: bot_type).count }.by(1) end + + it 'creates organization_user_detail with non-conflicting username' do + expect(bot_user.organization_user_details.count).to eq(1) + + detail = bot_user.organization_user_details.first + expect(detail.username).not_to eq(user.username) + end + + context 'when user belongs to another organization' do + let!(:user) { create(:user, :with_namespace, username: bot_username, in_organization: first_organization) } + + it 'creates a non-conflicting global username and simple per-org username' do + expect do + bot_user + end.to change { User.where(user_type: bot_type).count }.by(1) + + expect(bot_user.username).not_to eq(user.username) + + expect(bot_user.organization_user_details.count).to eq(1) + + detail = bot_user.organization_user_details.first + expect(detail.username).to eq(username) + end + end end context 'when a regular user exists with the bot user email' do @@ -49,19 +91,43 @@ create(:user, email: email) expect do - described_class.for_organization(organization).public_send(bot_type) + bot_user end.to change { User.where(user_type: bot_type).count }.by(1) end end context 'when a group namespace exists with path that is equal to the bot username' do - it 'creates a user with a non-conflicting username' do - create(:group, path: username) + let!(:group) { create(:group, path: bot_username) } + it 'creates a user with a non-conflicting username' do expect do - described_class.for_organization(organization).public_send(bot_type) + bot_user end.to change { User.where(user_type: bot_type).count }.by(1) end + + it 'creates organization_user_detail with non-conflicting username' do + expect(bot_user.organization_user_details.count).to eq(1) + + detail = bot_user.organization_user_details.first + expect(detail.username).not_to eq(group.path) + end + + context 'when the group namespace is in another organization' do + let!(:group) { create(:group, path: bot_username, organization: first_organization) } + + it 'creates a non-conflicting global username and simple per-org username' do + expect do + bot_user + end.to change { User.where(user_type: bot_type).count }.by(1) + + expect(bot_user.username).not_to eq(bot_username) + + expect(bot_user.organization_user_details.count).to eq(1) + + detail = bot_user.organization_user_details.first + expect(detail.username).to eq(username) + end + end end context 'when a domain allowlist is in place' do @@ -71,10 +137,50 @@ it 'creates the bot user' do expect do - described_class.for_organization(organization).public_send(bot_type) + bot_user end.to change { User.where(user_type: bot_type).count }.by(1) end end + + context 'when no organization is passed' do + subject(:bot_user) { described_class.public_send(bot_type) } + + it 'sets username without suffix' do + expect(bot_user.username).to eq(username) + end + + it 'sets display name without suffix' do + expect(bot_user.name).not_to include(organization.name) + end + end + + context 'when organization_users_internal FF is disabled' do + before do + stub_feature_flags(organization_users_internal: false) + end + + it 'assigns the default organization to the created user' do + expect(bot_user.organizations).to eq([first_organization]) + end + + it 'does not create a new user if one already exists for the first organization' do + described_class.for_organization(first_organization).public_send(bot_type) + + expect do + described_class.for_organization(organization).public_send(bot_type) + end.not_to change { User.count } + end + + it 'creates a user with no username or display name suffix' do + expect(bot_user.username).to include(username) + expect(bot_user.username).not_to include(organization.path) + expect(bot_user.name).not_to include("(#{organization.name})") + end + + it 'does not create an organization_user_detail' do + expect(bot_user.organization_user_details).to be_empty + end + end end shared_examples 'bot user avatars' do |bot_type, avatar_filename| diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index c0f4cd2e71f4558791da25a092fb4b0ec5ec0a4a..7134adae1dddcd5c4f00a99e27e48f894ee48c0b 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -283,6 +283,35 @@ end end end + + describe '#no_conflict_with_organization_user_details' do + let!(:user) { create(:user, :with_namespace, in_organization: organization) } + let!(:organization_user_detail) do + create(:organization_user_detail, organization: organization, user: user, username: 'test-username') + end + + let(:other_organization) { create(:organization) } + + it 'ensures no Organizations::OrganizationUserDetail has username conflicting with path' do + namespace = build(:namespace, path: 'test-username', organization: organization) + + expect(namespace).not_to be_valid + expect(namespace.errors[:path]).to include('has already been taken') + end + + it 'allows conflicting usernames for other organizations' do + namespace = build(:namespace, path: 'test-username', organization: other_organization) + + expect(namespace).to be_valid + end + + it 'does not check validation unless path has changed' do + namespace = create(:namespace) + + expect(namespace).not_to receive(:no_conflict_with_organization_user_details) + namespace.update!(two_factor_grace_period: 36) + end + end end describe "ReferencePatternValidation" do @@ -1794,6 +1823,92 @@ end end + describe ".username_reserved_for_organization?" do + subject(:username_reserved) { described_class.username_reserved_for_organization?(username, organization) } + + let(:username) { 'capyabra' } + + let_it_be(:user) { create(:user, name: 'capybara') } + let_it_be(:group) { create(:group, name: 'capybara-group') } + let_it_be(:subgroup) { create(:group, parent: group, name: 'capybara-subgroup') } + let_it_be(:project) { create(:project, group: group, name: 'capybara-project') } + + let_it_be(:other_organization) { create(:organization) } + let_it_be(:user_other_org) do + create(:user, :with_namespace, username: 'other-capybara', in_organization: other_organization) + end + + let_it_be(:group_other_org) { create(:group, path: 'other-capybara-group', organization: other_organization) } + + context 'when given a project name' do + let(:username) { 'capyabra-project' } + + it { is_expected.to eq(false) } + end + + context 'when given a sub-group name' do + let(:username) { 'capybara-subgroup' } + + it { is_expected.to eq(false) } + end + + context 'when given a top-level group' do + let(:username) { 'capybara-group' } + + it { is_expected.to eq(true) } + end + + context 'when given a top-level group in another organization' do + let(:username) { 'other-capybara-group' } + + it { is_expected.to eq(false) } + end + + context 'when given an existing username' do + let(:username) { 'capybara' } + + it { is_expected.to eq(true) } + end + + context 'when excluding a particular namespace from the check' do + subject(:username_reserved) do + described_class.username_reserved_for_organization?(username, organization, excluding: [user.namespace]) + end + + let(:username) { 'capybara' } + + it { is_expected.to eq(false) } + end + + context 'when exclude parameter contains nil values' do + subject(:username_reserved) do + described_class.username_reserved_for_organization?(username, organization, excluding: [nil, nil]) + end + + let(:username) { 'capybara' } + + it { is_expected.to eq(true) } + end + + context 'when given an existing username in another organization' do + let(:username) { 'other-capybara' } + + it { is_expected.to eq(false) } + end + + context 'when given a username with varying capitalization' do + let(:username) { 'CaPyBaRa' } + + it { is_expected.to eq(true) } + end + + context 'when given a username with varying capitalization in another organization' do + let(:username) { 'OtHeR-CaPyBaRa' } + + it { is_expected.to eq(false) } + end + end + describe "#default_branch_protection" do let(:namespace) { create(:namespace) } let(:default_branch_protection) { nil } diff --git a/spec/models/organizations/organization_user_detail_spec.rb b/spec/models/organizations/organization_user_detail_spec.rb index 268f6009db94ec29f1c94c9d5df685e0de0b4c51..ce0486ea9f88d7caf0bd6acf459fa10dbd29117a 100644 --- a/spec/models/organizations/organization_user_detail_spec.rb +++ b/spec/models/organizations/organization_user_detail_spec.rb @@ -14,5 +14,131 @@ it { is_expected.to validate_presence_of(:username) } it { is_expected.to validate_presence_of(:display_name) } it { is_expected.to validate_uniqueness_of(:username).scoped_to(:organization_id) } + + describe '#no_namespace_conflicts' do + subject(:organization_user_detail) do + build(:organization_user_detail, username: username, organization: organization) + end + + let_it_be(:organization) { create(:organization) } + let_it_be(:other_organization) { create(:organization) } + + let(:username) { 'capybara' } + + it { is_expected.to be_valid } + + context 'when a User exists in the same organization with the same username' do + let!(:user) { create(:user, :with_namespace, username: username, in_organization: organization) } + + it 'adds a validation error on username' do + expect(organization_user_detail).not_to be_valid + expect(organization_user_detail.errors[:username]).to include('has already been taken') + end + + context 'when the user is the same as the OrganizationUserDetail' do + subject(:organization_user_detail) do + build(:organization_user_detail, user: user, username: username, organization: organization) + end + + it { is_expected.to be_valid } + end + end + + context 'when a User exists in another organization with the same username' do + let!(:user) { create(:user, :with_namespace, username: username, in_organization: other_organization) } + + it { is_expected.to be_valid } + end + + context 'when a group exists in the same organization with a path equal to the username' do + let!(:group) { create(:group, path: username, organization: organization) } + + it 'adds a validation error on username' do + expect(organization_user_detail).not_to be_valid + expect(organization_user_detail.errors[:username]).to include('has already been taken') + end + end + + context 'when a group exists in another organization with a path equal to the username' do + let!(:group) { create(:group, path: username, organization: other_organization) } + + it { is_expected.to be_valid } + end + end + end + + describe 'scopes' do + let_it_be(:organization_user_detail) { create(:organization_user_detail) } + + describe '.for_references' do + it 'includes related records' do + instance = nil + + ActiveRecord::QueryRecorder.new(skip_cached: false) do + instance = described_class.for_references.where(id: organization_user_detail.id).first + end + + experiment = ActiveRecord::QueryRecorder.new(skip_cached: false) do + expect(instance.user).to be_a(User) + expect(instance.organization).to be_a(Organizations::Organization) + end + + expect(experiment.count).to eq(0) + end + end + + describe '.for_organization' do + it 'scopes query to organization' do + expect(described_class.for_organization(organization_user_detail.organization).count).to eq(1) + expect(described_class.for_organization(99999).count).to eq(0) + end + end + + describe '.with_usernames' do + it 'locates all users within argument' do + username = organization_user_detail.username + expect(described_class.with_usernames('fakeusername', username).count).to eq(1) + end + + it 'matches usernames case-insensitively' do + username = organization_user_detail.username + expect(described_class.with_usernames('fakeusername', username.upcase).count).to eq(1) + end + + it 'returns none when no arguments / nil arguments passed' do + expect(described_class.with_usernames.count).to eq(0) + expect(described_class.with_usernames(nil).count).to eq(0) + end + end + + describe 'Referable methods' do + describe 'reference_prefix' do + subject(:reference_prefix) { described_class.new.reference_prefix } + + it { is_expected.to eq('@') } + end + + describe 'reference_pattern' do + subject(:reference_pattern) { described_class.new.reference_pattern } + + it 'matches @username patterns' do + expect(reference_pattern).to be_a(Regexp) + + expect(reference_pattern.match?('@username')).to be true + expect(reference_pattern.match?('@user.name')).to be true + expect(reference_pattern.match?('@user-name')).to be true + expect(reference_pattern.match?('@-name')).to be false + expect(reference_pattern.match?('user@name')).to be false + end + end + + describe 'to_reference' do + subject(:to_reference) { described_class.new(username: username).to_reference } + + let(:username) { 'test_user_name' } + + it { is_expected.to eq('@test_user_name') } + end + end end end diff --git a/spec/services/projects/participants_service_spec.rb b/spec/services/projects/participants_service_spec.rb index ffaa955df673eb584572b5afdd7f3bae349c1191..6dcf33added20bef7889df50cf3fcf9f2276b1f2 100644 --- a/spec/services/projects/participants_service_spec.rb +++ b/spec/services/projects/participants_service_spec.rb @@ -7,11 +7,19 @@ let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project, :public) } let_it_be(:noteable) { create(:issue, project: project) } + let_it_be(:org_user_detail) do + create(:organization_user_detail, organization: project.organization, username: 'spec_bot') + end + + let_it_be(:other_org_user_detail) do + create(:organization_user_detail, username: 'spec_bot') + end let(:params) { {} } before_all do project.add_developer(user) + project.add_developer(org_user_detail.user) end before do @@ -26,7 +34,7 @@ def run_service group = create(:group, owners: user) expect(run_service.pluck(:username)).to eq([ - noteable.author.username, 'all', user.username, group.full_path + noteable.author.username, 'all', user.username, org_user_detail.username, group.full_path ]) end @@ -65,6 +73,34 @@ def run_service expect { run_service }.not_to exceed_query_limit(control) end + + it 'avoids N+1 OrganizationUserDetail queries' do + developer = create(:user) + project.add_developer(developer) + create( + :organization_user_detail, + user: developer, + organization: project.organization, + username: 'test_alias', + display_name: 'Test McAlias' + ) + + control = ActiveRecord::QueryRecorder.new { run_service.to_a } + + BatchLoader::Executor.clear_current + + developer2 = create(:user) + project.add_developer(developer2) + create( + :organization_user_detail, + user: developer2, + organization: project.organization, + username: 'test_alias2', + display_name: 'Test McAlias2' + ) + + expect { run_service.to_a }.not_to exceed_query_limit(control) + end end it 'does not return duplicate author' do @@ -87,6 +123,56 @@ def run_service end end + describe 'organization_user_detail items' do + subject(:org_user_detail_items) { run_service.select { |hash| hash[:original_username].present? } } + + it 'includes items for per-organization user details within the noteable organization' do + expect(org_user_detail_items).to include(a_hash_including( + type: User.name, + username: org_user_detail.username, + name: org_user_detail.display_name, + avatar_url: org_user_detail.user.avatar_url, + original_username: org_user_detail.user.username, + original_displayname: org_user_detail.user.name + )) + end + + it 'does not include items from other organizations' do + expect(org_user_detail_items).not_to include(a_hash_including( + username: other_org_user_detail.username, + original_username: other_org_user_detail.user.username + )) + end + + context 'when organization_users_internal FF is disabled' do + before do + stub_feature_flags(organization_users_internal: false) + end + + it { is_expected.to be_empty } + + it 'returns results in correct order' do + group = create(:group, owners: user) + + expect(run_service.pluck(:username)).to eq([ + noteable.author.username, 'all', user.username, org_user_detail.user.username, group.full_path + ]) + end + end + + context 'including other item types' do + subject(:items) { run_service } + + it 'does not seprately include user for organization_user_details items' do + matching_items = items.select do |item| + [org_user_detail.username, org_user_detail.user.username].include?(item[:username]) + end + expect(matching_items.count).to eq(1) + expect(matching_items.first).to have_key(:original_username) + end + end + end + describe 'group items' do subject(:group_items) { run_service.select { |hash| hash[:type].eql?('Group') } }