Merge pull request from GHSA-jr83-m233-gg6p

* Fix setting of Webspace Security System before Symfony RouteListener * Fix Redirect for Security System checker error * Add test case to avoid regression * Add test for none secured pages * Fix some PHP 7.2 issues * Remmove website based system listener test * Activate security only for specific test * Remove old ContentRouteProviderTests * Fix phpcs * Skip security listener test for older symfony security system * Fix code style * Fix phpstan issues * Throw deprecation when requestAnalyzer is still given * Fix code style * Mark new service as internal and final
sulu · Mar 4, 2024 · ec9c3f9 · ec9c3f9
1 parent 9bbc9c8
commit ec9c3f9
Show file tree

Hide file tree

Showing 17 changed files with 429 additions and 224 deletions.
diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -29621,12 +29621,12 @@ parameters:
 			path: src/Sulu/Bundle/SecurityBundle/EventListener/SuluSecurityListener.php
 
 		-
-			message: "#^If condition is always true\\.$#"
+			message: "#^Method Sulu\\\\Bundle\\\\SecurityBundle\\\\EventListener\\\\SystemListener\\:\\:onKernelRequest\\(\\) has no return type specified\\.$#"
 			count: 1
 			path: src/Sulu/Bundle/SecurityBundle/EventListener/SystemListener.php
 
 		-
-			message: "#^Method Sulu\\\\Bundle\\\\SecurityBundle\\\\EventListener\\\\SystemListener\\:\\:onKernelRequest\\(\\) has no return type specified\\.$#"
+			message: "#^Property Sulu\\\\Bundle\\\\SecurityBundle\\\\EventListener\\\\SystemListener\\:\\:\\$requestAnalyzer is never read, only written\\.$#"
 			count: 1
 			path: src/Sulu/Bundle/SecurityBundle/EventListener/SystemListener.php
 
@@ -30550,11 +30550,6 @@ parameters:
 			count: 1
 			path: src/Sulu/Bundle/SecurityBundle/Tests/Unit/EventListener/SuluSecurityListenerTest.php
 
-		-
-			message: "#^Method Sulu\\\\Bundle\\\\SecurityBundle\\\\EventListener\\\\SystemListenerTest\\:\\:provideSetWebsiteSystem\\(\\) has no return type specified\\.$#"
-			count: 1
-			path: src/Sulu/Bundle/SecurityBundle/Tests/Unit/EventListener/SystemListenerTest.php
-
 		-
 			message: "#^Class Sulu\\\\Bundle\\\\SecurityBundle\\\\Security\\\\AuthenticationEntryPoint does not have a constructor and must be instantiated without any parameters\\.$#"
 			count: 1
@@ -34920,6 +34915,11 @@ parameters:
 			count: 1
 			path: src/Sulu/Bundle/WebsiteBundle/Routing/ContentRouteProvider.php
 
+		-
+			message: "#^Property Sulu\\\\Bundle\\\\WebsiteBundle\\\\Routing\\\\ContentRouteProvider\\:\\:\\$securityChecker is never read, only written\\.$#"
+			count: 1
+			path: src/Sulu/Bundle/WebsiteBundle/Routing/ContentRouteProvider.php
+
 		-
 			message: "#^Strict comparison using \\=\\=\\= between null and string will always evaluate to false\\.$#"
 			count: 1

diff --git a/src/Sulu/Bundle/CoreBundle/Resources/config/request_analyzer.xml b/src/Sulu/Bundle/CoreBundle/Resources/config/request_analyzer.xml
@@ -51,6 +51,12 @@
             <tag name="sulu.context" context="website"/>
             <tag name="sulu.request_attributes" priority="-64"/>
         </service>
+        <service id="sulu_core.request_processor.system"
+                 class="Sulu\Component\Webspace\Analyzer\Attributes\SystemRequestProcessor">
+            <argument type="service" id="sulu_security.system_store" />
+            <argument>%sulu.context%</argument>
+            <tag name="sulu.request_attributes" priority="-32"/>
+        </service>
         <service id="sulu_core.request_processor.date_time"
                  class="Sulu\Component\Webspace\Analyzer\Attributes\DateTimeRequestProcessor">
             <tag name="sulu.context" context="website"/>

diff --git a/src/Sulu/Bundle/SecurityBundle/EventListener/SystemListener.php b/src/Sulu/Bundle/SecurityBundle/EventListener/SystemListener.php
@@ -27,7 +27,7 @@ class SystemListener implements EventSubscriberInterface
     private $systemStore;
 
     /**
-     * @var RequestAnalyzerInterface
+     * @var RequestAnalyzerInterface|null
      */
     private $requestAnalyzer;
 
@@ -38,10 +38,15 @@ class SystemListener implements EventSubscriberInterface
 
     public function __construct(
         SystemStoreInterface $systemStore,
-        RequestAnalyzerInterface $requestAnalyzer,
+        ?RequestAnalyzerInterface $requestAnalyzer,
         string $context
     ) {
         $this->systemStore = $systemStore;
+
+        if (null !== $requestAnalyzer) {
+            @trigger_deprecation('sulu/sulu', '2.4', 'The argument "%s" in class "%s" is deprecated and not longer required set `null` instead.', RequestAnalyzerInterface::class, __CLASS__);
+        }
+
         $this->requestAnalyzer = $requestAnalyzer;
         $this->context = $context;
     }
@@ -58,15 +63,5 @@ public function onKernelRequest(RequestEvent $requestEvent)
 
             return;
         }
-
-        $webspace = $this->requestAnalyzer->getWebspace();
-        if ($webspace) {
-            $security = $webspace->getSecurity();
-            if ($security) {
-                $this->systemStore->setSystem($security->getSystem());
-
-                return;
-            }
-        }
     }
 }
diff --git a/src/Sulu/Bundle/SecurityBundle/Resources/config/services.xml b/src/Sulu/Bundle/SecurityBundle/Resources/config/services.xml
@@ -306,7 +306,7 @@
 
         <service id="sulu_security.system_listener" class="Sulu\Bundle\SecurityBundle\EventListener\SystemListener">
             <argument type="service" id="sulu_security.system_store" />
-            <argument type="service" id="sulu_core.webspace.request_analyzer"/>
+            <argument>null</argument>
             <argument>%sulu.context%</argument>
 
             <tag name="kernel.event_subscriber"/>

diff --git a/src/Sulu/Bundle/SecurityBundle/Tests/Unit/EventListener/SystemListenerTest.php b/src/Sulu/Bundle/SecurityBundle/Tests/Unit/EventListener/SystemListenerTest.php
@@ -15,8 +15,6 @@
 use Prophecy\Prophecy\ObjectProphecy;
 use Sulu\Bundle\SecurityBundle\System\SystemStoreInterface;
 use Sulu\Component\Webspace\Analyzer\RequestAnalyzerInterface;
-use Sulu\Component\Webspace\Security;
-use Sulu\Component\Webspace\Webspace;
 use Symfony\Component\HttpKernel\Event\RequestEvent;
 
 class SystemListenerTest extends TestCase
@@ -46,33 +44,6 @@ public function testSetAdminSystem(): void
         $this->systemStore->setSystem('Sulu')->shouldBeCalled();
     }
 
-    public function provideSetWebsiteSystem()
-    {
-        return [
-            ['sulu-test'],
-            ['sulu-blog'],
-        ];
-    }
-
-    /**
-     * @dataProvider provideSetWebsiteSystem
-     */
-    public function testSetWebsiteSystem(string $system): void
-    {
-        $systemListener = $this->createSystemListener('website');
-        $requestEvent = $this->prophesize(RequestEvent::class);
-
-        $webspace = new Webspace();
-        $security = new Security();
-        $security->setSystem($system);
-        $webspace->setSecurity($security);
-        $this->requestAnalyzer->getWebspace()->willReturn($webspace);
-
-        $systemListener->onKernelRequest($requestEvent->reveal());
-
-        $this->systemStore->setSystem($system)->shouldBeCalled();
-    }
-
     private function createSystemListener(string $context): SystemListener
     {
         return new SystemListener($this->systemStore->reveal(), $this->requestAnalyzer->reveal(), $context);

diff --git a/src/Sulu/Bundle/WebsiteBundle/EventListener/RouterListener.php b/src/Sulu/Bundle/WebsiteBundle/EventListener/RouterListener.php
@@ -54,6 +54,7 @@ public function onKernelRequest(RequestEvent $event)
         // This call is required in all cases, because the default router needs our webspace information
         // Would be nice to also only call this if the _requestAnalyzer attribute is set, but it's set on the next line
         $this->requestAnalyzer->analyze($request);
+
         $this->baseRouteListener->onKernelRequest($event);
         if (false !== $request->attributes->getBoolean(static::REQUEST_ANALYZER, true)) {
             $this->requestAnalyzer->validate($request);

diff --git a/src/Sulu/Bundle/WebsiteBundle/EventListener/SecurityListener.php b/src/Sulu/Bundle/WebsiteBundle/EventListener/SecurityListener.php
@@ -0,0 +1,83 @@
+<?php
+
+/*
+ * This file is part of Sulu.
+ *
+ * (c) Sulu GmbH
+ *
+ * This source file is subject to the MIT license that is bundled
+ * with this source code in the file LICENSE.
+ */
+
+namespace Sulu\Bundle\WebsiteBundle\EventListener;
+
+use Sulu\Bundle\PageBundle\Document\BasePageDocument;
+use Sulu\Component\Content\Compat\Structure\PageBridge;
+use Sulu\Component\DocumentManager\Subscriber\EventSubscriberInterface;
+use Sulu\Component\Security\Authorization\PermissionTypes;
+use Sulu\Component\Security\Authorization\SecurityCheckerInterface;
+use Sulu\Component\Security\Authorization\SecurityCondition;
+use Sulu\Component\Webspace\Analyzer\Attributes\RequestAttributes;
+use Symfony\Component\HttpKernel\Event\RequestEvent;
+use Symfony\Component\HttpKernel\KernelEvents;
+
+class SecurityListener implements EventSubscriberInterface
+{
+    /**
+     * @var SecurityCheckerInterface|null
+     */
+    private $securityChecker;
+
+    public function __construct(
+        ?SecurityCheckerInterface $securityChecker = null
+    ) {
+        $this->securityChecker = $securityChecker;
+    }
+
+    public static function getSubscribedEvents(): array
+    {
+        return [
+            KernelEvents::REQUEST => [
+                ['onKernelRequest', 7], // set the security listener after the firewall and after the routing listener
+            ],
+        ];
+    }
+
+    public function onKernelRequest(RequestEvent $event): void
+    {
+        $request = $event->getRequest();
+
+        if (null === $this->securityChecker) {
+            return;
+        }
+
+        $requestAttributes = $request->attributes->get('_sulu');
+        if (!$requestAttributes instanceof RequestAttributes) {
+            return;
+        }
+
+        $webspace = $requestAttributes->getAttribute('webspace');
+
+        $structure = $request->attributes->get('structure');
+        if (!$structure instanceof PageBridge) {
+            return;
+        }
+
+        $document = $structure->getDocument();
+        if (!$document instanceof BasePageDocument) {
+            return;
+        }
+
+        if ($webspace->hasWebsiteSecurity()) {
+            $this->securityChecker->checkPermission(
+                new SecurityCondition(
+                    'sulu.webspaces.' . $document->getWebspaceName(),
+                    $document->getLocale(),
+                    \get_class($document),
+                    $document->getUuid()
+                ),
+                PermissionTypes::VIEW
+            );
+        }
+    }
+}
diff --git a/src/Sulu/Bundle/WebsiteBundle/Resources/config/services.xml b/src/Sulu/Bundle/WebsiteBundle/Resources/config/services.xml
@@ -299,6 +299,13 @@
             <tag name="kernel.event_subscriber"/>
         </service>
 
+        <service id="sulu_website.event_listener.security_listener"
+                 class="Sulu\Bundle\WebsiteBundle\EventListener\SecurityListener">
+            <argument type="service" id="sulu_security.security_checker" on-invalid="null"/>
+
+            <tag name="kernel.event_subscriber"/>
+        </service>
+
         <!-- reference-store -->
         <service id="sulu_website.reference_store_pool"
                  class="Sulu\Bundle\WebsiteBundle\ReferenceStore\ReferenceStorePool">

diff --git a/src/Sulu/Bundle/WebsiteBundle/Resources/config/website.xml b/src/Sulu/Bundle/WebsiteBundle/Resources/config/website.xml
@@ -21,7 +21,7 @@
             <argument type="service" id="sulu.content.structure_manager"/>
             <argument type="service" id="sulu_core.webspace.webspace_manager"/>
             <argument type="service" id="sulu_core.webspace.request_analyzer"/>
-            <argument type="service" id="sulu_security.security_checker" on-invalid="null"/>
+            <argument>null</argument><!-- not longer required -->
             <argument type="collection" />
 
             <tag name="sulu.context" context="website"/>

diff --git a/src/Sulu/Bundle/WebsiteBundle/Routing/ContentRouteProvider.php b/src/Sulu/Bundle/WebsiteBundle/Routing/ContentRouteProvider.php
@@ -24,16 +24,15 @@
 use Sulu\Component\Content\Exception\ResourceLocatorNotFoundException;
 use Sulu\Component\Content\Types\ResourceLocator\Strategy\ResourceLocatorStrategyPoolInterface;
 use Sulu\Component\DocumentManager\DocumentManagerInterface;
-use Sulu\Component\Security\Authorization\PermissionTypes;
 use Sulu\Component\Security\Authorization\SecurityCheckerInterface;
-use Sulu\Component\Security\Authorization\SecurityCondition;
 use Sulu\Component\Webspace\Analyzer\Attributes\RequestAttributes;
 use Sulu\Component\Webspace\Analyzer\RequestAnalyzerInterface;
 use Sulu\Component\Webspace\Manager\WebspaceManagerInterface;
 use Symfony\Cmf\Component\Routing\RouteProviderInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\Routing\Route;
 use Symfony\Component\Routing\RouteCollection;
+use Webmozart\Assert\Assert;
 
 /**
  * The PortalRouteProvider should load the dynamic routes created by Sulu.
@@ -97,6 +96,7 @@ public function __construct(
         $this->webspaceManager = $webspaceManager;
         $this->requestAnalyzer = $requestAnalyzer;
         $this->securityChecker = $securityChecker;
+        Assert::null($securityChecker, 'The security checker should be called by the SecurityListener not the ContentRouteProvider.'); // people who overwrite the ContentRouteProvider should make aware of that they also need to refactor this
         $this->defaultOptions = $defaultOptions;
     }
 
@@ -166,18 +166,6 @@ public function getRouteCollectionForRequest(Request $request)
                 return $collection;
             }
 
-            if ($this->securityChecker && $portal->getWebspace()->hasWebsiteSecurity()) {
-                $this->securityChecker->checkPermission(
-                    new SecurityCondition(
-                        'sulu.webspaces.' . $document->getWebspaceName(),
-                        $document->getLocale(),
-                        \get_class($document),
-                        $document->getUuid()
-                    ),
-                    PermissionTypes::VIEW
-                );
-            }
-
             if (\preg_match('/\/$/', $resourceLocator) && ('/' !== $resourceLocator || $prefix)) {
                 // redirect page to page without slash at the end
                 $url = $prefix . \rtrim($resourceLocator, '/');

diff --git a/src/Sulu/Bundle/WebsiteBundle/Tests/Application/Kernel.php b/src/Sulu/Bundle/WebsiteBundle/Tests/Application/Kernel.php
@@ -12,18 +12,27 @@
 namespace Sulu\Bundle\WebsiteBundle\Tests\Application;
 
 use Sulu\Bundle\TestBundle\Kernel\SuluTestKernel;
+use Symfony\Bundle\SecurityBundle\SecurityBundle;
 use Symfony\Component\Config\Loader\LoaderInterface;
 
 class Kernel extends SuluTestKernel
 {
+    /**
+     * @var string
+     */
+    private $appContext;
+
     /**
      * @param string $environment
      * @param bool $debug
      * @param string $suluContext
      */
     public function __construct($environment, $debug, $suluContext = self::CONTEXT_ADMIN)
     {
-        parent::__construct($environment, $debug, $suluContext);
+        $envParts = \explode('_', $environment, 2);
+        $this->appContext = $envParts[1] ?? '';
+
+        parent::__construct($envParts[0], $debug, $suluContext);
     }
 
     public function registerContainerConfiguration(LoaderInterface $loader): void
@@ -32,5 +41,35 @@ public function registerContainerConfiguration(LoaderInterface $loader): void
 
         $context = $this->getContext();
         $loader->load(__DIR__ . '/config/config_' . $context . '.yml');
+        if ('' !== $this->appContext) {
+            $loader->load(__DIR__ . '/config/config_' . $this->appContext . '.yml');
+        }
+    }
+
+    public function registerBundles(): iterable
+    {
+        $bundles = parent::registerBundles();
+
+        if ('with_security' === $this->appContext) {
+            $bundles[] = new SecurityBundle();
+        }
+
+        return $bundles;
+    }
+
+    /**
+     * @return string
+     */
+    public function getCacheDir()
+    {
+        return parent::getCacheDir() . \ltrim('/' . $this->appContext);
+    }
+
+    /**
+     * @return string
+     */
+    public function getCommonCacheDir()
+    {
+        return parent::getCommonCacheDir() . \ltrim('/' . $this->appContext);
     }
 }