Merge branch '499244-email-val' into 'master'

Invalid email should throw an error during registration

See merge request !169617



Merged-by: Divyam Tayal <divyamtayal18@gmail.com>
Reviewed-by: Eduardo Sanz García <esanz-garcia@gitlab.com>
Co-authored-by: Eduardo Sanz García <esanz-garcia@gitlab.com>

app/assets/javascripts/pages/sessions/new/email_format_validator.js

 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 emailRegexPattern = /[^@\s]+@[^@\s]+\.[^@\s]+/;
 const hintMessageSelector = '.validation-hint';
-const warningMessageSelector = '.validation-warning';
 
 export default class EmailFormatValidator extends InputValidator {
   constructor(opts = {}) {
@@ -24,19 +19,6 @@ export default class EmailFormatValidator extends InputValidator {
     const inputDomElement = event.target;
 
     EmailFormatValidator.setMessageVisibility(inputDomElement, hintMessageSelector);
-    EmailFormatValidator.setMessageVisibility(inputDomElement, warningMessageSelector);
-    EmailFormatValidator.validateEmailInput(inputDomElement);
-  }
-
-  static validateEmailInput(inputDomElement) {
-    const validEmail = inputDomElement.checkValidity();
-    const validPattern = inputDomElement.value.match(emailRegexPattern);
-
-    EmailFormatValidator.setMessageVisibility(
-      inputDomElement,
-      warningMessageSelector,
-      validEmail && !validPattern,
-    );
   }
 
   static setMessageVisibility(inputDomElement, messageSelector, isVisible = false) {

app/views/devise/registrations/_signup_box_form.html.haml

@@ -60,12 +60,11 @@
       = f.email_field :email,
         class: 'form-control gl-form-input middle js-validate-email js-track-error',
         data: { testid: 'new-user-email-field', track_action_for_errors: preregistration_tracking_label },
+        pattern: Gitlab::PathRegex::EMAIL_FORMAT_JS,
         required: true,
         title: _('Please provide a valid email address.')
       %p.validation-hint.gl-field-hint.text-secondary
         = _('We recommend a work email address.')
-      %p.validation-warning.gl-field-error-ignore.text-secondary.hide
-        = _('This email address does not look right, are you sure you typed it correctly?')
       -# This is used for providing entry to Jihu on email verification
       = render_if_exists 'devise/shared/signup_email_additional_info'
   .form-group.gl-mb-5

lib/gitlab/path_regex.rb

@@ -139,6 +139,13 @@ module PathRegex
     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}}
 
+    # 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
         illegal_words = Regexp.new(Regexp.union(ILLEGAL_ORGANIZATION_PATH_WORDS).source, Regexp::IGNORECASE)

locale/gitlab.pot

@@ -56094,9 +56094,6 @@ msgstr ""
 msgid "This domain is not verified. You will need to verify ownership before access is enabled."
 msgstr ""
-msgid "This email address does not look right, are you sure you typed it correctly?"
-msgstr ""
-
 msgid "This email supersedes any previous emails about scheduled deletion you may have received for %{project_link}."
 msgstr ""

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,62 @@
     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.'                | '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.internal-site.co.uk'          | 'several subdomains'
+      'a@3w.internal-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

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: ''
+    click_button _('Continue')
 
     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: ''
+    click_button _('Continue')
 
     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: ''
+    click_button _('Continue')
 
     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