Fix own avatar not updated when display name is changed

The avatar endpoint returns the avatar image or, if the user has no
avatar, the display name. In that later case the avatar is generated on
the browser based on the display name. The avatar endpoint response is
cached, so when the display name changes and the avatar is got again the
browser could use the cached value, in which case it would use the same
display name as before and the avatar would not change.

When the avatar is an image the cache is invalidated with the use of
the "version" parameter, which is increased when the image changes. When
the avatar cache was first introduced only the image avatars were
cached, but it was later changed to cache all avatar responses to limit
the requests made to the server. Thus, now the cache of the display name
is invalidated too by increasing the version of the avatar if the
display name changes and there is no explicit avatar set.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

2 files changed, 135 insertions, 3 deletions

diff --git a/lib/private/AvatarManager.php b/lib/private/AvatarManager.php
index b8c6c2a1eb6..da3ac62097d 100644
--- a/lib/private/AvatarManager.php
+++ b/lib/private/AvatarManager.php
@@ -75,6 +75,12 @@ class AvatarManager implements IAvatarManager {
 		$this->l = $l;
 		$this->logger = $logger;
 		$this->config = $config;
+
+		$this->userManager->listen('\OC\User', 'changeUser', function ($user, $feature, $value, $oldValue) {
+			if ($feature === 'displayName') {
+				$this->updateAvatarVersionOnDisplayNameChange($user);
+			}
+		});
 	}
 
 	/**
@@ -102,4 +108,22 @@ class AvatarManager implements IAvatarManager {
 
 		return new Avatar($folder, $this->l, $user, $this->logger, $this->config);
 	}
+
+	/**
+	 * Increases the avatar version if needed.
+	 *
+	 * When a user has no avatar set the avatar is generated in the client based
+	 * on the display name, so in that case a display name change acts like an
+	 * avatar change and its version has to be increased.
+	 *
+	 * @param \OC\User\User $user the user whose display name has changed
+	 */
+	private function updateAvatarVersionOnDisplayNameChange(\OC\User\User $user) {
+		$avatar = $this->getAvatar($user->getUID());
+
+		if (!$avatar->exists()) {
+			$this->config->setUserValue($user->getUID(), 'avatar', 'version',
+				(int)$this->config->getUserValue($user->getUID(), 'avatar', 'version', 0) + 1);
+		}
+	}
 }
diff --git a/tests/lib/AvatarManagerTest.php b/tests/lib/AvatarManagerTest.php
index 83a5cfd9b30..df1367a28d3 100644
--- a/tests/lib/AvatarManagerTest.php
+++ b/tests/lib/AvatarManagerTest.php
@@ -27,19 +27,21 @@ namespace Test;
 use OC\Avatar;
 use OC\AvatarManager;
 use OC\Files\AppData\AppData;
+use OC\User\Manager as UserManager;
+use OC\User\User;
 use OCP\Files\IAppData;
 use OCP\Files\SimpleFS\ISimpleFolder;
+use OCP\IAvatar;
 use OCP\IConfig;
 use OCP\IL10N;
 use OCP\ILogger;
 use OCP\IUser;
-use OCP\IUserManager;
 
 /**
  * Class AvatarManagerTest
  */
 class AvatarManagerTest extends \Test\TestCase {
-	/** @var IUserManager|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var UserManager|\PHPUnit_Framework_MockObject_MockObject */
 	private $userManager;
 	/** @var IAppData|\PHPUnit_Framework_MockObject_MockObject */
 	private $appData;
@@ -49,18 +51,33 @@ class AvatarManagerTest extends \Test\TestCase {
 	private $logger;
 	/** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */
 	private $config;
+	/** @var callable **/
+	private $changeUserCallback;
 	/** @var AvatarManager | \PHPUnit_Framework_MockObject_MockObject */
 	private $avatarManager;
 
 	public function setUp() {
 		parent::setUp();
 
-		$this->userManager = $this->createMock(IUserManager::class);
+		$this->userManager = $this->createMock(UserManager::class);
 		$this->appData = $this->createMock(IAppData::class);
 		$this->l10n = $this->createMock(IL10N::class);
 		$this->logger = $this->createMock(ILogger::class);
 		$this->config = $this->createMock(IConfig::class);
 
+		$this->userManager
+			->expects($this->any())
+			->method('listen')
+			->with(
+				'\OC\User',
+				'changeUser',
+				$this->callback(function($changeUserCallback) {
+					$this->changeUserCallback = $changeUserCallback;
+
+					return true;
+				})
+			);
+
 		$this->avatarManager = new AvatarManager(
 			$this->userManager,
 			$this->appData,
@@ -127,4 +144,95 @@ class AvatarManagerTest extends \Test\TestCase {
 		$expected = new Avatar($folder, $this->l10n, $user, $this->logger, $this->config);
 		$this->assertEquals($expected, $this->avatarManager->getAvatar('vaLid-USER'));
 	}
+
+	public function testUpdateAvatarVersionOnDisplayNameChange() {
+		$this->avatarManager =
+			$this->getMockBuilder(AvatarManager::class)
+				->setMethods(['getAvatar'])
+				->setConstructorArgs([
+					$this->userManager,
+					$this->appData,
+					$this->l10n,
+					$this->logger,
+					$this->config
+				])->getMock();
+
+		$user = $this->createMock(User::class);
+
+		$user->expects($this->any())
+			->method('getUID')
+			->will($this->returnValue(42));
+
+		$avatar = $this->createMock(IAvatar::class);
+
+		$this->avatarManager
+			->expects($this->once())
+			->method('getAvatar')
+			->with(42)
+			->will($this->returnValue($avatar));
+
+		$avatar->expects($this->once())
+			->method('exists')
+			->will($this->returnValue(false));
+
+		$this->config
+			->expects($this->once())
+			->method('getUserValue')
+			->with(42, 'avatar', 'version', 0)
+			->will($this->returnValue(15));
+
+		$this->config
+			->expects($this->once())
+			->method('setUserValue')
+			->with(42, 'avatar', 'version', 16);
+
+		call_user_func($this->changeUserCallback, $user, 'displayName', 'newValue', 'oldValue');
+	}
+
+	public function testUpdateAvatarVersionOnDisplayNameChangeUserWithAvatar() {
+		$this->avatarManager =
+			$this->getMockBuilder(AvatarManager::class)
+				->setMethods(['getAvatar'])
+				->setConstructorArgs([
+					$this->userManager,
+					$this->appData,
+					$this->l10n,
+					$this->logger,
+					$this->config
+				])->getMock();
+
+		$user = $this->createMock(User::class);
+
+		$user->expects($this->any())
+			->method('getUID')
+			->will($this->returnValue(42));
+
+		$avatar = $this->createMock(IAvatar::class);
+
+		$this->avatarManager
+			->expects($this->once())
+			->method('getAvatar')
+			->with(42)
+			->will($this->returnValue($avatar));
+
+		$avatar->expects($this->once())
+			->method('exists')
+			->will($this->returnValue(true));
+
+		$this->config
+			->expects($this->never())
+			->method('setUserValue');
+
+		call_user_func($this->changeUserCallback, $user, 'displayName', 'newValue', 'oldValue');
+	}
+
+	public function testUpdateAvatarVersionOnDisplayNameChangeDifferentChangedAttribute() {
+		$user = $this->createMock(User::class);
+
+		$this->config
+			->expects($this->never())
+			->method('setUserValue');
+
+		call_user_func($this->changeUserCallback, $user, 'otherAttribute', 'newValue', 'oldValue');
+	}
 }