Merge pull request #10115 from nextcloud/backport/10098/ldap-image-attribute

[stable13] allow admin to disable fetching of avatars as well as a specific attribute

9 files changed, 240 insertions, 51 deletions

diff --git a/apps/user_ldap/lib/Configuration.php b/apps/user_ldap/lib/Configuration.php
index 8522962172b..ff4d17a5b92 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',
@@ -535,4 +543,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 [strtolower($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 f2523c1e316..039e46aeefd 100644
--- a/apps/user_ldap/lib/Connection.php
+++ b/apps/user_ldap/lib/Connection.php
@@ -47,6 +47,7 @@ use OC\ServerNotAvailableException;
  * @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
 	 * in early configuration state.
diff --git a/apps/user_ldap/lib/User/Manager.php b/apps/user_ldap/lib/User/Manager.php
index b04a321652c..4a770102a76 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 e51b0abc80d..32cf4812461 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, Util::ERROR);
+			$this->log->log('avatar image data from LDAP invalid for '.$this->dn, Util::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 a840774693e..4eb72592789 100644
--- a/apps/user_ldap/lib/User_LDAP.php
+++ b/apps/user_ldap/lib/User_LDAP.php
@@ -102,6 +102,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;
@@ -549,7 +553,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
 			| ((intval($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 797d2598be4..ab1312860fa 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,36 @@ 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);
+	}
+
+	public function avatarRuleValueProvider() {
+		return [
+			['none', []],
+			['data:selfie', ['selfie']],
+			['data:sELFie', ['selfie']],
+			['data:', ['jpegphoto', 'thumbnailphoto']],
+			['default', ['jpegphoto', 'thumbnailphoto']],
+			['invalid#', ['jpegphoto', 'thumbnailphoto']],
+		];
+	}
 
-		$configuration->setConfiguration([$key => $input]);
-		$this->assertSame($configuration->$key, $expected);
+	/**
+	 * @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 da30b210b17..5399aa95a6a 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));
 	}
 
 }
diff --git a/apps/user_ldap/tests/User/UserTest.php b/apps/user_ldap/tests/User/UserTest.php
index a8d17554011..29d2ba3829f 100644
--- a/apps/user_ldap/tests/User/UserTest.php
+++ b/apps/user_ldap/tests/User/UserTest.php
@@ -622,7 +622,7 @@ class UserTest extends \Test\TestCase {
 		$this->access->expects($this->once())
 			->method('readAttribute')
 			->with($this->equalTo('uid=alice,dc=foo,dc=bar'),
-				$this->equalTo('jpegPhoto'))
+				$this->equalTo('jpegphoto'))
 			->will($this->returnValue(array('this is a photo')));
 
 		$image->expects($this->once())
@@ -657,6 +657,10 @@ class UserTest extends \Test\TestCase {
 
 		$uid = 'alice';
 		$dn  = 'uid=alice,dc=foo,dc=bar';
+		$this->access->connection->expects($this->any())
+			->method('resolveRule')
+			->with('avatar')
+			->willReturn(['jpegphoto', 'thumbnailphoto']);
 
 		$user = new User(
 			$uid, $dn, $this->access, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr);
@@ -672,11 +676,11 @@ class UserTest extends \Test\TestCase {
 			->method('readAttribute')
 			->willReturnCallback(function($dn, $attr) {
 				if($dn === 'uid=alice,dc=foo,dc=bar'
-					&& $attr === 'jpegPhoto')
+					&& $attr === 'jpegphoto')
 				{
 					return false;
 				} elseif($dn === 'uid=alice,dc=foo,dc=bar'
-					&& $attr === 'thumbnailPhoto')
+					&& $attr === 'thumbnailphoto')
 				{
 					return ['this is a photo'];
 				}
@@ -715,6 +719,10 @@ class UserTest extends \Test\TestCase {
 
 		$uid = 'alice';
 		$dn  = 'uid=alice,dc=foo,dc=bar';
+		$this->access->connection->expects($this->any())
+			->method('resolveRule')
+			->with('avatar')
+			->willReturn(['jpegphoto', 'thumbnailphoto']);
 
 		$user = new User(
 			$uid, $dn, $this->access, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr);
@@ -730,11 +738,11 @@ class UserTest extends \Test\TestCase {
 			->method('readAttribute')
 			->willReturnCallback(function($dn, $attr) {
 				if($dn === $dn
-					&& $attr === 'jpegPhoto')
+					&& $attr === 'jpegphoto')
 				{
 					return false;
 				} elseif($dn === $dn
-					&& $attr === 'thumbnailPhoto')
+					&& $attr === 'thumbnailphoto')
 				{
 					return ['this is a photo'];
 				}
@@ -765,6 +773,10 @@ class UserTest extends \Test\TestCase {
 
 		$uid = 'alice';
 		$dn  = 'uid=alice,dc=foo,dc=bar';
+		$this->access->connection->expects($this->any())
+			->method('resolveRule')
+			->with('avatar')
+			->willReturn(['jpegphoto', 'thumbnailphoto']);
 
 		$user = new User(
 			$uid, $dn, $this->access, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr);
@@ -783,11 +795,11 @@ class UserTest extends \Test\TestCase {
 			->method('readAttribute')
 			->willReturnCallback(function($dn, $attr) {
 				if($dn === $dn
-					&& $attr === 'jpegPhoto')
+					&& $attr === 'jpegphoto')
 				{
 					return false;
 				} elseif($dn === $dn
-					&& $attr === 'thumbnailPhoto')
+					&& $attr === 'thumbnailphoto')
 				{
 					return ['this is a photo'];
 				}
@@ -825,6 +837,11 @@ class UserTest extends \Test\TestCase {
 			->with($this->equalTo($uid))
 			->will($this->returnValue($avatar));
 
+		$this->access->connection->expects($this->any())
+			->method('resolveRule')
+			->with('avatar')
+			->willReturn(['jpegphoto', 'thumbnailphoto']);
+
 		$user = new User(
 			$uid, $dn, $this->access, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr);
 
@@ -867,6 +884,10 @@ class UserTest extends \Test\TestCase {
 
 		$uid = 'alice';
 		$dn  = 'uid=alice,dc=foo,dc=bar';
+		$this->access->connection->expects($this->any())
+			->method('resolveRule')
+			->with('avatar')
+			->willReturn(['jpegphoto', 'thumbnailphoto']);
 
 		$user = new User(
 			$uid, $dn, $this->access, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr);
@@ -931,6 +952,10 @@ class UserTest extends \Test\TestCase {
 
 		$uid = 'alice';
 		$dn  = 'uid=alice,dc=foo,dc=bar';
+		$this->access->connection->expects($this->any())
+			->method('resolveRule')
+			->with('avatar')
+			->willReturn(['jpegphoto', 'thumbnailphoto']);
 
 		$user = new User(
 			$uid, $dn, $this->access, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr);
@@ -1003,8 +1028,12 @@ class UserTest extends \Test\TestCase {
 		$this->access->expects($this->once())
 			->method('readAttribute')
 			->with($this->equalTo('uid=alice,dc=foo,dc=bar'),
-				$this->equalTo('jpegPhoto'))
+				$this->equalTo('jpegphoto'))
 			->will($this->returnValue(array('this is a photo')));
+		$this->connection->expects($this->any())
+			->method('resolveRule')
+			->with('avatar')
+			->willReturn(['jpegphoto', 'thumbnailphoto']);
 
 		$uid = 'alice';
 		$dn  = 'uid=alice,dc=foo,dc=bar';
@@ -1016,7 +1045,28 @@ class UserTest extends \Test\TestCase {
 		$this->assertSame('this is a photo', $photo);
 		//make sure readAttribute is not called again but the already fetched
 		//photo is returned
-		$photo = $user->getAvatarImage();
+		$user->getAvatarImage();
+	}
+
+	public function testGetAvatarImageDisabled() {
+		list(, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr) =
+			$this->getTestInstances();
+
+		$uid = 'alice';
+		$dn  = 'uid=alice,dc=foo,dc=bar';
+
+		$this->access->expects($this->never())
+			->method('readAttribute')
+			->with($this->equalTo($dn), $this->anything());
+		$this->access->connection->expects($this->any())
+			->method('resolveRule')
+			->with('avatar')
+			->willReturn([]);
+
+		$user = new User(
+			$uid, $dn, $this->access, $config, $filesys, $image, $log, $avaMgr, $userMgr, $notiMgr);
+
+		$this->assertFalse($user->getAvatarImage());
 	}
 
 	public function imageDataProvider() {
@@ -1062,16 +1112,20 @@ class UserTest extends \Test\TestCase {
 				}
 				return $name;
 			}));
-
-		$record = array(
-			strtolower($this->connection->ldapQuotaAttribute) => array('4096'),
-			strtolower($this->connection->ldapEmailAttribute) => array('alice@wonderland.org'),
-			strtolower($this->connection->ldapUserDisplayName) => array('Aaaaalice'),
+		$this->connection->expects($this->any())
+			->method('resolveRule')
+			->with('avatar')
+			->willReturn(['jpegphoto', 'thumbnailphoto']);
+
+		$record = [
+			strtolower($this->connection->ldapQuotaAttribute) => ['4096'],
+			strtolower($this->connection->ldapEmailAttribute) => ['alice@wonderland.org'],
+			strtolower($this->connection->ldapUserDisplayName) => ['Aaaaalice'],
 			'uid' => array($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 d84cb52c5e6..5136e62e7d1 100644
--- a/apps/user_ldap/tests/User_LDAPTest.php
+++ b/apps/user_ldap/tests/User_LDAPTest.php
@@ -1663,4 +1663,45 @@ class User_LDAPTest extends TestCase {
 
 		$this->assertFalse($ldap->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) {
+		$pluginManager = $this->createMock(UserPluginManager::class);
+		$pluginManager->expects($this->once())
+			->method('getImplementedActions')
+			->willReturn(0);
+
+		$access = $this->getAccessMock();
+		$access->connection->expects($this->any())
+			->method('__get')
+			->willReturnMap([
+				[$configurable, $value],
+			]);
+
+		$config = $this->createMock(IConfig::class);
+		$noti = $this->createMock(INotificationManager::class);
+		$session = $this->createMock(Session::class);
+
+		$backend = new User_LDAP(
+			$access,
+			$config,
+			$noti,
+			$session,
+			$pluginManager
+		);
+
+		$this->assertSame($expected, $backend->implementsActions($actionCode));
+	}
 }