From 1a47539ed8af1e76588ed8b5fc5eaaa206a03cee Mon Sep 17 00:00:00 2001 From: Abdul Wadood Date: Tue, 26 Mar 2024 09:48:40 +0530 Subject: [PATCH 1/3] Hide the `created_by` attribute from group/project non-admins After we enable the `webui_members_inherited_users` feature flag, we'll start returning the invited private group members to the project/group members for the `/projects/:id/members/all` & `/groups/:id/members/all` APIs. Here we're hiding the `created_at` attribute from the non-admins of the shared group/project. Changelog: changed --- ...ed_private_group_accessibility_assigner.rb | 9 +- lib/api/entities/member.rb | 3 +- lib/api/helpers/members_helpers.rb | 6 + lib/api/members.rb | 4 + ...ivate_group_accessibility_assigner_spec.rb | 12 - spec/requests/api/members_spec.rb | 238 +++++++++++++++++- 6 files changed, 244 insertions(+), 28 deletions(-) diff --git a/app/models/members/members/invited_private_group_accessibility_assigner.rb b/app/models/members/members/invited_private_group_accessibility_assigner.rb index cd92ed797725ee..808494bbbed209 100644 --- a/app/models/members/members/invited_private_group_accessibility_assigner.rb +++ b/app/models/members/members/invited_private_group_accessibility_assigner.rb @@ -12,11 +12,12 @@ class InvitedPrivateGroupAccessibilityAssigner include Gitlab::Utils::StrongMemoize def initialize(members, source:, current_user:) - if !members.is_a?(Array) && !members.is_a?(MembersPresenter) - raise ArgumentError, "members should be an instance of Array or MembersPresenter" - end + @members = if members.is_a?(ActiveRecord::Base) + Array.wrap(members) + else + members.to_a + end - @members = members @source = source @current_user = current_user end diff --git a/lib/api/entities/member.rb b/lib/api/entities/member.rb index 7ce1e73a0432ce..041080b361c849 100644 --- a/lib/api/entities/member.rb +++ b/lib/api/entities/member.rb @@ -6,7 +6,8 @@ class Member < Grape::Entity expose :user, merge: true, using: UserBasic expose :access_level expose :created_at - expose :created_by, with: UserBasic, expose_nil: false + expose :created_by, with: UserBasic, expose_nil: false, + if: ->(member) { member.is_source_accessible_to_current_user } expose :expires_at end end diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb index 128848ca47bc3b..93f44889094ed1 100644 --- a/lib/api/helpers/members_helpers.rb +++ b/lib/api/helpers/members_helpers.rb @@ -80,6 +80,12 @@ def present_member_invitations(invitations) present invitations, with: Entities::Invitation, current_user: current_user end + def assign_private_group_accessibility(members, source) + ::Members::InvitedPrivateGroupAccessibilityAssigner + .new(members, source: source, current_user: current_user) + .execute + end + def add_single_member_by_user_id(create_service_params) source = create_service_params[:source] user_id = create_service_params[:user_id] diff --git a/lib/api/members.rb b/lib/api/members.rb index e4bd29640cd6a5..ec0f2901444f9a 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -62,6 +62,8 @@ class Members < ::API::Base members = paginate(retrieve_members(source, params: params, deep: true)) + assign_private_group_accessibility(members, source) + present_members members end @@ -101,6 +103,8 @@ class Members < ::API::Base members = find_all_members(source).order(access_level: :desc) member = members.find_by!(user_id: params[:user_id]) + assign_private_group_accessibility(member, source) + present_members member end # rubocop: enable CodeReuse/ActiveRecord diff --git a/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb b/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb index 1f720b779cf44e..7edcf8a80d09fe 100644 --- a/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb +++ b/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb @@ -7,18 +7,6 @@ let_it_be(:user) { create(:user) } - describe '#initialize' do - it 'raises an error when initializing using ActiveRecord::Relation' do - members = Member.all - expect { described_class.new(members, source: nil, current_user: user) }.to raise_error(ArgumentError) - end - - it 'does not raise an error when initializing using Array or MembersPresenter' do - expect { described_class.new([], source: nil, current_user: user) }.not_to raise_error - expect { described_class.new(MembersPresenter.new(nil), source: nil, current_user: user) }.not_to raise_error - end - end - describe '#execute' do shared_examples 'assigns is_source_accessible_to_current_user' do let(:current_user) { user } diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index 682f0d57a31971..22f49ce46822f3 100644 --- a/spec/requests/api/members_spec.rb +++ b/spec/requests/api/members_spec.rb @@ -131,35 +131,124 @@ let(:linked_group_user) { create(:user) } let!(:project_group_link) { create(:project_group_link, project: project, group: linked_group) } let(:invited_group_developer) { create(:user, username: 'invited_group_developer') } - let(:invited_group) { create(:group) { |group| group.add_developer(invited_group_developer) } } + let(:invited_group) do + create(:group) do |group| + group.add_owner(maintainer) + group.add_developer(invited_group_developer, maintainer) + end + end let(:project) do create(:project, :public, group: nested_group) do |project| - project.add_developer(project_user) + project.add_developer(project_user, current_user: maintainer) end end let(:linked_group) do create(:group) do |linked_group| - linked_group.add_developer(linked_group_user) + linked_group.add_owner(maintainer) + linked_group.add_developer(linked_group_user, maintainer) end end let(:nested_group) do create(:group, parent: group) do |nested_group| - nested_group.add_developer(nested_user) + nested_group.add_developer(nested_user, maintainer) create(:group_group_link, :guest, shared_with_group: invited_group, shared_group: nested_group) end end - it 'finds all project members including inherited members and members shared into ancestor groups' do - get api("/projects/#{project.id}/members/all", developer) + context 'when invited groups have public visibility' do + it 'finds all project members including inherited members and members shared into ancestor groups' do + get api("/projects/#{project.id}/members/all", developer) - expect(response).to have_gitlab_http_status(:ok) - expect(response).to include_pagination_headers - expect(json_response).to be_an Array - expected_user_ids = [maintainer.id, developer.id, nested_user.id, project_user.id, linked_group_user.id, invited_group_developer.id] - expect(json_response.map { |u| u['id'] }).to match_array expected_user_ids + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expected_user_ids = [maintainer.id, developer.id, nested_user.id, project_user.id, linked_group_user.id, invited_group_developer.id] + expect(json_response.map { |u| u['id'] }).to match_array expected_user_ids + end + + it 'finds all group members including inherited members and members shared into ancestor groups' do + get api("/groups/#{nested_group.id}/members/all", developer) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expected_user_ids = [maintainer.id, developer.id, nested_user.id, invited_group_developer.id] + expect(json_response.map { |u| u['id'] }).to match_array expected_user_ids + end + end + + context 'when invited groups have private visibility' do + before do + linked_group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + invited_group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + context 'when current user is a member of the shared source' do + it 'hides the created_by attribute of the invited group members of the current project' do + get api("/projects/#{project.id}/members/all", developer) + + expected_user_ids = [maintainer.id, developer.id, nested_user.id, project_user.id, linked_group_user.id, invited_group_developer.id] + expect(json_response.map { |u| u['id'] }).to match_array expected_user_ids + + hidden_created_by_user_ids = [maintainer.id, linked_group_user.id, invited_group_developer.id] + hidden_created_by_members, revealed_created_by_members = json_response + .partition { |u| hidden_created_by_user_ids.include?(u['id']) } + + expect(hidden_created_by_members.map { |u| u['created_by'] }).to all(be_nil) + expect(revealed_created_by_members.map { |u| u['created_by'] }).to all(be_present) + end + + it 'hides the created_by attribute of the invited group members of the current group' do + get api("/groups/#{nested_group.id}/members/all", developer) + + expected_user_ids = [maintainer.id, developer.id, nested_user.id, invited_group_developer.id] + expect(json_response.map { |u| u['id'] }).to match_array expected_user_ids + + hidden_created_by_user_ids = [maintainer.id, invited_group_developer.id] + hidden_created_by_members, revealed_created_by_members = json_response + .partition { |u| hidden_created_by_user_ids.include?(u['id']) } + + expect(hidden_created_by_members.map { |u| u['created_by'] }).to all(be_nil) + expect(revealed_created_by_members.map { |u| u['created_by'] }).to all(be_present) + end + + context 'when current user is an admin of the shared source' do + it 'reveals the created_by attribute of all the invited group members of the current project' do + get api("/projects/#{project.id}/members/all", maintainer) + + expected_user_ids = [maintainer.id, developer.id, nested_user.id, project_user.id, linked_group_user.id, invited_group_developer.id] + expect(json_response.map { |u| u['id'] }).to match_array expected_user_ids + expect(json_response.reject { |u| u['id'] == maintainer.id }.map { |u| u['created_by'] }).to all(be_present) + end + + it 'reveals the created_by attribute of all the invited group members of the current group' do + get api("/groups/#{nested_group.id}/members/all", maintainer) + + expected_user_ids = [maintainer.id, developer.id, nested_user.id, invited_group_developer.id] + expect(json_response.map { |u| u['id'] }).to match_array expected_user_ids + expect(json_response.reject { |u| u['id'] == maintainer.id }.map { |u| u['created_by'] }).to all(be_present) + end + end + end + + context 'when current user is a non-member of the shared source' do + it 'does not return the members of the invited private group of the current project' do + get api("/projects/#{project.id}/members/all", create(:user)) + + expected_user_ids = [maintainer.id, developer.id, nested_user.id, project_user.id] + expect(json_response.map { |u| u['id'] }).to match_array expected_user_ids + end + + it 'does not return the members of the invited private group of the current group' do + get api("/groups/#{nested_group.id}/members/all", create(:user)) + + expected_user_ids = [maintainer.id, developer.id, nested_user.id] + expect(json_response.map { |u| u['id'] }).to match_array expected_user_ids + end + end end it 'returns only one member for each user without returning duplicated members with correct access levels' do @@ -208,6 +297,133 @@ end end + describe 'GET /:source_type/:id/members/all/:user_id' do + let(:nested_user) { create(:user) } + let(:project_user) { create(:user) } + let(:linked_group_user) { create(:user) } + let!(:project_group_link) { create(:project_group_link, project: project, group: linked_group) } + let(:invited_group_developer) { create(:user, username: 'invited_group_developer') } + let(:invited_group) do + create(:group) do |group| + group.add_owner(maintainer) + group.add_developer(invited_group_developer, maintainer) + end + end + + let(:project) do + create(:project, :public, group: nested_group) do |project| + project.add_developer(project_user, current_user: maintainer) + end + end + + let(:linked_group) do + create(:group) do |linked_group| + linked_group.add_owner(maintainer) + linked_group.add_developer(linked_group_user, maintainer) + end + end + + let(:nested_group) do + create(:group, parent: group) do |nested_group| + nested_group.add_developer(nested_user, maintainer) + create(:group_group_link, :guest, shared_with_group: invited_group, shared_group: nested_group) + end + end + + context 'when invited groups have public visibility' do + it 'finds all project members including inherited members and members shared into ancestor groups' do + get api("/projects/#{project.id}/members/all/#{linked_group_user.id}", developer) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['id']).to eq(linked_group_user.id) + expect(json_response['created_by']).to be_present + end + + it 'finds all group members including inherited members and members shared into ancestor groups' do + get api("/groups/#{nested_group.id}/members/all/#{invited_group_developer.id}", developer) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['id']).to eq(invited_group_developer.id) + expect(json_response['created_by']).to be_present + end + end + + context 'when invited groups have private visibility' do + before do + linked_group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + invited_group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + context 'when current user is a member of the shared source' do + it 'hides the created_by attribute of the invited group members of the current project' do + get api("/projects/#{project.id}/members/all/#{linked_group_user.id}", developer) + + expect(json_response['id']).to eq(linked_group_user.id) + expect(json_response['created_by']).to be_nil + end + + it 'hides the created_by attribute of the invited group members of the current group' do + get api("/groups/#{nested_group.id}/members/all/#{invited_group_developer.id}", developer) + + expect(json_response['id']).to eq(invited_group_developer.id) + expect(json_response['created_by']).to be_nil + end + + context 'when current user is an admin of the shared source' do + it 'reveals the created_by attribute of all the invited group members of the current project' do + get api("/projects/#{project.id}/members/all/#{linked_group_user.id}", maintainer) + + expect(json_response['id']).to eq(linked_group_user.id) + expect(json_response['created_by']).to be_present + end + + it 'reveals the created_by attribute of all the invited group members of the current group' do + get api("/groups/#{nested_group.id}/members/all/#{invited_group_developer.id}", maintainer) + + expect(json_response['id']).to eq(invited_group_developer.id) + expect(json_response['created_by']).to be_present + end + end + end + + context 'when current user is a non-member of the shared source' do + it 'does not return the member of the invited private group of the current project' do + get api("/projects/#{project.id}/members/all/#{linked_group_user.id}", create(:user)) + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'does not return the member of the invited private group of the current group' do + get api("/groups/#{nested_group.id}/members/all/#{invited_group_developer.id}", create(:user)) + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + it 'finds the inherited group member' do + get api("/groups/#{nested_group.id}/members/all/#{maintainer.id}", developer) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['id']).to eq(maintainer.id) + end + + context 'with a subgroup' do + let(:group) { create(:group, :private) } + let(:subgroup) { create(:group, :private, parent: group) } + let(:project) { create(:project, group: subgroup) } + + before do + subgroup.add_developer(developer) + end + + it 'subgroup member cannot get parent group members list' do + get api("/groups/#{group.id}/members/all/#{maintainer.id}", developer) + expect(response).to have_gitlab_http_status(:forbidden) + end + end + end + shared_examples 'GET /:source_type/:id/members/(all/):user_id' do |source_type, all| context "with :source_type == #{source_type.pluralize} and all == #{all}" do it_behaves_like 'a 404 response when source is private' do -- GitLab From 11673169c4a76b78d9a26ca334c7fa0fa7b58637 Mon Sep 17 00:00:00 2001 From: Abdul Wadood Date: Thu, 28 Mar 2024 10:27:19 +0530 Subject: [PATCH 2/3] Rename method to assign_invited_private_group_accessibility --- lib/api/helpers/members_helpers.rb | 2 +- lib/api/members.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb index 93f44889094ed1..382c0ca03d762b 100644 --- a/lib/api/helpers/members_helpers.rb +++ b/lib/api/helpers/members_helpers.rb @@ -80,7 +80,7 @@ def present_member_invitations(invitations) present invitations, with: Entities::Invitation, current_user: current_user end - def assign_private_group_accessibility(members, source) + def assign_invited_private_group_accessibility(members, source) ::Members::InvitedPrivateGroupAccessibilityAssigner .new(members, source: source, current_user: current_user) .execute diff --git a/lib/api/members.rb b/lib/api/members.rb index ec0f2901444f9a..ce12c09427d41a 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -62,7 +62,7 @@ class Members < ::API::Base members = paginate(retrieve_members(source, params: params, deep: true)) - assign_private_group_accessibility(members, source) + assign_invited_private_group_accessibility(members, source) present_members members end @@ -103,7 +103,7 @@ class Members < ::API::Base members = find_all_members(source).order(access_level: :desc) member = members.find_by!(user_id: params[:user_id]) - assign_private_group_accessibility(member, source) + assign_invited_private_group_accessibility(member, source) present_members member end -- GitLab From adf207e22c47d346b688b28e25098d7d1570846d Mon Sep 17 00:00:00 2001 From: Rutger Wessels Date: Wed, 3 Apr 2024 14:33:29 +0200 Subject: [PATCH 3/3] Use present_members_with_invited_private_group_accessibility helper --- lib/api/helpers/members_helpers.rb | 4 +++- lib/api/members.rb | 8 ++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb index 382c0ca03d762b..d367bc71eacd31 100644 --- a/lib/api/helpers/members_helpers.rb +++ b/lib/api/helpers/members_helpers.rb @@ -80,10 +80,12 @@ def present_member_invitations(invitations) present invitations, with: Entities::Invitation, current_user: current_user end - def assign_invited_private_group_accessibility(members, source) + def present_members_with_invited_private_group_accessibility(members, source) ::Members::InvitedPrivateGroupAccessibilityAssigner .new(members, source: source, current_user: current_user) .execute + + present_members members end def add_single_member_by_user_id(create_service_params) diff --git a/lib/api/members.rb b/lib/api/members.rb index ce12c09427d41a..eb9c052af60ae1 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -62,9 +62,7 @@ class Members < ::API::Base members = paginate(retrieve_members(source, params: params, deep: true)) - assign_invited_private_group_accessibility(members, source) - - present_members members + present_members_with_invited_private_group_accessibility(members, source) end desc 'Gets a member of a group or project.' do @@ -103,9 +101,7 @@ class Members < ::API::Base members = find_all_members(source).order(access_level: :desc) member = members.find_by!(user_id: params[:user_id]) - assign_invited_private_group_accessibility(member, source) - - present_members member + present_members_with_invited_private_group_accessibility(member, source) end # rubocop: enable CodeReuse/ActiveRecord -- GitLab