diff --git a/app/models/concerns/bulk_users_by_email_load.rb b/app/models/concerns/bulk_users_by_email_load.rb index 55143ead30a892f962ac89d6d97990cb78a25501..08474b465ad02366f8d82ca62d303ca9c4e58c06 100644 --- a/app/models/concerns/bulk_users_by_email_load.rb +++ b/app/models/concerns/bulk_users_by_email_load.rb @@ -6,7 +6,11 @@ module BulkUsersByEmailLoad included do def users_by_emails(emails) Gitlab::SafeRequestLoader.execute(resource_key: user_by_email_resource_key, resource_ids: emails) do |emails| - # have to consider all emails - even secondary, so use all_emails here + # We have to consider all emails - even secondary, so use all_emails here to accomplish that. + # The by_any_email method will search for lowercased emails only, which means the + # private_commit_email values may not get cached properly due to it being able to be non-lowercased. + # That is likely ok as the use of those in the current use of this construct is likely very rare. + # Perhaps to be looked at more in https://gitlab.com/gitlab-org/gitlab/-/issues/461885 grouped_users_by_email = User.by_any_email(emails, confirmed: true).preload(:emails).group_by(&:all_emails) grouped_users_by_email.each_with_object({}) do |(found_emails, users), h| diff --git a/app/models/user.rb b/app/models/user.rb index 9dd8d16787bff6b4892daf41ec00e7e23a1a6f42..0595cbba1314b52ed01dddfaefe9228043710ab2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -803,6 +803,11 @@ def by_any_email(emails, confirmed: false) items = [from_users, from_emails] + # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/461885 + # What about private commit emails with capitalized username, we'd never find them and + # since the private_commit_email derives from the username, it can + # be uppercase in parts. So we'll never find an existing user during the invite + # process by email if that is true as we are case sensitive in this case. user_ids = Gitlab::PrivateCommitEmail.user_ids_for_emails(Array(emails).map(&:downcase)) items << where(id: user_ids) if user_ids.present? diff --git a/app/services/members/creator_service.rb b/app/services/members/creator_service.rb index 57159c14b3b783e10f3c80284d1c6bdcd76408fe..a28eed8b966ba45d8871e5418df151176ddca181 100644 --- a/app/services/members/creator_service.rb +++ b/app/services/members/creator_service.rb @@ -112,11 +112,23 @@ def parse_users_list(source, list) # de-duplicate just in case as there is no controlling if user records and ids are sent multiple times users.uniq! - users_by_emails = source.users_by_emails(emails) # preloads our request store for all emails + # We need to downcase any input of emails here for our caching so that emails sent in with uppercase + # are also found since all emails are stored in users, emails tables downcase. user.private_commit_emails are + # not though, so we'll never cache those I guess at this layer for now. + # Since there is possibility of duplicate values once we downcase, we'll de-duplicate. + # Uniq call here has no current testable impact as it will get the same parsed_emails + # result without it, but it merely helps it do a bit less work. + case_insensitive_emails = emails.map(&:downcase).uniq + users_by_emails = source.users_by_emails(case_insensitive_emails) # preloads our request store for all emails # in case emails belong to a user that is being invited by user or user_id, remove them from # emails and let users/user_ids handle it. + # parsed_emails have to preserve casing due to the invite process also being used to update + # existing members and we have to let them be found if not lowercased. parsed_emails = emails.select do |email| - user = users_by_emails[email] + # Since we are caching by lowercased emails as a key for the users as they only + # ever have lowercased emails(except for private_commit_emails), we need to then + # operate against that cache for lookups like here with a matching lowercase. + user = users_by_emails[email.downcase] !user || (users.exclude?(user) && user_ids.exclude?(user.id)) end diff --git a/app/services/members/invite_member_builder.rb b/app/services/members/invite_member_builder.rb index e925121bb1e796179785730386c218ca6620a349..5164ab443abdb4e64149141cfc331411924b9faa 100644 --- a/app/services/members/invite_member_builder.rb +++ b/app/services/members/invite_member_builder.rb @@ -6,14 +6,20 @@ def execute if user_by_email find_or_initialize_member_by_user(user_by_email.id) else - source.members_and_requesters.find_or_initialize_by(invite_email: invitee) # rubocop:disable CodeReuse/ActiveRecord + source.members_and_requesters.find_or_initialize_by(invite_email: invitee).tap do |record| # rubocop:disable CodeReuse/ActiveRecord -- TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/462165 + # We do not want to cause misses for existing records as the invite process is also sometimes used + # as a way to update existing invites. + record.invite_email = invitee.downcase if record.new_record? + end end end private def user_by_email - source.users_by_emails([invitee])[invitee] + # Since we cache the user lookups for the emails in lowercase format, we + # now need to look them up the same way to ensure we don't get cache misses. + source.users_by_emails([invitee.downcase])[invitee.downcase] end end end diff --git a/app/services/members/invite_service.rb b/app/services/members/invite_service.rb index 0534c59c255192a6f6078b15e86eac9943fca9e9..4426bb8ae6fd45bfb94b5e062f106e0351d0e341 100644 --- a/app/services/members/invite_service.rb +++ b/app/services/members/invite_service.rb @@ -56,7 +56,14 @@ def add_error_for_member(member, existing_errors) end def invited_object(member) - return member.invite_email if member.invite_email + if member.invite_email + # We reverse here as the case with duplicate emails on the same request the last one is likely the issue as + # the first one will be committed to db first and so it will be the last instance of that email that has + # the error. + # For updates, they can still have an upper case email, so we need compare case insensitively on the both sides + # of this find. + return invites.reverse.find { |email| email.casecmp?(member.invite_email) } + end # There is a case where someone was invited by email, but the `user` record exists. # The member record returned will not have an invite_email attribute defined since @@ -71,7 +78,11 @@ def invited_object(member) if member.user_id.to_s.in?(invites) member.user.username else - member.user.all_emails.detect { |email| email.in?(invites) } + # We find the correct match here case insensitively user.all_emails since it can + # have an uppercase email for private_commit_email. + # We need to downcase our invites against the rest since the user could input + # uppercase invite and we need to find the case insensitive match on that. + invites.find { |email| email.downcase.in?(member.user.all_emails.map(&:downcase)) } end end end diff --git a/spec/requests/api/invitations_spec.rb b/spec/requests/api/invitations_spec.rb index a2697fa55f590bea901ff0bd0952e278d323709f..dff3205f55d9dbe85fca9e9b84affe291d8f50f6 100644 --- a/spec/requests/api/invitations_spec.rb +++ b/spec/requests/api/invitations_spec.rb @@ -418,22 +418,24 @@ def invite_member_by_email(source, source_type, email, created_by, access_level: end it 'does not exceed expected queries count with secondary emails', :request_store, :use_sql_query_cache do - create(:email, email: email, user: create(:user)) + create(:email, :confirmed, email: email, user: create(:user)) post invitations_url(project, maintainer), params: { email: email, access_level: Member::DEVELOPER } - create(:email, email: email2, user: create(:user)) + create(:email, :confirmed, email: email2, user: create(:user)) control = ActiveRecord::QueryRecorder.new(skip_cached: false) do post invitations_url(project, maintainer), params: { email: email2, access_level: Member::DEVELOPER } end - create(:email, email: 'email4@example.com', user: create(:user)) - create(:email, email: 'email6@example.com', user: create(:user)) + create(:email, :confirmed, email: 'email4@example.com', user: create(:user)) + create(:email, :confirmed, email: 'email6@example.com', user: create(:user)) + create(:email, :confirmed, email: 'email8@example.com', user: create(:user)) - emails = 'email3@example.com,email4@example.com,email5@example.com,email6@example.com,email7@example.com' + emails = 'email3@example.com,email4@example.com,email5@example.com,email6@example.com,email7@example.com,' \ + 'EMAIL8@EXamPle.com' - unresolved_n_plus_ones = 59 # currently there are 10 queries added per email + unresolved_n_plus_ones = 73 # currently there are 10 queries added per email expect do post invitations_url(project, maintainer), params: { email: emails, access_level: Member::DEVELOPER } diff --git a/spec/services/members/invite_member_builder_spec.rb b/spec/services/members/invite_member_builder_spec.rb index 62c33b42fa27c7f8fef196022928580e1a7a8a21..82c79e0ba7c7e358022ac7e27fcc0ae9051fa880 100644 --- a/spec/services/members/invite_member_builder_spec.rb +++ b/spec/services/members/invite_member_builder_spec.rb @@ -41,4 +41,36 @@ expect(member.invite_email).to eq email end end + + context 'with email downcase' do + let_it_be(:email) { 'TEST@eXAMPle.com' } + let(:invitee) { email } + + subject(:resulting_member) { described_class.new(source, invitee, {}).execute } + + it 'builds a new member and downcases the input' do + expect(resulting_member).to be_new_record + expect(resulting_member.invite_email).to eq 'test@example.com' + end + + context 'with existing member' do + before_all do + create(:group_member, :invited, invite_email: email, source: source) + end + + it 'finds the member with non downcased value' do + expect(resulting_member).not_to be_new_record + expect(resulting_member.invite_email).to eq email + end + + context 'with downcased invite email input' do + let(:invitee) { email.downcase } + + it 'does not find the existing member that has different casing' do + expect(resulting_member).to be_new_record + expect(resulting_member.invite_email).to eq invitee + end + end + end + end end diff --git a/spec/services/members/invite_service_spec.rb b/spec/services/members/invite_service_spec.rb index bf81388357fe123b318542a31b568b356a773eb9..d6c8cac251fdac4c966b324d70441e5252de12af 100644 --- a/spec/services/members/invite_service_spec.rb +++ b/spec/services/members/invite_service_spec.rb @@ -6,7 +6,7 @@ feature_category: :groups_and_projects do let_it_be(:project, reload: true) { create(:project) } let_it_be(:user) { project.first_owner } - let_it_be(:project_user) { create(:user) } + let_it_be(:project_user, reload: true) { create(:user) } let_it_be(:user_invited_by_id) { create(:user) } let_it_be(:namespace) { project.namespace } @@ -257,6 +257,50 @@ end end + context 'with case insensitive emails' do + let(:params) { { email: %w[email@example.org EMAIL@EXAMPLE.ORG] } } + + it 'only creates one member and returns the error object correctly formatted' do + expect_to_create_members(count: 1) + expect(result[:status]).to eq(:error) + expect(result[:message][params[:email].last]).to eq("The member's email address has already been taken") + end + + context 'when the invite already exists for the last email' do + let(:params) { { email: %w[EMAIL@EXAMPLE.ORG], access_level: -1 } } + + before do + create(:project_member, :invited, source: project, invite_email: 'EMAIL@EXAMPLE.ORG') + end + + it 'returns an error object correctly formatted' do + expect(result[:message]['EMAIL@EXAMPLE.ORG']).to eq("Access level is not included in the list") + end + end + + context 'with invite email sent in as upper case of an existing user email' do + let(:params) { { email: project_user.email.upcase } } + + before do + create(:project_member, source: project, user: project_user) + end + + it 'does not create a new member' do + expect_to_create_members(count: 0) + expect(result[:status]).to eq(:success) + end + end + + context 'with invite email sent in as upper case of an existing member user email' do + let(:params) { { email: user.email.upcase } } + + it 'does not create a new member' do + expect_not_to_create_members + expect(result[:message][user.email.upcase]).to eq("not authorized to update member") + end + end + end + context 'when observing invite limits' do context 'with emails and in general' do let_it_be(:emails) { Array(1..101).map { |n| "email#{n}@example.com" } } @@ -335,6 +379,24 @@ expect(project.users).to include project_user end end + + context 'with case sensitive private_commit_email' do + let(:email) do + "#{user.id}-bobby_tables@#{Gitlab::CurrentSettings.current_application_settings.commit_email_hostname}" + end + + let(:params) { { email: email.upcase } } + + it 'does not find the existing user and creates a new member as an invite' do + # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/461885 will change this logic so that the existing + # user is found once we are case insensitive for private_commit_email + expect(project.users).to include user + + expect_to_create_members(count: 1) + expect(result[:status]).to eq(:success) + expect(project.members.last).to be_invite + end + end end context 'when access level is not valid' do @@ -345,6 +407,42 @@ expect_not_to_create_members expect(result[:message][project_user.email]).to eq("Access level is not included in the list") end + + context 'with upper case private commit email due to username casing' do + let(:email) do + hostname = Gitlab::CurrentSettings.current_application_settings.commit_email_hostname + "#{project_user.id}-bobby_tables@#{hostname}" + end + + let(:params) { { email: email, access_level: -1 } } + + before do + project_user.update!(username: 'BOBBY_TABLES') + end + + it 'returns an error object correctly formatted' do + expect_not_to_create_members + expect(result[:message][email]).to eq("Access level is not included in the list") + end + + context 'with invite email sent in as upper case' do + let(:params) { { email: email.upcase, access_level: -1 } } + + it 'returns an error object correctly formatted' do + expect_not_to_create_members + expect(result[:message][email.upcase]).to eq("Access level is not included in the list") + end + end + end + + context 'with invite email sent in as upper case' do + let(:params) { { email: project_user.email.upcase, access_level: -1 } } + + it 'returns an error' do + expect_not_to_create_members + expect(result[:message][params[:email]]).to eq("Access level is not included in the list") + end + end end context 'with user_id' do diff --git a/spec/support/shared_examples/models/member_shared_examples.rb b/spec/support/shared_examples/models/member_shared_examples.rb index 757ee5950e80dec68f7327894e8d8614bc3f0108..4b5d991e5cac4288d544041ba454a0db80e59f8b 100644 --- a/spec/support/shared_examples/models/member_shared_examples.rb +++ b/spec/support/shared_examples/models/member_shared_examples.rb @@ -386,7 +386,7 @@ RSpec.shared_examples_for "bulk member creation" do let_it_be(:admin) { create(:admin, :without_default_org) } - let_it_be(:user1) { create(:user) } + let_it_be(:user1) { create(:user, email: 'bob@example.com') } let_it_be(:user2) { create(:user) } context 'when current user does not have permission' do @@ -480,6 +480,45 @@ expect(members).to all(be_persisted) end + context 'with uppercased email with user in same invite' do + it 'only creates the one member' do + members = described_class.add_members(source, [user1, user1.email.upcase], :maintainer) + + expect(members.map(&:user)).to contain_exactly(user1) + expect(members).to all(be_a(member_type)) + expect(members).to all(be_persisted) + end + end + + context 'with same email with different cases' do + let(:email) { 'bobby@example.com' } + + context 'when the lowercased email is invited first' do + it 'invites the first and errors on the uppercase one' do + expect do + members = described_class.add_members(source, [email, email.upcase], :maintainer) + + expect(source.members.invite.pluck(:invite_email)).to include(email) + expect(members.size).to eq(2) + expect(members.first).to be_persisted + expect(members.last).to be_invalid + end.to change { Member.count }.by(1) + end + end + + context 'when the lowercased email is invited last' do + it 'invites the first and finds updates that record for the second one' do + expect do + members = described_class.add_members(source, [email.upcase, email], :maintainer) + + expect(source.members.invite.pluck(:invite_email)).to include(email) + expect(members.size).to eq(2) + expect(members).to all(be_persisted) + end.to change { Member.count }.by(1) + end + end + end + context 'when a member already exists' do before do source.add_member(user1, :developer)