Merge branch 'rework-authorizations-performance' into 'master'

Rework project authorizations and nested groups for better performance

See merge request !10885

7 files changed, 124 insertions, 186 deletions

diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb
index 49a4132f763..0e10d91836d 100644
--- a/spec/models/concerns/routable_spec.rb
+++ b/spec/models/concerns/routable_spec.rb
@@ -115,123 +115,6 @@ describe Group, 'Routable' do
     end
   end
 
-  describe '.member_descendants' do
-    let!(:user) { create(:user) }
-    let!(:nested_group) { create(:group, parent: group) }
-
-    before { group.add_owner(user) }
-    subject { described_class.member_descendants(user.id) }
-
-    it { is_expected.to eq([nested_group]) }
-  end
-
-  describe '.member_self_and_descendants' do
-    let!(:user) { create(:user) }
-    let!(:nested_group) { create(:group, parent: group) }
-
-    before { group.add_owner(user) }
-    subject { described_class.member_self_and_descendants(user.id) }
-
-    it { is_expected.to match_array [group, nested_group] }
-  end
-
-  describe '.member_hierarchy' do
-    # foo/bar would also match foo/barbaz instead of just foo/bar and foo/bar/baz
-    let!(:user) { create(:user) }
-
-    #                group
-    #        _______ (foo) _______
-    #       |                     |
-    #       |                     |
-    # nested_group_1        nested_group_2
-    # (bar)                 (barbaz)
-    #       |                     |
-    #       |                     |
-    # nested_group_1_1      nested_group_2_1
-    # (baz)                 (baz)
-    #
-    let!(:nested_group_1) { create :group, parent: group, name: 'bar' }
-    let!(:nested_group_1_1) { create :group, parent: nested_group_1, name: 'baz' }
-    let!(:nested_group_2) { create :group, parent: group, name: 'barbaz' }
-    let!(:nested_group_2_1) { create :group, parent: nested_group_2, name: 'baz' }
-
-    context 'user is not a member of any group' do
-      subject { described_class.member_hierarchy(user.id) }
-
-      it 'returns an empty array' do
-        is_expected.to eq []
-      end
-    end
-
-    context 'user is member of all groups' do
-      before do
-        group.add_owner(user)
-        nested_group_1.add_owner(user)
-        nested_group_1_1.add_owner(user)
-        nested_group_2.add_owner(user)
-        nested_group_2_1.add_owner(user)
-      end
-      subject { described_class.member_hierarchy(user.id) }
-
-      it 'returns all groups' do
-        is_expected.to match_array [
-          group,
-          nested_group_1, nested_group_1_1,
-          nested_group_2, nested_group_2_1
-        ]
-      end
-    end
-
-    context 'user is member of the top group' do
-      before { group.add_owner(user) }
-      subject { described_class.member_hierarchy(user.id) }
-
-      it 'returns all groups' do
-        is_expected.to match_array [
-          group,
-          nested_group_1, nested_group_1_1,
-          nested_group_2, nested_group_2_1
-        ]
-      end
-    end
-
-    context 'user is member of the first child (internal node), branch 1' do
-      before { nested_group_1.add_owner(user) }
-      subject { described_class.member_hierarchy(user.id) }
-
-      it 'returns the groups in the hierarchy' do
-        is_expected.to match_array [
-          group,
-          nested_group_1, nested_group_1_1
-        ]
-      end
-    end
-
-    context 'user is member of the first child (internal node), branch 2' do
-      before { nested_group_2.add_owner(user) }
-      subject { described_class.member_hierarchy(user.id) }
-
-      it 'returns the groups in the hierarchy' do
-        is_expected.to match_array [
-          group,
-          nested_group_2, nested_group_2_1
-        ]
-      end
-    end
-
-    context 'user is member of the last child (leaf node)' do
-      before { nested_group_1_1.add_owner(user) }
-      subject { described_class.member_hierarchy(user.id) }
-
-      it 'returns the groups in the hierarchy' do
-        is_expected.to match_array [
-          group,
-          nested_group_1, nested_group_1_1
-        ]
-      end
-    end
-  end
-
   describe '#full_path' do
     let(:group) { create(:group) }
     let(:nested_group) { create(:group, parent: group) }
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 6ca1eb0374d..316bf153660 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -340,7 +340,7 @@ describe Group, models: true do
     it { expect(subject.parent).to be_kind_of(Group) }
   end
 
-  describe '#members_with_parents' do
+  describe '#members_with_parents', :nested_groups do
     let!(:group) { create(:group, :nested) }
     let!(:master) { group.parent.add_user(create(:user), GroupMember::MASTER) }
     let!(:developer) { group.add_user(create(:user), GroupMember::DEVELOPER) }
diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb
index 87ea2e70680..cf9c701e8c5 100644
--- a/spec/models/members/project_member_spec.rb
+++ b/spec/models/members/project_member_spec.rb
@@ -22,16 +22,15 @@ describe ProjectMember, models: true do
   end
 
   describe '.add_user' do
-    context 'when called with the project owner' do
-      it 'adds the user as a member' do
-        project = create(:empty_project)
+    it 'adds the user as a member' do
+      user = create(:user)
+      project = create(:empty_project)
 
-        expect(project.users).not_to include(project.owner)
+      expect(project.users).not_to include(user)
 
-        described_class.add_user(project, project.owner, :master, current_user: project.owner)
+      described_class.add_user(project, user, :master, current_user: project.owner)
 
-        expect(project.users.reload).to include(project.owner)
-      end
+      expect(project.users.reload).to include(user)
     end
   end
 
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index ff5e7c350aa..0e74f1ab1bd 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -287,21 +287,21 @@ describe Namespace, models: true do
     end
   end
 
-  describe '#ancestors' do
+  describe '#ancestors', :nested_groups do
     let(:group) { create(:group) }
     let(:nested_group) { create(:group, parent: group) }
     let(:deep_nested_group) { create(:group, parent: nested_group) }
     let(:very_deep_nested_group) { create(:group, parent: deep_nested_group) }
 
     it 'returns the correct ancestors' do
-      expect(very_deep_nested_group.ancestors).to eq([group, nested_group, deep_nested_group])
-      expect(deep_nested_group.ancestors).to eq([group, nested_group])
-      expect(nested_group.ancestors).to eq([group])
+      expect(very_deep_nested_group.ancestors).to include(group, nested_group, deep_nested_group)
+      expect(deep_nested_group.ancestors).to include(group, nested_group)
+      expect(nested_group.ancestors).to include(group)
       expect(group.ancestors).to eq([])
     end
   end
 
-  describe '#descendants' do
+  describe '#descendants', :nested_groups do
     let!(:group) { create(:group, path: 'git_lab') }
     let!(:nested_group) { create(:group, parent: group) }
     let!(:deep_nested_group) { create(:group, parent: nested_group) }
@@ -311,9 +311,9 @@ describe Namespace, models: true do
 
     it 'returns the correct descendants' do
       expect(very_deep_nested_group.descendants.to_a).to eq([])
-      expect(deep_nested_group.descendants.to_a).to eq([very_deep_nested_group])
-      expect(nested_group.descendants.to_a).to eq([deep_nested_group, very_deep_nested_group])
-      expect(group.descendants.to_a).to eq([nested_group, deep_nested_group, very_deep_nested_group])
+      expect(deep_nested_group.descendants.to_a).to include(very_deep_nested_group)
+      expect(nested_group.descendants.to_a).to include(deep_nested_group, very_deep_nested_group)
+      expect(group.descendants.to_a).to include(nested_group, deep_nested_group, very_deep_nested_group)
     end
   end
 
diff --git a/spec/models/project_group_link_spec.rb b/spec/models/project_group_link_spec.rb
index 9b711bfc007..4161b9158b1 100644
--- a/spec/models/project_group_link_spec.rb
+++ b/spec/models/project_group_link_spec.rb
@@ -23,7 +23,7 @@ describe ProjectGroupLink do
       expect(project_group_link).not_to be_valid
     end
 
-    it "doesn't allow a project to be shared with an ancestor of the group it is in" do
+    it "doesn't allow a project to be shared with an ancestor of the group it is in", :nested_groups do
       project_group_link.group = parent_group
 
       expect(project_group_link).not_to be_valid
diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb
index 942eeab251d..fb2d5f60009 100644
--- a/spec/models/project_team_spec.rb
+++ b/spec/models/project_team_spec.rb
@@ -81,7 +81,7 @@ describe ProjectTeam, models: true do
         user = create(:user)
         project.add_guest(user)
 
-        expect(project.team.members).to contain_exactly(user)
+        expect(project.team.members).to contain_exactly(user, project.owner)
       end
 
       it 'returns project members of a specified level' do
@@ -100,7 +100,8 @@ describe ProjectTeam, models: true do
           group_access: Gitlab::Access::GUEST
         )
 
-        expect(project.team.members).to contain_exactly(group_member.user)
+        expect(project.team.members).
+          to contain_exactly(group_member.user, project.owner)
       end
 
       it 'returns invited members of a group of a specified level' do
@@ -137,7 +138,10 @@ describe ProjectTeam, models: true do
 
   describe '#find_member' do
     context 'personal project' do
-      let(:project) { create(:empty_project, :public, :access_requestable) }
+      let(:project) do
+        create(:empty_project, :public, :access_requestable)
+      end
+
       let(:requester) { create(:user) }
 
       before do
@@ -200,7 +204,9 @@ describe ProjectTeam, models: true do
     let(:requester) { create(:user) }
 
     context 'personal project' do
-      let(:project) { create(:empty_project, :public, :access_requestable) }
+      let(:project) do
+        create(:empty_project, :public, :access_requestable)
+      end
 
       context 'when project is not shared with group' do
         before do
@@ -244,7 +250,9 @@ describe ProjectTeam, models: true do
 
     context 'group project' do
       let(:group) { create(:group, :access_requestable) }
-      let!(:project) { create(:empty_project, group: group) }
+      let!(:project) do
+        create(:empty_project, group: group)
+      end
 
       before do
         group.add_master(master)
@@ -265,8 +273,15 @@ describe ProjectTeam, models: true do
     let(:group) { create(:group) }
     let(:developer) { create(:user) }
     let(:master) { create(:user) }
-    let(:personal_project) { create(:empty_project, namespace: developer.namespace) }
-    let(:group_project) { create(:empty_project, namespace: group) }
+
+    let(:personal_project) do
+      create(:empty_project, namespace: developer.namespace)
+    end
+
+    let(:group_project) do
+      create(:empty_project, namespace: group)
+    end
+
     let(:members_project) { create(:empty_project) }
     let(:shared_project) { create(:empty_project) }
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index aabdac4bb75..9edf34b05ad 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -627,16 +627,6 @@ describe User, models: true do
     it { expect(User.without_projects).to include user_without_project2 }
   end
 
-  describe '.not_in_project' do
-    before do
-      User.delete_all
-      @user = create :user
-      @project = create(:empty_project)
-    end
-
-    it { expect(User.not_in_project(@project)).to include(@user, @project.owner) }
-  end
-
   describe 'user creation' do
     describe 'normal user' do
       let(:user) { create(:user, name: 'John Smith') }
@@ -1561,48 +1551,103 @@ describe User, models: true do
     end
   end
 
-  describe '#nested_groups' do
+  describe '#all_expanded_groups' do
+    # foo/bar would also match foo/barbaz instead of just foo/bar and foo/bar/baz
     let!(:user) { create(:user) }
-    let!(:group) { create(:group) }
-    let!(:nested_group) { create(:group, parent: group) }
 
-    before do
-      group.add_owner(user)
+    #                group
+    #        _______ (foo) _______
+    #       |                     |
+    #       |                     |
+    # nested_group_1        nested_group_2
+    # (bar)                 (barbaz)
+    #       |                     |
+    #       |                     |
+    # nested_group_1_1      nested_group_2_1
+    # (baz)                 (baz)
+    #
+    let!(:group) { create :group }
+    let!(:nested_group_1) { create :group, parent: group, name: 'bar' }
+    let!(:nested_group_1_1) { create :group, parent: nested_group_1, name: 'baz' }
+    let!(:nested_group_2) { create :group, parent: group, name: 'barbaz' }
+    let!(:nested_group_2_1) { create :group, parent: nested_group_2, name: 'baz' }
 
-      # Add more data to ensure method does not include wrong groups
-      create(:group).add_owner(create(:user))
+    subject { user.all_expanded_groups }
+
+    context 'user is not a member of any group' do
+      it 'returns an empty array' do
+        is_expected.to eq([])
+      end
     end
 
-    it { expect(user.nested_groups).to eq([nested_group]) }
-  end
+    context 'user is member of all groups' do
+      before do
+        group.add_owner(user)
+        nested_group_1.add_owner(user)
+        nested_group_1_1.add_owner(user)
+        nested_group_2.add_owner(user)
+        nested_group_2_1.add_owner(user)
+      end
 
-  describe '#all_expanded_groups' do
-    let!(:user) { create(:user) }
-    let!(:group) { create(:group) }
-    let!(:nested_group_1) { create(:group, parent: group) }
-    let!(:nested_group_2) { create(:group, parent: group) }
+      it 'returns all groups' do
+        is_expected.to match_array [
+          group,
+          nested_group_1, nested_group_1_1,
+          nested_group_2, nested_group_2_1
+        ]
+      end
+    end
 
-    before { nested_group_1.add_owner(user) }
+    context 'user is member of the top group' do
+      before { group.add_owner(user) }
 
-    it { expect(user.all_expanded_groups).to match_array [group, nested_group_1] }
-  end
+      if Group.supports_nested_groups?
+        it 'returns all groups' do
+          is_expected.to match_array [
+            group,
+            nested_group_1, nested_group_1_1,
+            nested_group_2, nested_group_2_1
+          ]
+        end
+      else
+        it 'returns the top-level groups' do
+          is_expected.to match_array [group]
+        end
+      end
+    end
 
-  describe '#nested_groups_projects' do
-    let!(:user) { create(:user) }
-    let!(:group) { create(:group) }
-    let!(:nested_group) { create(:group, parent: group) }
-    let!(:project) { create(:empty_project, namespace: group) }
-    let!(:nested_project) { create(:empty_project, namespace: nested_group) }
+    context 'user is member of the first child (internal node), branch 1', :nested_groups do
+      before { nested_group_1.add_owner(user) }
 
-    before do
-      group.add_owner(user)
+      it 'returns the groups in the hierarchy' do
+        is_expected.to match_array [
+          group,
+          nested_group_1, nested_group_1_1
+        ]
+      end
+    end
+
+    context 'user is member of the first child (internal node), branch 2', :nested_groups do
+      before { nested_group_2.add_owner(user) }
 
-      # Add more data to ensure method does not include wrong projects
-      other_project = create(:empty_project, namespace: create(:group, :nested))
-      other_project.add_developer(create(:user))
+      it 'returns the groups in the hierarchy' do
+        is_expected.to match_array [
+          group,
+          nested_group_2, nested_group_2_1
+        ]
+      end
     end
 
-    it { expect(user.nested_groups_projects).to eq([nested_project]) }
+    context 'user is member of the last child (leaf node)', :nested_groups do
+      before { nested_group_1_1.add_owner(user) }
+
+      it 'returns the groups in the hierarchy' do
+        is_expected.to match_array [
+          group,
+          nested_group_1, nested_group_1_1
+        ]
+      end
+    end
   end
 
   describe '#refresh_authorized_projects', redis: true do
@@ -1622,10 +1667,6 @@ describe User, models: true do
       expect(user.project_authorizations.count).to eq(2)
     end
 
-    it 'sets the authorized_projects_populated column' do
-      expect(user.authorized_projects_populated).to eq(true)
-    end
-
     it 'stores the correct access levels' do
       expect(user.project_authorizations.where(access_level: Gitlab::Access::GUEST).exists?).to eq(true)
       expect(user.project_authorizations.where(access_level: Gitlab::Access::REPORTER).exists?).to eq(true)
@@ -1735,7 +1776,7 @@ describe User, models: true do
       end
     end
 
-    context 'with 2FA requirement on nested parent group' do
+    context 'with 2FA requirement on nested parent group', :nested_groups do
       let!(:group1) { create :group, require_two_factor_authentication: true }
       let!(:group1a) { create :group, require_two_factor_authentication: false, parent: group1 }
 
@@ -1750,7 +1791,7 @@ describe User, models: true do
       end
     end
 
-    context 'with 2FA requirement on nested child group' do
+    context 'with 2FA requirement on nested child group', :nested_groups do
       let!(:group1) { create :group, require_two_factor_authentication: false }
       let!(:group1a) { create :group, require_two_factor_authentication: true, parent: group1 }