diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb
index 0282b378d881765a0ea0ff7229c7390b6616d639..0754123a3cfbfd1f54ec79dea7a86857ed33d185 100644
--- a/app/finders/groups_finder.rb
+++ b/app/finders/groups_finder.rb
@@ -39,7 +39,7 @@ def execute
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 @@ def owned_groups
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
diff --git a/changelogs/unreleased/feature-show-only-groups-user-is-member-of-in-dashboard.yml b/changelogs/unreleased/feature-show-only-groups-user-is-member-of-in-dashboard.yml
new file mode 100644
index 0000000000000000000000000000000000000000..6e2273ed9af9a152c43bc6814456acd39a57d855
--- /dev/null
+++ b/changelogs/unreleased/feature-show-only-groups-user-is-member-of-in-dashboard.yml
@@ -0,0 +1,5 @@
+---
+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
diff --git a/doc/api/groups.md b/doc/api/groups.md
index 1aed8aac64e6c32d417aead24a1e93ad1c414723..923fd662a5b7593c051b7e862dd1667c836ad1a2 100644
--- a/doc/api/groups.md
+++ b/doc/api/groups.md
@@ -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` |
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 4a4df1b8b9e05fd9358f33357947164035ec280f..92e3d5cc10a4fcf2199d4101b396801389846f9f 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -37,13 +37,11 @@ class Groups < Grape::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 @@ def present_groups(params, groups)
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 @@ def present_groups(params, groups)
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
diff --git a/lib/api/helpers/custom_attributes.rb b/lib/api/helpers/custom_attributes.rb
index 70e4eda95f826420a665ccfb84b57b209e4d41bf..10d652e33f5a6998dcacd963456976d42ec10703 100644
--- a/lib/api/helpers/custom_attributes.rb
+++ b/lib/api/helpers/custom_attributes.rb
@@ -7,6 +7,9 @@ module CustomAttributes
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 = {})
diff --git a/spec/features/dashboard/groups_list_spec.rb b/spec/features/dashboard/groups_list_spec.rb
index a71020002dc3ed1cc03c4fc3eb050166bf4ee9be..ed47f7ed390ac0323a5992c6b6676168ffc1d6ca 100644
--- a/spec/features/dashboard/groups_list_spec.rb
+++ b/spec/features/dashboard/groups_list_spec.rb
@@ -40,7 +40,7 @@ def click_group_caret(group)
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 @@ def click_group_caret(group)
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 @@ def click_group_caret(group)
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 @@ def click_group_caret(group)
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
diff --git a/spec/finders/groups_finder_spec.rb b/spec/finders/groups_finder_spec.rb
index abc470788e107e4c25696461d5586444dd2f9ad8..16c0d418d98b92dd1ae6ad9aac97bb6a90553e41 100644
--- a/spec/finders/groups_finder_spec.rb
+++ b/spec/finders/groups_finder_spec.rb
@@ -2,43 +2,71 @@
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) }
-
- context 'without a user' do
- subject { described_class.new.execute }
-
- it { is_expected.to eq([public_group]) }
+ let(:user) { create(:user) }
+
+ describe 'root level groups' do
+ using RSpec::Parameterized::TableSyntax
+
+ 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)
+
+ :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)
+
+ :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)
+
+ :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 'with a user' do
- subject { described_class.new(user).execute }
-
- context 'normal user' do
- 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 contain_exactly(public_group) }
+ 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
+
+ @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'),
+
+ 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')
+ }
+
+ 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
- context 'user is member of the private group' do
- before do
- private_group.add_guest(user)
- end
+ subject { described_class.new(User.last, params).execute.to_a }
- it { is_expected.to contain_exactly(public_group, internal_group, private_group) }
- end
+ 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) }