Merge pull request #8709 from magento-performance/ACPT-1718

andimov · web-flow · commit 2ad9f140b5c1 · 2024-03-04T19:10:41.000-06:00
ACPT-1718: Fixing: Application Server does not support session/cookie authorization
diff --git a/app/code/Magento/Cookie/etc/di.xml b/app/code/Magento/Cookie/etc/di.xml
@@ -7,5 +7,6 @@
 -->
 <config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd">
     <preference for="Magento\Framework\Stdlib\CookieManagerInterface" type="Magento\Framework\Stdlib\Cookie\PhpCookieManager" />
+    <preference for="Magento\Framework\Stdlib\CookieDisablerInterface" type="Magento\Framework\Stdlib\Cookie\PhpCookieDisabler" />
 </config>
 
diff --git a/app/code/Magento/PageCache/Model/App/Response/HttpPlugin.php b/app/code/Magento/PageCache/Model/App/Response/HttpPlugin.php
@@ -22,10 +22,12 @@ class HttpPlugin
      */
     public function beforeSendResponse(HttpResponse $subject)
     {
-        if ($subject instanceof NotCacheableInterface || $subject->headersSent()) {
+        if ($subject instanceof NotCacheableInterface
+            || $subject->headersSent()
+            || $subject->getMetadata("NotCacheable")
+        ) {
             return;
         }
-
         $subject->sendVary();
     }
 }
diff --git a/app/etc/di.xml b/app/etc/di.xml
@@ -94,6 +94,7 @@
     <preference for="Magento\Framework\Stdlib\Cookie\CookieScopeInterface" type="Magento\Framework\Stdlib\Cookie\CookieScope" />
     <preference for="Magento\Framework\Stdlib\Cookie\CookieReaderInterface" type="Magento\Framework\Stdlib\Cookie\PhpCookieReader" />
     <preference for="Magento\Framework\Stdlib\CookieManagerInterface" type="Magento\Framework\Stdlib\Cookie\PhpCookieManager" />
+    <preference for="Magento\Framework\Stdlib\CookieDisablerInterface" type="Magento\Framework\Stdlib\Cookie\PhpCookieDisabler" />
     <preference for="Magento\Framework\TranslateInterface" type="Magento\Framework\Translate" />
     <preference for="Magento\Framework\Config\ScopeListInterface" type="interceptionConfigScope" />
     <preference for="Magento\Framework\View\Design\Theme\Label\ListInterface" type="Magento\Theme\Model\ResourceModel\Theme\Collection" />
diff --git a/dev/tests/api-functional/testsuite/Magento/GraphQl/GraphQl/GraphQlSessionTest.php b/dev/tests/api-functional/testsuite/Magento/GraphQl/GraphQl/GraphQlSessionTest.php
@@ -48,6 +48,10 @@ public function setUp(): void
     /**
      * Test for checking if graphQL query sets session cookies
      *
+     * Note: The reason why the first response doesn't have cookies, but the subsequent responses do is
+     * because Magento/Framework/App/PageCache/Kernel.php removes Set-Cookie headers when the response has a
+     * public Cache-Control.  This test asserts that behaviour.
+     *
      * @magentoApiDataFixture Magento/Catalog/_files/categories.php
      * @magentoConfigFixture graphql/session/disable 0
      */
@@ -71,8 +75,7 @@ public function testCheckSessionCookieWithGetCategoryList(): void
         $result = $this->graphQlClient->getWithResponseHeaders($query, [], '', [], true);
         $this->assertEmpty($result['cookies']);
         // perform secondary request after cookies have been flushed
-        $result = $this->graphQlClient->getWithResponseHeaders($query, [], '', []);
-
+        $result = $this->graphQlClient->getWithResponseHeaders($query, [], '', [], true);
         // may have other cookies than session
         $this->assertNotEmpty($result['cookies']);
         $this->assertAnyCookieMatchesRegex('/PHPSESSID=[a-z0-9]+;/', $result['cookies']);
@@ -280,4 +283,46 @@ private function assertNoCookiesMatchRegex(string $pattern, array $cookies): voi
         }
         $this->assertTrue($result, 'Failed assertion. At least one cookie in the array matches pattern: ' . $pattern);
     }
+
+    /**
+     * Tests that Magento\Customer\Model\Session works properly when graphql/session/disable=0
+     *
+     * @magentoApiDataFixture Magento/Customer/_files/customer.php
+     * @magentoConfigFixture graphql/session/disable 0
+     */
+    public function testCustomerCanQueryOwnEmailUsingSession() : void
+    {
+        $query = '{customer{email}}';
+        $result = $this->graphQlClient->postWithResponseHeaders($query, [], '', $this->getAuthHeaders(), true);
+        // cookies are never empty and session is restarted for the authorized customer regardless current session
+        $this->assertNotEmpty($result['cookies']);
+        $this->assertAnyCookieMatchesRegex('/PHPSESSID=[a-z0-9]+;/', $result['cookies']);
+        $this->assertEquals('customer@example.com', $result['body']['customer']['email'] ?? '');
+        $result = $this->graphQlClient->postWithResponseHeaders($query, [], '', $this->getAuthHeaders());
+        // cookies are never empty and session is restarted for the authorized customer
+        // regardless current session and missing flush
+        $this->assertNotEmpty($result['cookies']);
+        $this->assertAnyCookieMatchesRegex('/PHPSESSID=[a-z0-9]+;/', $result['cookies']);
+        $this->assertEquals('customer@example.com', $result['body']['customer']['email'] ?? '');
+        /* Note: This third request is the actual one that tests that the session cookie is properly used.
+         * This time we don't send the Authorization header and rely on Cookie header instead.
+         * Because of bug in postWithResponseHeaders's $flushCookies parameter not being properly used,
+         * We have to manually set cookie header ourselves. :-(
+         */
+        $cookiesToSend = '';
+        foreach ($result['cookies'] as $cookie) {
+            preg_match('/^([^;]*);/', $cookie, $matches);
+            if (!strlen($matches[1] ?? '')) {
+                continue;
+            }
+            if (!empty($cookiesToSend)) {
+                $cookiesToSend .= '; ';
+            }
+            $cookiesToSend .= $matches[1];
+        }
+        $result = $this->graphQlClient->postWithResponseHeaders($query, [], '', ['Cookie: ' . $cookiesToSend]);
+        $this->assertNotEmpty($result['cookies']);
+        $this->assertAnyCookieMatchesRegex('/PHPSESSID=[a-z0-9]+;/', $result['cookies']);
+        $this->assertEquals('customer@example.com', $result['body']['customer']['email'] ?? '');
+    }
 }
diff --git a/lib/internal/Magento/Framework/App/PageCache/Kernel.php b/lib/internal/Magento/Framework/App/PageCache/Kernel.php
@@ -7,6 +7,7 @@
 
 use Magento\Framework\App\State as AppState;
 use Magento\Framework\App\ObjectManager;
+use Magento\Framework\Stdlib\CookieDisablerInterface;
 
 /**
  * Builtin cache processor
@@ -68,6 +69,9 @@ class Kernel
      */
     private $identifierForSave;
 
+    // phpcs:disable Magento2.Commenting.ClassPropertyPHPDocFormatting
+    private readonly CookieDisablerInterface $cookieDisabler;
+
     /**
      * @param Cache $cache
      * @param \Magento\Framework\App\PageCache\IdentifierInterface $identifier
@@ -79,6 +83,7 @@ class Kernel
      * @param AppState|null $state
      * @param \Magento\PageCache\Model\Cache\Type|null $fullPageCache
      * @param  \Magento\Framework\App\PageCache\IdentifierInterface|null $identifierForSave
+     * @param CookieDisablerInterface|null $cookieDisabler
      * @SuppressWarnings(PHPMD.ExcessiveParameterList)
      */
     public function __construct(
@@ -91,7 +96,8 @@ public function __construct(
         \Magento\Framework\Serialize\SerializerInterface $serializer = null,
         AppState $state = null,
         \Magento\PageCache\Model\Cache\Type $fullPageCache = null,
-        \Magento\Framework\App\PageCache\IdentifierInterface $identifierForSave = null
+        \Magento\Framework\App\PageCache\IdentifierInterface $identifierForSave = null,
+        ?CookieDisablerInterface $cookieDisabler = null,
     ) {
         $this->cache = $cache;
         $this->identifier = $identifier;
@@ -113,6 +119,7 @@ public function __construct(
         $this->identifierForSave = $identifierForSave ?? ObjectManager::getInstance()->get(
             \Magento\Framework\App\PageCache\IdentifierInterface::class
         );
+        $this->cookieDisabler = $cookieDisabler ?? ObjectManager::getInstance()->get(CookieDisablerInterface::class);
     }
 
     /**
@@ -163,9 +170,7 @@ public function process(\Magento\Framework\App\Response\Http $response)
                 if ($this->state->getMode() != AppState::MODE_DEVELOPER) {
                     $response->clearHeader('X-Magento-Tags');
                 }
-                if (!headers_sent()) {
-                    header_remove('Set-Cookie');
-                }
+                $this->cookieDisabler->setCookiesDisabled(true);
 
                 $this->fullPageCache->save(
                     $this->serializer->serialize($this->getPreparedData($response)),
diff --git a/lib/internal/Magento/Framework/Session/SaveHandler/Redis.php b/lib/internal/Magento/Framework/Session/SaveHandler/Redis.php
@@ -9,6 +9,7 @@
 use Cm\RedisSession\Handler\LoggerInterface;
 use Cm\RedisSession\ConnectionFailedException;
 use Cm\RedisSession\ConcurrentConnectionsExceededException;
+use Magento\Framework\Exception\LocalizedException;
 use Magento\Framework\Exception\SessionException;
 use Magento\Framework\Phrase;
 use Magento\Framework\Filesystem;
@@ -82,7 +83,7 @@ public function read($sessionId)
         try {
             $result = $this->getConnection()->read($sessionId);
         } catch (ConcurrentConnectionsExceededException $e) {
-            require $this->filesystem->getDirectoryRead(DirectoryList::PUB)->getAbsolutePath('errors/503.php');
+            throw new LocalizedException(__("Redis session exceeded concurrent connections"), $e);
         }
 
         return $result;
diff --git a/lib/internal/Magento/Framework/Session/SessionManager.php b/lib/internal/Magento/Framework/Session/SessionManager.php
@@ -638,13 +638,6 @@ private function initIniOptions()
      */
     public function _resetState(): void
     {
-        if (session_status() === PHP_SESSION_ACTIVE) {
-            session_write_close();
-            session_id('');
-        }
-        session_name('PHPSESSID');
-        session_unset();
         static::$urlHostCache = [];
-        $_SESSION = [];
     }
 }
diff --git a/lib/internal/Magento/Framework/Stdlib/Cookie/PhpCookieDisabler.php b/lib/internal/Magento/Framework/Stdlib/Cookie/PhpCookieDisabler.php
@@ -0,0 +1,26 @@
+<?php
+/**
+ * Copyright © Magento, Inc. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+declare(strict_types=1);
+
+namespace Magento\Framework\Stdlib\Cookie;
+
+use Magento\Framework\Stdlib\CookieDisablerInterface;
+
+/**
+ * Disables sending the cookies that are currently set.
+ */
+class PhpCookieDisabler implements CookieDisablerInterface
+{
+    /**
+     * @inheritDoc
+     */
+    public function setCookiesDisabled(bool $disabled) : void
+    {
+        if ($disabled && !headers_sent()) {
+            header_remove('Set-Cookie');
+        }
+    }
+}
diff --git a/lib/internal/Magento/Framework/Stdlib/CookieDisablerInterface.php b/lib/internal/Magento/Framework/Stdlib/CookieDisablerInterface.php
@@ -0,0 +1,22 @@
+<?php
+/**
+ * Copyright © Magento, Inc. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+declare(strict_types=1);
+
+namespace Magento\Framework\Stdlib;
+
+/**
+ * This interface is for when you need to disable all cookies from being sent in the HTTP response
+ */
+interface CookieDisablerInterface
+{
+    /**
+     * Set Cookies Disabled.  If true, cookies won't be sent.
+     *
+     * @param bool $disabled
+     * @return void
+     */
+    public function setCookiesDisabled(bool $disabled) : void;
+}
diff --git a/pub/errors/default/page.phtml b/pub/errors/default/page.phtml
@@ -19,7 +19,7 @@
 </head>
 <body>
     <main class="page-main">
-        <?php require_once $contentTemplate; ?>
+        <?php require $contentTemplate; ?>
     </main>
 </body>
 </html>
diff --git a/pub/errors/processor.php b/pub/errors/processor.php
@@ -188,25 +188,23 @@ public function __construct(
         $this->serializer = $serializer ?: ObjectManager::getInstance()->get(Json::class);
         $this->escaper = $escaper ?: ObjectManager::getInstance()->get(Escaper::class);
         $this->documentRoot = $documentRoot ?? ObjectManager::getInstance()->get(DocumentRoot::class);
-
         if (!empty($_SERVER['SCRIPT_NAME'])) {
             if (in_array(basename($_SERVER['SCRIPT_NAME'], '.php'), ['404', '503', 'report'])) {
                 $this->_scriptName = dirname($_SERVER['SCRIPT_NAME']);
             } else {
                 $this->_scriptName = $_SERVER['SCRIPT_NAME'];
             }
         }
-
         $this->_indexDir = $this->_getIndexDir();
         $this->_root  = is_dir($this->_indexDir . 'app');
-
         $this->_prepareConfig();
         if (isset($_GET['skin'])) {
             $this->_setSkin($_GET['skin']);
         }
         if (isset($_GET['id'])) {
             $this->loadReport($_GET['id']);
         }
+        $response->setMetadata("NotCacheable", true);
     }
 
     /**
@@ -449,7 +447,7 @@ protected function _renderPage($template)
         $html = '';
         if ($baseTemplate && $contentTemplate) {
             ob_start();
-            require_once $baseTemplate;
+            require $baseTemplate;
             $html = ob_get_clean();
         }
         return $html;
diff --git a/pub/errors/processorFactory.php b/pub/errors/processorFactory.php
@@ -3,10 +3,13 @@
  * Copyright © Magento, Inc. All rights reserved.
  * See COPYING.txt for license details.
  */
+// phpcs:disable PSR1.Files.SideEffects
 namespace Magento\Framework\Error;
 
+// phpcs:ignore Magento2.Functions.DiscouragedFunction,Magento2.Security.IncludeFile
 require_once realpath(__DIR__) . '/../../app/bootstrap.php';
-require_once 'processor.php';
+require_once 'processor.php'; // phpcs:ignore Magento2.Security.IncludeFile
+use Magento\Framework\App\ObjectManager as AppObjectManager;
 
 /**
  * Error processor factory
@@ -20,9 +23,15 @@ class ProcessorFactory
      */
     public function createProcessor()
     {
-        $objectManagerFactory = \Magento\Framework\App\Bootstrap::createObjectManagerFactory(BP, $_SERVER);
-        $objectManager = $objectManagerFactory->create($_SERVER);
-        $response = $objectManager->create(\Magento\Framework\App\Response\Http::class);
-        return new Processor($response);
+        try {
+            $objectManager = AppObjectManager::getInstance();
+            return $objectManager->create(Processor::class);
+        } catch (\RuntimeException $exception) {
+            // phpcs:ignore Magento2.Security.Superglobal
+            $objectManagerFactory = \Magento\Framework\App\Bootstrap::createObjectManagerFactory(BP, $_SERVER);
+            $objectManager = $objectManagerFactory->create($_SERVER); // phpcs:ignore Magento2.Security.Superglobal
+            $response = $objectManager->create(\Magento\Framework\App\Response\Http::class);
+            return new Processor($response);
+        }
     }
 }
diff --git a/pub/get.php b/pub/get.php
@@ -62,13 +62,13 @@
             $fileRelativePath = str_replace(rtrim($mediaDirectory, '/') . '/', '', $fileAbsolutePath);
 
             if (!$isAllowed($fileRelativePath, $allowedResources)) {
-                require_once 'errors/404.php';
+                require 'errors/404.php';
                 exit;
             }
 
             if (is_readable($fileAbsolutePath)) {
                 if (is_dir($fileAbsolutePath)) {
-                    require_once 'errors/404.php';
+                    require 'errors/404.php';
                     exit;
                 }
 

Original file line number	Diff line number	Diff line change
`@@ -22,10 +22,12 @@ class HttpPlugin`
`22`	`22`	`*/`
`23`	`23`	`public function beforeSendResponse(HttpResponse $subject)`
`24`	`24`	`{`
`25`		`- if ($subject instanceof NotCacheableInterface \|\| $subject->headersSent()) {`
	`25`	`+ if ($subject instanceof NotCacheableInterface`
	`26`	`+ \|\| $subject->headersSent()`
	`27`	`+ \|\| $subject->getMetadata("NotCacheable")`
	`28`	`+ ) {`
`26`	`29`	`return;`
`27`	`30`	`}`
`28`		`-`
`29`	`31`	`$subject->sendVary();`
`30`	`32`	`}`
`31`	`33`	`}`
Original file line number	Diff line number	Diff line change
`@@ -638,13 +638,6 @@ private function initIniOptions()`
`638`	`638`	`*/`
`639`	`639`	`public function _resetState(): void`
`640`	`640`	`{`
`641`		`- if (session_status() === PHP_SESSION_ACTIVE) {`
`642`		`- session_write_close();`
`643`		`- session_id('');`
`644`		`- }`
`645`		`- session_name('PHPSESSID');`
`646`		`- session_unset();`
`647`	`641`	`static::$urlHostCache = [];`
`648`		`- $_SESSION = [];`
`649`	`642`	`}`
`650`	`643`	`}`