From 5b379150a903b8a53d1aafcc3c45d3304d510946 Mon Sep 17 00:00:00 2001 From: David Marteau Date: Thu, 11 Jul 2024 13:55:55 +0200 Subject: [PATCH 1/3] Fix WMS layer access control check --- .../services/wms/qgswmsrendercontext.cpp | 90 +++++++++++++++---- src/server/services/wms/qgswmsrendercontext.h | 12 ++- .../test_qgsserver_accesscontrol_wms.py | 23 ++++- ...rver_accesscontrol_wms_getlegendgraphic.py | 2 +- .../qgis_server_accesscontrol/project_grp.qgs | 2 +- 5 files changed, 108 insertions(+), 21 deletions(-) diff --git a/src/server/services/wms/qgswmsrendercontext.cpp b/src/server/services/wms/qgswmsrendercontext.cpp index e58a1527704..309517a92d4 100644 --- a/src/server/services/wms/qgswmsrendercontext.cpp +++ b/src/server/services/wms/qgswmsrendercontext.cpp @@ -47,11 +47,21 @@ void QgsWmsRenderContext::setParameters( const QgsWmsParameters ¶meters ) searchLayersToRender(); removeUnwantedLayers(); - checkLayerReadPermissions(); std::reverse( mLayersToRender.begin(), mLayersToRender.end() ); } +bool QgsWmsRenderContext::addLayerToRender( QgsMapLayer *layer ) +{ + bool allowed = checkLayerReadPermissions( layer ); + if ( allowed ) + { + mLayersToRender.append( layer ); + } + return allowed; +} + + void QgsWmsRenderContext::setFlag( const Flag flag, const bool on ) { if ( on ) @@ -186,6 +196,7 @@ qreal QgsWmsRenderContext::dotsPerMm() const QStringList QgsWmsRenderContext::flattenedQueryLayers( const QStringList &layerNames ) const { + QStringList result; std::function findLeaves = [ & ]( const QString & name ) -> QStringList { @@ -335,7 +346,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 +355,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 ) ) + { + layerGroup.push_back( layer ); + } } } else @@ -354,16 +369,25 @@ void QgsWmsRenderContext::initLayerGroupsRecursive( const QgsLayerTreeGroup *gro QList groupLayersList; for ( const auto &tl : treeGroupLayers ) { - groupLayersList << tl->layer(); + auto layer = tl->layer(); + if ( checkLayerReadPermissions( layer ) ) + { + 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 +466,16 @@ void QgsWmsRenderContext::searchLayersToRender() { const QList layers = mNicknameLayers.values( layerName ); for ( QgsMapLayer *lyr : layers ) + { if ( !mLayersToRender.contains( lyr ) ) { - mLayersToRender.append( lyr ); + if ( !addLayerToRender( lyr ) ) + { + throw QgsSecurityException( QStringLiteral( "Your are not allowed to access the layer %1" ).arg( lyr->name() ) ); + + } } + } } } @@ -456,10 +486,16 @@ void QgsWmsRenderContext::searchLayersToRender() { const QList layers = mNicknameLayers.values( layerName ); for ( QgsMapLayer *lyr : layers ) + { if ( !mLayersToRender.contains( lyr ) ) { - mLayersToRender.append( lyr ); + if ( !addLayerToRender( lyr ) ) + { + throw QgsSecurityException( QStringLiteral( "Your are not allowed to access the layer %1" ).arg( lyr->name() ) ); + + } } + } } } } @@ -495,7 +531,13 @@ void QgsWmsRenderContext::searchLayersToRenderSld() if ( mNicknameLayers.contains( lname ) ) { mSlds[lname] = namedElem; - mLayersToRender.append( mNicknameLayers.values( lname ) ); + for ( const auto layer : mNicknameLayers.values( lname ) ) + { + if ( !addLayerToRender( layer ) ) + { + throw QgsSecurityException( QStringLiteral( "Your are not allowed to access the layer %1" ).arg( layer->name() ) ); + } + } } else if ( mLayerGroups.contains( lname ) ) { @@ -540,7 +582,12 @@ void QgsWmsRenderContext::searchLayersToRenderStyle() { // to delete later mExternalLayers.append( layer.release() ); - mLayersToRender.append( mExternalLayers.last() ); + auto lyr = mExternalLayers.last(); + if ( !addLayerToRender( lyr ) ) + { + throw QgsSecurityException( QStringLiteral( "Your are not allowed to access the layer %1" ).arg( lyr->name() ) ); + + } } } else if ( mNicknameLayers.contains( nickname ) ) @@ -550,7 +597,13 @@ void QgsWmsRenderContext::searchLayersToRenderStyle() mStyles[nickname] = style; } - mLayersToRender.append( mNicknameLayers.values( nickname ) ); + for ( const auto layer : mNicknameLayers.values( nickname ) ) + { + if ( !addLayerToRender( layer ) ) + { + throw QgsSecurityException( QStringLiteral( "Your are not allowed to access the layer %1" ).arg( layer->name() ) ); + } + } } else if ( mLayerGroups.contains( nickname ) ) { @@ -575,7 +628,10 @@ void QgsWmsRenderContext::searchLayersToRenderStyle() for ( const auto &name : layersFromGroup ) { - mLayersToRender.append( mNicknameLayers.values( name ) ); + for ( const auto layer : mNicknameLayers.values( name ) ) + { + addLayerToRender( layer ); + } } } else @@ -852,17 +908,17 @@ bool QgsWmsRenderContext::isExternalLayer( const QString &name ) const return false; } -void QgsWmsRenderContext::checkLayerReadPermissions() +bool QgsWmsRenderContext::checkLayerReadPermissions( QgsMapLayer *layer ) { #ifdef HAVE_SERVER_PYTHON_PLUGINS - for ( const auto layer : mLayersToRender ) + if ( !accessControl()->layerReadPermission( layer ) ) { - if ( !accessControl()->layerReadPermission( layer ) ) - { - throw QgsSecurityException( QStringLiteral( "You are not allowed to access to the layer: %1" ).arg( layer->name() ) ); - } + QString msg = QStringLiteral( "Checking forbidden access for layer: %1" ).arg( layer->name() ); + QgsMessageLog::logMessage( msg, "Server", Qgis::MessageLevel::Info ); + 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 eca1e1c0249..414939246b1 100644 --- a/src/server/services/wms/qgswmsrendercontext.h +++ b/src/server/services/wms/qgswmsrendercontext.h @@ -292,7 +292,17 @@ namespace QgsWms void searchLayersToRenderStyle(); void removeUnwantedLayers(); - void checkLayerReadPermissions(); + /** + * Adds the layer to the list of layers to be rendered if the layer is readable + * Returns true if the layer is readable, false otherwise + */ + bool addLayerToRender( QgsMapLayer *layer ); + + /** + * Check layer read permissions + * Returns true if the layer is readable, false otherwise + */ + bool checkLayerReadPermissions( QgsMapLayer *layer ); 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 cb03d9bc977..55c45896be8 100644 --- a/tests/src/python/test_qgsserver_accesscontrol_wms.py +++ b/tests/src/python/test_qgsserver_accesscontrol_wms.py @@ -276,10 +276,31 @@ class TestQgsServerAccessControlWMS(TestQgsServerAccessControl): 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/src/python/test_qgsserver_accesscontrol_wms_getlegendgraphic.py b/tests/src/python/test_qgsserver_accesscontrol_wms_getlegendgraphic.py index 3175b006dd5..36a54040723 100644 --- a/tests/src/python/test_qgsserver_accesscontrol_wms_getlegendgraphic.py +++ b/tests/src/python/test_qgsserver_accesscontrol_wms_getlegendgraphic.py @@ -98,7 +98,7 @@ class TestQgsServerAccessControlWMSGetlegendgraphic(TestQgsServerAccessControl): 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 GetLegendGraphic" ) diff --git a/tests/testdata/qgis_server_accesscontrol/project_grp.qgs b/tests/testdata/qgis_server_accesscontrol/project_grp.qgs index 637e924f52e..8aa1e5f6896 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 From 84de2ff9eedf0b0621cebfcd5e11493364209384 Mon Sep 17 00:00:00 2001 From: David Marteau Date: Thu, 5 Sep 2024 14:27:11 +0200 Subject: [PATCH 2/3] Fix 'unused parameter' error Fix 'unused parameter' error when compiling without HAVE_SERVER_PYTHON_PLUGINS defined --- src/server/services/wms/qgswmsrendercontext.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/server/services/wms/qgswmsrendercontext.cpp b/src/server/services/wms/qgswmsrendercontext.cpp index 309517a92d4..2fedb4e4258 100644 --- a/src/server/services/wms/qgswmsrendercontext.cpp +++ b/src/server/services/wms/qgswmsrendercontext.cpp @@ -918,6 +918,7 @@ bool QgsWmsRenderContext::checkLayerReadPermissions( QgsMapLayer *layer ) return false; } #endif + Q_UNUSED( layer ) return true; } From e4d9f91191e70c547a1e341d808ed4e9e35b63fb Mon Sep 17 00:00:00 2001 From: rldhont Date: Fri, 13 Sep 2024 11:04:32 +0200 Subject: [PATCH 3/3] const allowed Co-authored-by: Alessandro Pasotti --- src/server/services/wms/qgswmsrendercontext.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/services/wms/qgswmsrendercontext.cpp b/src/server/services/wms/qgswmsrendercontext.cpp index 2fedb4e4258..aad8f3ad9ab 100644 --- a/src/server/services/wms/qgswmsrendercontext.cpp +++ b/src/server/services/wms/qgswmsrendercontext.cpp @@ -53,7 +53,7 @@ void QgsWmsRenderContext::setParameters( const QgsWmsParameters ¶meters ) bool QgsWmsRenderContext::addLayerToRender( QgsMapLayer *layer ) { - bool allowed = checkLayerReadPermissions( layer ); + const bool allowed = checkLayerReadPermissions( layer ); if ( allowed ) { mLayersToRender.append( layer );