Secure vulerability and add specs

4 files changed, 67 insertions, 7 deletions

diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index 298769c0eb8..e74e5f008d7 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -53,7 +53,6 @@ class GroupPolicy < BasePolicy
   rule { admin }.enable :read_group
 
   rule { has_projects }.policy do
-    enable :read_group
     enable :read_list
     enable :read_label
   end
diff --git a/changelogs/unreleased/security-shared-project-private-group.yml b/changelogs/unreleased/security-shared-project-private-group.yml
new file mode 100644
index 00000000000..3b21daa5491
--- /dev/null
+++ b/changelogs/unreleased/security-shared-project-private-group.yml
@@ -0,0 +1,5 @@
+---
+title: Fixed ability to see private groups by users not belonging to given group
+merge_request:
+author:
+type: security
diff --git a/spec/features/security/group/private_access_spec.rb b/spec/features/security/group/private_access_spec.rb
index 4705cd12d23..3238e07fe15 100644
--- a/spec/features/security/group/private_access_spec.rb
+++ b/spec/features/security/group/private_access_spec.rb
@@ -27,7 +27,7 @@ describe 'Private Group access' do
     it { is_expected.to be_allowed_for(:developer).of(group) }
     it { is_expected.to be_allowed_for(:reporter).of(group) }
     it { is_expected.to be_allowed_for(:guest).of(group) }
-    it { is_expected.to be_allowed_for(project_guest) }
+    it { is_expected.to be_denied_for(project_guest) }
     it { is_expected.to be_denied_for(:user) }
     it { is_expected.to be_denied_for(:external) }
     it { is_expected.to be_denied_for(:visitor) }
@@ -42,7 +42,7 @@ describe 'Private Group access' do
     it { is_expected.to be_allowed_for(:developer).of(group) }
     it { is_expected.to be_allowed_for(:reporter).of(group) }
     it { is_expected.to be_allowed_for(:guest).of(group) }
-    it { is_expected.to be_allowed_for(project_guest) }
+    it { is_expected.to be_denied_for(project_guest) }
     it { is_expected.to be_denied_for(:user) }
     it { is_expected.to be_denied_for(:external) }
     it { is_expected.to be_denied_for(:visitor) }
@@ -58,7 +58,7 @@ describe 'Private Group access' do
     it { is_expected.to be_allowed_for(:developer).of(group) }
     it { is_expected.to be_allowed_for(:reporter).of(group) }
     it { is_expected.to be_allowed_for(:guest).of(group) }
-    it { is_expected.to be_allowed_for(project_guest) }
+    it { is_expected.to be_denied_for(project_guest) }
     it { is_expected.to be_denied_for(:user) }
     it { is_expected.to be_denied_for(:external) }
     it { is_expected.to be_denied_for(:visitor) }
@@ -73,7 +73,7 @@ describe 'Private Group access' do
     it { is_expected.to be_allowed_for(:developer).of(group) }
     it { is_expected.to be_allowed_for(:reporter).of(group) }
     it { is_expected.to be_allowed_for(:guest).of(group) }
-    it { is_expected.to be_allowed_for(project_guest) }
+    it { is_expected.to be_denied_for(project_guest) }
     it { is_expected.to be_denied_for(:user) }
     it { is_expected.to be_denied_for(:external) }
     it { is_expected.to be_denied_for(:visitor) }
@@ -93,4 +93,28 @@ describe 'Private Group access' do
     it { is_expected.to be_denied_for(:visitor) }
     it { is_expected.to be_denied_for(:external) }
   end
+
+  describe 'GET /groups/:path for shared projects' do
+    let(:project) { create(:project, :public) }
+    before do
+      Projects::GroupLinks::CreateService.new(
+        project,
+        create(:user),
+        link_group_access: ProjectGroupLink::DEVELOPER
+      ).execute(group)
+    end
+
+    subject { group_path(group) }
+
+    it { is_expected.to be_allowed_for(:admin) }
+    it { is_expected.to be_allowed_for(:owner).of(group) }
+    it { is_expected.to be_allowed_for(:maintainer).of(group) }
+    it { is_expected.to be_allowed_for(:developer).of(group) }
+    it { is_expected.to be_allowed_for(:reporter).of(group) }
+    it { is_expected.to be_allowed_for(:guest).of(group) }
+    it { is_expected.to be_denied_for(project_guest) }
+    it { is_expected.to be_denied_for(:user) }
+    it { is_expected.to be_denied_for(:external) }
+    it { is_expected.to be_denied_for(:visitor) }
+  end
 end
diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb
index af6d6f084a9..34e9dbffee1 100644
--- a/spec/policies/group_policy_spec.rb
+++ b/spec/policies/group_policy_spec.rb
@@ -74,6 +74,38 @@ describe GroupPolicy do
     end
   end
 
+  context 'with no user and public project' do
+    let(:project) { create(:project, :public) }
+    let(:user) { create(:user) }
+    let(:current_user) { nil }
+
+    before do
+      Projects::GroupLinks::CreateService.new(
+        project,
+        user,
+        link_group_access: ProjectGroupLink::DEVELOPER
+      ).execute(group)
+    end
+
+    it { expect_disallowed(:read_group) }
+  end
+
+  context 'with foreign user and public project' do
+    let(:project) { create(:project, :public) }
+    let(:user) { create(:user) }
+    let(:current_user) { create(:user) }
+
+    before do
+      Projects::GroupLinks::CreateService.new(
+        project,
+        user,
+        link_group_access: ProjectGroupLink::DEVELOPER
+      ).execute(group)
+    end
+
+    it { expect_disallowed(:read_group) }
+  end
+
   context 'has projects' do
     let(:current_user) { create(:user) }
     let(:project) { create(:project, namespace: group) }
@@ -83,7 +115,7 @@ describe GroupPolicy do
     end
 
     it do
-      expect_allowed(:read_group, :read_list, :read_label)
+      expect_allowed(:read_list, :read_label)
     end
 
     context 'in subgroups', :nested_groups do
@@ -91,7 +123,7 @@ describe GroupPolicy do
       let(:project) { create(:project, namespace: subgroup) }
 
       it do
-        expect_allowed(:read_group, :read_list, :read_label)
+        expect_allowed(:read_list, :read_label)
       end
     end
   end