Commit bee09952 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 da3e653e
class Explore::GroupsController < Explore::ApplicationController
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.sort(@sort = params[:sort])
@groups = @groups.page(params[:page])
......
......@@ -64,7 +64,7 @@ class GroupsController < Groups::ApplicationController
end
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?
end
......
class GroupsFinder < UnionFinder
def execute(current_user = nil)
segments = all_groups(current_user)
def initialize(current_user = nil, params = {})
@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
private
def all_groups(current_user)
attr_reader :current_user, :params
def all_groups
groups = []
groups << current_user.authorized_groups if current_user
......@@ -15,4 +21,10 @@ class GroupsFinder < UnionFinder
groups
end
def by_parent(groups)
return groups unless params[:parent]
groups.where(parent: params[:parent])
end
end
---
title: "Do not show private groups on subgroups page if user doesn't have access to"
merge_request:
author:
......@@ -60,7 +60,7 @@ module API
elsif current_user.admin
Group.all
elsif params[:all_available]
GroupsFinder.new.execute(current_user)
GroupsFinder.new(current_user).execute
else
current_user.groups
end
......
......@@ -53,7 +53,7 @@ module API
groups = if current_user.admin
Group.all
elsif params[:all_available]
GroupsFinder.new.execute(current_user)
GroupsFinder.new(current_user).execute
else
current_user.groups
end
......
......@@ -26,6 +26,41 @@ describe GroupsController do
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
let(:issue_1) { create(:issue, project: project) }
let(:issue_2) { create(:issue, project: project) }
......
......@@ -3,29 +3,64 @@ require 'spec_helper'
describe GroupsFinder do
describe '#execute' do
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
describe 'without a user' do
subject { finder.execute }
context 'root level groups' do
let!(:private_group) { create(:group, :private) }
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]) }
end
describe 'with a user' do
subject { finder.execute(user) }
context 'with a user' do
subject { described_class.new(user).execute }
context 'normal user' do
it { is_expected.to eq([public_group, internal_group]) }
it { is_expected.to contain_exactly(public_group, internal_group) }
end
context 'external user' do
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
......
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