Commit ea4eb460 authored by Douwe Maan's avatar Douwe Maan Committed by Bob Van Landuyt

Merge branch 'tc-fix-private-subgroups-shown' into 'security'

Use GroupsFinder to find subgroups the user has access to

See merge request !2096
parent e5e94618
class Explore::GroupsController < Explore::ApplicationController class Explore::GroupsController < Explore::ApplicationController
def index def index
@groups = GroupsFinder.new.execute(current_user) @groups = GroupsFinder.new(current_user).execute
@groups = @groups.search(params[:filter_groups]) if params[:filter_groups].present? @groups = @groups.search(params[:filter_groups]) if params[:filter_groups].present?
@groups = @groups.sort(@sort = params[:sort]) @groups = @groups.sort(@sort = params[:sort])
@groups = @groups.page(params[:page]) @groups = @groups.page(params[:page])
......
...@@ -64,7 +64,7 @@ class GroupsController < Groups::ApplicationController ...@@ -64,7 +64,7 @@ class GroupsController < Groups::ApplicationController
end end
def subgroups def subgroups
@nested_groups = group.children @nested_groups = GroupsFinder.new(current_user, parent: group).execute
@nested_groups = @nested_groups.search(params[:filter_groups]) if params[:filter_groups].present? @nested_groups = @nested_groups.search(params[:filter_groups]) if params[:filter_groups].present?
end end
......
class GroupsFinder < UnionFinder class GroupsFinder < UnionFinder
def execute(current_user = nil) def initialize(current_user = nil, params = {})
segments = all_groups(current_user) @current_user = current_user
@params = params
end
find_union(segments, Group).with_route.order_id_desc def execute
groups = find_union(all_groups, Group).with_route.order_id_desc
by_parent(groups)
end end
private private
def all_groups(current_user) attr_reader :current_user, :params
def all_groups
groups = [] groups = []
groups << current_user.authorized_groups if current_user groups << current_user.authorized_groups if current_user
...@@ -15,4 +21,10 @@ class GroupsFinder < UnionFinder ...@@ -15,4 +21,10 @@ class GroupsFinder < UnionFinder
groups groups
end end
def by_parent(groups)
return groups unless params[:parent]
groups.where(parent: params[:parent])
end
end end
---
title: "Do not show private groups on subgroups page if user doesn't have access to"
merge_request:
author:
...@@ -52,7 +52,7 @@ module API ...@@ -52,7 +52,7 @@ module API
elsif current_user.admin elsif current_user.admin
Group.all Group.all
elsif params[:all_available] elsif params[:all_available]
GroupsFinder.new.execute(current_user) GroupsFinder.new(current_user).execute
else else
current_user.groups current_user.groups
end end
......
...@@ -45,7 +45,7 @@ module API ...@@ -45,7 +45,7 @@ module API
groups = if current_user.admin groups = if current_user.admin
Group.all Group.all
elsif params[:all_available] elsif params[:all_available]
GroupsFinder.new.execute(current_user) GroupsFinder.new(current_user).execute
else else
current_user.groups current_user.groups
end end
......
...@@ -26,6 +26,41 @@ describe GroupsController do ...@@ -26,6 +26,41 @@ describe GroupsController do
end end
end end
describe 'GET #subgroups' do
let!(:public_subgroup) { create(:group, :public, parent: group) }
let!(:private_subgroup) { create(:group, :private, parent: group) }
context 'as a user' do
before do
sign_in(user)
end
it 'shows the public subgroups' do
get :subgroups, id: group.to_param
expect(assigns(:nested_groups)).to contain_exactly(public_subgroup)
end
context 'being member' do
it 'shows public and private subgroups the user is member of' do
private_subgroup.add_guest(user)
get :subgroups, id: group.to_param
expect(assigns(:nested_groups)).to contain_exactly(public_subgroup, private_subgroup)
end
end
end
context 'as a guest' do
it 'shows the public subgroups' do
get :subgroups, id: group.to_param
expect(assigns(:nested_groups)).to contain_exactly(public_subgroup)
end
end
end
describe 'GET #issues' do describe 'GET #issues' do
let(:issue_1) { create(:issue, project: project) } let(:issue_1) { create(:issue, project: project) }
let(:issue_2) { create(:issue, project: project) } let(:issue_2) { create(:issue, project: project) }
......
...@@ -3,29 +3,64 @@ require 'spec_helper' ...@@ -3,29 +3,64 @@ require 'spec_helper'
describe GroupsFinder do describe GroupsFinder do
describe '#execute' do describe '#execute' do
let(:user) { create(:user) } let(:user) { create(:user) }
let!(:private_group) { create(:group, :private) }
let!(:internal_group) { create(:group, :internal) }
let!(:public_group) { create(:group, :public) }
let(:finder) { described_class.new }
describe 'execute' do context 'root level groups' do
describe 'without a user' do let!(:private_group) { create(:group, :private) }
subject { finder.execute } let!(:internal_group) { create(:group, :internal) }
let!(:public_group) { create(:group, :public) }
context 'without a user' do
subject { described_class.new.execute }
it { is_expected.to eq([public_group]) } it { is_expected.to eq([public_group]) }
end end
describe 'with a user' do context 'with a user' do
subject { finder.execute(user) } subject { described_class.new(user).execute }
context 'normal user' do context 'normal user' do
it { is_expected.to eq([public_group, internal_group]) } it { is_expected.to contain_exactly(public_group, internal_group) }
end end
context 'external user' do context 'external user' do
let(:user) { create(:user, external: true) } let(:user) { create(:user, external: true) }
it { is_expected.to eq([public_group]) } it { is_expected.to contain_exactly(public_group) }
end
context 'user is member of the private group' do
before do
private_group.add_guest(user)
end
it { is_expected.to contain_exactly(public_group, internal_group, private_group) }
end
end
end
context 'subgroups' do
let!(:parent_group) { create(:group, :public) }
let!(:public_subgroup) { create(:group, :public, parent: parent_group) }
let!(:internal_subgroup) { create(:group, :internal, parent: parent_group) }
let!(:private_subgroup) { create(:group, :private, parent: parent_group) }
context 'without a user' do
it 'only returns public subgroups' do
expect(described_class.new(nil, parent: parent_group).execute).to contain_exactly(public_subgroup)
end
end
context 'with a user' do
it 'returns public and internal subgroups' do
expect(described_class.new(user, parent: parent_group).execute).to contain_exactly(public_subgroup, internal_subgroup)
end
context 'being member' do
it 'returns public subgroups, internal subgroups, and private subgroups user is member of' do
private_subgroup.add_guest(user)
expect(described_class.new(user, parent: parent_group).execute).to contain_exactly(public_subgroup, internal_subgroup, private_subgroup)
end
end end
end end
end end
......
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