From 343036e55c0b41891fc86aafc0cbb3077503ab64 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 4 Jul 2018 00:10:43 +0200 Subject: [PATCH 1/2] allow admin to disable fetching of avatars as well as a specific attribute Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Configuration.php | 40 ++++++++++++++++++++ apps/user_ldap/lib/Connection.php | 10 +++++ apps/user_ldap/lib/User/Manager.php | 9 +++-- apps/user_ldap/lib/User/User.php | 16 +++++--- apps/user_ldap/lib/User_LDAP.php | 6 ++- apps/user_ldap/tests/ConfigurationTest.php | 44 ++++++++++++++++++++-- apps/user_ldap/tests/User/ManagerTest.php | 34 ++++++++--------- 7 files changed, 126 insertions(+), 33 deletions(-) diff --git a/apps/user_ldap/lib/Configuration.php b/apps/user_ldap/lib/Configuration.php index 6240e8d8f5..3871003fae 100644 --- a/apps/user_ldap/lib/Configuration.php +++ b/apps/user_ldap/lib/Configuration.php @@ -35,8 +35,13 @@ namespace OCA\User_LDAP; /** * @property int ldapPagingSize holds an integer + * @property string ldapUserAvatarRule */ class Configuration { + const AVATAR_PREFIX_DEFAULT = 'default'; + const AVATAR_PREFIX_NONE = 'none'; + const AVATAR_PREFIX_DATA_ATTRIBUTE = 'data:'; + protected $configPrefix = null; protected $configRead = false; /** @@ -61,6 +66,7 @@ class Configuration { 'ldapIgnoreNamingRules' => null, 'ldapUserDisplayName' => null, 'ldapUserDisplayName2' => null, + 'ldapUserAvatarRule' => null, 'ldapGidNumber' => null, 'ldapUserFilterObjectclass' => null, 'ldapUserFilterGroups' => null, @@ -472,6 +478,7 @@ class Configuration { 'ldap_experienced_admin' => 0, 'ldap_dynamic_group_member_url' => '', 'ldap_default_ppolicy_dn' => '', + 'ldap_user_avatar_rule' => 'default', ); } @@ -495,6 +502,7 @@ class Configuration { 'ldap_userfilter_groups' => 'ldapUserFilterGroups', 'ldap_userlist_filter' => 'ldapUserFilter', 'ldap_user_filter_mode' => 'ldapUserFilterMode', + 'ldap_user_avatar_rule' => 'ldapUserAvatarRule', 'ldap_login_filter' => 'ldapLoginFilter', 'ldap_login_filter_mode' => 'ldapLoginFilterMode', 'ldap_loginfilter_email' => 'ldapLoginFilterEmail', @@ -536,4 +544,36 @@ class Configuration { return $array; } + /** + * @param string $rule + * @return array + * @throws \RuntimeException + */ + public function resolveRule($rule) { + if($rule === 'avatar') { + return $this->getAvatarAttributes(); + } + throw new \RuntimeException('Invalid rule'); + } + + public function getAvatarAttributes() { + $value = $this->ldapUserAvatarRule ?: self::AVATAR_PREFIX_DEFAULT; + $defaultAttributes = ['jpegphoto', 'thumbnailphoto']; + + if($value === self::AVATAR_PREFIX_NONE) { + return []; + } + if(strpos($value, self::AVATAR_PREFIX_DATA_ATTRIBUTE) === 0) { + $attribute = trim(substr($value, strlen(self::AVATAR_PREFIX_DATA_ATTRIBUTE))); + if($attribute === '') { + return $defaultAttributes; + } + return [$attribute]; + } + if($value !== self::AVATAR_PREFIX_DEFAULT) { + \OC::$server->getLogger()->warning('Invalid config value to ldapUserAvatarRule; falling back to default.'); + } + return $defaultAttributes; + } + } diff --git a/apps/user_ldap/lib/Connection.php b/apps/user_ldap/lib/Connection.php index 977b7c5442..85e6ad6fd9 100644 --- a/apps/user_ldap/lib/Connection.php +++ b/apps/user_ldap/lib/Connection.php @@ -48,6 +48,7 @@ use OCP\ILogger; * @property string ldapUserFilter * @property string ldapUserDisplayName * @property string ldapUserDisplayName2 + * @property string ldapUserAvatarRule * @property boolean turnOnPasswordChange * @property boolean hasPagedResultSupport * @property string[] ldapBaseUsers @@ -169,6 +170,15 @@ class Connection extends LDAPUtility { } } + /** + * @param string $rule + * @return array + * @throws \RuntimeException + */ + public function resolveRule($rule) { + return $this->configuration->resolveRule($rule); + } + /** * sets whether the result of the configuration validation shall * be ignored when establishing the connection. Used by the Wizard diff --git a/apps/user_ldap/lib/User/Manager.php b/apps/user_ldap/lib/User/Manager.php index 55fc7499be..c48193c7ad 100644 --- a/apps/user_ldap/lib/User/Manager.php +++ b/apps/user_ldap/lib/User/Manager.php @@ -163,6 +163,7 @@ class Manager { /** * returns a list of attributes that will be processed further, e.g. quota, * email, displayname, or others. + * * @param bool $minimal - optional, set to true to skip attributes with big * payload * @return string[] @@ -190,10 +191,10 @@ class Manager { if(!$minimal) { // attributes that are not really important but may come with big // payload. - $attributes = array_merge($attributes, array( - 'jpegphoto', - 'thumbnailphoto' - )); + $attributes = array_merge( + $attributes, + $this->access->getConnection()->resolveRule('avatar') + ); } return $attributes; diff --git a/apps/user_ldap/lib/User/User.php b/apps/user_ldap/lib/User/User.php index f64c0b4b44..02764a72ec 100644 --- a/apps/user_ldap/lib/User/User.php +++ b/apps/user_ldap/lib/User/User.php @@ -245,10 +245,12 @@ class User { $this->connection->writeToCache($cacheKey, $groups); //Avatar - $attrs = array('jpegphoto', 'thumbnailphoto'); - foreach ($attrs as $attr) { - if(isset($ldapEntry[$attr])) { - $this->avatarImage = $ldapEntry[$attr][0]; + /** @var Connection $connection */ + $connection = $this->access->getConnection(); + $attributes = $connection->resolveRule('avatar'); + foreach ($attributes as $attribute) { + if(isset($ldapEntry[$attribute])) { + $this->avatarImage = $ldapEntry[$attribute][0]; // the call to the method that saves the avatar in the file // system must be postponed after the login. It is to ensure // external mounts are mounted properly (e.g. with login @@ -348,7 +350,9 @@ class User { } $this->avatarImage = false; - $attributes = array('jpegPhoto', 'thumbnailPhoto'); + /** @var Connection $connection */ + $connection = $this->access->getConnection(); + $attributes = $connection->resolveRule('avatar'); foreach($attributes as $attribute) { $result = $this->access->readAttribute($this->dn, $attribute); if($result !== false && is_array($result) && isset($result[0])) { @@ -575,7 +579,7 @@ class User { */ private function setOwnCloudAvatar() { if(!$this->image->valid()) { - $this->log->log('jpegPhoto data invalid for '.$this->dn, ILogger::ERROR); + $this->log->log('avatar image data from LDAP invalid for '.$this->dn, ILogger::ERROR); return false; } //make sure it is a square and not bigger than 128x128 diff --git a/apps/user_ldap/lib/User_LDAP.php b/apps/user_ldap/lib/User_LDAP.php index 1b0c07f0ca..ca7e0b304e 100644 --- a/apps/user_ldap/lib/User_LDAP.php +++ b/apps/user_ldap/lib/User_LDAP.php @@ -103,6 +103,10 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn return $this->userPluginManager->canChangeAvatar($uid); } + if(!$this->implementsActions(Backend::PROVIDE_AVATAR)) { + return true; + } + $user = $this->access->userManager->get($uid); if(!$user instanceof User) { return false; @@ -550,7 +554,7 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn return (bool)((Backend::CHECK_PASSWORD | Backend::GET_HOME | Backend::GET_DISPLAYNAME - | Backend::PROVIDE_AVATAR + | (($this->access->connection->ldapUserAvatarRule !== 'none') ? Backend::PROVIDE_AVATAR : 0) | Backend::COUNT_USERS | (((int)$this->access->connection->turnOnPasswordChange === 1)? Backend::SET_PASSWORD :0) | $this->userPluginManager->getImplementedActions()) diff --git a/apps/user_ldap/tests/ConfigurationTest.php b/apps/user_ldap/tests/ConfigurationTest.php index 797d2598be..26217ea130 100644 --- a/apps/user_ldap/tests/ConfigurationTest.php +++ b/apps/user_ldap/tests/ConfigurationTest.php @@ -23,7 +23,16 @@ namespace OCA\User_LDAP\Tests; +use OCA\User_LDAP\Configuration; + class ConfigurationTest extends \Test\TestCase { + /** @var Configuration */ + protected $configuration; + + public function setUp() { + parent::setUp(); + $this->configuration = new Configuration('t01', false); + } public function configurationDataProvider() { $inputWithDN = array( @@ -84,6 +93,10 @@ class ConfigurationTest extends \Test\TestCase { // default behaviour, one case is enough, special needs must be tested // individually 'set string value' => array('ldapHost', $inputString, $expectedString), + + 'set avatar rule, default' => ['ldapUserAvatarRule', 'default', 'default'], + 'set avatar rule, none' => ['ldapUserAvatarRule', 'none', 'none'], + 'set avatar rule, data attribute' => ['ldapUserAvatarRule', 'data:jpegPhoto', 'data:jpegPhoto'], ); } @@ -91,10 +104,35 @@ class ConfigurationTest extends \Test\TestCase { * @dataProvider configurationDataProvider */ public function testSetValue($key, $input, $expected) { - $configuration = new \OCA\User_LDAP\Configuration('t01', false); + $this->configuration->setConfiguration([$key => $input]); + $this->assertSame($this->configuration->$key, $expected); + } - $configuration->setConfiguration([$key => $input]); - $this->assertSame($configuration->$key, $expected); + public function avatarRuleValueProvider() { + return [ + ['none', []], + ['data:selfie', ['selfie']], + ['data:', ['jpegphoto', 'thumbnailphoto']], + ['default', ['jpegphoto', 'thumbnailphoto']], + ['invalid#', ['jpegphoto', 'thumbnailphoto']], + ]; + } + + /** + * @dataProvider avatarRuleValueProvider + */ + public function testGetAvatarAttributes($setting, $expected) { + $this->configuration->setConfiguration(['ldapUserAvatarRule' => $setting]); + $this->assertSame($expected, $this->configuration->getAvatarAttributes()); + } + + /** + * @dataProvider avatarRuleValueProvider + */ + public function testResolveRule($setting, $expected) { + $this->configuration->setConfiguration(['ldapUserAvatarRule' => $setting]); + // so far the only thing that can get resolved :) + $this->assertSame($expected, $this->configuration->resolveRule('avatar')); } } diff --git a/apps/user_ldap/tests/User/ManagerTest.php b/apps/user_ldap/tests/User/ManagerTest.php index da30b210b1..5399aa95a6 100644 --- a/apps/user_ldap/tests/User/ManagerTest.php +++ b/apps/user_ldap/tests/User/ManagerTest.php @@ -238,7 +238,17 @@ class ManagerTest extends \Test\TestCase { $this->assertNull($user); } - public function testGetAttributesAll() { + public function attributeRequestProvider() { + return [ + [ false ], + [ true ], + ]; + } + + /** + * @dataProvider attributeRequestProvider + */ + public function testGetAttributes($minimal) { list($access, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr, $notiMgr) = $this->getTestInstances(); @@ -246,28 +256,14 @@ class ManagerTest extends \Test\TestCase { $manager->setLdapAccess($access); $connection = $access->getConnection(); - $connection->setConfiguration(array('ldapEmailAttribute' => 'mail')); + $connection->setConfiguration(['ldapEmailAttribute' => 'mail', 'ldapUserAvatarRule' => 'default']); - $attributes = $manager->getAttributes(); + $attributes = $manager->getAttributes($minimal); $this->assertTrue(in_array('dn', $attributes)); $this->assertTrue(in_array($access->getConnection()->ldapEmailAttribute, $attributes)); - $this->assertTrue(in_array('jpegphoto', $attributes)); - $this->assertTrue(in_array('thumbnailphoto', $attributes)); - } - - public function testGetAttributesMinimal() { - list($access, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr, $notiMgr) = - $this->getTestInstances(); - - $manager = new Manager($config, $filesys, $log, $avaMgr, $image, $dbc, $userMgr, $notiMgr); - $manager->setLdapAccess($access); - - $attributes = $manager->getAttributes(true); - - $this->assertTrue(in_array('dn', $attributes)); - $this->assertTrue(!in_array('jpegphoto', $attributes)); - $this->assertTrue(!in_array('thumbnailphoto', $attributes)); + $this->assertSame(!$minimal, in_array('jpegphoto', $attributes)); + $this->assertSame(!$minimal, in_array('thumbnailphoto', $attributes)); } } From 846ab25fc0dc34b2456d5a7914298006d06ddaee Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 5 Jul 2018 11:29:19 +0200 Subject: [PATCH 2/2] adjust and add more unit tests Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Configuration.php | 2 +- apps/user_ldap/tests/ConfigurationTest.php | 1 + apps/user_ldap/tests/User/UserTest.php | 82 +++++++++++++++++----- apps/user_ldap/tests/User_LDAPTest.php | 27 +++++++ 4 files changed, 95 insertions(+), 17 deletions(-) diff --git a/apps/user_ldap/lib/Configuration.php b/apps/user_ldap/lib/Configuration.php index 3871003fae..25d6485959 100644 --- a/apps/user_ldap/lib/Configuration.php +++ b/apps/user_ldap/lib/Configuration.php @@ -568,7 +568,7 @@ class Configuration { if($attribute === '') { return $defaultAttributes; } - return [$attribute]; + return [strtolower($attribute)]; } if($value !== self::AVATAR_PREFIX_DEFAULT) { \OC::$server->getLogger()->warning('Invalid config value to ldapUserAvatarRule; falling back to default.'); diff --git a/apps/user_ldap/tests/ConfigurationTest.php b/apps/user_ldap/tests/ConfigurationTest.php index 26217ea130..ab1312860f 100644 --- a/apps/user_ldap/tests/ConfigurationTest.php +++ b/apps/user_ldap/tests/ConfigurationTest.php @@ -112,6 +112,7 @@ class ConfigurationTest extends \Test\TestCase { return [ ['none', []], ['data:selfie', ['selfie']], + ['data:sELFie', ['selfie']], ['data:', ['jpegphoto', 'thumbnailphoto']], ['default', ['jpegphoto', 'thumbnailphoto']], ['invalid#', ['jpegphoto', 'thumbnailphoto']], diff --git a/apps/user_ldap/tests/User/UserTest.php b/apps/user_ldap/tests/User/UserTest.php index aa6498be08..837c72a3a3 100644 --- a/apps/user_ldap/tests/User/UserTest.php +++ b/apps/user_ldap/tests/User/UserTest.php @@ -503,7 +503,7 @@ class UserTest extends \Test\TestCase { $this->access->expects($this->once()) ->method('readAttribute') ->with($this->equalTo($this->dn), - $this->equalTo('jpegPhoto')) + $this->equalTo('jpegphoto')) ->will($this->returnValue(['this is a photo'])); $this->image->expects($this->once()) @@ -536,6 +536,11 @@ class UserTest extends \Test\TestCase { ->with($this->equalTo($this->uid)) ->will($this->returnValue($avatar)); + $this->connection->expects($this->any()) + ->method('resolveRule') + ->with('avatar') + ->willReturn(['jpegphoto', 'thumbnailphoto']); + $this->user->updateAvatar(); } @@ -544,11 +549,11 @@ class UserTest extends \Test\TestCase { ->method('readAttribute') ->willReturnCallback(function($dn, $attr) { if($dn === $this->dn - && $attr === 'jpegPhoto') + && $attr === 'jpegphoto') { return false; } elseif($dn === $this->dn - && $attr === 'thumbnailPhoto') + && $attr === 'thumbnailphoto') { return ['this is a photo']; } @@ -585,6 +590,11 @@ class UserTest extends \Test\TestCase { ->with($this->equalTo($this->uid)) ->will($this->returnValue($avatar)); + $this->connection->expects($this->any()) + ->method('resolveRule') + ->with('avatar') + ->willReturn(['jpegphoto', 'thumbnailphoto']); + $this->user->updateAvatar(); } @@ -593,11 +603,11 @@ class UserTest extends \Test\TestCase { ->method('readAttribute') ->willReturnCallback(function($dn, $attr) { if($dn === $this->dn - && $attr === 'jpegPhoto') + && $attr === 'jpegphoto') { return false; } elseif($dn === $this->dn - && $attr === 'thumbnailPhoto') + && $attr === 'thumbnailphoto') { return ['this is a photo']; } @@ -626,6 +636,11 @@ class UserTest extends \Test\TestCase { $this->avatarManager->expects($this->never()) ->method('getAvatar'); + $this->connection->expects($this->any()) + ->method('resolveRule') + ->with('avatar') + ->willReturn(['jpegphoto', 'thumbnailphoto']); + $this->user->updateAvatar(); } @@ -634,11 +649,11 @@ class UserTest extends \Test\TestCase { ->method('readAttribute') ->willReturnCallback(function($dn, $attr) { if($dn === $this->dn - && $attr === 'jpegPhoto') + && $attr === 'jpegphoto') { return false; } elseif($dn === $this->dn - && $attr === 'thumbnailPhoto') + && $attr === 'thumbnailphoto') { return ['this is a photo']; } @@ -676,6 +691,11 @@ class UserTest extends \Test\TestCase { ->with($this->equalTo($this->uid)) ->will($this->returnValue($avatar)); + $this->connection->expects($this->any()) + ->method('resolveRule') + ->with('avatar') + ->willReturn(['jpegphoto', 'thumbnailphoto']); + $this->assertFalse($this->user->updateAvatar()); } @@ -710,6 +730,11 @@ class UserTest extends \Test\TestCase { $this->avatarManager->expects($this->never()) ->method('getAvatar'); + $this->connection->expects($this->any()) + ->method('resolveRule') + ->with('avatar') + ->willReturn(['jpegphoto', 'thumbnailphoto']); + $this->user->updateAvatar(); } @@ -756,6 +781,11 @@ class UserTest extends \Test\TestCase { $this->anything()) ->will($this->returnValue(true)); + $this->connection->expects($this->any()) + ->method('resolveRule') + ->with('avatar') + ->willReturn(['jpegphoto', 'thumbnailphoto']); + $this->user->update(); } @@ -802,8 +832,12 @@ class UserTest extends \Test\TestCase { $this->access->expects($this->once()) ->method('readAttribute') ->with($this->equalTo($this->dn), - $this->equalTo('jpegPhoto')) + $this->equalTo('jpegphoto')) ->will($this->returnValue(['this is a photo'])); + $this->connection->expects($this->any()) + ->method('resolveRule') + ->with('avatar') + ->willReturn(['jpegphoto', 'thumbnailphoto']); $photo = $this->user->getAvatarImage(); $this->assertSame('this is a photo', $photo); @@ -812,6 +846,18 @@ class UserTest extends \Test\TestCase { $this->user->getAvatarImage(); } + public function testGetAvatarImageDisabled() { + $this->access->expects($this->never()) + ->method('readAttribute') + ->with($this->equalTo($this->dn), $this->anything()); + $this->connection->expects($this->any()) + ->method('resolveRule') + ->with('avatar') + ->willReturn([]); + + $this->assertFalse($this->user->getAvatarImage()); + } + public function imageDataProvider() { return [ [ false, false ], @@ -859,16 +905,20 @@ class UserTest extends \Test\TestCase { } return $name; })); + $this->connection->expects($this->any()) + ->method('resolveRule') + ->with('avatar') + ->willReturn(['jpegphoto', 'thumbnailphoto']); - $record = array( - strtolower($this->connection->ldapQuotaAttribute) => array('4096'), - strtolower($this->connection->ldapEmailAttribute) => array('alice@wonderland.org'), - strtolower($this->connection->ldapUserDisplayName) => array('Aaaaalice'), + $record = [ + strtolower($this->connection->ldapQuotaAttribute) => ['4096'], + strtolower($this->connection->ldapEmailAttribute) => ['alice@wonderland.org'], + strtolower($this->connection->ldapUserDisplayName) => ['Aaaaalice'], 'uid' => [$this->uid], - 'homedirectory' => array('Alice\'s Folder'), - 'memberof' => array('cn=groupOne', 'cn=groupTwo'), - 'jpegphoto' => array('here be an image') - ); + 'homedirectory' => ['Alice\'s Folder'], + 'memberof' => ['cn=groupOne', 'cn=groupTwo'], + 'jpegphoto' => ['here be an image'] + ]; foreach($requiredMethods as $method) { $userMock->expects($this->once()) diff --git a/apps/user_ldap/tests/User_LDAPTest.php b/apps/user_ldap/tests/User_LDAPTest.php index 927a7550f6..447b91decf 100644 --- a/apps/user_ldap/tests/User_LDAPTest.php +++ b/apps/user_ldap/tests/User_LDAPTest.php @@ -1396,4 +1396,31 @@ class User_LDAPTest extends TestCase { $this->assertFalse($this->backend->createUser('uid', 'password')); } + + public function actionProvider() { + return [ + [ 'ldapUserAvatarRule', 'default', Backend::PROVIDE_AVATAR, true] , + [ 'ldapUserAvatarRule', 'data:selfiePhoto', Backend::PROVIDE_AVATAR, true], + [ 'ldapUserAvatarRule', 'none', Backend::PROVIDE_AVATAR, false], + [ 'turnOnPasswordChange', 0, Backend::SET_PASSWORD, false], + [ 'turnOnPasswordChange', 1, Backend::SET_PASSWORD, true], + ]; + } + + /** + * @dataProvider actionProvider + */ + public function testImplementsAction($configurable, $value, $actionCode, $expected) { + $this->pluginManager->expects($this->once()) + ->method('getImplementedActions') + ->willReturn(0); + + $this->connection->expects($this->any()) + ->method('__get') + ->willReturnMap([ + [$configurable, $value], + ]); + + $this->assertSame($expected, $this->backend->implementsActions($actionCode)); + } }