diff --git a/lib/gitlab/import/placeholder_user_creator.rb b/lib/gitlab/import/placeholder_user_creator.rb index 011c4d49e1a946da35a450748f1bfddf0be898cf..033c2b713e4adafa07239a43dcddecb8f6d5fdb4 100644 --- a/lib/gitlab/import/placeholder_user_creator.rb +++ b/lib/gitlab/import/placeholder_user_creator.rb @@ -29,8 +29,9 @@ def execute def placeholder_name # Some APIs don't expose users' names, so set a default if it's nil - name = source_name || "#{import_type} Source User" - "Placeholder #{name}" + return "Placeholder #{import_type} Source User" unless source_name + + "Placeholder #{source_name.slice(0, 127)}" end def placeholder_username @@ -42,14 +43,21 @@ def placeholder_username def placeholder_email email_pattern = "#{valid_source_username}_placeholder_user_%s@#{Settings.gitlab.host}" - lambda_for_unique_email = ->(email) { User.find_by_email(email) } + lambda_for_unique_email = ->(email) { User.find_by_email(email) || ::Email.find_by_email(email) } uniquify_string(email_pattern, lambda_for_unique_email) end def valid_source_username - return "#{import_type}_source_username" unless source_username + return fallback_username unless source_username + + sanitized_source_username = source_username.gsub(/[^A-Za-z0-9]/, '') + return fallback_username if sanitized_source_username.empty? + + sanitized_source_username.slice(0, User::MAX_USERNAME_LENGTH - 55) + end - source_username.slice(0, User::MAX_USERNAME_LENGTH - 55) + def fallback_username + "#{import_type}_source_username" end def uniquify_string(base_pattern, lambda_for_uniqueness) diff --git a/spec/lib/gitlab/import/placeholder_user_creator_spec.rb b/spec/lib/gitlab/import/placeholder_user_creator_spec.rb index 62b62c4f66c7e184806f37351c1e3425078e80d9..03d006d8d1d090b20be46a0b2a1a78e1a9f9275a 100644 --- a/spec/lib/gitlab/import/placeholder_user_creator_spec.rb +++ b/spec/lib/gitlab/import/placeholder_user_creator_spec.rb @@ -24,8 +24,8 @@ new_placeholder_user = User.where(user_type: :placeholder).last expect(new_placeholder_user.name).to eq("Placeholder #{source_name}") - expect(new_placeholder_user.username).to match(/^#{source_username}_placeholder_user_\d+$/) - expect(new_placeholder_user.email).to match(/^#{source_username}_placeholder_user_\d+@#{Settings.gitlab.host}$/) + expect(new_placeholder_user.username).to match(/^aprycontributor_placeholder_user_\d+$/) + expect(new_placeholder_user.email).to match(/^aprycontributor_placeholder_user_\d+@#{Settings.gitlab.host}$/) end context 'when there are non-unique usernames on the same import source' do @@ -38,7 +38,18 @@ end end - context 'and the incoming source_user attributes are invalid' do + context 'when generating a unique email address' do + it 'validates against all stored email addresses' do + existing_user = create(:user, email: 'aprycontributor_placeholder_user_1@localhost') + existing_user.emails.create!(email: 'aprycontributor_placeholder_user_2@localhost') + + placeholder_user = service.execute + + expect(placeholder_user.email).to eq('aprycontributor_placeholder_user_3@localhost') + end + end + + context 'when the incoming source_user attributes are invalid' do context 'when source_name is nil' do let(:source_name) { nil } @@ -49,6 +60,17 @@ end end + context 'when source_name is too long' do + let(:source_name) { 'a' * 500 } + + it 'truncates the source name to 127 characters' do + placeholder_user = service.execute + + expect(placeholder_user.first_name).to eq('Placeholder') + expect(placeholder_user.last_name).to eq('a' * 127) + end + end + context 'when source_username is nil' do let(:source_username) { nil } @@ -59,6 +81,26 @@ end end + context 'when the source_username contains invalid characters' do + using RSpec::Parameterized::TableSyntax + + where(:input_username, :expected_output) do + '.asdf' | 'asdf_placeholder_user_1' + 'asdf^ghjk' | 'asdfghjk_placeholder_user_1' + '.' | 'github_source_username_placeholder_user_1' + end + + with_them do + let(:source_username) { input_username } + + it do + placeholder_user = service.execute + + expect(placeholder_user.username).to eq(expected_output) + end + end + end + context 'when source_username is too long' do let(:source_username) { 'a' * 500 } diff --git a/spec/lib/gitlab/import/source_user_mapper_spec.rb b/spec/lib/gitlab/import/source_user_mapper_spec.rb index 6e2f8d65d6f600d5bee36f3665d31f8ef52c2596..92d714e852fb0bcfc7ba39c4e47e77c01a5f9040 100644 --- a/spec/lib/gitlab/import/source_user_mapper_spec.rb +++ b/spec/lib/gitlab/import/source_user_mapper_spec.rb @@ -47,8 +47,8 @@ new_placeholder_user = User.where(user_type: :placeholder).last expect(new_placeholder_user.name).to eq("Placeholder #{source_name}") - expect(new_placeholder_user.username).to match(/^#{source_username}_placeholder_user_\d+$/) - expect(new_placeholder_user.email).to match(/^#{source_username}_placeholder_user_\d+@#{Settings.gitlab.host}$/) + expect(new_placeholder_user.username).to match(/^aprycontributor_placeholder_user_\d+$/) + expect(new_placeholder_user.email).to match(/^aprycontributor_placeholder_user_\d+@#{Settings.gitlab.host}$/) end end