From 697d6e915a2f3e0c000e23c22a987c91edeebd3f Mon Sep 17 00:00:00 2001 From: Mattia Date: Tue, 25 Mar 2025 12:09:16 +0100 Subject: [PATCH 1/7] [Fixes #12996] Allow changing ownership of a single resource --- geonode/people/api/views.py | 12 +++++++++--- geonode/people/tests.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/geonode/people/api/views.py b/geonode/people/api/views.py index 5e9cf680e46..d78fd40f6b7 100644 --- a/geonode/people/api/views.py +++ b/geonode/people/api/views.py @@ -148,7 +148,7 @@ def transfer_resources(self, request, pk=None): user = self.get_object() admin = get_user_model().objects.filter(is_superuser=True, is_staff=True).first() target_user = request.data.get("owner") - + transfer_resource_subset = request.POST.getlist("resources", None) target = None if target_user == "DEFAULT": if not admin: @@ -160,7 +160,13 @@ def transfer_resources(self, request, pk=None): if target == user: return Response("Cannot reassign to self", status=400) - # transfer to target - ResourceBase.objects.filter(owner=user).update(owner=target or user) + if transfer_resource_subset: + # transfer_resources + ResourceBase.objects\ + .filter(owner=user, pk__in=transfer_resource_subset)\ + .update(owner=target or user) + else: + # transfer all the resources to target + ResourceBase.objects.filter(owner=user).update(owner=target or user) return Response("Resources transfered successfully", status=200) diff --git a/geonode/people/tests.py b/geonode/people/tests.py index 46e4047c8b7..f907c7348d0 100644 --- a/geonode/people/tests.py +++ b/geonode/people/tests.py @@ -1286,3 +1286,32 @@ def test_transfer_resources_nopayload(self): self.assertTrue(bobby_resources.exists()) later_bobby_resources = ResourceBase.objects.filter(owner=bobby).all() self.assertTrue(set(prior_bobby_resources) == set(later_bobby_resources)) + + def test_transfer_resource_subset(self): + """ + user wants to transfer resources to target + """ + bobby = get_user_model().objects.get(username="bobby") + self.assertTrue(self.client.login(username="bobby", password="bob")) + self.assertTrue(bobby.is_authenticated) + # check bobbys resources + resource_to_transfer = ResourceBase.objects.filter(owner=bobby).first() + second_resource = ResourceBase.objects.filter(owner=bobby).last() + new_owner = get_user_model().objects.exclude(username__in=["bobby", "AnonymousUser"]).first() + + # call api to transfer bobby resource to the other user + response = self.client.post( + path=f"{reverse('users-list')}/{bobby.pk}/transfer_resources", + data={"owner": new_owner.pk, "resources": [resource_to_transfer.pk, second_resource.pk]}, + ) + # response should be 200 + self.assertEqual(response.status_code, 200) + + resource_to_transfer.refresh_from_db() + second_resource.refresh_from_db() + # Check that bobby does not own the resource anymore + self.assertTrue(resource_to_transfer.owner != bobby) + self.assertTrue(second_resource.owner != bobby) + # since the payload say "default" + self.assertTrue(resource_to_transfer.owner == new_owner) + self.assertTrue(second_resource.owner == new_owner) From 7e3439e9309ff0fd4451f02f323025eaaa68efa2 Mon Sep 17 00:00:00 2001 From: Mattia Date: Tue, 25 Mar 2025 12:15:23 +0100 Subject: [PATCH 2/7] [Fixes #12996] Allow changing ownership of a single resource --- geonode/people/api/views.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/geonode/people/api/views.py b/geonode/people/api/views.py index d78fd40f6b7..3a500a7f5ab 100644 --- a/geonode/people/api/views.py +++ b/geonode/people/api/views.py @@ -162,9 +162,7 @@ def transfer_resources(self, request, pk=None): if transfer_resource_subset: # transfer_resources - ResourceBase.objects\ - .filter(owner=user, pk__in=transfer_resource_subset)\ - .update(owner=target or user) + ResourceBase.objects.filter(owner=user, pk__in=transfer_resource_subset).update(owner=target or user) else: # transfer all the resources to target ResourceBase.objects.filter(owner=user).update(owner=target or user) From fa4ebbce51f8d00eb5123a85f76846599c745d51 Mon Sep 17 00:00:00 2001 From: Mattia Date: Fri, 11 Apr 2025 12:28:17 +0200 Subject: [PATCH 3/7] [Fixes #12996] Allow changing ownership of a single resource --- geonode/people/api/views.py | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/geonode/people/api/views.py b/geonode/people/api/views.py index 3a500a7f5ab..26edf9404ec 100644 --- a/geonode/people/api/views.py +++ b/geonode/people/api/views.py @@ -21,6 +21,7 @@ from geonode.people.utils import get_available_users from django.contrib.auth import get_user_model from django.shortcuts import get_object_or_404 +from geonode.resource.manager import resource_manager class UserViewSet(DynamicModelViewSet): @@ -147,7 +148,7 @@ def remove_from_group_manager(self, request, pk=None): def transfer_resources(self, request, pk=None): user = self.get_object() admin = get_user_model().objects.filter(is_superuser=True, is_staff=True).first() - target_user = request.data.get("owner") + target_user = request.data.get("owner") # 1002 transfer_resource_subset = request.POST.getlist("resources", None) target = None if target_user == "DEFAULT": @@ -160,11 +161,25 @@ def transfer_resources(self, request, pk=None): if target == user: return Response("Cannot reassign to self", status=400) + filter_payload = {} + if transfer_resource_subset: # transfer_resources - ResourceBase.objects.filter(owner=user, pk__in=transfer_resource_subset).update(owner=target or user) - else: - # transfer all the resources to target - ResourceBase.objects.filter(owner=user).update(owner=target or user) + filter_payload["pk__in"] = transfer_resource_subset + + if not user.is_superuser: + """ + if the request user is not admin, we will filter for the resources + thats owns + """ + filter_payload["owner"] = user + + for instance in ResourceBase.objects.filter(**filter_payload).iterator(): + """ + We should reassing all the permissions to the new resource owner + we can use the resource manager because inside it will automatically update + the owner + """ + resource_manager.set_permissions(instance.uuid, instance, owner=target or user) return Response("Resources transfered successfully", status=200) From 092233097beb9f6fce7b8497e7d6a6e9d77a6288 Mon Sep 17 00:00:00 2001 From: Mattia Date: Fri, 11 Apr 2025 12:58:12 +0200 Subject: [PATCH 4/7] [Fixes #12996] Allow changing ownership of a single resource --- geonode/people/api/views.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/geonode/people/api/views.py b/geonode/people/api/views.py index 26edf9404ec..5b2f61bd4c6 100644 --- a/geonode/people/api/views.py +++ b/geonode/people/api/views.py @@ -148,8 +148,9 @@ def remove_from_group_manager(self, request, pk=None): def transfer_resources(self, request, pk=None): user = self.get_object() admin = get_user_model().objects.filter(is_superuser=True, is_staff=True).first() - target_user = request.data.get("owner") # 1002 - transfer_resource_subset = request.POST.getlist("resources", None) + target_user = request.data.get("newOwner") # the new owner + previous_owner = request.data.get("currentOwner") # the previous owner, usually it match the user + transfer_resource_subset = request.data.get("resources", None) target = None if target_user == "DEFAULT": if not admin: @@ -161,25 +162,19 @@ def transfer_resources(self, request, pk=None): if target == user: return Response("Cannot reassign to self", status=400) - filter_payload = {} + # we need to filter by the previous owner id + filter_payload = dict(owner=previous_owner or user) if transfer_resource_subset: # transfer_resources filter_payload["pk__in"] = transfer_resource_subset - if not user.is_superuser: - """ - if the request user is not admin, we will filter for the resources - thats owns - """ - filter_payload["owner"] = user - for instance in ResourceBase.objects.filter(**filter_payload).iterator(): """ We should reassing all the permissions to the new resource owner we can use the resource manager because inside it will automatically update the owner """ - resource_manager.set_permissions(instance.uuid, instance, owner=target or user) + resource_manager.set_permissions(instance.uuid, instance, owner=target or user, permissions=None) return Response("Resources transfered successfully", status=200) From 95307d4c554e740cac34d9330c025acd2e968b9f Mon Sep 17 00:00:00 2001 From: Mattia Date: Mon, 14 Apr 2025 12:42:02 +0200 Subject: [PATCH 5/7] [Fixes #12996] Fix permissions reassign of API to transfer resoruces --- geonode/people/api/views.py | 60 ++++++++++++++++++++++++------------- 1 file changed, 39 insertions(+), 21 deletions(-) diff --git a/geonode/people/api/views.py b/geonode/people/api/views.py index 5b2f61bd4c6..4e9547a9a77 100644 --- a/geonode/people/api/views.py +++ b/geonode/people/api/views.py @@ -22,6 +22,7 @@ from django.contrib.auth import get_user_model from django.shortcuts import get_object_or_404 from geonode.resource.manager import resource_manager +from geonode.security.registry import permissions_registry class UserViewSet(DynamicModelViewSet): @@ -152,29 +153,46 @@ def transfer_resources(self, request, pk=None): previous_owner = request.data.get("currentOwner") # the previous owner, usually it match the user transfer_resource_subset = request.data.get("resources", None) target = None - if target_user == "DEFAULT": - if not admin: - return Response("Principal User not found", status=500) - target = admin - else: - target = get_object_or_404(get_user_model(), id=target_user) - if target == user: - return Response("Cannot reassign to self", status=400) + if user.is_superuser or ( + not user.is_superuser + and ResourceBase.objects.filter(owner=user, pk__in=transfer_resource_subset).count() + == len(transfer_resource_subset) + ): + + if target_user == "DEFAULT": + if not admin: + return Response("Principal User not found", status=500) + target = admin + else: + target = get_object_or_404(get_user_model(), id=target_user) + + if target == user: + return Response("Cannot reassign to self", status=400) + + # we need to filter by the previous owner id + filter_payload = dict(owner=previous_owner or user) - # we need to filter by the previous owner id - filter_payload = dict(owner=previous_owner or user) + if transfer_resource_subset: + # transfer_resources + filter_payload["pk__in"] = transfer_resource_subset - if transfer_resource_subset: - # transfer_resources - filter_payload["pk__in"] = transfer_resource_subset + for instance in ResourceBase.objects.filter(**filter_payload).iterator(): + """ + We should reassing all the permissions to the new resource owner + we can use the resource manager because inside it will automatically update + the owner + """ + perms = permissions_registry.get_perms(instance=instance, include_virtual=False) + prev_owner = get_user_model().objects.fitler(pk=previous_owner).first() - for instance in ResourceBase.objects.filter(**filter_payload).iterator(): - """ - We should reassing all the permissions to the new resource owner - we can use the resource manager because inside it will automatically update - the owner - """ - resource_manager.set_permissions(instance.uuid, instance, owner=target or user, permissions=None) + if prev_owner and not prev_owner.is_superuser: + perms["users"].pop(prev_owner) - return Response("Resources transfered successfully", status=200) + resource_manager.set_permissions(instance.uuid, instance, owner=target or user, permissions=perms) + + return Response("Resources transfered successfully", status=200) + + return Response( + {"error": "The user does not have any right to perform this action on this resource"}, status=403 + ) From 03cc34eec7925fb75a6a4f38e870dc95c3277908 Mon Sep 17 00:00:00 2001 From: Mattia Date: Mon, 14 Apr 2025 13:42:28 +0200 Subject: [PATCH 6/7] [Fixes #12996] Fix permissions reassign of API to transfer resoruces --- geonode/people/api/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/geonode/people/api/views.py b/geonode/people/api/views.py index 4e9547a9a77..2cc9963c9dd 100644 --- a/geonode/people/api/views.py +++ b/geonode/people/api/views.py @@ -184,7 +184,7 @@ def transfer_resources(self, request, pk=None): the owner """ perms = permissions_registry.get_perms(instance=instance, include_virtual=False) - prev_owner = get_user_model().objects.fitler(pk=previous_owner).first() + prev_owner = get_user_model().objects.filter(pk=previous_owner).first() if prev_owner and not prev_owner.is_superuser: perms["users"].pop(prev_owner) From f9f200876a739ccc9ffe287327ae8dab15773f7d Mon Sep 17 00:00:00 2001 From: Mattia Date: Mon, 14 Apr 2025 16:18:53 +0200 Subject: [PATCH 7/7] [Fixes #12996] Fix permissions reassign of API to transfer resources --- geonode/base/api/views.py | 6 ++++ geonode/people/api/views.py | 55 +++++++++++++++++++++++++++++++------ geonode/people/tests.py | 26 ++++++++++++------ 3 files changed, 70 insertions(+), 17 deletions(-) diff --git a/geonode/base/api/views.py b/geonode/base/api/views.py index 9e41dfef9cd..11d2d6fc991 100644 --- a/geonode/base/api/views.py +++ b/geonode/base/api/views.py @@ -618,6 +618,9 @@ def resource_service_permissions(self, request, pk, *args, **kwargs): ) elif request.method == "PUT": perms_spec_compact = PermSpecCompact(request.data, resource) + if resource.dirty_state: + raise Exception("Cannot update if the resource is in dirty state") + resource.set_dirty_state() _exec_request = ExecutionRequest.objects.create( user=request.user, func_name="set_permissions", @@ -634,6 +637,9 @@ def resource_service_permissions(self, request, pk, *args, **kwargs): perms_spec_compact_patch = PermSpecCompact(request.data, resource) perms_spec_compact_resource = PermSpecCompact(perms_spec.compact, resource) perms_spec_compact_resource.merge(perms_spec_compact_patch) + if resource.dirty_state: + raise Exception("Cannot update if the resource is in dirty state") + resource.set_dirty_state() _exec_request = ExecutionRequest.objects.create( user=request.user, func_name="set_permissions", diff --git a/geonode/people/api/views.py b/geonode/people/api/views.py index 2cc9963c9dd..245e403e052 100644 --- a/geonode/people/api/views.py +++ b/geonode/people/api/views.py @@ -1,3 +1,23 @@ +######################################################################### +# +# Copyright (C) 2025 OSGeo +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# +######################################################################### + +import logging from django.conf import settings from dynamic_rest.filters import DynamicFilterBackend, DynamicSortingFilter from oauth2_provider.contrib.rest_framework import OAuth2Authentication @@ -25,6 +45,9 @@ from geonode.security.registry import permissions_registry +logger = logging.getLogger() + + class UserViewSet(DynamicModelViewSet): """ API endpoint that allows users to be viewed or edited. @@ -151,9 +174,16 @@ def transfer_resources(self, request, pk=None): admin = get_user_model().objects.filter(is_superuser=True, is_staff=True).first() target_user = request.data.get("newOwner") # the new owner previous_owner = request.data.get("currentOwner") # the previous owner, usually it match the user - transfer_resource_subset = request.data.get("resources", None) + transfer_resource_subset = ( + request.data.get("resources", None) + if not hasattr(request.data, "getlist") + else request.data.getlist("resources", None) + ) target = None + if not target_user and previous_owner: + return Response("Payload not passed", status=400) + if user.is_superuser or ( not user.is_superuser and ResourceBase.objects.filter(owner=user, pk__in=transfer_resource_subset).count() @@ -183,13 +213,22 @@ def transfer_resources(self, request, pk=None): we can use the resource manager because inside it will automatically update the owner """ - perms = permissions_registry.get_perms(instance=instance, include_virtual=False) - prev_owner = get_user_model().objects.filter(pk=previous_owner).first() - - if prev_owner and not prev_owner.is_superuser: - perms["users"].pop(prev_owner) - - resource_manager.set_permissions(instance.uuid, instance, owner=target or user, permissions=perms) + try: + # putting the resource in dirty state + instance.set_dirty_state() + # updating the perms with the new owner + perms = permissions_registry.get_perms(instance=instance, include_virtual=False) + prev_owner = get_user_model().objects.filter(pk=previous_owner).first() + + if prev_owner and not prev_owner.is_superuser: + perms["users"].pop(prev_owner) + # calling the registry to update the perms + resource_manager.set_permissions(instance.uuid, instance, owner=target or user, permissions=perms) + except Exception as e: + logger.exeption(e) + finally: + # clearing the dirty state + instance.clear_dirty_state() return Response("Resources transfered successfully", status=200) diff --git a/geonode/people/tests.py b/geonode/people/tests.py index f907c7348d0..1878d558073 100644 --- a/geonode/people/tests.py +++ b/geonode/people/tests.py @@ -35,7 +35,7 @@ from geonode.layers.models import Dataset from geonode.people import profileextractors -from geonode.base.populate_test_data import all_public, create_models, remove_models +from geonode.base.populate_test_data import all_public, create_models, create_single_dataset, remove_models from django.db.models import Q from geonode.security.registry import permissions_registry @@ -1150,7 +1150,8 @@ def test_transfer_resources_all(self): self.assertTrue(bobby_resources.exists()) # call api response = self.client.post( - path=f"{reverse('users-list')}/{bobby.pk}/transfer_resources", data={"owner": norman.id} + path=f"{reverse('users-list')}/{bobby.pk}/transfer_resources", + data={"currentOwner": bobby.id, "newOwner": norman.id}, ) # check that bobby owns the resources no more self.assertFalse(bobby_resources.exists()) @@ -1174,7 +1175,8 @@ def test_transfer_resources_invalid_user(self): self.assertTrue(bobby_resources.exists()) # call api response = self.client.post( - path=f"{reverse('users-list')}/{bobby}/transfer_resources", data={"owner": invalid_user_id} + path=f"{reverse('users-list')}/{bobby}/transfer_resources", + data={"currentOwner": bobby.id, "newOwner": invalid_user_id}, ) # response should be 404 self.assertEqual(response.status_code, 404) @@ -1200,7 +1202,8 @@ def test_transfer_resources_default(self): self.assertTrue(bobby_resources.exists()) # call api response = self.client.post( - path=f"{reverse('users-list')}/{bobby.pk}/transfer_resources", data={"owner": "DEFAULT"} + path=f"{reverse('users-list')}/{bobby.pk}/transfer_resources", + data={"currentOwner": bobby.id, "newOwner": "DEFAULT"}, ) self.assertTrue(response.status_code == 200) # check that bobby owns the resources no more @@ -1232,7 +1235,8 @@ def test_transfer_resources_to_missing_default(self): self.assertTrue(bobby_resources.exists()) # call api response = self.client.post( - path=f"{reverse('users-list')}/{bobby.pk}/transfer_resources", data={"owner": "DEFAULT"} + path=f"{reverse('users-list')}/{bobby.pk}/transfer_resources", + data={"newOwner": "DEFAULT", "previousOwner": bobby.pk}, ) self.assertTrue(response.status_code == 500) self.assertEqual(response.data, "Principal User not found") @@ -1257,7 +1261,8 @@ def test_transfer_resources_to_self(self): # call api response = self.client.post( - path=f"{reverse('users-list')}/{bobby.pk}/transfer_resources", data={"owner": bobby.pk} + path=f"{reverse('users-list')}/{bobby.pk}/transfer_resources", + data={"previousOwner": bobby.pk, "newOwner": bobby.pk}, ) self.assertTrue(response.status_code == 400) self.assertEqual(response.data, "Cannot reassign to self") @@ -1277,7 +1282,6 @@ def test_transfer_resources_nopayload(self): bobby_resources = ResourceBase.objects.filter(owner=bobby) prior_bobby_resources = bobby_resources.all() self.assertTrue(bobby_resources.exists()) - # call api response = self.client.post(path=f"{reverse('users-list')}/{bobby.pk}/transfer_resources", data={}) # response should be 404 @@ -1296,13 +1300,17 @@ def test_transfer_resource_subset(self): self.assertTrue(bobby.is_authenticated) # check bobbys resources resource_to_transfer = ResourceBase.objects.filter(owner=bobby).first() - second_resource = ResourceBase.objects.filter(owner=bobby).last() + second_resource = create_single_dataset("test for api", owner=bobby) new_owner = get_user_model().objects.exclude(username__in=["bobby", "AnonymousUser"]).first() # call api to transfer bobby resource to the other user response = self.client.post( path=f"{reverse('users-list')}/{bobby.pk}/transfer_resources", - data={"owner": new_owner.pk, "resources": [resource_to_transfer.pk, second_resource.pk]}, + data={ + "newOwner": new_owner.pk, + "currentOwner": bobby.pk, + "resources": [resource_to_transfer.pk, second_resource.pk], + }, ) # response should be 200 self.assertEqual(response.status_code, 200)