Merge branch 'improve-share-locking-feature-for-subgroups' into 'master'

Improve "Share with group lock" feature for subgroups

Closes #30550

See merge request !13944

3 files changed, 36 insertions, 26 deletions

diff --git a/app/services/concerns/update_visibility_level.rb b/app/services/concerns/update_visibility_level.rb
new file mode 100644
index 00000000000..536fcc6acce
--- /dev/null
+++ b/app/services/concerns/update_visibility_level.rb
@@ -0,0 +1,15 @@
+module UpdateVisibilityLevel
+  def valid_visibility_level_change?(target, new_visibility)
+    # check that user is allowed to set specified visibility_level
+    if new_visibility && new_visibility.to_i != target.visibility_level
+      unless can?(current_user, :change_visibility_level, target) &&
+          Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
+
+        deny_visibility_level(target, new_visibility)
+        return false
+      end
+    end
+
+    true
+  end
+end
diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb
index 1d65c76d282..08e3efb96e3 100644
--- a/app/services/groups/update_service.rb
+++ b/app/services/groups/update_service.rb
@@ -1,18 +1,13 @@
 module Groups
   class UpdateService < Groups::BaseService
+    include UpdateVisibilityLevel
+
     def execute
       reject_parent_id!
 
-      # check that user is allowed to set specified visibility_level
-      new_visibility = params[:visibility_level]
-      if new_visibility && new_visibility.to_i != group.visibility_level
-        unless can?(current_user, :change_visibility_level, group) &&
-            Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
+      return false unless valid_visibility_level_change?(group, params[:visibility_level])
 
-          deny_visibility_level(group, new_visibility)
-          return group
-        end
-      end
+      return false unless valid_share_with_group_lock_change?
 
       group.assign_attributes(params)
 
@@ -30,5 +25,19 @@ module Groups
     def reject_parent_id!
       params.except!(:parent_id)
     end
+
+    def valid_share_with_group_lock_change?
+      return true unless changing_share_with_group_lock?
+      return true if can?(current_user, :change_share_with_group_lock, group)
+
+      group.errors.add(:share_with_group_lock, s_('GroupSettings|cannot be disabled when the parent group "Share with group lock" is enabled, except by the owner of the parent group'))
+      false
+    end
+
+    def changing_share_with_group_lock?
+      return false if params[:share_with_group_lock].nil?
+
+      params[:share_with_group_lock] != group.share_with_group_lock
+    end
   end
 end
diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index cf69007bc3b..cb4ffcab778 100644
--- a/app/services/projects/update_service.rb
+++ b/app/services/projects/update_service.rb
@@ -1,7 +1,9 @@
 module Projects
   class UpdateService < BaseService
+    include UpdateVisibilityLevel
+
     def execute
-      unless visibility_level_allowed?
+      unless valid_visibility_level_change?(project, params[:visibility_level])
         return error('New visibility level not allowed!')
       end
 
@@ -28,22 +30,6 @@ module Projects
 
     private
 
-    def visibility_level_allowed?
-      # check that user is allowed to set specified visibility_level
-      new_visibility = params[:visibility_level]
-
-      if new_visibility && new_visibility.to_i != project.visibility_level
-        unless can?(current_user, :change_visibility_level, project) &&
-            Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
-
-          deny_visibility_level(project, new_visibility)
-          return false
-        end
-      end
-
-      true
-    end
-
     def renaming_project_with_container_registry_tags?
       new_path = params[:path]