Merge pull request #31235 from nextcloud/techdebt/noid/extract-request-id

Extract request id handling to dedicated class so it can be injected without DB dependency

Author: nickvergessen

apps/dav/tests/unit/Connector/Sabre/FileTest.php

@@ -35,14 +35,14 @@
 use OC\Files\Storage\Temporary;
 use OC\Files\Storage\Wrapper\PermissionsMask;
 use OC\Files\View;
-use OC\Security\SecureRandom;
 use OCA\DAV\Connector\Sabre\File;
 use OCP\Constants;
 use OCP\Files\ForbiddenException;
 use OCP\Files\Storage;
 use OCP\IConfig;
+use OCP\IRequestId;
 use OCP\Lock\ILockingProvider;
-use OCP\Security\ISecureRandom;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\HookHelper;
 use Test\TestCase;
 use Test\Traits\MountProviderTrait;
@@ -64,11 +64,11 @@ class FileTest extends TestCase {
 	 */
 	private $user;
 
-	/** @var IConfig | \PHPUnit\Framework\MockObject\MockObject */
+	/** @var IConfig|MockObject */
 	protected $config;
 
-	/** @var ISecureRandom */
-	protected $secureRandom;
+	/** @var IRequestId|MockObject */
+	protected $requestId;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -83,8 +83,8 @@ protected function setUp(): void {
 
 		$this->loginAsUser($this->user);
 
-		$this->config = $this->getMockBuilder('\OCP\IConfig')->getMock();
-		$this->secureRandom = new SecureRandom();
+		$this->config = $this->createMock(IConfig::class);
+		$this->requestId = $this->createMock(IRequestId::class);
 	}
 
 	protected function tearDown(): void {
@@ -96,7 +96,7 @@ protected function tearDown(): void {
 	}
 
 	/**
-	 * @return \PHPUnit\Framework\MockObject\MockObject|Storage
+	 * @return MockObject|Storage
 	 */
 	private function getMockStorage() {
 		$storage = $this->getMockBuilder(Storage::class)
@@ -184,7 +184,7 @@ public function testSimplePutFails($thrownException, $expectedException, $checkP
 			->setConstructorArgs([['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]])
 			->getMock();
 		\OC\Files\Filesystem::mount($storage, [], $this->user . '/');
-		/** @var View | \PHPUnit\Framework\MockObject\MockObject $view */
+		/** @var View | MockObject $view */
 		$view = $this->getMockBuilder(View::class)
 			->setMethods(['getRelativePath', 'resolvePath'])
 			->getMock();
@@ -330,7 +330,7 @@ private function doPut($path, $viewRoot = null, Request $request = null) {
 			null
 		);
 
-		/** @var \OCA\DAV\Connector\Sabre\File | \PHPUnit\Framework\MockObject\MockObject $file */
+		/** @var \OCA\DAV\Connector\Sabre\File | MockObject $file */
 		$file = $this->getMockBuilder(\OCA\DAV\Connector\Sabre\File::class)
 			->setConstructorArgs([$view, $info, null, $request])
 			->setMethods(['header'])
@@ -416,7 +416,7 @@ public function testPutSingleFileLegalMtime($requestMtime, $resultMtime) {
 			'server' => [
 				'HTTP_X_OC_MTIME' => $requestMtime,
 			]
-		], $this->secureRandom, $this->config, null);
+		], $this->requestId, $this->config, null);
 		$file = 'foo.txt';
 
 		if ($resultMtime === null) {
@@ -439,7 +439,7 @@ public function testChunkedPutLegalMtime($requestMtime, $resultMtime) {
 			'server' => [
 				'HTTP_X_OC_MTIME' => $requestMtime,
 			]
-		], $this->secureRandom, $this->config, null);
+		], $this->requestId, $this->config, null);
 
 		$_SERVER['HTTP_OC_CHUNKED'] = true;
 		$file = 'foo.txt';

lib/base.php

@@ -161,7 +161,11 @@ public static function initPaths() {
 				'SCRIPT_FILENAME' => $_SERVER['SCRIPT_FILENAME'],
 			],
 		];
-		$fakeRequest = new \OC\AppFramework\Http\Request($params, new \OC\Security\SecureRandom(), new \OC\AllConfig(new \OC\SystemConfig(self::$config)));
+		$fakeRequest = new \OC\AppFramework\Http\Request(
+			$params,
+			new \OC\AppFramework\Http\RequestId($_SERVER['UNIQUE_ID'] ?? '', new \OC\Security\SecureRandom()),
+			new \OC\AllConfig(new \OC\SystemConfig(self::$config))
+		);
 		$scriptName = $fakeRequest->getScriptName();
 		if (substr($scriptName, -1) == '/') {
 			$scriptName .= 'index.php';

lib/composer/composer/autoload_classmap.php

@@ -412,6 +412,7 @@
     'OCP\\INavigationManager' => $baseDir . '/lib/public/INavigationManager.php',
     'OCP\\IPreview' => $baseDir . '/lib/public/IPreview.php',
     'OCP\\IRequest' => $baseDir . '/lib/public/IRequest.php',
+    'OCP\\IRequestId' => $baseDir . '/lib/public/IRequestId.php',
     'OCP\\ISearch' => $baseDir . '/lib/public/ISearch.php',
     'OCP\\IServerContainer' => $baseDir . '/lib/public/IServerContainer.php',
     'OCP\\ISession' => $baseDir . '/lib/public/ISession.php',
@@ -635,6 +636,7 @@
     'OC\\AppFramework\\Http\\Dispatcher' => $baseDir . '/lib/private/AppFramework/Http/Dispatcher.php',
     'OC\\AppFramework\\Http\\Output' => $baseDir . '/lib/private/AppFramework/Http/Output.php',
     'OC\\AppFramework\\Http\\Request' => $baseDir . '/lib/private/AppFramework/Http/Request.php',
+    'OC\\AppFramework\\Http\\RequestId' => $baseDir . '/lib/private/AppFramework/Http/RequestId.php',
     'OC\\AppFramework\\Logger' => $baseDir . '/lib/private/AppFramework/Logger.php',
     'OC\\AppFramework\\Middleware\\AdditionalScriptsMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/AdditionalScriptsMiddleware.php',
     'OC\\AppFramework\\Middleware\\CompressionMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/CompressionMiddleware.php',

lib/composer/composer/autoload_static.php

@@ -441,6 +441,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
         'OCP\\INavigationManager' => __DIR__ . '/../../..' . '/lib/public/INavigationManager.php',
         'OCP\\IPreview' => __DIR__ . '/../../..' . '/lib/public/IPreview.php',
         'OCP\\IRequest' => __DIR__ . '/../../..' . '/lib/public/IRequest.php',
+        'OCP\\IRequestId' => __DIR__ . '/../../..' . '/lib/public/IRequestId.php',
         'OCP\\ISearch' => __DIR__ . '/../../..' . '/lib/public/ISearch.php',
         'OCP\\IServerContainer' => __DIR__ . '/../../..' . '/lib/public/IServerContainer.php',
         'OCP\\ISession' => __DIR__ . '/../../..' . '/lib/public/ISession.php',
@@ -664,6 +665,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
         'OC\\AppFramework\\Http\\Dispatcher' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Dispatcher.php',
         'OC\\AppFramework\\Http\\Output' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Output.php',
         'OC\\AppFramework\\Http\\Request' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Request.php',
+        'OC\\AppFramework\\Http\\RequestId' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/RequestId.php',
         'OC\\AppFramework\\Logger' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Logger.php',
         'OC\\AppFramework\\Middleware\\AdditionalScriptsMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/AdditionalScriptsMiddleware.php',
         'OC\\AppFramework\\Middleware\\CompressionMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/CompressionMiddleware.php',

lib/private/AppFramework/Http/Request.php

@@ -48,8 +48,8 @@
 use OC\Security\TrustedDomainHelper;
 use OCP\IConfig;
 use OCP\IRequest;
+use OCP\IRequestId;
 use OCP\Security\ICrypto;
-use OCP\Security\ISecureRandom;
 
 /**
  * Class for accessing variables in the request.
@@ -92,12 +92,10 @@ class Request implements \ArrayAccess, \Countable, IRequest {
 		'method',
 		'requesttoken',
 	];
-	/** @var ISecureRandom */
-	protected $secureRandom;
+	/** @var RequestId */
+	protected $requestId;
 	/** @var IConfig */
 	protected $config;
-	/** @var string */
-	protected $requestId = '';
 	/** @var ICrypto */
 	protected $crypto;
 	/** @var CsrfTokenManager|null */
@@ -117,20 +115,20 @@ class Request implements \ArrayAccess, \Countable, IRequest {
 	 *        - array 'cookies' the $_COOKIE array
 	 *        - string 'method' the request method (GET, POST etc)
 	 *        - string|false 'requesttoken' the requesttoken or false when not available
-	 * @param ISecureRandom $secureRandom
+	 * @param IRequestId $requestId
 	 * @param IConfig $config
 	 * @param CsrfTokenManager|null $csrfTokenManager
 	 * @param string $stream
 	 * @see https://www.php.net/manual/en/reserved.variables.php
 	 */
 	public function __construct(array $vars,
-								ISecureRandom $secureRandom,
+								IRequestId $requestId,
 								IConfig $config,
 								CsrfTokenManager $csrfTokenManager = null,
 								string $stream = 'php://input') {
 		$this->inputStream = $stream;
 		$this->items['params'] = [];
-		$this->secureRandom = $secureRandom;
+		$this->requestId = $requestId;
 		$this->config = $config;
 		$this->csrfTokenManager = $csrfTokenManager;
 
@@ -571,16 +569,7 @@ public function passesLaxCookieCheck(): bool {
 	 * @return string
 	 */
 	public function getId(): string {
-		if (isset($this->server['UNIQUE_ID'])) {
-			return $this->server['UNIQUE_ID'];
-		}
-
-		if (empty($this->requestId)) {
-			$validChars = ISecureRandom::CHAR_ALPHANUMERIC;
-			$this->requestId = $this->secureRandom->generate(20, $validChars);
-		}
-
-		return $this->requestId;
+		return $this->requestId->getId();
 	}
 
 	/**

lib/private/AppFramework/Http/RequestId.php

@@ -0,0 +1,52 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2022, Joas Schilling <coding@schilljs.com>
+ *
+ * @author Joas Schilling <coding@schilljs.com>
+ *
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program. If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+namespace OC\AppFramework\Http;
+
+use OCP\IRequestId;
+use OCP\Security\ISecureRandom;
+
+class RequestId implements IRequestId {
+	protected ISecureRandom $secureRandom;
+	protected string $requestId;
+
+	public function __construct(string $uniqueId,
+								ISecureRandom $secureRandom) {
+		$this->requestId = $uniqueId;
+		$this->secureRandom = $secureRandom;
+	}
+
+	/**
+	 * Returns an ID for the request, value is not guaranteed to be unique and is mostly meant for logging
+	 * If `mod_unique_id` is installed this value will be taken.
+	 * @return string
+	 */
+	public function getId(): string {
+		if (empty($this->requestId)) {
+			$validChars = ISecureRandom::CHAR_ALPHANUMERIC;
+			$this->requestId = $this->secureRandom->generate(20, $validChars);
+		}
+
+		return $this->requestId;
+	}
+}

lib/private/DB/Connection.php

@@ -53,6 +53,7 @@
 use OC\SystemConfig;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\ILogger;
+use OCP\IRequestId;
 use OCP\PreConditionNotMetException;
 
 class Connection extends \Doctrine\DBAL\Connection {
@@ -281,11 +282,16 @@ public function executeStatement($sql, array $params = [], array $types = []): i
 	}
 
 	protected function logQueryToFile(string $sql): void {
-		$logFile = $this->systemConfig->getValue('query_log_file', '');
+		$logFile = $this->systemConfig->getValue('query_log_file');
 		if ($logFile !== '' && is_writable(dirname($logFile)) && (!file_exists($logFile) || is_writable($logFile))) {
+			$prefix = '';
+			if ($this->systemConfig->getValue('query_log_file_requestid') === 'yes') {
+				$prefix .= \OC::$server->get(IRequestId::class)->getId() . "\t";
+			}
+
 			file_put_contents(
 				$this->systemConfig->getValue('query_log_file', ''),
-				$sql . "\n",
+				$prefix . $sql . "\n",
 				FILE_APPEND
 			);
 		}

lib/private/Server.php

@@ -60,6 +60,7 @@
 use OC\App\AppStore\Fetcher\CategoryFetcher;
 use OC\AppFramework\Bootstrap\Coordinator;
 use OC\AppFramework\Http\Request;
+use OC\AppFramework\Http\RequestId;
 use OC\AppFramework\Utility\TimeFactory;
 use OC\Authentication\Events\LoginFailed;
 use OC\Authentication\Listeners\LoginFailedListener;
@@ -205,6 +206,7 @@
 use OCP\INavigationManager;
 use OCP\IPreview;
 use OCP\IRequest;
+use OCP\IRequestId;
 use OCP\ISearch;
 use OCP\IServerContainer;
 use OCP\ISession;
@@ -1033,7 +1035,7 @@ public function __construct($webRoot, \OC\Config $config) {
 						: '',
 					'urlParams' => $urlParams,
 				],
-				$this->get(ISecureRandom::class),
+				$this->get(IRequestId::class),
 				$this->get(\OCP\IConfig::class),
 				$this->get(CsrfTokenManager::class),
 				$stream
@@ -1042,6 +1044,13 @@ public function __construct($webRoot, \OC\Config $config) {
 		/** @deprecated 19.0.0 */
 		$this->registerDeprecatedAlias('Request', \OCP\IRequest::class);
 
+		$this->registerService(IRequestId::class, function (ContainerInterface $c): IRequestId {
+			return new RequestId(
+				$_SERVER['UNIQUE_ID'] ?? '',
+				$this->get(ISecureRandom::class)
+			);
+		});
+
 		$this->registerService(IMailer::class, function (Server $c) {
 			return new Mailer(
 				$c->get(\OCP\IConfig::class),
@@ -1214,7 +1223,7 @@ public function __construct($webRoot, \OC\Config $config) {
 		$this->registerAlias(EventDispatcherInterface::class, \OC\EventDispatcher\SymfonyAdapter::class);
 
 		$this->registerService('CryptoWrapper', function (ContainerInterface $c) {
-			// FIXME: Instantiiated here due to cyclic dependency
+			// FIXME: Instantiated here due to cyclic dependency
 			$request = new Request(
 				[
 					'get' => $_GET,
@@ -1227,7 +1236,7 @@ public function __construct($webRoot, \OC\Config $config) {
 						? $_SERVER['REQUEST_METHOD']
 						: null,
 				],
-				$c->get(ISecureRandom::class),
+				$c->get(IRequestId::class),
 				$c->get(\OCP\IConfig::class)
 			);
 

lib/public/IRequestId.php

@@ -0,0 +1,39 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2022, Joas Schilling <coding@schilljs.com>
+ *
+ * @author Joas Schilling <coding@schilljs.com>
+ *
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program. If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OCP;
+
+/**
+ * @since 24.0.0
+ */
+interface IRequestId {
+	/**
+	 * Returns an ID for the request, value is not guaranteed to be unique and is mostly meant for logging
+	 * If `mod_unique_id` is installed this value will be taken.
+	 *
+	 * @return string
+	 * @since 24.0.0
+	 */
+	public function getId(): string;
+}