can't list users as a normal user (graph api)

[nirajacharya2]

Describe the bug

when listing the users as a normal user like marie it gives 401
https://owncloud.dev/libre-graph-api/#/users/ListUsers

Steps to reproduce

1. list users

curl -k -v "https://localhost:9200/graph/v1.0/users" -u marie:radioactivity

response:

> GET /graph/v1.0/users HTTP/1.1
> Host: localhost:9200
> Authorization: Basic bWFyaWU6cmFkaW9hY3Rpdml0eQ==
> User-Agent: curl/7.81.0
> Accept: */*
>
* TLSv1.2 (IN), TLS header, Supplemental data (23):
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* TLSv1.2 (IN), TLS header, Supplemental data (23):
* Mark bundle as not supporting multiuse
< HTTP/1.1 401 Unauthorized
< Content-Length: 156
< Content-Security-Policy: frame-ancestors 'none'
< Content-Type: application/json
< Date: Wed, 22 Nov 2023 10:33:41 GMT
< Vary: Origin
< X-Content-Type-Options: nosniff
< X-Frame-Options: DENY
< X-Graph-Version: 5.0.0-alpha.2+fc862cc18e
< X-Request-Id: neer-OptiPlex-3050/zwHJxt74Jy-001454
<
{"error":{"code":"accessDenied","innererror":{"date":"2023-11-22T10:33:41Z","request-id":"neer-OptiPlex-3050/zwHJxt74Jy-001455"},"message":"Unauthorized"}}
* Connection #0 to host localhost left intact

Expected behavior

marie should get the list of users

Actual behavior

marie gets denied

Setup

Please describe how you started the server and provide a list of relevant environment variables or configuration files.

OCIS_XXX=master

Additional context

Add any other context about the problem here.

[micbar]

This is an admin route. Needs admin permissions.

[individual-it]

@micbar how should a user then be able to share? For sharing a user needs ids of other user/groups

[micbar]

Good question.

@butonic we need a user search like on ocs, where you can start typing and get users/groups back.

[individual-it]

And then we need to change getUsers and getGroups in the PHP SDK to use that user search, or create new functions

[rhafer]

@butonic we need a user search like on ocs, where you can start typing and get users/groups back.

@micbar get graph/v1.0/users?$search=searchterm already allows that ($filter as well to some extent). I guess we mainly need to carefully define which attributes a "normal" user is allowed to see from other users. (And then lift the AdminOnly protection on that endpoint)

[individual-it]

Yes, from the usage (in the PHP SDK) that would be preferable, any user can query the endpoint, some get more some get less data.

[jvillafanez]

Basic plan:

1. Remove the "require admin" restriction from the graph/v1.0/users route
2. Check whether the user in the reva context (which I guess is the user making the request) has the admin role.
3. Implement the $select parameter (either not implemented right now, or I don't know how to make it work)
   1. If the user has the admin role and no "select" is being used, then show all user attributes (current behavior)
   2. If the user has the admin role and there are selected attributes, show only those attributes (attributes might be shown but they should be empty, need to check the behavior)
   3. If the user does NOT have the admin role and no "select" is being used, then show only the relevant attributes (to be decided which ones (*))
   4. If the user does NOT have the admin role and there are selected attributes, we'll use the relevant attributes (point 3.3) as base and select from those. This means that attributes outside of the relevant ones will be ignored and won't be shown.
4. (optional) Make the relevant attributes (point 3.3) configurable. I think this is out of scope for now, and we'll probably need to decide if it's worthy or not. Maybe for a follow up.

(*) Based on the user model in the libre-graph-api-go repo, I think we can use just the Id, DisplayName and Mail. The Surname and GivenName might be ok to be public, but I don't think they'll be useful.

Note that queries, filters and other stuff will remain without changes. In particular, regular users might still filter users by specific roles, or list users from any group (we'll likely need to implement additional restrictions to prevent this)

[micbar]

We need to make sure that we get this in before friday (feature freeze).

[jvillafanez]

I've uploaded code into the "normal_user_list" branch. Code changes can be seen in https://github.com/owncloud/ocis/compare/normal_user_list

That should cover points 1 and 2.
The select can be considered as hardcoded right now, so it will only show the id, displayname and the mail of the users regardless of what the user has requested (it needs additional work).

Note that specific filters for the regular users still needs to be implemented. Right now, any user can list every user in the system.

[rhafer]

@jvillafanez I think you plan looks great just a few comments:

Basic plan:

1. Remove the "require admin" restriction from the `graph/v1.0/users` route

2. Check whether the user in the reva context (which I guess is the user making the request) has the admin role.

Instead of just checking for the admin role I think it would be better to check if the user's role assigment has the Account Management permission. The Admin role check was a crutch for the beginning IMO.

Also we should restrict "normal" user to queries that contain a $search parameter of a certain length (e.g. at least 3 characters). All other queries ($filter, $expand, ...) should be forbidden for now IMO.

3. Implement the `$select` parameter (either not implemented right now, or I don't know how to make it work)

It's not implemented. But I don't think we need it currently.

   3. If the user does NOT have the admin role and no "select" is being used, then show only the relevant attributes (to be decided which ones (*))

☝️ This is the part we need IMO.

(*) Based on the user model in the libre-graph-api-go repo, I think we can use just the Id, DisplayName and Mail. The Surname and GivenName might be ok to be public, but I don't think they'll be useful.

👍

Note that queries, filters and other stuff will remain without changes. In particular, regular users might still filter users by specific roles, or list users from any group (we'll likely need to implement additional restrictions to prevent this)

All of this should stay limited to users with the Account Management permission for now IMO. As said above the only thing we should allow for unprivileged user is querys of the type graph/v1.0/users?$search=searchterm

[jvillafanez]

Unless I've missed something, it should be done:

• No changes for people with account management permissions (mostly admins I guess)
• Regular users will have the following limitations:
  • Only "id", "displayName" and "mail" will be shown per user.
  • "$search" option MUST be present in the query, with at least 3 characters
  • "$filter", "$apply", "$expand" and "$compute" options aren't allowed
• "$select" option is currently ignored for anyone.

I'll try to add some tests and make the "official" PR.

[rhafer]

Unless I've missed something, it should be done:

* No changes for people with account management permissions (mostly admins I guess)

* Regular users will have the following limitations:
  
  * Only "id", "displayName" and "mail" will be shown per user.
  * "$search" option MUST be present in the query, with at least 3 characters
  * "$filter", "$apply", "$expand" and "$compute" options aren't allowed

* "$select" option is currently ignored for anyone.

I'll try to add some tests and make the "official" PR.

Yes. That is the minimal stuff that's needed for the sharing NG story. We can think about how we add all the other bells and whistles later.

[micbar]

done.