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

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

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;
 		}
-		$this->image->loadFromBase64(base64_encode($avatarImage));
-		$this->setOwnCloudAvatar();
+		if(!$this->image->loadFromBase64(base64_encode($avatarImage))) {
+			return false;
+		}
+		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, Util::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, Util::ERROR);
-			return;
+			return false;
 		}
 
 		if(!$this->fs->isLoaded()) {
@@ -590,11 +592,13 @@ class User {
 		try {
 			$avatar = $this->avatarManager->getAvatar($this->uid);
 			$avatar->set($this->image);
+			return true;
 		} catch (\Exception $e) {
 			\OC::$server->getLogger()->notice(
 				'Could not set avatar for ' . $this->dn	. ', because: ' . $e->getMessage(),
 				['app' => 'user_ldap']);
 		}
+		return false;
 	}
 
 	/**

apps/user_ldap/lib/User_LDAP.php

@@ -92,8 +92,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)) {
@@ -104,11 +106,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);
 	}
 
 	/**

apps/user_ldap/tests/User/UserTest.php

@@ -625,6 +625,9 @@ class UserTest extends \Test\TestCase {
 				$this->equalTo('jpegPhoto'))
 			->will($this->returnValue(array('this is a photo')));
 
+		$image->expects($this->once())
+			->method('loadFromBase64')
+			->willReturn('imageResource');
 		$image->expects($this->once())
 			->method('valid')
 			->will($this->returnValue(true));
@@ -680,6 +683,9 @@ class UserTest extends \Test\TestCase {
 				return null;
 			});
 
+		$image->expects($this->once())
+			->method('loadFromBase64')
+			->willReturn('imageResource');
 		$image->expects($this->once())
 			->method('valid')
 			->will($this->returnValue(true));
@@ -716,6 +722,115 @@ class UserTest extends \Test\TestCase {
 		$user->updateAvatar();
 	}
 
+	public function testUpdateAvatarCorruptPhotoProvided() {
+		list(, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr) =
+			$this->getTestInstances();
+
+		$this->access->expects($this->any())
+			->method('readAttribute')
+			->willReturnCallback(function($dn, $attr) {
+				if($dn === $dn
+					&& $attr === 'jpegPhoto')
+				{
+					return false;
+				} elseif($dn === $dn
+					&& $attr === 'thumbnailPhoto')
+				{
+					return ['this is a photo'];
+				}
+				return null;
+			});
+
+		$image->expects($this->once())
+			->method('loadFromBase64')
+			->willReturn(false);
+		$image->expects($this->never())
+			->method('valid');
+		$image->expects($this->never())
+			->method('width');
+		$image->expects($this->never())
+			->method('height');
+		$image->expects($this->never())
+			->method('centerCrop');
+
+		$filesys->expects($this->never())
+			->method('isLoaded');
+
+		$avatar = $this->createMock(IAvatar::class);
+		$avatar->expects($this->never())
+			->method('set');
+
+		$avaMgr->expects($this->never())
+			->method('getAvatar');
+
+		$uid = 'alice';
+		$dn  = 'uid=alice,dc=foo,dc=bar';
+
+		$user = new User(
+			$uid, $dn, $this->access, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr);
+
+		$user->updateAvatar();
+	}
+
+	public function testUpdateAvatarUnsupportedThumbnailPhotoProvided() {
+		list(, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr) =
+			$this->getTestInstances();
+
+		$uid = 'alice';
+		$dn  = 'uid=alice,dc=foo,dc=bar';
+
+		$this->access->expects($this->any())
+			->method('readAttribute')
+			->willReturnCallback(function($dn, $attr) {
+				if($dn === $dn
+					&& $attr === 'jpegPhoto')
+				{
+					return false;
+				} elseif($dn === $dn
+					&& $attr === 'thumbnailPhoto')
+				{
+					return ['this is a photo'];
+				}
+				return null;
+			});
+
+		$image->expects($this->once())
+			->method('loadFromBase64')
+			->willReturn('imageResource');
+		$image->expects($this->once())
+			->method('valid')
+			->will($this->returnValue(true));
+		$image->expects($this->once())
+			->method('width')
+			->will($this->returnValue(128));
+		$image->expects($this->once())
+			->method('height')
+			->will($this->returnValue(128));
+		$image->expects($this->once())
+			->method('centerCrop')
+			->will($this->returnValue(true));
+
+		$filesys->expects($this->once())
+			->method('isLoaded')
+			->will($this->returnValue(true));
+
+		$avatar = $this->createMock(IAvatar::class);
+		$avatar->expects($this->once())
+			->method('set')
+			->with($this->isInstanceOf($image))
+			->willThrowException(new \Exception());
+
+		$avaMgr->expects($this->once())
+			->method('getAvatar')
+			->with($this->equalTo($uid))
+			->will($this->returnValue($avatar));
+
+		$user = new User(
+			$uid, $dn, $this->access, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr);
+
+		$this->assertFalse($user->updateAvatar());
+	}
+
 	public function testUpdateAvatarNotProvided() {
 		list(, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr) =
 			$this->getTestInstances();
@@ -904,6 +1019,14 @@ class UserTest extends \Test\TestCase {
 		$photo = $user->getAvatarImage();
 	}
 
+	public function imageDataProvider() {
+		return [
+			[ false, false ],
+			[ 'corruptData', false ],
+			[ 'validData', true ],
+		];
+	}
+
 	public function testProcessAttributes() {
 		list(, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr) =
 			$this->getTestInstances();

apps/user_ldap/tests/User_LDAPTest.php

@@ -46,6 +46,7 @@ use OC\HintException;
 use OCA\User_LDAP\User\User;
 use OCA\User_LDAP\User_LDAP as UserLDAP;
 use OCA\User_LDAP\User_LDAP;
+use OCA\User_LDAP\UserPluginManager;
 use OCP\IAvatarManager;
 use OCP\IConfig;
 use OCP\IDBConnection;
@@ -1476,6 +1477,48 @@ class User_LDAPTest extends TestCase {
 		$this->assertEquals($ldap->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);
+
+		$access = $this->getAccessMock();
+		$access->userManager->expects($this->atLeastOnce())
+			->method('get')
+			->willReturn($user);
+
+		$config = $this->createMock(IConfig::class);
+		$noti = $this->createMock(INotificationManager::class);
+		$session = $this->createMock(Session::class);
+		$pluginManager = $this->createMock(UserPluginManager::class);
+
+		$ldap = new User_LDAP(
+			$access,
+			$config,
+			$noti,
+			$session,
+			$pluginManager
+		);
+
+		/** @noinspection PhpUnhandledExceptionInspection */
+		$this->assertSame($expected, $ldap->canChangeAvatar('uid'));
+	}
+
 	public function testCanChangeAvatarWithPlugin() {
 		$pluginManager = $this->getMockBuilder('\OCA\User_LDAP\UserPluginManager')
 			->setMethods(['implementsActions','canChangeAvatar'])