Emails without top level domain can't be submitted during registration

• Please check this box if this contribution uses AI-generated content (including content generated by GitLab Duo features) as outlined in the GitLab DCO & CLA. As a benefit of being a GitLab Community Contributor, you can request access to GitLab Duo.

What does this MR do and why?

Displays Please provide a valid email address when the email address doesn't have a valid top-level-domain (TLD):

Invalid email	Before	After	Reason
a@b	This email address does not look right, are you sure you typed it correctly?	Please provide a valid email address	no TLD
a@b.c	No error	Please provide a valid email address	TLD less than two characters

No changes in the rest of other invalid emails:

Invalid email	Same behaviour before and after	Reason
"A"@b.co	Please provide a valid email address	quoted emails
ab.co	Please provide a valid email address	no @ symbol
a@b@c.co	Please provide a valid email address	several @ symbol
a@-b.co	Please provide a valid email address	domain starting with hyphen
a@b-.co	Please provide a valid email address	domain finishing with hyphen
a@example_me.co	Please provide a valid email address	domain with underscore
a@[123.123.123.123]	Please provide a valid email address	IP addresses

Closes #499244 (closed)

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before	After

Screenshots or screen recordings

1. Go to either:

• /users/sign_up or
• /-/trial_registrations/new (SAAS enabled)

1. Check the email field in the registration

Related to #499244 (closed)

changed the description

added devopsgovern groupauthentication sectionsec typebug labels

added pipelinetier-1 label

Thanks for your contribution to GitLab @divyamtayal!

• If you need help, page a coach by clicking here or come say hi on Discord.
• When you're ready, request a review by clicking here.
• We welcome AI-generated contributions and offer complimentary access to GitLab Duo! Check out the top of the merge request description to learn more about using AI while contributing.
• To add labels to your merge request, comment @gitlab-bot label ~"label1" ~"label2".

This message was generated automatically. You're welcome to improve it.

added Community contribution workflowin dev labels

assigned to @divyamtayal

changed the description

Hi @eduardosanz, can you pls review the changes.

added linked-issue label

	2 Warnings
	180f2ae6: The commit subject must not end with a period. For more information, take a look at our Commit message guidelines.
	91746f7c: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.

	1 Message
	CHANGELOG missing: If this merge request needs a changelog entry, add the Changelog trailer to the commit message you want to add to the changelog. If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

Reviewer roulette

Category	Reviewer	Maintainer
backend	@alexbuijs (UTC+1)	@sashi_kumar (UTC+0)
frontend	@nradina (UTC+1)	@vitallium (UTC+1)
test for spec/features/*	@hmuralidhar (UTC+11)	Maintainer review is optional for test for spec/features/*
UX	@pedroms (UTC+0)	Maintainer review is optional for UX
groupauthentication	Reviewer review is optional for groupauthentication	@adil.farrukh

Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

requested review from @eduardosanz

@divyamtayal, thanks for working on this!

One important question: do you think we should prevent the submission of the form if the email is not valid?

I left a couple of questions.

requested changes

@divyamtayal, we should add a spec with allowed and not allowed email, probably a feature test. Feature tests are slow but given the integration with JS it may be the only possibility.

changed the description

@eduardosanz I have done the changes as required. Left with writing feature test case.

added 408 commits

• cdeb958e...a80c5860 - 406 commits from branch gitlab-org:master
• 12182fa5 - Modified the email validator flow
• 588cc39e - Merge branch 'master' of https://gitlab.com/gitlab-community/gitlab into 499244-email-val

Compare with previous version

added 1 commit

• 3699caec - Updating Email Format

Compare with previous version

resolved all threads

@eduardosanz can you guide me a bit about writing test cases for it

@divyamtayal, I would like to provide you with a patch:

Click to expand

diff --git a/app/assets/javascripts/pages/sessions/new/email_format_validator.js b/app/assets/javascripts/pages/sessions/new/email_format_validator.js
index 0e13ac1a9248..0f60ca8b2d0d 100644
--- a/app/assets/javascripts/pages/sessions/new/email_format_validator.js
+++ b/app/assets/javascripts/pages/sessions/new/email_format_validator.js
@@ -1,8 +1,5 @@
 import InputValidator from '~/validators/input_validator';
 
-// It checks if email contains at least one character, number or whatever except
-// another "@" or whitespace before "@", at least two characters except
-// another "@" or whitespace after "@" and one dot in between
 const hintMessageSelector = '.validation-hint';
 
 export default class EmailFormatValidator extends InputValidator {
diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb
index b136ab061764..98d84b97dc78 100644
--- a/lib/gitlab/path_regex.rb
+++ b/lib/gitlab/path_regex.rb
@@ -138,7 +138,13 @@ module PathRegex
     NAMESPACE_FORMAT_REGEX = /(?:#{NAMESPACE_FORMAT_REGEX_JS})#{NO_SUFFIX_REGEX}/
     PROJECT_PATH_FORMAT_REGEX = /(?:#{PATH_REGEX_STR})#{NO_SUFFIX_REGEX}/
     FULL_NAMESPACE_FORMAT_REGEX = %r{(#{NAMESPACE_FORMAT_REGEX}/){,#{Namespace::NUMBER_OF_ANCESTORS_ALLOWED}}#{NAMESPACE_FORMAT_REGEX}}
-    EMAIL_FORMAT_JS = /^[a-zA-Z0-9]+([._%+-][a-zA-Z0-9]+)*@([a-zA-Z0-9]+\.)+[a-zA-Z]{2,}/
+
+    # The email pattern should be compilable to a Regex with the option v. Hence the - character must be quoted.
+    LOCAL_PART_1 = '[a-zA-Z0-9]+' # First character must be a-z, A-Z or a number. We don't allow quoted emails.
+    LOCAL_PART_2 = '([._%+\-]\w+)*' # The characters ._%+- cannot be consecutive, they must be followed by an alphanumeric character.
+    DOMAIN = '([a-zA-Z0-9]+([a-zA-Z0-9\-][a-zA-Z0-9]+)*\.)+' # Domain or subdomains must not start or finish with hyphen
+    TLD = '[a-zA-Z]{2,}' # TLD
+    EMAIL_FORMAT_JS = LOCAL_PART_1 + LOCAL_PART_2 + '@' + DOMAIN + TLD
 
     def organization_route_regex
       @organization_route_regex ||= begin
diff --git a/spec/features/users/signup_spec.rb b/spec/features/users/signup_spec.rb
index a363c1b50ac2..f96358a1f227 100644
--- a/spec/features/users/signup_spec.rb
+++ b/spec/features/users/signup_spec.rb
@@ -42,6 +42,7 @@
 
 RSpec.describe 'Signup', :with_current_organization, :js, feature_category: :user_management do
   include TermsHelper
+  using RSpec::Parameterized::TableSyntax
 
   let(:new_user) { build_stubbed(:user) }
 
@@ -365,5 +366,60 @@
     it_behaves_like 'user email validation' do
       let(:path) { new_user_registration_path }
     end
+
+    where(:email, :reason) do
+      '_@b.co'              | 'hyphen as first character in the local-part'
+      '"A"@b.co'            | 'quoted emails'
+      'a..a@b.co'           | 'consecutive dots in the local-part'
+      'a.@b.co'             | 'local-part ending in dot'
+      'a/a@b.co'            | 'local-part with slash' # this is actually allowed
+      'a!a@b.co'            | 'local-part with exclamation mark' # this is actually allowed
+      'a-@b.co'             | 'local-part ending in hyphen' # this is actually allowed
+      'ab.co'               | 'no @ symbol'
+      'a@b@c.co'            | 'several @ symbol'
+      'a@-b.co'             | 'domain starting with hyphen'
+      'a@b-.co'             | 'domain finishing with hypen'
+      'a@example_me.co'     | 'domain with underscore'
+      'a@[123.123.123.123]' | 'IP addresses'
+      'a@b'                 | 'no TLD'
+      'a@b.c'               | 'TLD less than two characters'
+    end
+
+    with_them do
+      cause = params[:reason]
+      it "doesn't accept emails with #{cause}" do
+        new_user.email = email
+        visit new_user_registration_path
+
+        fill_in_sign_up_form(new_user)
+
+        expect(page).to have_current_path new_user_registration_path
+        expect(page).to have_content(_("Please provide a valid email address."))
+      end
+    end
+  end
+
+  context 'with valid email' do
+    where(:email, :reason) do
+      '6@b.co'                              | 'alphanumerical first character in the local-part'
+      '012345678901234567890123456789@b.co' | 'long local-part'
+      'a.a.a@b.co'                          | 'non consecutive dots in the local-part'
+      'a_a_a@b.co'                          | 'non consecutive _ in the local-part'
+      'a%a%a@b.co'                          | 'non consecutive % in the local-part'
+      'a+a+a@b.co'                          | 'non consecutive + in the local-part'
+      'a-a-a@b.co'                          | 'non consecutive - in the local-part'
+      'a@wwww.interna-site.co.uk'           | 'several subdomains'
+      'a@b.example'                         | 'valid TLD'
+    end
+
+    with_them do
+      cause = params[:reason]
+      it "accepts emails with #{cause}" do
+        new_user.email = email
+        visit new_user_registration_path
+
+        expect { fill_in_sign_up_form(new_user) }.to change { User.count }.by(1)
+      end
+    end
   end
 end
diff --git a/spec/support/shared_examples/features/trial_email_validation_shared_example.rb b/spec/support/shared_examples/features/trial_email_validation_shared_example.rb
index 81c9ac1164b2..734867fc43cc 100644
--- a/spec/support/shared_examples/features/trial_email_validation_shared_example.rb
+++ b/spec/support/shared_examples/features/trial_email_validation_shared_example.rb
@@ -4,35 +4,27 @@
   let(:email_hint_message) { _('We recommend a work email address.') }
   let(:email_error_message) { _('Please provide a valid email address.') }
 
-  let(:email_warning_message) do
-    _('This email address does not look right, are you sure you typed it correctly?')
-  end
-
   it 'shows an error message until a correct email is entered' do
     visit path
     expect(page).to have_content(email_hint_message)
     expect(page).not_to have_content(email_error_message)
-    expect(page).not_to have_content(email_warning_message)
 
     fill_in 'new_user_email', with: 'foo@'
     fill_in 'new_user_first_name', with: ''
 
     expect(page).not_to have_content(email_hint_message)
     expect(page).to have_content(email_error_message)
-    expect(page).not_to have_content(email_warning_message)
 
     fill_in 'new_user_email', with: 'foo@bar'
     fill_in 'new_user_first_name', with: ''
 
     expect(page).not_to have_content(email_hint_message)
-    expect(page).not_to have_content(email_error_message)
-    expect(page).to have_content(email_warning_message)
+    expect(page).to have_content(email_error_message)
 
     fill_in 'new_user_email', with: 'foo@gitlab.com'
     fill_in 'new_user_first_name', with: ''
 
     expect(page).not_to have_content(email_hint_message)
     expect(page).not_to have_content(email_error_message)
-    expect(page).not_to have_content(email_warning_message)
   end
 end

I am marking as draft. When you are ready turn it to ready and ping @fernando-c.

@fernando-c, do you think we will need a feature flag to derisk this. I am a little nervous blocking registrations.

requested changes

marked this merge request as draft

mentioned in issue #499244 (closed)

assigned to @eduardosanz

changed milestone to %17.6

added Deliverable label

removed review request for @eduardosanz

added 2538 commits

• 3699caec...965b3a0c - 2534 commits from branch gitlab-org:master
• 9ff93be5 - Invalid email should throw an error during registration
• bf68a77f - Modified the email validator flow
• d58d3799 - Updating Email Format
• 91746f7c - Correct implementation and add tests

Compare with previous version

marked this merge request as ready

added backend frontend labels

	2 Warnings
	a97dad22: The commit subject must not end with a period. For more information, take a look at our Commit message guidelines.
	6de3b24a: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.

	1 Message
	CHANGELOG missing: If this merge request needs a changelog entry, add the Changelog trailer to the commit message you want to add to the changelog. If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

Reviewer roulette

Category	Reviewer	Maintainer
backend	@freinink (UTC-7)	@schin1 (UTC+8)
frontend	@lwanko (UTC+1)	@iamphill (UTC+0)
test for spec/features/*	@freinink (UTC-7)	Maintainer review is optional for test for spec/features/*
UX	@pedroms (UTC+0)	Maintainer review is optional for UX
groupauthentication	Reviewer review is optional for groupauthentication	@sgarg_gitlab (UTC+5.5)

Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

requested review from @fernando-c

Bundle size analysis [beta]

This compares changes in bundle size for entry points between the commits 6f5c75fd and 9e2e542a

Special assets

Entrypoint / Name	Size before	Size after	Diff	Diff in percent
average	4.41 MB	4.41 MB	-	-0.0 %
mainChunk	3.32 MB	3.32 MB	-	0.0 %

---

Note: We do not have exact data for 6f5c75fd. So we have used data from: 3b2cfe31.
The intended commit has no webpack pipeline, so we chose the last commit with one before it.

Please look at the full report for more details

---

Read more about how this report works.

Generated by Danger

changed the description

added 1 commit

• 2deaf54f - Fix broken test

Compare with previous version

added UX label

Thanks for helping us improve the UX of GitLab. Your contribution is appreciated! We have pinged our UX team, so stay tuned for their feedback.

This message was generated automatically. Improve it or delete it.

added Leading Organization label

added 1 commit

• 180f2ae6 - Changing LOCAL_PART to more lenient.

Compare with previous version

approved this merge request

added pipeline:mr-approved label

added pipelinetier-2 label and removed pipelinetier-1 label

Before you set this MR to auto-merge

This merge request will progress on pipeline tiers until it reaches the last tier: pipelinetier-3.

Before you set this MR to auto-merge, please check the following:

• You are the last maintainer of this merge request
• The latest pipeline for this merge request is pipelinetier-3 (You can find which tier it is in the pipeline name)
• This pipeline is recent enough (created in the last 8 hours)

If all the criteria above apply, please set auto-merge for this merge request.

See pipeline tiers and merging a merge request for more details.

added 420 commits

• 180f2ae6...5513165a - 414 commits from branch gitlab-org:master
• a04b0581 - Invalid email should throw an error during registration
• 24720182 - Modified the email validator flow
• 137ba6c3 - Updating Email Format
• 63969e84 - Correct implementation and add tests
• 20a0ad1a - Fix broken test
• 55ba7365 - Changing LOCAL_PART to more lenient.

Compare with previous version

added 1 commit

• cc7a9e7c - Changing LOCAL_PART to more lenient.

Compare with previous version

resolved all threads

@fernando-c, with the latest commit that @divyamtayal and I have made, we are 100% confident that we are not blocking any valid email, but we are preventing submitting emails that don't have a top level domain (TLD) in the address. That's what it was requested in the issue.

We are maintaining the same behaviour as the default <input type="email" /> in the browser we support (tested in Chrome, Safari and Firefox), just enforcing the presence of the top level domain.

This change allow us to get even more strict, if we want, in the future.

This is ready for review.

Generated by gitlab_quality-test_tooling.

---

Slow tests detected in this merge request. These slow tests might be related to this merge request's changes.

Click to expand

Job	File	Name	Duration	Expected duration
#8273195828	spec/features/admin/users/users_spec.rb#L177	Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking	66.9 s	< 50.13 s
#8273195547	spec/lib/release_highlights/validator_spec.rb#L82	ReleaseHighlights::Validator when validating all files they should have no errors	88.1 s	< 27.12 s
#8298976542	spec/features/admin/users/users_spec.rb#L177	Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking	68.29 s	< 50.13 s
#8298976200	spec/lib/release_highlights/validator_spec.rb#L82	ReleaseHighlights::Validator when validating all files they should have no errors	90.38 s	< 27.12 s
#8310009492	spec/features/admin/users/users_spec.rb#L177	Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking	67.21 s	< 50.13 s
#8310009236	spec/lib/release_highlights/validator_spec.rb#L82	ReleaseHighlights::Validator when validating all files they should have no errors	85.56 s	< 27.12 s
#8352232032	spec/features/admin/users/users_spec.rb#L177	Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking	66.46 s	< 50.13 s
#8352229300	spec/lib/release_highlights/validator_spec.rb#L82	ReleaseHighlights::Validator when validating all files they should have no errors	80.13 s	< 27.12 s

added rspec:slow test detected label

E2E Test Result Summary

allure-report-publisher generated test report!

e2e-test-on-gdk: test report for 9e2e542a

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Verify      | 40     | 0      | 1       | 0     | 41    | ✅     |
| Create      | 111    | 0      | 2       | 0     | 113   | ✅     |
| Govern      | 49     | 0      | 2       | 0     | 51    | ✅     |
| Plan        | 63     | 0      | 0       | 0     | 63    | ✅     |
| Data Stores | 25     | 0      | 0       | 0     | 25    | ✅     |
| Monitor     | 8      | 0      | 0       | 0     | 8     | ✅     |
| Package     | 24     | 0      | 0       | 0     | 24    | ✅     |
| Secure      | 4      | 0      | 0       | 0     | 4     | ✅     |
| Manage      | 1      | 0      | 0       | 0     | 1     | ✅     |
| Release     | 4      | 0      | 0       | 0     | 4     | ✅     |
| Analytics   | 1      | 0      | 0       | 0     | 1     | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 330    | 0      | 5       | 0     | 335   | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+

@eduardosanz @divyamtayal I think we are pretty close! Good work on the MR.

I left some feedback. Here is a summary:

1. I think we can do additional code cleanup
2. What are the verification steps to verify trial registration flow? It also uses the same rails partial. I was unable to confirm since I don't know how to get to the trial sign-up

added 677 commits

• cc7a9e7c...7ce4cb50 - 670 commits from branch gitlab-org:master
• 0520e77d - Invalid email should throw an error during registration
• da50942b - Modified the email validator flow
• e078cba8 - Updating Email Format
• 1acb800a - Correct implementation and add tests
• b032014e - Fix broken test
• ec0086f4 - Changing LOCAL_PART to more lenient.
• fd24f7f0 - Remove unneeded code

Compare with previous version

changed the description

resolved all threads

approved this merge request

resolved all threads

enabled automatic add to merge train when checks pass

requested review from @sgarg_gitlab

resolved all threads

@eduardosanz I do not think we are following this instruction here -

We want to strongly highlight that the email is potentially invalid but we shouldn't block the account from being created.

We can rely on the safeguard in the next step where an unconfirmed account will be deleted in 3 days as having the final say on whether an email is invalid or not

Since currently I am not able to submit the form in case email is not of correct format

While with our earlier version form could be submitted even though the email is not of correct format but user is shown a warning.

@sgarg_gitlab, we didn't go for this recommendation:

We want to strongly highlight that the email is potentially invalid but we shouldn't block the account from being created.

We concluded that it is better to block form submission if the email doesn't contain a top level domain, like .com. If the domain is mistyped, .dom or .co, we will rely on the safeguard mechanism of unverified emails, that you mention.

We believe this is less friction for the users. What do you think?

Thanks for the clarification @eduardosanz but can you please point me to where we have a go ahead from the product for this change? Since by the looks of it I am not sure if thats what we agreed upon. Even though I do not see its documented anywhere what is the expected behaviour as such.

Current change looks more logical to me though

I understand. I am asking for approval in #499244 (comment 2207527544).

@sgarg_gitlab and @fernando-c, we are going to close this MR in favour of Display bolder warning if email TLD is missing (!172716 - merged).

I would add you as reviewers for that one.

changed title from Invalid email should throw an error during registration to Emails without domain can't be submitted during registration

changed title from Emails without domain can't be submitted during registration to Emails without top level domain can't be submitted during registration

added 740 commits

• fd24f7f0...b9a2017b - 732 commits from branch gitlab-org:master
• 7af262cd - Invalid email should throw an error during registration
• 0ee8230a - Modified the email validator flow
• 56905fdc - Updating Email Format
• 6de3b24a - Correct implementation and add tests
• 0bcc1a9c - Fix broken test
• a97dad22 - Changing LOCAL_PART to more lenient.
• e7ff97d0 - Remove unneeded code
• 9e2e542a - Corrected some mistakes

Compare with previous version

enabled automatic add to merge train when checks pass

cancelled automatic add to merge train

enabled automatic add to merge train when checks pass

changed the description

removed review request for @sgarg_gitlab and @fernando-c

closed

aborted automatic add to merge train because merge request was closed

@divyamtayal, how was your code review experience with this merge request? Please tell us how we can continue to iterate and improve:

1. React with a or a on this comment to describe your experience.
2. Create a new comment starting with @gitlab-bot feedback below, and leave any additional feedback you have for us in the comment.

As a benefit of being a GitLab Community Contributor, you can request access to GitLab Duo. With Code Suggestions, Chat and more AI-powered features, GitLab Duo helps to boost your efficiency and effectiveness by reducing the time required to write and understand code. Visit the Duo access project to request a GitLab Duo license and learn more about the benefits of GitLab Duo.

Subscribe to the GitLab Community Newsletter for contributor-focused content and opportunities to level up.

Thanks for your help!

This message was generated automatically. Improve it or delete it.