diff --git a/geonode/upload/handlers/common/raster.py b/geonode/upload/handlers/common/raster.py index 029662415ca..f3e374bdb06 100644 --- a/geonode/upload/handlers/common/raster.py +++ b/geonode/upload/handlers/common/raster.py @@ -37,6 +37,8 @@ from geonode.upload.handlers.utils import create_alternate, should_be_imported from geonode.upload.models import ResourceHandlerInfo from geonode.upload.orchestrator import orchestrator +from geonode.security.permissions import _to_compact_perms +from geonode.security.registry import permissions_registry from geonode.upload.utils import find_key_recursively, ImporterRequestAction as ira from osgeo import gdal from geonode.upload.celery_app import importer_app @@ -181,9 +183,22 @@ def create_asset_and_link(self, resource, files, action=None): response.raise_for_status() def overwrite_geoserver_resource(self, resource: List[str], catalog, store, workspace): - # we need to delete the resource before recreating it - self._delete_resource(resource, catalog, workspace) - self._delete_store(resource, catalog, workspace) + try: + self._delete_resource(resource, catalog, workspace) + except Exception as e: + logger.warning( + f"Could not delete existing resource '{resource.get('name')}' from GeoServer " + f"before replace. GeoServer returned: {e}. " + f"Proceeding — the resource will be overwritten by publish." + ) + try: + self._delete_store(resource, catalog, workspace) + except Exception as e: + logger.warning( + f"Could not delete existing store '{resource.get('name')}' from GeoServer " + f"before replace. GeoServer returned: {e}. " + f"Proceeding — the store will be overwritten by publish." + ) return self.publish_resources([resource], catalog, store, workspace) def _delete_store(self, resource, catalog, workspace): @@ -380,14 +395,23 @@ def overwrite_geonode_resource( _exec = self._get_execution_request_object(execution_id) - dataset = resource_type.objects.filter(alternate__icontains=alternate, owner=_exec.user) + dataset = resource_type.objects.filter(pk=_exec.input_params.get("resource_pk")).first() _overwrite = _exec.action == ira.REPLACE.value - # if the layer exists, we just update the information of the dataset by - # let it recreate the catalogue - if dataset.exists() and _overwrite: - dataset = dataset.first() + if dataset and _overwrite: + perms = _to_compact_perms( + permissions_registry.get_perms( + instance=dataset, + user=_exec.user, + ) + ) + if not any(p in perms for p in ("manage", "edit")): + raise ImportException( + f"User does not have permission to replace dataset '{layer_name}'. " + f"'edit' or 'manage' permission is required." + ) + resolved_resource_manager = resource_manager_registry.get_for_instance(dataset) dataset = resolved_resource_manager.update( dataset.uuid, @@ -410,12 +434,12 @@ def overwrite_geonode_resource( resolved_resource_manager.set_thumbnail(dataset.uuid, instance=dataset, overwrite=True) dataset.refresh_from_db() return dataset - elif not dataset.exists() and _overwrite: + elif not dataset and _overwrite: logger.warning( f"The dataset required {alternate} does not exists, but an overwrite is required, the resource will be created" ) return self.create_geonode_resource(layer_name, alternate, execution_id, resource_type, asset) - elif not dataset.exists() and not _overwrite: + elif not dataset and not _overwrite: logger.warning("The resource does not exists, please use 'create_geonode_resource' to create one") return diff --git a/geonode/upload/handlers/common/tests_raster.py b/geonode/upload/handlers/common/tests_raster.py index 6169e9b0ee5..66699a18d0a 100644 --- a/geonode/upload/handlers/common/tests_raster.py +++ b/geonode/upload/handlers/common/tests_raster.py @@ -16,6 +16,7 @@ # along with this program. If not, see . # ######################################################################### +from unittest.mock import MagicMock from django.test import TestCase from mock import patch from geonode.upload.handlers.common.raster import BaseRasterFileHandler @@ -93,3 +94,82 @@ def test_import_resource_should_work(self, import_orchestrator): finally: if exec_id: ExecutionRequest.objects.filter(exec_id=exec_id).delete() + + def test_overwrite_geoserver_resource_delete_resource_failure_is_non_fatal(self): + """ + If _delete_resource raises during a replace, overwrite_geoserver_resource + must log a warning and continue to publish_resources rather than failing. + """ + mock_catalog = MagicMock() + mock_store = MagicMock() + mock_workspace = MagicMock() + resource = {"name": "test_raster"} + + with patch.object(self.handler, "_delete_resource", side_effect=Exception("GeoServer 500")): + with patch.object(self.handler, "_delete_store") as mock_delete_store: + with patch.object(self.handler, "publish_resources", return_value=True) as mock_publish: + result = self.handler.overwrite_geoserver_resource( + resource, mock_catalog, mock_store, mock_workspace + ) + # _delete_store must still be attempted even if _delete_resource failed + mock_delete_store.assert_called_once() + mock_publish.assert_called_once() + self.assertTrue(result) + + def test_overwrite_geoserver_resource_delete_store_failure_is_non_fatal(self): + """ + If _delete_store raises during a replace, overwrite_geoserver_resource + must log a warning and continue to publish_resources rather than failing. + """ + mock_catalog = MagicMock() + mock_store = MagicMock() + mock_workspace = MagicMock() + resource = {"name": "test_raster"} + + with patch.object(self.handler, "_delete_resource"): + with patch.object(self.handler, "_delete_store", side_effect=Exception("GeoServer 500")): + with patch.object(self.handler, "publish_resources", return_value=True) as mock_publish: + result = self.handler.overwrite_geoserver_resource( + resource, mock_catalog, mock_store, mock_workspace + ) + mock_publish.assert_called_once() + self.assertTrue(result) + + def test_overwrite_geoserver_resource_both_deletes_fail_still_publishes(self): + """ + If both _delete_resource and _delete_store raise, publish_resources + must still be called — the store will be overwritten by publish. + """ + mock_catalog = MagicMock() + mock_store = MagicMock() + mock_workspace = MagicMock() + resource = {"name": "test_raster"} + + with patch.object(self.handler, "_delete_resource", side_effect=Exception("GeoServer 500")): + with patch.object(self.handler, "_delete_store", side_effect=Exception("GeoServer 500")): + with patch.object(self.handler, "publish_resources", return_value=True) as mock_publish: + result = self.handler.overwrite_geoserver_resource( + resource, mock_catalog, mock_store, mock_workspace + ) + mock_publish.assert_called_once() + self.assertTrue(result) + + def test_overwrite_geoserver_resource_success_calls_all_steps(self): + """ + When everything works, all three steps must be called in order. + """ + mock_catalog = MagicMock() + mock_store = MagicMock() + mock_workspace = MagicMock() + resource = {"name": "test_raster"} + + with patch.object(self.handler, "_delete_resource") as mock_delete_resource: + with patch.object(self.handler, "_delete_store") as mock_delete_store: + with patch.object(self.handler, "publish_resources", return_value=True) as mock_publish: + result = self.handler.overwrite_geoserver_resource( + resource, mock_catalog, mock_store, mock_workspace + ) + mock_delete_resource.assert_called_once_with(resource, mock_catalog, mock_workspace) + mock_delete_store.assert_called_once_with(resource, mock_catalog, mock_workspace) + mock_publish.assert_called_once_with([resource], mock_catalog, mock_store, mock_workspace) + self.assertTrue(result) diff --git a/geonode/upload/handlers/common/tests_vector.py b/geonode/upload/handlers/common/tests_vector.py index 939d0773408..d544f0f2908 100644 --- a/geonode/upload/handlers/common/tests_vector.py +++ b/geonode/upload/handlers/common/tests_vector.py @@ -25,7 +25,7 @@ from django.test import TestCase from django.urls import reverse from mock import MagicMock, patch -from geonode.upload.api.exceptions import UpsertException +from geonode.upload.api.exceptions import ImportException, UpsertException from geonode.upload.handlers.common.vector import BaseVectorFileHandler, import_with_ogr2ogr from django.contrib.auth import get_user_model from geonode.upload import project_dir @@ -38,8 +38,8 @@ from dynamic_models.models import ModelSchema from osgeo import ogr from django.test.utils import override_settings +from geonode.upload.utils import ImporterRequestAction as ira from geoserver.catalog import Catalog - from geonode.upload.tests.utils import TransactionImporterBaseTestSupport from geonode.utils import OGC_Servers_Handler from geonode.upload.utils import create_vrt_file, has_incompatible_field_names @@ -598,6 +598,184 @@ def test_copy_table_with_ogr2ogr(self, mock_popen): self.assertIn("new_table", call_args) self.assertIn("original_table", call_args) + def test_setup_dynamic_model_non_owner_with_edit_permission_can_replace(self): + """ + A non-owner user with 'edit' permission should be allowed to replace + the dataset. The dataset lookup must not be scoped by owner. + """ + exec_id = None + try: + non_owner, _ = get_user_model().objects.get_or_create(username="non_owner_editor") + exec_id = orchestrator.create_execution_request( + user=non_owner, + func_name="funct1", + step="step", + input_params={"files": self.valid_files, "skip_existing_layer": False}, + ) + resource = self.handler.create_geonode_resource( + "stazioni_metropolitana", + "stazioni_metropolitana", + str(exec_id), + ) + ExecutionRequest.objects.filter(exec_id=exec_id).update( + input_params={"files": self.valid_files, "skip_existing_layer": False, "resource_pk": resource.pk} + ) + + with patch("geonode.upload.handlers.common.vector._to_compact_perms", return_value=["edit"]): + with patch("geonode.upload.handlers.common.vector.permissions_registry") as mock_registry: + mock_registry.get_perms.return_value = MagicMock() + layers = ogr.Open(self.valid_gpkg) + dynamic_model, layer_name, celery_group = self.handler.setup_dynamic_model( + layer=[x for x in layers][0], + execution_id=str(exec_id), + should_be_overwritten=True, + username=non_owner, + ) + self.assertIsNotNone(dynamic_model) + self.assertIsNotNone(layer_name) + finally: + if exec_id: + ExecutionRequest.objects.filter(exec_id=exec_id).delete() + + def test_setup_dynamic_model_user_without_permission_cannot_replace(self): + """ + A user without 'edit' or 'manage' permission should get an ImportException + when trying to replace a dataset they don't own. + """ + exec_id = None + try: + stranger, _ = get_user_model().objects.get_or_create(username="stranger_user") + exec_id = orchestrator.create_execution_request( + user=stranger, + func_name="funct1", + step="step", + input_params={"files": self.valid_files, "skip_existing_layer": False}, + ) + resource = self.handler.create_geonode_resource( + "stazioni_metropolitana", + "stazioni_metropolitana", + str(exec_id), + ) + ExecutionRequest.objects.filter(exec_id=exec_id).update( + input_params={"files": self.valid_files, "skip_existing_layer": False, "resource_pk": resource.pk} + ) + + with patch("geonode.upload.handlers.common.vector._to_compact_perms", return_value=["view"]): + with patch("geonode.upload.handlers.common.vector.permissions_registry") as mock_registry: + mock_registry.get_perms.return_value = MagicMock() + layers = ogr.Open(self.valid_gpkg) + with self.assertRaises(ImportException): + self.handler.setup_dynamic_model( + layer=[x for x in layers][0], + execution_id=str(exec_id), + should_be_overwritten=True, + username=stranger, + ) + finally: + if exec_id: + ExecutionRequest.objects.filter(exec_id=exec_id).delete() + + def test_setup_dynamic_model_get_or_create_prevents_unique_constraint_crash(self): + """ + If a ModelSchema already exists but is not found by the iexact lookup + (e.g. name normalization mismatch between .shp and .zip), the second + branch must not crash with a unique constraint violation. + """ + exec_id = None + schema = None + try: + exec_id = orchestrator.create_execution_request( + user=self.user, + func_name="funct1", + step="step", + input_params={"files": self.valid_files, "skip_existing_layer": False}, + ) + # Pre-create the schema to simulate a stale/orphaned entry + schema, _ = ModelSchema.objects.get_or_create(name="stazioni_metropolitana", db_name="datastore") + layers = ogr.Open(self.valid_gpkg) + # Should not raise IntegrityError — get_or_create absorbs the collision + dynamic_model, layer_name, celery_group = self.handler.setup_dynamic_model( + layer=[x for x in layers][0], + execution_id=str(exec_id), + should_be_overwritten=False, + username=self.user, + ) + self.assertIsNotNone(dynamic_model) + finally: + if exec_id: + ExecutionRequest.objects.filter(exec_id=exec_id).delete() + if schema: + schema.delete() + + def test_overwrite_geonode_resource_non_owner_with_manage_permission_can_overwrite(self): + """ + A non-owner with 'manage' permission should be able to overwrite + a dataset via overwrite_geonode_resource. + """ + exec_id = None + try: + non_owner, _ = get_user_model().objects.get_or_create(username="non_owner_manager") + exec_id = orchestrator.create_execution_request( + user=non_owner, + func_name="funct1", + step="step", + action=ira.REPLACE.value, + input_params={ + "files": self.valid_files, + "resource_pk": self.layer.pk, + }, + ) + + with patch("geonode.upload.handlers.common.vector._to_compact_perms", return_value=["manage"]): + with patch("geonode.upload.handlers.common.vector.permissions_registry") as mock_registry: + mock_registry.get_perms.return_value = MagicMock() + with patch.object( + self.handler, "refresh_geonode_resource", return_value=self.layer + ) as mock_refresh: + result = self.handler.overwrite_geonode_resource( + layer_name="stazioni_metropolitana", + alternate="stazioni_metropolitana", + execution_id=str(exec_id), + ) + mock_refresh.assert_called_once() + self.assertIsNotNone(result) + finally: + if exec_id: + ExecutionRequest.objects.filter(exec_id=exec_id).delete() + + def test_overwrite_geonode_resource_user_without_permission_returns_none(self): + """ + A user without 'edit' or 'manage' permission should get None returned + from overwrite_geonode_resource without touching the dataset. + """ + exec_id = None + try: + stranger, _ = get_user_model().objects.get_or_create(username="stranger_overwrite") + exec_id = orchestrator.create_execution_request( + user=stranger, + func_name="funct1", + step="step", + action=ira.REPLACE.value, + input_params={ + "files": self.valid_files, + "resource_pk": self.layer.pk, + }, + ) + + with patch("geonode.upload.handlers.common.vector._to_compact_perms", return_value=["view"]): + with patch("geonode.upload.handlers.common.vector.permissions_registry") as mock_registry: + mock_registry.get_perms.return_value = MagicMock() + with patch.object(self.handler, "refresh_geonode_resource"): + with self.assertRaises(ImportException): + self.handler.overwrite_geonode_resource( + layer_name="stazioni_metropolitana", + alternate="stazioni_metropolitana", + execution_id=str(exec_id), + ) + finally: + if exec_id: + ExecutionRequest.objects.filter(exec_id=exec_id).delete() + class TestUpsertBaseVectorHandler(TransactionImporterBaseTestSupport): """ diff --git a/geonode/upload/handlers/common/vector.py b/geonode/upload/handlers/common/vector.py index ad81ca983ba..8b66f0e964a 100644 --- a/geonode/upload/handlers/common/vector.py +++ b/geonode/upload/handlers/common/vector.py @@ -681,47 +681,72 @@ def setup_dynamic_model( is_dynamic_model_managed = _exec_obj.input_params.get("is_dynamic_model_managed", False) workspace = DataPublisher(None).workspace + if resource_pk := _exec_obj.input_params.get("resource_pk", None): - user_datasets = Dataset.objects.filter(owner=username, pk=resource_pk) - user_dataset = user_datasets.first() + available_dataset = Dataset.objects.filter(pk=resource_pk) + user_dataset = available_dataset.first() if user_dataset: dynamic_schema = ModelSchema.objects.filter(name__iexact=user_dataset.name) else: dynamic_schema = ModelSchema.objects.none() else: - user_datasets = Dataset.objects.filter(owner=username, alternate__iexact=f"{workspace.name}:{layer_name}") + available_dataset = Dataset.objects.filter(alternate__iexact=f"{workspace.name}:{layer_name}") dynamic_schema = ModelSchema.objects.filter(name__iexact=layer_name) dynamic_schema_exists = dynamic_schema.exists() - dataset_exists = user_datasets.exists() + dataset_exists = available_dataset.exists() + + # evaluate if the user has the correct permissions for the resource. + # if can "edit" / manage the resource, it should be able to replace it + if should_be_overwritten and dataset_exists: + perms = _to_compact_perms( + permissions_registry.get_perms( + instance=available_dataset.first(), + user=username, + ) + ) + if not any(p in perms for p in ("manage", "edit")): + raise ImportException( + f"User '{username}' does not have permission to replace dataset '{layer_name}'. " + f"'edit' or 'manage' permission is required." + ) if dataset_exists and dynamic_schema_exists and should_be_overwritten: """ - If the user have a dataset, the dynamic model has already been created and is in overwrite mode, + If the user has a dataset, the dynamic model has already been created and is in overwrite mode, we just take the dynamic_model to overwrite the existing one """ dynamic_schema = dynamic_schema.get() - layer_name = user_datasets.first().alternate.split(":")[-1] + layer_name = available_dataset.first().alternate.split(":")[-1] + elif not dataset_exists and not dynamic_schema_exists: """ - cames here when is a new brand upload or when (for any reasons) the dataset exists but the - dynamic model has not been created before + Comes here when is a brand new upload or when (for any reasons) the dataset exists but the + dynamic model has not been created before. """ - # layer_name = create_alternate(layer_name, execution_id) - dynamic_schema = ModelSchema.objects.create( + dynamic_schema, created = ModelSchema.objects.get_or_create( name=layer_name, - db_name="datastore", - managed=is_dynamic_model_managed, - db_table_name=layer_name, + defaults={ + "db_name": "datastore", + "managed": is_dynamic_model_managed, + "db_table_name": layer_name, + }, ) + if not created: + logger.warning( + f"ModelSchema for '{layer_name}' already exists but was not found by the " + f"iexact lookup — possible name normalization mismatch between file formats " + f"(e.g. .shp vs .zip). Reusing the existing schema for execution {execution_id}." + ) + elif ( (not dataset_exists and dynamic_schema_exists) or (dataset_exists and dynamic_schema_exists and not should_be_overwritten) or (dataset_exists and not dynamic_schema_exists) ): """ - it comes here when the layer should not be overrided so we append the UUID - to the layer to let it proceed to the next steps + The layer should not be overwritten so we append the UUID + to the layer name to let it proceed to the next steps """ layer_name = create_alternate(layer_name, execution_id) dynamic_schema, _ = ModelSchema.objects.get_or_create( @@ -920,24 +945,33 @@ def overwrite_geonode_resource( ): _exec = self._get_execution_request_object(execution_id) - dataset = resource_type.objects.filter(pk=_exec.input_params.get("resource_pk"), owner=_exec.user) + dataset = resource_type.objects.filter(pk=_exec.input_params.get("resource_pk")).first() _overwrite = _exec.action == ira.REPLACE.value # if the layer exists, we just update the information of the dataset by # let it recreate the catalogue - if dataset.exists() and _overwrite: - dataset = dataset.first() + if dataset and _overwrite: + perms = _to_compact_perms( + permissions_registry.get_perms( + instance=dataset, + user=_exec.user, + ) + ) + if not any(p in perms for p in ("manage", "edit")): + logger.error( + f"User '{_exec.user}' does not have permission to overwrite dataset " + f"'{dataset.alternate}'. 'edit' or 'manage' permission is required." + ) + raise ImportException( + f"User does not have permission to replace dataset '{layer_name}'. " + f"'edit' or 'manage' permission is required." + ) dataset = self.refresh_geonode_resource( str(_exec.exec_id), asset, dataset, create_asset=False, layer_name=layer_name ) return dataset - elif not dataset.exists() and _overwrite: - logger.warning( - f"The dataset required {alternate} does not exists, but an overwrite is required, the resource will be created" - ) - return self.create_geonode_resource(layer_name, alternate, execution_id, resource_type, asset) - elif not dataset.exists() and not _overwrite: + elif not dataset and not _overwrite: logger.warning("The resource does not exists, please use 'create_geonode_resource' to create one") return @@ -1109,10 +1143,32 @@ def _import_resource_rollback(self, exec_id, instance_name=None, *args, **kwargs schema = None if settings.IMPORTER_ENABLE_DYN_MODELS: schema = ModelSchema.objects.filter(name=instance_name).first() + if schema is not None: - _model_editor = ModelSchemaEditor(initial_model=instance_name, db_name=schema.db_name) - _model_editor.drop_table(schema.as_model()) - ModelSchema.objects.filter(name=instance_name).delete() + # before destroying the schema, check whether a dataset that + # predates this execution references it. If so, this execution did not + # create the schema and must not delete it. + dataset_alternate = instance_name + if ":" not in dataset_alternate: + workspace = DataPublisher(None).workspace.name + dataset_alternate = f"{workspace}:{dataset_alternate}" + pre_existing_dataset = ( + Dataset.objects.filter(alternate__iexact=dataset_alternate) + .exclude(resourcehandlerinfo__execution_request__exec_id=exec_id) + .exists() + ) + + if pre_existing_dataset: + logger.warning( + f"Rollback skipping schema deletion for '{instance_name}': " + f"a dataset not created by execution {exec_id} references this schema. " + f"The schema will be preserved to avoid data loss." + ) + else: + _model_editor = ModelSchemaEditor(initial_model=instance_name, db_name=schema.db_name) + _model_editor.drop_table(schema.as_model()) + ModelSchema.objects.filter(name=instance_name).delete() + elif schema is None: try: logger.warning("Dynamic model does not exists, removing ogr2ogr table in progress") @@ -1121,7 +1177,7 @@ def _import_resource_rollback(self, exec_id, instance_name=None, *args, **kwargs return db_name = os.getenv("DEFAULT_BACKEND_DATASTORE", "datastore") with connections[db_name].cursor() as cursor: - cursor.execute(f"DROP TABLE {instance_name}") + cursor.execute(f"DROP TABLE IF EXISTS {instance_name}") except Exception as e: logger.warning(e) pass