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

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

See merge request !169617



Merged-by: Eduardo Sanz García <esanz-garcia@gitlab.com>
Approved-by: Sascha Eggenberger <seggenberger@gitlab.com>
Approved-by: Fernando Cardenas <14022659-fernando-c@users.noreply.gitlab.com>
Reviewed-by: Eduardo Sanz García <esanz-garcia@gitlab.com>
Co-authored-by: divyamtayal <divyamtayal18@gmail.com>

app/assets/javascripts/pages/registrations/new/index.js

 import NoEmojiValidator from '~/emoji/no_emoji_validator';
 import LengthValidator from '~/validators/length_validator';
 import UsernameValidator from '~/pages/sessions/new/username_validator';
-import EmailFormatValidator from '~/pages/sessions/new/email_format_validator';
 import { initLanguageSwitcher } from '~/language_switcher';
 import { initPasswordInput } from '~/authentication/password';
 import Tracking from '~/tracking';
@@ -10,7 +9,6 @@ import { renderGFM } from '~/behaviors/markdown/render_gfm';
 new UsernameValidator(); // eslint-disable-line no-new
 new LengthValidator(); // eslint-disable-line no-new
 new NoEmojiValidator(); // eslint-disable-line no-new
-new EmailFormatValidator(); // eslint-disable-line no-new
 
 Tracking.enableFormTracking({
   forms: { allow: ['new_user'] },

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 = {}) {
-    super();
-
-    const container = opts.container || '';
-
-    document
-      .querySelectorAll(`${container} .js-validate-email`)
-      .forEach((element) =>
-        element.addEventListener('keyup', EmailFormatValidator.eventHandler.bind(this)),
-      );
-  }
-
-  static eventHandler(event) {
-    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) {
-    const messageElement = inputDomElement.parentElement.querySelector(messageSelector);
-    messageElement.classList.toggle('hide', !isVisible);
-  }
-}

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.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

ee/app/assets/javascripts/pages/trial_registrations/new/index.js

@@ -5,7 +5,6 @@ import NoEmojiValidator from '~/emoji/no_emoji_validator';
 import LengthValidator from '~/validators/length_validator';
 import SigninTabsMemoizer from '~/pages/sessions/new/signin_tabs_memoizer';
 import UsernameValidator from '~/pages/sessions/new/username_validator';
-import EmailFormatValidator from '~/pages/sessions/new/email_format_validator';
 import Tracking from '~/tracking';
 import { setupArkoseLabsForSignup } from 'ee/arkose_labs';
 import initPasswordValidator from 'ee/password/password_validator';
@@ -18,7 +17,6 @@ new UsernameValidator(); // eslint-disable-line no-new
 new LengthValidator(); // eslint-disable-line no-new
 new SigninTabsMemoizer(); // eslint-disable-line no-new
 new NoEmojiValidator(); // eslint-disable-line no-new
-new EmailFormatValidator(); // eslint-disable-line no-new
 
 trackFreeTrialAccountSubmissions();
 

lib/gitlab/path_regex.rb

@@ -139,6 +139,12 @@ 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 = '[^"@]+' # Any character except @ and double quotes.
+    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 + '@' + 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

@@ -56359,9 +56359,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,51 @@
     it_behaves_like 'user email validation' do
       let(:path) { new_user_registration_path }
     end
+
+    where(:email, :reason) do
+      '"A"@b.co'            | 'quoted emails'
+      'ab.co'               | 'no @ symbol'
+      'a@b@c.co'            | 'several @ symbol'
+      'a@-b.co'             | 'domain starting with hyphen'
+      'a@b-.co'             | 'domain finishing with hyphen'
+      '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@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