Resolve "API endpoint that returns all members, including the inherited membership through ancestor group"

5 files changed, 200 insertions, 36 deletions

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