Make Members with Owner and Master roles always able to create subgroups

3 files changed, 40 insertions, 22 deletions

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 994e736d66e..3769a2cde33 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -10,7 +10,7 @@ class GroupsController < Groups::ApplicationController
 
   # Authorize
   before_action :authorize_admin_group!, only: [:edit, :update, :destroy, :projects]
-  before_action :authorize_create_group!, only: [:new, :create]
+  before_action :authorize_create_group!, only: [:new]
 
   before_action :group_projects, only: [:projects, :activity, :issues, :merge_requests]
   before_action :group_merge_requests, only: [:merge_requests]
@@ -25,14 +25,7 @@ class GroupsController < Groups::ApplicationController
   end
 
   def new
-    @group = Group.new
-
-    if params[:parent_id].present?
-      parent = Group.find_by(id: params[:parent_id])
-      if can?(current_user, :create_subgroup, parent)
-        @group.parent = parent
-      end
-    end
+    @group = Group.new(params.permit(:parent_id))
   end
 
   def create
@@ -128,9 +121,14 @@ class GroupsController < Groups::ApplicationController
   end
 
   def authorize_create_group!
-    unless can?(current_user, :create_group)
-      return render_404
-    end
+    allowed = if params[:parent_id].present?
+                parent = Group.find_by(id: params[:parent_id])
+                can?(current_user, :create_subgroup, parent)
+              else
+                can?(current_user, :create_group)
+              end
+
+    render_404 unless allowed
   end
 
   def determine_layout
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index d9fd6501419..420991ff6d6 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -49,7 +49,7 @@ class GroupPolicy < BasePolicy
     enable :change_visibility_level
   end
 
-  rule { owner & can_create_group & nested_groups_supported }.enable :create_subgroup
+  rule { owner & nested_groups_supported }.enable :create_subgroup
 
   rule { public_group | logged_in_viewable }.enable :view_globally
 
diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb
index c7c27621085..70e50aa0f12 100644
--- a/app/services/groups/create_service.rb
+++ b/app/services/groups/create_service.rb
@@ -8,15 +8,7 @@ module Groups
     def execute
       @group = Group.new(params)
 
-      unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
-        deny_visibility_level(@group)
-        return @group
-      end
-
-      if @group.parent && !can?(current_user, :create_subgroup, @group.parent)
-        @group.parent = nil
-        @group.errors.add(:parent_id, 'You don’t have permission to create a subgroup in this group.')
-
+      unless can_use_visibility_level? && can_create_group?
         return @group
       end
 
@@ -39,5 +31,33 @@ module Groups
     def create_chat_team?
       Gitlab.config.mattermost.enabled && @chat_team && group.chat_team.nil?
     end
+
+    def can_create_group?
+      if @group.subgroup?
+        unless can?(current_user, :create_subgroup, @group.parent)
+          @group.parent = nil
+          @group.errors.add(:parent_id, 'You don’t have permission to create a subgroup in this group.')
+
+          return false
+        end
+      else
+        unless can?(current_user, :create_group)
+          @group.errors.add(:base, 'You don’t have permission to create groups.')
+
+          return false
+        end
+      end
+
+      true
+    end
+
+    def can_use_visibility_level?
+      unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
+        deny_visibility_level(@group)
+        return false
+      end
+
+      true
+    end
   end
 end