From 219b84b234b8d2f2b1748afb4f98f5ed213713f3 Mon Sep 17 00:00:00 2001 From: Doug Stull Date: Thu, 2 May 2024 11:34:47 -0400 Subject: [PATCH 1/2] Downcase invite emails as the service layer - normalize the data for case sensitivity to match user and email model handling. Changelog: changed --- .../concerns/bulk_users_by_email_load.rb | 6 +- app/models/user.rb | 5 + app/services/members/creator_service.rb | 16 +++- app/services/members/invite_member_builder.rb | 10 +- app/services/members/invite_service.rb | 15 ++- spec/requests/api/invitations_spec.rb | 14 +-- .../members/invite_member_builder_spec.rb | 32 +++++++ spec/services/members/invite_service_spec.rb | 95 ++++++++++++++++++- .../models/member_shared_examples.rb | 41 +++++++- 9 files changed, 219 insertions(+), 15 deletions(-) diff --git a/app/models/concerns/bulk_users_by_email_load.rb b/app/models/concerns/bulk_users_by_email_load.rb index 55143ead30a892..b970e896b52cf8 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 it 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 9dd8d16787bff6..0595cbba1314b5 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 57159c14b3b783..a28eed8b966ba4 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 e925121bb1e796..5164ab443abdb4 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 0534c59c255192..4426bb8ae6fd45 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 a2697fa55f590b..e4d757c9f0549f 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 62c33b42fa27c7..82c79e0ba7c7e3 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 bf81388357fe12..124467bd3c29b2 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" } } @@ -345,6 +389,55 @@ 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 + + context 'with and existing member user' do + let(:email) do + "#{user.id}-bobby_tables@#{Gitlab::CurrentSettings.current_application_settings.commit_email_hostname}" + end + + let(:params) { { email: email.upcase } } + + xit 'does not create a new member' do + expect_not_to_create_members + expect(result[:message][email.upcase]).to eq("User already exists in source") + 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 757ee5950e80de..4b5d991e5cac42 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) -- GitLab From 4c33e55e6db76811a177610a9524ecd62cd6e538 Mon Sep 17 00:00:00 2001 From: Doug Stull Date: Tue, 21 May 2024 07:29:27 -0400 Subject: [PATCH 2/2] Correct and clarify private commit email example --- .../concerns/bulk_users_by_email_load.rb | 2 +- spec/requests/api/invitations_spec.rb | 2 +- spec/services/members/invite_service_spec.rb | 31 +++++++++++-------- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/app/models/concerns/bulk_users_by_email_load.rb b/app/models/concerns/bulk_users_by_email_load.rb index b970e896b52cf8..08474b465ad023 100644 --- a/app/models/concerns/bulk_users_by_email_load.rb +++ b/app/models/concerns/bulk_users_by_email_load.rb @@ -9,7 +9,7 @@ def users_by_emails(emails) # 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 it likely very rare. + # 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) diff --git a/spec/requests/api/invitations_spec.rb b/spec/requests/api/invitations_spec.rb index e4d757c9f0549f..dff3205f55d9db 100644 --- a/spec/requests/api/invitations_spec.rb +++ b/spec/requests/api/invitations_spec.rb @@ -433,7 +433,7 @@ def invite_member_by_email(source, source_type, email, created_by, access_level: 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,' \ - 'EMAIL8@EXamPle.com' + 'EMAIL8@EXamPle.com' unresolved_n_plus_ones = 73 # currently there are 10 queries added per email diff --git a/spec/services/members/invite_service_spec.rb b/spec/services/members/invite_service_spec.rb index 124467bd3c29b2..d6c8cac251fdac 100644 --- a/spec/services/members/invite_service_spec.rb +++ b/spec/services/members/invite_service_spec.rb @@ -379,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 @@ -415,19 +433,6 @@ expect(result[:message][email.upcase]).to eq("Access level is not included in the list") end end - - context 'with and existing member user' do - let(:email) do - "#{user.id}-bobby_tables@#{Gitlab::CurrentSettings.current_application_settings.commit_email_hostname}" - end - - let(:params) { { email: email.upcase } } - - xit 'does not create a new member' do - expect_not_to_create_members - expect(result[:message][email.upcase]).to eq("User already exists in source") - end - end end context 'with invite email sent in as upper case' do -- GitLab