Merge branch 'bojan/add-new-group-policy' into 'master'

Hide `New subgroup` button if visibility is restricted

See merge request !148758



Merged-by: Manoj M J <mmj@gitlab.com>
Approved-by: Thong Kuah <tkuah@gitlab.com>
Approved-by: Manoj M J <mmj@gitlab.com>
Approved-by: Jarka Košanová <jarka@gitlab.com>
Reviewed-by: Jarka Košanová <jarka@gitlab.com>
Reviewed-by: Bojan Marjanovic <bmarjanovic@gitlab.com>
Co-authored-by: bmarjanovic <bmarjanovic@gitlab.com>

app/policies/group_policy.rb

@@ -61,6 +61,24 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
     @subject.project_creation_level == ::Gitlab::Access::NO_ONE_PROJECT_ACCESS || allowed_visibility_levels.empty?
   end
 
+  condition(:create_subgroup_disabled, scope: :subject) do
+    next true if @user.nil?
+
+    visibility_levels = if @user.can_admin_all_resources?
+                          # admin can create groups even with restricted visibility levels
+                          Gitlab::VisibilityLevel.values
+                        else
+                          Gitlab::VisibilityLevel.allowed_levels
+                        end
+
+    # visibility_level_allowed? is not supporting root-groups, so we have to create a dummy sub-group.
+    subgroup = Group.new(parent_id: @subject.id)
+
+    # if a subgroup with none of the remaining visibility levels can be allowed by the group,
+    # then it means that the `Create subgroup` button must be disabled.
+    visibility_levels.none? { |level| subgroup.visibility_level_allowed?(level) }
+  end
+
   condition(:developer_maintainer_access, scope: :subject) do
     @subject.project_creation_level == ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS
   end
@@ -312,6 +330,10 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
     prevent :import_projects
   end
 
+  rule { create_subgroup_disabled }.policy do
+    prevent :create_subgroup
+  end
+
   rule { owner | admin | organization_owner }.policy do
     enable :owner_access
     enable :read_statistics

spec/controllers/groups_controller_spec.rb

@@ -128,6 +128,33 @@
 
   describe 'GET #new' do
     context 'when creating subgroups' do
+      context 'when user does not have `:create_subgroup` permissions' do
+        before do
+          sign_in(user)
+          allow(controller).to receive(:can?).with(user, :create_subgroup, group).and_return(false)
+        end
+
+        it 'returns a 404' do
+          get :new, params: { parent_id: group.id }
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+
+      context 'when user has `:create_subgroup` permissions' do
+        before do
+          sign_in(user)
+          allow(controller).to receive(:can?).with(user, :create_subgroup, group).and_return(true)
+        end
+
+        it 'renders `new` template' do
+          get :new, params: { parent_id: group.id }
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(response).to render_template(:new)
+        end
+      end
+
       [true, false].each do |can_create_group_status|
         context "and can_create_group is #{can_create_group_status}" do
           before do

spec/features/groups_spec.rb

@@ -582,6 +582,14 @@
             expect(page).not_to have_link('New project')
           end
         end
+
+        it 'does not display the "New subgroup" button' do
+          visit group_path(group)
+
+          within_testid 'group-buttons' do
+            expect(page).not_to have_link('New subgroup')
+          end
+        end
       end
     end
   end

spec/policies/group_policy_spec.rb

@@ -690,31 +690,31 @@
       let_it_be(:private) { Gitlab::VisibilityLevel::PRIVATE }
       let_it_be(:policy) { :create_projects }
 
-      where(:restricted_visibility_levels, :group_visibility, :can_create_project?) do
-        []                                            | ref(:public)   | true
-        []                                            | ref(:internal) | true
-        []                                            | ref(:private)  | true
-        [ref(:public)]                                | ref(:public)   | true
-        [ref(:public)]                                | ref(:internal) | true
-        [ref(:public)]                                | ref(:private)  | true
-        [ref(:internal)]                              | ref(:public)   | true
-        [ref(:internal)]                              | ref(:internal) | true
-        [ref(:internal)]                              | ref(:private)  | true
-        [ref(:private)]                               | ref(:public)   | true
-        [ref(:private)]                               | ref(:internal) | true
-        [ref(:private)]                               | ref(:private)  | false
-        [ref(:public), ref(:internal)]                | ref(:public)   | true
-        [ref(:public), ref(:internal)]                | ref(:internal) | true
-        [ref(:public), ref(:internal)]                | ref(:private)  | true
-        [ref(:public), ref(:private)]                 | ref(:public)   | true
-        [ref(:public), ref(:private)]                 | ref(:internal) | true
-        [ref(:public), ref(:private)]                 | ref(:private)  | false
-        [ref(:private), ref(:internal)]               | ref(:public)   | true
-        [ref(:private), ref(:internal)]               | ref(:internal) | false
-        [ref(:private), ref(:internal)]               | ref(:private)  | false
-        [ref(:public), ref(:internal), ref(:private)] | ref(:public)   | false
-        [ref(:public), ref(:internal), ref(:private)] | ref(:internal) | false
-        [ref(:public), ref(:internal), ref(:private)] | ref(:private)  | false
+      where(:restricted_visibility_levels, :group_visibility, :can_create_project?, :can_create_subgroups?) do
+        []                                            | ref(:public)   | true  | true
+        []                                            | ref(:internal) | true  | true
+        []                                            | ref(:private)  | true  | true
+        [ref(:public)]                                | ref(:public)   | true  | true
+        [ref(:public)]                                | ref(:internal) | true  | true
+        [ref(:public)]                                | ref(:private)  | true  | true
+        [ref(:internal)]                              | ref(:public)   | true  | true
+        [ref(:internal)]                              | ref(:internal) | true  | true
+        [ref(:internal)]                              | ref(:private)  | true  | true
+        [ref(:private)]                               | ref(:public)   | true  | true
+        [ref(:private)]                               | ref(:internal) | true  | true
+        [ref(:private)]                               | ref(:private)  | false | false
+        [ref(:public), ref(:internal)]                | ref(:public)   | true  | true
+        [ref(:public), ref(:internal)]                | ref(:internal) | true  | true
+        [ref(:public), ref(:internal)]                | ref(:private)  | true  | true
+        [ref(:public), ref(:private)]                 | ref(:public)   | true  | true
+        [ref(:public), ref(:private)]                 | ref(:internal) | true  | true
+        [ref(:public), ref(:private)]                 | ref(:private)  | false | false
+        [ref(:private), ref(:internal)]               | ref(:public)   | true  | true
+        [ref(:private), ref(:internal)]               | ref(:internal) | false | false
+        [ref(:private), ref(:internal)]               | ref(:private)  | false | false
+        [ref(:public), ref(:internal), ref(:private)] | ref(:public)   | false | false
+        [ref(:public), ref(:internal), ref(:private)] | ref(:internal) | false | false
+        [ref(:public), ref(:internal), ref(:private)] | ref(:private)  | false | false
       end
 
       with_them do
@@ -726,6 +726,7 @@
         context 'with non-admin user' do
           let(:current_user) { owner }
 
+          it { is_expected.to(can_create_subgroups? ? be_allowed(:create_subgroup) : be_disallowed(:create_subgroup)) }
           it { is_expected.to(can_create_project? ? be_allowed(policy) : be_disallowed(policy)) }
         end