Merge pull request #34612 from nextcloud/stable25-fix-background-appdata-scrope

[stable25] Scope the appdata theming storage for global and users

apps/theming/lib/Controller/UserThemeController.php

@@ -156,7 +156,8 @@ class UserThemeController extends OCSController {
 	 * @NoAdminRequired
 	 */
 	public function setBackground(string $type = 'default', string $value = ''): JSONResponse {
-		$currentVersion = (int)$this->config->getUserValue($this->userId, Application::APP_ID, 'backgroundVersion', '0');
+		$currentVersion = (int)$this->config->getUserValue($this->userId, Application::APP_ID, 'userCacheBuster', '0');
+
 		try {
 			switch ($type) {
 				case 'shipped':
@@ -179,14 +180,14 @@ class UserThemeController extends OCSController {
 		} catch (\Throwable $e) {
 			return new JSONResponse(['error' => $e->getMessage()], Http::STATUS_INTERNAL_SERVER_ERROR);
 		}
+
 		$currentVersion++;
-		$this->config->setUserValue($this->userId, Application::APP_ID, 'backgroundVersion', (string)$currentVersion);
-		// FIXME replace with user-specific cachebuster increase https://github.com/nextcloud/server/issues/34472
-		$this->themingDefaults->increaseCacheBuster();
+		$this->config->setUserValue($this->userId, Application::APP_ID, 'userCacheBuster', (string)$currentVersion);
+
 		return new JSONResponse([
 			'type' => $type,
 			'value' => $value,
-			'version' => $this->config->getUserValue($this->userId, Application::APP_ID, 'backgroundVersion', $currentVersion)
+			'version' => $currentVersion,
 		]);
 	}
 }

apps/theming/lib/ImageManager.php

@@ -66,14 +66,13 @@ class ImageManager {
 								IURLGenerator $urlGenerator,
 								ICacheFactory $cacheFactory,
 								ILogger $logger,
-								ITempManager $tempManager
-	) {
+								ITempManager $tempManager) {
 		$this->config = $config;
-		$this->appData = $appData;
 		$this->urlGenerator = $urlGenerator;
 		$this->cacheFactory = $cacheFactory;
 		$this->logger = $logger;
 		$this->tempManager = $tempManager;
+		$this->appData = $appData;
 	}
 
 	public function getImageUrl(string $key, bool $useSvg = true): string {
@@ -106,10 +105,12 @@ class ImageManager {
 	 */
 	public function getImage(string $key, bool $useSvg = true): ISimpleFile {
 		$logo = $this->config->getAppValue('theming', $key . 'Mime', '');
-		$folder = $this->appData->getFolder('images');
+		$folder = $this->getRootFolder()->getFolder('images');
+
 		if ($logo === '' || !$folder->fileExists($key)) {
 			throw new NotFoundException();
 		}
+
 		if (!$useSvg && $this->shouldReplaceIcons()) {
 			if (!$folder->fileExists($key . '.png')) {
 				try {
@@ -127,6 +128,7 @@ class ImageManager {
 				return $folder->getFile($key . '.png');
 			}
 		}
+
 		return $folder->getFile($key);
 	}
 
@@ -158,9 +160,9 @@ class ImageManager {
 	public function getCacheFolder(): ISimpleFolder {
 		$cacheBusterValue = $this->config->getAppValue('theming', 'cachebuster', '0');
 		try {
-			$folder = $this->appData->getFolder($cacheBusterValue);
+			$folder = $this->getRootFolder()->getFolder($cacheBusterValue);
 		} catch (NotFoundException $e) {
-			$folder = $this->appData->newFolder($cacheBusterValue);
+			$folder = $this->getRootFolder()->newFolder($cacheBusterValue);
 			$this->cleanup();
 		}
 		return $folder;
@@ -202,13 +204,13 @@ class ImageManager {
 	public function delete(string $key): void {
 		/* ignore exceptions, since we don't want to fail hard if something goes wrong during cleanup */
 		try {
-			$file = $this->appData->getFolder('images')->getFile($key);
+			$file = $this->getRootFolder()->getFolder('images')->getFile($key);
 			$file->delete();
 		} catch (NotFoundException $e) {
 		} catch (NotPermittedException $e) {
 		}
 		try {
-			$file = $this->appData->getFolder('images')->getFile($key . '.png');
+			$file = $this->getRootFolder()->getFolder('images')->getFile($key . '.png');
 			$file->delete();
 		} catch (NotFoundException $e) {
 		} catch (NotPermittedException $e) {
@@ -219,9 +221,9 @@ class ImageManager {
 		$this->delete($key);
 
 		try {
-			$folder = $this->appData->getFolder('images');
+			$folder = $this->getRootFolder()->getFolder('images');
 		} catch (NotFoundException $e) {
-			$folder = $this->appData->newFolder('images');
+			$folder = $this->getRootFolder()->newFolder('images');
 		}
 
 		$target = $folder->newFile($key);
@@ -288,7 +290,7 @@ class ImageManager {
 	 */
 	public function cleanup() {
 		$currentFolder = $this->getCacheFolder();
-		$folders = $this->appData->getDirectoryListing();
+		$folders = $this->getRootFolder()->getDirectoryListing();
 		foreach ($folders as $folder) {
 			if ($folder->getName() !== 'images' && $folder->getName() !== $currentFolder->getName()) {
 				$folder->delete();
@@ -316,4 +318,12 @@ class ImageManager {
 		$cache->set('shouldReplaceIcons', $value);
 		return $value;
 	}
+
+	private function getRootFolder(): ISimpleFolder {
+		try {
+			return $this->appData->getFolder('global');
+		} catch (NotFoundException $e) {
+			return $this->appData->newFolder('global');
+		}
+	}
 }

apps/theming/lib/Jobs/MigrateBackgroundImages.php

@@ -34,6 +34,7 @@ use OCP\BackgroundJob\QueuedJob;
 use OCP\Files\AppData\IAppDataFactory;
 use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
+use OCP\Files\SimpleFS\ISimpleFolder;
 use OCP\IConfig;
 
 class MigrateBackgroundImages extends QueuedJob {
@@ -57,7 +58,6 @@ class MigrateBackgroundImages extends QueuedJob {
 			return;
 		}
 
-		$themingData = $this->appDataFactory->get(Application::APP_ID);
 		$dashboardData = $this->appDataFactory->get('dashboard');
 
 		$userIds = $this->config->getUsersForUserValue('theming', 'background', 'custom');
@@ -79,11 +79,8 @@ class MigrateBackgroundImages extends QueuedJob {
 
 				// migration
 				$file = $dashboardData->getFolder($userId)->getFile('background.jpg');
-				try {
-					$targetDir = $themingData->getFolder($userId);
-				} catch (NotFoundException $e) {
-					$targetDir = $themingData->newFolder($userId);
-				}
+				$targetDir = $this->getUserFolder($userId);
+
 				if (!$targetDir->fileExists('background.jpg')) {
 					$targetDir->newFile('background.jpg', $file->getContent());
 				}
@@ -104,4 +101,23 @@ class MigrateBackgroundImages extends QueuedJob {
 			$this->jobList->add(self::class);
 		}
 	}
+
+	/**
+	 * Get the root location for users theming data
+	 */
+	protected function getUserFolder(string $userId): ISimpleFolder {
+		$themingData = $this->appDataFactory->get(Application::APP_ID);
+
+		try {
+			$rootFolder = $themingData->getFolder('users');
+		} catch (NotFoundException $e) {
+			$rootFolder = $themingData->newFolder('users');
+		}
+
+		try {
+			return $rootFolder->getFolder($userId);
+		} catch (NotFoundException $e) {
+			return $rootFolder->newFolder($userId);
+		}
+	}
 }

apps/theming/lib/Listener/BeforeTemplateRenderedListener.php

@@ -89,11 +89,6 @@ class BeforeTemplateRenderedListener implements IEventListener {
 				$this->config->getUserValue($userId, Application::APP_ID, 'background', 'default'),
 			);
 
-			$this->initialState->provideInitialState(
-				'backgroundVersion',
-				$this->config->getUserValue($userId, Application::APP_ID, 'backgroundVersion', 0),
-			);
-
 			$this->initialState->provideInitialState(
 				'themingDefaultBackground',
 				 $this->config->getAppValue('theming', 'backgroundMime', ''),

apps/theming/lib/Service/BackgroundService.php

@@ -136,19 +136,19 @@ class BackgroundService {
 	private string $userId;
 	private IAppDataFactory $appDataFactory;
 
-	public function __construct(
-		IRootFolder     $rootFolder,
-		IAppDataFactory $appDataFactory,
-		IConfig         $config,
-		?string         $userId
-	) {
+	public function __construct(IRootFolder $rootFolder,
+								IAppData $appData,
+								IConfig $config,
+								?string $userId,
+								IAppDataFactory $appDataFactory) {
 		if ($userId === null) {
 			return;
 		}
+
 		$this->rootFolder = $rootFolder;
-		$this->appData = $appDataFactory->get(Application::APP_ID);
 		$this->config = $config;
 		$this->userId = $userId;
+		$this->appData = $appData;
 		$this->appDataFactory = $appDataFactory;
 	}
 
@@ -167,12 +167,15 @@ class BackgroundService {
 	public function setFileBackground($path): void {
 		$this->config->setUserValue($this->userId, Application::APP_ID, 'background', 'custom');
 		$userFolder = $this->rootFolder->getUserFolder($this->userId);
+
 		/** @var File $file */
 		$file = $userFolder->get($path);
 		$image = new \OCP\Image();
+
 		if ($image->loadFromFileHandle($file->fopen('r')) === false) {
 			throw new InvalidArgumentException('Invalid image file');
 		}
+
 		$this->getAppDataFolder()->newFile('background.jpg', $file->fopen('r'));
 	}
 
@@ -207,14 +210,21 @@ class BackgroundService {
 	}
 
 	/**
+	 * Storing the data in appdata/theming/users/USERID
+	 *
 	 * @return ISimpleFolder
 	 * @throws NotPermittedException
 	 */
 	private function getAppDataFolder(): ISimpleFolder {
 		try {
-			return $this->appData->getFolder($this->userId);
+			$rootFolder = $this->appData->getFolder('users');
 		} catch (NotFoundException $e) {
-			return $this->appData->newFolder($this->userId);
+			$rootFolder = $this->appData->newFolder('users');
+		}
+		try {
+			return $rootFolder->getFolder($this->userId);
+		} catch (NotFoundException $e) {
+			return $rootFolder->newFolder($this->userId);
 		}
 	}
 }

apps/theming/lib/Service/ThemeInjectionService.php

@@ -22,8 +22,11 @@
  */
 namespace OCA\Theming\Service;
 
+use OCA\Theming\AppInfo\Application;
 use OCA\Theming\Themes\DefaultTheme;
+use OCP\IConfig;
 use OCP\IURLGenerator;
+use OCP\IUserSession;
 use OCP\Util;
 
 class ThemeInjectionService {
@@ -31,13 +34,23 @@ class ThemeInjectionService {
 	private IURLGenerator $urlGenerator;
 	private ThemesService $themesService;
 	private DefaultTheme $defaultTheme;
+	private IConfig $config;
+	private ?string $userId;
 
 	public function __construct(IURLGenerator $urlGenerator,
 								ThemesService $themesService,
-								DefaultTheme $defaultTheme) {
+								DefaultTheme $defaultTheme,
+								IConfig $config,
+								IUserSession $userSession) {
 		$this->urlGenerator = $urlGenerator;
 		$this->themesService = $themesService;
 		$this->defaultTheme = $defaultTheme;
+		$this->config = $config;
+		if ($userSession->getUser() !== null) {
+			$this->userId = $userSession->getUser()->getUID();
+		} else {
+			$this->userId = null;
+		}
 	}
 
 	public function injectHeaders() {
@@ -50,13 +63,13 @@ class ThemeInjectionService {
 
 		// Default theme fallback
 		$this->addThemeHeader($defaultTheme->getId());
-		
+
 		// Themes applied by media queries
 		foreach($mediaThemes as $theme) {
 			$this->addThemeHeader($theme->getId(), true, $theme->getMediaQuery());
 		}
 
-		// Themes 
+		// Themes
 		foreach($this->themesService->getThemes() as $theme) {
 			// Ignore default theme as already processed first
 			if ($theme->getId() === $this->defaultTheme->getId()) {
@@ -68,15 +81,24 @@ class ThemeInjectionService {
 
 	/**
 	 * Inject theme header into rendered page
-	 * 
+	 *
 	 * @param string $themeId the theme ID
 	 * @param bool $plain request the :root syntax
 	 * @param string $media media query to use in the <link> element
 	 */
 	private function addThemeHeader(string $themeId, bool $plain = true, string $media = null) {
+		$cacheBuster = $this->config->getAppValue('theming', 'cachebuster', '0');
+		if ($this->userId !== null) {
+			// need to bust the cache for the CSS file when the user background changed as its
+			// URL is served in those files
+			$userCacheBuster = $this->config->getUserValue($this->userId, Application::APP_ID, 'userCacheBuster', '0');
+			$cacheBuster .= $this->userId . '_' . $userCacheBuster;
+		}
+
 		$linkToCSS = $this->urlGenerator->linkToRoute('theming.Theming.getThemeStylesheet', [
 			'themeId' => $themeId,
 			'plain' => $plain,
+			'v' => substr(sha1($cacheBuster), 0, 8),
 		]);
 		Util::addHeader('link', [
 			'rel' => 'stylesheet',

apps/theming/lib/Themes/DefaultTheme.php

@@ -239,9 +239,11 @@ class DefaultTheme implements ITheme {
 		$user = $this->userSession->getUser();
 		if ($appManager->isEnabledForUser(Application::APP_ID) && $user !== null) {
 			$themingBackground = $this->config->getUserValue($user->getUID(), Application::APP_ID, 'background', 'default');
+			$currentVersion = (int)$this->config->getUserValue($user->getUID(), Application::APP_ID, 'userCacheBuster', '0');
 
 			if ($themingBackground === 'custom') {
-				$variables['--image-main-background'] = "url('" . $this->urlGenerator->linkToRouteAbsolute('theming.userTheme.getBackground') . "')";
+				$cacheBuster = substr(sha1($user->getUID() . '_' . $currentVersion), 0, 8);
+				$variables['--image-main-background'] = "url('" . $this->urlGenerator->linkToRouteAbsolute('theming.userTheme.getBackground') . "?v=$cacheBuster')";
 			} elseif (isset(BackgroundService::SHIPPED_BACKGROUNDS[$themingBackground])) {
 				$variables['--image-main-background'] = "url('" . $this->urlGenerator->linkTo(Application::APP_ID, "/img/background/$themingBackground") . "')";
 			} elseif (substr($themingBackground, 0, 1) === '#') {

apps/theming/src/UserThemes.vue

@@ -88,7 +88,6 @@ const enforceTheme = loadState('theming', 'enforceTheme', '')
 const shortcutsDisabled = loadState('theming', 'shortcutsDisabled', false)
 
 const background = loadState('theming', 'background')
-const backgroundVersion = loadState('theming', 'backgroundVersion')
 const themingDefaultBackground = loadState('theming', 'themingDefaultBackground')
 const shippedBackgroundList = loadState('theming', 'shippedBackgrounds')
 
@@ -109,7 +108,6 @@ export default {
 			enforceTheme,
 			shortcutsDisabled,
 			background,
-			backgroundVersion,
 			themingDefaultBackground,
 		}
 	},
@@ -169,10 +167,10 @@ export default {
 	methods: {
 		updateBackground(data) {
 			this.background = (data.type === 'custom' || data.type === 'default') ? data.type : data.value
-			this.backgroundVersion = data.version
 			this.updateGlobalStyles()
 			this.$emit('update:background')
 		},
+
 		updateGlobalStyles() {
 			// Override primary-invert-if-bright and color-primary-text if background is set
 			const isBackgroundBright = shippedBackgroundList[this.background]?.theming === 'dark'

apps/theming/tests/ImageManagerTest.php

@@ -56,6 +56,8 @@ class ImageManagerTest extends TestCase {
 	private $logger;
 	/** @var ITempManager|MockObject */
 	private $tempManager;
+	/** @var ISimpleFolder|MockObject */
+	private $rootFolder;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -65,6 +67,7 @@ class ImageManagerTest extends TestCase {
 		$this->cacheFactory = $this->createMock(ICacheFactory::class);
 		$this->logger = $this->createMock(ILogger::class);
 		$this->tempManager = $this->createMock(ITempManager::class);
+		$this->rootFolder = $this->createMock(ISimpleFolder::class);
 		$this->imageManager = new ImageManager(
 			$this->config,
 			$this->appData,
@@ -73,6 +76,11 @@ class ImageManagerTest extends TestCase {
 			$this->logger,
 			$this->tempManager
 		);
+		$this->appData
+			->expects($this->any())
+			->method('getFolder')
+			->with('global')
+			->willReturn($this->rootFolder);
 	}
 
 	private function checkImagick() {
@@ -120,7 +128,7 @@ class ImageManagerTest extends TestCase {
 				->willReturn($newFile);
 			$newFile->expects($this->once())
 				->method('putContent');
-			$this->appData->expects($this->once())
+			$this->rootFolder->expects($this->once())
 				->method('getFolder')
 				->with('images')
 				->willReturn($folder);
@@ -200,7 +208,7 @@ class ImageManagerTest extends TestCase {
 			->method('getAppValue')
 			->with('theming', 'cachebuster', '0')
 			->willReturn('0');
-		$this->appData->expects($this->once())
+		$this->rootFolder->expects($this->once())
 			->method('getFolder')
 			->with('0')
 			->willReturn($folder);
@@ -212,18 +220,18 @@ class ImageManagerTest extends TestCase {
 			->method('getAppValue')
 			->with('theming', 'cachebuster', '0')
 			->willReturn('0');
-		$this->appData->expects($this->exactly(2))
+		$this->rootFolder->expects($this->exactly(2))
 			->method('getFolder')
 			->with('0')
 			->willReturnOnConsecutiveCalls(
 				$this->throwException(new NotFoundException()),
 				$folder,
 			);
-		$this->appData->expects($this->once())
+		$this->rootFolder->expects($this->once())
 			->method('newFolder')
 			->with('0')
 			->willReturn($folder);
-		$this->appData->expects($this->once())
+		$this->rootFolder->expects($this->once())
 			->method('getDirectoryListing')
 			->willReturn([]);
 		$this->assertEquals($folder, $this->imageManager->getCacheFolder());
@@ -291,7 +299,7 @@ class ImageManagerTest extends TestCase {
 			->method('getAppValue')
 			->with('theming', 'cachebuster', '0')
 			->willReturn('0');
-		$this->appData->expects($this->once())
+		$this->rootFolder->expects($this->once())
 			->method('getFolder')
 			->with('0')
 			->willReturn($folder);
@@ -316,10 +324,10 @@ class ImageManagerTest extends TestCase {
 			->method('getAppValue')
 			->with('theming','cachebuster','0')
 			->willReturn('2');
-		$this->appData->expects($this->once())
+		$this->rootFolder->expects($this->once())
 			->method('getDirectoryListing')
 			->willReturn($folders);
-		$this->appData->expects($this->once())
+		$this->rootFolder->expects($this->once())
 			->method('getFolder')
 			->with('2')
 			->willReturn($folders[2]);
@@ -346,24 +354,26 @@ class ImageManagerTest extends TestCase {
 		$folder->expects($this->any())
 			->method('getFile')
 			->willReturn($oldFile);
+
 		if ($folderExists) {
-			$this->appData
+			$this->rootFolder
 				->expects($this->any())
 				->method('getFolder')
 				->with('images')
 				->willReturn($folder);
 		} else {
-			$this->appData
+			$this->rootFolder
 				->expects($this->any())
 				->method('getFolder')
 				->with('images')
 				->willThrowException(new NotFoundException());
-			$this->appData
+			$this->rootFolder
 				->expects($this->any())
 				->method('newFolder')
 				->with('images')
 				->willReturn($folder);
 		}
+
 		$folder->expects($this->once())
 			->method('newFile')
 			->with($key)

lib/private/Files/SimpleFS/SimpleFolder.php

@@ -91,4 +91,19 @@ class SimpleFolder implements ISimpleFolder {
 			return new SimpleFile($file);
 		}
 	}
+
+	public function getFolder(string $name): ISimpleFolder {
+		$folder = $this->folder->get($name);
+
+		if (!($folder instanceof Folder)) {
+			throw new NotFoundException();
+		}
+
+		return new SimpleFolder($folder);
+	}
+
+	public function newFolder(string $path): ISimpleFolder {
+		$folder = $this->folder->newFolder($path);
+		return new SimpleFolder($folder);
+	}
 }

lib/public/Files/SimpleFS/ISimpleFolder.php

@@ -80,4 +80,21 @@ interface ISimpleFolder {
 	 * @since 11.0.0
 	 */
 	public function getName(): string;
+
+	/**
+	 * Get the folder named $name from the current folder
+	 *
+	 * @throws NotFoundException
+	 * @since 25.0.0
+	 */
+	public function getFolder(string $name): ISimpleFolder;
+
+	/**
+	 * Creates a new folder with $name in the current folder
+	 *
+	 * @param string|resource|null $content @since 19.0.0
+	 * @throws NotPermittedException
+	 * @since 25.0.0
+	 */
+	public function newFolder(string $path): ISimpleFolder;
 }

tests/lib/Files/SimpleFS/SimpleFolderTest.php

@@ -28,6 +28,7 @@ use OC\Files\Storage\Temporary;
 use OCP\Files\Folder;
 use OCP\Files\NotFoundException;
 use OCP\Files\SimpleFS\ISimpleFile;
+use OCP\Files\SimpleFS\ISimpleFolder;
 use Test\Traits\MountProviderTrait;
 use Test\Traits\UserTrait;
 
@@ -109,4 +110,22 @@ class SimpleFolderTest extends \Test\TestCase {
 		$this->assertInstanceOf(ISimpleFile::class, $result[0]);
 		$this->assertInstanceOf(ISimpleFile::class, $result[1]);
 	}
+
+	public function testGetFolder() {
+		$this->folder->newFolder('exists');
+
+		$result = $this->simpleFolder->getFolder('exists');
+		$this->assertInstanceOf(ISimpleFolder::class, $result);
+
+		$this->expectException(NotFoundException::class);
+		$this->simpleFolder->getFolder('not-exists');
+	}
+
+	public function testNewFolder() {
+		$result = $this->simpleFolder->newFolder('folder');
+		$this->assertInstanceOf(ISimpleFolder::class, $result);
+		$result->newFile('file');
+
+		$this->assertTrue($this->folder->nodeExists('folder'));
+	}
 }