Merge branch 'validate_restricted_signup_email_for_inviting_member' into 'master'

Return API error when inviting email restricted for sign-up

See merge request gitlab-org/gitlab!64807

app/models/concerns/restricted_signup.rb

+# frozen_string_literal: true
+module RestrictedSignup
+  extend ActiveSupport::Concern
+
+  private
+
+  def validate_admin_signup_restrictions(email)
+    return if allowed_domain?(email)
+
+    if allowlist_present?
+      return _('domain is not authorized for sign-up.')
+    elsif denied_domain?(email)
+      return _('is not from an allowed domain.')
+    elsif restricted_email?(email)
+      return _('is not allowed. Try again with a different email address, or contact your GitLab admin.')
+    end
+
+    nil
+  end
+
+  def denied_domain?(email)
+    return false unless Gitlab::CurrentSettings.domain_denylist_enabled?
+
+    denied_domains = Gitlab::CurrentSettings.domain_denylist
+    denied_domains.present? && domain_matches?(denied_domains, email)
+  end
+
+  def allowlist_present?
+    Gitlab::CurrentSettings.domain_allowlist.present?
+  end
+
+  def allowed_domain?(email)
+    allowed_domains = Gitlab::CurrentSettings.domain_allowlist
+    allowlist_present? && domain_matches?(allowed_domains, email)
+  end
+
+  def restricted_email?(email)
+    return false unless Gitlab::CurrentSettings.email_restrictions_enabled?
+
+    restrictions = Gitlab::CurrentSettings.email_restrictions
+    restrictions.present? && Gitlab::UntrustedRegexp.new(restrictions).match?(email)
+  end
+
+  def domain_matches?(email_domains, email)
+    signup_domain = Mail::Address.new(email).domain
+    email_domains.any? do |domain|
+      escaped = Regexp.escape(domain).gsub('\*', '.*?')
+      regexp = Regexp.new "^#{escaped}$", Regexp::IGNORECASE
+      signup_domain =~ regexp
+    end
+  end
+end

app/models/member.rb

@@ -12,6 +12,7 @@ class Member < ApplicationRecord
   include Gitlab::Utils::StrongMemoize
   include FromUnion
   include UpdateHighestRole
+  include RestrictedSignup
 
   AVATAR_SIZE = 40
   ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT = 10
@@ -42,6 +43,7 @@ class Member < ApplicationRecord
       scope: [:source_type, :source_id],
       allow_nil: true
     }
+  validate :signup_email_valid?, on: :create, if: ->(member) { member.invite_email.present? }
   validates :user_id,
     uniqueness: {
       message: _('project bots cannot be added to other groups / projects')
@@ -433,6 +435,12 @@ class Member < ApplicationRecord
     end
   end
 
+  def signup_email_valid?
+    error = validate_admin_signup_restrictions(invite_email)
+
+    errors.add(:user, error) if error
+  end
+
   def update_highest_role?
     return unless user_id.present?
 

app/models/user.rb

@@ -26,6 +26,7 @@ class User < ApplicationRecord
   include UpdateHighestRole
   include HasUserType
   include Gitlab::Auth::Otp::Fortinet
+  include RestrictedSignup
 
   DEFAULT_NOTIFICATION_LEVEL = :participating
 
@@ -237,8 +238,7 @@ class User < ApplicationRecord
   validate :owns_notification_email, if: :notification_email_changed?
   validate :owns_public_email, if: :public_email_changed?
   validate :owns_commit_email, if: :commit_email_changed?
-  validate :signup_domain_valid?, on: :create, if: ->(user) { !user.created_by_id }
-  validate :check_email_restrictions, on: :create, if: ->(user) { !user.created_by_id }
+  validate :signup_email_valid?, on: :create, if: ->(user) { !user.created_by_id }
   validate :check_username_format, if: :username_changed?
 
   validates :theme_id, allow_nil: true, inclusion: { in: Gitlab::Themes.valid_ids,
@@ -2072,51 +2072,10 @@ class User < ApplicationRecord
     end
   end
 
-  def signup_domain_valid?
-    valid = true
-    error = nil
+  def signup_email_valid?
+    error = validate_admin_signup_restrictions(email)
 
-    if Gitlab::CurrentSettings.domain_denylist_enabled?
-      blocked_domains = Gitlab::CurrentSettings.domain_denylist
-      if domain_matches?(blocked_domains, email)
-        error = 'is not from an allowed domain.'
-        valid = false
-      end
-    end
-
-    allowed_domains = Gitlab::CurrentSettings.domain_allowlist
-    unless allowed_domains.blank?
-      if domain_matches?(allowed_domains, email)
-        valid = true
-      else
-        error = "domain is not authorized for sign-up"
-        valid = false
-      end
-    end
-
-    errors.add(:email, error) unless valid
-
-    valid
-  end
-
-  def domain_matches?(email_domains, email)
-    signup_domain = Mail::Address.new(email).domain
-    email_domains.any? do |domain|
-      escaped = Regexp.escape(domain).gsub('\*', '.*?')
-      regexp = Regexp.new "^#{escaped}$", Regexp::IGNORECASE
-      signup_domain =~ regexp
-    end
-  end
-
-  def check_email_restrictions
-    return unless Gitlab::CurrentSettings.email_restrictions_enabled?
-
-    restrictions = Gitlab::CurrentSettings.email_restrictions
-    return if restrictions.blank?
-
-    if Gitlab::UntrustedRegexp.new(restrictions).match?(email)
-      errors.add(:email, _('is not allowed. Try again with a different email address, or contact your GitLab admin.'))
-    end
+    errors.add(:email, error) if error
   end
 
   def check_username_format

ee/spec/requests/api/invitations_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe API::Invitations, 'EE Invitations' do
+  include GroupAPIHelpers
+
+  let_it_be(:admin) { create(:user, :admin, email: 'admin@example.com') }
+  let_it_be(:group) { create(:group) }
+
+  let(:url) { "/groups/#{group.id}/invitations" }
+  let(:invite_email) { 'restricted@example.org' }
+
+  shared_examples 'restricted email error' do |message, code|
+    it 'returns an http error response and the validation message' do
+      post api(url, admin),
+      params: { email: invite_email, access_level: Member::MAINTAINER }
+
+      expect(response).to have_gitlab_http_status(code)
+      expect(json_response['message'][invite_email]).to eq message
+    end
+  end
+
+  shared_examples 'admin signup restrictions email error' do
+    context 'when restricted by admin signup restriction - denylist' do
+      before do
+        stub_application_setting(domain_denylist_enabled: true)
+        stub_application_setting(domain_denylist: ['example.org'])
+      end
+
+      # this response code should be changed to 4xx: https://gitlab.com/gitlab-org/gitlab/-/issues/321706
+      it_behaves_like 'restricted email error', 'User is not from an allowed domain.', :created
+    end
+
+    context 'when restricted by admin signup restriction - allowlist' do
+      before do
+        stub_application_setting(domain_allowlist: ['example.com'])
+      end
+
+      # this response code should be changed to 4xx: https://gitlab.com/gitlab-org/gitlab/-/issues/321706
+      it_behaves_like 'restricted email error', 'User domain is not authorized for sign-up.', :created
+    end
+
+    context 'when restricted by admin signup restriction - email restrictions' do
+      before do
+        stub_application_setting(email_restrictions_enabled: true)
+        stub_application_setting(email_restrictions: '([\+]|\b(\w*example.org\w*)\b)')
+      end
+
+      # this response code should be changed to 4xx: https://gitlab.com/gitlab-org/gitlab/-/issues/321706
+      it_behaves_like 'restricted email error', 'User is not allowed. Try again with a different email address, or contact your GitLab admin.', :created
+    end
+  end
+
+  describe 'POST /groups/:id/invitations' do
+    context 'when the group is restricted by admin signup restrictions' do
+      it_behaves_like 'admin signup restrictions email error'
+    end
+
+    context 'when the group is restricted by group signup restriction - allowed domains for signup' do
+      before do
+        stub_licensed_features(group_allowed_email_domains: true)
+        create(:allowed_email_domain, group: group, domain: 'example.com')
+      end
+
+      # this response code should be changed to 4xx: https://gitlab.com/gitlab-org/gitlab/-/issues/321706
+      it_behaves_like 'restricted email error', "Invite email email does not match the allowed domain of example.com", :success
+    end
+  end
+
+  describe 'POST /projects/:id/invitations' do
+    let_it_be(:project) { create(:project, namespace: group) }
+
+    let(:url) { "/projects/#{project.id}/invitations" }
+
+    context 'when the project is restricted by admin signup restrictions' do
+      it_behaves_like 'admin signup restrictions email error'
+    end
+  end
+end

locale/gitlab.pot

@@ -39094,6 +39094,9 @@ msgstr ""
 msgid "does not have a supported extension. Only %{extension_list} are supported"
 msgstr ""
 
+msgid "domain is not authorized for sign-up."
+msgstr ""
+
 msgid "download it"
 msgstr ""
 
@@ -39320,6 +39323,9 @@ msgstr ""
 msgid "is not an email you own"
 msgstr ""
 
+msgid "is not from an allowed domain."
+msgstr ""
+
 msgid "is not in the group enforcing Group Managed Account"
 msgstr ""
 

spec/models/member_spec.rb

@@ -64,6 +64,49 @@ RSpec.describe Member do
       end
     end
 
+    context 'with admin signup restrictions' do
+      context 'when allowed domains for signup is enabled' do
+        before do
+          stub_application_setting(domain_allowlist: ['example.com'])
+        end
+
+        it 'adds an error message when email is not accepted' do
+          member = build(:group_member, :invited, invite_email: 'info@gitlab.com')
+
+          expect(member).not_to be_valid
+          expect(member.errors.messages[:user].first).to eq(_('domain is not authorized for sign-up.'))
+        end
+      end
+
+      context 'when denylist is enabled' do
+        before do
+          stub_application_setting(domain_denylist_enabled: true)
+          stub_application_setting(domain_denylist: ['example.org'])
+        end
+
+        it 'adds an error message when email is denied' do
+          member = build(:group_member, :invited, invite_email: 'denylist@example.org')
+
+          expect(member).not_to be_valid
+          expect(member.errors.messages[:user].first).to eq(_('is not from an allowed domain.'))
+        end
+      end
+
+      context 'when email restrictions is enabled' do
+        before do
+          stub_application_setting(email_restrictions_enabled: true)
+          stub_application_setting(email_restrictions: '([\+]|\b(\w*gitlab.com\w*)\b)')
+        end
+
+        it 'adds an error message when email is not accepted' do
+          member = build(:group_member, :invited, invite_email: 'info@gitlab.com')
+
+          expect(member).not_to be_valid
+          expect(member.errors.messages[:user].first).to eq(_('is not allowed. Try again with a different email address, or contact your GitLab admin.'))
+        end
+      end
+    end
+
     context "when a child member inherits its access level" do
       let(:user) { create(:user) }
       let(:member) { create(:group_member, :developer, user: user) }

spec/models/user_spec.rb

@@ -496,7 +496,7 @@ RSpec.describe User do
     describe 'email' do
       context 'when no signup domains allowed' do
         before do
-          allow_any_instance_of(ApplicationSetting).to receive(:domain_allowlist).and_return([])
+          stub_application_setting(domain_allowlist: [])
         end
 
         it 'accepts any email' do
@@ -507,7 +507,7 @@ RSpec.describe User do
 
       context 'bad regex' do
         before do
-          allow_any_instance_of(ApplicationSetting).to receive(:domain_allowlist).and_return(['([a-zA-Z0-9]+)+\.com'])
+          stub_application_setting(domain_allowlist: ['([a-zA-Z0-9]+)+\.com'])
         end
 
         it 'does not hang on evil input' do
@@ -521,7 +521,7 @@ RSpec.describe User do
 
       context 'when a signup domain is allowed and subdomains are allowed' do
         before do
-          allow_any_instance_of(ApplicationSetting).to receive(:domain_allowlist).and_return(['example.com', '*.example.com'])
+          stub_application_setting(domain_allowlist: ['example.com', '*.example.com'])
         end
 
         it 'accepts info@example.com' do
@@ -537,12 +537,13 @@ RSpec.describe User do
         it 'rejects example@test.com' do
           user = build(:user, email: "example@test.com")
           expect(user).to be_invalid
+          expect(user.errors.messages[:email].first).to eq(_('domain is not authorized for sign-up.'))
         end
       end
 
       context 'when a signup domain is allowed and subdomains are not allowed' do
         before do
-          allow_any_instance_of(ApplicationSetting).to receive(:domain_allowlist).and_return(['example.com'])
+          stub_application_setting(domain_allowlist: ['example.com'])
         end
 
         it 'accepts info@example.com' do
@@ -553,11 +554,13 @@ RSpec.describe User do
         it 'rejects info@test.example.com' do
           user = build(:user, email: "info@test.example.com")
           expect(user).to be_invalid
+          expect(user.errors.messages[:email].first).to eq(_('domain is not authorized for sign-up.'))
         end
 
         it 'rejects example@test.com' do
           user = build(:user, email: "example@test.com")
           expect(user).to be_invalid
+          expect(user.errors.messages[:email].first).to eq(_('domain is not authorized for sign-up.'))
         end
 
         it 'accepts example@test.com when added by another user' do
@@ -568,13 +571,13 @@ RSpec.describe User do
 
       context 'domain denylist' do
         before do
-          allow_any_instance_of(ApplicationSetting).to receive(:domain_denylist_enabled?).and_return(true)
-          allow_any_instance_of(ApplicationSetting).to receive(:domain_denylist).and_return(['example.com'])
+          stub_application_setting(domain_denylist_enabled: true)
+          stub_application_setting(domain_denylist: ['example.com'])
         end
 
         context 'bad regex' do
           before do
-            allow_any_instance_of(ApplicationSetting).to receive(:domain_denylist).and_return(['([a-zA-Z0-9]+)+\.com'])
+            stub_application_setting(domain_denylist: ['([a-zA-Z0-9]+)+\.com'])
           end
 
           it 'does not hang on evil input' do
@@ -595,6 +598,7 @@ RSpec.describe User do
           it 'rejects info@example.com' do
             user = build(:user, email: 'info@example.com')
             expect(user).not_to be_valid
+            expect(user.errors.messages[:email].first).to eq(_('is not from an allowed domain.'))
           end
 
           it 'accepts info@example.com when added by another user' do
@@ -605,8 +609,8 @@ RSpec.describe User do
 
         context 'when a signup domain is denied but a wildcard subdomain is allowed' do
           before do
-            allow_any_instance_of(ApplicationSetting).to receive(:domain_denylist).and_return(['test.example.com'])
-            allow_any_instance_of(ApplicationSetting).to receive(:domain_allowlist).and_return(['*.example.com'])
+            stub_application_setting(domain_denylist: ['test.example.com'])
+            stub_application_setting(domain_allowlist: ['*.example.com'])
           end
 
           it 'gives priority to allowlist and allow info@test.example.com' do
@@ -617,7 +621,7 @@ RSpec.describe User do
 
         context 'with both lists containing a domain' do
           before do
-            allow_any_instance_of(ApplicationSetting).to receive(:domain_allowlist).and_return(['test.com'])
+            stub_application_setting(domain_allowlist: ['test.com'])
           end
 
           it 'accepts info@test.com' do
@@ -628,6 +632,7 @@ RSpec.describe User do
           it 'rejects info@example.com' do
             user = build(:user, email: 'info@example.com')
             expect(user).not_to be_valid
+            expect(user.errors.messages[:email].first).to eq(_('domain is not authorized for sign-up.'))
           end
         end
       end