fix: Do not DI the database connection to prevent cyclic dependency

Signed-off-by: Ferdinand Thiessen <[email protected]>
nextcloud · Aug 21, 2024 · 35d6505 · 35d6505
1 parent ae720d6
commit 35d6505
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 30 deletions.
diff --git a/lib/private/Files/FilenameValidator.php b/lib/private/Files/FilenameValidator.php
@@ -27,6 +27,8 @@ class FilenameValidator implements IFilenameValidator {
 
 	private IL10N $l10n;
 
+	private ?IDBConnection $database;
+
 	/**
 	 * @var list<string>
 	 */
@@ -48,7 +50,6 @@ class FilenameValidator implements IFilenameValidator {
 
 	public function __construct(
 		IFactory $l10nFactory,
-		private IDBConnection $database,
 		private IConfig $config,
 		private LoggerInterface $logger,
 	) {
@@ -187,6 +188,11 @@ public function validateFilename(string $filename): void {
 			throw new FileNameTooLongException();
 		}
 
+		// We need to lazy load the database as otherwise there is a cyclic dependency
+		if (!isset($this->database)) {
+			$this->database = \OCP\Server::get(IDBConnection::class);
+		}
+
 		if (!$this->database->supports4ByteText()) {
 			// verify database - e.g. mysql only 3-byte chars
 			if (preg_match('%(?:

diff --git a/tests/lib/Files/FilenameValidatorTest.php b/tests/lib/Files/FilenameValidatorTest.php
@@ -24,12 +24,16 @@
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
+/**
+ * @group DB
+ */
 class FilenameValidatorTest extends TestCase {
 
 	protected IFactory&MockObject $l10n;
 	protected IConfig&MockObject $config;
 	protected IDBConnection&MockObject $database;
 	protected LoggerInterface&MockObject $logger;
+	protected bool $dbSupportsUtf8 = true;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -45,7 +49,13 @@ protected function setUp(): void {
 		$this->config = $this->createMock(IConfig::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->database = $this->createMock(IDBConnection::class);
-		$this->database->method('supports4ByteText')->willReturn(true);
+		$this->database->method('supports4ByteText')->willReturnCallback(fn () => $this->dbSupportsUtf8);
+		$this->overwriteService(IDBConnection::class, $this->database);
+	}
+
+	protected function tearDown(): void {
+		$this->restoreAllServices();
+		parent::tearDown();
 	}
 
 	/**
@@ -67,7 +77,7 @@ public function testValidateFilename(
 				'getForbiddenExtensions',
 				'getForbiddenFilenames',
 			])
-			->setConstructorArgs([$this->l10n, $this->database, $this->config, $this->logger])
+			->setConstructorArgs([$this->l10n, $this->config, $this->logger])
 			->getMock();
 
 		$validator->method('getForbiddenBasenames')
@@ -106,7 +116,7 @@ public function testIsFilenameValid(
 				'getForbiddenFilenames',
 				'getForbiddenCharacters',
 			])
-			->setConstructorArgs([$this->l10n, $this->database, $this->config, $this->logger])
+			->setConstructorArgs([$this->l10n, $this->config, $this->logger])
 			->getMock();
 
 		$validator->method('getForbiddenBasenames')
@@ -186,20 +196,17 @@ public function dataValidateFilename(): array {
 	 * @dataProvider data4ByteUnicode
 	 */
 	public function testDatabaseDoesNotSupport4ByteText($filename): void {
-		$database = $this->createMock(IDBConnection::class);
-		$database->expects($this->once())
-			->method('supports4ByteText')
-			->willReturn(false);
+		$this->dbSupportsUtf8 = false;
+
 		$this->expectException(InvalidCharacterInPathException::class);
-		$validator = new FilenameValidator($this->l10n, $database, $this->config, $this->logger);
+		$validator = new FilenameValidator($this->l10n, $this->config, $this->logger);
 		$validator->validateFilename($filename);
 	}
 
 	public function data4ByteUnicode(): array {
 		return [
 			['plane 1 𐪅'],
 			['emoji 😶‍🌫️'],
-
 		];
 	}
 
@@ -208,7 +215,7 @@ public function data4ByteUnicode(): array {
 	 */
 	public function testInvalidAsciiCharactersAreAlwaysForbidden(string $filename): void {
 		$this->expectException(InvalidPathException::class);
-		$validator = new FilenameValidator($this->l10n, $this->database, $this->config, $this->logger);
+		$validator = new FilenameValidator($this->l10n, $this->config, $this->logger);
 		$validator->validateFilename($filename);
 	}
 
@@ -252,15 +259,13 @@ public function dataInvalidAsciiCharacters(): array {
 	/**
 	 * @dataProvider dataIsForbidden
 	 */
-	public function testIsForbidden(string $filename, array $forbiddenNames, array $forbiddenBasenames, bool $expected): void {
+	public function testIsForbidden(string $filename, array $forbiddenNames, bool $expected): void {
 		/** @var FilenameValidator&MockObject */
 		$validator = $this->getMockBuilder(FilenameValidator::class)
 			->onlyMethods(['getForbiddenFilenames', 'getForbiddenBasenames'])
-			->setConstructorArgs([$this->l10n, $this->database, $this->config, $this->logger])
+			->setConstructorArgs([$this->l10n, $this->config, $this->logger])
 			->getMock();
 
-		$validator->method('getForbiddenBasenames')
-			->willReturn($forbiddenBasenames);
 		$validator->method('getForbiddenFilenames')
 			->willReturn($forbiddenNames);
 
@@ -270,27 +275,19 @@ public function testIsForbidden(string $filename, array $forbiddenNames, array $
 	public function dataIsForbidden(): array {
 		return [
 			'valid name' => [
-				'a: b.txt', ['.htaccess'], [], false
+				'a: b.txt', ['.htaccess'], false
 			],
 			'valid name with some more parameters' => [
-				'a: b.txt', ['.htaccess'], [], false
+				'a: b.txt', ['.htaccess'], false
 			],
 			'valid name as only full forbidden should be matched' => [
-				'.htaccess.sample', ['.htaccess'], [], false,
+				'.htaccess.sample', ['.htaccess'], false,
 			],
 			'forbidden name' => [
-				'.htaccess', ['.htaccess'], [], true
+				'.htaccess', ['.htaccess'], true
 			],
 			'forbidden name - name is case insensitive' => [
-				'COM1', ['.htaccess', 'com1'], [], true,
-			],
-			'forbidden name - basename is checked' => [
-				// needed for Windows namespaces
-				'com1.suffix', ['.htaccess'], ['com1'], true
-			],
-			'forbidden name - basename is checked also with multiple extensions' => [
-				// needed for Windows namespaces
-				'com1.tar.gz', ['.htaccess'], ['com1'], true
+				'COM1', ['.htaccess', 'com1'], true,
 			],
 		];
 	}

diff --git a/tests/lib/TestCase.php b/tests/lib/TestCase.php
@@ -508,9 +508,9 @@ protected function getGroupAnnotations(): array {
 	}
 
 	protected function IsDatabaseAccessAllowed() {
-		// on travis-ci.org we allow database access in any case - otherwise
+		// on CI we allow database access in any case - otherwise
 		// this will break all apps right away
-		if (getenv('TRAVIS') == true) {
+		if (getenv('CI') == true) {
 			return true;
 		}
 		$annotations = $this->getGroupAnnotations();