let user set avatar in nextcloud von LDAP provides invalid image data

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

4 files changed, 152 insertions, 13 deletions

diff --git a/apps/user_ldap/lib/User/User.php b/apps/user_ldap/lib/User/User.php
index 5dfeb6da544..f64c0b4b447 100644
--- a/apps/user_ldap/lib/User/User.php
+++ b/apps/user_ldap/lib/User/User.php
@@ -552,35 +552,37 @@ class User {
 
 	/**
 	 * @brief attempts to get an image from LDAP and sets it as Nextcloud avatar
-	 * @return null
+	 * @return bool
 	 */
-	public function updateAvatar() {
-		if($this->wasRefreshed('avatar')) {
-			return;
+	public function updateAvatar($force = false) {
+		if(!$force && $this->wasRefreshed('avatar')) {
+			return false;
 		}
 		$avatarImage = $this->getAvatarImage();
 		if($avatarImage === false) {
 			//not set, nothing left to do;
-			return;
+			return false;
+		}
+		if(!$this->image->loadFromBase64(base64_encode($avatarImage))) {
+			return false;
 		}
-		$this->image->loadFromBase64(base64_encode($avatarImage));
-		$this->setOwnCloudAvatar();
+		return $this->setOwnCloudAvatar();
 	}
 
 	/**
 	 * @brief sets an image as Nextcloud avatar
-	 * @return null
+	 * @return bool
 	 */
 	private function setOwnCloudAvatar() {
 		if(!$this->image->valid()) {
 			$this->log->log('jpegPhoto data invalid for '.$this->dn, ILogger::ERROR);
-			return;
+			return false;
 		}
 		//make sure it is a square and not bigger than 128x128
 		$size = min(array($this->image->width(), $this->image->height(), 128));
 		if(!$this->image->centerCrop($size)) {
 			$this->log->log('croping image for avatar failed for '.$this->dn, ILogger::ERROR);
-			return;
+			return false;
 		}
 
 		if(!$this->fs->isLoaded()) {
@@ -590,6 +592,7 @@ class User {
 		try {
 			$avatar = $this->avatarManager->getAvatar($this->uid);
 			$avatar->set($this->image);
+			return true;
 		} catch (\Exception $e) {
 			\OC::$server->getLogger()->logException($e, [
 				'message' => 'Could not set avatar for ' . $this->dn,
@@ -597,6 +600,7 @@ class User {
 				'app' => 'user_ldap',
 			]);
 		}
+		return false;
 	}
 
 	/**
diff --git a/apps/user_ldap/lib/User_LDAP.php b/apps/user_ldap/lib/User_LDAP.php
index 9c7d5711794..1b0c07f0ca3 100644
--- a/apps/user_ldap/lib/User_LDAP.php
+++ b/apps/user_ldap/lib/User_LDAP.php
@@ -93,8 +93,10 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 
 	/**
 	 * checks whether the user is allowed to change his avatar in Nextcloud
+	 *
 	 * @param string $uid the Nextcloud user name
 	 * @return boolean either the user can or cannot
+	 * @throws \Exception
 	 */
 	public function canChangeAvatar($uid) {
 		if ($this->userPluginManager->implementsActions(Backend::PROVIDE_AVATAR)) {
@@ -105,11 +107,11 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 		if(!$user instanceof User) {
 			return false;
 		}
-		if($user->getAvatarImage() === false) {
+		$imageData = $user->getAvatarImage();
+		if($imageData === false) {
 			return true;
 		}
-
-		return false;
+		return !$user->updateAvatar(true);
 	}
 
 	/**
diff --git a/apps/user_ldap/tests/User/UserTest.php b/apps/user_ldap/tests/User/UserTest.php
index 3f3b33fbead..aa6498be082 100644
--- a/apps/user_ldap/tests/User/UserTest.php
+++ b/apps/user_ldap/tests/User/UserTest.php
@@ -507,6 +507,9 @@ class UserTest extends \Test\TestCase {
 			->will($this->returnValue(['this is a photo']));
 
 		$this->image->expects($this->once())
+			->method('loadFromBase64')
+			->willReturn('imageResource');
+		$this->image->expects($this->once())
 			->method('valid')
 			->will($this->returnValue(true));
 		$this->image->expects($this->once())
@@ -553,6 +556,9 @@ class UserTest extends \Test\TestCase {
 			});
 
 		$this->image->expects($this->once())
+			->method('loadFromBase64')
+			->willReturn('imageResource');
+		$this->image->expects($this->once())
 			->method('valid')
 			->will($this->returnValue(true));
 		$this->image->expects($this->once())
@@ -582,6 +588,97 @@ class UserTest extends \Test\TestCase {
 		$this->user->updateAvatar();
 	}
 
+	public function testUpdateAvatarCorruptPhotoProvided() {
+		$this->access->expects($this->any())
+			->method('readAttribute')
+			->willReturnCallback(function($dn, $attr) {
+				if($dn === $this->dn
+					&& $attr === 'jpegPhoto')
+				{
+					return false;
+				} elseif($dn === $this->dn
+					&& $attr === 'thumbnailPhoto')
+				{
+					return ['this is a photo'];
+				}
+				return null;
+			});
+
+		$this->image->expects($this->once())
+			->method('loadFromBase64')
+			->willReturn(false);
+		$this->image->expects($this->never())
+			->method('valid');
+		$this->image->expects($this->never())
+			->method('width');
+		$this->image->expects($this->never())
+			->method('height');
+		$this->image->expects($this->never())
+			->method('centerCrop');
+
+		$this->filesystemhelper->expects($this->never())
+			->method('isLoaded');
+
+		$avatar = $this->createMock(IAvatar::class);
+		$avatar->expects($this->never())
+			->method('set');
+
+		$this->avatarManager->expects($this->never())
+			->method('getAvatar');
+
+		$this->user->updateAvatar();
+	}
+
+	public function testUpdateAvatarUnsupportedThumbnailPhotoProvided() {
+		$this->access->expects($this->any())
+			->method('readAttribute')
+			->willReturnCallback(function($dn, $attr) {
+				if($dn === $this->dn
+					&& $attr === 'jpegPhoto')
+				{
+					return false;
+				} elseif($dn === $this->dn
+					&& $attr === 'thumbnailPhoto')
+				{
+					return ['this is a photo'];
+				}
+				return null;
+			});
+
+		$this->image->expects($this->once())
+			->method('loadFromBase64')
+			->willReturn('imageResource');
+		$this->image->expects($this->once())
+			->method('valid')
+			->will($this->returnValue(true));
+		$this->image->expects($this->once())
+			->method('width')
+			->will($this->returnValue(128));
+		$this->image->expects($this->once())
+			->method('height')
+			->will($this->returnValue(128));
+		$this->image->expects($this->once())
+			->method('centerCrop')
+			->will($this->returnValue(true));
+
+		$this->filesystemhelper->expects($this->once())
+			->method('isLoaded')
+			->will($this->returnValue(true));
+
+		$avatar = $this->createMock(IAvatar::class);
+		$avatar->expects($this->once())
+			->method('set')
+			->with($this->isInstanceOf($this->image))
+			->willThrowException(new \Exception());
+
+		$this->avatarManager->expects($this->once())
+			->method('getAvatar')
+			->with($this->equalTo($this->uid))
+			->will($this->returnValue($avatar));
+
+		$this->assertFalse($this->user->updateAvatar());
+	}
+
 	public function testUpdateAvatarNotProvided() {
 		$this->access->expects($this->any())
 			->method('readAttribute')
@@ -715,6 +812,14 @@ class UserTest extends \Test\TestCase {
 		$this->user->getAvatarImage();
 	}
 
+	public function imageDataProvider() {
+		return [
+			[ false, false ],
+			[ 'corruptData', false ],
+			[ 'validData', true ],
+		];
+	}
+
 	public function testProcessAttributes() {
 		$requiredMethods = array(
 			'markRefreshTime',
diff --git a/apps/user_ldap/tests/User_LDAPTest.php b/apps/user_ldap/tests/User_LDAPTest.php
index 8def34045ec..927a7550f60 100644
--- a/apps/user_ldap/tests/User_LDAPTest.php
+++ b/apps/user_ldap/tests/User_LDAPTest.php
@@ -1312,6 +1312,34 @@ class User_LDAPTest extends TestCase {
 		$this->assertEquals($this->backend->setPassword('uid', 'password'),'result');
 	}
 
+	public function avatarDataProvider() {
+		return [
+			[ 'validImageData', false ],
+			[ 'corruptImageData', true ],
+			[ false, true]
+		];
+	}
+
+	/** @dataProvider avatarDataProvider */
+	public function testCanChangeAvatar($imageData, $expected) {
+		$isValidImage  = strpos((string)$imageData, 'valid') === 0;
+
+		$user = $this->createMock(User::class);
+		$user->expects($this->once())
+			->method('getAvatarImage')
+			->willReturn($imageData);
+		$user->expects($this->atMost(1))
+			->method('updateAvatar')
+			->willReturn($isValidImage);
+
+		$this->userManager->expects($this->atLeastOnce())
+			->method('get')
+			->willReturn($user);
+
+		/** @noinspection PhpUnhandledExceptionInspection */
+		$this->assertSame($expected, $this->backend->canChangeAvatar('uid'));
+	}
+
 	public function testCanChangeAvatarWithPlugin() {
 		$this->pluginManager->expects($this->once())
 			->method('implementsActions')