Review advanced workflow

[etj]

Advanced workflow should work as descripted in the issues #3564, #6551, #7135.

• review functionalities
• make sure permissions are assigned properly to all kind of resources
• implement test cases

Steps to Reproduce the Problem

Specifications

• GeoNode version: at least 3.3.x

[giohappy]

In my opinion the current RESOURCE_PUBLISHING, ADMIN_MODERATE_UPLOADS and GROUP_PRIVATE_RESOURCES are not enough to manage the cases in a clean way. They're neither orthogonal, nor ordered in terms of strictness. Moreover their combination can lead to hard to follow (and implement!) logic, which is spread inside several modules, tasks and views.

The proposal is to destructure the advanced workflow into a set of configurations, which cover the current logic and other conflicting cases that cannot be implemented now.
The following two points are at the base of the proposal:

• By default the not published state should only be considered the same as draft.. In this moment the PUBLISHED state mix of concepts, since it's used both in the meaning of "draft" / "completed" resource, but it also drives some automations about its public visibility (permissions). This automation should be stated explaicitely through configurations.
• All the workflow logic should only be driven by permissions. We already have the "publish_resourcebase" permissions (but it's not used coherently inside the code!). A new "approve_resourcebase" will be created.

The following 'free' settings would replace (and extend the flexibility) RESOURCE_PUBLISHING, ADMIN_MODERATE_UPLOADS and GROUP_PRIVATE_RESOURCES

'ADD_ANONYMOUS_PERMS_ON_PUBLISHED': <boolen> // automatically add visibility and downlaod permissions to published resources
'ADD_ANONYMOUS_PERMS_ON_APPROVED': <boolen> // automatically add visibility and downlaod permissions to completed resources
'RESOURCE_DEFAULTS': {
	'ANONYMOUS_PERMS': <list of perms>, // notice that this will 
	'OWNER_PERMS': <list of perms>,  // if we give only visibility we obtain the current ADMIN_MODERATE_UPLOADS effect (permissions panels not visibile to onwers)
	'OWNER_GROUPS_PERMS': <list of perms>,
	'RESOURCE_GROUP_PERMS': <list of perms>,
	'MANAGERS_PERMS': <list of perms>,
	'IS_APPROVED': <boolean>,
	'IS_PUBLISHED': <boolean>,
}

The following example is a GeoNode instance without an active advanced workflow:

'ADD_ANONYMOUS_PERMS_ON_PUBLISHED': False
'ADD_ANONYMOUS_PERMS_ON_APPROVED': False
'RESOURCE_DEFAULTS': {
	'ANONYMOUS_PERMS': [],
	'OWNER_PERMS': <all permissions including, publish_resourcebase and approve_resourcebase>,
	'OWNER_GROUPS_PERMS': [],
	'RESOURCE_GROUP_PERMS': [],
	'MANAGERS_PERMS': [],
	'IS_APPROVED': True,
	'IS_PUBLISHED': True,
}

The following example implements what is descrived in the docs when RESOURCE_PUBLISHING, ADMIN_MODERATE_UPLOADS and GROUP_PRIVATE_RESOURCES are all True

'ADD_ANONYMOUS_PERMS_ON_PUBLISHED': False
'ADD_ANONYMOUS_PERMS_ON_APPROVED': False
'RESOURCE_DEFAULTS': {
	'ANONYMOUS_PERMS': [],
	'OWNER_PERMS': [view_resourcebase, download_resourcebase, change_resourcebase, change_resourcebase_metadata],  // if we give only visibility we obtain the current ADMIN_MODERATE_UPLOADS effect (permissions panels not visibile to onwers)
	'OWNER_GROUPS_PERMS': [view_resourcebase, download_resourcebase],
	'RESOURCE_GROUP_PERMS': [view_resourcebase, download_resourcebase],
	'MANAGERS_PERMS': [view_resourcebase, download_resourcebase, change_resourcebase, change_resourcebase_metadata, complete_resourcebase, approve_resourcebase], // no change_resourcebase_permissions
	'IS_APPROVED': False,
	'IS_PUBLISHED': False,
}

Notice that RESOURCE_DEFAULTS.ANONYMOUS_PERMS would replace / set the current DEFAULT_ANONYMOUS_VIEW_PERMISSION and DEFAULT_ANONYMOUS_DOWNLOAD_PERMISSION settings.

@mattiagiupponi we should quicly analyse the impact of such a refactoring, taking also into account the migration of an instance with the advanced workflow already in place.
@etj @afabiani opinions?

[afabiani]

It is better to use some "acronyms" to describe the list of permissions similar to the compact perm_spec, otherwise we will face a set of possible issues, like:

1. We might miss some specific permissions like in the case of the Layers/Datasets
2. It would be very easy to make some mistakes writing all those long perms lists
3. It is hard to remember all the available perms everytime

Also, we should consider getting rid of those two options currently available on settings, as you already stated in the NOTE at the end.

# Whether the uplaoded resources should be public and downloadable by default
# or not
DEFAULT_ANONYMOUS_VIEW_PERMISSION = ast.literal_eval(
    os.getenv('DEFAULT_ANONYMOUS_VIEW_PERMISSION', 'True')
)
DEFAULT_ANONYMOUS_DOWNLOAD_PERMISSION = ast.literal_eval(
    os.getenv('DEFAULT_ANONYMOUS_DOWNLOAD_PERMISSION', 'True')
)

Overall, this adds more flexibility for sure, but it looks to me quite hard for a user to understand.

[etj]

About point 2, still wondering why we don't have constants or an enum for the permissions set (talking about 32x and 33x of course)

[giohappy]

It is better to use some "acronyms" to describe the list of permissions similar to the compact perm_spec, otherwise we will face a set of possible issues

I see your point @afabiani. We could use and extend the namings we have introduced for the master branch.
However, for 3.3.x, I would only use them as aliases for the settings.

[mattiagiupponi]

So, in a nutshell:

• Find a way to translate RESOURCE_PUBLISHING / ADMIN_MODERATE_UPLOADS into a permissions JSON
• Creation of new set of settings RESOURCE_DEFAULTS, ADD_ANONYMOUS_PERMS_ON_APPROVED, ADD_ANONYMOUS_PERMS_ON_PUBLISHED
• Replace in the code of the old settings related to the advanced workflow: RESOURCE_PUBLISHING / ADMIN_MODERATE_UPLOADS
• Creation of new permission: approve_resourcebase
• refactoring of set_default_permissions and set_permissions under a common method that will be able to handle to above configurations
• Migration file to:
  • assign the new permission to the GroupManager when the advanced workflow is enabled
  • assign the new permission to the ResourceOwner when the advanced workflow is disabled
• fix accordingly the get_visible_resource

[giohappy]

@mattiagiupponi the controls for the disabling of editing by the owner when the advanced workflow is active should also be adaptet.

[giohappy]

@etj I move this to inbox and tag it for master only.

[etj]

@giohappy the advanced workflow should be fixed in 3.3.x also. If we need a deeper refactoring in master branch, let's open a new issue for that.

[etj]

Reviewing the PR against 3.3.x.

First set of test have be run with a vanilla instance, no setting changed.

The behaviour seems to be ok when the resource is initially created; when the metadata is edited and a group is assigned, all permission on the resource are granted to the group managers, which is not in the desired behaviour.

A little research showed that permission are set over and over also in HTTP GET calls, for instance:

• During a document view, set_owner_permissions_according_to_workflow() is explicitly called:

  geonode/geonode/documents/views.py

  Lines 100 to 101 in 3aa47d3

  	permission_manager = ManageResourceOwnerPermissions(document)
  	permission_manager.set_owner_permissions_according_to_workflow()

• in the same view a _perms_info_json() function is called

  geonode/geonode/documents/views.py

  Line 119 in 3aa47d3

  permissions_json = _perms_info_json(document)

  
  which calls a _perms_info() function

  geonode/geonode/security/views.py

  Line 50 in 3aa47d3

  info = _perms_info(obj)

  
  which calls a get_all_level_info() method

  geonode/geonode/security/views.py

  Line 45 in 3aa47d3

  info = obj.get_all_level_info()

  
  which assign permssions here
  

  geonode/geonode/security/models.py

  Line 98 in 3aa47d3

  assign_perm(perm, manager, resource)

  
  and here
  

  geonode/geonode/security/models.py

  Line 112 in 3aa47d3

  assign_perm(perm, manager, resource)

Now, I see a few problems here:

1. HTTP GET calls should NOT change the DB, if not for increasing page hits and similar
2. get_xxx functions should NOT have such kind of side effects.
3. Permission settings should be done after specific actions in the workflow, and not randomly during GET pages
   3a) Permission settings should not be called over and over and over

[etj]

You may find useful this query to retrieve the perms assigned to the various resources:

select title, name, permname from
(
	(select rb.title as title, pp.username as name,ap.name as permname 
	from guardian_userobjectpermission gup, auth_permission ap, base_resourcebase rb,people_profile pp 
	where cast(gup.object_pk as integer) = rb.id
	and gup.permission_id =ap.id 
	and gup.user_id =pp.id 
	order by title,username,ap.name)
union all 
	select rb.title, '[' || ag."name"||']' as name, ap.name as permname 
	from guardian_groupobjectpermission gup, auth_permission ap, base_resourcebase rb, auth_group ag 
	where cast(gup.object_pk as integer) = rb.id
	and gup.permission_id =ap.id 
	and gup.group_id  = ag.id 
	order by title,name,permname
) as xxx
--where title like '%1000%'
order by title,name,permname

[etj]

Copying and modifyng the comment in the PR:

Scenario 1: Default values: AUTO PUBLISH

• RESOURCE_PUBLISHING = False
ADMIN_MODERATE_UPLOADS = False

• When user creates a resource

  • OWNER gets all the owner permissions (publish resource included)
  • ANONYMOUS can view and download

Scenario 2: SIMPLE PUBLISHING

• RESOURCE_PUBLISHING = True (Autopublishing is disabled)
  ADMIN_MODERATE_UPLOADS = False

• When user creates a resource

  • OWNER gets all the owner permissions (publish_resource and change_resourcebase_permissions INCLUDED)
  • Group MANAGERS of the user's groups will get the owner permissions (publish_resource EXCLUDED)
  • Group MEMBERS of the user's groups will get the view_resourcebase permission
  • ANONYMOUS can not view and download if the resource is not published

• When resource has a group assigned:

  • OWNER gets all the owner permissions (publish_resource and change_resourcebase_permissions INCLUDED)
  • Group MANAGERS of the resource's group will get the owner permissions (publish_resource EXCLUDED)
  • Group MEMBERS of the resource's group will get the view_resourcebase permission

Scenario 3: ADVANCED WORKFLOW

• RESOURCE_PUBLISHING = True
ADMIN_MODERATE_UPLOADS = True

• When user creates a resource

  • OWNER gets all the owner permissions (publish_resource and change_resourcebase_permissions EXCLUDED)
  • Group MANAGERS of the user's groups will get the owner permissions (publish_resource INCLUDED)
  • Group MEMBERS of the user's groups will get the view_resourcebase permission
  • ANONYMOUS can not view and download if the resource is not published

• When resource has a group assigned:

  • OWNER gets all the owner permissions (publish_resource and change_resourcebase_permissions EXCLUDED)
  • Group MANAGERS of the resource's group will get the owner permissions (publish_resource INCLUDED)
  • Group MEMBERS of the resource's group will get the view_resourcebase permission

Scenario 4: SIMPLE WORKFLOW

• RESOURCE_PUBLISHING = False
ADMIN_MODERATE_UPLOADS = True

• NOTE: Is it even possibile? when the resource is automatically published, can it be un-published?
  If this combination is not allowed, we should either stop the process when reading the settings or log a warning and force a safe combination.

• When user creates a resource

  • OWNER gets all the owner permissions (publish_resource and change_resourcebase_permissions INCLUDED)
  • Group MANAGERS of the user's groups will get the owner permissions (publish_resource INCLUDED)
  • Group MEMBERS of the user's group will get the view_resourcebase permission
  • ANONYMOUS can view and download

Recap:

• OWNER can always publish, except in the ADVANCED WORKFLOW
• Group MANAGERS have publish privs only when ADMIN_MODERATE_UPLOADS is True
• Group MEMBERS have always access to the resource, except for the AUTOPUBLISH, where everybody has access to it.

[giohappy]

@etj I think you gathered all the current scenarios. BTW we should also include the effects on visibility for anonymous users.
In Scenario 1 "Any" (anonymous or registered) are assigned view_resourcebase and download_resourcebase permissions automatically.
In Scenario 2, if I remember correctly, "Any" can see the resource even if unpublished, correct me if I'm wrong.
In Scenario 3 the resource is visiable to "Any" only after being approved.

We should review this logic too. The doubt is if we should assign / unassign permissions when the resource status change, or if the RESOURCE_PUBLISHING and ADMIN_MODERATE_UPLOADS should be taken into account dynamically when evaluating the final permissions. My proposal @etj, for other similar scenarios, was to use "virtual permissions", i.e. make the security module calculate the final permissions based on context. For example when the READ ONLY global configuration is set, the permissions for ALL users filter out any change_* permissions.

[etj]

@giohappy Added descr for "anonymous" privs.
I'm against rewriting perms on status change
Of course the final authorization is computed according to settings, user privs and resource status.

[gannebamm]

About Scenario 3: ADVANCED WORKFLOW

If we have https://docs.geonode.org/en/master/basic/settings/index.html#group-private-resources the 'publishing' should not make the resource publicly available but only get rid of the 'pending approval' label. This one:

Not that one:
https://docs.geonode.org/en/master/admin/admin_panel/index.html#id58

[giohappy]

@gannebamm "pending approval" depends on the approved flag, it's not related to the visibility to private groups.