Add validation to check visibility level of sub groups.

2 files changed, 60 insertions, 4 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index 15355418d05..fdd175341b3 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -26,6 +26,7 @@ class Group < Namespace
 
   validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
   validate :visibility_level_allowed_by_projects
+  validate :visibility_level_allowed_by_sub_groups, if: :visibility_level_changed?
   validate :visibility_level_allowed_by_parent
 
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
@@ -112,14 +113,25 @@ class Group < Namespace
   end
 
   def visibility_level_allowed_by_projects
-    allowed_by_projects = self.projects.where('visibility_level > ?', self.visibility_level).none?
+    check_visibility_level_for(:projects)
+  end
+
+  def visibility_level_allowed_by_sub_groups
+    check_visibility_level_for(:children)
+  end
 
-    unless allowed_by_projects
+  def check_visibility_level_for(children_type)
+    base_query = public_send(children_type)
+    children_have_higher_visibility = base_query.where('visibility_level > ?', visibility_level).exists?
+
+    if children_have_higher_visibility
+      children_label = children_type == :projects ? 'projects' : 'sub groups'
       level_name = Gitlab::VisibilityLevel.level_name(visibility_level).downcase
-      self.errors.add(:visibility_level, "#{level_name} is not allowed since there are projects with higher visibility.")
+
+      self.errors.add(:visibility_level, "#{level_name} is not allowed since there are #{children_label} with higher visibility.")
     end
 
-    allowed_by_projects
+    children_have_higher_visibility
   end
 
   def avatar_url(**args)
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index a3310cf1dce..c3342afb7fd 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -117,6 +117,50 @@ describe Group do
         end
       end
     end
+
+    describe '#visibility_level_allowed_by_projects' do
+      let!(:internal_group) { create(:group, :internal) }
+      let!(:internal_project) { create(:project, :internal, group: internal_group) }
+
+      context 'when group has a lower visibility' do
+        it 'is invalid' do
+          internal_group.visibility_level = Gitlab::VisibilityLevel::PRIVATE
+
+          expect(internal_group).to be_invalid
+          expect(internal_group.errors[:visibility_level]).to include('private is not allowed since there are projects with higher visibility.')
+        end
+      end
+
+      context 'when group has a higher visibility' do
+        it 'is valid' do
+          internal_group.visibility_level = Gitlab::VisibilityLevel::PUBLIC
+
+          expect(internal_group).to be_valid
+        end
+      end
+    end
+
+    describe '#visibility_level_allowed_by_sub_groups' do
+      let!(:internal_group) { create(:group, :internal) }
+      let!(:internal_sub_group) { create(:group, :internal, parent: internal_group) }
+
+      context 'when parent group has a lower visibility' do
+        it 'is invalid' do
+          internal_group.visibility_level = Gitlab::VisibilityLevel::PRIVATE
+
+          expect(internal_group).to be_invalid
+          expect(internal_group.errors[:visibility_level]).to include('private is not allowed since there are sub groups with higher visibility.')
+        end
+      end
+
+      context 'when parent group has a higher visibility' do
+        it 'is valid' do
+          internal_group.visibility_level = Gitlab::VisibilityLevel::PUBLIC
+
+          expect(internal_group).to be_valid
+        end
+      end
+    end
   end
 
   describe '.visible_to_user' do