From 389f3ac43b10e0ff50fbc4a52ce49b960094e8a9 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Mon, 24 Jul 2017 11:37:06 +0100
Subject: Ensure that `Group#members_with_parents` only includes real users

The `members` table can have entries where `user_id: nil`, because people can
invite group members by email. We never want to include those as members,
because it might cause confusion with the anonymous (logged out) user.
---
 app/models/group.rb       | 6 +++++-
 spec/models/group_spec.rb | 4 ++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/app/models/group.rb b/app/models/group.rb
index dfa4e8adedd..bd5735ed82e 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -167,10 +167,14 @@ class Group < Namespace
   end
 
   def has_owner?(user)
+    return false unless user
+
     members_with_parents.owners.where(user_id: user).any?
   end
 
   def has_master?(user)
+    return false unless user
+
     members_with_parents.masters.where(user_id: user).any?
   end
 
@@ -212,7 +216,7 @@ class Group < Namespace
   end
 
   def members_with_parents
-    GroupMember.non_request.where(source_id: ancestors.pluck(:id).push(id))
+    GroupMember.active.where(source_id: ancestors.pluck(:id).push(id)).where.not(user_id: nil)
   end
 
   def users_with_parents
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 770176451fe..d8e868265ed 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -236,6 +236,7 @@ describe Group, models: true do
   describe '#has_owner?' do
     before do
       @members = setup_group_members(group)
+      create(:group_member, :invited, :owner, group: group)
     end
 
     it { expect(group.has_owner?(@members[:owner])).to be_truthy }
@@ -244,11 +245,13 @@ describe Group, models: true do
     it { expect(group.has_owner?(@members[:reporter])).to be_falsey }
     it { expect(group.has_owner?(@members[:guest])).to be_falsey }
     it { expect(group.has_owner?(@members[:requester])).to be_falsey }
+    it { expect(group.has_owner?(nil)).to be_falsey }
   end
 
   describe '#has_master?' do
     before do
       @members = setup_group_members(group)
+      create(:group_member, :invited, :master, group: group)
     end
 
     it { expect(group.has_master?(@members[:owner])).to be_falsey }
@@ -257,6 +260,7 @@ describe Group, models: true do
     it { expect(group.has_master?(@members[:reporter])).to be_falsey }
     it { expect(group.has_master?(@members[:guest])).to be_falsey }
     it { expect(group.has_master?(@members[:requester])).to be_falsey }
+    it { expect(group.has_master?(nil)).to be_falsey }
   end
 
   describe '#lfs_enabled?' do
-- 
cgit v1.2.1