Skip to content

[Backport 5.1.x] [Fixes #14386] Replace Dataset does not keep in account the user perm… #14397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
github-actions wants to merge 1 commit into
base: 5.1.x
Choose a base branch
from
+378 −40
Open

[Backport 5.1.x] [Fixes #14386] Replace Dataset does not keep in account the user perm… #14397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 34 additions & 10 deletions geonode/upload/handlers/common/raster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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,
Expand All @@ -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

Expand Down
80 changes: 80 additions & 0 deletions geonode/upload/handlers/common/tests_raster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
#########################################################################
from unittest.mock import MagicMock
from django.test import TestCase
from mock import patch
from geonode.upload.handlers.common.raster import BaseRasterFileHandler
Expand Down Expand Up @@ -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)
182 changes: 180 additions & 2 deletions geonode/upload/handlers/common/tests_vector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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):
"""
Expand Down
Loading
Loading