geonode icon indicating copy to clipboard operation
geonode copied to clipboard

Create moderators group and permissions for publishing and approval

Open marthamareal opened this issue 3 years ago • 6 comments

Expected Behavior

We want to introduce the moderators group, which will be used as a "role" like we have done for contributors.

Two new permissions approve_resourcebase and publish_resorcebase will be created and assigned to the moderators group. Users assigned to this group will have the permissions to publish and/or approve any resource.

Documentation for the advanced workflow must be updated accordingly.

Actual Behavior

The current advanced workflow system rely on admins and group managers to grant permissions for the approval and publishing of resources (approve and publish metadata flags).

Steps to Reproduce the Problem

Specifications

  • GeoNode version: master
  • Installation method (manual, GeoNode Docker, SPCGeoNode Docker):
  • Platform:
  • Additional details:

marthamareal avatar Jul 09 '21 09:07 marthamareal

I report below the description from our own internal task.

The idea is to centralize the permission checks for the advanced workflow under the security module, and check wether a user has publishing / moderating permissions.

This method to check the moderator permissions will return true if the user:

Something similar for the check of permissions to publish. User can publish if:

  • is an admin
  • has publish_resources permissions
  • he has the (already implemented) publish_resourcebase permission on the resource. This resource specific permission could probably be dropped since, AFAIK, only an admin can publish if ADMIN_MODERATE_UPLOADS is active.

Long story short, we want to continue the work of basing the security logic on groups and permissions, rather then specific users'attributes (group manager, memeber of a group, etc.).

Some highlights of the critical points where the logic has been applied:

At client side there are few places where the legacy views disable the form checkboxes accordingly, e.g.:

NOTE: a new issue will be opened afterwards to make the REST API v2 aware of this logic, and drive the client functionalities accordingly. The client shouldn't be aweare of the underlying workflows. The backend will send back the user permissions based on the underlying logic. So, for example, id the resource has been approved (and the advanced workflow is active) the returned permissions on the resource for any other user shouldn't include the edit permissions.

giohappy avatar Sep 02 '21 10:09 giohappy

@giohappy, Given the advanced workflow does this mean, that on creating the moderators group, should we just assign them only the approve_resources perm?

marthamareal avatar Sep 06 '21 10:09 marthamareal

Some clarifications. The steps are

  1. create the new moderators group, create the new approve_resources and publish_resources and assign these permissions to the new group.
  2. create a new approve_resourcebase permissions, along with the already existing publish_resourcebase permission
  3. Adapt the security.utils methods to keep into account moderators when returning the uer permissions on a resource (see below)
  4. Adapt GeoNode views where the advanced workflows settings are checked to enable / disable the approves and is published fields (see below)

"Virtual" permissions

When permissions are retrieved for a resource (get_visible_resources, get_resources_with_perms, get_users_with_perms, resolve_object, and so forth), the final permissions will be returned either if the user has those permissions directly assigned or if the user has global permissions that apply to the case.

Currently the group manager of the group assigned to the resource has change_resourcebase_metadata assigned. This assignment is triggered by changes to the group's manager or the group assigned to the resource. So, permissions for this user are returned automatically when we check the user permissions on a resource.

On the other side, the new moderators won't be assigned direct permissions on resources. They will get "virtual" permissions from being moderators. This means that the methods that rely on permissions (spread all over GeoNode!) should determine and return not only the "real" permissions of a user but also "virtual" permissions. This is one of the reasons we want to remove security code duplication the most as possible, and move it under the security module.

As an example, when we retriev the permissions for a resource we should add the following case:

If user has approve_resources or user has publish_resources, for any resource the following permissions will be returned:

  • view_resourcebase
  • download_resourcebase
  • change_resoucebase_metadata
  • approve_resourcebase
  • publish_resourcebase

Notice that if we implement this approach we gen get ridd of the assignment of real permissions for the group managers.

@afabiani please give your opinion on this, since it goes hand in hand of our recent discussion on a similar topic for compact permissions.

Can approve and can publish conditions

In the end the following conditions must be implemented:

if the user is "group manager for the resources" or user.perms.get("approve_resources") or is admin: -> return approve_resourcebase for the user

user.perms.get("publish_resources") or is admin: -> return publish_resourcebase for the user

User can_approve and can_publish

We want to have a new people.Profile.can_approve and people.Profile.can_publish methods (which will rely on the security utils) to check if the user

{resourcetype}_metadata views check wether to make the is_published and approved checkboxes disabled or not, dependeing on the advanced workflow settings and the user permissions. We want to remove this duplication of code and move the checks inside two new people.Profile.can_approve and people.Profile.can_publish methods, that will rely on the security utils.

For example, the following lines https://github.com/GeoNode/geonode/blob/9a1784071ffc65a98ab19019b172564dff8a5b2e/geonode/layers/views.py?plain=1#L1116-L1131

if settings.ADMIN_MODERATE_UPLOADS:
	if not request.user.is_superuser:
		can_change_metadata = request.user.has_perm(
			'change_resourcebase_metadata',
			layer.get_self_resource())
		try:
			is_manager = request.user.groupmember_set.all().filter(role='manager').exists()
		except Exception:
			is_manager = False

		if not is_manager or not can_change_metadata:
			if settings.RESOURCE_PUBLISHING:
				dataset_form.fields['is_published'].widget.attrs.update(
					{'disabled': 'true'})
			dataset_form.fields['is_approved'].widget.attrs.update(
				{'disabled': 'true'})

shoild become something like this:

if not user.can_appprove(resource):
        if not user.can_publish(resource):
            dataset_form.fields['is_published'].widget.attrs.update(
                {'disabled': 'true'})
        dataset_form.fields['is_approved'].widget.attrs.update(
            {'disabled': 'true'})

giohappy avatar Sep 06 '21 14:09 giohappy

@giohappy I agree with your analysis.

I also want to highlight that part of this task will be to improve and update the set_workflow_perms paths accordingly, in order to recognize the Group Managers and assign them to the correct Authority Groups. We should also remove them whenever they are demoted.

ResourceBase inherits the set_workflow_perms method from the Security model

https://github.com/GeoNode/geonode/blob/837da7c218d22251e1ba30ddbcc5b0160ef1fbbf/geonode/security/models.py#L144

The latter completely relies on the ResourceManager which:

  1. Checks for the managers and assigns the permissions accordingly https://github.com/GeoNode/geonode/blob/837da7c218d22251e1ba30ddbcc5b0160ef1fbbf/geonode/resource/manager.py#L700

  2. Invokes the Concrete Resource Manager in order to finalize the process (in this case sets the GeoServer rules) https://github.com/GeoNode/geonode/blob/837da7c218d22251e1ba30ddbcc5b0160ef1fbbf/geonode/geoserver/manager.py#L507

afabiani avatar Sep 07 '21 08:09 afabiani

@afabiani on second thouth I have a doubt regarding "moderators". In my plan users having this role (Django Group assigned) will should have the following permissions:

  • view_resourcebase
  • download_resourcebase
  • change_resoucebase_metadata
  • approve_resourcebase
  • publish_resourcebase

The idea was to make these permissions virtual, or dynamic, instead of actually assigning the permissions on the resources to the single users. By the way this implies that Geofence rules for those Django groups should be managed accordingly. Questions:

  • does GeoNode return Django Groups (not GeoNode GroupProfiles!) to the Geoserver Role service?
  • how complex would it be to manage (GeoNode side) the rules for Django Groups?

giohappy avatar Sep 07 '21 09:09 giohappy

@giohappy currently the GeoFence rules relies already on Django Group slugs, not on GeoNode GroupProfiles.

afabiani avatar Sep 07 '21 13:09 afabiani