From 263d5bbc7bd2790ee3bb03ca96ec81ef7b4e8883 Mon Sep 17 00:00:00 2001
From: David Marteau <dmarteau@3liz.com>
Date: Tue, 1 Oct 2024 13:12:01 +0200
Subject: [PATCH] [server] Fix sub-optimal checks in wms access control

---
 .../services/wms/qgswmsrendercontext.cpp      | 72 +++++++++++++------
 src/server/services/wms/qgswmsrendercontext.h |  2 +-
 2 files changed, 50 insertions(+), 24 deletions(-)

diff --git a/src/server/services/wms/qgswmsrendercontext.cpp b/src/server/services/wms/qgswmsrendercontext.cpp
index 23ebaaede004..cbcf336e21fb 100644
--- a/src/server/services/wms/qgswmsrendercontext.cpp
+++ b/src/server/services/wms/qgswmsrendercontext.cpp
@@ -206,15 +206,19 @@ QStringList QgsWmsRenderContext::flattenedQueryLayers( const QStringList &layerN
       const auto &layers  { mLayerGroups[ name ] };
       for ( const auto &l : layers )
       {
-        const auto nick { layerNickname( *l ) };
-        // This handles the case for root (fake) group
-        if ( mLayerGroups.contains( nick ) )
+        // Only add allowed layers
+        if ( checkLayerReadPermissions( l ) )
         {
-          _result.append( name );
-        }
-        else
-        {
-          _result.append( findLeaves( nick ) );
+          const auto nick { layerNickname( *l ) };
+          // This handles the case for root (fake) group
+          if ( mLayerGroups.contains( nick ) )
+          {
+            _result.append( name );
+          }
+          else
+          {
+            _result.append( findLeaves( nick ) );
+          }
         }
       }
     }
@@ -355,11 +359,7 @@ void QgsWmsRenderContext::initLayerGroupsRecursive( const QgsLayerTreeGroup *gro
     {
       for ( const auto &tl : treeGroupLayers )
       {
-        auto layer = tl->layer();
-        if ( checkLayerReadPermissions( layer ) )
-        {
-          layerGroup.push_back( layer );
-        }
+        layerGroup.push_back( tl->layer() );
       }
     }
     else
@@ -369,11 +369,7 @@ void QgsWmsRenderContext::initLayerGroupsRecursive( const QgsLayerTreeGroup *gro
       QList<QgsMapLayer *> groupLayersList;
       for ( const auto &tl : treeGroupLayers )
       {
-        auto layer = tl->layer();
-        if ( checkLayerReadPermissions( layer ) )
-        {
-          groupLayersList << layer;
-        }
+        groupLayersList << tl->layer();
       }
       for ( const auto &l : projectLayerOrder )
       {
@@ -548,11 +544,28 @@ void QgsWmsRenderContext::searchLayersToRenderSld()
           throw QgsBadRequestException( QgsServiceException::OGC_LayerNotDefined,
                                         param );
         }
+
+        bool layerAdded = false;
         for ( QgsMapLayer *layer : mLayerGroups[lname] )
         {
-          const QString name = layerNickname( *layer );
-          mSlds[name] = namedElem;
-          mLayersToRender.insert( 0, layer );
+          // Insert only allowed layers
+          if ( checkLayerReadPermissions( layer ) )
+          {
+            const QString name = layerNickname( *layer );
+            mSlds[name] = namedElem;
+            mLayersToRender.insert( 0, layer );
+            layerAdded = true;
+          }
+        }
+        // No layers have been added, consider the group
+        // as non-existent.
+        if ( !layerAdded )
+        {
+          QgsWmsParameter param( QgsWmsParameter::LAYER );
+          param.mValue = lname;
+          throw QgsBadRequestException( QgsServiceException::OGC_LayerNotDefined,
+                                        param );
+
         }
       }
       else
@@ -626,13 +639,26 @@ void QgsWmsRenderContext::searchLayersToRenderStyle()
         layersFromGroup.push_front( nickname );
       }
 
+      bool layerAdded = false;
       for ( const auto &name : layersFromGroup )
       {
         for ( const auto layer : mNicknameLayers.values( name ) )
         {
-          addLayerToRender( layer );
+          if ( addLayerToRender( layer ) )
+          {
+            layerAdded = true;
+          }
         }
       }
+      // No layers have been added, consider the group
+      // as non-existent.
+      if ( !layerAdded )
+      {
+        QgsWmsParameter param( QgsWmsParameter::LAYER );
+        param.mValue = nickname;
+        throw QgsBadRequestException( QgsServiceException::OGC_LayerNotDefined,
+                                      param );
+      }
     }
     else
     {
@@ -908,7 +934,7 @@ bool QgsWmsRenderContext::isExternalLayer( const QString &name ) const
   return false;
 }
 
-bool QgsWmsRenderContext::checkLayerReadPermissions( QgsMapLayer *layer )
+bool QgsWmsRenderContext::checkLayerReadPermissions( QgsMapLayer *layer ) const
 {
 #ifdef HAVE_SERVER_PYTHON_PLUGINS
   if ( !accessControl()->layerReadPermission( layer ) )
diff --git a/src/server/services/wms/qgswmsrendercontext.h b/src/server/services/wms/qgswmsrendercontext.h
index 89a35b511f31..e9af1d0f0bab 100644
--- a/src/server/services/wms/qgswmsrendercontext.h
+++ b/src/server/services/wms/qgswmsrendercontext.h
@@ -299,7 +299,7 @@ namespace QgsWms
        * Check layer read permissions
        * Returns true if the layer is readable, false otherwise
        */
-      bool checkLayerReadPermissions( QgsMapLayer *layer );
+      bool checkLayerReadPermissions( QgsMapLayer *layer ) const;
 
       bool layerScaleVisibility( const QString &name ) const;