Merge branch '33260-allow-admins-to-list-admins' into 'master'

Re-instate is_admin flag in users API is current user is an admin

Closes #33260

See merge request !12211

6 files changed, 53 insertions, 4 deletions

diff --git a/changelogs/unreleased/33260-allow-admins-to-list-admins.yml b/changelogs/unreleased/33260-allow-admins-to-list-admins.yml
new file mode 100644
index 00000000000..a3b2053e005
--- /dev/null
+++ b/changelogs/unreleased/33260-allow-admins-to-list-admins.yml
@@ -0,0 +1,4 @@
+---
+title: Reinstate is_admin flag in users api when authenticated user is an admin
+merge_request: 12211
+author: rickettm
diff --git a/doc/api/users.md b/doc/api/users.md
index b1ebd7b0c47..cf09b8f44aa 100644
--- a/doc/api/users.md
+++ b/doc/api/users.md
@@ -62,6 +62,7 @@ GET /users
     "avatar_url": "http://localhost:3000/uploads/user/avatar/1/index.jpg",
     "web_url": "http://localhost:3000/john_smith",
     "created_at": "2012-05-23T08:00:58Z",
+    "is_admin": false,
     "bio": null,
     "location": null,
     "skype": "",
@@ -94,6 +95,7 @@ GET /users
     "avatar_url": "http://localhost:3000/uploads/user/avatar/2/index.jpg",
     "web_url": "http://localhost:3000/jack_smith",
     "created_at": "2012-05-23T08:01:01Z",
+    "is_admin": false,
     "bio": null,
     "location": null,
     "skype": "",
@@ -197,6 +199,7 @@ Parameters:
   "avatar_url": "http://localhost:3000/uploads/user/avatar/1/index.jpg",
   "web_url": "http://localhost:3000/john_smith",
   "created_at": "2012-05-23T08:00:58Z",
+  "is_admin": false,
   "bio": null,
   "location": null,
   "skype": "",
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 412443a2405..8bce79529e6 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -43,11 +43,14 @@ module API
       expose :external
     end
 
-    class UserWithPrivateDetails < UserPublic
-      expose :private_token
+    class UserWithAdmin < UserPublic
       expose :admin?, as: :is_admin
     end
 
+    class UserWithPrivateDetails < UserWithAdmin
+      expose :private_token
+    end
+
     class Email < Grape::Entity
       expose :id, :email
     end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 7257ecb5b67..bfb69d6dc18 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -59,7 +59,7 @@ module API
 
         users = UsersFinder.new(current_user, params).execute
 
-        entity = current_user.admin? ? Entities::UserPublic : Entities::UserBasic
+        entity = current_user.admin? ? Entities::UserWithAdmin : Entities::UserBasic
         present paginate(users), with: entity
       end
 
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index bc869ea1108..750682bde52 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -11,7 +11,7 @@ describe API::Users do
   let(:not_existing_user_id) { (User.maximum('id') || 0 ) + 10 }
   let(:not_existing_pat_id) { (PersonalAccessToken.maximum('id') || 0 ) + 10 }
 
-  describe "GET /users" do
+  describe 'GET /users' do
     context "when unauthenticated" do
       it "returns authentication error" do
         get api("/users")
@@ -76,6 +76,12 @@ describe API::Users do
 
         expect(response).to have_http_status(403)
       end
+
+      it 'does not reveal the `is_admin` flag of the user' do
+        get api('/users', user)
+
+        expect(json_response.first.keys).not_to include 'is_admin'
+      end
     end
 
     context "when admin" do
@@ -92,6 +98,7 @@ describe API::Users do
         expect(json_response.first.keys).to include 'two_factor_enabled'
         expect(json_response.first.keys).to include 'last_sign_in_at'
         expect(json_response.first.keys).to include 'confirmed_at'
+        expect(json_response.first.keys).to include 'is_admin'
       end
 
       it "returns an array of external users" do
diff --git a/spec/requests/api/v3/users_spec.rb b/spec/requests/api/v3/users_spec.rb
index e9c57f7c6c3..6d7401f9764 100644
--- a/spec/requests/api/v3/users_spec.rb
+++ b/spec/requests/api/v3/users_spec.rb
@@ -7,6 +7,38 @@ describe API::V3::Users do
   let(:email)   { create(:email, user: user) }
   let(:ldap_blocked_user) { create(:omniauth_user, provider: 'ldapmain', state: 'ldap_blocked') }
 
+  describe 'GET /users' do
+    context 'when authenticated' do
+      it 'returns an array of users' do
+        get v3_api('/users', user)
+
+        expect(response).to have_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+        username = user.username
+        expect(json_response.detect do |user|
+          user['username'] == username
+        end['username']).to eq(username)
+      end
+    end
+
+    context 'when authenticated as user' do
+      it 'does not reveal the `is_admin` flag of the user' do
+        get v3_api('/users', user)
+
+        expect(json_response.first.keys).not_to include 'is_admin'
+      end
+    end
+
+    context 'when authenticated as admin' do
+      it 'reveals the `is_admin` flag of the user' do
+        get v3_api('/users', admin)
+
+        expect(json_response.first.keys).to include 'is_admin'
+      end
+    end
+  end
+
   describe 'GET /user/:id/keys' do
     before { admin }