Merge branch 'tc-fix-private-subgroups-shown' into 'security'

Use GroupsFinder to find subgroups the user has access to

See merge request !2096

8 files changed, 105 insertions, 19 deletions

diff --git a/app/controllers/explore/groups_controller.rb b/app/controllers/explore/groups_controller.rb
index 68228c095da..81883c543ba 100644
--- a/app/controllers/explore/groups_controller.rb
+++ b/app/controllers/explore/groups_controller.rb
@@ -1,6 +1,6 @@
 class Explore::GroupsController < Explore::ApplicationController
   def index
-    @groups = GroupsFinder.new.execute(current_user)
+    @groups = GroupsFinder.new(current_user).execute
     @groups = @groups.search(params[:filter_groups]) if params[:filter_groups].present?
     @groups = @groups.sort(@sort = params[:sort])
     @groups = @groups.page(params[:page])
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 46c3ff10694..1515173d0ac 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -64,7 +64,7 @@ class GroupsController < Groups::ApplicationController
   end
 
   def subgroups
-    @nested_groups = group.children
+    @nested_groups = GroupsFinder.new(current_user, parent: group).execute
     @nested_groups = @nested_groups.search(params[:filter_groups]) if params[:filter_groups].present?
   end
 
diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb
index d932a17883f..f68610e197c 100644
--- a/app/finders/groups_finder.rb
+++ b/app/finders/groups_finder.rb
@@ -1,13 +1,19 @@
 class GroupsFinder < UnionFinder
-  def execute(current_user = nil)
-    segments = all_groups(current_user)
+  def initialize(current_user = nil, params = {})
+    @current_user = current_user
+    @params = params
+  end
 
-    find_union(segments, Group).with_route.order_id_desc
+  def execute
+    groups = find_union(all_groups, Group).with_route.order_id_desc
+    by_parent(groups)
   end
 
   private
 
-  def all_groups(current_user)
+  attr_reader :current_user, :params
+
+  def all_groups
     groups = []
 
     groups << current_user.authorized_groups if current_user
@@ -15,4 +21,10 @@ class GroupsFinder < UnionFinder
 
     groups
   end
+
+  def by_parent(groups)
+    return groups unless params[:parent]
+
+    groups.where(parent: params[:parent])
+  end
 end
diff --git a/changelogs/unreleased/tc-fix-private-subgroups-shown.yml b/changelogs/unreleased/tc-fix-private-subgroups-shown.yml
new file mode 100644
index 00000000000..82e03921854
--- /dev/null
+++ b/changelogs/unreleased/tc-fix-private-subgroups-shown.yml
@@ -0,0 +1,4 @@
+---
+title: "Do not show private groups on subgroups page if user doesn't have access to"
+merge_request:
+author:
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 09d105f6b4c..9ccc75681f9 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -52,7 +52,7 @@ module API
                  elsif current_user.admin
                    Group.all
                  elsif params[:all_available]
-                   GroupsFinder.new.execute(current_user)
+                   GroupsFinder.new(current_user).execute
                  else
                    current_user.groups
                  end
diff --git a/lib/api/v3/groups.rb b/lib/api/v3/groups.rb
index 63d464b926b..dbf7a3cf785 100644
--- a/lib/api/v3/groups.rb
+++ b/lib/api/v3/groups.rb
@@ -45,7 +45,7 @@ module API
           groups = if current_user.admin
                      Group.all
                    elsif params[:all_available]
-                     GroupsFinder.new.execute(current_user)
+                     GroupsFinder.new(current_user).execute
                    else
                      current_user.groups
                    end
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index 073b87a1cb4..4c8d82a1677 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -26,6 +26,41 @@ describe GroupsController do
     end
   end
 
+  describe 'GET #subgroups' do
+    let!(:public_subgroup) { create(:group, :public, parent: group) }
+    let!(:private_subgroup) { create(:group, :private, parent: group) }
+
+    context 'as a user' do
+      before do
+        sign_in(user)
+      end
+
+      it 'shows the public subgroups' do
+        get :subgroups, id: group.to_param
+
+        expect(assigns(:nested_groups)).to contain_exactly(public_subgroup)
+      end
+
+      context 'being member' do
+        it 'shows public and private subgroups the user is member of' do
+          private_subgroup.add_guest(user)
+
+          get :subgroups, id: group.to_param
+
+          expect(assigns(:nested_groups)).to contain_exactly(public_subgroup, private_subgroup)
+        end
+      end
+    end
+
+    context 'as a guest' do
+      it 'shows the public subgroups' do
+        get :subgroups, id: group.to_param
+
+        expect(assigns(:nested_groups)).to contain_exactly(public_subgroup)
+      end
+    end
+  end
+
   describe 'GET #issues' do
     let(:issue_1) { create(:issue, project: project) }
     let(:issue_2) { create(:issue, project: project) }
diff --git a/spec/finders/groups_finder_spec.rb b/spec/finders/groups_finder_spec.rb
index d5d111e8d15..5b3591550c1 100644
--- a/spec/finders/groups_finder_spec.rb
+++ b/spec/finders/groups_finder_spec.rb
@@ -3,29 +3,64 @@ require 'spec_helper'
 describe GroupsFinder do
   describe '#execute' do
     let(:user)            { create(:user) }
-    let!(:private_group)  { create(:group, :private) }
-    let!(:internal_group) { create(:group, :internal) }
-    let!(:public_group)   { create(:group, :public) }
-    let(:finder)          { described_class.new }
 
-    describe 'execute' do
-      describe 'without a user' do
-        subject { finder.execute }
+    context 'root level groups' do
+      let!(:private_group)  { create(:group, :private) }
+      let!(:internal_group) { create(:group, :internal) }
+      let!(:public_group)   { create(:group, :public) }
+
+      context 'without a user' do
+        subject { described_class.new.execute }
 
         it { is_expected.to eq([public_group]) }
       end
 
-      describe 'with a user' do
-        subject { finder.execute(user) }
+      context 'with a user' do
+        subject { described_class.new(user).execute }
 
         context 'normal user' do
-          it { is_expected.to eq([public_group, internal_group]) }
+          it { is_expected.to contain_exactly(public_group, internal_group) }
         end
 
         context 'external user' do
           let(:user) { create(:user, external: true) }
 
-          it { is_expected.to eq([public_group]) }
+          it { is_expected.to contain_exactly(public_group) }
+        end
+
+        context 'user is member of the private group' do
+          before do
+            private_group.add_guest(user)
+          end
+
+          it { is_expected.to contain_exactly(public_group, internal_group, private_group) }
+        end
+      end
+    end
+
+    context 'subgroups' do
+      let!(:parent_group) { create(:group, :public) }
+      let!(:public_subgroup) { create(:group, :public, parent: parent_group) }
+      let!(:internal_subgroup) { create(:group, :internal, parent: parent_group) }
+      let!(:private_subgroup) { create(:group, :private, parent: parent_group) }
+
+      context 'without a user' do
+        it 'only returns public subgroups' do
+          expect(described_class.new(nil, parent: parent_group).execute).to contain_exactly(public_subgroup)
+        end
+      end
+
+      context 'with a user' do
+        it 'returns public and internal subgroups' do
+          expect(described_class.new(user, parent: parent_group).execute).to contain_exactly(public_subgroup, internal_subgroup)
+        end
+
+        context 'being member' do
+          it 'returns public subgroups, internal subgroups, and private subgroups user is member of' do
+            private_subgroup.add_guest(user)
+
+            expect(described_class.new(user, parent: parent_group).execute).to contain_exactly(public_subgroup, internal_subgroup, private_subgroup)
+          end
         end
       end
     end