Merge branch 'security-validate-project-members' into 'master'

Don't allow to add users to project with email different than group setting

See merge request gitlab-org/security/gitlab!1510

ee/app/models/ee/group_member.rb

@@ -33,54 +33,6 @@ module EE
       end
     end
 
-    def group_has_domain_limitations?
-      group.licensed_feature_available?(:group_allowed_email_domains) && group_allowed_email_domains.any?
-    end
-
-    def group_domain_limitations
-      if user
-        return if user.project_bot?
-
-        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
-      return if matches_at_least_one_group_allowed_email_domain?(user.email)
-
-      errors.add(:user, email_does_not_match_any_allowed_domains(user.email))
-    end
-
-    def validate_invitation_email
-      return if matches_at_least_one_group_allowed_email_domain?(invite_email)
-
-      errors.add(:invite_email, email_does_not_match_any_allowed_domains(invite_email))
-    end
-
-    def group_saml_identity
-      return unless source.saml_provider
-
-      if user.group_saml_identities.loaded?
-        user.group_saml_identities.detect { |i| i.saml_provider_id == source.saml_provider.id }
-      else
-        user.group_saml_identities.find_by(saml_provider: source.saml_provider)
-      end
-    end
-
     def provisioned_by_this_group?
       user&.user_detail&.provisioned_by_group_id == source_id
     end
@@ -95,25 +47,6 @@ module EE
       errors.add(:access_level, "is not included in the list")
     end
 
-    def email_does_not_match_any_allowed_domains(email)
-      n_("email does not match the allowed domain of %{email_domains}", "email does not match the allowed domains: %{email_domains}", group_allowed_email_domains.size) %
-        { email_domains: group_allowed_email_domains.map(&:domain).join(', ') }
-    end
-
-    def email_not_verified
-      _("email '%{email}' is not a verified email." % { email: user.email })
-    end
-
-    def group_allowed_email_domains
-      group.root_ancestor_allowed_email_domains
-    end
-
-    def matches_at_least_one_group_allowed_email_domain?(email)
-      group_allowed_email_domains.any? do |allowed_email_domain|
-        allowed_email_domain.email_matches_domain?(email)
-      end
-    end
-
     override :post_create_hook
     def post_create_hook
       super

ee/app/models/ee/member.rb

@@ -41,5 +41,80 @@ module EE
     def source_kind
       source.is_a?(Group) && source.parent.present? ? 'Sub group' : source.class.to_s
     end
+
+    def group_has_domain_limitations?
+      return false unless group
+
+      group.licensed_feature_available?(:group_allowed_email_domains) && group_allowed_email_domains.any?
+    end
+
+    def group_domain_limitations
+      return unless group
+
+      if user
+        return if user.project_bot?
+
+        validate_users_email
+        validate_email_verified
+      else
+        validate_invitation_email
+      end
+    end
+
+    def group_saml_identity(root_ancestor: false)
+      saml_group = root_ancestor ? group.root_ancestor : group
+
+      return unless saml_group.saml_provider
+
+      if user.group_saml_identities.loaded?
+        user.group_saml_identities.detect { |i| i.saml_provider_id == saml_group.saml_provider.id }
+      else
+        user.group_saml_identities.find_by(saml_provider: saml_group.saml_provider)
+      end
+    end
+
+    private
+
+    def group_allowed_email_domains
+      return [] unless group
+
+      group.root_ancestor_allowed_email_domains
+    end
+
+    def validate_users_email
+      return if matches_at_least_one_group_allowed_email_domain?(user.email)
+
+      errors.add(:user, email_does_not_match_any_allowed_domains(user.email))
+    end
+
+    def validate_invitation_email
+      return if matches_at_least_one_group_allowed_email_domain?(invite_email)
+
+      errors.add(:invite_email, email_does_not_match_any_allowed_domains(invite_email))
+    end
+
+    def validate_email_verified
+      return if user.primary_email_verified?
+
+      return if group_saml_identity(root_ancestor: true).present?
+      return if group.root_ancestor.scim_identities.for_user(user).exists?
+
+      errors.add(:user, email_not_verified)
+    end
+
+    def email_does_not_match_any_allowed_domains(email)
+      n_("email does not match the allowed domain of %{email_domains}", "email does not match the allowed domains: %{email_domains}", group_allowed_email_domains.size) %
+        { email_domains: group_allowed_email_domains.map(&:domain).join(', ') }
+    end
+
+    def matches_at_least_one_group_allowed_email_domain?(email)
+      group_allowed_email_domains.any? do |allowed_email_domain|
+        allowed_email_domain.email_matches_domain?(email)
+      end
+    end
+
+    def email_not_verified
+      _("email '%{email}' is not a verified email." % { email: user.email })
+    end
   end
 end

ee/app/models/ee/project_member.rb

@@ -9,6 +9,7 @@ module EE
 
       validate :sso_enforcement, if: :group, unless: :project_bot
       validate :gma_enforcement, if: :group, unless: :project_bot
+      validate :group_domain_limitations, if: -> { group && group_has_domain_limitations? }, on: :create
 
       before_destroy :delete_member_branch_protection
       before_destroy :delete_protected_environment_acceses
@@ -44,5 +45,11 @@ module EE
     def provisioned_by_this_group?
       false
     end
+
+    def group_saml_identity(root_ancestor: false)
+      return unless group
+
+      super
+    end
   end
 end

ee/spec/models/group_member_spec.rb

@@ -7,132 +7,12 @@ RSpec.describe GroupMember do
   it_behaves_like 'member validations'
 
   describe 'validations' do
-    describe 'group domain limitations' do
-      let(:group) { create(:group) }
-      let(:gitlab_user) { create(:user, email: 'test@gitlab.com') }
-      let(:gmail_user) { create(:user, email: 'test@gmail.com') }
-      let(:unconfirmed_gitlab_user) { create(:user, :unconfirmed, email: 'unverified@gitlab.com') }
-      let(:acme_user) { create(:user, email: 'user@acme.com') }
-
-      before do
-        create(:allowed_email_domain, group: group, domain: 'gitlab.com')
-        create(:allowed_email_domain, group: group, domain: 'acme.com')
-      end
-
-      context 'when group has email domain feature switched on' do
-        before do
-          stub_licensed_features(group_allowed_email_domains: true)
-        end
-
-        it 'users email must match at least one of the allowed domain emails' do
-          expect(build(:group_member, group: group, user: gmail_user)).to be_invalid
-          expect(build(:group_member, group: group, user: gitlab_user)).to be_valid
-          expect(build(:group_member, group: group, user: acme_user)).to be_valid
-        end
-
-        it 'shows proper error message' do
-          group_member = build(:group_member, group: group, user: gmail_user)
-
-          expect(group_member).to be_invalid
-          expect(group_member.errors[:user]).to include("email does not match the allowed domains: gitlab.com, acme.com")
-        end
-
-        it 'shows proper error message for single domain limitation' do
-          group.allowed_email_domains.last.destroy!
-          group_member = build(:group_member, group: group, user: gmail_user)
-
-          expect(group_member).to be_invalid
-          expect(group_member.errors[:user]).to include("email does not match the allowed domain of gitlab.com")
-        end
-
-        it 'invited email must match at least one of the allowed domain emails' do
-          expect(build(:group_member, group: group, user: nil, invite_email: 'user@gmail.com')).to be_invalid
-          expect(build(:group_member, group: group, user: nil, invite_email: 'user@gitlab.com')).to be_valid
-          expect(build(:group_member, group: group, user: nil, invite_email: 'invite@acme.com')).to be_valid
-        end
-
-        it 'user emails matching allowed domain must be verified' do
-          group_member = build(:group_member, group: group, user: unconfirmed_gitlab_user)
-
-          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 project bot users' do
-          let_it_be(:project_bot) { create(:user, :project_bot, email: "bot@example.com") }
-
-          it 'bot user email does not match' do
-            expect(group.allowed_email_domains.include?(project_bot.email)).to be_falsey
-          end
-
-          it 'allows the project bot user' do
-            expect(build(:group_member, group: group, user: project_bot)).to be_valid
-          end
-        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: unconfirmed_gitlab_user, saml_provider: saml_provider)
-          end
-
-          it 'user emails does not have to be verified' do
-            expect(build(:group_member, group: group, user: unconfirmed_gitlab_user)).to be_valid
-          end
-        end
-
-        context 'with group SCIM users' do
-          let!(:scim_identity) do
-            create(:scim_identity, user: unconfirmed_gitlab_user, group: group)
-          end
-
-          it 'user emails does not have to be verified' do
-            expect(build(:group_member, group: group, user: unconfirmed_gitlab_user)).to be_valid
-          end
-        end
+    describe '#group_domain_validations' do
+      let(:member_type) { :group_member }
+      let(:source) { group }
+      let(:nested_source) { create(:group, parent: group) }
 
-        context 'when group is subgroup' do
-          let(:subgroup) { create(:group, parent: group) }
-
-          it 'users email must match at least one of the allowed domain emails' do
-            expect(build(:group_member, group: subgroup, user: gmail_user)).to be_invalid
-            expect(build(:group_member, group: subgroup, user: gitlab_user)).to be_valid
-            expect(build(:group_member, group: subgroup, user: acme_user)).to be_valid
-          end
-
-          it 'invited email must match at least one of the allowed domain emails' 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
-            expect(build(:group_member, group: subgroup, user: nil, invite_email: 'invite@acme.com')).to be_valid
-          end
-
-          it 'user emails matching allowed domain must be verified' do
-            group_member = build(:group_member, group: subgroup, user: unconfirmed_gitlab_user)
-
-            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
-
-      context 'when group has email domain feature switched off' do
-        it 'users email need not match allowed domain emails' do
-          expect(build(:group_member, group: group, user: gmail_user)).to be_valid
-          expect(build(:group_member, group: group, user: gitlab_user)).to be_valid
-          expect(build(:group_member, group: group, user: acme_user)).to be_valid
-        end
-
-        it 'invited email need not match allowed domain emails' 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
-          expect(build(:group_member, group: group, invite_email: 'user@acme.com')).to be_valid
-        end
-
-        it 'user emails does not have to be verified' do
-          expect(build(:group_member, group: group, user: unconfirmed_gitlab_user)).to be_valid
-        end
-      end
+      it_behaves_like 'member group domain validations'
     end
 
     describe 'access level inclusion' do
@@ -225,42 +105,6 @@ RSpec.describe GroupMember do
     end
   end
 
-  describe '#group_saml_identity' do
-    subject(:group_saml_identity) { member.group_saml_identity }
-
-    let!(:member) { create :group_member }
-
-    context 'without saml_provider' do
-      it { is_expected.to eq nil }
-    end
-
-    context 'with saml_provider enabled' do
-      let!(:saml_provider) { create(:saml_provider, group: member.group) }
-
-      context 'when member has no connected identity' do
-        it { is_expected.to eq nil }
-      end
-
-      context 'when member has connected identity' do
-        let!(:group_related_identity) do
-          create(:group_saml_identity, user: member.user, saml_provider: saml_provider)
-        end
-
-        it 'returns related identity' do
-          expect(group_saml_identity).to eq group_related_identity
-        end
-      end
-
-      context 'when member has connected identity of different group' do
-        before do
-          create(:group_saml_identity, user: member.user)
-        end
-
-        it { is_expected.to eq nil }
-      end
-    end
-  end
-
   context 'group member webhooks', :sidekiq_inline do
     let_it_be_with_refind(:group) { create(:group_with_plan, plan: :ultimate_plan) }
     let_it_be(:group_hook) { create(:group_hook, group: group, member_events: true) }

ee/spec/models/member_spec.rb

@@ -67,4 +67,98 @@ RSpec.describe Member, type: :model do
       it { is_expected.to eq('Project') }
     end
   end
+
+  describe '#group_saml_identity' do
+    shared_examples_for 'member with group saml identity' do
+      context 'without saml_provider' do
+        it { is_expected.to eq nil }
+      end
+
+      context 'with saml_provider enabled' do
+        let!(:saml_provider) { create(:saml_provider, group: member.group) }
+
+        context 'when member has no connected identity' do
+          it { is_expected.to eq nil }
+        end
+
+        context 'when member has connected identity' do
+          let!(:group_related_identity) do
+            create(:group_saml_identity, user: member.user, saml_provider: saml_provider)
+          end
+
+          it 'returns related identity' do
+            expect(group_saml_identity).to eq group_related_identity
+          end
+        end
+
+        context 'when member has connected identity of different group' do
+          before do
+            create(:group_saml_identity, user: member.user)
+          end
+
+          it { is_expected.to eq nil }
+        end
+      end
+    end
+
+    shared_examples_for 'member with group saml identity on the top level' do
+      let!(:saml_provider) { create(:saml_provider, group: parent_group) }
+
+      let!(:group_related_identity) do
+        create(:group_saml_identity, user: member.user, saml_provider: saml_provider)
+      end
+
+      it 'returns related identity' do
+        expect(member.group_saml_identity(root_ancestor: true)).to eq group_related_identity
+      end
+    end
+
+    describe 'for group members' do
+      context 'when member is in a top-level group' do
+        let(:member) { create :group_member }
+
+        subject(:group_saml_identity) { member.group_saml_identity }
+
+        it_behaves_like 'member with group saml identity'
+      end
+
+      context 'when member is in a subgroup' do
+        let(:parent_group) { create(:group) }
+        let(:group) { create(:group, parent: parent_group) }
+        let(:member) { create(:group_member, source: group) }
+
+        it_behaves_like 'member with group saml identity on the top level'
+      end
+    end
+
+    describe 'for project members' do
+      context 'when project is nested in a group' do
+        let(:group) { create(:group) }
+        let(:project) { create(:project, namespace: group)}
+        let(:member) { create :project_member, source: project }
+
+        subject(:group_saml_identity) { member.group_saml_identity }
+
+        it_behaves_like 'member with group saml identity'
+      end
+
+      context 'when project is nested in a subgroup' do
+        let(:parent_group) { create(:group)}
+        let(:group) { create(:group, parent: parent_group) }
+        let(:project) { create(:project, namespace: group)}
+        let(:member) { create :project_member, source: project }
+
+        it_behaves_like 'member with group saml identity on the top level'
+      end
+
+      context 'when project is nested in a personal namespace' do
+        let(:project) { create(:project, namespace: create(:user).namespace )}
+        let(:member) { create :project_member, source: project }
+
+        it 'returns nothing' do
+          expect(member.group_saml_identity(root_ancestor: true)).to be_nil
+        end
+      end
+    end
+  end
 end

ee/spec/models/project_member_spec.rb

@@ -89,6 +89,22 @@ RSpec.describe ProjectMember do
     end
   end
 
+  describe '#group_domain_validations' do
+    let(:member_type) { :project_member }
+    let(:source) { create(:project, namespace: group) }
+    let(:subgroup) { create(:group, parent: group) }
+    let(:nested_source) { create(:project, namespace: subgroup) }
+
+    it_behaves_like 'member group domain validations'
+
+    it 'does not validate personal projects' do
+      unconfirmed_gitlab_user = create(:user, :unconfirmed, email: 'unverified@gitlab.com')
+      member = create(:project, namespace: create(:user).namespace).add_developer(unconfirmed_gitlab_user)
+
+      expect(member).to be_valid
+    end
+  end
+
   describe '#provisioned_by_this_group?' do
     let_it_be(:member) { build(:project_member) }
 

ee/spec/support/shared_examples/models/member_shared_examples.rb

@@ -52,3 +52,157 @@ RSpec.shared_examples 'member validations' do
     end
   end
 end
+
+RSpec.shared_examples 'member group domain validations' do
+  context 'validates group domain limitations' do
+    let(:group) { create(:group) }
+    let(:gitlab_user) { create(:user, email: 'test@gitlab.com') }
+    let(:gmail_user) { create(:user, email: 'test@gmail.com') }
+    let(:unconfirmed_gitlab_user) { create(:user, :unconfirmed, email: 'unverified@gitlab.com') }
+    let(:acme_user) { create(:user, email: 'user@acme.com') }
+
+    before do
+      create(:allowed_email_domain, group: group, domain: 'gitlab.com')
+      create(:allowed_email_domain, group: group, domain: 'acme.com')
+    end
+
+    context 'when project parent has email domain feature switched on' do
+      before do
+        stub_licensed_features(group_allowed_email_domains: true)
+      end
+
+      it 'users email must match at least one of the allowed domain emails' do
+        expect(build(member_type, source: source, user: gmail_user)).to be_invalid
+        expect(build(member_type, source: source, user: gitlab_user)).to be_valid
+        expect(build(member_type, source: source, user: acme_user)).to be_valid
+      end
+
+      it 'shows proper error message' do
+        member = build(member_type, source: source, user: gmail_user)
+
+        expect(member).to be_invalid
+        expect(member.errors[:user]).to include("email does not match the allowed domains: gitlab.com, acme.com")
+      end
+
+      it 'shows proper error message for single domain limitation' do
+        group.allowed_email_domains.last.destroy!
+        member = build(member_type, source: source, user: gmail_user)
+
+        expect(member).to be_invalid
+        expect(member.errors[:user]).to include("email does not match the allowed domain of gitlab.com")
+      end
+
+      it 'invited email must match at least one of the allowed domain emails' do
+        expect(build(member_type, source: source, user: nil, invite_email: 'user@gmail.com')).to be_invalid
+        expect(build(member_type, source: source, user: nil, invite_email: 'user@gitlab.com')).to be_valid
+        expect(build(member_type, source: source, user: nil, invite_email: 'invite@acme.com')).to be_valid
+      end
+
+      it 'user emails matching allowed domain must be verified' do
+        project_member = build(member_type, source: source, user: unconfirmed_gitlab_user)
+
+        expect(project_member).to be_invalid
+        expect(project_member.errors[:user]).to include("email 'unverified@gitlab.com' is not a verified email.")
+      end
+
+      context 'with project bot users' do
+        let_it_be(:project_bot) { create(:user, :project_bot, email: "bot@example.com") }
+
+        it 'bot user email does not match' do
+          expect(group.allowed_email_domains.include?(project_bot.email)).to be_falsey
+        end
+
+        it 'allows the project bot user' do
+          expect(build(member_type, source: source, user: project_bot)).to be_valid
+        end
+      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: unconfirmed_gitlab_user, saml_provider: saml_provider)
+        end
+
+        it 'user emails does not have to be verified' do
+          expect(build(member_type, source: source, user: unconfirmed_gitlab_user)).to be_valid
+        end
+      end
+
+      context 'with group SCIM users' do
+        let!(:scim_identity) do
+          create(:scim_identity, user: unconfirmed_gitlab_user, group: group)
+        end
+
+        it 'user emails does not have to be verified' do
+          expect(build(member_type, source: source, user: unconfirmed_gitlab_user)).to be_valid
+        end
+      end
+
+      context 'when group is subgroup' do
+        it 'users email must match at least one of the allowed domain emails' do
+          expect(build(member_type, source: nested_source, user: gmail_user)).to be_invalid
+          expect(build(member_type, source: nested_source, user: gitlab_user)).to be_valid
+          expect(build(member_type, source: nested_source, user: acme_user)).to be_valid
+        end
+
+        it 'invited email must match at least one of the allowed domain emails' do
+          expect(build(member_type, source: nested_source, user: nil, invite_email: 'user@gmail.com')).to be_invalid
+          expect(build(member_type, source: nested_source, user: nil, invite_email: 'user@gitlab.com')).to be_valid
+          expect(build(member_type, source: nested_source, user: nil, invite_email: 'invite@acme.com')).to be_valid
+        end
+
+        it 'user emails matching allowed domain must be verified' do
+          member = build(member_type, source: nested_source, user: unconfirmed_gitlab_user)
+
+          expect(member).to be_invalid
+          expect(member.errors[:user]).to include("email 'unverified@gitlab.com' is not a verified email.")
+        end
+
+        context 'with group SCIM users' do
+          let!(:scim_identity) do
+            create(:scim_identity, user: unconfirmed_gitlab_user, group: group)
+          end
+
+          it 'user emails does not have to be verified' do
+            expect(build(member_type, source: nested_source, user: unconfirmed_gitlab_user)).to be_valid
+          end
+        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: unconfirmed_gitlab_user, saml_provider: saml_provider)
+          end
+
+          it 'user emails does not have to be verified' do
+            expect(build(member_type, source: nested_source, user: unconfirmed_gitlab_user)).to be_valid
+          end
+        end
+      end
+    end
+
+    context 'when project parent group has email domain feature switched off' do
+      before do
+        stub_licensed_features(group_allowed_email_domains: false)
+      end
+
+      it 'users email need not match allowed domain emails' do
+        expect(build(member_type, source: source, user: gmail_user)).to be_valid
+        expect(build(member_type, source: source, user: gitlab_user)).to be_valid
+        expect(build(member_type, source: source, user: acme_user)).to be_valid
+      end
+
+      it 'invited email need not match allowed domain emails' do
+        expect(build(member_type, source: source, invite_email: 'user@gmail.com')).to be_valid
+        expect(build(member_type, source: source, invite_email: 'user@gitlab.com')).to be_valid
+        expect(build(member_type, source: source, invite_email: 'user@acme.com')).to be_valid
+      end
+
+      it 'user emails does not have to be verified' do
+        expect(build(member_type, source: source, user: unconfirmed_gitlab_user)).to be_valid
+      end
+    end
+  end
+end