From a4dda465c215042c84ee5954fec3a8f7203de5e2 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 3 Jul 2018 00:38:25 +0200 Subject: [PATCH] let user set avatar in nextcloud von LDAP provides invalid image data Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/User/User.php | 24 +++--- apps/user_ldap/lib/User_LDAP.php | 8 +- apps/user_ldap/tests/User/UserTest.php | 105 +++++++++++++++++++++++++ apps/user_ldap/tests/User_LDAPTest.php | 28 +++++++ 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 5dfeb6da54..f64c0b4b44 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; } - $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, 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 9c7d571179..1b0c07f0ca 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 3f3b33fbea..aa6498be08 100644 --- a/apps/user_ldap/tests/User/UserTest.php +++ b/apps/user_ldap/tests/User/UserTest.php @@ -506,6 +506,9 @@ class UserTest extends \Test\TestCase { $this->equalTo('jpegPhoto')) ->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)); @@ -552,6 +555,9 @@ class UserTest extends \Test\TestCase { return null; }); + $this->image->expects($this->once()) + ->method('loadFromBase64') + ->willReturn('imageResource'); $this->image->expects($this->once()) ->method('valid') ->will($this->returnValue(true)); @@ -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 8def34045e..927a7550f6 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')