From 66a6b442b5543a571ba405d328651677774b699a Mon Sep 17 00:00:00 2001
From: Joas Schilling <coding@schilljs.com>
Date: Fri, 13 Sep 2024 09:16:35 +0200
Subject: [PATCH] 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 <coding@schilljs.com>
---
 lib/private/AllConfig.php   |  6 +++---
 tests/lib/AllConfigTest.php | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/lib/private/AllConfig.php b/lib/private/AllConfig.php
index 58eee772fbf91..d48de298fad5e 100644
--- 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
index c703c47c94eb4..48df3416df196 100644
--- 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');