Merge pull request #5292 from nextcloud/fix/move-userid-injection-to-controller

Move userId to controller to fix tagging despite option disabled

14 files changed, 84 insertions, 54 deletions

diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
index 43a447537..65c7e68d2 100644
--- a/lib/AppInfo/Application.php
+++ b/lib/AppInfo/Application.php
@@ -61,7 +61,7 @@ use OCA\Mail\Service\MailManager;
 use OCA\Mail\Service\MailTransmission;
 use OCA\Mail\Service\Search\MailSearch;
 use OCA\Mail\Service\TrustedSenderService;
-use OCA\Mail\Service\UserPreferenceSevice;
+use OCA\Mail\Service\UserPreferenceService;
 use OCP\AppFramework\App;
 use OCP\AppFramework\Bootstrap\IBootContext;
 use OCP\AppFramework\Bootstrap\IBootstrap;
@@ -96,7 +96,7 @@ class Application extends App implements IBootstrap {
 		$context->registerServiceAlias(IMailSearch::class, MailSearch::class);
 		$context->registerServiceAlias(IMailTransmission::class, MailTransmission::class);
 		$context->registerServiceAlias(ITrustedSenderService::class, TrustedSenderService::class);
-		$context->registerServiceAlias(IUserPreferences::class, UserPreferenceSevice::class);
+		$context->registerServiceAlias(IUserPreferences::class, UserPreferenceService::class);
 
 		$context->registerEventListener(DraftSavedEvent::class, DeleteDraftListener::class);
 		$context->registerEventListener(MailboxesSynchronizedEvent::class, MailboxesSynchronizedSpecialMailboxesUpdater::class);
diff --git a/lib/Contracts/IUserPreferences.php b/lib/Contracts/IUserPreferences.php
index efaed8e45..765cae347 100644
--- a/lib/Contracts/IUserPreferences.php
+++ b/lib/Contracts/IUserPreferences.php
@@ -29,15 +29,17 @@ namespace OCA\Mail\Contracts;
 interface IUserPreferences {
 
 	/**
+	 * @param string $userId
 	 * @param string $key
 	 * @param mixed $value
 	 * @return mixed new value
 	 */
-	public function setPreference($key, $value);
+	public function setPreference(string $userId, $key, $value);
 
 	/**
+	 * @param string $userId
 	 * @param string $key
 	 * @param mixed|null $default
 	 */
-	public function getPreference($key, $default = null);
+	public function getPreference(string $userId, $key, $default = null);
 }
diff --git a/lib/Controller/PageController.php b/lib/Controller/PageController.php
index 554ab07ef..a337254e4 100644
--- a/lib/Controller/PageController.php
+++ b/lib/Controller/PageController.php
@@ -143,7 +143,7 @@ class PageController extends Controller {
 		);
 		$this->initialStateService->provideInitialState(
 			'account-settings',
-			json_decode($this->preferences->getPreference('account-settings', '[]'), true, 512, JSON_THROW_ON_ERROR) ?? []
+			json_decode($this->preferences->getPreference($this->currentUserId, 'account-settings', '[]'), true, 512, JSON_THROW_ON_ERROR) ?? []
 		);
 		$this->initialStateService->provideInitialState(
 			'tags',
@@ -155,10 +155,10 @@ class PageController extends Controller {
 			[
 				'attachment-size-limit' => $this->config->getSystemValue('app.mail.attachment-size-limit', 0),
 				'app-version' => $this->config->getAppValue('mail', 'installed_version'),
-				'external-avatars' => $this->preferences->getPreference('external-avatars', 'true'),
-				'reply-mode' => $this->preferences->getPreference('reply-mode', 'top'),
-				'collect-data' => $this->preferences->getPreference('collect-data', 'true'),
-				'tag-classified-messages' => $this->preferences->getPreference('tag-classified-messages', 'true'),
+				'external-avatars' => $this->preferences->getPreference($this->currentUserId, 'external-avatars', 'true'),
+				'reply-mode' => $this->preferences->getPreference($this->currentUserId, 'reply-mode', 'top'),
+				'collect-data' => $this->preferences->getPreference($this->currentUserId, 'collect-data', 'true'),
+				'tag-classified-messages' => $this->preferences->getPreference($this->currentUserId, 'tag-classified-messages', 'true'),
 			]);
 		$this->initialStateService->provideInitialState(
 			'prefill_displayName',
diff --git a/lib/Controller/PreferencesController.php b/lib/Controller/PreferencesController.php
index 201b8be38..2d0e7d3cd 100644
--- a/lib/Controller/PreferencesController.php
+++ b/lib/Controller/PreferencesController.php
@@ -37,14 +37,19 @@ class PreferencesController extends Controller {
 	/** @var IUserPreferences */
 	private $userPreference;
 
+	/** @var string */
+	private $userId;
+
 	/**
 	 * @param IRequest $request
 	 * @param IUserPreferences $userPreference
+	 * @param string $UserId
 	 */
-	public function __construct(IRequest $request, IUserPreferences $userPreference) {
+	public function __construct(IRequest $request, IUserPreferences $userPreference, string $UserId) {
 		parent::__construct('mail', $request);
 
 		$this->userPreference = $userPreference;
+		$this->userId = $UserId;
 	}
 
 	/**
@@ -56,7 +61,7 @@ class PreferencesController extends Controller {
 	 */
 	public function show(string $id): JSONResponse {
 		return new JSONResponse([
-			'value' => $this->userPreference->getPreference($id)
+			'value' => $this->userPreference->getPreference($this->userId, $id)
 		]);
 	}
 
@@ -74,7 +79,8 @@ class PreferencesController extends Controller {
 			throw new ClientException('key or value missing');
 		}
 
-		$newValue = $this->userPreference->setPreference($key, $value);
+
+		$newValue = $this->userPreference->setPreference($this->userId, $key, $value);
 
 		return new JSONResponse([
 			'value' => $newValue,
diff --git a/lib/Listener/AddressCollectionListener.php b/lib/Listener/AddressCollectionListener.php
index 9486c8ded..2b430aacd 100644
--- a/lib/Listener/AddressCollectionListener.php
+++ b/lib/Listener/AddressCollectionListener.php
@@ -56,7 +56,7 @@ class AddressCollectionListener implements IEventListener {
 		if (!($event instanceof MessageSentEvent)) {
 			return;
 		}
-		if ($this->preferences->getPreference('collect-data', 'true') !== 'true') {
+		if ($this->preferences->getPreference($event->getAccount()->getUserId(), 'collect-data', 'true') !== 'true') {
 			$this->logger->debug('Not collecting email addresses because the user opted out');
 			return;
 		}
diff --git a/lib/Listener/NewMessageClassificationListener.php b/lib/Listener/NewMessageClassificationListener.php
index d370d9b9d..501fb3a6e 100644
--- a/lib/Listener/NewMessageClassificationListener.php
+++ b/lib/Listener/NewMessageClassificationListener.php
@@ -79,8 +79,8 @@ class NewMessageClassificationListener implements IEventListener {
 			return;
 		}
 
-		$allowTagging = $this->preferences->getPreference('tag-classified-messages');
-		if ($allowTagging === "false") {
+		$allowTagging = $this->preferences->getPreference($event->getAccount()->getUserId(), 'tag-classified-messages');
+		if ($allowTagging === 'false') {
 			return;
 		}
 
diff --git a/lib/Service/AvatarService.php b/lib/Service/AvatarService.php
index 71473ae24..03e9a2c8b 100644
--- a/lib/Service/AvatarService.php
+++ b/lib/Service/AvatarService.php
@@ -77,8 +77,8 @@ class AvatarService implements IAvatarService {
 	/**
 	 * @return bool
 	 */
-	private function externalAvatarsAllowed(): bool {
-		return $this->preferences->getPreference('external-avatars', 'true') === 'true';
+	private function externalAvatarsAllowed(string $uid): bool {
+		return $this->preferences->getPreference($uid, 'external-avatars', 'true') === 'true';
 	}
 
 	/**
@@ -115,7 +115,7 @@ class AvatarService implements IAvatarService {
 			return null;
 		}
 
-		$avatar = $this->source->fetch($email, $this->avatarFactory, $this->externalAvatarsAllowed());
+		$avatar = $this->source->fetch($email, $this->avatarFactory, $this->externalAvatarsAllowed($uid));
 		if (is_null($avatar) || !$this->hasAllowedMime($avatar)) {
 			// Cannot locate any avatar -> nothing to do here
 
diff --git a/lib/Service/UserPreferenceSevice.php b/lib/Service/UserPreferenceService.php
index aff05ace0..192435546 100644
--- a/lib/Service/UserPreferenceSevice.php
+++ b/lib/Service/UserPreferenceService.php
@@ -29,38 +29,36 @@ namespace OCA\Mail\Service;
 use OCA\Mail\Contracts\IUserPreferences;
 use OCP\IConfig;
 
-class UserPreferenceSevice implements IUserPreferences {
+class UserPreferenceService implements IUserPreferences {
 
 	/** @var IConfig */
 	private $config;
 
-	/** @var string */
-	private $UserId;
-
 	/**
 	 * @param IConfig $config
-	 * @param string $UserId
 	 */
-	public function __construct(IConfig $config, $UserId) {
+	public function __construct(IConfig $config) {
 		$this->config = $config;
-		$this->UserId = $UserId;
 	}
 
 	/**
+	 * @param string $userId
 	 * @param string $key
 	 * @param mixed $value
 	 * @return mixed new value
 	 */
-	public function setPreference($key, $value) {
-		$this->config->setUserValue($this->UserId, 'mail', $key, $value);
+	public function setPreference(string $userId, $key, $value) {
+		$this->config->setUserValue($userId, 'mail', $key, $value);
 		return $value;
 	}
 
 	/**
+	 * @param string $userId
 	 * @param string $key
 	 * @param mixed|null $default
+	 * @return string
 	 */
-	public function getPreference($key, $default = null) {
-		return $this->config->getUserValue($this->UserId, 'mail', $key, $default);
+	public function getPreference(string $userId, $key, $default = null) {
+		return $this->config->getUserValue($userId, 'mail', $key, $default);
 	}
 }
diff --git a/tests/Integration/Service/AvatarServiceIntegrationTest.php b/tests/Integration/Service/AvatarServiceIntegrationTest.php
index 93d042712..ee8fe4b58 100644
--- a/tests/Integration/Service/AvatarServiceIntegrationTest.php
+++ b/tests/Integration/Service/AvatarServiceIntegrationTest.php
@@ -24,6 +24,7 @@
 
 namespace OCA\Mail\Tests\Integration\Service;
 
+use ChristophWurst\Nextcloud\Testing\TestUser;
 use OC;
 use OCA\Mail\Contracts\IAvatarService;
 use ChristophWurst\Nextcloud\Testing\TestCase;
@@ -31,10 +32,12 @@ use OCP\ICache;
 use OCP\ICacheFactory;
 
 class AvatarServiceIntegrationTest extends TestCase {
+	use TestUser;
 
 	/** @var IAvatarService */
 	private $service;
 
+
 	private function clearCache() {
 		/* @var $cacheFactory ICacheFactory */
 		$cacheFactory = OC::$server->query(ICacheFactory::class);
diff --git a/tests/Unit/Controller/PageControllerTest.php b/tests/Unit/Controller/PageControllerTest.php
index 0d9b95015..ea075c71b 100644
--- a/tests/Unit/Controller/PageControllerTest.php
+++ b/tests/Unit/Controller/PageControllerTest.php
@@ -129,11 +129,11 @@ class PageControllerTest extends TestCase {
 		$this->preferences->expects($this->exactly(5))
 			->method('getPreference')
 			->willReturnMap([
-				['account-settings', '[]', json_encode([])],
-				['external-avatars', 'true', 'true'],
-				['reply-mode', 'top', 'bottom'],
-				['collect-data', 'true', 'true'],
-				['tag-classified-messages', 'true', 'true'],
+				[$this->userId, 'account-settings', '[]', json_encode([])],
+				[$this->userId, 'external-avatars', 'true', 'true'],
+				[$this->userId, 'reply-mode', 'top', 'bottom'],
+				[$this->userId, 'collect-data', 'true', 'true'],
+				[$this->userId, 'tag-classified-messages', 'true', 'true'],
 			]);
 		$this->accountService->expects($this->once())
 			->method('findByUserId')
diff --git a/tests/Unit/Controller/PreferencesControllerTest.php b/tests/Unit/Controller/PreferencesControllerTest.php
index ec60ffa91..97aa1ac22 100644
--- a/tests/Unit/Controller/PreferencesControllerTest.php
+++ b/tests/Unit/Controller/PreferencesControllerTest.php
@@ -45,13 +45,13 @@ class PreferencesControllerTest extends TestCase {
 		$request = $this->createMock(IRequest::class);
 		$this->preferences = $this->createMock(IUserPreferences::class);
 
-		$this->controller = new PreferencesController($request, $this->preferences);
+		$this->controller = new PreferencesController($request, $this->preferences, 'george');
 	}
 
 	public function testGetPreference() {
 		$this->preferences->expects($this->once())
 			->method('getPreference')
-			->with('test')
+			->with('george', 'test')
 			->willReturn(123);
 		$expected = new JSONResponse(['value' => 123]);
 
@@ -63,8 +63,8 @@ class PreferencesControllerTest extends TestCase {
 	public function testSetPreference() {
 		$this->preferences->expects($this->once())
 			->method('setPreference')
-			->with('test')
-			->willReturnArgument(1);
+			->with('george', 'test')
+			->willReturnArgument(2);
 		$expected = new JSONResponse([
 			'value' => 123,
 		]);
diff --git a/tests/Unit/Listener/AddressCollectionListenerTest.php b/tests/Unit/Listener/AddressCollectionListenerTest.php
index df3814fc0..c3c4f34d9 100644
--- a/tests/Unit/Listener/AddressCollectionListenerTest.php
+++ b/tests/Unit/Listener/AddressCollectionListenerTest.php
@@ -82,10 +82,15 @@ class AddressCollectionListenerTest extends TestCase {
 	}
 
 	public function testHandleOptOut() {
-		$event = $this->createMock(MessageSentEvent::class);
+		$account = $this->createConfiguredMock(Account::class, [
+			'getUserId' => 'test'
+		]);
+		$event = $this->createConfiguredMock(MessageSentEvent::class, [
+			'getAccount' => $account
+		]);
 		$this->preferences->expects($this->once())
 			->method('getPreference')
-			->with('collect-data', 'true')
+			->with('test', 'collect-data', 'true')
 			->willReturn('false');
 		$this->addressCollector->expects($this->never())
 			->method('addAddresses');
@@ -95,7 +100,9 @@ class AddressCollectionListenerTest extends TestCase {
 
 	public function testHandle() {
 		/** @var Account|MockObject $account */
-		$account = $this->createMock(Account::class);
+		$account = $this->createConfiguredMock(Account::class, [
+			'getUserId' => 'test'
+		]);
 		/** @var NewMessageData|MockObject $newMessageData */
 		$newMessageData = $this->createMock(NewMessageData::class);
 		/** @var RepliedMessageData|MockObject $repliedMessageData */
@@ -104,7 +111,7 @@ class AddressCollectionListenerTest extends TestCase {
 		$message = $this->createMock(IMessage::class);
 		$this->preferences->expects($this->once())
 			->method('getPreference')
-			->with('collect-data', 'true')
+			->with('test','collect-data', 'true')
 			->willReturn('true');
 		/** @var Horde_Mime_Mail|MockObject $mail */
 		$mail = $this->createMock(Horde_Mime_Mail::class);
diff --git a/tests/Unit/Service/AvatarServiceTest.php b/tests/Unit/Service/AvatarServiceTest.php
index 4d0c5162d..9c1551acc 100644
--- a/tests/Unit/Service/AvatarServiceTest.php
+++ b/tests/Unit/Service/AvatarServiceTest.php
@@ -36,6 +36,7 @@ use OCA\Mail\Service\Avatar\IAvatarSource;
 use OCA\Mail\Service\AvatarService;
 use ChristophWurst\Nextcloud\Testing\TestCase;
 use OCP\IURLGenerator;
+use OCP\IUser;
 use PHPUnit\Framework\MockObject\MockObject;
 
 class AvatarServiceTest extends TestCase {
@@ -62,6 +63,9 @@ class AvatarServiceTest extends TestCase {
 	/** @var AvatarService */
 	private $avatarService;
 
+	/** @var IUser|MockObject */
+	private $user;
+
 	protected function setUp(): void {
 		parent::setUp();
 
@@ -71,9 +75,19 @@ class AvatarServiceTest extends TestCase {
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
 		$this->avatarFactory = $this->createMock(AvatarFactory::class);
 		$this->preferences = $this->createMock(IUserPreferences::class);
-
-		$this->avatarService = new AvatarService($this->source, $this->downloader,
-			$this->cache, $this->urlGenerator, $this->avatarFactory, $this->preferences);
+		$this->user = $this->createConfiguredMock(IUser::class, [
+			'getUID' => 'test'
+		]);
+
+		$this->avatarService = new AvatarService(
+			$this->source,
+			$this->downloader,
+			$this->cache,
+			$this->urlGenerator,
+			$this->avatarFactory,
+			$this->preferences,
+			$this->user
+		);
 	}
 
 	public function testGetCachedAvatarUrl() {
@@ -95,7 +109,7 @@ class AvatarServiceTest extends TestCase {
 		$uid = 'john';
 		$this->preferences->expects($this->once())
 			->method('getPreference')
-			->with('external-avatars', 'true')
+			->with($uid, 'external-avatars', 'true')
 			->willReturn('true');
 		$this->cache->expects($this->once())
 			->method('get')
@@ -119,7 +133,7 @@ class AvatarServiceTest extends TestCase {
 		$uid = 'john';
 		$this->preferences->expects($this->once())
 			->method('getPreference')
-			->with('external-avatars', 'true')
+			->with($uid, 'external-avatars', 'true')
 			->willReturn('true');
 		$this->cache->expects($this->once())
 			->method('get')
@@ -145,7 +159,7 @@ class AvatarServiceTest extends TestCase {
 		$avatar = new Avatar('https://doe.com/favicon.ico', 'image/png');
 		$this->preferences->expects($this->once())
 			->method('getPreference')
-			->with('external-avatars', 'true')
+			->with($uid, 'external-avatars', 'true')
 			->willReturn('false');
 		$this->cache->expects($this->once())
 			->method('get')
@@ -170,7 +184,7 @@ class AvatarServiceTest extends TestCase {
 		$avatar = new Avatar('https://doe.com/favicon.ico', 'image/png');
 		$this->preferences->expects($this->once())
 			->method('getPreference')
-			->with('external-avatars', 'true')
+			->with($uid, 'external-avatars', 'true')
 			->willReturn('true');
 		$this->cache->expects($this->once())
 			->method('get')
diff --git a/tests/Unit/Service/UserPreferenceServiceTest.php b/tests/Unit/Service/UserPreferenceServiceTest.php
index 1b1d6434b..d2d5f1255 100644
--- a/tests/Unit/Service/UserPreferenceServiceTest.php
+++ b/tests/Unit/Service/UserPreferenceServiceTest.php
@@ -24,7 +24,7 @@
 
 namespace OCA\Mail\Tests\Unit\Service;
 
-use OCA\Mail\Service\UserPreferenceSevice;
+use OCA\Mail\Service\UserPreferenceService;
 use ChristophWurst\Nextcloud\Testing\TestCase;
 use OCP\IConfig;
 
@@ -36,14 +36,14 @@ class UserPreferenceServiceTest extends TestCase {
 	/** @var string */
 	private $userId = 'claire';
 
-	/** @var UserPreferenceSevice */
+	/** @var UserPreferenceService */
 	private $service;
 
 	protected function setUp(): void {
 		parent::setUp();
 
 		$this->config = $this->createMock(IConfig::class);
-		$this->service = new UserPreferenceSevice($this->config, $this->userId);
+		$this->service = new UserPreferenceService($this->config, $this->userId);
 	}
 
 	public function testGetPreference() {
@@ -53,7 +53,7 @@ class UserPreferenceServiceTest extends TestCase {
 			->willReturn('123');
 		$expected = '123';
 
-		$actual = $this->service->getPreference('test');
+		$actual = $this->service->getPreference($this->userId, 'test');
 
 		$this->assertEquals($expected, $actual);
 	}
@@ -64,6 +64,6 @@ class UserPreferenceServiceTest extends TestCase {
 			->with($this->userId, 'mail', 'test', '123')
 			->willReturn('123');
 
-		$this->service->setPreference('test', '123');
+		$this->service->setPreference($this->userId, 'test', '123');
 	}
 }