From 68249eb0c96f4508aa6bb8854fdfa3fce4e4c3f7 Mon Sep 17 00:00:00 2001 From: mattiagiupponi <51856725+mattiagiupponi@users.noreply.github.com> Date: Tue, 8 Nov 2022 11:37:00 +0100 Subject: [PATCH] [Fixes #10214] metadata_only filter not working properly (#10215) * [Fixes #10214] metadata_only filter not working properly * [Fixes #10214] metadata_only filter not working properly * [Fixes #10214] metadata_only filter not working properly Co-authored-by: Alessio Fabiani --- geonode/base/api/permissions.py | 35 +++++++++++----------- geonode/base/api/tests.py | 51 +++++++++++++++++++-------------- geonode/security/tests.py | 32 +++++++++++++++++++++ geonode/security/utils.py | 8 ++++-- 4 files changed, 83 insertions(+), 43 deletions(-) diff --git a/geonode/base/api/permissions.py b/geonode/base/api/permissions.py index 24c76cb2ea6..24764d91ae9 100644 --- a/geonode/base/api/permissions.py +++ b/geonode/base/api/permissions.py @@ -17,16 +17,17 @@ # ######################################################################### import logging +from django.conf import settings from django.contrib.auth import get_user_model from django.shortcuts import get_object_or_404 from rest_framework import permissions from rest_framework.filters import BaseFilterBackend from geonode.security.permissions import BASIC_MANAGE_PERMISSIONS, DOWNLOAD_PERMISSIONS, EDIT_PERMISSIONS, VIEW_PERMISSIONS - +from distutils.util import strtobool from geonode.security.utils import ( get_users_with_perms, - get_resources_with_perms) + get_visible_resources) from geonode.groups.models import GroupProfile from rest_framework.permissions import DjangoModelPermissions from guardian.shortcuts import get_objects_for_user @@ -194,26 +195,22 @@ class ResourceBasePermissionsFilter(BaseFilterBackend): A filter backend that limits results to those where the requesting user has read object level permissions. """ - shortcut_kwargs = { - 'accept_global_perms': True, - } def filter_queryset(self, request, queryset, view): - # We want to defer this import until runtime, rather than import-time. - # See https://github.com/encode/django-rest-framework/issues/4608 - # (Also see #1624 for why we need to make this import explicitly) - - user = request.user - # perm_format = '%(app_label)s.view_%(model_name)s' - # permission = self.perm_format % { - # 'app_label': queryset.model._meta.app_label, - # 'model_name': queryset.model._meta.model_name, - # } - - obj_with_perms = get_resources_with_perms(user, shortcut_kwargs=self.shortcut_kwargs) - logger.debug(f" user: {user} -- obj_with_perms: {obj_with_perms}") - return queryset.filter(id__in=obj_with_perms.values('id')) + try: + metadata_only = strtobool(request.query_params.get("filter{metadata_only}", "None")) + except Exception: + metadata_only = None + + return get_visible_resources( + queryset, + request.user, + metadata_only=metadata_only, + admin_approval_required=settings.ADMIN_MODERATE_UPLOADS, + unpublished_not_visible=settings.RESOURCE_PUBLISHING, + private_groups_not_visibile=settings.GROUP_PRIVATE_RESOURCES + ) class UserHasPerms(DjangoModelPermissions): diff --git a/geonode/base/api/tests.py b/geonode/base/api/tests.py index 66c1d8a7347..1f0bfdb9c59 100644 --- a/geonode/base/api/tests.py +++ b/geonode/base/api/tests.py @@ -436,6 +436,13 @@ def test_base_resources(self): response = self.client.get(url, format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) + self.assertEqual(response.data['total'], 28) + + url = "{base_url}?{params}".format(base_url=reverse('base-resources-list'), params="filter{metadata_only}=false") + # Anonymous + response = self.client.get(url, format='json') + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.data), 5) self.assertEqual(response.data['total'], 26) response.data['resources'][0].get('executions') # Pagination @@ -494,7 +501,7 @@ def test_base_resources(self): # Admin self.assertTrue(self.client.login(username='admin', password='admin')) - response = self.client.get(f"{url}?page_size=17", format='json') + response = self.client.get(f"{url}&page_size=17", format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) self.assertEqual(response.data['total'], 26) @@ -505,25 +512,27 @@ def test_base_resources(self): resource = ResourceBase.objects.filter(owner__username='bobby').first() self.assertEqual(resource.owner.username, 'bobby') # Admin - response = self.client.get(f"{url}/{resource.id}/", format='json') + url_with_id = "{base_url}/{res_id}?{params}".format(base_url=reverse('base-resources-list'), res_id=resource.id, params="filter{metadata_only}=false") + + response = self.client.get(f"{url_with_id}", format='json') self.assertEqual(response.data['resource']['state'], enumerations.STATE_PROCESSED) self.assertEqual(response.data['resource']['sourcetype'], enumerations.SOURCE_TYPE_LOCAL) self.assertTrue('change_resourcebase' in list(response.data['resource']['perms'])) # Annonymous self.assertIsNone(self.client.logout()) - response = self.client.get(f"{url}/{resource.id}/", format='json') + response = self.client.get(f"{url_with_id}", format='json') self.assertFalse('change_resourcebase' in list(response.data['resource']['perms'])) # user owner self.assertTrue(self.client.login(username='bobby', password='bob')) - response = self.client.get(f"{url}/{resource.id}/", format='json') + response = self.client.get(f"{url_with_id}", format='json') self.assertTrue('change_resourcebase' in list(response.data['resource']['perms'])) # user not owner and not assigned self.assertTrue(self.client.login(username='norman', password='norman')) - response = self.client.get(f"{url}/{resource.id}/", format='json') + response = self.client.get(f"{url_with_id}", format='json') self.assertFalse('change_resourcebase' in list(response.data['resource']['perms'])) # Check executions are returned when deffered # all resources - response = self.client.get(f'{url}?include[]=executions', format='json') + response = self.client.get(f'{url}&include[]=executions', format='json') self.assertEqual(response.status_code, 200) self.assertIsNotNone(response.data['resources'][0].get('executions')) # specific resource @@ -555,7 +564,7 @@ def test_base_resources(self): ) }] self.assertTrue(self.client.login(username='bobby', password='bob')) - response = self.client.get(f'{url}/{resource.id}?include[]=executions', format='json') + response = self.client.get(f'{url_with_id}&include[]=executions', format='json') self.assertEqual(response.status_code, 200) self.assertIsNotNone(response.data['resource'].get('executions')) self.assertEqual(response.data['resource'].get('executions'), expected_executions_results) @@ -567,7 +576,7 @@ def test_base_resources(self): self.assertEqual(6, resource.tkeywords.count()) # Admin self.assertTrue(self.client.login(username='admin', password='admin')) - response = self.client.get(f"{url}/{resource.id}/", format='json') + response = self.client.get(f"{url_with_id}", format='json') self.assertIsNotNone(response.data['resource']['tkeywords']) self.assertEqual(6, len(response.data['resource']['tkeywords'])) self.assertListEqual( @@ -721,7 +730,7 @@ def test_filter_resources(self): self.assertTrue(self.client.login(username='admin', password='admin')) # Filter by owner == bobby - response = self.client.get(f"{url}?filter{{owner.username}}=bobby", format='json') + response = self.client.get(f"{url}?filter{{owner.username}}=bobby&filter{{metadata_only}}=false", format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) self.assertEqual(response.data['total'], 3) @@ -729,7 +738,7 @@ def test_filter_resources(self): self.assertEqual(len(response.data['resources']), 3) # Filter by resource_type == document - response = self.client.get(f"{url}?filter{{resource_type}}=document", format='json') + response = self.client.get(f"{url}?filter{{resource_type}}=document&filter{{metadata_only}}=false", format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) self.assertEqual(response.data['total'], 9) @@ -738,7 +747,7 @@ def test_filter_resources(self): # Filter by resource_type == layer and title like 'common morx' response = self.client.get( - f"{url}?filter{{resource_type}}=dataset&filter{{title.icontains}}=common morx", format='json') + f"{url}?filter{{resource_type}}=dataset&filter{{title.icontains}}=common morx&filter{{metadata_only}}=false", format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) self.assertEqual(response.data['total'], 1) @@ -747,7 +756,7 @@ def test_filter_resources(self): # Filter by Keywords response = self.client.get( - f"{url}?filter{{keywords.name}}=here", format='json') + f"{url}?filter{{keywords.name}}=here&filter{{metadata_only}}=false", format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) self.assertEqual(response.data['total'], 1) @@ -756,7 +765,7 @@ def test_filter_resources(self): # Filter by Metadata Regions response = self.client.get( - f"{url}?filter{{regions.name.icontains}}=Italy", format='json') + f"{url}?filter{{regions.name.icontains}}=Italy&filter{{metadata_only}}=false", format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) self.assertEqual(response.data['total'], 0) @@ -765,7 +774,7 @@ def test_filter_resources(self): # Filter by Metadata Categories response = self.client.get( - f"{url}?filter{{category.identifier}}=elevation", format='json') + f"{url}?filter{{category.identifier}}=elevation&filter{{metadata_only}}=false", format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) self.assertEqual(response.data['total'], 6) @@ -773,21 +782,21 @@ def test_filter_resources(self): self.assertEqual(len(response.data['resources']), 6) # Extent Filter - response = self.client.get(f"{url}?page_size=26&extent=-180,-90,180,90", format='json') + response = self.client.get(f"{url}?page_size=26&extent=-180,-90,180,90&filter{{metadata_only}}=false", format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) self.assertEqual(response.data['total'], 26) # Pagination self.assertEqual(len(response.data['resources']), 26) - response = self.client.get(f"{url}?page_size=26&extent=0,0,100,100", format='json') + response = self.client.get(f"{url}?page_size=26&extent=0,0,100,100&filter{{metadata_only}}=false", format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) self.assertEqual(response.data['total'], 26) # Pagination self.assertEqual(len(response.data['resources']), 26) - response = self.client.get(f"{url}?page_size=26&extent=-10,-10,-1,-1", format='json') + response = self.client.get(f"{url}?page_size=26&extent=-10,-10,-1,-1&filter{{metadata_only}}=false", format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) self.assertEqual(response.data['total'], 12) @@ -797,7 +806,7 @@ def test_filter_resources(self): # Extent Filter: Crossing Dateline extent = "-180.0000,56.9689,-162.5977,70.7435,155.9180,56.9689,180.0000,70.7435" response = self.client.get( - f"{url}?page_size=26&extent={extent}", format='json') + f"{url}?page_size=26&extent={extent}&filter{{metadata_only}}=false", format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) self.assertEqual(response.data['total'], 12) @@ -816,7 +825,7 @@ def test_sort_resources(self): f"{url}?sort[]=title", format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) - self.assertEqual(response.data['total'], 26) + self.assertEqual(response.data['total'], 28) # Pagination self.assertEqual(len(response.data['resources']), 10) @@ -830,7 +839,7 @@ def test_sort_resources(self): f"{url}?sort[]=-title", format='json') self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 5) - self.assertEqual(response.data['total'], 26) + self.assertEqual(response.data['total'], 28) # Pagination self.assertEqual(len(response.data['resources']), 10) @@ -1663,7 +1672,7 @@ def test_embed_urls(self): resources = ResourceBase.objects.all() for resource in resources: - url = reverse('base-resources-detail', kwargs={'pk': resource.pk}) + url = "{base_url}?{params}".format(base_url=reverse('base-resources-detail', kwargs={'pk': resource.pk}), params="filter{metadata_only}=false") response = self.client.get(url, format='json') if resource.title.endswith('metadata true'): self.assertEqual(response.status_code, 404) diff --git a/geonode/security/tests.py b/geonode/security/tests.py index 00170888e52..842793747ba 100644 --- a/geonode/security/tests.py +++ b/geonode/security/tests.py @@ -1505,6 +1505,38 @@ def test_get_visible_resources_should_return_updated_resource_with_metadata_only actual = get_visible_resources(queryset=layers, user=get_user_model().objects.get(username=self.user)) self.assertEqual(layers.filter(dirty_state=False).count(), len(actual)) + def test_get_visible_resources_should_return_resource_with_metadata_only_true(self): + ''' + If metadata only is provided, it should return only the metadata resources + ''' + try: + dataset = create_single_dataset("dataset_with_metadata_only_True") + dataset.metadata_only = True + dataset.save() + + layers = Dataset.objects.all() + actual = get_visible_resources(queryset=layers, metadata_only=True, user=get_user_model().objects.get(username=self.user)) + self.assertEqual(1, actual.count()) + finally: + if dataset: + dataset.delete() + + def test_get_visible_resources_should_return_resource_with_metadata_only_none(self): + ''' + If metadata only is provided, it should return only the metadata resources + ''' + try: + dataset = create_single_dataset("dataset_with_metadata_only_True") + dataset.metadata_only = True + dataset.save() + + layers = Dataset.objects.all() + actual = get_visible_resources(queryset=layers, metadata_only=None, user=get_user_model().objects.get(username=self.user)) + self.assertEqual(layers.count(), actual.count()) + finally: + if dataset: + dataset.delete() + @override_settings( ADMIN_MODERATE_UPLOADS=True, RESOURCE_PUBLISHING=True, diff --git a/geonode/security/utils.py b/geonode/security/utils.py index af6c681333b..b0bc16f57c8 100644 --- a/geonode/security/utils.py +++ b/geonode/security/utils.py @@ -78,9 +78,11 @@ def get_visible_resources(queryset, except Exception: pass - # Hide Dirty State Resources - filter_set = queryset.filter( - Q(dirty_state=False) & Q(metadata_only=metadata_only)) + filter_set = queryset.filter(dirty_state=False) + + if metadata_only is not None: + # Hide Dirty State Resources + filter_set = filter_set.filter(metadata_only=metadata_only) if not is_admin: if user: