Commit bc7877e8 authored by Roger Rüttimann's avatar Roger Rüttimann Committed by Douwe Maan

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

parent 36043ab9
...@@ -39,7 +39,7 @@ class GroupsFinder < UnionFinder ...@@ -39,7 +39,7 @@ class GroupsFinder < UnionFinder
def all_groups def all_groups
return [owned_groups] if params[:owned] 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 = []
groups << Gitlab::GroupHierarchy.new(groups_for_ancestors, groups_for_descendants).all_groups if current_user groups << Gitlab::GroupHierarchy.new(groups_for_ancestors, groups_for_descendants).all_groups if current_user
...@@ -67,6 +67,10 @@ class GroupsFinder < UnionFinder ...@@ -67,6 +67,10 @@ class GroupsFinder < UnionFinder
end end
def include_public_groups? 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
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: ...@@ -10,7 +10,7 @@ Parameters:
| Attribute | Type | Required | Description | | Attribute | Type | Required | Description |
| --------- | ---- | -------- | ----------- | | --------- | ---- | -------- | ----------- |
| `skip_groups` | array of integers | no | Skip the group IDs passed | | `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 | | `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` | | `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` | | `sort` | string | no | Order groups in `asc` or `desc` order. Default is `asc` |
...@@ -94,7 +94,7 @@ Parameters: ...@@ -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 | | `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 | | `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 | | `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` | | `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` | | `sort` | string | no | Order groups in `asc` or `desc` order. Default is `asc` |
......
...@@ -37,13 +37,11 @@ module API ...@@ -37,13 +37,11 @@ module API
use :pagination use :pagination
end end
def find_groups(params) def find_groups(params, parent_id = nil)
find_params = { find_params = params.slice(:all_available, :custom_attributes, :owned)
all_available: params[:all_available], find_params[:parent] = find_group!(parent_id) if parent_id
custom_attributes: params[:custom_attributes], find_params[:all_available] =
owned: params[:owned] find_params.fetch(:all_available, current_user&.full_private_access?)
}
find_params[:parent] = find_group!(params[:id]) if params[:id]
groups = GroupsFinder.new(current_user, find_params).execute groups = GroupsFinder.new(current_user, find_params).execute
groups = groups.search(params[:search]) if params[:search].present? groups = groups.search(params[:search]) if params[:search].present?
...@@ -85,7 +83,7 @@ module API ...@@ -85,7 +83,7 @@ module API
use :with_custom_attributes use :with_custom_attributes
end end
get do get do
groups = find_groups(params) groups = find_groups(declared_params(include_missing: false), params[:id])
present_groups params, groups present_groups params, groups
end end
...@@ -213,7 +211,7 @@ module API ...@@ -213,7 +211,7 @@ module API
use :with_custom_attributes use :with_custom_attributes
end end
get ":id/subgroups" do get ":id/subgroups" do
groups = find_groups(params) groups = find_groups(declared_params(include_missing: false), params[:id])
present_groups params, groups present_groups params, groups
end end
......
...@@ -7,6 +7,9 @@ module API ...@@ -7,6 +7,9 @@ module API
helpers do helpers do
params :with_custom_attributes do params :with_custom_attributes do
optional :with_custom_attributes, type: Boolean, default: false, desc: 'Include custom attributes in the response' 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 end
def with_custom_attributes(collection_or_resource, options = {}) def with_custom_attributes(collection_or_resource, options = {})
......
...@@ -40,7 +40,7 @@ feature 'Dashboard Groups page', :js do ...@@ -40,7 +40,7 @@ feature 'Dashboard Groups page', :js do
expect(page).to have_content(nested_group.name) expect(page).to have_content(nested_group.name)
end end
describe 'when filtering groups', :nested_groups do context 'when filtering groups', :nested_groups do
before do before do
group.add_owner(user) group.add_owner(user)
nested_group.add_owner(user) nested_group.add_owner(user)
...@@ -75,7 +75,7 @@ feature 'Dashboard Groups page', :js do ...@@ -75,7 +75,7 @@ feature 'Dashboard Groups page', :js do
end end
end end
describe 'group with subgroups', :nested_groups do context 'with subgroups', :nested_groups do
let!(:subgroup) { create(:group, :public, parent: group) } let!(:subgroup) { create(:group, :public, parent: group) }
before do before do
...@@ -106,7 +106,7 @@ feature 'Dashboard Groups page', :js do ...@@ -106,7 +106,7 @@ feature 'Dashboard Groups page', :js do
end end
end end
describe 'when using pagination' do context 'when using pagination' do
let(:group) { create(:group, created_at: 5.days.ago) } let(:group) { create(:group, created_at: 5.days.ago) }
let(:group2) { create(:group, created_at: 2.days.ago) } let(:group2) { create(:group, created_at: 2.days.ago) }
...@@ -141,4 +141,20 @@ feature 'Dashboard Groups page', :js do ...@@ -141,4 +141,20 @@ feature 'Dashboard Groups page', :js do
expect(page).not_to have_selector("#group-#{group2.id}") expect(page).not_to have_selector("#group-#{group2.id}")
end end
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 end
...@@ -4,41 +4,69 @@ describe GroupsFinder do ...@@ -4,41 +4,69 @@ describe GroupsFinder do
describe '#execute' do describe '#execute' do
let(:user) { create(:user) } let(:user) { create(:user) }
context 'root level groups' do describe 'root level groups' do
let!(:private_group) { create(:group, :private) } using RSpec::Parameterized::TableSyntax
let!(:internal_group) { create(:group, :internal) }
let!(:public_group) { create(:group, :public) }
context 'without a user' do where(:user_type, :params, :results) do
subject { described_class.new.execute } 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]) } :regular | { all_available: true } | %i(public_group internal_group user_public_group user_internal_group
end 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 :external | { all_available: true } | %i(public_group user_public_group user_internal_group user_private_group)
subject { described_class.new(user).execute } :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 :admin | { all_available: true } | %i(public_group internal_group private_group user_public_group
it { is_expected.to contain_exactly(public_group, internal_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 end
context 'external user' do with_them do
let(:user) { create(:user, external: true) } 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) } @groups = {
end 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 user_private_group: create(:group, :private, name: 'user_private_group'),
before do user_internal_group: create(:group, :internal, name: 'user_internal_group'),
private_group.add_guest(user) user_public_group: create(:group, :public, name: 'user_public_group')
end }
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 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 context 'subgroups', :nested_groups do
let(:user) { create(:user) }
let!(:parent_group) { create(:group, :public) } let!(:parent_group) { create(:group, :public) }
let!(:public_subgroup) { create(:group, :public, parent: parent_group) } let!(:public_subgroup) { create(:group, :public, parent: parent_group) }
let!(:internal_subgroup) { create(:group, :internal, 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