From 7f342f6492e63a2b87ed4e6b57ce595946d61630 Mon Sep 17 00:00:00 2001 From: David Marteau Date: Thu, 11 Jul 2024 13:55:55 +0200 Subject: [PATCH] Fix WMS layer access control check --- .../services/wms/qgswmsrendercontext.cpp | 73 +++++++++++++++---- src/server/services/wms/qgswmsrendercontext.h | 4 +- .../test_qgsserver_accesscontrol_wms.py | 23 +++++- .../qgis_server_accesscontrol/project_grp.qgs | 2 +- 4 files changed, 84 insertions(+), 18 deletions(-) diff --git a/src/server/services/wms/qgswmsrendercontext.cpp b/src/server/services/wms/qgswmsrendercontext.cpp index e58a1527704c4..c21b90cc95696 100644 --- a/src/server/services/wms/qgswmsrendercontext.cpp +++ b/src/server/services/wms/qgswmsrendercontext.cpp @@ -47,11 +47,19 @@ void QgsWmsRenderContext::setParameters( const QgsWmsParameters ¶meters ) searchLayersToRender(); removeUnwantedLayers(); - checkLayerReadPermissions(); std::reverse( mLayersToRender.begin(), mLayersToRender.end() ); } +void QgsWmsRenderContext::addLayerToRender( QgsMapLayer *layer, bool queryLayer ) +{ + if ( checkLayerReadPermissions( layer, queryLayer ) ) + { + mLayersToRender.append( layer ); + } +} + + void QgsWmsRenderContext::setFlag( const Flag flag, const bool on ) { if ( on ) @@ -186,6 +194,7 @@ qreal QgsWmsRenderContext::dotsPerMm() const QStringList QgsWmsRenderContext::flattenedQueryLayers( const QStringList &layerNames ) const { + QStringList result; std::function findLeaves = [ & ]( const QString & name ) -> QStringList { @@ -335,7 +344,7 @@ void QgsWmsRenderContext::initLayerGroupsRecursive( const QgsLayerTreeGroup *gro { if ( !groupName.isEmpty() ) { - mLayerGroups[groupName] = QList(); + QList layerGroup; const auto projectLayerTreeRoot { mProject->layerTreeRoot() }; const auto treeGroupLayers { group->findLayers() }; // Fast track if there is no custom layer order, @@ -344,7 +353,11 @@ void QgsWmsRenderContext::initLayerGroupsRecursive( const QgsLayerTreeGroup *gro { for ( const auto &tl : treeGroupLayers ) { - mLayerGroups[groupName].push_back( tl->layer() ); + auto layer = tl->layer(); + if ( checkLayerReadPermissions( layer, false ) ) + { + layerGroup.push_back( layer ); + } } } else @@ -354,16 +367,25 @@ void QgsWmsRenderContext::initLayerGroupsRecursive( const QgsLayerTreeGroup *gro QList groupLayersList; for ( const auto &tl : treeGroupLayers ) { - groupLayersList << tl->layer(); + auto layer = tl->layer(); + if ( checkLayerReadPermissions( layer, false ) ) + { + groupLayersList << layer; + } } for ( const auto &l : projectLayerOrder ) { if ( groupLayersList.contains( l ) ) { - mLayerGroups[groupName].push_back( l ); + layerGroup.push_back( l ); } } } + + if ( !layerGroup.empty() ) + { + mLayerGroups[groupName] = layerGroup; + } } for ( const QgsLayerTreeNode *child : group->children() ) @@ -442,10 +464,12 @@ void QgsWmsRenderContext::searchLayersToRender() { const QList layers = mNicknameLayers.values( layerName ); for ( QgsMapLayer *lyr : layers ) + { if ( !mLayersToRender.contains( lyr ) ) { - mLayersToRender.append( lyr ); + addLayerToRender( lyr, true ); } + } } } @@ -456,10 +480,12 @@ void QgsWmsRenderContext::searchLayersToRender() { const QList layers = mNicknameLayers.values( layerName ); for ( QgsMapLayer *lyr : layers ) + { if ( !mLayersToRender.contains( lyr ) ) { - mLayersToRender.append( lyr ); + addLayerToRender( lyr, true ); } + } } } } @@ -495,7 +521,10 @@ void QgsWmsRenderContext::searchLayersToRenderSld() if ( mNicknameLayers.contains( lname ) ) { mSlds[lname] = namedElem; - mLayersToRender.append( mNicknameLayers.values( lname ) ); + for ( const auto layer : mNicknameLayers.values( lname ) ) + { + addLayerToRender( layer, true ); + } } else if ( mLayerGroups.contains( lname ) ) { @@ -540,7 +569,7 @@ void QgsWmsRenderContext::searchLayersToRenderStyle() { // to delete later mExternalLayers.append( layer.release() ); - mLayersToRender.append( mExternalLayers.last() ); + addLayerToRender( mExternalLayers.last(), true ); } } else if ( mNicknameLayers.contains( nickname ) ) @@ -550,7 +579,10 @@ void QgsWmsRenderContext::searchLayersToRenderStyle() mStyles[nickname] = style; } - mLayersToRender.append( mNicknameLayers.values( nickname ) ); + for ( const auto layer : mNicknameLayers.values( nickname ) ) + { + addLayerToRender( layer, true ); + } } else if ( mLayerGroups.contains( nickname ) ) { @@ -575,7 +607,10 @@ void QgsWmsRenderContext::searchLayersToRenderStyle() for ( const auto &name : layersFromGroup ) { - mLayersToRender.append( mNicknameLayers.values( name ) ); + for ( const auto layer : mNicknameLayers.values( name ) ) + { + addLayerToRender( layer, false ); + } } } else @@ -852,17 +887,25 @@ bool QgsWmsRenderContext::isExternalLayer( const QString &name ) const return false; } -void QgsWmsRenderContext::checkLayerReadPermissions() +bool QgsWmsRenderContext::checkLayerReadPermissions( QgsMapLayer *layer, bool queryLayer ) { #ifdef HAVE_SERVER_PYTHON_PLUGINS - for ( const auto layer : mLayersToRender ) + if ( !accessControl()->layerReadPermission( layer ) ) { - if ( !accessControl()->layerReadPermission( layer ) ) + QString msg = QStringLiteral( "Checking forbidden access for layer: %1" ).arg( layer->name() ); + QgsMessageLog::logMessage( msg, "Server", Qgis::MessageLevel::Info ); + + // Check if a layer has been requested explicitly + // In this case raise a forbidden access exception + if ( queryLayer ) { - throw QgsSecurityException( QStringLiteral( "You are not allowed to access to the layer: %1" ).arg( layer->name() ) ); + throw QgsSecurityException( QStringLiteral( "Your are not allowed to access the layer %1" ).arg( layer->name() ) ); } + return false; + } #endif + return true; } #ifdef HAVE_SERVER_PYTHON_PLUGINS diff --git a/src/server/services/wms/qgswmsrendercontext.h b/src/server/services/wms/qgswmsrendercontext.h index eca1e1c0249e3..11a2c7960011c 100644 --- a/src/server/services/wms/qgswmsrendercontext.h +++ b/src/server/services/wms/qgswmsrendercontext.h @@ -292,7 +292,9 @@ namespace QgsWms void searchLayersToRenderStyle(); void removeUnwantedLayers(); - void checkLayerReadPermissions(); + void addLayerToRender( QgsMapLayer *layer, bool queryLayer ); + + bool checkLayerReadPermissions( QgsMapLayer *layer, bool queryLayer ); bool layerScaleVisibility( const QString &name ) const; diff --git a/tests/src/python/test_qgsserver_accesscontrol_wms.py b/tests/src/python/test_qgsserver_accesscontrol_wms.py index cb03d9bc977a6..55c45896be8ce 100644 --- a/tests/src/python/test_qgsserver_accesscontrol_wms.py +++ b/tests/src/python/test_qgsserver_accesscontrol_wms.py @@ -276,10 +276,31 @@ def test_wms_getmap_grp(self): headers.get("Content-Type"), "text/xml; charset=utf-8", f"Content type for GetMap is wrong: {headers.get('Content-Type')}") self.assertTrue( - str(response).find('') != -1, + str(response).find('') != -1, "Not allowed do a GetMap on Country_grp" ) + # Check group ACL. + # The whole group should not fail since it contains + # allowed layers. + query_string = "&".join(["%s=%s" % i for i in list({ + "MAP": urllib.parse.quote(self.projectPath), + "SERVICE": "WMS", + "VERSION": "1.1.1", + "REQUEST": "GetMap", + "LAYERS": "project_grp", + "STYLES": "", + "FORMAT": "image/png", + "BBOX": "-16817707,-6318936.5,5696513,16195283.5", + "HEIGHT": "500", + "WIDTH": "500", + "SRS": "EPSG:3857" + }.items())]) + response, headers = self._get_restricted(query_string) + self.assertEqual( + headers.get("Content-Type"), "image/png", + f"Content type for GetMap is wrong: {headers.get('Content-Type')}") + def test_wms_getfeatureinfo_hello(self): query_string = "&".join(["%s=%s" % i for i in list({ "MAP": urllib.parse.quote(self.projectPath), diff --git a/tests/testdata/qgis_server_accesscontrol/project_grp.qgs b/tests/testdata/qgis_server_accesscontrol/project_grp.qgs index 637e924f52e6d..8aa1e5f68966f 100644 --- a/tests/testdata/qgis_server_accesscontrol/project_grp.qgs +++ b/tests/testdata/qgis_server_accesscontrol/project_grp.qgs @@ -3742,7 +3742,7 @@ def my_form_open(dialog, layer, feature): false - + project_grp false Simple test app. true