Commit f5458eb6 authored by Eugenia Grieff's avatar Eugenia Grieff

Check for group confidential access in EpicsFinder

- When `EpicsFinder` is used used with param
`include_ancestor_groups` set as true, we need to
check for epic confidential access in all ancestor
goups, no only the root ancestor

Changelog: fixed
EE: true
parent f066c8fd
...@@ -215,9 +215,16 @@ class EpicsFinder < IssuableFinder ...@@ -215,9 +215,16 @@ class EpicsFinder < IssuableFinder
def groups_with_confidential_access(groups) def groups_with_confidential_access(groups)
return ::Group.none unless current_user return ::Group.none unless current_user
# groups is an array, not a relation here so we have to use `map` # `same_root` should be set only if we are sure that all groups
group_ids = groups.map(&:id) # have the same ancestor root group. This is safe since it can only be the
GroupMember.by_group_ids(group_ids).by_user_id(current_user).non_guests.select(:source_id) # single group sent in params, or permissioned_related_groups that can
# include ancestors and descendants, so all have the same ancestor root group.
# See https://gitlab.com/gitlab-org/gitlab/issues/11539
Group.preset_root_ancestor_for(groups)
DeclarativePolicy.user_scope do
groups.select { |group| Ability.allowed?(current_user, :read_confidential_epic, group) }
end
end end
# @param include_confidential [Boolean] if this method should factor in # @param include_confidential [Boolean] if this method should factor in
......
...@@ -190,13 +190,20 @@ RSpec.describe EpicsFinder do ...@@ -190,13 +190,20 @@ RSpec.describe EpicsFinder do
let_it_be(:subgroup2) { create(:group, :private, parent: subgroup) } let_it_be(:subgroup2) { create(:group, :private, parent: subgroup) }
let_it_be(:subgroup_epic) { create(:epic, group: subgroup) } let_it_be(:subgroup_epic) { create(:epic, group: subgroup) }
let_it_be(:subgroup2_epic) { create(:epic, group: subgroup2) } let_it_be(:subgroup2_epic) { create(:epic, group: subgroup2) }
let_it_be(:confidential_epic) { create(:epic, :confidential, group: subgroup2) }
before do before do
subgroup.add_guest(subgroup_guest) subgroup.add_guest(subgroup_guest)
end end
it 'returns all epics that belong to the given group and its subgroups' do it 'returns all epics that belong to the given group and its subgroups' do
expect(epics).to contain_exactly(epic1, epic2, epic3, subgroup_epic, subgroup2_epic, epic5) expect(epics)
.to contain_exactly(epic1, epic2, epic3, subgroup_epic, subgroup2_epic, epic5, confidential_epic)
end
it 'does not return confidential epic if user has no permissions to read them' do
expect(described_class.new(subgroup_guest, { group_id: subgroup.id }).execute)
.to contain_exactly(subgroup_epic, subgroup2_epic)
end end
describe 'hierarchy params' do describe 'hierarchy params' do
...@@ -205,7 +212,7 @@ RSpec.describe EpicsFinder do ...@@ -205,7 +212,7 @@ RSpec.describe EpicsFinder do
subject { epics(finder_params.merge(group_id: subgroup.id)) } subject { epics(finder_params.merge(group_id: subgroup.id)) }
it 'excludes ancestor groups and includes descendant groups by default' do it 'excludes ancestor groups and includes descendant groups by default' do
is_expected.to contain_exactly(subgroup_epic, subgroup2_epic) is_expected.to contain_exactly(subgroup_epic, subgroup2_epic, confidential_epic)
end end
context 'when user is a member of a subgroup project' do context 'when user is a member of a subgroup project' do
...@@ -228,6 +235,34 @@ RSpec.describe EpicsFinder do ...@@ -228,6 +235,34 @@ RSpec.describe EpicsFinder do
end end
end end
context 'when user is a member of an ancestor group that is not the root ancestor' do
let_it_be(:subgroup_reporter) { create(:user) }
let(:finder) { described_class.new(subgroup_reporter, finder_params) }
let(:finder_params) { { include_descendant_groups: false, group_id: subgroup2.id } }
before do
subgroup.add_reporter(subgroup_reporter)
end
context 'when include_ancestor_groups is false' do
it 'includes subgroups confidential epics and no epics from ancestors' do
finder_params[:include_ancestor_groups] = false
expect(finder.execute).to contain_exactly(subgroup2_epic, confidential_epic)
end
end
context 'when include_ancestor_groups is true' do
it 'includes subgroups confidential epics and epics from ancestors' do
finder_params[:include_ancestor_groups] = true
expect(finder.execute)
.to contain_exactly(subgroup_epic, subgroup2_epic, confidential_epic)
end
end
end
context 'when include_descendant_groups is false' do context 'when include_descendant_groups is false' do
context 'and include_ancestor_groups is false' do context 'and include_ancestor_groups is false' do
let(:finder_params) { { include_descendant_groups: false, include_ancestor_groups: false } } let(:finder_params) { { include_descendant_groups: false, include_ancestor_groups: false } }
...@@ -254,13 +289,13 @@ RSpec.describe EpicsFinder do ...@@ -254,13 +289,13 @@ RSpec.describe EpicsFinder do
context 'and include_ancestor_groups is false' do context 'and include_ancestor_groups is false' do
let(:finder_params) { { include_ancestor_groups: false } } let(:finder_params) { { include_ancestor_groups: false } }
it { is_expected.to contain_exactly(subgroup_epic, subgroup2_epic) } it { is_expected.to contain_exactly(subgroup_epic, subgroup2_epic, confidential_epic) }
end end
context 'and include_ancestor_groups is true' do context 'and include_ancestor_groups is true' do
let(:finder_params) { { include_ancestor_groups: true } } let(:finder_params) { { include_ancestor_groups: true } }
it { is_expected.to contain_exactly(subgroup_epic, subgroup2_epic, epic1, epic2, epic3, epic5) } it { is_expected.to contain_exactly(subgroup_epic, subgroup2_epic, epic1, epic2, epic3, epic5, confidential_epic) }
context "when user does not have permission to view ancestor groups" do context "when user does not have permission to view ancestor groups" do
let(:finder_params) { { group_id: subgroup.id, include_ancestor_groups: true } } let(:finder_params) { { group_id: subgroup.id, include_ancestor_groups: true } }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment