fix(config): Throw PreconditionException always when it didn't match

Previously even when the precondition did not match, the call "passed" when the after value was the expected one. This however can lead to race conditions, duplicate code excutions and other things. Signed-off-by: Joas Schilling <[email protected]>
nextcloud · Sep 13, 2024 · 66a6b44 · 66a6b44
1 parent 91533fb
commit 66a6b44
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 3 deletions.
diff --git a/lib/private/AllConfig.php b/lib/private/AllConfig.php
@@ -242,10 +242,10 @@ public function setUserValue($userId, $appName, $key, $value, $preCondition = nu
 		$prevValue = $this->getUserValue($userId, $appName, $key, null);
 
 		if ($prevValue !== null) {
-			if ($prevValue === (string)$value) {
-				return;
-			} elseif ($preCondition !== null && $prevValue !== (string)$preCondition) {
+			if ($preCondition !== null && $prevValue !== (string)$preCondition) {
 				throw new PreConditionNotMetException();
+			} elseif ($prevValue === (string)$value) {
+				return;
 			} else {
 				$qb = $this->connection->getQueryBuilder();
 				$qb->update('preferences')

diff --git a/tests/lib/AllConfigTest.php b/tests/lib/AllConfigTest.php
@@ -182,6 +182,42 @@ public function testSetUserValueWithPreConditionFailure() {
 		$config->deleteUserValue('userPreCond1', 'appPreCond', 'keyPreCond');
 	}
 
+	public function testSetUserValueWithPreConditionFailureWhenResultStillMatches(): void {
+		$this->expectException(\OCP\PreConditionNotMetException::class);
+
+		$config = $this->getConfig();
+
+		$selectAllSQL = 'SELECT `userid`, `appid`, `configkey`, `configvalue` FROM `*PREFIX*preferences` WHERE `userid` = ?';
+
+		$config->setUserValue('userPreCond1', 'appPreCond', 'keyPreCond', 'valuePreCond');
+
+		$result = $this->connection->executeQuery($selectAllSQL, ['userPreCond1'])->fetchAll();
+
+		$this->assertCount(1, $result);
+		$this->assertEquals([
+			'userid' => 'userPreCond1',
+			'appid' => 'appPreCond',
+			'configkey' => 'keyPreCond',
+			'configvalue' => 'valuePreCond'
+		], $result[0]);
+
+		// test if the method throws with invalid precondition when the value is the same
+		$config->setUserValue('userPreCond1', 'appPreCond', 'keyPreCond', 'valuePreCond', 'valuePreCond3');
+
+		$result = $this->connection->executeQuery($selectAllSQL, ['userPreCond1'])->fetchAll();
+
+		$this->assertCount(1, $result);
+		$this->assertEquals([
+			'userid' => 'userPreCond1',
+			'appid' => 'appPreCond',
+			'configkey' => 'keyPreCond',
+			'configvalue' => 'valuePreCond'
+		], $result[0]);
+
+		// cleanup
+		$config->deleteUserValue('userPreCond1', 'appPreCond', 'keyPreCond');
+	}
+
 	public function testSetUserValueUnchanged() {
 		// TODO - FIXME until the dependency injection is handled properly (in AllConfig)
 		$this->markTestSkipped('Skipped because this is just testable if database connection can be injected');