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

Improve "Share with group lock" feature for subgroups

Closes #30550

See merge request !13944

7 files changed, 126 insertions, 33 deletions

diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb
index eab1feb8a1f..36b79da1bde 100644
--- a/app/helpers/groups_helper.rb
+++ b/app/helpers/groups_helper.rb
@@ -3,6 +3,10 @@ module GroupsHelper
     can?(current_user, :change_visibility_level, group)
   end
 
+  def can_change_share_with_group_lock?(group)
+    can?(current_user, :change_share_with_group_lock, group)
+  end
+
   def group_icon(group)
     if group.is_a?(String)
       group = Group.find_by_full_path(group)
@@ -65,6 +69,20 @@ module GroupsHelper
       { group_name: group.name }
   end
 
+  def share_with_group_lock_help_text(group)
+    return default_help unless group.parent&.share_with_group_lock?
+
+    if group.share_with_group_lock?
+      if can?(current_user, :change_share_with_group_lock, group.parent)
+        ancestor_locked_but_you_can_override(group)
+      else
+        ancestor_locked_so_ask_the_owner(group)
+      end
+    else
+      ancestor_locked_and_has_been_overridden(group)
+    end
+  end
+
   private
 
   def group_title_link(group, hidable: false, show_avatar: false)
@@ -80,4 +98,45 @@ module GroupsHelper
       output.html_safe
     end
   end
+
+  def ancestor_group(group)
+    ancestor = oldest_consecutively_locked_ancestor(group)
+    if can?(current_user, :read_group, ancestor)
+      link_to ancestor.name, group_path(ancestor)
+    else
+      ancestor.name
+    end
+  end
+
+  def remove_the_share_with_group_lock_from_ancestor(group)
+    ancestor = oldest_consecutively_locked_ancestor(group)
+    text = s_("GroupSettings|remove the share with group lock from %{ancestor_group_name}") % { ancestor_group_name: ancestor.name }
+    if can?(current_user, :admin_group, ancestor)
+      link_to text, edit_group_path(ancestor)
+    else
+      text
+    end
+  end
+
+  def oldest_consecutively_locked_ancestor(group)
+    group.ancestors.find do |group|
+      !group.has_parent? || !group.parent.share_with_group_lock?
+    end
+  end
+
+  def default_help
+    s_("GroupSettings|This setting will be applied to all subgroups unless overridden by a group owner.")
+  end
+
+  def ancestor_locked_but_you_can_override(group)
+    s_("GroupSettings|This setting is applied on %{ancestor_group}. You can override the setting or %{remove_ancestor_share_with_group_lock}.").html_safe % { ancestor_group: ancestor_group(group), remove_ancestor_share_with_group_lock: remove_the_share_with_group_lock_from_ancestor(group) }
+  end
+
+  def ancestor_locked_so_ask_the_owner(group)
+    s_("GroupSettings|This setting is applied on %{ancestor_group}. To share projects in this group with another group, ask the owner to override the setting or %{remove_ancestor_share_with_group_lock}.").html_safe % { ancestor_group: ancestor_group(group), remove_ancestor_share_with_group_lock: remove_the_share_with_group_lock_from_ancestor(group) }
+  end
+
+  def ancestor_locked_and_has_been_overridden(group)
+    s_("GroupSettings|This setting is applied on %{ancestor_group} and has been overridden on this subgroup.").html_safe % { ancestor_group: ancestor_group(group) }
+  end
 end
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index e7cbc5170e8..4a9a23fea1f 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -44,6 +44,10 @@ class Namespace < ActiveRecord::Base
 
   after_commit :refresh_access_of_projects_invited_groups, on: :update, if: -> { previous_changes.key?('share_with_group_lock') }
 
+  before_create :sync_share_with_group_lock_with_parent
+  before_update :sync_share_with_group_lock_with_parent, if: :parent_changed?
+  after_update :force_share_with_group_lock_on_descendants, if: -> { share_with_group_lock_changed? && share_with_group_lock? }
+
   # Legacy Storage specific hooks
 
   after_update :move_dir, if: :path_changed?
@@ -219,4 +223,14 @@ class Namespace < ActiveRecord::Base
       errors.add(:parent_id, "has too deep level of nesting")
     end
   end
+
+  def sync_share_with_group_lock_with_parent
+    if parent&.share_with_group_lock?
+      self.share_with_group_lock = true
+    end
+  end
+
+  def force_share_with_group_lock_on_descendants
+    descendants.update_all(share_with_group_lock: true)
+  end
 end
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index 8ada661e571..d9fd6501419 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -15,6 +15,11 @@ class GroupPolicy < BasePolicy
 
   condition(:nested_groups_supported, scope: :global) { Group.supports_nested_groups? }
 
+  condition(:has_parent, scope: :subject) { @subject.has_parent? }
+  condition(:share_with_group_locked, scope: :subject) { @subject.share_with_group_lock? }
+  condition(:parent_share_with_group_locked, scope: :subject) { @subject.parent&.share_with_group_lock? }
+  condition(:can_change_parent_share_with_group_lock) { can?(:change_share_with_group_lock, @subject.parent) }
+
   condition(:has_projects) do
     GroupProjectsFinder.new(group: @subject, current_user: @user).execute.any?
   end
@@ -54,6 +59,8 @@ class GroupPolicy < BasePolicy
   rule { ~can?(:view_globally) }.prevent   :request_access
   rule { has_access }.prevent              :request_access
 
+  rule { owner & (~share_with_group_locked | ~has_parent | ~parent_share_with_group_locked | can_change_parent_share_with_group_lock) }.enable :change_share_with_group_lock
+
   def access_level
     return GroupMember::NO_ACCESS if @user.nil?
 
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]
 
diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml
index 839f23e69fd..0d3308833b7 100644
--- a/app/views/groups/edit.html.haml
+++ b/app/views/groups/edit.html.haml
@@ -28,17 +28,20 @@
         .col-sm-offset-2.col-sm-10
           = render 'shared/allow_request_access', form: f
 
-      = render 'group_admin_settings', f: f
-
       .form-group
-        %hr
-        = f.label :share_with_group_lock, class: 'control-label' do
-          Share with group lock
+        %label.control-label
+          = s_("GroupSettings|Share with group lock")
         .col-sm-10
           .checkbox
-            = f.check_box :share_with_group_lock
-            %span.descr Prevent sharing a project with another group within this group
+            = f.label :share_with_group_lock do
+              = f.check_box :share_with_group_lock, disabled: !can_change_share_with_group_lock?(@group)
+              %strong
+                - group_link = link_to @group.name, group_path(@group)
+                = s_("GroupSettings|Prevent sharing a project within %{group} with other groups").html_safe % { group: group_link }
+              %br
+              %span.descr= share_with_group_lock_help_text(@group)
 
+      = render 'group_admin_settings', f: f
 
       .form-actions
         = f.submit 'Save group', class: "btn btn-save"