GeoNode / geonode

GeoNode is an open source platform that facilitates the creation, sharing, and collaborative use of geospatial data.
https://geonode.org/
Other
1.46k stars 1.13k forks source link

User defined permissions will break the permissions exposed by the API #12639

Closed ridoo closed 1 month ago

ridoo commented 1 month ago

Expected Behavior

Create and add custom permissions to a user (via group) does not have any effect on handling user permissions.

Actual Behavior

Adding new permissions to a user, which are not known by GeoNode, raises an exception while mapping permissions in people/models.py#perms. As a result the API skips the perms attribute completely. With the API skipping that attribute, geonode-mapstore-client sees no permissions given to a user. For example a user who has actual contributor permissions, will not have the ability to (for example) add new resources.

Steps to Reproduce the Problem

See my discussion about custom permissions to reproduce.

  1. Add new user (id 1001) to contributor group
  2. Check /api/v2/users/1001 contains perms attribute
  3. Add a custom permission to a group a user belongs to
  4. Check /api/v2/users/1001 is missing perms attribute

Specifications

ridoo commented 1 month ago

@giohappy @mattiagiupponi the solution seems as easy as doing

# return constant names defined by GeoNode
#perms = [PERMISSIONS[db_perm] for db_perm in group_perms]
perms = group_perms

~but I am unsure, if that mapping has a specific reason~ In any case the mapping should stay the same but could be changed to allow unknown permissions. I tested locally, and did not see any other things break on geonode-mapstore-client side.

Do you want me to create a PR for it?

However, looking at the whole method

https://github.com/GeoNode/geonode/blob/677c5783112b92581177237f5412e5a2d4263176/geonode/people/models.py#L199-L216

it seems that staff users do not get extra permissions at all. Is this intended?

kikislater commented 1 month ago

Interesting ! Just to link issues bout staff: https://github.com/GeoNode/geonode/issues/12551

ridoo commented 1 month ago

BTW: The perms attribute is also skipped when a user gets non-custom permissions like adding site.

@giohappy @mattiagiupponi the solution seems as easy as doing

# return constant names defined by GeoNode
#perms = [PERMISSIONS[db_perm] for db_perm in group_perms]
perms = group_perms

This quick guess omits the actual mapping. However, I created a PR which adds all permissions (even for admin and staff).

giohappy commented 1 month ago

@ridoo I think it's fine to include custom permissions here. GeoNode only tracks the permissions it is aware of because there's a lot of logic that depends on the exact names of permissions. See for example all the methods under the security module. By the way, in this specific case, I don't see any problem. If I'm not wrong that property is only read by the API serializer, so it shouldn't interfere with the rest.