Validate nature of parent for Group & Namespace

This change adds validation to check the nature
of parent for Group & Namespace records

app/models/namespace.rb

@@ -63,6 +63,7 @@ class Namespace < ApplicationRecord
 
   validates :max_artifacts_size, numericality: { only_integer: true, greater_than: 0, allow_nil: true }
 
+  validate :validate_parent_type, if: -> { Feature.enabled?(:validate_namespace_parent_type) }
   validate :nesting_level_allowed
   validate :changing_shared_runners_enabled_is_allowed
   validate :changing_allow_descendants_override_disabled_shared_runners_is_allowed
@@ -448,6 +449,16 @@ class Namespace < ApplicationRecord
     end
   end
 
+  def validate_parent_type
+    return unless has_parent?
+
+    if user?
+      errors.add(:parent_id, 'a user namespace cannot have a parent')
+    elsif group?
+      errors.add(:parent_id, 'a group cannot have a user namespace as its parent') if parent.user?
+    end
+  end
+
   def sync_share_with_group_lock_with_parent
     if parent&.share_with_group_lock?
       self.share_with_group_lock = true

config/feature_flags/development/validate_namespace_parent_type.yml

+---
+name: validate_namespace_parent_type
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/54094
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/322101
+milestone: '13.10'
+type: development
+group: group::access
+default_enabled: false

ee/app/models/ee/group.rb

@@ -485,7 +485,6 @@ module EE
 
     def execute_subgroup_hooks(event)
       return unless subgroup?
-      return unless parent.group? # TODO: Remove this after fixing https://gitlab.com/gitlab-org/gitlab/-/issues/301013
       return unless feature_available?(:group_webhooks)
 
       run_after_commit do

ee/spec/models/application_setting_spec.rb

@@ -368,24 +368,31 @@ RSpec.describe ApplicationSetting do
       end
 
       context 'namespaces' do
-        let(:namespaces) { create_list(:namespace, 2) }
-        let!(:indexed_namespace) { create :elasticsearch_indexed_namespace, namespace: namespaces.last }
+        context 'with personal namespaces' do
+          let(:namespaces) { create_list(:namespace, 2) }
+          let!(:indexed_namespace) { create :elasticsearch_indexed_namespace, namespace: namespaces.last }
 
-        it 'tells you if a namespace is allowed to be indexed' do
-          expect(setting.elasticsearch_indexes_namespace?(namespaces.last)).to be_truthy
-          expect(setting.elasticsearch_indexes_namespace?(namespaces.first)).to be_falsey
+          it 'tells you if a namespace is allowed to be indexed' do
+            expect(setting.elasticsearch_indexes_namespace?(namespaces.last)).to be_truthy
+            expect(setting.elasticsearch_indexes_namespace?(namespaces.first)).to be_falsey
+          end
         end
 
-        it 'returns namespaces that are allowed to be indexed' do
-          child_namespace = create(:namespace, parent: namespaces.first)
-          create :elasticsearch_indexed_namespace, namespace: child_namespace
+        context 'with groups' do
+          let(:groups) { create_list(:group, 2) }
+          let!(:indexed_namespace) { create :elasticsearch_indexed_namespace, namespace: groups.last }
+
+          it 'returns groups that are allowed to be indexed' do
+            child_group = create(:group, parent: groups.first)
+            create :elasticsearch_indexed_namespace, namespace: child_group
 
-          child_namespace_indexed_through_parent = create(:namespace, parent: namespaces.last)
+            child_group_indexed_through_parent = create(:group, parent: groups.last)
 
-          expect(setting.elasticsearch_limited_namespaces).to match_array(
-            [namespaces.last, child_namespace, child_namespace_indexed_through_parent])
-          expect(setting.elasticsearch_limited_namespaces(true)).to match_array(
-            [namespaces.last, child_namespace])
+            expect(setting.elasticsearch_limited_namespaces).to match_array(
+              [groups.last, child_group, child_group_indexed_through_parent])
+            expect(setting.elasticsearch_limited_namespaces(true)).to match_array(
+              [groups.last, child_group])
+          end
         end
 
         describe '#elasticsearch_indexes_project?' do

ee/spec/services/compliance_management/frameworks/create_service_spec.rb

@@ -34,17 +34,22 @@ RSpec.describe ComplianceManagement::Frameworks::CreateService do
     end
 
     context 'namespace has a parent' do
-      let_it_be_with_reload(:namespace) { create(:namespace, :with_hierarchy) }
-      let(:descendant) { namespace.descendants.first }
+      let_it_be(:user) { create(:user) }
+      let_it_be_with_reload(:group) { create(:group, :with_hierarchy) }
+      let(:descendant) { group.descendants.first }
 
-      subject { described_class.new(namespace: descendant, params: params, current_user: namespace.owner) }
+      before do
+        group.add_owner(user)
+      end
+
+      subject { described_class.new(namespace: descendant, params: params, current_user: user) }
 
       it 'responds with a successful service response' do
         expect(subject.execute.success?).to be true
       end
 
       it 'creates the new framework in the root namespace' do
-        expect(subject.execute.payload[:framework].namespace).to eq(namespace)
+        expect(subject.execute.payload[:framework].namespace).to eq(group)
       end
     end
 

ee/spec/services/projects/update_pages_service_spec.rb

@@ -3,9 +3,8 @@
 require "spec_helper"
 
 RSpec.describe Projects::UpdatePagesService do
-  let(:root_namespace) { create(:namespace, max_pages_size: 300) }
-  let(:namespace) { create(:namespace, parent: root_namespace, max_pages_size: 200) }
-  let(:project) { create(:project, :repository, namespace: namespace, max_pages_size: 250) }
+  let(:group) { create(:group, :nested, max_pages_size: 200) }
+  let(:project) { create(:project, :repository, namespace: group, max_pages_size: 250) }
   let(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit('HEAD').sha) }
   let(:build) { create(:ci_build, pipeline: pipeline, ref: 'HEAD') }
 

spec/factories/groups.rb

@@ -61,5 +61,35 @@ FactoryBot.define do
     trait :allow_descendants_override_disabled_shared_runners do
       allow_descendants_override_disabled_shared_runners { true }
     end
+
+    # Construct a hierarchy underneath the group.
+    # Each group will have `children` amount of children,
+    # and `depth` levels of descendants.
+    trait :with_hierarchy do
+      transient do
+        children { 4 }
+        depth    { 4 }
+      end
+
+      after(:create) do |group, evaluator|
+        def create_graph(parent: nil, children: 4, depth: 4)
+          return unless depth > 1
+
+          children.times do
+            factory_name = parent.model_name.singular
+            child = FactoryBot.create(factory_name, parent: parent)
+            create_graph(parent: child, children: children, depth: depth - 1)
+          end
+
+          parent
+        end
+
+        create_graph(
+          parent:   group,
+          children: evaluator.children,
+          depth:    evaluator.depth
+        )
+      end
+    end
   end
 end

spec/factories/namespaces.rb

@@ -34,36 +34,6 @@ FactoryBot.define do
       association :namespace_settings, factory: :namespace_settings
     end
 
-    # Construct a hierarchy underneath the namespace.
-    # Each namespace will have `children` amount of children,
-    # and `depth` levels of descendants.
-    trait :with_hierarchy do
-      transient do
-        children { 4 }
-        depth    { 4 }
-      end
-
-      after(:create) do |namespace, evaluator|
-        def create_graph(parent: nil, children: 4, depth: 4)
-          return unless depth > 1
-
-          children.times do
-            factory_name = parent.model_name.singular
-            child = FactoryBot.create(factory_name, parent: parent)
-            create_graph(parent: child, children: children, depth: depth - 1)
-          end
-
-          parent
-        end
-
-        create_graph(
-          parent:   namespace,
-          children: evaluator.children,
-          depth:    evaluator.depth
-        )
-      end
-    end
-
     trait :shared_runners_disabled do
       shared_runners_enabled { false }
     end

spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb

@@ -81,7 +81,7 @@ RSpec.describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase, :
   end
 
   describe '#rename_path_for_routable' do
-    context 'for namespaces' do
+    context 'for personal namespaces' do
       let(:namespace) { create(:namespace, path: 'the-path') }
 
       it "renames namespaces called the-path" do
@@ -119,13 +119,16 @@ RSpec.describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase, :
 
         expect(project.route.reload.path).to eq('the-path-but-not-really/the-project')
       end
+    end
 
-      context "the-path namespace -> subgroup -> the-path0 project" do
+    context 'for groups' do
+      context "the-path group -> subgroup -> the-path0 project" do
         it "updates the route of the project correctly" do
-          subgroup = create(:group, path: "subgroup", parent: namespace)
+          group = create(:group, path: 'the-path')
+          subgroup = create(:group, path: "subgroup", parent: group)
           project = create(:project, :repository, path: "the-path0", namespace: subgroup)
 
-          subject.rename_path_for_routable(migration_namespace(namespace))
+          subject.rename_path_for_routable(migration_namespace(group))
 
           expect(project.route.reload.path).to eq("the-path0/subgroup/the-path0")
         end
@@ -158,23 +161,27 @@ RSpec.describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase, :
   end
 
   describe '#perform_rename' do
-    describe 'for namespaces' do
-      let(:namespace) { create(:namespace, path: 'the-path') }
-
+    context 'for personal namespaces' do
       it 'renames the path' do
+        namespace = create(:namespace, path: 'the-path')
+
         subject.perform_rename(migration_namespace(namespace), 'the-path', 'renamed')
 
         expect(namespace.reload.path).to eq('renamed')
+        expect(namespace.reload.route.path).to eq('renamed')
       end
+    end
 
-      it 'renames all the routes for the namespace' do
-        child = create(:group, path: 'child', parent: namespace)
+    context 'for groups' do
+      it 'renames all the routes for the group' do
+        group = create(:group, path: 'the-path')
+        child = create(:group, path: 'child', parent: group)
         project = create(:project, :repository, namespace: child, path: 'the-project')
-        other_one = create(:namespace, path: 'the-path-is-similar')
+        other_one = create(:group, path: 'the-path-is-similar')
 
-        subject.perform_rename(migration_namespace(namespace), 'the-path', 'renamed')
+        subject.perform_rename(migration_namespace(group), 'the-path', 'renamed')
 
-        expect(namespace.reload.route.path).to eq('renamed')
+        expect(group.reload.route.path).to eq('renamed')
         expect(child.reload.route.path).to eq('renamed/child')
         expect(project.reload.route.path).to eq('renamed/child/the-project')
         expect(other_one.reload.route.path).to eq('the-path-is-similar')

spec/models/group_spec.rb

@@ -64,6 +64,59 @@ RSpec.describe Group do
     it { is_expected.to validate_presence_of :two_factor_grace_period }
     it { is_expected.to validate_numericality_of(:two_factor_grace_period).is_greater_than_or_equal_to(0) }
 
+    context 'validating the parent of a group' do
+      context 'when the group has no parent' do
+        it 'allows a group to have no parent associated with it' do
+          group = build(:group)
+
+          expect(group).to be_valid
+        end
+      end
+
+      context 'when the group has a parent' do
+        it 'does not allow a group to have a namespace as its parent' do
+          group = build(:group, parent: build(:namespace))
+
+          expect(group).not_to be_valid
+          expect(group.errors[:parent_id].first).to eq('a group cannot have a user namespace as its parent')
+        end
+
+        it 'allows a group to have another group as its parent' do
+          group = build(:group, parent: build(:group))
+
+          expect(group).to be_valid
+        end
+      end
+
+      context 'when the feature flag `validate_namespace_parent_type` is disabled' do
+        before do
+          stub_feature_flags(validate_namespace_parent_type: false)
+        end
+
+        context 'when the group has no parent' do
+          it 'allows a group to have no parent associated with it' do
+            group = build(:group)
+
+            expect(group).to be_valid
+          end
+        end
+
+        context 'when the group has a parent' do
+          it 'allows a group to have a namespace as its parent' do
+            group = build(:group, parent: build(:namespace))
+
+            expect(group).to be_valid
+          end
+
+          it 'allows a group to have another group as its parent' do
+            group = build(:group, parent: build(:group))
+
+            expect(group).to be_valid
+          end
+        end
+      end
+    end
+
     describe 'path validation' do
       it 'rejects paths reserved on the root namespace when the group has no parent' do
         group = build(:group, path: 'api')

spec/models/namespace/traversal_hierarchy_spec.rb

@@ -3,41 +3,41 @@
 require 'spec_helper'
 
 RSpec.describe Namespace::TraversalHierarchy, type: :model do
-  let_it_be(:root, reload: true) { create(:namespace, :with_hierarchy) }
+  let_it_be(:root, reload: true) { create(:group, :with_hierarchy) }
 
   describe '.for_namespace' do
-    let(:hierarchy) { described_class.for_namespace(namespace) }
+    let(:hierarchy) { described_class.for_namespace(group) }
 
     context 'with root group' do
-      let(:namespace) { root }
+      let(:group) { root }
 
       it { expect(hierarchy.root).to eq root }
     end
 
     context 'with child group' do
-      let(:namespace) { root.children.first.children.first }
+      let(:group) { root.children.first.children.first }
 
       it { expect(hierarchy.root).to eq root }
     end
 
     context 'with group outside of hierarchy' do
-      let(:namespace) { create(:namespace) }
+      let(:group) { create(:namespace) }
 
       it { expect(hierarchy.root).not_to eq root }
     end
   end
 
   describe '.new' do
-    let(:hierarchy) { described_class.new(namespace) }
+    let(:hierarchy) { described_class.new(group) }
 
     context 'with root group' do
-      let(:namespace) { root }
+      let(:group) { root }
 
       it { expect(hierarchy.root).to eq root }
     end
 
     context 'with child group' do
-      let(:namespace) { root.children.first }
+      let(:group) { root.children.first }
 
       it { expect { hierarchy }.to raise_error(StandardError, 'Must specify a root node') }
     end

spec/models/onboarding_progress_spec.rb

@@ -106,7 +106,7 @@ RSpec.describe OnboardingProgress do
     end
 
     context 'when not given a root namespace' do
-      let(:namespace) { create(:namespace, parent: build(:namespace)) }
+      let(:namespace) { create(:group, parent: build(:group)) }
 
       it 'does not add a record for the namespace' do
         expect { onboard }.not_to change(described_class, :count).from(0)

spec/models/project_spec.rb

@@ -145,7 +145,7 @@ RSpec.describe Project, factory_default: :keep do
     end
 
     it_behaves_like 'model with wiki' do
-      let_it_be(:container) { create(:project, :wiki_repo) }
+      let_it_be(:container) { create(:project, :wiki_repo, namespace: create(:group)) }
       let(:container_without_wiki) { create(:project) }
     end
 

spec/requests/api/ci/runner/jobs_artifacts_spec.rb

@@ -17,9 +17,9 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
   end
 
   describe '/api/v4/jobs' do
-    let(:root_namespace) { create(:namespace) }
-    let(:namespace) { create(:namespace, parent: root_namespace) }
-    let(:project) { create(:project, namespace: namespace, shared_runners_enabled: false) }
+    let(:parent_group) { create(:group) }
+    let(:group) { create(:group, parent: parent_group) }
+    let(:project) { create(:project, namespace: group, shared_runners_enabled: false) }
     let(:pipeline) { create(:ci_pipeline, project: project, ref: 'master') }
     let(:runner) { create(:ci_runner, :project, projects: [project]) }
     let(:user) { create(:user) }
@@ -78,7 +78,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
 
           before do
             stub_application_setting(max_artifacts_size: application_max_size)
-            root_namespace.update!(max_artifacts_size: sample_max_size)
+            parent_group.update!(max_artifacts_size: sample_max_size)
           end
 
           it_behaves_like 'failed request'
@@ -90,8 +90,8 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
 
           before do
             stub_application_setting(max_artifacts_size: application_max_size)
-            root_namespace.update!(max_artifacts_size: root_namespace_max_size)
-            namespace.update!(max_artifacts_size: sample_max_size)
+            parent_group.update!(max_artifacts_size: root_namespace_max_size)
+            group.update!(max_artifacts_size: sample_max_size)
           end
 
           it_behaves_like 'failed request'
@@ -104,8 +104,8 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
 
           before do
             stub_application_setting(max_artifacts_size: application_max_size)
-            root_namespace.update!(max_artifacts_size: root_namespace_max_size)
-            namespace.update!(max_artifacts_size: child_namespace_max_size)
+            parent_group.update!(max_artifacts_size: root_namespace_max_size)
+            group.update!(max_artifacts_size: child_namespace_max_size)
             project.update!(max_artifacts_size: sample_max_size)
           end
 

spec/requests/api/ci/runner/jobs_put_spec.rb

@@ -17,9 +17,8 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
   end
 
   describe '/api/v4/jobs' do
-    let(:root_namespace) { create(:namespace) }
-    let(:namespace) { create(:namespace, parent: root_namespace) }
-    let(:project) { create(:project, namespace: namespace, shared_runners_enabled: false) }
+    let(:group) { create(:group, :nested) }
+    let(:project) { create(:project, namespace: group, shared_runners_enabled: false) }
     let(:pipeline) { create(:ci_pipeline, project: project, ref: 'master') }
     let(:runner) { create(:ci_runner, :project, projects: [project]) }
     let(:user) { create(:user) }

spec/requests/api/ci/runner/jobs_request_post_spec.rb

@@ -17,9 +17,8 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
   end
 
   describe '/api/v4/jobs' do
-    let(:root_namespace) { create(:namespace) }
-    let(:namespace) { create(:namespace, parent: root_namespace) }
-    let(:project) { create(:project, namespace: namespace, shared_runners_enabled: false) }
+    let(:group) { create(:group, :nested) }
+    let(:project) { create(:project, namespace: group, shared_runners_enabled: false) }
     let(:pipeline) { create(:ci_pipeline, project: project, ref: 'master') }
     let(:runner) { create(:ci_runner, :project, projects: [project]) }
     let(:user) { create(:user) }

spec/requests/api/ci/runner/jobs_trace_spec.rb

@@ -17,9 +17,8 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
   end
 
   describe '/api/v4/jobs' do
-    let(:root_namespace) { create(:namespace) }
-    let(:namespace) { create(:namespace, parent: root_namespace) }
-    let(:project) { create(:project, namespace: namespace, shared_runners_enabled: false) }
+    let(:group) { create(:group, :nested) }
+    let(:project) { create(:project, namespace: group, shared_runners_enabled: false) }
     let(:pipeline) { create(:ci_pipeline, project: project, ref: 'master') }
     let(:runner) { create(:ci_runner, :project, projects: [project]) }
     let(:user) { create(:user) }

spec/services/onboarding_progress_service_spec.rb

@@ -45,8 +45,7 @@ RSpec.describe OnboardingProgressService do
   end
 
   describe '#execute' do
-    let(:namespace) { create(:namespace, parent: root_namespace) }
-    let(:root_namespace) { nil }
+    let(:namespace) { create(:namespace) }
     let(:action) { :namespace_action }
 
     subject(:execute_service) { described_class.new(namespace).execute(action: :subscription_created) }
@@ -64,16 +63,16 @@ RSpec.describe OnboardingProgressService do
     end
 
     context 'when the namespace is not the root' do
-      let(:root_namespace) { build(:namespace) }
+      let(:group) { create(:group, :nested) }
 
       before do
-        OnboardingProgress.onboard(root_namespace)
+        OnboardingProgress.onboard(group)
       end
 
-      it 'registers a namespace onboarding progress action for the root namespace' do
+      it 'does not register a namespace onboarding progress action' do
         execute_service
 
-        expect(OnboardingProgress.completed?(root_namespace, :subscription_created)).to eq(true)
+        expect(OnboardingProgress.completed?(group, :subscription_created)).to be(nil)
       end
     end
 
@@ -83,7 +82,7 @@ RSpec.describe OnboardingProgressService do
       it 'does not register a namespace onboarding progress action' do
         execute_service
 
-        expect(OnboardingProgress.completed?(root_namespace, :subscription_created)).to be(nil)
+        expect(OnboardingProgress.completed?(namespace, :subscription_created)).to be(nil)
       end
     end
   end