Merge branch 'ek-update-insufficient-domain-validation' into 'master'

Update domain validation settings

See merge request gitlab-org/gitlab!68437

ee/app/assets/javascripts/groups/settings/access_restriction_field/index.js

@@ -2,7 +2,7 @@ import Vue from 'vue';
 import { __, sprintf } from '~/locale';
 import CommaSeparatedListTokenSelector from '../components/comma_separated_list_token_selector.vue';
 
-export default (selector, props = {}, qaSelector, customValidator) => {
+export default ({ selector, props = {}, qaSelector, customValidator = null }) => {
   const el = document.querySelector(selector);
 
   if (!el) return;

ee/app/assets/javascripts/pages/groups/edit/index.js

@@ -4,18 +4,21 @@ import validateRestrictedIpAddress from 'ee/groups/settings/access_restriction_f
 import createFlash from '~/flash';
 import { __ } from '~/locale';
 
-initAccessRestrictionField('.js-allowed-email-domains', {
+initAccessRestrictionField({
+  selector: '.js-allowed-email-domains',
+  props: {
     placeholder: __('Enter domain'),
     regexErrorMessage: __('The domain you entered is misformatted.'),
     disallowedValueErrorMessage: __('The domain you entered is not allowed.'),
+  },
 });
 
-initAccessRestrictionField(
-  '.js-ip-restriction',
-  { placeholder: __('Enter IP address range') },
-  'ip_restriction_field',
-  validateRestrictedIpAddress,
-);
+initAccessRestrictionField({
+  selector: '.js-ip-restriction',
+  props: { placeholder: __('Enter IP address range') },
+  qaSelector: 'ip_restriction_field',
+  customValidator: validateRestrictedIpAddress,
+});
 
 const complianceFrameworksList = document.querySelector('#js-compliance-frameworks-list');
 

ee/app/models/allowed_email_domain.rb

@@ -14,14 +14,30 @@ class AllowedEmailDomain < ApplicationRecord
     'icloud.com'
   ].freeze
 
-  VALID_DOMAIN_REGEX = /\w*\./.freeze
+  ##
+  # NOTE: If we need to change this regex, we need to ensure we use the same regex in ruby and JS
+  #
+  # VALID_DOMAIN_BASE defines the core of the regex we use for validating email addresses
+  # For ruby code we should use the `VALID_DOMAIN_REGEX` regex. The JS_VALID_DOMAIN_REGEX
+  # should only be passed to the frontend for validation in javascript. This is because of
+  # the differences in how ruby interprets `\A` and `\z`
+  #
+  # More information: https://gitlab.com/gitlab-org/gitlab/-/issues/321510
+  #
+  VALID_DOMAIN_BASE = '(?=.*\.)[0-9a-zA-Z.-]+'
+
+  # VALID_DOMAIN_REGEX is the regex we should be using to validate email domains in ruby
+  VALID_DOMAIN_REGEX = /\A#{VALID_DOMAIN_BASE}\z/.freeze
+
+  # JS_VALID_DOMAIN_REGEX is only for use on the frontend in javascript/vue
+  JS_VALID_DOMAIN_REGEX = /^#{VALID_DOMAIN_BASE}$/.freeze
 
   validates :group_id, presence: true
   validates :domain, presence: true
   validate :allow_root_group_only
   validates :domain, exclusion: { in: RESERVED_DOMAINS,
     message: _('The domain you entered is not allowed.') }
-  validates :domain, format: { with: VALID_DOMAIN_REGEX,
+  validates :domain, if: :domain_changed?, format: { with: VALID_DOMAIN_REGEX,
     message: _('The domain you entered is misformatted.') }
 
   belongs_to :group, class_name: 'Group', foreign_key: :group_id

ee/app/views/groups/settings/_allowed_email_domain.html.haml

@@ -7,7 +7,7 @@
     = _('Restrict membership by email domain')
   .js-allowed-email-domains{ data: { hidden_input_id: hidden_input_id,
     label_id: label_id,
-    regex_validator: AllowedEmailDomain::VALID_DOMAIN_REGEX.source,
+    regex_validator: AllowedEmailDomain::JS_VALID_DOMAIN_REGEX.source,
     disallowed_values: AllowedEmailDomain::RESERVED_DOMAINS.to_json } }
   = f.hidden_field :allowed_email_domains_list, id: hidden_input_id
   .form-text.text-muted

ee/spec/features/groups/group_settings_spec.rb

@@ -366,6 +366,48 @@ RSpec.describe 'Edit group settings' do
     end
   end
 
+  describe 'email domain validation', :js do
+    let(:domain_field_selector) { '[placeholder="Enter domain"]' }
+
+    before do
+      stub_licensed_features(group_allowed_email_domains: true)
+    end
+
+    def update_email_domains(new_domain)
+      visit edit_group_path(group)
+
+      find(domain_field_selector).set(new_domain)
+      find(domain_field_selector).send_keys(:enter)
+    end
+
+    it 'is visible' do
+      visit edit_group_path(group)
+
+      expect(page).to have_content("Restrict membership by email domain")
+    end
+
+    it 'displays an error for invalid emails' do
+      new_invalid_domain = "gitlab./com!"
+
+      update_email_domains(new_invalid_domain)
+
+      expect(page).to have_content("The domain you entered is misformatted")
+    end
+
+    it 'will save valid domains' do
+      new_domain = "gitlab.com"
+
+      update_email_domains(new_domain)
+
+      expect(page).not_to have_content("The domain you entered is misformatted")
+
+      click_button 'Save changes'
+      wait_for_requests
+
+      expect(page).to have_content("Group 'Foo bar' was successfully updated.")
+    end
+  end
+
   def save_permissions_group
     page.within('.gs-permissions') do
       click_button 'Save changes'

ee/spec/models/allowed_email_domain_spec.rb

@@ -30,20 +30,30 @@ RSpec.describe AllowedEmailDomain do
     describe '#valid domain' do
       subject { described_class.new(group: create(:group), domain: domain) }
 
+      let_it_be(:group) { create(:group) }
+
       context 'valid domain' do
-        let(:domain) { 'gitlab.com' }
+        let(:valid_domains) { ['gitlab.com', 'gitlab.co.uk', 'GITLAB.COM'] }
 
         it 'succeeds' do
-          expect(subject.valid?).to be_truthy
+          valid_domains.each do |domain|
+            overridden_subject = described_class.new(group: group, domain: domain)
+            expect(overridden_subject.valid?).to be_truthy
+          end
         end
       end
 
       context 'invalid domain' do
-        let(:domain) { 'gitlab' }
+        let(:invalid_domains) do
+          ['gitlab', 'git?lab.com', 'gitlab.com!', 'gitlab.@com', 'gitla>b.com', 'gitl<a>b@.c?om!']
+        end
 
         it 'fails' do
-          expect(subject.valid?).to be_falsey
-          expect(subject.errors[:domain]).to include('The domain you entered is misformatted.')
+          invalid_domains.each do |domain|
+            overridden_subject = described_class.new(group: group, domain: domain)
+            expect(overridden_subject.valid?).to be_falsey
+            expect(overridden_subject.errors[:domain]).to include('The domain you entered is misformatted.')
+          end
         end
       end