Unable to clone a resource

[marthamareal]

Expected Behavior

All users non-admin and admins should be able to clone resources via the list option and SaveAs in view

Actual Behavior

• Non-admin users get 403 trying to clone a resource added by another user.(they can clone their own resource successfuly)
• Clone for some resources is not visible even if the requests returned 200. eg clone https://development.demo.geonode.org/catalogue/#/dataset/205 as admin

Steps to Reproduce the Problem

1. Login and view dataset eg https://development.demo.geonode.org/catalogue/#/dataset/205
2. Click on Save > SaveAs
3. Notice request fails with 403

Specifications

• GeoNode version: 4.x
• Installation method (manual, GeoNode Docker, SPCGeoNode Docker):
• Platform:
• Additional details:

[giohappy]

@mattiagiupponi somehow related to the new API permission checks?

[mattiagiupponi]

@mattiagiupponi somehow related to the new API permission checks?

I'll give a deeper check, but it may be possible. The user should have one of the specific permission on that resource

[mattiagiupponi]

Here is the analysis:
The UserHasPerm class will check if a pre-defined set of permissions are available on the resource for a specific user.

In this case, the user performs a PUT, and the class checks if the provided user has the EDIT_PERMISSIONS so in detail: ['base.change_resourcebase', 'base.change_resourcebasee_metadata']

In the case of CLONE, the user doesn't have the above perms on the resource, so the access is denied.

@giohappy we need to decide how to proceed here

[giohappy]

@mattiagiupponi we need a more fine-grained control for specific methods and requests. I don't like very much the usage of PUT here, but a part from this, what we need is a specific permission check that overrides the generic ones for PUT (which assumes edit permissions).
In the case of cloning the two required permissions area:

• add_resource (not bound to a resource), since you need the permission to create a new resource
• download_resourcebase (bound to the resource), since you need to have permission to download the resource, not only view it, to be able to clone it.

Ideally, we should have a configurable DjangoModelPermissions subclass, to be attached to the specific method.
Something like UserHasPerm(DjangoModelPermissions), with a constructor where we can pass the required permission specifiers.

[mattiagiupponi]

makes sense, lemme write a POC to see if may suit the needed

[mattiagiupponi]

@giohappy Follow a POC of a possible solution.

Permission classes attribute on the API will be something like this:

permission_classes=[
    IsAuthenticated, UserHasPerms(
        perms_dict={
            "PUT":['add_resource', 'download_resourcebase']
        }
    )
]

Where the perms list will be defined for each endpoint. The perms_dict is not mandatory, so we can always rely on the default permissions defined.
The UserHasPerms class will change as follow:

def has_permission(self, request, view):
        from geonode.base.models import ResourceBase

        queryset = self._queryset(view)
        perms = self.perms_dict.get(request.method, None) or self.get_required_permissions(request.method, queryset.model)
        .......

The main logic will be the same where:

• if the request is done on a single resource, the user must have one of the permissions defined for that method on the resource
• If the request is done on a listing, returns a queryset of objects for which a given user has all
  permissions present at perms.

How does it look @giohappy ?

[giohappy]

looks good to me

[marthamareal]

@mattiagiupponi, @giohappy For resources that don't have download permission, eg dashboard, geostory and map will still return 403

[mattiagiupponi]

That's true, only datasets can be downloaded. Since the cloning endpoint is common, we have three choices here:

1. Create a single endpoint for each resource and use the UserHasPerms to define the permissions needed for each endpoint (I don't like this solution, but better to also give this possibility)
2. Make the UserHasPerms even more granular, by defining the resource within the method, for example something like:

perms_dict= {
    "dataset" : {
        "PUT": ['add_resource', 'download_resourcebase']
    },
    "geoapp": {
        "PUT": ['add_resource']
    }
    .....
}

or having a default if the resource is not specified:

perms_dict= {
    "dataset" : {
        "PUT": ['add_resource', 'download_resourcebase']
    },
    "default": {
        "PUT": ['add_resource']
    }
    .....
}

3. Let only the "add_resourcebase" be the only permission required.

@marthamareal @giohappy any thoughts?

[giohappy]

@mattiagiupponi the second option looks the best to me.
For sure not the latter!

[marthamareal]

• download_resourcebase (bound to the resource), since you need to have permission to download the resource, not only view it, to be able to clone it.

@giohappy Is this something new to be implemented, because at the moment a user can clone with only View permission.

[mattiagiupponi]

• download_resourcebase (bound to the resource), since you need to have permission to download the resource, not only view it, to be able to clone it.

@giohappy Is this something new to be implemented, at the moment a user can clone with only View permission.

Indeed, we perform an ANY to let the user pass... converting it to an ALL may introduce other regression. I guess we need to talk about it or let the dict of the user perms define which operation is needed

[giohappy]

@mattiagiupponi yes, we definitely need to use granular rules for specific actions, and they will be defined with the dict.
If user were able to clone without download permissions it's the right moment to fix it ;)

[mattiagiupponi]

In the end, the permission class will look like the following:

permission_classes=[
            IsAuthenticated, UserHasPerms(
                perms_dict={
                    "dataset": {
                        "PUT": ['add_resourcebase', 'download_resourcebase'], "rule": all
                    },
                    "document": {
                        "PUT": ['add_resourcebase', 'download_resourcebase'], "rule": all
                    },
                    "default": {
                        "PUT": ['add_resourcebase']
                    }
                }
            )
        ]

• if the resource type is not defined, the "default" is used
• if the rule is not defined, the "any" is used