owncloud / ocis

:atom_symbol: ownCloud Infinite Scale Stack
https://doc.owncloud.com/ocis/next/
Apache License 2.0
1.39k stars 182 forks source link

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

Closed nirajacharya2 closed 10 months ago

nirajacharya2 commented 11 months ago

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.

```console OCIS_XXX=master ```

Additional context

Add any other context about the problem here.

micbar commented 11 months ago

This is an admin route. Needs admin permissions.

individual-it commented 11 months ago

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

micbar commented 11 months ago

Good question.

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

individual-it commented 11 months ago

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

rhafer commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

jvillafanez commented 11 months ago

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 commented 11 months ago

@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 (*))

:point_up: 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.

:+1:

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 commented 11 months ago

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

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

rhafer commented 11 months ago

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 commented 10 months ago

done.