Commit 7317ef39 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'feature/filter-groups-for-admin-in-dashboard' into 'master'

show only groups an admin is a member of in dashboards/grops

See merge request gitlab-org/gitlab-ce!17884
parents 36043ab9 bc7877e8
......@@ -39,7 +39,7 @@ class GroupsFinder < UnionFinder
def all_groups
return [owned_groups] if params[:owned]
return [Group.all] if current_user&.full_private_access?
return [Group.all] if current_user&.full_private_access? && all_available?
groups = []
groups << Gitlab::GroupHierarchy.new(groups_for_ancestors, groups_for_descendants).all_groups if current_user
......@@ -67,6 +67,10 @@ class GroupsFinder < UnionFinder
end
def include_public_groups?
current_user.nil? || params.fetch(:all_available, true)
current_user.nil? || all_available?
end
def all_available?
params.fetch(:all_available, true)
end
end
---
title: For group dashboard, we no longer show groups which the visitor is not a member of (this applies to admins and auditors)
merge_request: 17884
author: Roger Rüttimann
type: changed
......@@ -10,7 +10,7 @@ Parameters:
| Attribute | Type | Required | Description |
| --------- | ---- | -------- | ----------- |
| `skip_groups` | array of integers | no | Skip the group IDs passed |
| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users) |
| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users, `true` for admin) |
| `search` | string | no | Return the list of authorized groups matching the search criteria |
| `order_by` | string | no | Order groups by `name` or `path`. Default is `name` |
| `sort` | string | no | Order groups in `asc` or `desc` order. Default is `asc` |
......@@ -94,7 +94,7 @@ Parameters:
| --------- | ---- | -------- | ----------- |
| `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) of the parent group |
| `skip_groups` | array of integers | no | Skip the group IDs passed |
| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users) |
| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users, `true` for admin) |
| `search` | string | no | Return the list of authorized groups matching the search criteria |
| `order_by` | string | no | Order groups by `name` or `path`. Default is `name` |
| `sort` | string | no | Order groups in `asc` or `desc` order. Default is `asc` |
......
......@@ -37,13 +37,11 @@ module API
use :pagination
end
def find_groups(params)
find_params = {
all_available: params[:all_available],
custom_attributes: params[:custom_attributes],
owned: params[:owned]
}
find_params[:parent] = find_group!(params[:id]) if params[:id]
def find_groups(params, parent_id = nil)
find_params = params.slice(:all_available, :custom_attributes, :owned)
find_params[:parent] = find_group!(parent_id) if parent_id
find_params[:all_available] =
find_params.fetch(:all_available, current_user&.full_private_access?)
groups = GroupsFinder.new(current_user, find_params).execute
groups = groups.search(params[:search]) if params[:search].present?
......@@ -85,7 +83,7 @@ module API
use :with_custom_attributes
end
get do
groups = find_groups(params)
groups = find_groups(declared_params(include_missing: false), params[:id])
present_groups params, groups
end
......@@ -213,7 +211,7 @@ module API
use :with_custom_attributes
end
get ":id/subgroups" do
groups = find_groups(params)
groups = find_groups(declared_params(include_missing: false), params[:id])
present_groups params, groups
end
......
......@@ -7,6 +7,9 @@ module API
helpers do
params :with_custom_attributes do
optional :with_custom_attributes, type: Boolean, default: false, desc: 'Include custom attributes in the response'
optional :custom_attributes, type: Hash,
desc: 'Filter with custom attributes'
end
def with_custom_attributes(collection_or_resource, options = {})
......
......@@ -40,7 +40,7 @@ feature 'Dashboard Groups page', :js do
expect(page).to have_content(nested_group.name)
end
describe 'when filtering groups', :nested_groups do
context 'when filtering groups', :nested_groups do
before do
group.add_owner(user)
nested_group.add_owner(user)
......@@ -75,7 +75,7 @@ feature 'Dashboard Groups page', :js do
end
end
describe 'group with subgroups', :nested_groups do
context 'with subgroups', :nested_groups do
let!(:subgroup) { create(:group, :public, parent: group) }
before do
......@@ -106,7 +106,7 @@ feature 'Dashboard Groups page', :js do
end
end
describe 'when using pagination' do
context 'when using pagination' do
let(:group) { create(:group, created_at: 5.days.ago) }
let(:group2) { create(:group, created_at: 2.days.ago) }
......@@ -141,4 +141,20 @@ feature 'Dashboard Groups page', :js do
expect(page).not_to have_selector("#group-#{group2.id}")
end
end
context 'when signed in as admin' do
let(:admin) { create(:admin) }
it 'shows only groups admin is member of' do
group.add_owner(admin)
expect(another_group).to be_persisted
sign_in(admin)
visit dashboard_groups_path
wait_for_requests
expect(page).to have_content(group.name)
expect(page).not_to have_content(another_group.name)
end
end
end
......@@ -4,41 +4,69 @@ describe GroupsFinder do
describe '#execute' do
let(:user) { create(:user) }
context 'root level groups' do
let!(:private_group) { create(:group, :private) }
let!(:internal_group) { create(:group, :internal) }
let!(:public_group) { create(:group, :public) }
describe 'root level groups' do
using RSpec::Parameterized::TableSyntax
context 'without a user' do
subject { described_class.new.execute }
where(:user_type, :params, :results) do
nil | { all_available: true } | %i(public_group user_public_group)
nil | { all_available: false } | %i(public_group user_public_group)
nil | {} | %i(public_group user_public_group)
it { is_expected.to eq([public_group]) }
end
:regular | { all_available: true } | %i(public_group internal_group user_public_group user_internal_group
user_private_group)
:regular | { all_available: false } | %i(user_public_group user_internal_group user_private_group)
:regular | {} | %i(public_group internal_group user_public_group user_internal_group user_private_group)
context 'with a user' do
subject { described_class.new(user).execute }
:external | { all_available: true } | %i(public_group user_public_group user_internal_group user_private_group)
:external | { all_available: false } | %i(user_public_group user_internal_group user_private_group)
:external | {} | %i(public_group user_public_group user_internal_group user_private_group)
context 'normal user' do
it { is_expected.to contain_exactly(public_group, internal_group) }
:admin | { all_available: true } | %i(public_group internal_group private_group user_public_group
user_internal_group user_private_group)
:admin | { all_available: false } | %i(user_public_group user_internal_group user_private_group)
:admin | {} | %i(public_group internal_group private_group user_public_group user_internal_group
user_private_group)
end
context 'external user' do
let(:user) { create(:user, external: true) }
with_them do
before do
# Fixme: Because of an issue: https://github.com/tomykaira/rspec-parameterized/issues/8#issuecomment-381888428
# The groups need to be created here, not with let syntax, and also compared by name and not ids
it { is_expected.to contain_exactly(public_group) }
end
@groups = {
private_group: create(:group, :private, name: 'private_group'),
internal_group: create(:group, :internal, name: 'internal_group'),
public_group: create(:group, :public, name: 'public_group'),
context 'user is member of the private group' do
before do
private_group.add_guest(user)
end
user_private_group: create(:group, :private, name: 'user_private_group'),
user_internal_group: create(:group, :internal, name: 'user_internal_group'),
user_public_group: create(:group, :public, name: 'user_public_group')
}
it { is_expected.to contain_exactly(public_group, internal_group, private_group) }
if user_type
user =
case user_type
when :regular
create(:user)
when :external
create(:user, external: true)
when :admin
create(:user, :admin)
end
@groups.values_at(:user_private_group, :user_internal_group, :user_public_group).each do |group|
group.add_developer(user)
end
end
end
subject { described_class.new(User.last, params).execute.to_a }
it { is_expected.to match_array(@groups.values_at(*results)) }
end
end
context 'subgroups', :nested_groups do
let(:user) { create(:user) }
let!(:parent_group) { create(:group, :public) }
let!(:public_subgroup) { create(:group, :public, parent: parent_group) }
let!(:internal_subgroup) { create(:group, :internal, parent: parent_group) }
......
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