Make subgroup_creation_level default to maintainer at SQL level

- Migration updates existing groups to "owner", then sets default to
  "maintainer" so that new groups will default to that
- Update spec examples

9 files changed, 61 insertions, 25 deletions

diff --git a/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb
index eed0ba25f27..85ac89af46e 100644
--- a/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb
+++ b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb
@@ -12,6 +12,7 @@ class AddGroupCreationLevelToNamespaces < ActiveRecord::Migration[5.1]
                               :subgroup_creation_level,
                               :integer,
                               default: 0)
+      change_column_default(:namespaces, :subgroup_creation_level, 1)
     end
   end
 
diff --git a/db/schema.rb b/db/schema.rb
index 29367551efb..683ee685372 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1442,7 +1442,7 @@ ActiveRecord::Schema.define(version: 20190626175626) do
     t.integer "project_creation_level"
     t.boolean "auto_devops_enabled"
     t.datetime_with_timezone "last_ci_minutes_notification_at"
-    t.integer "subgroup_creation_level", default: 0, null: false
+    t.integer "subgroup_creation_level", default: 1, null: false
     t.index ["created_at"], name: "index_namespaces_on_created_at", using: :btree
     t.index ["name", "parent_id"], name: "index_namespaces_on_name_and_parent_id", unique: true, using: :btree
     t.index ["name"], name: "index_namespaces_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"}
diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb
index 72f389513f8..398f587bafe 100644
--- a/spec/controllers/admin/groups_controller_spec.rb
+++ b/spec/controllers/admin/groups_controller_spec.rb
@@ -70,14 +70,13 @@ describe Admin::GroupsController do
     end
 
     it 'updates the subgroup_creation_level successfully' do
-      MAINTAINER = ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS
+      OWNER = ::Gitlab::Access::OWNER_SUBGROUP_ACCESS
 
       expect do
         post :update,
              params: { id: group.to_param,
-                       group: { subgroup_creation_level: MAINTAINER } }
-      end.to change { group.reload.subgroup_creation_level }
-               .to(MAINTAINER)
+                       group: { subgroup_creation_level: OWNER } }
+      end.to change { group.reload.subgroup_creation_level }.to(OWNER)
     end
   end
 end
diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb
index 2f50fbfe2fa..b67ab955ffc 100644
--- a/spec/factories/groups.rb
+++ b/spec/factories/groups.rb
@@ -5,7 +5,6 @@ FactoryBot.define do
     type 'Group'
     owner nil
     project_creation_level ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS
-    subgroup_creation_level ::Gitlab::Access::OWNER_SUBGROUP_ACCESS
 
     after(:create) do |group|
       if group.owner
@@ -46,5 +45,9 @@ FactoryBot.define do
     trait :auto_devops_disabled do
       auto_devops_enabled false
     end
+
+    trait :owner_subgroup_creation_only do
+      subgroup_creation_level ::Gitlab::Access::OWNER_SUBGROUP_ACCESS
+    end
   end
 end
diff --git a/spec/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb
index 163906010fa..48ba9064327 100644
--- a/spec/features/groups/show_spec.rb
+++ b/spec/features/groups/show_spec.rb
@@ -72,10 +72,11 @@ describe 'Group show page' do
       context 'when subgroups are supported', :js, :nested_groups do
         before do
           allow(Group).to receive(:supports_nested_objects?) { true }
-          visit path
         end
 
         it 'allows creating subgroups' do
+          visit path
+
           expect(page)
             .to have_css("li[data-text='New subgroup']", visible: false)
         end
@@ -84,10 +85,11 @@ describe 'Group show page' do
       context 'when subgroups are not supported' do
         before do
           allow(Group).to receive(:supports_nested_objects?) { false }
-          visit path
         end
 
         it 'does not allow creating subgroups' do
+          visit path
+
           expect(page)
             .not_to have_selector("li[data-text='New subgroup']", visible: false)
         end
@@ -102,10 +104,9 @@ describe 'Group show page' do
       context 'when subgroups are supported', :js, :nested_groups do
         before do
           allow(Group).to receive(:supports_nested_objects?) { true }
-          visit path
         end
 
-        context 'when subgroup_creation_level is set to maintainer' do
+        context 'when subgroup_creation_level is set to maintainers' do
           let(:group) do
             create(:group,
                    subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS)
@@ -113,7 +114,9 @@ describe 'Group show page' do
 
           it 'allows creating subgroups' do
             visit path
-            expect(page).to have_css("li[data-text='New subgroup']", visible: false)
+
+            expect(page)
+              .to have_css("li[data-text='New subgroup']", visible: false)
           end
         end
 
@@ -125,6 +128,7 @@ describe 'Group show page' do
 
           it 'does not allow creating subgroups' do
             visit path
+
             expect(page)
               .not_to have_css("li[data-text='New subgroup']", visible: false)
           end
@@ -134,10 +138,11 @@ describe 'Group show page' do
       context 'when subgroups are not supported' do
         before do
           allow(Group).to receive(:supports_nested_objects?) { false }
-          visit path
         end
 
         it 'does not allow creating subgroups' do
+          visit path
+
           expect(page)
             .not_to have_selector("li[data-text='New subgroup']", visible: false)
         end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 3a5ae14ab46..12b3bfdf015 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -1026,9 +1026,8 @@ describe Group do
 
   describe 'subgroup_creation_level' do
     it 'defaults to maintainers' do
-      group = create (:group)
-
-      expect(group.subgroup_creation_level).to eq(Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS)
+      expect(group.subgroup_creation_level)
+        .to eq(Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS)
     end
   end
 end
diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb
index da186f63eca..7fba62d2aa8 100644
--- a/spec/policies/group_policy_spec.rb
+++ b/spec/policies/group_policy_spec.rb
@@ -98,12 +98,38 @@ describe GroupPolicy do
   context 'maintainer' do
     let(:current_user) { maintainer }
 
-    it do
-      expect_allowed(*guest_permissions)
-      expect_allowed(*reporter_permissions)
-      expect_allowed(*developer_permissions)
-      expect_allowed(*maintainer_permissions)
-      expect_disallowed(*owner_permissions)
+    context 'with subgroup_creation level set to maintainer' do
+      let(:group) { create(:group,
+                           :private,
+                           subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) }
+
+      it do
+        allow(Group).to receive(:supports_nested_objects?).and_return(true)
+
+        create_subgroup_permission = [:create_subgroup]
+        updated_maintainer_permissions =
+          maintainer_permissions + create_subgroup_permission
+        updated_owner_permissions =
+          owner_permissions - create_subgroup_permission
+
+        expect_allowed(*guest_permissions)
+        expect_allowed(*reporter_permissions)
+        expect_allowed(*developer_permissions)
+        expect_allowed(*updated_maintainer_permissions)
+        expect_disallowed(*updated_owner_permissions)
+      end
+    end
+
+    context 'with subgroup_creation_level set to owner' do
+      it do
+        allow(Group).to receive(:supports_nested_objects?).and_return(true)
+
+        expect_allowed(*guest_permissions)
+        expect_allowed(*reporter_permissions)
+        expect_allowed(*developer_permissions)
+        expect_allowed(*maintainer_permissions)
+        expect_disallowed(*owner_permissions)
+      end
     end
   end
 
@@ -181,7 +207,10 @@ describe GroupPolicy do
   end
 
   describe 'private nested group use the highest access level from the group and inherited permissions', :nested_groups do
-    let(:nested_group) { create(:group, :private, parent: group) }
+    let(:nested_group) { create(:group,
+                                :private,
+                                :owner_subgroup_creation_only,
+                                parent: group) }
 
     before do
       nested_group.add_guest(guest)
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index c41408fba65..52d926d5484 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/spec/requests/api/groups_spec.rb
@@ -803,10 +803,10 @@ describe API::Groups do
           group2.add_maintainer(user1)
         end
 
-        it 'cannot create subgroups' do
+        it 'can create subgroups' do
           post api("/groups", user1), params: { parent_id: group2.id, name: 'foo', path: 'foo' }
 
-          expect(response).to have_gitlab_http_status(403)
+          expect(response).to have_gitlab_http_status(201)
         end
       end
     end
diff --git a/spec/support/shared_contexts/policies/group_policy_shared_context.rb b/spec/support/shared_contexts/policies/group_policy_shared_context.rb
index b4808ac0068..74389c4d82b 100644
--- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb
+++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb
@@ -7,7 +7,7 @@ RSpec.shared_context 'GroupPolicy context' do
   let(:maintainer) { create(:user) }
   let(:owner) { create(:user) }
   let(:admin) { create(:admin) }
-  let(:group) { create(:group, :private) }
+  let(:group) { create(:group, :private, :owner_subgroup_creation_only) }
 
   let(:guest_permissions) do
     %i[