From 046796cc3a98068e99abed152145e76c4636959c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=99=88=20=20jacopo=20beschi=20=F0=9F=99=89?= Date: Wed, 25 Jul 2018 21:45:42 +0000 Subject: Resolve "API endpoint that returns all members, including the inherited membership through ancestor group" --- .../32783-api-all-members-with-ancestors.yml | 6 ++ doc/api/members.md | 75 +++++++++++++- lib/api/helpers/members_helpers.rb | 25 +++++ lib/api/members.rb | 17 ++++ spec/requests/api/members_spec.rb | 113 +++++++++++++++------ 5 files changed, 200 insertions(+), 36 deletions(-) create mode 100644 changelogs/unreleased/32783-api-all-members-with-ancestors.yml diff --git a/changelogs/unreleased/32783-api-all-members-with-ancestors.yml b/changelogs/unreleased/32783-api-all-members-with-ancestors.yml new file mode 100644 index 00000000000..ca53d02845d --- /dev/null +++ b/changelogs/unreleased/32783-api-all-members-with-ancestors.yml @@ -0,0 +1,6 @@ +--- +title: Adds API endpoint /api/v4/(project/group)/:id/members/all to list also inherited + members +merge_request: 19748 +author: Jacopo Beschi @jacopo-beschi +type: added diff --git a/doc/api/members.md b/doc/api/members.md index 8ebe464c359..7b228b92594 100644 --- a/doc/api/members.md +++ b/doc/api/members.md @@ -40,7 +40,9 @@ Example response: "username": "raymond_smith", "name": "Raymond Smith", "state": "active", - "created_at": "2012-10-22T14:13:35Z", + "avatar_url": "https://www.gravatar.com/avatar/c2525a7f58ae3776070e44c106c48e15?s=80&d=identicon", + "web_url": "http://192.168.1.8:3000/root", + "expires_at": "2012-10-22T14:13:35Z", "access_level": 30 }, { @@ -48,7 +50,65 @@ Example response: "username": "john_doe", "name": "John Doe", "state": "active", - "created_at": "2012-10-22T14:13:35Z", + "avatar_url": "https://www.gravatar.com/avatar/c2525a7f58ae3776070e44c106c48e15?s=80&d=identicon", + "web_url": "http://192.168.1.8:3000/root", + "expires_at": "2012-10-22T14:13:35Z", + "access_level": 30 + } +] +``` + +## List all members of a group or project including inherited members + +Gets a list of group or project members viewable by the authenticated user, including inherited members through ancestor groups. + +``` +GET /groups/:id/members/all +GET /projects/:id/members/all +``` + +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer/string | yes | The ID or [URL-encoded path of the project or group](README.md#namespaced-path-encoding) owned by the authenticated user | +| `query` | string | no | A query string to search for members | + +```bash +curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/groups/:id/members/all +curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/:id/members/all +``` + +Example response: + +```json +[ + { + "id": 1, + "username": "raymond_smith", + "name": "Raymond Smith", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/c2525a7f58ae3776070e44c106c48e15?s=80&d=identicon", + "web_url": "http://192.168.1.8:3000/root", + "expires_at": "2012-10-22T14:13:35Z", + "access_level": 30 + }, + { + "id": 2, + "username": "john_doe", + "name": "John Doe", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/c2525a7f58ae3776070e44c106c48e15?s=80&d=identicon", + "web_url": "http://192.168.1.8:3000/root", + "expires_at": "2012-10-22T14:13:35Z", + "access_level": 30 + }, + { + "id": 3, + "username": "foo_bar", + "name": "Foo bar", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/c2525a7f58ae3776070e44c106c48e15?s=80&d=identicon", + "web_url": "http://192.168.1.8:3000/root", + "expires_at": "2012-11-22T14:13:35Z", "access_level": 30 } ] @@ -81,7 +141,8 @@ Example response: "username": "raymond_smith", "name": "Raymond Smith", "state": "active", - "created_at": "2012-10-22T14:13:35Z", + "avatar_url": "https://www.gravatar.com/avatar/c2525a7f58ae3776070e44c106c48e15?s=80&d=identicon", + "web_url": "http://192.168.1.8:3000/root", "access_level": 30, "expires_at": null } @@ -116,7 +177,9 @@ Example response: "username": "raymond_smith", "name": "Raymond Smith", "state": "active", - "created_at": "2012-10-22T14:13:35Z", + "avatar_url": "https://www.gravatar.com/avatar/c2525a7f58ae3776070e44c106c48e15?s=80&d=identicon", + "web_url": "http://192.168.1.8:3000/root", + "expires_at": "2012-10-22T14:13:35Z", "access_level": 30 } ``` @@ -150,7 +213,9 @@ Example response: "username": "raymond_smith", "name": "Raymond Smith", "state": "active", - "created_at": "2012-10-22T14:13:35Z", + "avatar_url": "https://www.gravatar.com/avatar/c2525a7f58ae3776070e44c106c48e15?s=80&d=identicon", + "web_url": "http://192.168.1.8:3000/root", + "expires_at": "2012-10-22T14:13:35Z", "access_level": 40 } ``` diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb index a50ea0b52aa..fed8846e505 100644 --- a/lib/api/helpers/members_helpers.rb +++ b/lib/api/helpers/members_helpers.rb @@ -10,6 +10,31 @@ module API def authorize_admin_source!(source_type, source) authorize! :"admin_#{source_type}", source end + + def find_all_members(source_type, source) + members = source_type == 'project' ? find_all_members_for_project(source) : find_all_members_for_group(source) + members.non_invite + .non_request + end + + def find_all_members_for_project(project) + shared_group_ids = project.project_group_links.pluck(:group_id) + project_group_ids = project.group&.self_and_ancestors&.pluck(:id) + source_ids = [project.id, project_group_ids, shared_group_ids] + .flatten + .compact + Member.includes(:user) + .joins(user: :project_authorizations) + .where(project_authorizations: { project_id: project.id }) + .where(source_id: source_ids) + end + + def find_all_members_for_group(group) + source_ids = group.self_and_ancestors.pluck(:id) + Member.includes(:user) + .where(source_id: source_ids) + .where(source_type: 'Namespace') + end end end end diff --git a/lib/api/members.rb b/lib/api/members.rb index 8b12986d09e..3d2220fed96 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -28,6 +28,23 @@ module API present members, with: Entities::Member end + desc 'Gets a list of group or project members viewable by the authenticated user, including those who gained membership through ancestor group.' do + success Entities::Member + end + params do + optional :query, type: String, desc: 'A query string to search for members' + use :pagination + end + get ":id/members/all" do + source = find_source(source_type, params[:id]) + + members = find_all_members(source_type, source) + members = members.includes(:user).references(:user).merge(User.search(params[:query])) if params[:query].present? + members = paginate(members) + + present members, with: Entities::Member + end + desc 'Gets a member of a group or project.' do success Entities::Member end diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index 01bbe7f5ec6..c621760b6c4 100644 --- a/spec/requests/api/members_spec.rb +++ b/spec/requests/api/members_spec.rb @@ -22,10 +22,16 @@ describe API::Members do end end - shared_examples 'GET /:sources/:id/members' do |source_type| - context "with :sources == #{source_type.pluralize}" do + shared_examples 'GET /:source_type/:id/members/(all)' do |source_type, all| + let(:members_url) do + "/#{source_type.pluralize}/#{source.id}/members".tap do |url| + url << "/all" if all + end + end + + context "with :source_type == #{source_type.pluralize}" do it_behaves_like 'a 404 response when source is private' do - let(:route) { get api("/#{source_type.pluralize}/#{source.id}/members", stranger) } + let(:route) { get api(members_url, stranger) } end %i[maintainer developer access_requester stranger].each do |type| @@ -33,7 +39,7 @@ describe API::Members do it 'returns 200' do user = public_send(type) - get api("/#{source_type.pluralize}/#{source.id}/members", user) + get api(members_url, user) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -46,23 +52,23 @@ describe API::Members do it 'avoids N+1 queries' do # Establish baseline - get api("/#{source_type.pluralize}/#{source.id}/members", maintainer) + get api(members_url, maintainer) control = ActiveRecord::QueryRecorder.new do - get api("/#{source_type.pluralize}/#{source.id}/members", maintainer) + get api(members_url, maintainer) end project.add_developer(create(:user)) expect do - get api("/#{source_type.pluralize}/#{source.id}/members", maintainer) + get api(members_url, maintainer) end.not_to exceed_query_limit(control) end it 'does not return invitees' do create(:"#{source_type}_member", invite_token: '123', invite_email: 'test@abc.com', source: source, user: nil) - get api("/#{source_type.pluralize}/#{source.id}/members", developer) + get api(members_url, developer) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -72,7 +78,7 @@ describe API::Members do end it 'finds members with query string' do - get api("/#{source_type.pluralize}/#{source.id}/members", developer), query: maintainer.username + get api(members_url, developer), query: maintainer.username expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -82,7 +88,7 @@ describe API::Members do end it 'finds all members with no query specified' do - get api("/#{source_type.pluralize}/#{source.id}/members", developer), query: '' + get api(members_url, developer), query: '' expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -93,8 +99,51 @@ describe API::Members do end end - shared_examples 'GET /:sources/:id/members/:user_id' do |source_type| - context "with :sources == #{source_type.pluralize}" do + describe 'GET /:source_type/:id/members/all', :nested_groups 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(:project) do + create(:project, :public, group: nested_group) do |project| + project.add_developer(project_user) + end + end + + let(:linked_group) do + create(:group) do |linked_group| + linked_group.add_developer(linked_group_user) + end + end + + let(:nested_group) do + create(:group, parent: group) do |nested_group| + nested_group.add_developer(nested_user) + end + end + + it 'finds all project members including inherited members' do + get api("/projects/#{project.id}/members/all", developer) + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.map { |u| u['id'] }).to match_array [maintainer.id, developer.id, nested_user.id, project_user.id, linked_group_user.id] + end + + it 'finds all group members including inherited members' do + get api("/groups/#{nested_group.id}/members/all", developer) + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.map { |u| u['id'] }).to match_array [maintainer.id, developer.id, nested_user.id] + end + end + + shared_examples 'GET /:source_type/:id/members/:user_id' do |source_type| + context "with :source_type == #{source_type.pluralize}" do it_behaves_like 'a 404 response when source is private' do let(:route) { get api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", stranger) } end @@ -124,8 +173,8 @@ describe API::Members do end end - shared_examples 'POST /:sources/:id/members' do |source_type| - context "with :sources == #{source_type.pluralize}" do + shared_examples 'POST /:source_type/:id/members' do |source_type| + context "with :source_type == #{source_type.pluralize}" do it_behaves_like 'a 404 response when source is private' do let(:route) do post api("/#{source_type.pluralize}/#{source.id}/members", stranger), @@ -205,8 +254,8 @@ describe API::Members do end end - shared_examples 'PUT /:sources/:id/members/:user_id' do |source_type| - context "with :sources == #{source_type.pluralize}" do + shared_examples 'PUT /:source_type/:id/members/:user_id' do |source_type| + context "with :source_type == #{source_type.pluralize}" do it_behaves_like 'a 404 response when source is private' do let(:route) do put api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", stranger), @@ -262,8 +311,8 @@ describe API::Members do end end - shared_examples 'DELETE /:sources/:id/members/:user_id' do |source_type| - context "with :sources == #{source_type.pluralize}" do + shared_examples 'DELETE /:source_type/:id/members/:user_id' do |source_type| + context "with :source_type == #{source_type.pluralize}" do it_behaves_like 'a 404 response when source is private' do let(:route) { delete api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", stranger) } end @@ -323,43 +372,45 @@ describe API::Members do end end - it_behaves_like 'GET /:sources/:id/members', 'project' do - let(:source) { project } - end + [false, true].each do |all| + it_behaves_like 'GET /:source_type/:id/members/(all)', 'project', all do + let(:source) { project } + end - it_behaves_like 'GET /:sources/:id/members', 'group' do - let(:source) { group } + it_behaves_like 'GET /:source_type/:id/members/(all)', 'group', all do + let(:source) { group } + end end - it_behaves_like 'GET /:sources/:id/members/:user_id', 'project' do + it_behaves_like 'GET /:source_type/:id/members/:user_id', 'project' do let(:source) { project } end - it_behaves_like 'GET /:sources/:id/members/:user_id', 'group' do + it_behaves_like 'GET /:source_type/:id/members/:user_id', 'group' do let(:source) { group } end - it_behaves_like 'POST /:sources/:id/members', 'project' do + it_behaves_like 'POST /:source_type/:id/members', 'project' do let(:source) { project } end - it_behaves_like 'POST /:sources/:id/members', 'group' do + it_behaves_like 'POST /:source_type/:id/members', 'group' do let(:source) { group } end - it_behaves_like 'PUT /:sources/:id/members/:user_id', 'project' do + it_behaves_like 'PUT /:source_type/:id/members/:user_id', 'project' do let(:source) { project } end - it_behaves_like 'PUT /:sources/:id/members/:user_id', 'group' do + it_behaves_like 'PUT /:source_type/:id/members/:user_id', 'group' do let(:source) { group } end - it_behaves_like 'DELETE /:sources/:id/members/:user_id', 'project' do + it_behaves_like 'DELETE /:source_type/:id/members/:user_id', 'project' do let(:source) { project } end - it_behaves_like 'DELETE /:sources/:id/members/:user_id', 'group' do + it_behaves_like 'DELETE /:source_type/:id/members/:user_id', 'group' do let(:source) { group } end -- cgit v1.2.1