Patch #29459
closedAdd admin flag to users API
Description
Currently, it is not possible to distinguish admin users from "normal" users via the API. This patch adds the admin
flag to the users API. The flag is added to the index action (visible only to admins) and on the show action to admins only.
Note that due to a peculiarity of the API builder, the field is only included in JSON responses if the value is true
. For XML, it is included with true
and false
values respectively. With the JSON API, it is thus not possible to distinguish a list of non-admin users from a list of users without the permission to see the admin status.
Files
Related issues
Updated by Go MAEDA almost 6 years ago
- Target version changed from Candidate for next minor release to 4.1.0
Updated by Go MAEDA almost 6 years ago
IMHO, it is better to remove '|| (User.current == @user)
' from the patch. Non-admin users can know whether they are admin or not by accessing /users.(xml|json)
.
Index: app/views/users/show.api.rsb
===================================================================
--- app/views/users/show.api.rsb (revision 17471)
+++ app/views/users/show.api.rsb (working copy)
@@ -1,6 +1,7 @@
api.user do
api.id @user.id
api.login @user.login if User.current.admin? || (User.current == @user)
+ api.admin @user.admin? if User.current.admin? || (User.current == @user)
api.firstname @user.firstname
api.lastname @user.lastname
api.mail @user.mail if User.current.admin? || !@user.pref.hide_mail
When a non-admin user gets their information via XML API, the response contains an '<admin>' element and they can know that they are non-admin user by seeing the value. But when a non-admin user accesses JSON API, it is impossible to make sure that whether they are admin or not because the response does not have 'admin' key, as you already mentioned. The behavior and specification will be different between XML and JSON. I think it is confusing.
Updated by Holger Just almost 6 years ago
Go MAEDA wrote:
IMHO, it is better to remove '
|| (User.current @user)
' from the patch. Non-admin users can know whether they are admin or not by accessing/users.(xml|json)
.
Unfortunately, only admin users can access /users.(xml|json)
. The endpoint is not accessible to "normal" users at all. Even in the web UI, there is currently no possibility for a non-admin user to find which users have admin permissions. Thus, I think there is a general need for any kind of check to restrict this information to only authorized persons.
For the API, I think the most common use-case for this added flag is for an API client to find whether its given credentials grant them admin permissions. If we remove the check for || (User.current @user)
, this check will be sort-of possible still, since admin users will still return true
in /users/current.(json|xml)
here while non-admin users will not contain the field at all (both in JSON and XML). Deoending on the client, this might make it a bit awkward to distinguish between a true value and a missing tag, but could probably be made to work.
If we retain the check, for XML, we will have a true
or false
value in the output for the current user, making it more consistent here. It will still not include the field for other users.
JSON will still only include the field if the value is true
(which I think is a bug in the API builder which should be fixed separately). However, I think JSON clients are usually better equipped to handle "fluid" responses than XML clients.
Thus, in the end, I still think that it might a bit more consistent to include the field for the current user (esp. if or when we get to fix this API builder issue), handling the field the same way as the other filtered user fields. But I don't want to fight over this. If you think it should be removed to only show the field to admin users in any case, I can live with that too.
In both cases, you the following rule holds:
- If the
admin
field is present and contains atrue
value, the user is an admin - If you are querying
users/current.(xml|json)
and theadmin
field is missing orfalse
, the current user is not an admin - If you are querying a different user and the field is not included, you don't know anything.
Updated by Go MAEDA almost 6 years ago
- Status changed from New to Closed
- Assignee set to Go MAEDA
- Target version changed from 4.1.0 to 4.0.0
Committed. Thank you for sharing the patch.
Updated by Holger Just almost 6 years ago
- Status changed from Closed to Reopened
- Assignee changed from Go MAEDA to Holger Just
Thank you for committing the patch.
I will update the REST Users API documentation page accordingly.
Updated by Marius BĂLTEANU almost 6 years ago
- Status changed from Reopened to Closed
- Assignee changed from Holger Just to Go MAEDA
Holger Just wrote:
Thank you for committing the patch.
I will update the REST Users API documentation page accordingly.
I've created a new ticket to track the documentation update. Please see #29871.
Updated by Marius BĂLTEANU almost 6 years ago
- Related to Feature #29871: Add admin flag to REST Users API documentation added