qgis · rldhont · Aug 29, 2024 · Jul 11, 2024
diff --git a/src/server/services/wms/qgswmsrendercontext.cpp b/src/server/services/wms/qgswmsrendercontext.cpp
@@ -47,11 +47,21 @@ void QgsWmsRenderContext::setParameters( const QgsWmsParameters &parameters )
 
   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 <QStringList( const QString &name )> findLeaves = [ & ]( const QString & name ) -> QStringList
   {
@@ -335,7 +346,7 @@ void QgsWmsRenderContext::initLayerGroupsRecursive( const QgsLayerTreeGroup *gro
 {
   if ( !groupName.isEmpty() )
   {
-    mLayerGroups[groupName] = QList<QgsMapLayer *>();
+    QList<QgsMapLayer *> 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<QgsMapLayer *> 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<QgsMapLayer *> 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<QgsMapLayer *> 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
@@ -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
@@ -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('<ServiceException code="Security">') != -1,
+            str(response).find('<ServiceException code="LayerNotDefined">') != -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
@@ -98,7 +98,7 @@ def test_wms_getlegendgraphic_country_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('<ServiceException code="Security">') != -1,
+            str(response).find('<ServiceException code="LayerNotDefined">') != -1,
             "Not allowed GetLegendGraphic"
         )
 

diff --git 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):
     <WMSRequestDefinedDataSources type="bool">false</WMSRequestDefinedDataSources>
     <WMSRestrictedComposers type="QStringList"/>
     <WMSRestrictedLayers type="QStringList"/>
-    <WMSRootName type="QString"></WMSRootName>
+    <WMSRootName type="QString">project_grp</WMSRootName>
     <WMSSegmentizeFeatureInfoGeometry type="bool">false</WMSSegmentizeFeatureInfoGeometry>
     <WMSServiceAbstract type="QString">Simple test app.</WMSServiceAbstract>
     <WMSServiceCapabilities type="bool">true</WMSServiceCapabilities>