Skip to content

[stable13] let user set avatar in nextcloud if LDAP provides invalid image data #10089

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
blizzz merged 1 commit into from
Jul 4, 2018
Merged

[stable13] let user set avatar in nextcloud if LDAP provides invalid image data #10089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions apps/user_ldap/lib/User/User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,35 +552,37 @@ public function updateAvatarPostLogin($params) {

/**
* @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, 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()) {
Expand All @@ -590,11 +592,13 @@ private function setOwnCloudAvatar() {
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;
}

/**
Expand Down
8 changes: 5 additions & 3 deletions apps/user_ldap/lib/User_LDAP.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,10 @@ public function postDeleteUser() {

/**
* 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)) {
Expand All @@ -104,11 +106,11 @@ public function canChangeAvatar($uid) {
if(!$user instanceof User) {
return false;
}
if($user->getAvatarImage() === false) {
$imageData = $user->getAvatarImage();
if($imageData === false) {
return true;
}

return false;
return !$user->updateAvatar(true);
}

/**
Expand Down
123 changes: 123 additions & 0 deletions apps/user_ldap/tests/User/UserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,9 @@ public function testUpdateAvatarJpegPhotoProvided() {
$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));
Expand Down Expand Up @@ -680,6 +683,9 @@ public function testUpdateAvatarThumbnailPhotoProvided() {
return null;
});

$image->expects($this->once())
->method('loadFromBase64')
->willReturn('imageResource');
$image->expects($this->once())
->method('valid')
->will($this->returnValue(true));
Expand Down Expand Up @@ -716,6 +722,115 @@ public function testUpdateAvatarThumbnailPhotoProvided() {
$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();
Expand Down Expand Up @@ -904,6 +1019,14 @@ public function testGetAvatarImageProvided() {
$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();
Expand Down
43 changes: 43 additions & 0 deletions apps/user_ldap/tests/User_LDAPTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
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;
Expand Down Expand Up @@ -1476,6 +1477,48 @@ public function testSetPasswordWithPlugin() {
$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'])
Expand Down