Include group parents into read access for project and group

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

5 files changed, 93 insertions, 6 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index ac8a82c8c1e..50c949d84aa 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -155,15 +155,17 @@ class Group < Namespace
   end
 
   def has_owner?(user)
-    owners.include?(user)
+    members_with_parents.owners.where(user_id: user).any?
   end
 
   def has_master?(user)
-    members.masters.where(user_id: user).any?
+    members_with_parents.masters.where(user_id: user).any?
   end
 
+  # Check if user is a last owner of the group.
+  # Parent owners are ignored for nested groups.
   def last_owner?(user)
-    has_owner?(user) && owners.size == 1
+    owners.include?(user) && owners.size == 1
   end
 
   def avatar_type
@@ -189,6 +191,14 @@ class Group < Namespace
   end
 
   def refresh_members_authorized_projects
-    UserProjectAccessChangedService.new(users.pluck(:id)).execute
+    UserProjectAccessChangedService.new(users_with_parents.pluck(:id)).execute
+  end
+
+  def members_with_parents
+    GroupMember.where(requested_at: nil, source_id: parents.map(&:id).push(id))
+  end
+
+  def users_with_parents
+    User.where(id: members_with_parents.pluck(:user_id))
   end
 end
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index 6f943feb2a7..0be6e113655 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -4,7 +4,7 @@ class GroupPolicy < BasePolicy
     return unless @user
 
     globally_viewable = @subject.public? || (@subject.internal? && !@user.external?)
-    member = @subject.users.include?(@user)
+    member = @subject.users_with_parents.include?(@user)
     owner = @user.admin? || @subject.has_owner?(@user)
     master = owner || @subject.has_master?(@user)
 
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index b5db9c12622..eaf3035dfe1 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -245,7 +245,7 @@ class ProjectPolicy < BasePolicy
   def project_group_member?(user)
     project.group &&
       (
-        project.group.members.exists?(user_id: user.id) ||
+        project.group.members_with_parents.exists?(user_id: user.id) ||
         project.group.requesters.exists?(user_id: user.id)
       )
   end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 7d5ecfbaa64..45fe927202b 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -277,4 +277,15 @@ describe Group, models: true do
     it { is_expected.to be_valid }
     it { expect(subject.parent).to be_kind_of(Group) }
   end
+
+  describe '#members_with_parents' do
+    let!(:group) { create(:group, :nested) }
+    let!(:master) { group.parent.add_user(create(:user), GroupMember::MASTER) }
+    let!(:developer) { group.add_user(create(:user), GroupMember::DEVELOPER) }
+
+    it 'returns parents members' do
+      expect(group.members_with_parents).to include(developer)
+      expect(group.members_with_parents).to include(master)
+    end
+  end
 end
diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb
index a20ac303a53..5c34ff04152 100644
--- a/spec/policies/group_policy_spec.rb
+++ b/spec/policies/group_policy_spec.rb
@@ -105,4 +105,70 @@ describe GroupPolicy, models: true do
       is_expected.to include(*owner_permissions)
     end
   end
+
+  describe 'private nested group inherit permissions' do
+    let(:nested_group) { create(:group, :private, parent: group) }
+
+    subject { described_class.abilities(current_user, nested_group).to_set }
+
+    context 'with no user' do
+      let(:current_user) { nil }
+
+      it do
+        is_expected.not_to include(:read_group)
+        is_expected.not_to include(*master_permissions)
+        is_expected.not_to include(*owner_permissions)
+      end
+    end
+
+    context 'guests' do
+      let(:current_user) { guest }
+
+      it do
+        is_expected.to include(:read_group)
+        is_expected.not_to include(*master_permissions)
+        is_expected.not_to include(*owner_permissions)
+      end
+    end
+
+    context 'reporter' do
+      let(:current_user) { reporter }
+
+      it do
+        is_expected.to include(:read_group)
+        is_expected.not_to include(*master_permissions)
+        is_expected.not_to include(*owner_permissions)
+      end
+    end
+
+    context 'developer' do
+      let(:current_user) { developer }
+
+      it do
+        is_expected.to include(:read_group)
+        is_expected.not_to include(*master_permissions)
+        is_expected.not_to include(*owner_permissions)
+      end
+    end
+
+    context 'master' do
+      let(:current_user) { master }
+
+      it do
+        is_expected.to include(:read_group)
+        is_expected.to include(*master_permissions)
+        is_expected.not_to include(*owner_permissions)
+      end
+    end
+
+    context 'owner' do
+      let(:current_user) { owner }
+
+      it do
+        is_expected.to include(:read_group)
+        is_expected.to include(*master_permissions)
+        is_expected.to include(*owner_permissions)
+      end
+    end
+  end
 end