From 4967f7bf48bf2b6a0e89557fb7483a9af8062444 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 18 Jun 2026 16:35:51 +0200 Subject: [PATCH 1/7] fix: Use a CappedMemoryCache for UserConfig to avoid memory exhaustion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When moving user preferences from AllConfig to UserConfig, the CappedMemoryCache became an array, which means memory can be filled with user preferences if you loop on all users and check a preference on them. Signed-off-by: Côme Chilliet --- lib/private/Config/UserConfig.php | 63 ++++++++--------------------- tests/lib/Config/UserConfigTest.php | 21 +++++----- 2 files changed, 26 insertions(+), 58 deletions(-) diff --git a/lib/private/Config/UserConfig.php b/lib/private/Config/UserConfig.php index a1623c1b34f6a..aac1fda629d65 100644 --- a/lib/private/Config/UserConfig.php +++ b/lib/private/Config/UserConfig.php @@ -12,6 +12,7 @@ use InvalidArgumentException; use JsonException; use OC\AppFramework\Bootstrap\Coordinator; +use OCP\Cache\CappedMemoryCache; use OCP\Config\Exceptions\IncorrectTypeException; use OCP\Config\Exceptions\TypeConflictException; use OCP\Config\Exceptions\UnknownKeyException; @@ -56,16 +57,12 @@ class UserConfig implements IUserConfig { private const ENCRYPTION_PREFIX = '$UserConfigEncryption$'; private const ENCRYPTION_PREFIX_LENGTH = 22; // strlen(self::ENCRYPTION_PREFIX) - /** @var array>> [ass'user_id' => ['app_id' => ['key' => 'value']]] */ - private array $fastCache = []; // cache for normal config keys - /** @var array>> ['user_id' => ['app_id' => ['key' => 'value']]] */ - private array $lazyCache = []; // cache for lazy config keys + /** @var CappedMemoryCache>> [ass'user_id' => ['app_id' => ['key' => 'value']]] cache for normal config keys */ + private CappedMemoryCache $fastCache; + /** @var CappedMemoryCache>> ['user_id' => ['app_id' => ['key' => 'value']]] cache for lazy config keys */ + private CappedMemoryCache $lazyCache; /** @var array>>> ['user_id' => ['app_id' => ['key' => ['type' => ValueType, 'flags' => bitflag]]]] */ private array $valueDetails = []; // type for all config values - /** @var array ['user_id' => bool] */ - private array $fastLoaded = []; - /** @var array ['user_id' => bool] */ - private array $lazyLoaded = []; /** @var array, aliases: array, strictness: Strictness}> ['app_id' => ['strictness' => ConfigLexiconStrictness, 'entries' => ['config_key' => ConfigLexiconEntry[]]] */ private array $configLexiconDetails = []; private bool $ignoreLexiconAliases = false; @@ -90,6 +87,8 @@ public function __construct( protected ICrypto $crypto, protected IEventDispatcher $dispatcher, ) { + $this->fastCache = new CappedMemoryCache(); + $this->lazyCache = new CappedMemoryCache(); } /** @@ -1725,8 +1724,9 @@ public function deleteAllUserConfig(string $userId): void { #[\Override] public function clearCache(string $userId, bool $reload = false): void { $this->assertParams($userId, allowEmptyApp: true); - $this->lazyLoaded[$userId] = $this->fastLoaded[$userId] = false; - $this->lazyCache[$userId] = $this->fastCache[$userId] = $this->valueDetails[$userId] = []; + unset($this->lazyCache[$userId]); + unset($this->fastCache[$userId]); + $this->valueDetails[$userId] = []; if (!$reload) { return; @@ -1742,8 +1742,9 @@ public function clearCache(string $userId, bool $reload = false): void { */ #[\Override] public function clearCacheAll(): void { - $this->lazyLoaded = $this->fastLoaded = []; - $this->lazyCache = $this->fastCache = $this->valueDetails = $this->configLexiconDetails = []; + $this->lazyCache = new CappedMemoryCache(); + $this->fastCache = new CappedMemoryCache(); + $this->valueDetails = $this->configLexiconDetails = []; } /** @@ -1756,10 +1757,8 @@ public function clearCacheAll(): void { */ public function statusCache(): array { return [ - 'fastLoaded' => $this->fastLoaded, - 'fastCache' => $this->fastCache, - 'lazyLoaded' => $this->lazyLoaded, - 'lazyCache' => $this->lazyCache, + 'fastCache' => $this->fastCache->getData(), + 'lazyCache' => $this->lazyCache->getData(), 'valueDetails' => $this->valueDetails, ]; } @@ -1866,7 +1865,6 @@ private function loadConfig(string $userId, ?bool $lazy = false): void { $this->valueDetails[$userId][$row['appid']][$row['configkey']] = ['type' => ValueType::from((int)($row['type'] ?? 0)), 'flags' => (int)($row['flags'] ?? 0)]; } $result->closeCursor(); - $this->setAsLoaded($userId, $lazy); } /** @@ -1882,37 +1880,10 @@ private function loadConfig(string $userId, ?bool $lazy = false): void { */ private function isLoaded(string $userId, ?bool $lazy): bool { if ($lazy === null) { - return ($this->lazyLoaded[$userId] ?? false) && ($this->fastLoaded[$userId] ?? false); + return isset($this->lazyCache[$userId]) && isset($this->fastCache[$userId]); } - return $lazy ? $this->lazyLoaded[$userId] ?? false : $this->fastLoaded[$userId] ?? false; - } - - /** - * if $lazy is: - * - false: set fast config as loaded - * - true : set lazy config as loaded - * - null : set both config as loaded - * - * @param string $userId - * @param bool $lazy - */ - private function setAsLoaded(string $userId, ?bool $lazy): void { - if ($lazy === null) { - $this->fastLoaded[$userId] = $this->lazyLoaded[$userId] = true; - return; - } - - // We also create empty entry to keep both fastLoaded/lazyLoaded synced - if ($lazy) { - $this->lazyLoaded[$userId] = true; - $this->fastLoaded[$userId] = $this->fastLoaded[$userId] ?? false; - $this->fastCache[$userId] = $this->fastCache[$userId] ?? []; - } else { - $this->fastLoaded[$userId] = true; - $this->lazyLoaded[$userId] = $this->lazyLoaded[$userId] ?? false; - $this->lazyCache[$userId] = $this->lazyCache[$userId] ?? []; - } + return $lazy ? isset($this->lazyCache[$userId]) : isset($this->fastCache[$userId]); } /** diff --git a/tests/lib/Config/UserConfigTest.php b/tests/lib/Config/UserConfigTest.php index 5ac90a02a3d5f..c913545ba588a 100644 --- a/tests/lib/Config/UserConfigTest.php +++ b/tests/lib/Config/UserConfigTest.php @@ -308,8 +308,6 @@ private function generateUserConfig(array $preLoading = []): IUserConfig { // confirm cache status $status = $userConfig->statusCache(); - $this->assertSame([], $status['fastLoaded'], $msg); - $this->assertSame([], $status['lazyLoaded'], $msg); $this->assertSame([], $status['fastCache'], $msg); $this->assertSame([], $status['lazyCache'], $msg); foreach ($preLoading as $preLoadUser) { @@ -318,12 +316,12 @@ private function generateUserConfig(array $preLoading = []): IUserConfig { // confirm cache status $status = $userConfig->statusCache(); - $this->assertSame(true, $status['fastLoaded'][$preLoadUser], $msg); - $this->assertSame(false, $status['lazyLoaded'][$preLoadUser], $msg); + $this->assertTrue(isset($status['fastCache'][$preLoadUser]), $msg); + $this->assertFalse(isset($status['lazyCache'][$preLoadUser]), $msg); $apps = array_values(array_diff(array_keys($this->basePreferences[$preLoadUser]), ['only-lazy'])); $this->assertEqualsCanonicalizing($apps, array_keys($status['fastCache'][$preLoadUser]), $msg); - $this->assertSame([], array_keys($status['lazyCache'][$preLoadUser]), $msg); + $this->assertSame([], $status['lazyCache'][$preLoadUser] ?? [], $msg); } return $userConfig; @@ -1669,12 +1667,13 @@ public function testDeleteAllPreferences(): void { public function testClearCache(): void { $userConfig = $this->generateUserConfig(['user1', 'user2']); $userConfig->clearCache('user1'); - - $this->assertEquals(true, $userConfig->statusCache()['fastLoaded']['user2']); - $this->assertEquals(false, $userConfig->statusCache()['fastLoaded']['user1']); + $status = $userConfig->statusCache(); + $this->assertTrue(isset($status['fastCache']['user2'])); + $this->assertFalse(isset($status['fastCache']['user1'])); $this->assertEquals('value2a', $userConfig->getValueString('user1', 'app2', 'key2')); - $this->assertEquals(false, $userConfig->statusCache()['lazyLoaded']['user1']); - $this->assertEquals(true, $userConfig->statusCache()['fastLoaded']['user1']); + $status = $userConfig->statusCache(); + $this->assertFalse(isset($status['lazyCache']['user1'])); + $this->assertTrue(isset($status['fastCache']['user1'])); } public function testClearCacheAll(): void { @@ -1682,9 +1681,7 @@ public function testClearCacheAll(): void { $userConfig->clearCacheAll(); $this->assertEqualsCanonicalizing( [ - 'fastLoaded' => [], 'fastCache' => [], - 'lazyLoaded' => [], 'lazyCache' => [], 'valueDetails' => [], ], From 4e342619679fdf4d4ea605c1cb3d3e47dde25ab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 18 Jun 2026 17:28:20 +0200 Subject: [PATCH 2/7] chore: merge valueDetails into fastCache and lazyCache to avoid out-of-sync issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It feels like a UserConfigEntry class would make sense instead of the currently used array to represent an entry. Signed-off-by: Côme Chilliet --- lib/private/Config/UserConfig.php | 171 +++++++++++++++------------- tests/lib/Config/UserConfigTest.php | 13 +-- 2 files changed, 99 insertions(+), 85 deletions(-) diff --git a/lib/private/Config/UserConfig.php b/lib/private/Config/UserConfig.php index aac1fda629d65..f9d5e59628d2f 100644 --- a/lib/private/Config/UserConfig.php +++ b/lib/private/Config/UserConfig.php @@ -57,12 +57,10 @@ class UserConfig implements IUserConfig { private const ENCRYPTION_PREFIX = '$UserConfigEncryption$'; private const ENCRYPTION_PREFIX_LENGTH = 22; // strlen(self::ENCRYPTION_PREFIX) - /** @var CappedMemoryCache>> [ass'user_id' => ['app_id' => ['key' => 'value']]] cache for normal config keys */ + /** @var CappedMemoryCache>> cache for normal config keys */ private CappedMemoryCache $fastCache; - /** @var CappedMemoryCache>> ['user_id' => ['app_id' => ['key' => 'value']]] cache for lazy config keys */ + /** @var CappedMemoryCache>> cache for lazy config keys */ private CappedMemoryCache $lazyCache; - /** @var array>>> ['user_id' => ['app_id' => ['key' => ['type' => ValueType, 'flags' => bitflag]]]] */ - private array $valueDetails = []; // type for all config values /** @var array, aliases: array, strictness: Strictness}> ['app_id' => ['strictness' => ConfigLexiconStrictness, 'entries' => ['config_key' => ConfigLexiconEntry[]]] */ private array $configLexiconDetails = []; private bool $ignoreLexiconAliases = false; @@ -204,11 +202,13 @@ public function isSensitive(string $userId, string $app, string $key, ?bool $laz $this->loadConfig($userId, $lazy); $this->matchAndApplyLexiconDefinition($userId, $app, $key); - if (!isset($this->valueDetails[$userId][$app][$key])) { - throw new UnknownKeyException('unknown config key'); + if (isset($this->fastCache[$userId][$app][$key])) { + return $this->isFlagged(self::FLAG_SENSITIVE, $this->fastCache[$userId][$app][$key]['flags']); + } elseif (isset($this->lazyCache[$userId][$app][$key])) { + return $this->isFlagged(self::FLAG_SENSITIVE, $this->lazyCache[$userId][$app][$key]['flags']); + } else { + throw new UnknownKeyException('Unknown config key ' . $app . '/' . $key); } - - return $this->isFlagged(self::FLAG_SENSITIVE, $this->valueDetails[$userId][$app][$key]['flags']); } /** @@ -229,11 +229,13 @@ public function isIndexed(string $userId, string $app, string $key, ?bool $lazy $this->loadConfig($userId, $lazy); $this->matchAndApplyLexiconDefinition($userId, $app, $key); - if (!isset($this->valueDetails[$userId][$app][$key])) { - throw new UnknownKeyException('unknown config key'); + if (isset($this->fastCache[$userId][$app][$key])) { + return $this->isFlagged(self::FLAG_INDEXED, $this->fastCache[$userId][$app][$key]['flags']); + } elseif (isset($this->lazyCache[$userId][$app][$key])) { + return $this->isFlagged(self::FLAG_INDEXED, $this->lazyCache[$userId][$app][$key]['flags']); + } else { + throw new UnknownKeyException('Unknown config key ' . $app . '/' . $key); } - - return $this->isFlagged(self::FLAG_INDEXED, $this->valueDetails[$userId][$app][$key]['flags']); } /** @@ -350,11 +352,12 @@ public function getValuesByApps(string $userId, string $key, bool $lazy = false, $values = []; foreach (array_keys($cache) as $app) { if (isset($cache[$app][$key])) { - $value = $cache[$app][$key]; + $valueDetail = $cache[$app][$key]; try { - $this->decryptSensitiveValue($userId, $app, $key, $value); - $value = $this->convertTypedValue($value, $typedAs ?? $this->getValueType($userId, $app, $key, $lazy)); + $this->decryptSensitiveValue($userId, $app, $key, $valueDetail); + $value = $this->convertTypedValue($valueDetail['value'], $typedAs ?? $this->getValueType($userId, $app, $key, $lazy)); } catch (IncorrectTypeException|UnknownKeyException) { + $value = $valueDetail['value']; } $values[$app] = $value; } @@ -792,12 +795,27 @@ private function getTypedValue( $this->loadConfig($userId, $lazy); + /** + * - the pair $app/$key cannot exist in both array, + * - we should still return an existing non-lazy value even if current method + * is called with $lazy is true + * + * This way, lazyCache will be empty until the load for lazy config value is requested. + */ + if (isset($this->lazyCache[$userId][$app][$key])) { + $valueDetail = $this->lazyCache[$userId][$app][$key]; + } elseif (isset($this->fastCache[$userId][$app][$key])) { + $valueDetail = $this->fastCache[$userId][$app][$key]; + } else { + return $default; + } + /** * We ignore check if mixed type is requested. * If type of stored value is set as mixed, we don't filter. * If type of stored value is defined, we compare with the one requested. */ - $knownType = $this->valueDetails[$userId][$app][$key]['type'] ?? null; + $knownType = $valueDetail['type'] ?? null; if ($type !== ValueType::MIXED && $knownType !== null && $knownType !== ValueType::MIXED @@ -806,22 +824,9 @@ private function getTypedValue( throw new TypeConflictException('conflict with value type from database'); } - /** - * - the pair $app/$key cannot exist in both array, - * - we should still return an existing non-lazy value even if current method - * is called with $lazy is true - * - * This way, lazyCache will be empty until the load for lazy config value is requested. - */ - if (isset($this->lazyCache[$userId][$app][$key])) { - $value = $this->lazyCache[$userId][$app][$key]; - } elseif (isset($this->fastCache[$userId][$app][$key])) { - $value = $this->fastCache[$userId][$app][$key]; - } else { - return $default; - } + $this->decryptSensitiveValue($userId, $app, $key, $valueDetail); - $this->decryptSensitiveValue($userId, $app, $key, $value); + $value = $valueDetail['value']; // in case the key was modified while running matchAndApplyLexiconDefinition() we are // interested to check options in case a modification of the value is needed @@ -851,11 +856,13 @@ public function getValueType(string $userId, string $app, string $key, ?bool $la $this->loadConfig($userId, $lazy); $this->matchAndApplyLexiconDefinition($userId, $app, $key); - if (!isset($this->valueDetails[$userId][$app][$key]['type'])) { - throw new UnknownKeyException('unknown config key'); + if (isset($this->fastCache[$userId][$app][$key])) { + return $this->fastCache[$userId][$app][$key]['type']; + } elseif (isset($this->lazyCache[$userId][$app][$key])) { + return $this->lazyCache[$userId][$app][$key]['type']; + } else { + throw new UnknownKeyException('Unknown config key ' . $app . '/' . $key); } - - return $this->valueDetails[$userId][$app][$key]['type']; } /** @@ -877,11 +884,13 @@ public function getValueFlags(string $userId, string $app, string $key, bool $la $this->loadConfig($userId, $lazy); $this->matchAndApplyLexiconDefinition($userId, $app, $key); - if (!isset($this->valueDetails[$userId][$app][$key])) { - throw new UnknownKeyException('unknown config key'); + if (isset($this->fastCache[$userId][$app][$key])) { + return $this->fastCache[$userId][$app][$key]['flags']; + } elseif (isset($this->lazyCache[$userId][$app][$key])) { + return $this->lazyCache[$userId][$app][$key]['flags']; + } else { + throw new UnknownKeyException('Unknown config key ' . $app . '/' . $key); } - - return $this->valueDetails[$userId][$app][$key]['flags']; } /** @@ -1216,14 +1225,14 @@ private function setTypedValue( * We cannot insert a new row, meaning we need to update an already existing one */ if (!$inserted) { - $currType = $this->valueDetails[$userId][$app][$key]['type'] ?? null; + $currType = $this->fastCache[$userId][$app][$key]['type'] ?? $this->lazyCache[$userId][$app][$key]['type'] ?? null; if ($currType === null) { // this might happen when switching lazy loading status $this->loadConfigAll($userId); - if (!isset($this->valueDetails[$userId][$app][$key])) { + if (!isset($this->fastCache[$userId][$app][$key]) && !isset($this->lazyCache[$userId][$app][$key])) { throw new UnknownKeyException("unknown key $app $key for $userId even though $updateReason"); } - $currType = $this->valueDetails[$userId][$app][$key]['type'] ?? null; + $currType = $this->fastCache[$userId][$app][$key]['type'] ?? $this->lazyCache[$userId][$app][$key]['type'] ?? null; } /** @@ -1278,15 +1287,16 @@ private function setTypedValue( } // update local cache - if ($lazy) { - $this->lazyCache[$userId][$app][$key] = $value; - } else { - $this->fastCache[$userId][$app][$key] = $value; - } - $this->valueDetails[$userId][$app][$key] = [ + $valueDetail = [ + 'value' => $value, 'type' => $type, 'flags' => $flags ]; + if ($lazy) { + $this->lazyCache[$userId][$app][$key] = $valueDetail; + } else { + $this->fastCache[$userId][$app][$key] = $valueDetail; + } return true; } @@ -1311,7 +1321,7 @@ public function updateType(string $userId, string $app, string $key, ValueType $ $this->assertParams($userId, $app, $key); $this->loadConfigAll($userId); $this->matchAndApplyLexiconDefinition($userId, $app, $key); - $this->isLazy($userId, $app, $key); // confirm key exists + $lazy = $this->isLazy($userId, $app, $key); // confirm key exists $update = $this->connection->getQueryBuilder(); $update->update('preferences') @@ -1321,7 +1331,11 @@ public function updateType(string $userId, string $app, string $key, ValueType $ ->andWhere($update->expr()->eq('configkey', $update->createNamedParameter($key))); $update->executeStatement(); - $this->valueDetails[$userId][$app][$key]['type'] = $type; + if ($lazy) { + $this->lazyCache[$userId][$app][$key]['type'] = $type; + } else { + $this->fastCache[$userId][$app][$key]['type'] = $type; + } return true; } @@ -1362,14 +1376,15 @@ public function updateSensitive(string $userId, string $app, string $key, bool $ throw new UnknownKeyException('unknown config key'); } - $value = $cache[$userId][$app][$key]; + $valueDetail = $cache[$userId][$app][$key]; $flags = $this->getValueFlags($userId, $app, $key); if ($sensitive) { $flags |= self::FLAG_SENSITIVE; - $value = self::ENCRYPTION_PREFIX . $this->crypto->encrypt($value); + $value = self::ENCRYPTION_PREFIX . $this->crypto->encrypt($valueDetail['value']); } else { $flags &= ~self::FLAG_SENSITIVE; - $this->decryptSensitiveValue($userId, $app, $key, $value); + $this->decryptSensitiveValue($userId, $app, $key, $valueDetail); + $value = $valueDetail['value']; } $update = $this->connection->getQueryBuilder(); @@ -1381,7 +1396,7 @@ public function updateSensitive(string $userId, string $app, string $key, bool $ ->andWhere($update->expr()->eq('configkey', $update->createNamedParameter($key))); $update->executeStatement(); - $this->valueDetails[$userId][$app][$key]['flags'] = $flags; + $cache[$userId][$app][$key]['flags'] = $flags; return true; } @@ -1451,7 +1466,7 @@ public function updateIndexed(string $userId, string $app, string $key, bool $in throw new UnknownKeyException('unknown config key'); } - $value = $cache[$userId][$app][$key]; + $value = $cache[$userId][$app][$key]['value']; $flags = $this->getValueFlags($userId, $app, $key); if ($indexed) { $indexed = $value; @@ -1469,7 +1484,7 @@ public function updateIndexed(string $userId, string $app, string $key, bool $in ->andWhere($update->expr()->eq('configkey', $update->createNamedParameter($key))); $update->executeStatement(); - $this->valueDetails[$userId][$app][$key]['flags'] = $flags; + $cache[$userId][$app][$key]['flags'] = $flags; return true; } @@ -1619,15 +1634,15 @@ public function getDetails(string $userId, string $app, string $key): array { throw new UnknownKeyException('unknown config key'); } - $value = $cache[$app][$key]; + $valueDetail = $cache[$app][$key]; $sensitive = $this->isSensitive($userId, $app, $key, null); - $this->decryptSensitiveValue($userId, $app, $key, $value); + $this->decryptSensitiveValue($userId, $app, $key, $valueDetail); return [ 'userId' => $userId, 'app' => $app, 'key' => $key, - 'value' => $value, + 'value' => $valueDetail['value'], 'type' => $type->value, 'lazy' => $lazy, 'typeString' => $typeString, @@ -1658,7 +1673,6 @@ public function deleteUserConfig(string $userId, string $app, string $key): void unset($this->lazyCache[$userId][$app][$key]); unset($this->fastCache[$userId][$app][$key]); - unset($this->valueDetails[$userId][$app][$key]); } /** @@ -1726,7 +1740,6 @@ public function clearCache(string $userId, bool $reload = false): void { $this->assertParams($userId, allowEmptyApp: true); unset($this->lazyCache[$userId]); unset($this->fastCache[$userId]); - $this->valueDetails[$userId] = []; if (!$reload) { return; @@ -1744,7 +1757,7 @@ public function clearCache(string $userId, bool $reload = false): void { public function clearCacheAll(): void { $this->lazyCache = new CappedMemoryCache(); $this->fastCache = new CappedMemoryCache(); - $this->valueDetails = $this->configLexiconDetails = []; + $this->configLexiconDetails = []; } /** @@ -1759,7 +1772,6 @@ public function statusCache(): array { return [ 'fastCache' => $this->fastCache->getData(), 'lazyCache' => $this->lazyCache->getData(), - 'valueDetails' => $this->valueDetails, ]; } @@ -1857,12 +1869,16 @@ private function loadConfig(string $userId, ?bool $lazy = false): void { $rows = $result->fetchAllAssociative(); foreach ($rows as $row) { + $valueDetail = [ + 'value' => $row['configvalue'] ?? '', + 'type' => ValueType::from((int)($row['type'] ?? 0)), + 'flags' => (int)($row['flags'] ?? 0), + ]; if ($this->migrationCompleted && (($row['lazy'] ?? ($lazy ?? 0) ? 1 : 0) === 1)) { - $this->lazyCache[$userId][$row['appid']][$row['configkey']] = $row['configvalue'] ?? ''; + $this->lazyCache[$userId][$row['appid']][$row['configkey']] = $valueDetail; } else { - $this->fastCache[$userId][$row['appid']][$row['configkey']] = $row['configvalue'] ?? ''; + $this->fastCache[$userId][$row['appid']][$row['configkey']] = $valueDetail; } - $this->valueDetails[$userId][$row['appid']][$row['configkey']] = ['type' => ValueType::from((int)($row['type'] ?? 0)), 'flags' => (int)($row['flags'] ?? 0)]; } $result->closeCursor(); } @@ -1896,7 +1912,8 @@ private function isLoaded(string $userId, ?bool $lazy): bool { * @return array */ private function formatAppValues(string $userId, string $app, array $values, bool $filtered = false): array { - foreach ($values as $key => $value) { + foreach ($values as $key => $valueDetail) { + $value = $valueDetail['value']; //$key = (string)$key; try { $type = $this->getValueType($userId, $app, (string)$key); @@ -1904,12 +1921,13 @@ private function formatAppValues(string $userId, string $app, array $values, boo continue; } - if ($this->isFlagged(self::FLAG_SENSITIVE, $this->valueDetails[$userId][$app][$key]['flags'] ?? 0)) { + if ($this->isFlagged(self::FLAG_SENSITIVE, $valueDetail['flags'] ?? 0)) { if ($filtered) { $value = IConfig::SENSITIVE_VALUE; $type = ValueType::STRING; } else { - $this->decryptSensitiveValue($userId, $app, (string)$key, $value); + $this->decryptSensitiveValue($userId, $app, (string)$key, $valueDetail); + $value = $valueDetail['value']; } } @@ -1949,28 +1967,25 @@ private function convertTypedValue(string $value, ValueType $type): string|int|f /** * will change referenced $value with the decrypted value in case of encrypted (sensitive value) * - * @param string $userId - * @param string $app - * @param string $key - * @param string $value + * @param array{type: ValueType, flags: int, value: string} $valueDetail */ - private function decryptSensitiveValue(string $userId, string $app, string $key, string &$value): void { - if (!$this->isFlagged(self::FLAG_SENSITIVE, $this->valueDetails[$userId][$app][$key]['flags'] ?? 0)) { + private function decryptSensitiveValue(string $userId, string $app, string $key, array &$valueDetail): void { + if (!$this->isFlagged(self::FLAG_SENSITIVE, $valueDetail['flags'] ?? 0)) { return; } - if (!str_starts_with($value, self::ENCRYPTION_PREFIX)) { + if (!str_starts_with($valueDetail['value'], self::ENCRYPTION_PREFIX)) { return; } try { - $value = $this->crypto->decrypt(substr($value, self::ENCRYPTION_PREFIX_LENGTH)); + $valueDetail['value'] = $this->crypto->decrypt(substr($valueDetail['value'], self::ENCRYPTION_PREFIX_LENGTH)); } catch (\Exception $e) { $this->logger->warning('could not decrypt sensitive value', [ 'userId' => $userId, 'app' => $app, 'key' => $key, - 'value' => $value, + 'value' => $valueDetail['value'], 'exception' => $e ]); } diff --git a/tests/lib/Config/UserConfigTest.php b/tests/lib/Config/UserConfigTest.php index c913545ba588a..a5affdec85823 100644 --- a/tests/lib/Config/UserConfigTest.php +++ b/tests/lib/Config/UserConfigTest.php @@ -1423,8 +1423,8 @@ public function testUpdateSensitive( $this->assertEquals($sensitive, $userConfig->isSensitive($userId, $app, $key)); if ($sensitive) { $this->assertEquals(true, str_starts_with( - $userConfig->statusCache()['fastCache'][$userId][$app][$key] - ?? $userConfig->statusCache()['lazyCache'][$userId][$app][$key], + $userConfig->statusCache()['fastCache'][$userId][$app][$key]['value'] + ?? $userConfig->statusCache()['lazyCache'][$userId][$app][$key]['value'], '$UserConfigEncryption$') ); } @@ -1452,8 +1452,8 @@ public function testUpdateGlobalSensitive(bool $sensitive): void { $userConfig->getValueString($userId, $app, $key); // cache loading for userId $this->assertEquals( !$sensitive, str_starts_with( - $userConfig->statusCache()['fastCache'][$userId][$app][$key] - ?? $userConfig->statusCache()['lazyCache'][$userId][$app][$key], + $userConfig->statusCache()['fastCache'][$userId][$app][$key]['value'] + ?? $userConfig->statusCache()['lazyCache'][$userId][$app][$key]['value'], '$UserConfigEncryption$' ) ); @@ -1466,8 +1466,8 @@ public function testUpdateGlobalSensitive(bool $sensitive): void { $this->assertEquals($sensitive, $userConfig->isSensitive($userId, $app, $key)); // should only work if updateGlobalSensitive drop cache $this->assertEquals($sensitive, str_starts_with( - $userConfig->statusCache()['fastCache'][$userId][$app][$key] - ?? $userConfig->statusCache()['lazyCache'][$userId][$app][$key], + $userConfig->statusCache()['fastCache'][$userId][$app][$key]['value'] + ?? $userConfig->statusCache()['lazyCache'][$userId][$app][$key]['value'], '$UserConfigEncryption$') ); } @@ -1683,7 +1683,6 @@ public function testClearCacheAll(): void { [ 'fastCache' => [], 'lazyCache' => [], - 'valueDetails' => [], ], $userConfig->statusCache() ); From 2f94bc4d9da669a0a158e5a1026fff9c2f00acc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 18 Jun 2026 17:46:01 +0200 Subject: [PATCH 3/7] chore: Use IUserConfig in User class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/User/User.php | 64 +++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/lib/private/User/User.php b/lib/private/User/User.php index 9b1361d1b569d..d99e35b4504ea 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -14,6 +14,7 @@ use OC\Hooks\Emitter; use OCP\Accounts\IAccountManager; use OCP\Comments\ICommentsManager; +use OCP\Config\IUserConfig; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\FileInfo; use OCP\Group\Events\BeforeUserRemovedEvent; @@ -45,13 +46,12 @@ use OCP\UserInterface; use OCP\Util; use Psr\Log\LoggerInterface; -use function json_decode; -use function json_encode; class User implements IUser { private const CONFIG_KEY_MANAGERS = 'manager'; private IConfig $config; + private IUserConfig $userConfig; private IURLGenerator $urlGenerator; private IAssertion $assertion; protected ?IAccountManager $accountManager = null; @@ -72,10 +72,12 @@ public function __construct( private IEventDispatcher $dispatcher, private Emitter|Manager|null $emitter = null, ?IConfig $config = null, + ?IUserConfig $userConfig = null, ?IURLGenerator $urlGenerator = null, ?IAssertion $assertion = null, ) { $this->config = $config ?? Server::get(IConfig::class); + $this->userConfig = $userConfig ?? Server::get(IUserConfig::class); $this->urlGenerator = $urlGenerator ?? Server::get(IURLGenerator::class); $this->assertion = $assertion ?? Server::get(IAssertion::class); } @@ -151,9 +153,9 @@ public function setSystemEMailAddress(string $mailAddress): void { $mailAddress = mb_strtolower(trim($mailAddress)); if ($mailAddress === '') { - $this->config->deleteUserValue($this->uid, 'settings', 'email'); + $this->userConfig->deleteUserConfig($this->uid, 'settings', 'email'); } else { - $this->config->setUserValue($this->uid, 'settings', 'email', $mailAddress); + $this->userConfig->setValueString($this->uid, 'settings', 'email', $mailAddress); } $primaryAddress = $this->getPrimaryEMailAddress(); @@ -174,7 +176,7 @@ public function setSystemEMailAddress(string $mailAddress): void { public function setPrimaryEMailAddress(string $mailAddress): void { $mailAddress = mb_strtolower(trim($mailAddress)); if ($mailAddress === '') { - $this->config->deleteUserValue($this->uid, 'settings', 'primary_email'); + $this->userConfig->deleteUserConfig($this->uid, 'settings', 'primary_email'); return; } @@ -186,7 +188,7 @@ public function setPrimaryEMailAddress(string $mailAddress): void { if ($property === null || $property->getLocallyVerified() !== IAccountManager::VERIFIED) { throw new InvalidArgumentException('Only verified emails can be set as primary'); } - $this->config->setUserValue($this->uid, 'settings', 'primary_email', $mailAddress); + $this->userConfig->setValueString($this->uid, 'settings', 'primary_email', $mailAddress); } private function ensureAccountManager() { @@ -202,7 +204,7 @@ private function ensureAccountManager() { #[\Override] public function getLastLogin(): int { if ($this->lastLogin === null) { - $this->lastLogin = (int)$this->config->getUserValue($this->uid, 'login', 'lastLogin', 0); + $this->lastLogin = $this->userConfig->getValueInt($this->uid, 'login', 'lastLogin'); } return $this->lastLogin; } @@ -214,7 +216,7 @@ public function getLastLogin(): int { #[\Override] public function getFirstLogin(): int { if ($this->firstLogin === null) { - $this->firstLogin = (int)$this->config->getUserValue($this->uid, 'login', 'firstLogin', 0); + $this->firstLogin = $this->userConfig->getValueInt($this->uid, 'login', 'firstLogin'); } return $this->firstLogin; } @@ -231,7 +233,7 @@ public function updateLastLoginTimestamp(): bool { if ($now - $previousLogin > 60) { $this->lastLogin = $now; - $this->config->setUserValue($this->uid, 'login', 'lastLogin', (string)$this->lastLogin); + $this->userConfig->setValueInt($this->uid, 'login', 'lastLogin', $this->lastLogin); } if ($firstLogin === 0) { @@ -241,7 +243,7 @@ public function updateLastLoginTimestamp(): bool { /* Unknown first login, most likely was before upgrade to Nextcloud 31 */ $this->firstLogin = -1; } - $this->config->setUserValue($this->uid, 'login', 'firstLogin', (string)$this->firstLogin); + $this->userConfig->setValueInt($this->uid, 'login', 'firstLogin', $this->firstLogin); } return $firstTimeLogin; @@ -265,15 +267,15 @@ public function delete(): bool { // Set delete flag on the user - this is needed to ensure that the user data is removed if there happen any exception in the backend // because we can not restore the user meaning we could not rollback to any stable state otherwise. - $this->config->setUserValue($this->uid, 'core', 'deleted', 'true'); + $this->userConfig->setValueBool($this->uid, 'core', 'deleted', true); // We also need to backup the home path as this can not be reconstructed later if the original backend uses custom home paths - $this->config->setUserValue($this->uid, 'core', 'deleted.home-path', $this->getHome()); + $this->userConfig->setValueString($this->uid, 'core', 'deleted.home-path', $this->getHome()); // Try to delete the user on the backend $result = $this->backend->deleteUser($this->uid); if ($result === false) { // The deletion was aborted or something else happened, we are in a defined state, so remove the delete flag - $this->config->deleteUserValue($this->uid, 'core', 'deleted'); + $this->userConfig->deleteUserConfig($this->uid, 'core', 'deleted'); return false; } @@ -310,10 +312,10 @@ public function delete(): bool { // exactly here we are in an undefined state as the data is still present but the user does not exist on the system anymore. $database->beginTransaction(); // Remove all user settings - $this->config->deleteAllUserValues($this->uid); + $this->userConfig->deleteAllUserConfig($this->uid); // But again set flag that this user is about to be deleted - $this->config->setUserValue($this->uid, 'core', 'deleted', 'true'); - $this->config->setUserValue($this->uid, 'core', 'deleted.home-path', $this->getHome()); + $this->userConfig->setValueBool($this->uid, 'core', 'deleted', true); + $this->userConfig->setValueString($this->uid, 'core', 'deleted.home-path', $this->getHome()); // Commit the transaction so we are in a defined state: either the preferences are removed or an exception occurred but the delete flag is still present $database->commit(); } catch (\Throwable $e) { @@ -328,7 +330,7 @@ public function delete(): bool { $this->dispatcher->dispatchTyped(new UserDeletedEvent($this)); // Finally we can unset the delete flag and all other states - $this->config->deleteAllUserValues($this->uid); + $this->userConfig->deleteAllUserConfig($this->uid); return true; } @@ -469,8 +471,8 @@ public function canEditProperty(string $property): bool { public function isEnabled(): bool { $queryDatabaseValue = function (): bool { if ($this->enabled === null) { - $enabled = $this->config->getUserValue($this->uid, 'core', 'enabled', 'true'); - $this->enabled = $enabled === 'true'; + // FIXME Should we short-circuit userConfig here to avoid loading the whole config of a user to check enabled? + $this->enabled = $this->userConfig->getValueBool($this->uid, 'core', 'enabled', true); } return $this->enabled; }; @@ -488,7 +490,7 @@ public function isEnabled(): bool { public function setEnabled(bool $enabled = true): void { $oldStatus = $this->isEnabled(); $setDatabaseValue = function (bool $enabled): void { - $this->config->setUserValue($this->uid, 'core', 'enabled', $enabled ? 'true' : 'false'); + $this->userConfig->setValueBool($this->uid, 'core', 'enabled', $enabled); $this->enabled = $enabled; }; @@ -499,8 +501,7 @@ public function setEnabled(bool $enabled = true): void { if ($this->backend instanceof IProvideEnabledStateBackend) { $queryDatabaseValue = function (): bool { if ($this->enabled === null) { - $enabled = $this->config->getUserValue($this->uid, 'core', 'enabled', 'true'); - $this->enabled = $enabled === 'true'; + $this->enabled = $this->userConfig->getValueBool($this->uid, 'core', 'enabled', true); } return $this->enabled; }; @@ -526,13 +527,13 @@ public function getEMailAddress(): ?string { #[\Override] public function getSystemEMailAddress(): ?string { - $email = $this->config->getUserValue($this->uid, 'settings', 'email', null); + $email = $this->userConfig->getValueString($this->uid, 'settings', 'email', ''); return $email ? mb_strtolower(trim($email)) : null; } #[\Override] public function getPrimaryEMailAddress(): ?string { - $email = $this->config->getUserValue($this->uid, 'settings', 'primary_email', null); + $email = $this->userConfig->getValueString($this->uid, 'settings', 'primary_email', ''); return $email ? mb_strtolower(trim($email)) : null; } @@ -550,7 +551,7 @@ public function getQuota(): string { if ($overwriteQuota) { $quota = $overwriteQuota; } else { - $quota = $this->config->getUserValue($this->uid, 'files', 'quota', 'default'); + $quota = $this->userConfig->getValueString($this->uid, 'files', 'quota', 'default'); } if ($quota === 'default') { $quota = $this->config->getAppValue('files', 'default_quota', 'none'); @@ -593,7 +594,7 @@ public function getQuotaBytes(): int|float { */ #[\Override] public function setQuota($quota): void { - $oldQuota = $this->config->getUserValue($this->uid, 'files', 'quota', ''); + $oldQuota = $this->userConfig->getValueString($this->uid, 'files', 'quota', ''); if ($quota !== 'none' && $quota !== 'default') { $bytesQuota = Util::computerFileSize($quota); if ($bytesQuota === false) { @@ -602,7 +603,7 @@ public function setQuota($quota): void { $quota = Util::humanFileSize($bytesQuota); } if ($quota !== $oldQuota) { - $this->config->setUserValue($this->uid, 'files', 'quota', $quota); + $this->userConfig->setValueString($this->uid, 'files', 'quota', $quota); $this->triggerChange('quota', $quota, $oldQuota); } \OC_Helper::clearStorageInfo('/' . $this->uid . '/files'); @@ -610,23 +611,22 @@ public function setQuota($quota): void { #[\Override] public function getManagerUids(): array { - $encodedUids = $this->config->getUserValue( + return $this->userConfig->getValueArray( $this->uid, 'settings', self::CONFIG_KEY_MANAGERS, - '[]' + [] ); - return json_decode($encodedUids, false, 512, JSON_THROW_ON_ERROR); } #[\Override] public function setManagerUids(array $uids): void { $oldUids = $this->getManagerUids(); - $this->config->setUserValue( + $this->userConfig->setValueArray( $this->uid, 'settings', self::CONFIG_KEY_MANAGERS, - json_encode($uids, JSON_THROW_ON_ERROR) + $uids, ); $this->triggerChange('managers', $uids, $oldUids); } From 00d58abd0b1f7b4676ffa24a8385f4edbd266831 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 18 Jun 2026 18:09:06 +0200 Subject: [PATCH 4/7] chore: Adapt UserTest to IUserConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- tests/lib/User/UserTest.php | 158 +++++++++++++++++++----------------- 1 file changed, 82 insertions(+), 76 deletions(-) diff --git a/tests/lib/User/UserTest.php b/tests/lib/User/UserTest.php index 76bcac21c01af..6b0db4de58110 100644 --- a/tests/lib/User/UserTest.php +++ b/tests/lib/User/UserTest.php @@ -14,6 +14,7 @@ use OC\User\Database; use OC\User\User; use OCP\Comments\ICommentsManager; +use OCP\Config\IUserConfig; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\FileInfo; use OCP\Files\Storage\IStorageFactory; @@ -250,15 +251,17 @@ public function testGetHomeNotSupported(): void { $allConfig = $this->getMockBuilder(IConfig::class) ->disableOriginalConstructor() ->getMock(); - $allConfig->expects($this->any()) - ->method('getUserValue') - ->willReturn(true); $allConfig->expects($this->any()) ->method('getSystemValueString') ->with($this->equalTo('datadirectory')) ->willReturn('arbitrary/path'); - $user = new User('foo', $backend, $this->dispatcher, null, $allConfig); + $userConfig = $this->createMock(IUserConfig::class); + $userConfig->expects($this->any()) + ->method('getValueBool') + ->willReturn(true); + + $user = new User('foo', $backend, $this->dispatcher, null, $allConfig, $userConfig); $this->assertEquals('arbitrary/path/foo', $user->getHome()); } @@ -416,8 +419,10 @@ public function testDeleteHooks(bool $result, int $expectedHooks): void { $config->method('getSystemValueInt') ->willReturnArgument(1); + $userConfig = $this->createMock(IUserConfig::class); + $emitter = new PublicEmitter(); - $user = new User('foo', $backend, $this->dispatcher, $emitter, $config); + $user = new User('foo', $backend, $this->dispatcher, $emitter, $config, $userConfig); $hook = function (IUser $user) use ($test, &$hooksCalled): void { $hooksCalled++; @@ -431,8 +436,8 @@ public function testDeleteHooks(bool $result, int $expectedHooks): void { $notificationManager = $this->createMock(INotificationManager::class); if ($result) { - $config->expects($this->atLeastOnce()) - ->method('deleteAllUserValues') + $userConfig->expects($this->atLeastOnce()) + ->method('deleteAllUserConfig') ->with('foo'); $commentsManager->expects($this->once()) @@ -454,8 +459,8 @@ public function testDeleteHooks(bool $result, int $expectedHooks): void { ->method('markProcessed') ->with($notification); } else { - $config->expects($this->never()) - ->method('deleteAllUserValues'); + $userConfig->expects($this->never()) + ->method('deleteAllUserConfig'); $commentsManager->expects($this->never()) ->method('deleteReferencesOfActor'); @@ -497,12 +502,17 @@ public function testDeleteRecoverState() { $config->method('getSystemValueInt') ->willReturnArgument(1); - $userConfig = []; - $config->expects(self::atLeast(2)) - ->method('setUserValue') - ->willReturnCallback(function (): void { - $userConfig[] = func_get_args(); - }); + $userConfig = $this->createMock(IUserConfig::class); + $userConfig->expects(self::once()) + ->method('setValueBool') + ->with( + 'foo', 'core', 'deleted', true + ); + $userConfig->expects(self::once()) + ->method('setValueString') + ->with( + 'foo', 'core', 'deleted.home-path', '/home/path' + ); $commentsManager = $this->createMock(ICommentsManager::class); $commentsManager->expects($this->once()) @@ -514,7 +524,7 @@ public function testDeleteRecoverState() { $user = $this->getMockBuilder(User::class) ->onlyMethods(['getHome']) - ->setConstructorArgs(['foo', $backend, $this->dispatcher, null, $config]) + ->setConstructorArgs(['foo', $backend, $this->dispatcher, null, $config, $userConfig]) ->getMock(); $user->expects(self::atLeastOnce()) @@ -523,14 +533,6 @@ public function testDeleteRecoverState() { $user->delete(); - $this->assertEqualsCanonicalizing( - [ - ['foo', 'core', 'deleted', 'true', null], - ['foo', 'core', 'deleted.backup-home', '/home/path', null], - ], - $userConfig, - ); - $this->restoreService(ICommentsManager::class); } @@ -548,7 +550,7 @@ public function testGetCloudId(string $absoluteUrl, string $cloudId): void { $urlGenerator->method('getAbsoluteURL') ->withAnyParameters() ->willReturn($absoluteUrl); - $user = new User('foo', $backend, $this->dispatcher, null, null, $urlGenerator); + $user = new User('foo', $backend, $this->dispatcher, null, null, null, $urlGenerator); $this->assertEquals($cloudId, $user->getCloudId()); } @@ -568,16 +570,16 @@ public function testSetEMailAddressEmpty(): void { $emitter = new PublicEmitter(); $emitter->listen('\OC\User', 'changeUser', $hook); - $config = $this->createMock(IConfig::class); - $config->expects($this->once()) - ->method('deleteUserValue') + $userConfig = $this->createMock(IUserConfig::class); + $userConfig->expects($this->once()) + ->method('deleteUserConfig') ->with( 'foo', 'settings', 'email' ); - $user = new User('foo', $backend, $this->dispatcher, $emitter, $config); + $user = new User('foo', $backend, $this->dispatcher, $emitter, null, $userConfig); $user->setSystemEMailAddress(''); } @@ -597,9 +599,9 @@ public function testSetEMailAddress(): void { $emitter = new PublicEmitter(); $emitter->listen('\OC\User', 'changeUser', $hook); - $config = $this->createMock(IConfig::class); - $config->expects($this->once()) - ->method('setUserValue') + $userConfig = $this->createMock(IUserConfig::class); + $userConfig->expects($this->once()) + ->method('setValueString') ->with( 'foo', 'settings', @@ -607,7 +609,7 @@ public function testSetEMailAddress(): void { 'foo@bar.com' ); - $user = new User('foo', $backend, $this->dispatcher, $emitter, $config); + $user = new User('foo', $backend, $this->dispatcher, $emitter, null, $userConfig); $user->setSystemEMailAddress('foo@bar.com'); } @@ -623,14 +625,14 @@ public function testSetEMailAddressNoChange(): void { $dispatcher->expects($this->never()) ->method('dispatch'); - $config = $this->createMock(IConfig::class); - $config->expects($this->any()) - ->method('getUserValue') + $userConfig = $this->createMock(IUserConfig::class); + $userConfig->expects($this->any()) + ->method('getValueString') ->willReturn('foo@bar.com'); - $config->expects($this->any()) - ->method('setUserValue'); + $userConfig->expects($this->any()) + ->method('setValueString'); - $user = new User('foo', $backend, $dispatcher, $emitter, $config); + $user = new User('foo', $backend, $dispatcher, $emitter, null, $userConfig); $user->setSystemEMailAddress('foo@bar.com'); } @@ -650,9 +652,9 @@ public function testSetQuota(): void { $emitter = new PublicEmitter(); $emitter->listen('\OC\User', 'changeUser', $hook); - $config = $this->createMock(IConfig::class); - $config->expects($this->once()) - ->method('setUserValue') + $userConfig = $this->createMock(IUserConfig::class); + $userConfig->expects($this->once()) + ->method('setValueString') ->with( 'foo', 'files', @@ -660,7 +662,7 @@ public function testSetQuota(): void { '23 TB' ); - $user = new User('foo', $backend, $this->dispatcher, $emitter, $config); + $user = new User('foo', $backend, $this->dispatcher, $emitter, null, $userConfig); $user->setQuota('23 TB'); } @@ -674,7 +676,8 @@ public function testGetDefaultUnlimitedQuota(): void { ->method('emit'); $config = $this->createMock(IConfig::class); - $user = new User('foo', $backend, $this->dispatcher, $emitter, $config); + $userConfig = $this->createMock(IUserConfig::class); + $user = new User('foo', $backend, $this->dispatcher, $emitter, $config, $userConfig); $userValueMap = [ ['foo', 'files', 'quota', 'default', 'default'], @@ -684,7 +687,7 @@ public function testGetDefaultUnlimitedQuota(): void { // allow unlimited quota ['files', 'allow_unlimited_quota', '1', '1'], ]; - $config->method('getUserValue') + $userConfig->method('getValueString') ->willReturnMap($userValueMap); $config->method('getAppValue') ->willReturnMap($appValueMap); @@ -702,7 +705,8 @@ public function testGetDefaultUnlimitedQuotaForbidden(): void { ->method('emit'); $config = $this->createMock(IConfig::class); - $user = new User('foo', $backend, $this->dispatcher, $emitter, $config); + $userConfig = $this->createMock(IUserConfig::class); + $user = new User('foo', $backend, $this->dispatcher, $emitter, $config, $userConfig); $userValueMap = [ ['foo', 'files', 'quota', 'default', 'default'], @@ -715,7 +719,7 @@ public function testGetDefaultUnlimitedQuotaForbidden(): void { // expect seeing 1 GB used as fallback value ['files', 'default_quota', '1 GB', '1 GB'], ]; - $config->method('getUserValue') + $userConfig->method('getValueString') ->willReturnMap($userValueMap); $config->method('getAppValue') ->willReturnMap($appValueMap); @@ -732,22 +736,22 @@ public function testSetQuotaAddressNoChange(): void { $emitter->expects($this->never()) ->method('emit'); - $config = $this->createMock(IConfig::class); - $config->expects($this->any()) - ->method('getUserValue') + $userConfig = $this->createMock(IUserConfig::class); + $userConfig->expects($this->any()) + ->method('getValueString') ->willReturn('23 TB'); - $config->expects($this->never()) - ->method('setUserValue'); + $userConfig->expects($this->never()) + ->method('setValueString'); - $user = new User('foo', $backend, $this->dispatcher, $emitter, $config); + $user = new User('foo', $backend, $this->dispatcher, $emitter, null, $userConfig); $user->setQuota('23 TB'); } public function testGetLastLogin(): void { $backend = $this->createMock(\Test\Util\User\Dummy::class); - $config = $this->createMock(IConfig::class); - $config->method('getUserValue') + $userConfig = $this->createMock(IUserConfig::class); + $userConfig->method('getValueInt') ->willReturnCallback(function ($uid, $app, $key, $default) { if ($uid === 'foo' && $app === 'login' && $key === 'lastLogin') { return 42; @@ -756,7 +760,7 @@ public function testGetLastLogin(): void { } }); - $user = new User('foo', $backend, $this->dispatcher, null, $config); + $user = new User('foo', $backend, $this->dispatcher, null, null, $userConfig); $this->assertSame(42, $user->getLastLogin()); } @@ -764,37 +768,37 @@ public function testSetEnabled(): void { $backend = $this->createMock(\Test\Util\User\Dummy::class); $backend->method('getBackendName')->willReturn('foo'); - $config = $this->createMock(IConfig::class); - $config->expects($this->once()) - ->method('setUserValue') + $userConfig = $this->createMock(IUserConfig::class); + $userConfig->expects($this->once()) + ->method('setValueBool') ->with( $this->equalTo('foo'), $this->equalTo('core'), $this->equalTo('enabled'), - 'true' + true ); /* dav event listener gets the manager list from config */ - $config->expects(self::any()) - ->method('getUserValue') + $userConfig->expects(self::any()) + ->method('getValueBool') ->willReturnCallback( - fn ($user, $app, $key, $default) => ($key === 'enabled' ? 'false' : $default) + fn ($user, $app, $key, $default) => ($key === 'enabled' ? false : $default) ); - $user = new User('foo', $backend, $this->dispatcher, null, $config); + $user = new User('foo', $backend, $this->dispatcher, null, null, $userConfig); $user->setEnabled(true); } public function testSetDisabled(): void { $backend = $this->createMock(\Test\Util\User\Dummy::class); - $config = $this->createMock(IConfig::class); - $config->expects($this->once()) - ->method('setUserValue') + $userConfig = $this->createMock(IUserConfig::class); + $userConfig->expects($this->once()) + ->method('setValueBool') ->with( $this->equalTo('foo'), $this->equalTo('core'), $this->equalTo('enabled'), - 'false' + false ); $user = $this->getMockBuilder(User::class) @@ -803,7 +807,8 @@ public function testSetDisabled(): void { $backend, $this->dispatcher, null, - $config, + null, + $userConfig, ]) ->onlyMethods(['isEnabled', 'triggerChange']) ->getMock(); @@ -824,9 +829,9 @@ public function testSetDisabled(): void { public function testSetDisabledAlreadyDisabled(): void { $backend = $this->createMock(\Test\Util\User\Dummy::class); - $config = $this->createMock(IConfig::class); - $config->expects($this->never()) - ->method('setUserValue'); + $userConfig = $this->createMock(IUserConfig::class); + $userConfig->expects($this->never()) + ->method('setValueBool'); $user = $this->getMockBuilder(User::class) ->setConstructorArgs([ @@ -834,7 +839,8 @@ public function testSetDisabledAlreadyDisabled(): void { $backend, $this->dispatcher, null, - $config, + null, + $userConfig, ]) ->onlyMethods(['isEnabled', 'triggerChange']) ->getMock(); @@ -851,8 +857,8 @@ public function testSetDisabledAlreadyDisabled(): void { public function testGetEMailAddress(): void { $backend = $this->createMock(\Test\Util\User\Dummy::class); - $config = $this->createMock(IConfig::class); - $config->method('getUserValue') + $userConfig = $this->createMock(IUserConfig::class); + $userConfig->method('getValueString') ->willReturnCallback(function ($uid, $app, $key, $default) { if ($uid === 'foo' && $app === 'settings' && $key === 'email') { return 'foo@bar.com'; @@ -861,7 +867,7 @@ public function testGetEMailAddress(): void { } }); - $user = new User('foo', $backend, $this->dispatcher, null, $config); + $user = new User('foo', $backend, $this->dispatcher, null, null, $userConfig); $this->assertSame('foo@bar.com', $user->getEMailAddress()); } } From 7f4e722c6b9967d9b405e5574758d1da5fc2e499 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 22 Jun 2026 16:47:09 +0200 Subject: [PATCH 5/7] feat: Use a class to materialize user config entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Config/UserConfig.php | 179 ++++++++------------ lib/private/Config/UserConfigEntry.php | 106 ++++++++++++ tests/lib/Config/UserConfigTest.php | 24 ++- 5 files changed, 194 insertions(+), 117 deletions(-) create mode 100644 lib/private/Config/UserConfigEntry.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index cd4bfedd8aa1a..793023b02ee1b 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1366,6 +1366,7 @@ 'OC\\Config\\ConfigManager' => $baseDir . '/lib/private/Config/ConfigManager.php', 'OC\\Config\\PresetManager' => $baseDir . '/lib/private/Config/PresetManager.php', 'OC\\Config\\UserConfig' => $baseDir . '/lib/private/Config/UserConfig.php', + 'OC\\Config\\UserConfigEntry' => $baseDir . '/lib/private/Config/UserConfigEntry.php', 'OC\\Console\\Application' => $baseDir . '/lib/private/Console/Application.php', 'OC\\Console\\TimestampFormatter' => $baseDir . '/lib/private/Console/TimestampFormatter.php', 'OC\\ContactsManager' => $baseDir . '/lib/private/ContactsManager.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 3d664c1bf0621..20089dd536ed7 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1407,6 +1407,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Config\\ConfigManager' => __DIR__ . '/../../..' . '/lib/private/Config/ConfigManager.php', 'OC\\Config\\PresetManager' => __DIR__ . '/../../..' . '/lib/private/Config/PresetManager.php', 'OC\\Config\\UserConfig' => __DIR__ . '/../../..' . '/lib/private/Config/UserConfig.php', + 'OC\\Config\\UserConfigEntry' => __DIR__ . '/../../..' . '/lib/private/Config/UserConfigEntry.php', 'OC\\Console\\Application' => __DIR__ . '/../../..' . '/lib/private/Console/Application.php', 'OC\\Console\\TimestampFormatter' => __DIR__ . '/../../..' . '/lib/private/Console/TimestampFormatter.php', 'OC\\ContactsManager' => __DIR__ . '/../../..' . '/lib/private/ContactsManager.php', diff --git a/lib/private/Config/UserConfig.php b/lib/private/Config/UserConfig.php index f9d5e59628d2f..2e3b6baeb52e9 100644 --- a/lib/private/Config/UserConfig.php +++ b/lib/private/Config/UserConfig.php @@ -1,6 +1,7 @@ >> cache for normal config keys */ + /** @var CappedMemoryCache>> cache for normal config keys */ private CappedMemoryCache $fastCache; - /** @var CappedMemoryCache>> cache for lazy config keys */ + /** @var CappedMemoryCache>> cache for lazy config keys */ private CappedMemoryCache $lazyCache; /** @var array, aliases: array, strictness: Strictness}> ['app_id' => ['strictness' => ConfigLexiconStrictness, 'entries' => ['config_key' => ConfigLexiconEntry[]]] */ private array $configLexiconDetails = []; @@ -203,9 +202,9 @@ public function isSensitive(string $userId, string $app, string $key, ?bool $laz $this->matchAndApplyLexiconDefinition($userId, $app, $key); if (isset($this->fastCache[$userId][$app][$key])) { - return $this->isFlagged(self::FLAG_SENSITIVE, $this->fastCache[$userId][$app][$key]['flags']); + return $this->fastCache[$userId][$app][$key]->isSensitive(); } elseif (isset($this->lazyCache[$userId][$app][$key])) { - return $this->isFlagged(self::FLAG_SENSITIVE, $this->lazyCache[$userId][$app][$key]['flags']); + return $this->lazyCache[$userId][$app][$key]->isSensitive(); } else { throw new UnknownKeyException('Unknown config key ' . $app . '/' . $key); } @@ -230,9 +229,9 @@ public function isIndexed(string $userId, string $app, string $key, ?bool $lazy $this->matchAndApplyLexiconDefinition($userId, $app, $key); if (isset($this->fastCache[$userId][$app][$key])) { - return $this->isFlagged(self::FLAG_INDEXED, $this->fastCache[$userId][$app][$key]['flags']); + return $this->fastCache[$userId][$app][$key]->isIndexed(); } elseif (isset($this->lazyCache[$userId][$app][$key])) { - return $this->isFlagged(self::FLAG_INDEXED, $this->lazyCache[$userId][$app][$key]['flags']); + return $this->lazyCache[$userId][$app][$key]->isIndexed(); } else { throw new UnknownKeyException('Unknown config key ' . $app . '/' . $key); } @@ -352,12 +351,12 @@ public function getValuesByApps(string $userId, string $key, bool $lazy = false, $values = []; foreach (array_keys($cache) as $app) { if (isset($cache[$app][$key])) { - $valueDetail = $cache[$app][$key]; + $entry = $cache[$app][$key]; try { - $this->decryptSensitiveValue($userId, $app, $key, $valueDetail); - $value = $this->convertTypedValue($valueDetail['value'], $typedAs ?? $this->getValueType($userId, $app, $key, $lazy)); + $value = $this->getDecryptedSensitiveValue($userId, $app, $key, $entry); + $value = $this->convertTypedValue($value, $typedAs ?? $entry->getType()); } catch (IncorrectTypeException|UnknownKeyException) { - $value = $valueDetail['value']; + $value = $entry->getRawValue(); } $values[$app] = $value; } @@ -803,9 +802,9 @@ private function getTypedValue( * This way, lazyCache will be empty until the load for lazy config value is requested. */ if (isset($this->lazyCache[$userId][$app][$key])) { - $valueDetail = $this->lazyCache[$userId][$app][$key]; + $entry = $this->lazyCache[$userId][$app][$key]; } elseif (isset($this->fastCache[$userId][$app][$key])) { - $valueDetail = $this->fastCache[$userId][$app][$key]; + $entry = $this->fastCache[$userId][$app][$key]; } else { return $default; } @@ -815,18 +814,15 @@ private function getTypedValue( * If type of stored value is set as mixed, we don't filter. * If type of stored value is defined, we compare with the one requested. */ - $knownType = $valueDetail['type'] ?? null; + $knownType = $entry->getType(); if ($type !== ValueType::MIXED - && $knownType !== null && $knownType !== ValueType::MIXED && $type !== $knownType) { $this->logger->warning('conflict with value type from database', ['app' => $app, 'key' => $key, 'type' => $type, 'knownType' => $knownType]); throw new TypeConflictException('conflict with value type from database'); } - $this->decryptSensitiveValue($userId, $app, $key, $valueDetail); - - $value = $valueDetail['value']; + $value = $this->getDecryptedSensitiveValue($userId, $app, $key, $entry); // in case the key was modified while running matchAndApplyLexiconDefinition() we are // interested to check options in case a modification of the value is needed @@ -857,9 +853,9 @@ public function getValueType(string $userId, string $app, string $key, ?bool $la $this->matchAndApplyLexiconDefinition($userId, $app, $key); if (isset($this->fastCache[$userId][$app][$key])) { - return $this->fastCache[$userId][$app][$key]['type']; + return $this->fastCache[$userId][$app][$key]->getType(); } elseif (isset($this->lazyCache[$userId][$app][$key])) { - return $this->lazyCache[$userId][$app][$key]['type']; + return $this->lazyCache[$userId][$app][$key]->getType(); } else { throw new UnknownKeyException('Unknown config key ' . $app . '/' . $key); } @@ -885,9 +881,9 @@ public function getValueFlags(string $userId, string $app, string $key, bool $la $this->matchAndApplyLexiconDefinition($userId, $app, $key); if (isset($this->fastCache[$userId][$app][$key])) { - return $this->fastCache[$userId][$app][$key]['flags']; + return $this->fastCache[$userId][$app][$key]->getFlags(); } elseif (isset($this->lazyCache[$userId][$app][$key])) { - return $this->lazyCache[$userId][$app][$key]['flags']; + return $this->lazyCache[$userId][$app][$key]->getFlags(); } else { throw new UnknownKeyException('Unknown config key ' . $app . '/' . $key); } @@ -1160,16 +1156,23 @@ private function setTypedValue( $inserted = $refreshCache = false; $origValue = $value; - $sensitive = $this->isFlagged(self::FLAG_SENSITIVE, $flags); - if ($sensitive || ($this->hasKey($userId, $app, $key, $lazy) && $this->isSensitive($userId, $app, $key, $lazy))) { - $value = self::ENCRYPTION_PREFIX . $this->crypto->encrypt($value); - $flags |= self::FLAG_SENSITIVE; + $newEntry = new UserConfigEntry( + type:$type, + flags:$flags, + value:$value, + crypto:$this->crypto, + ); + - $sensitive = $newEntry->isSensitive(); + if ($newEntry->isSensitive() || ($this->hasKey($userId, $app, $key, $lazy) && $this->isSensitive($userId, $app, $key, $lazy))) { + $newEntry->setSensitive(true); } + $value = $newEntry->getDecryptedSensitiveValue(); + $flags = $newEntry->getFlags(); // if requested, we fill the 'indexed' field with current value $indexed = ''; - if ($type !== ValueType::ARRAY && $this->isFlagged(self::FLAG_INDEXED, $flags)) { - if ($this->isFlagged(self::FLAG_SENSITIVE, $flags)) { + if ($type !== ValueType::ARRAY && $newEntry->isIndexed()) { + if ($newEntry->isSensitive()) { $this->logger->warning('sensitive value are not to be indexed'); } elseif (strlen($value) > self::USER_MAX_LENGTH) { $this->logger->warning('value is too lengthy to be indexed'); @@ -1225,14 +1228,14 @@ private function setTypedValue( * We cannot insert a new row, meaning we need to update an already existing one */ if (!$inserted) { - $currType = $this->fastCache[$userId][$app][$key]['type'] ?? $this->lazyCache[$userId][$app][$key]['type'] ?? null; + $currType = ($this->fastCache[$userId][$app][$key] ?? $this->lazyCache[$userId][$app][$key] ?? null)?->getType(); if ($currType === null) { // this might happen when switching lazy loading status $this->loadConfigAll($userId); if (!isset($this->fastCache[$userId][$app][$key]) && !isset($this->lazyCache[$userId][$app][$key])) { throw new UnknownKeyException("unknown key $app $key for $userId even though $updateReason"); } - $currType = $this->fastCache[$userId][$app][$key]['type'] ?? $this->lazyCache[$userId][$app][$key]['type'] ?? null; + $currType = ($this->fastCache[$userId][$app][$key] ?? $this->lazyCache[$userId][$app][$key] ?? null)?->getType(); } /** @@ -1287,15 +1290,16 @@ private function setTypedValue( } // update local cache - $valueDetail = [ - 'value' => $value, - 'type' => $type, - 'flags' => $flags - ]; + $entry = new UserConfigEntry( + value: $value, + type: $type, + flags: $flags, + crypto:$this->crypto, + ); if ($lazy) { - $this->lazyCache[$userId][$app][$key] = $valueDetail; + $this->lazyCache[$userId][$app][$key] = $entry; } else { - $this->fastCache[$userId][$app][$key] = $valueDetail; + $this->fastCache[$userId][$app][$key] = $entry; } return true; @@ -1332,9 +1336,9 @@ public function updateType(string $userId, string $app, string $key, ValueType $ $update->executeStatement(); if ($lazy) { - $this->lazyCache[$userId][$app][$key]['type'] = $type; + $this->lazyCache[$userId][$app][$key]->setType($type); } else { - $this->fastCache[$userId][$app][$key]['type'] = $type; + $this->fastCache[$userId][$app][$key]->setType($type); } return true; @@ -1376,28 +1380,18 @@ public function updateSensitive(string $userId, string $app, string $key, bool $ throw new UnknownKeyException('unknown config key'); } - $valueDetail = $cache[$userId][$app][$key]; - $flags = $this->getValueFlags($userId, $app, $key); - if ($sensitive) { - $flags |= self::FLAG_SENSITIVE; - $value = self::ENCRYPTION_PREFIX . $this->crypto->encrypt($valueDetail['value']); - } else { - $flags &= ~self::FLAG_SENSITIVE; - $this->decryptSensitiveValue($userId, $app, $key, $valueDetail); - $value = $valueDetail['value']; - } + $entry = $cache[$userId][$app][$key]; + $entry->setSensitive($sensitive); $update = $this->connection->getQueryBuilder(); $update->update('preferences') - ->set('flags', $update->createNamedParameter($flags, IQueryBuilder::PARAM_INT)) - ->set('configvalue', $update->createNamedParameter($value)) + ->set('flags', $update->createNamedParameter($entry->getFlags(), IQueryBuilder::PARAM_INT)) + ->set('configvalue', $update->createNamedParameter($entry->getRawValue())) ->where($update->expr()->eq('userid', $update->createNamedParameter($userId))) ->andWhere($update->expr()->eq('appid', $update->createNamedParameter($app))) ->andWhere($update->expr()->eq('configkey', $update->createNamedParameter($key))); $update->executeStatement(); - $cache[$userId][$app][$key]['flags'] = $flags; - return true; } @@ -1466,26 +1460,23 @@ public function updateIndexed(string $userId, string $app, string $key, bool $in throw new UnknownKeyException('unknown config key'); } - $value = $cache[$userId][$app][$key]['value']; - $flags = $this->getValueFlags($userId, $app, $key); + $entry = $cache[$userId][$app][$key]; + $entry->setIndexed($indexed); if ($indexed) { - $indexed = $value; + $indexed = $entry->getRawValue(); } else { - $flags &= ~self::FLAG_INDEXED; $indexed = ''; } $update = $this->connection->getQueryBuilder(); $update->update('preferences') - ->set('flags', $update->createNamedParameter($flags, IQueryBuilder::PARAM_INT)) + ->set('flags', $update->createNamedParameter($entry->getFlags(), IQueryBuilder::PARAM_INT)) ->set('indexed', $update->createNamedParameter($indexed)) ->where($update->expr()->eq('userid', $update->createNamedParameter($userId))) ->andWhere($update->expr()->eq('appid', $update->createNamedParameter($app))) ->andWhere($update->expr()->eq('configkey', $update->createNamedParameter($key))); $update->executeStatement(); - $cache[$userId][$app][$key]['flags'] = $flags; - return true; } @@ -1634,19 +1625,18 @@ public function getDetails(string $userId, string $app, string $key): array { throw new UnknownKeyException('unknown config key'); } - $valueDetail = $cache[$app][$key]; - $sensitive = $this->isSensitive($userId, $app, $key, null); - $this->decryptSensitiveValue($userId, $app, $key, $valueDetail); + $entry = $cache[$app][$key]; + $value = $this->getDecryptedSensitiveValue($userId, $app, $key, $entry); return [ 'userId' => $userId, 'app' => $app, 'key' => $key, - 'value' => $valueDetail['value'], + 'value' => $value, 'type' => $type->value, 'lazy' => $lazy, 'typeString' => $typeString, - 'sensitive' => $sensitive + 'sensitive' => $entry->isSensitive(), ]; } @@ -1775,16 +1765,6 @@ public function statusCache(): array { ]; } - /** - * @param int $needle bitflag to search - * @param int $flags all flags - * - * @return bool TRUE if bitflag $needle is set in $flags - */ - private function isFlagged(int $needle, int $flags): bool { - return (($needle & $flags) !== 0); - } - /** * Confirm the string set for app and key fit the database description * @@ -1869,11 +1849,12 @@ private function loadConfig(string $userId, ?bool $lazy = false): void { $rows = $result->fetchAllAssociative(); foreach ($rows as $row) { - $valueDetail = [ - 'value' => $row['configvalue'] ?? '', - 'type' => ValueType::from((int)($row['type'] ?? 0)), - 'flags' => (int)($row['flags'] ?? 0), - ]; + $valueDetail = new UserConfigEntry( + value: $row['configvalue'] ?? '', + type: ValueType::from((int)($row['type'] ?? 0)), + flags: (int)($row['flags'] ?? 0), + crypto:$this->crypto, + ); if ($this->migrationCompleted && (($row['lazy'] ?? ($lazy ?? 0) ? 1 : 0) === 1)) { $this->lazyCache[$userId][$row['appid']][$row['configkey']] = $valueDetail; } else { @@ -1912,22 +1893,16 @@ private function isLoaded(string $userId, ?bool $lazy): bool { * @return array */ private function formatAppValues(string $userId, string $app, array $values, bool $filtered = false): array { - foreach ($values as $key => $valueDetail) { - $value = $valueDetail['value']; - //$key = (string)$key; - try { - $type = $this->getValueType($userId, $app, (string)$key); - } catch (UnknownKeyException) { - continue; - } + foreach ($values as $key => $entry) { + $value = $entry->getRawValue(); + $type = $entry->getType(); - if ($this->isFlagged(self::FLAG_SENSITIVE, $valueDetail['flags'] ?? 0)) { + if ($entry->isSensitive()) { if ($filtered) { $value = IConfig::SENSITIVE_VALUE; $type = ValueType::STRING; } else { - $this->decryptSensitiveValue($userId, $app, (string)$key, $valueDetail); - $value = $valueDetail['value']; + $value = $this->getDecryptedSensitiveValue($userId, $app, (string)$key, $entry); } } @@ -1964,30 +1939,18 @@ private function convertTypedValue(string $value, ValueType $type): string|int|f return $value; } - /** - * will change referenced $value with the decrypted value in case of encrypted (sensitive value) - * - * @param array{type: ValueType, flags: int, value: string} $valueDetail - */ - private function decryptSensitiveValue(string $userId, string $app, string $key, array &$valueDetail): void { - if (!$this->isFlagged(self::FLAG_SENSITIVE, $valueDetail['flags'] ?? 0)) { - return; - } - - if (!str_starts_with($valueDetail['value'], self::ENCRYPTION_PREFIX)) { - return; - } - + private function getDecryptedSensitiveValue(string $userId, string $app, string $key, UserConfigEntry $entry): string { try { - $valueDetail['value'] = $this->crypto->decrypt(substr($valueDetail['value'], self::ENCRYPTION_PREFIX_LENGTH)); + return $entry->getDecryptedSensitiveValue(); } catch (\Exception $e) { $this->logger->warning('could not decrypt sensitive value', [ 'userId' => $userId, 'app' => $app, 'key' => $key, - 'value' => $valueDetail['value'], + 'value' => $entry->getRawValue(), 'exception' => $e ]); + return $entry->getRawValue(); } } diff --git a/lib/private/Config/UserConfigEntry.php b/lib/private/Config/UserConfigEntry.php new file mode 100644 index 0000000000000..f0a77567b30dd --- /dev/null +++ b/lib/private/Config/UserConfigEntry.php @@ -0,0 +1,106 @@ +value; + } + + public function getType(): ValueType { + return $this->type; + } + + public function setType(ValueType $type): void { + $this->type = $type; + } + + public function getFlags(): int { + return $this->flags; + } + + public function isFlagged(int $mask): bool { + return (($mask & $this->flags) === $mask); + } + + public function isSensitive(): bool { + return $this->isFlagged(IUserConfig::FLAG_SENSITIVE); + } + + public function isIndexed(): bool { + return $this->isFlagged(IUserConfig::FLAG_INDEXED); + } + + /** + * will change referenced $value with the decrypted value in case of encrypted (sensitive value) + * + * @param array{type: ValueType, flags: int, value: string} $valueDetail + */ + public function getDecryptedSensitiveValue(): string { + if (!$this->isFlagged(IUserConfig::FLAG_SENSITIVE)) { + return $this->value; + } + + if ($this->decryptedValue !== null) { + return $this->decryptedValue; + } + + if (!str_starts_with($this->value, self::ENCRYPTION_PREFIX)) { + return $this->value; + } + + $this->decryptedValue = $this->crypto->decrypt(substr($this->value, self::ENCRYPTION_PREFIX_LENGTH)); + return $this->decryptedValue; + } + + public function setSensitive(bool $sensitive): void { + if ($sensitive) { + $this->flags |= IUserConfig::FLAG_SENSITIVE; + $this->decryptedValue = $this->value; + $this->value = self::ENCRYPTION_PREFIX . $this->crypto->encrypt($this->value); + } else { + $clearValue = $this->getDecryptedSensitiveValue(); + $this->flags &= ~IUserConfig::FLAG_SENSITIVE; + $this->value = $clearValue; + $this->decryptedValue = null; + } + } + + public function setIndexed(bool $indexed): void { + if ($indexed) { + $this->flags |= IUserConfig::FLAG_INDEXED; + $this->decryptedValue = $this->value; + $this->value = self::ENCRYPTION_PREFIX . $this->crypto->encrypt($this->value); + } else { + $clearValue = $this->getDecryptedSensitiveValue(); + $this->flags &= ~IUserConfig::FLAG_INDEXED; + $this->value = $clearValue; + $this->decryptedValue = null; + } + } +} diff --git a/tests/lib/Config/UserConfigTest.php b/tests/lib/Config/UserConfigTest.php index a5affdec85823..7d0feb84f14c0 100644 --- a/tests/lib/Config/UserConfigTest.php +++ b/tests/lib/Config/UserConfigTest.php @@ -11,6 +11,7 @@ use OC\Config\ConfigManager; use OC\Config\PresetManager; use OC\Config\UserConfig; +use OC\Config\UserConfigEntry; use OCP\Config\Exceptions\TypeConflictException; use OCP\Config\Exceptions\UnknownKeyException; use OCP\Config\IUserConfig; @@ -230,7 +231,7 @@ protected function setUp(): void { $flags = $row[4] ?? 0; if ((UserConfig::FLAG_SENSITIVE & $flags) !== 0) { if (!isset($this->basePreferences[$userId][$appId][$key]['encrypted'])) { - $value = self::invokePrivate(UserConfig::class, 'ENCRYPTION_PREFIX') + $value = self::invokePrivate(UserConfigEntry::class, 'ENCRYPTION_PREFIX') . $this->crypto->encrypt((string)$value); $this->basePreferences[$userId][$appId][$key]['encrypted'] = $value; } else { @@ -1422,10 +1423,13 @@ public function testUpdateSensitive( $userConfig = $this->generateUserConfig($preload ?? []); $this->assertEquals($sensitive, $userConfig->isSensitive($userId, $app, $key)); if ($sensitive) { - $this->assertEquals(true, str_starts_with( - $userConfig->statusCache()['fastCache'][$userId][$app][$key]['value'] - ?? $userConfig->statusCache()['lazyCache'][$userId][$app][$key]['value'], - '$UserConfigEncryption$') + $statusCache = $userConfig->statusCache(); + $this->assertEquals( + true, + str_starts_with( + ($statusCache['fastCache'][$userId][$app][$key] + ?? $statusCache['lazyCache'][$userId][$app][$key])->getRawValue(), + '$UserConfigEncryption$') ); } } @@ -1450,10 +1454,11 @@ public function testUpdateGlobalSensitive(bool $sensitive): void { $this->assertEquals($value, $userConfig->getValueString('user1', $app, $key)); foreach (['user1', 'user2', 'user3', 'user4'] as $userId) { $userConfig->getValueString($userId, $app, $key); // cache loading for userId + $statusCache = $userConfig->statusCache(); $this->assertEquals( !$sensitive, str_starts_with( - $userConfig->statusCache()['fastCache'][$userId][$app][$key]['value'] - ?? $userConfig->statusCache()['lazyCache'][$userId][$app][$key]['value'], + ($statusCache['fastCache'][$userId][$app][$key] + ?? $statusCache['lazyCache'][$userId][$app][$key])->getRawValue(), '$UserConfigEncryption$' ) ); @@ -1464,10 +1469,11 @@ public function testUpdateGlobalSensitive(bool $sensitive): void { $this->assertEquals($value, $userConfig->getValueString('user1', $app, $key)); foreach (['user1', 'user2', 'user3', 'user4'] as $userId) { $this->assertEquals($sensitive, $userConfig->isSensitive($userId, $app, $key)); + $statusCache = $userConfig->statusCache(); // should only work if updateGlobalSensitive drop cache $this->assertEquals($sensitive, str_starts_with( - $userConfig->statusCache()['fastCache'][$userId][$app][$key]['value'] - ?? $userConfig->statusCache()['lazyCache'][$userId][$app][$key]['value'], + ($statusCache['fastCache'][$userId][$app][$key] + ?? $statusCache['lazyCache'][$userId][$app][$key])->getRawValue(), '$UserConfigEncryption$') ); } From 66fed880063a9fed68671e6d59cf37df431403d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 30 Jun 2026 14:31:14 +0200 Subject: [PATCH 6/7] fix: Revert using setValueBool in User class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Too many code relies on the value being stored as a true/false string. setValueBool currently stores 1/0 and breaks things. Signed-off-by: Côme Chilliet --- lib/private/User/User.php | 6 +++--- tests/lib/User/UserTest.php | 35 ++++++++++++++++++----------------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/lib/private/User/User.php b/lib/private/User/User.php index d99e35b4504ea..7c8e5377e2bf4 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -267,7 +267,7 @@ public function delete(): bool { // Set delete flag on the user - this is needed to ensure that the user data is removed if there happen any exception in the backend // because we can not restore the user meaning we could not rollback to any stable state otherwise. - $this->userConfig->setValueBool($this->uid, 'core', 'deleted', true); + $this->config->setUserValue($this->uid, 'core', 'deleted', 'true'); // We also need to backup the home path as this can not be reconstructed later if the original backend uses custom home paths $this->userConfig->setValueString($this->uid, 'core', 'deleted.home-path', $this->getHome()); @@ -314,7 +314,7 @@ public function delete(): bool { // Remove all user settings $this->userConfig->deleteAllUserConfig($this->uid); // But again set flag that this user is about to be deleted - $this->userConfig->setValueBool($this->uid, 'core', 'deleted', true); + $this->config->setUserValue($this->uid, 'core', 'deleted', 'true'); $this->userConfig->setValueString($this->uid, 'core', 'deleted.home-path', $this->getHome()); // Commit the transaction so we are in a defined state: either the preferences are removed or an exception occurred but the delete flag is still present $database->commit(); @@ -490,7 +490,7 @@ public function isEnabled(): bool { public function setEnabled(bool $enabled = true): void { $oldStatus = $this->isEnabled(); $setDatabaseValue = function (bool $enabled): void { - $this->userConfig->setValueBool($this->uid, 'core', 'enabled', $enabled); + $this->config->setUserValue($this->uid, 'core', 'enabled', $enabled ? 'true' : 'false'); $this->enabled = $enabled; }; diff --git a/tests/lib/User/UserTest.php b/tests/lib/User/UserTest.php index 6b0db4de58110..895dd8e39e333 100644 --- a/tests/lib/User/UserTest.php +++ b/tests/lib/User/UserTest.php @@ -501,13 +501,11 @@ public function testDeleteRecoverState() { ->willReturnArgument(1); $config->method('getSystemValueInt') ->willReturnArgument(1); + $config->expects(self::once()) + ->method('setUserValue') + ->with('foo', 'core', 'deleted', 'true'); $userConfig = $this->createMock(IUserConfig::class); - $userConfig->expects(self::once()) - ->method('setValueBool') - ->with( - 'foo', 'core', 'deleted', true - ); $userConfig->expects(self::once()) ->method('setValueString') ->with( @@ -768,37 +766,38 @@ public function testSetEnabled(): void { $backend = $this->createMock(\Test\Util\User\Dummy::class); $backend->method('getBackendName')->willReturn('foo'); - $userConfig = $this->createMock(IUserConfig::class); - $userConfig->expects($this->once()) - ->method('setValueBool') + $config = $this->createMock(IConfig::class); + $config->expects($this->once()) + ->method('setUserValue') ->with( $this->equalTo('foo'), $this->equalTo('core'), $this->equalTo('enabled'), - true + 'true' ); /* dav event listener gets the manager list from config */ + $userConfig = $this->createMock(IUserConfig::class); $userConfig->expects(self::any()) ->method('getValueBool') ->willReturnCallback( fn ($user, $app, $key, $default) => ($key === 'enabled' ? false : $default) ); - $user = new User('foo', $backend, $this->dispatcher, null, null, $userConfig); + $user = new User('foo', $backend, $this->dispatcher, null, $config, $userConfig); $user->setEnabled(true); } public function testSetDisabled(): void { $backend = $this->createMock(\Test\Util\User\Dummy::class); - $userConfig = $this->createMock(IUserConfig::class); - $userConfig->expects($this->once()) - ->method('setValueBool') + $config = $this->createMock(IConfig::class); + $config->expects($this->once()) + ->method('setUserValue') ->with( $this->equalTo('foo'), $this->equalTo('core'), $this->equalTo('enabled'), - false + 'false' ); $user = $this->getMockBuilder(User::class) @@ -807,8 +806,7 @@ public function testSetDisabled(): void { $backend, $this->dispatcher, null, - null, - $userConfig, + $config, ]) ->onlyMethods(['isEnabled', 'triggerChange']) ->getMock(); @@ -829,6 +827,9 @@ public function testSetDisabled(): void { public function testSetDisabledAlreadyDisabled(): void { $backend = $this->createMock(\Test\Util\User\Dummy::class); + $config = $this->createMock(IConfig::class); + $config->expects($this->never()) + ->method('setUserValue'); $userConfig = $this->createMock(IUserConfig::class); $userConfig->expects($this->never()) ->method('setValueBool'); @@ -839,7 +840,7 @@ public function testSetDisabledAlreadyDisabled(): void { $backend, $this->dispatcher, null, - null, + $config, $userConfig, ]) ->onlyMethods(['isEnabled', 'triggerChange']) From e9866ef5e726fc1d19b0f0423bdebf26df96066e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 30 Jun 2026 15:12:55 +0200 Subject: [PATCH 7/7] chore: Suppress psalm false-positives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Config/UserConfig.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/private/Config/UserConfig.php b/lib/private/Config/UserConfig.php index 2e3b6baeb52e9..e786d13f10d58 100644 --- a/lib/private/Config/UserConfig.php +++ b/lib/private/Config/UserConfig.php @@ -1297,8 +1297,10 @@ private function setTypedValue( crypto:$this->crypto, ); if ($lazy) { + /** @psalm-suppress InvalidArgument https://github.com/vimeo/psalm/issues/6350 */ $this->lazyCache[$userId][$app][$key] = $entry; } else { + /** @psalm-suppress InvalidArgument https://github.com/vimeo/psalm/issues/6350 */ $this->fastCache[$userId][$app][$key] = $entry; } @@ -1856,8 +1858,10 @@ private function loadConfig(string $userId, ?bool $lazy = false): void { crypto:$this->crypto, ); if ($this->migrationCompleted && (($row['lazy'] ?? ($lazy ?? 0) ? 1 : 0) === 1)) { + /** @psalm-suppress InvalidArgument https://github.com/vimeo/psalm/issues/6350 */ $this->lazyCache[$userId][$row['appid']][$row['configkey']] = $valueDetail; } else { + /** @psalm-suppress InvalidArgument https://github.com/vimeo/psalm/issues/6350 */ $this->fastCache[$userId][$row['appid']][$row['configkey']] = $valueDetail; } }