[Backport 5.0.x] Replace Dataset does not keep in account the user perm(#14387)#14398
[Backport 5.0.x] Replace Dataset does not keep in account the user perm(#14387)#14398sijandh35 wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the robustness and permission handling of GeoNode's raster and vector upload handlers during resource overwrites, and adds comprehensive unit tests. The review feedback identifies a critical regression in the vector handler where a missing branch prevents resource creation during an overwrite if the dataset does not exist. Additionally, the feedback points out a potential SQL injection vulnerability in raw SQL execution, a type hint mismatch in the raster handler, and a defensive check opportunity during rollback.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| elif not dataset and not _overwrite: | ||
| logger.warning("The resource does not exists, please use 'create_geonode_resource' to create one") | ||
| return |
There was a problem hiding this comment.
The elif not dataset and _overwrite: branch was accidentally removed during refactoring. If a user attempts to replace/overwrite a dataset that does not exist yet, the method will silently return None instead of creating the resource. This breaks the expected behavior and differs from the raster handler implementation. Please restore this branch to ensure the resource is created when overwrite is requested but the dataset is missing.
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 and not _overwrite:
logger.warning("The resource does not exists, please use 'create_geonode_resource' to create one")
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}") |
There was a problem hiding this comment.
Directly interpolating instance_name into the raw SQL query introduces a potential SQL injection vulnerability. Even if the input is sanitized elsewhere, it is highly recommended to use the database connection's quote_name utility to safely escape the table identifier before executing the query.
| 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}") | |
| db_name = os.getenv("DEFAULT_BACKEND_DATASTORE", "datastore") | |
| with connections[db_name].cursor() as cursor: | |
| safe_instance_name = connections[db_name].ops.quote_name(instance_name) | |
| cursor.execute(f"DROP TABLE IF EXISTS {safe_instance_name}") |
| @@ -184,9 +186,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): | |||
There was a problem hiding this comment.
The type hint for resource is specified as List[str], but the implementation calls .get('name') on it, and the corresponding tests pass a dictionary. Please update the type hint to dict to prevent static analysis errors and improve code readability.
| def overwrite_geoserver_resource(self, resource: List[str], catalog, store, workspace): | |
| def overwrite_geoserver_resource(self, resource: dict, catalog, store, workspace): |
| dataset_alternate = instance_name | ||
| if ":" not in dataset_alternate: | ||
| workspace = DataPublisher(None).workspace.name | ||
| dataset_alternate = f"{workspace}:{dataset_alternate}" |
There was a problem hiding this comment.
If instance_name is None, performing ":" not in dataset_alternate will raise a TypeError. Adding a defensive check to ensure dataset_alternate is not None or empty before performing the membership check prevents potential runtime crashes.
| dataset_alternate = instance_name | |
| if ":" not in dataset_alternate: | |
| workspace = DataPublisher(None).workspace.name | |
| dataset_alternate = f"{workspace}:{dataset_alternate}" | |
| dataset_alternate = instance_name | |
| if dataset_alternate and ":" not in dataset_alternate: | |
| workspace = DataPublisher(None).workspace.name | |
| dataset_alternate = f"{workspace}:{dataset_alternate}" |
sijandh35
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 5.0.x #14398 +/- ##
========================================
Coverage ? 66.00%
========================================
Files ? 945
Lines ? 57034
Branches ? 7744
========================================
Hits ? 37643
Misses ? 17824
Partials ? 1567 🚀 New features to boost your workflow:
|
2 participants
(cherry picked from commit 79d213c) PR: #14387
Checklist
For all pull requests:
The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):
Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.