Fix access request bug for groups with restricted email domains

This change allows only user with a verified email to be
member of a group when the group has restricted membership
based on email domain

app/controllers/concerns/membership_actions.rb

@@ -53,10 +53,16 @@ module MembershipActions
   end
 
   def request_access
-    membershipable.request_access(current_user)
+    access_requester = membershipable.request_access(current_user)
 
-    redirect_to polymorphic_path(membershipable),
-                notice: _('Your request for access has been queued for review.')
+    if access_requester.persisted?
+      redirect_to polymorphic_path(membershipable),
+                  notice: _('Your request for access has been queued for review.')
+    else
+      redirect_to polymorphic_path(membershipable),
+                  alert: _("Your request for access could not be processed: %{error_meesage}") %
+                    { error_meesage: access_requester.errors.full_messages.to_sentence }
+    end
   end
 
   def approve_access_request

ee/app/models/ee/group_member.rb

@@ -36,7 +36,23 @@ module EE
     end
 
     def group_domain_limitations
-      user ? validate_users_email : validate_invitation_email
+      if user
+        validate_users_email
+        validate_email_verified
+      else
+        validate_invitation_email
+      end
+    end
+
+    def validate_email_verified
+      return if user.primary_email_verified?
+
+      # Do not validate if emails are verified
+      # for users created via SAML/SCIM.
+      return if group_saml_identity.present?
+      return if source.scim_identities.for_user(user).exists?
+
+      errors.add(:user, email_not_verified)
     end
 
     def validate_users_email
@@ -67,6 +83,10 @@ module EE
       _("email '%{email}' does not match the allowed domain of '%{email_domain}'" % { email: email, email_domain: group_allowed_email_domain.domain })
     end
 
+    def email_not_verified
+      _("email '%{email}' is not a verified email." % { email: user.email })
+    end
+
     def group_allowed_email_domain
       group.root_ancestor_allowed_email_domain
     end

ee/changelogs/unreleased/security-fix-group-domain-allowed-email-should-be-verified.yml

+---
+title: Allow only users with a verified email to be member of a group when the group
+  has restricted membership based on email domain
+merge_request:
+author:
+type: security

ee/spec/controllers/groups/group_members_controller_spec.rb

@@ -76,4 +76,134 @@ describe Groups::GroupMembersController do
       end
     end
   end
+
+  describe 'POST request_access' do
+    before do
+      create(:allowed_email_domain, group: group)
+      sign_in(requesting_user)
+    end
+
+    shared_examples_for 'creates a new access request' do
+      it 'creates a new access request to the group' do
+        post :request_access, params: { group_id: group }
+
+        expect(response).to set_flash.to 'Your request for access has been queued for review.'
+        expect(response).to redirect_to(group_path(group))
+        expect(group.requesters.exists?(user_id: requesting_user)).to be_truthy
+        expect(group.users).not_to include requesting_user
+      end
+    end
+
+    shared_examples_for 'creates access request for a verified user with email belonging to the allowed domain' do
+      context 'for a user with a verified email belonging to the allowed domain' do
+        let(:email) { 'verified@gitlab.com' }
+        let(:requesting_user) { create(:user, email: email, confirmed_at: Time.current) }
+
+        it_behaves_like 'creates a new access request'
+      end
+    end
+
+    context 'when users with unconfirmed emails are allowed to log-in' do
+      before do
+        stub_feature_flags(soft_email_confirmation: true)
+      end
+
+      context 'when group has email domain feature enabled' do
+        before do
+          stub_licensed_features(group_allowed_email_domains: true)
+        end
+
+        context 'for a user with an un-verified email belonging to the allowed domain' do
+          let(:email) { 'unverified@gitlab.com' }
+          let(:requesting_user) { create(:user, email: email, confirmed_at: nil) }
+
+          it 'does not create a new access request' do
+            post :request_access, params: { group_id: group }
+
+            expect(response).to set_flash.to "Your request for access could not be processed: "\
+              "User email 'unverified@gitlab.com' is not a verified email."
+            expect(response).to redirect_to(group_path(group))
+            expect(group.requesters.exists?(user_id: requesting_user)).to be_falsey
+            expect(group.users).not_to include requesting_user
+          end
+        end
+
+        it_behaves_like 'creates access request for a verified user with email belonging to the allowed domain'
+      end
+
+      context 'when group has email domain feature disabled' do
+        let(:email) { 'unverified@gitlab.com' }
+        let(:requesting_user) { create(:user, email: email, confirmed_at: nil) }
+
+        before do
+          stub_licensed_features(group_allowed_email_domains: false)
+        end
+
+        context 'for a user with an un-verified email belonging to the allowed domain' do
+          it_behaves_like 'creates a new access request'
+        end
+
+        context 'for a user with an un-verified email belonging to a domain different from the allowed domain' do
+          let(:email) { 'unverified@gmail.com' }
+
+          it_behaves_like 'creates a new access request'
+        end
+
+        it_behaves_like 'creates access request for a verified user with email belonging to the allowed domain'
+      end
+    end
+
+    context 'when users with unconfirmed emails are not allowed to log-in' do
+      before do
+        stub_feature_flags(soft_email_confirmation: false)
+      end
+
+      shared_examples_for 'does not create a new access request due to user pending confirmation' do
+        it 'does not create a new access request due to user pending confirmation' do
+          post :request_access, params: { group_id: group }
+
+          expect(response).to redirect_to(new_user_session_path)
+          expect(response).to set_flash.to 'You have to confirm your email address before continuing.'
+          expect(group.requesters.exists?(user_id: requesting_user)).to be_falsey
+          expect(group.users).not_to include requesting_user
+        end
+      end
+
+      context 'when group has email domain feature enabled' do
+        before do
+          stub_licensed_features(group_allowed_email_domains: true)
+        end
+
+        context 'for a user with an un-verified email belonging to the allowed domain' do
+          let(:email) { 'unverified@gitlab.com' }
+          let(:requesting_user) { create(:user, email: email, confirmed_at: nil) }
+
+          it_behaves_like 'does not create a new access request due to user pending confirmation'
+        end
+
+        it_behaves_like 'creates access request for a verified user with email belonging to the allowed domain'
+      end
+
+      context 'when group has email domain feature disabled' do
+        let(:email) { 'unverified@gitlab.com' }
+        let(:requesting_user) { create(:user, email: email, confirmed_at: nil) }
+
+        before do
+          stub_licensed_features(group_allowed_email_domains: false)
+        end
+
+        context 'for a user with an un-verified email belonging to the allowed domain' do
+          it_behaves_like 'does not create a new access request due to user pending confirmation'
+        end
+
+        context 'for a user with an un-verified email belonging to a domain different from the allowed domain' do
+          let(:email) { 'unverified@gmail.com' }
+
+          it_behaves_like 'does not create a new access request due to user pending confirmation'
+        end
+
+        it_behaves_like 'creates access request for a verified user with email belonging to the allowed domain'
+      end
+    end
+  end
 end

ee/spec/models/group_member_spec.rb

@@ -11,6 +11,7 @@ describe GroupMember do
       let(:group) { create(:group) }
       let(:user) { create(:user, email: 'test@gitlab.com') }
       let(:user_2) { create(:user, email: 'test@gmail.com') }
+      let(:user_3) { create(:user, email: 'unverified@gitlab.com', confirmed_at: nil) }
 
       before do
         create(:allowed_email_domain, group: group)
@@ -31,6 +32,35 @@ describe GroupMember do
           expect(build(:group_member, group: group, user: nil, invite_email: 'user@gitlab.com')).to be_valid
         end
 
+        it 'user emails matching allowed domain must be verified' do
+          group_member = build(:group_member, group: group, user: user_3)
+
+          expect(group_member).to be_invalid
+          expect(group_member.errors[:user]).to include("email 'unverified@gitlab.com' is not a verified email.")
+        end
+
+        context 'with group SAML users' do
+          let(:saml_provider) { create(:saml_provider, group: group) }
+
+          let!(:group_related_identity) do
+            create(:group_saml_identity, user: user_3, saml_provider: saml_provider)
+          end
+
+          it 'user emails does not have to be verified' do
+            expect(build(:group_member, group: group, user: user_3)).to be_valid
+          end
+        end
+
+        context 'with group SCIM users' do
+          let!(:scim_identity) do
+            create(:scim_identity, user: user_3, group: group)
+          end
+
+          it 'user emails does not have to be verified' do
+            expect(build(:group_member, group: group, user: user_3)).to be_valid
+          end
+        end
+
         context 'when group is subgroup' do
           let(:subgroup) { create(:group, parent: group) }
 
@@ -43,6 +73,13 @@ describe GroupMember do
             expect(build(:group_member, group: subgroup, user: nil, invite_email: 'user@gmail.com')).to be_invalid
             expect(build(:group_member, group: subgroup, user: nil, invite_email: 'user@gitlab.com')).to be_valid
           end
+
+          it 'user emails matching allowed domain must be verified' do
+            group_member = build(:group_member, group: subgroup, user: user_3)
+
+            expect(group_member).to be_invalid
+            expect(group_member.errors[:user]).to include("email 'unverified@gitlab.com' is not a verified email.")
+          end
         end
       end
 
@@ -56,6 +93,10 @@ describe GroupMember do
           expect(build(:group_member, group: group, invite_email: 'user@gmail.com')).to be_valid
           expect(build(:group_member, group: group, invite_email: 'user@gitlab.com')).to be_valid
         end
+
+        it 'user emails does not have to be verified' do
+          expect(build(:group_member, group: group, user: user_3)).to be_valid
+        end
       end
     end
   end

locale/gitlab.pot

@@ -25465,6 +25465,9 @@ msgstr ""
 msgid "Your projects"
 msgstr ""
 
+msgid "Your request for access could not be processed: %{error_meesage}"
+msgstr ""
+
 msgid "Your request for access has been queued for review."
 msgstr ""
 
@@ -25903,6 +25906,9 @@ msgstr ""
 msgid "email '%{email}' does not match the allowed domain of '%{email_domain}'"
 msgstr ""
 
+msgid "email '%{email}' is not a verified email."
+msgstr ""
+
 msgid "enabled"
 msgstr ""