Merge branch 'epics-finder-confidential' into 'master'

Filter confidential epics

See merge request gitlab-org/gitlab!31337

ee/app/finders/epics_finder.rb

@@ -88,7 +88,8 @@ class EpicsFinder < IssuableFinder
                ::Group.groups_user_can_read_epics(related_groups, current_user, same_root: true)
              end
 
-    Epic.where(group: groups)
+    epics = Epic.where(group: groups)
+    with_confidentiality_access_check(epics, groups)
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
@@ -146,4 +147,36 @@ class EpicsFinder < IssuableFinder
     items.where(parent_id: params[:parent_id])
   end
   # rubocop: enable CodeReuse/ActiveRecord
+
+  def with_confidentiality_access_check(epics, groups)
+    return epics unless Feature.enabled?(:confidential_epics_query, group)
+    return epics if can_read_all_related_groups?(groups)
+
+    epics.not_confidential_or_in_groups(groups_with_confidential_access(groups))
+  end
+
+  def groups_with_confidential_access(groups)
+    return ::Group.none unless current_user
+
+    # groups is an array, not a relation here so we have to use `map`
+    group_ids = groups.map(&:id)
+    GroupMember.by_group_ids(group_ids).by_user_id(current_user).non_guests.select(:source_id)
+  end
+
+  def can_read_all_related_groups?(groups)
+    return false unless current_user
+
+    # If a user is a member of a group, he also inherits access to all subgroups,
+    # so here we check if user is member of the top-level group (from the
+    # list of groups being requested) - this is checked by
+    # `read_confidential_epic` policy. If that's the case we don't need to
+    # check membership on subgroups.
+    #
+    # `groups` is a list of groups in the same group hierarchy, by default
+    # these should be ordered by nested level in the group hierarchy in
+    # descending order (so top-level first), except if we fetch ancestors
+    # - in that case top-level group is group's root parent
+    parent = params.fetch(:include_ancestor_groups, false) ? groups.first.root_ancestor : group
+    Ability.allowed?(current_user, :read_confidential_epic, parent)
+  end
 end

ee/app/models/ee/epic.rb

@@ -107,6 +107,10 @@ module EE
 
       scope :counts_by_state, -> { group(:state_id).count }
 
+      scope :not_confidential_or_in_groups, -> (groups) do
+        where.not(confidential: true).or(where(confidential: true, group_id: groups))
+      end
+
       MAX_HIERARCHY_DEPTH = 5
 
       def etag_caching_enabled?

ee/app/models/ee/group_member.rb

@@ -11,6 +11,8 @@ module EE
       validate :sso_enforcement, if: :group
       validate :group_domain_limitations, if: :group_has_domain_limitations?
 
+      scope :by_group_ids, ->(group_ids) { where(source_id: group_ids) }
+
       scope :with_ldap_dn, -> { joins(user: :identities).where("identities.provider LIKE ?", 'ldap%') }
       scope :with_identity_provider, ->(provider) do
         joins(user: :identities).where(identities: { provider: provider })

ee/app/policies/ee/group_policy.rb

@@ -130,6 +130,7 @@ module EE
         enable :create_epic
         enable :admin_epic
         enable :update_epic
+        enable :read_confidential_epic
       end
 
       rule { reporter & subepics_available }.policy do
@@ -141,6 +142,7 @@ module EE
       rule { ~can?(:read_cross_project) }.policy do
         prevent :read_group_contribution_analytics
         prevent :read_epic
+        prevent :read_confidential_epic
         prevent :create_epic
         prevent :admin_epic
         prevent :update_epic

ee/spec/finders/epics_finder_spec.rb

@@ -57,8 +57,8 @@ describe EpicsFinder do
           expect(epics).to contain_exactly(epic1, epic2, epic3)
         end
 
-        it 'does not execute more than 8 SQL queries' do
-          expect { epics.to_a }.not_to exceed_all_query_limit(8)
+        it 'does not execute more than 9 SQL queries' do
+          expect { epics.to_a }.not_to exceed_all_query_limit(9)
         end
 
         context 'sorting' do
@@ -296,6 +296,65 @@ describe EpicsFinder do
             end
           end
         end
+
+        context 'with confidential epics' do
+          let_it_be(:ancestor_group) { create(:group, :public) }
+          let_it_be(:base_group) { create(:group, :public, parent: ancestor_group) }
+          let_it_be(:base_epic1) { create(:epic, :confidential, group: base_group) }
+          let_it_be(:base_epic2) { create(:epic, group: base_group) }
+          let_it_be(:private_group1) { create(:group, :private, parent: base_group) }
+          let_it_be(:private_epic1) { create(:epic, group: private_group1) }
+          let_it_be(:private_epic2) { create(:epic, :confidential, group: private_group1) }
+          let_it_be(:public_group1) { create(:group, :public, parent: base_group) }
+          let_it_be(:public_epic1) { create(:epic, group: public_group1) }
+          let_it_be(:public_epic2) { create(:epic, :confidential, group: public_group1) }
+
+          subject { described_class.new(search_user, group_id: base_group.id).execute }
+
+          it 'returns only public epics' do
+            expect(subject).to match_array([base_epic2, public_epic1])
+          end
+
+          context 'when user is member of ancestor group' do
+            before do
+              ancestor_group.add_developer(search_user)
+            end
+
+            it 'returns all nested epics' do
+              expect(subject).to match_array([base_epic1, base_epic2, private_epic1, private_epic2, public_epic1, public_epic2])
+            end
+          end
+
+          context 'when user is member of private subgroup' do
+            before do
+              private_group1.add_developer(search_user)
+            end
+
+            it 'returns also confidential epics from this subgroup' do
+              expect(subject).to match_array([base_epic2, private_epic1, private_epic2, public_epic1])
+            end
+          end
+
+          context 'when user is member of public subgroup' do
+            before do
+              public_group1.add_developer(search_user)
+            end
+
+            it 'returns also confidential epics from this subgroup' do
+              expect(subject).to match_array([base_epic2, public_epic1, public_epic2])
+            end
+          end
+
+          context 'when confidential_epics_query is disabled' do
+            before do
+              stub_feature_flags(confidential_epics_query: false)
+            end
+
+            it 'returns also confidential epics' do
+              expect(subject).to match_array([base_epic1, base_epic2, public_epic1, public_epic2])
+            end
+          end
+        end
       end
     end
   end

ee/spec/models/epic_spec.rb

@@ -28,6 +28,16 @@ describe Epic do
         expect(described_class.public_only).to eq([public_epic])
       end
     end
+
+    describe '.not_confidential_or_in_groups' do
+      it 'returns only epics which are either not confidential or in the group' do
+        confidential_epic = create(:epic, confidential: true, group: group)
+        group_epic = create(:epic, group: group)
+        create(:epic, confidential: true)
+
+        expect(described_class.not_confidential_or_in_groups(group)).to match_array([confidential_epic, group_epic])
+      end
+    end
   end
 
   describe 'validations' do

ee/spec/models/group_member_spec.rb

@@ -60,25 +60,38 @@ describe GroupMember do
     end
   end
 
-  describe '.with_saml_identity' do
-    let(:saml_provider) { create :saml_provider }
-    let(:group) { saml_provider.group }
-    let!(:member) do
-      create(:group_member, group: group).tap do |m|
-        create(:group_saml_identity, saml_provider: saml_provider, user: m.user)
+  describe 'scopes' do
+    describe '.by_group_ids' do
+      it 'returns only members from selected groups' do
+        group = create(:group)
+        member1 = create(:group_member, group: group)
+        member2 = create(:group_member, group: group)
+        create(:group_member)
+
+        expect(described_class.by_group_ids([group.id])).to match_array([member1, member2])
       end
     end
-    let!(:member_without_identity) do
-      create(:group_member, group: group)
-    end
-    let!(:member_with_different_identity) do
-      create(:group_member, group: group).tap do |m|
-        create(:group_saml_identity, user: m.user)
+
+    describe '.with_saml_identity' do
+      let(:saml_provider) { create :saml_provider }
+      let(:group) { saml_provider.group }
+      let!(:member) do
+        create(:group_member, group: group).tap do |m|
+          create(:group_saml_identity, saml_provider: saml_provider, user: m.user)
+        end
+      end
+      let!(:member_without_identity) do
+        create(:group_member, group: group)
+      end
+      let!(:member_with_different_identity) do
+        create(:group_member, group: group).tap do |m|
+          create(:group_saml_identity, user: m.user)
+        end
       end
-    end
 
-    it 'returns members with identity linked to given saml provider' do
-      expect(described_class.with_saml_identity(saml_provider)).to eq([member])
+      it 'returns members with identity linked to given saml provider' do
+        expect(described_class.with_saml_identity(saml_provider)).to eq([member])
+      end
     end
   end
 

ee/spec/policies/group_policy_spec.rb

@@ -8,7 +8,7 @@ describe GroupPolicy do
   context 'when epics feature is disabled' do
     let(:current_user) { owner }
 
-    it { is_expected.to be_disallowed(:read_epic, :create_epic, :admin_epic, :destroy_epic) }
+    it { is_expected.to be_disallowed(:read_epic, :create_epic, :admin_epic, :destroy_epic, :read_confidential_epic) }
   end
 
   context 'when epics feature is enabled' do
@@ -16,9 +16,51 @@ describe GroupPolicy do
       stub_licensed_features(epics: true)
     end
 
-    let(:current_user) { owner }
+    context 'when user is owner' do
+      let(:current_user) { owner }
+
+      it { is_expected.to be_allowed(:read_epic, :create_epic, :admin_epic, :destroy_epic, :read_confidential_epic) }
+    end
+
+    context 'when user is maintainer' do
+      let(:current_user) { maintainer }
+
+      it { is_expected.to be_allowed(:read_epic, :create_epic, :admin_epic, :read_confidential_epic) }
+      it { is_expected.to be_disallowed(:destroy_epic) }
+    end
+
+    context 'when user is developer' do
+      let(:current_user) { developer }
+
+      it { is_expected.to be_allowed(:read_epic, :create_epic, :admin_epic, :read_confidential_epic) }
+      it { is_expected.to be_disallowed(:destroy_epic) }
+    end
+
+    context 'when user is reporter' do
+      let(:current_user) { reporter }
 
-    it { is_expected.to be_allowed(:read_epic, :create_epic, :admin_epic, :destroy_epic) }
+      it { is_expected.to be_allowed(:read_epic, :create_epic, :admin_epic, :read_confidential_epic) }
+      it { is_expected.to be_disallowed(:destroy_epic) }
+    end
+
+    context 'when user is guest' do
+      let(:current_user) { guest }
+
+      it { is_expected.to be_allowed(:read_epic) }
+      it { is_expected.to be_disallowed(:create_epic, :admin_epic, :destroy_epic, :read_confidential_epic) }
+    end
+
+    context 'when user is not member' do
+      let(:current_user) { create(:user) }
+
+      it { is_expected.to be_disallowed(:read_epic, :create_epic, :admin_epic, :destroy_epic, :read_confidential_epic) }
+    end
+
+    context 'when user is anonymous' do
+      let(:current_user) { nil }
+
+      it { is_expected.to be_disallowed(:read_epic, :create_epic, :admin_epic, :destroy_epic, :read_confidential_epic) }
+    end
   end
 
   context 'when iterations feature is disabled' do