Merge pull request #17464 from owncloud/fix-17232

fix uncaught exception on not permitted file types when setting avatar, fixes #17232

9 files changed, 340 insertions, 98 deletions

diff --git a/apps/user_ldap/lib/user/user.php b/apps/user_ldap/lib/user/user.php
index 54ee7a9b543..ac5d8f5a374 100644
--- a/apps/user_ldap/lib/user/user.php
+++ b/apps/user_ldap/lib/user/user.php
@@ -342,7 +342,13 @@ class User {
 		}
 
 		$avatar = $this->avatarManager->getAvatar($this->uid);
-		$avatar->set($this->image);
+		try {
+			$avatar->set($this->image);
+		} catch (\Exception $e) {
+			\OC::$server->getLogger()->notice(
+				'Could not set avatar for ' . $this->dn	. ', because: ' . $e->getMessage(),
+				['app' => 'user_ldap']);
+		}
 	}
 
 }
diff --git a/apps/user_ldap/tests/integration/abstractintegrationtest.php b/apps/user_ldap/tests/integration/abstractintegrationtest.php
new file mode 100644
index 00000000000..f0f5e2de0a4
--- /dev/null
+++ b/apps/user_ldap/tests/integration/abstractintegrationtest.php
@@ -0,0 +1,137 @@
+<?php
+/**
+ * @author Arthur Schiwon <blizzz@owncloud.com>
+ *
+ * @copyright Copyright (c) 2015, ownCloud, Inc.
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OCA\user_ldap\tests\integration;
+
+use OCA\user_ldap\lib\Access;
+use OCA\user_ldap\lib\Connection;
+use OCA\user_ldap\lib\LDAP;
+use OCA\user_ldap\lib\user\Manager;
+
+abstract class AbstractIntegrationTest {
+	/** @var  LDAP */
+	protected $ldap;
+
+	/** @var  Connection */
+	protected $connection;
+
+	/** @var Access */
+	protected $access;
+
+	/** @var Manager */
+	protected $userManager;
+
+	/** @var  string */
+	protected $base;
+
+	/** @var string[] */
+	protected $server;
+
+	public function __construct($host, $port, $bind, $pwd, $base) {
+		$this->base = $base;
+		$this->server = [
+			'host' => $host,
+			'port' => $port,
+			'dn'   => $bind,
+			'pwd'  => $pwd
+		];
+	}
+
+	/**
+	 * prepares the LDAP environment and sets up a test configuration for
+	 * the LDAP backend.
+	 */
+	public function init() {
+		$this->initLDAPWrapper();
+		$this->initConnection();
+		$this->initUserManager();
+		$this->initAccess();
+
+	}
+
+	/**
+	 * initializes the test LDAP wrapper
+	 */
+	protected function initLDAPWrapper() {
+		$this->ldap = new LDAP();
+	}
+
+	/**
+	 * sets up the LDAP configuration to be used for the test
+	 */
+	protected function initConnection() {
+		$this->connection = new Connection($this->ldap, '', null);
+		$this->connection->setConfiguration([
+			'ldapHost' => $this->server['host'],
+			'ldapPort' => $this->server['port'],
+			'ldapBase' => $this->base,
+			'ldapAgentName' => $this->server['dn'],
+			'ldapAgentPassword' => $this->server['pwd'],
+			'ldapUserFilter' => 'objectclass=inetOrgPerson',
+			'ldapUserDisplayName' => 'cn',
+			'ldapGroupDisplayName' => 'cn',
+			'ldapLoginFilter' => '(|(uid=%uid)(samaccountname=%uid))',
+			'ldapCacheTTL' => 0,
+			'ldapConfigurationActive' => 1,
+		]);
+	}
+
+	/**
+	 * initializes an LDAP user manager instance
+	 * @return Manager
+	 */
+	protected function initUserManager() {
+		$this->userManager = new FakeManager();
+	}
+
+	/**
+	 * initializes the Access test instance
+	 */
+	protected function initAccess() {
+		$this->access = new Access($this->connection, $this->ldap, $this->userManager);
+	}
+
+	/**
+	 * runs the test cases while outputting progress and result information
+	 *
+	 * If a test failed, the script is exited with return code 1.
+	 */
+	public function run() {
+		$methods = get_class_methods($this);
+		$atLeastOneCaseRan = false;
+		foreach($methods as $method) {
+			if(strpos($method, 'case') === 0) {
+				print("running $method " . PHP_EOL);
+				if(!$this->$method()) {
+					print(PHP_EOL . '>>> !!! Test ' . $method . ' FAILED !!! <<<' . PHP_EOL . PHP_EOL);
+					exit(1);
+				}
+				$atLeastOneCaseRan = true;
+			}
+		}
+		if($atLeastOneCaseRan) {
+			print('Tests succeeded' . PHP_EOL);
+		} else {
+			print('No Test was available.' . PHP_EOL);
+			exit(1);
+		}
+	}
+}
diff --git a/apps/user_ldap/tests/integration/data/avatar-invalid.gif b/apps/user_ldap/tests/integration/data/avatar-invalid.gif
new file mode 100644
index 00000000000..000108834d8
--- /dev/null
+++ b/apps/user_ldap/tests/integration/data/avatar-invalid.gif
diff --git a/apps/user_ldap/tests/integration/data/avatar-valid.jpg b/apps/user_ldap/tests/integration/data/avatar-valid.jpg
new file mode 100644
index 00000000000..61b5ec2e730
--- /dev/null
+++ b/apps/user_ldap/tests/integration/data/avatar-valid.jpg
diff --git a/apps/user_ldap/tests/integration/fakemanager.php b/apps/user_ldap/tests/integration/fakemanager.php
new file mode 100644
index 00000000000..afc9c552a98
--- /dev/null
+++ b/apps/user_ldap/tests/integration/fakemanager.php
@@ -0,0 +1,33 @@
+<?php
+/**
+ * @author Arthur Schiwon <blizzz@owncloud.com>
+ *
+ * @copyright Copyright (c) 2015, ownCloud, Inc.
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OCA\user_ldap\tests\integration;
+
+/**
+ * Class FakeManager
+ *
+ * this is a mock of \OCA\user_ldap\lib\user\Manager which is a dependency of
+ * Access, that pulls plenty more things in. Because it is not needed in the
+ * scope of these tests, we replace it with a mock.
+ */
+class FakeManager extends \OCA\user_ldap\lib\user\Manager {
+	public function __construct() {}
+}
diff --git a/apps/user_ldap/tests/integration/lib/IntegrationTestAccessGroupsMatchFilter.php b/apps/user_ldap/tests/integration/lib/IntegrationTestAccessGroupsMatchFilter.php
index 92035d94b4b..17d046187e8 100644
--- a/apps/user_ldap/tests/integration/lib/IntegrationTestAccessGroupsMatchFilter.php
+++ b/apps/user_ldap/tests/integration/lib/IntegrationTestAccessGroupsMatchFilter.php
@@ -1,72 +1,42 @@
 <?php
 /**
- * Created by PhpStorm.
- * User: blizzz
- * Date: 26.06.15
- * Time: 18:13
+ * @author Arthur Schiwon <blizzz@owncloud.com>
+ *
+ * @copyright Copyright (c) 2015, ownCloud, Inc.
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
  */
 
-use OCA\user_ldap\lib\LDAP;
-
-require_once __DIR__  . '/../../../../../lib/base.php';
-
-class IntegrationTestAccessGroupsMatchFilter {
-	/** @var  LDAP */
-	protected $ldap;
+namespace OCA\user_ldap\tests\integration\lib;
 
-	/** @var  \OCA\user_ldap\lib\Connection */
-	protected $connection;
+use OCA\user_ldap\lib\Connection;
+use OCA\user_ldap\tests\integration\AbstractIntegrationTest;
 
-	/** @var \OCA\user_ldap\lib\Access */
-	protected $access;
-
-	/** @var  string */
-	protected $base;
-
-	/** @var string[] */
-	protected $server;
+require_once __DIR__  . '/../../../../../lib/base.php';
 
-	public function __construct($host, $port, $bind, $pwd, $base) {
-		$this->base = $base;
-		$this->server = [
-			'host' => $host,
-			'port' => $port,
-			'dn'   => $bind,
-			'pwd'  => $pwd
-		];
-	}
+class IntegrationTestAccessGroupsMatchFilter extends AbstractIntegrationTest {
 
 	/**
-	 * prepares the LDAP environement and sets up a test configuration for
+	 * prepares the LDAP environment and sets up a test configuration for
 	 * the LDAP backend.
 	 */
 	public function init() {
-		require('setup-scripts/createExplicitUsers.php');
-		require('setup-scripts/createExplicitGroups.php');
-		require('setup-scripts/createExplicitGroupsDifferentOU.php');
-
-		$this->initLDAPWrapper();
-		$this->initConnection();
-		$this->initAccess();
-	}
-
-	/**
-	 * runs the test cases while outputting progress and result information
-	 *
-	 * If a test failed, the script is exited with return code 1.
-	 */
-	public function run() {
-		$cases = ['case1', 'case2', 'case3'];
-
-		foreach ($cases as $case) {
-			print("running $case " . PHP_EOL);
-			if (!$this->$case()) {
-				print(PHP_EOL . '>>> !!! Test ' . $case . ' FAILED !!! <<<' . PHP_EOL . PHP_EOL);
-				exit(1);
-			}
-		}
-
-		print('Tests succeeded' . PHP_EOL);
+		require(__DIR__ . '/../setup-scripts/createExplicitUsers.php');
+		require(__DIR__ . '/../setup-scripts/createExplicitGroups.php');
+		require(__DIR__ . '/../setup-scripts/createExplicitGroupsDifferentOU.php');
+		parent::init();
 	}
 
 	/**
@@ -75,7 +45,7 @@ class IntegrationTestAccessGroupsMatchFilter {
 	 *
 	 * @return bool
 	 */
-	private function case1() {
+	protected function case1() {
 		$this->connection->setConfiguration(['ldapGroupFilter' => 'cn=RedGroup']);
 
 		$dns = ['cn=RedGroup,ou=Groups,' . $this->base];
@@ -89,7 +59,7 @@ class IntegrationTestAccessGroupsMatchFilter {
 	 *
 	 * @return bool
 	 */
-	private function case2() {
+	protected function case2() {
 		$this->connection->setConfiguration(['ldapGroupFilter' => '(|(cn=RedGroup)(cn=PurpleGroup))']);
 
 		$dns = [
@@ -113,7 +83,7 @@ class IntegrationTestAccessGroupsMatchFilter {
 	 *
 	 * @return bool
 	 */
-	private function case3() {
+	protected function case3() {
 		$this->connection->setConfiguration(['ldapGroupFilter' => '(objectclass=groupOfNames)']);
 
 		$dns = [
@@ -132,53 +102,21 @@ class IntegrationTestAccessGroupsMatchFilter {
 	}
 
 	/**
-	 * initializes the Access test instance
-	 */
-	private function initAccess() {
-		$this->access = new \OCA\user_ldap\lib\Access($this->connection, $this->ldap, new FakeManager());
-	}
-
-	/**
-	 * initializes the test LDAP wrapper
-	 */
-	private function initLDAPWrapper() {
-		$this->ldap = new LDAP();
-	}
-
-	/**
 	 * sets up the LDAP configuration to be used for the test
 	 */
-	private function initConnection() {
-		$this->connection = new \OCA\user_ldap\lib\Connection($this->ldap, '', null);
+	protected function initConnection() {
+		parent::initConnection();
 		$this->connection->setConfiguration([
-			'ldapHost' => $this->server['host'],
-			'ldapPort' => $this->server['port'],
-			'ldapBase' => $this->base,
 			'ldapBaseGroups' => 'ou=Groups,' . $this->base,
-			'ldapAgentName' => $this->server['dn'],
-			'ldapAgentPassword' => $this->server['pwd'],
 			'ldapUserFilter' => 'objectclass=inetOrgPerson',
 			'ldapUserDisplayName' => 'displayName',
 			'ldapGroupDisplayName' => 'cn',
 			'ldapLoginFilter' => 'uid=%uid',
-			'ldapCacheTTL' => 0,
-			'ldapConfigurationActive' => 1,
 		]);
 	}
 }
 
-/**
- * Class FakeManager
- *
- * this is a mock of \OCA\user_ldap\lib\user\Manager which is a dependency of
- * Access, that pulls plenty more things in. Because it is not needed in the
- * scope of these tests, we replace it with a mock.
- */
-class FakeManager extends \OCA\user_ldap\lib\user\Manager {
-	public function __construct() {}
-}
-
-require_once('setup-scripts/config.php');
+require_once(__DIR__ . '/../setup-scripts/config.php');
 $test = new IntegrationTestAccessGroupsMatchFilter($host, $port, $adn, $apwd, $bdn);
 $test->init();
 $test->run();
diff --git a/apps/user_ldap/tests/integration/lib/user/IntegrationTestUserAvatar.php b/apps/user_ldap/tests/integration/lib/user/IntegrationTestUserAvatar.php
new file mode 100644
index 00000000000..a03d6b0c0c6
--- /dev/null
+++ b/apps/user_ldap/tests/integration/lib/user/IntegrationTestUserAvatar.php
@@ -0,0 +1,128 @@
+<?php
+
+use OCA\user_ldap\lib\user\User;
+use OCA\User_LDAP\Mapping\UserMapping;
+use OCA\user_ldap\tests\integration\AbstractIntegrationTest;
+
+require_once __DIR__  . '/../../../../../../lib/base.php';
+
+class IntegrationTestUserAvatar extends AbstractIntegrationTest {
+	/** @var  UserMapping */
+	protected $mapping;
+
+	/**
+	 * prepares the LDAP environment and sets up a test configuration for
+	 * the LDAP backend.
+	 */
+	public function init() {
+		require(__DIR__ . '/../../setup-scripts/createExplicitUsers.php');
+		parent::init();
+		$this->mapping = new UserMapping(\OC::$server->getDatabaseConnection());
+		$this->mapping->clear();
+		$this->access->setUserMapper($this->mapping);
+		$userBackend  = new OCA\user_ldap\USER_LDAP($this->access, \OC::$server->getConfig());
+		\OC_User::useBackend($userBackend);
+	}
+
+	/**
+	 * A method that does the common steps of test cases 1 and 2. The evaluation
+	 * is not happening here.
+	 *
+	 * @param string $dn
+	 * @param string $username
+	 * @param string $image
+	 */
+	private function execFetchTest($dn, $username, $image) {
+		$this->setJpegPhotoAttribute($dn, $image);
+
+		// assigns our self-picked oc username to the dn
+		$this->mapping->map($dn, $username, 'fakeUUID-' . $username);
+
+		// initialize home folder and make sure that the user will update
+		// also remove an possibly existing avatar
+		\OC_Util::tearDownFS();
+		\OC_Util::setupFS($username);
+		\OC::$server->getUserFolder($username);
+		\OC::$server->getConfig()->deleteUserValue($username, 'user_ldap', User::USER_PREFKEY_LASTREFRESH);
+		if(\OC::$server->getAvatarManager()->getAvatar($username)->exists()) {
+			\OC::$server->getAvatarManager()->getAvatar($username)->remove();
+		}
+
+		// finally attempt to get the avatar set
+		$user = $this->userManager->get($dn);
+		$user->updateAvatar();
+	}
+
+	/**
+	 * tests whether an avatar can be retrieved from LDAP and stored correctly
+	 *
+	 * @return bool
+	 */
+	protected function case1() {
+		$image = file_get_contents(__DIR__ . '/../../data/avatar-valid.jpg');
+		$dn = 'uid=alice,ou=Users,' . $this->base;
+		$username = 'alice1337';
+
+		$this->execFetchTest($dn, $username, $image);
+
+		return \OC::$server->getAvatarManager()->getAvatar($username)->exists();
+	}
+
+	/**
+	 * tests whether an image received from LDAP which is of an invalid file
+	 * type is dealt with properly (i.e. not set and not dying).
+	 *
+	 * @return bool
+	 */
+	protected function case2() {
+		// gif by Pmspinner from https://commons.wikimedia.org/wiki/File:Avatar2469_3.gif
+		$image = file_get_contents(__DIR__ . '/../../data/avatar-invalid.gif');
+		$dn = 'uid=boris,ou=Users,' . $this->base;
+		$username = 'boris7844';
+
+		$this->execFetchTest($dn, $username, $image);
+
+		return !\OC::$server->getAvatarManager()->getAvatar($username)->exists();
+	}
+
+	/**
+	 * This writes an image to the 'jpegPhoto' attribute on LDAP.
+	 *
+	 * @param string $dn
+	 * @param string $image An image read via file_get_contents
+	 * @throws \OC\ServerNotAvailableException
+	 */
+	private function setJpegPhotoAttribute($dn, $image) {
+		$changeSet = ['jpegphoto' => $image];
+		ldap_mod_add($this->connection->getConnectionResource(), $dn, $changeSet);
+	}
+
+	protected function initUserManager() {
+		$this->userManager = new \OCA\user_ldap\lib\user\Manager(
+			\OC::$server->getConfig(),
+			new \OCA\user_ldap\lib\FilesystemHelper(),
+			new \OCA\user_ldap\lib\LogWrapper(),
+			\OC::$server->getAvatarManager(),
+			new \OCP\Image(),
+			\OC::$server->getDatabaseConnection()
+		);
+	}
+
+	/**
+	 * sets up the LDAP configuration to be used for the test
+	 */
+	protected function initConnection() {
+		parent::initConnection();
+		$this->connection->setConfiguration([
+			'ldapUserFilter' => 'objectclass=inetOrgPerson',
+			'ldapUserDisplayName' => 'displayName',
+			'ldapGroupDisplayName' => 'cn',
+			'ldapLoginFilter' => 'uid=%uid',
+		]);
+	}
+}
+
+require_once(__DIR__ . '/../../setup-scripts/config.php');
+$test = new IntegrationTestUserAvatar($host, $port, $adn, $apwd, $bdn);
+$test->init();
+$test->run();
diff --git a/apps/user_ldap/tests/integration/run-test.sh b/apps/user_ldap/tests/integration/run-test.sh
index e07e9b43408..7a29db25670 100755
--- a/apps/user_ldap/tests/integration/run-test.sh
+++ b/apps/user_ldap/tests/integration/run-test.sh
@@ -13,5 +13,5 @@ fi
 
 
 # sleep is necessary, otherwise the LDAP server cannot be connected to, yet.
-setup-scripts/start.sh && sleep 2 && php -f "$TESTSCRIPT"
+setup-scripts/start.sh && sleep 5 && php -f "$TESTSCRIPT"
 setup-scripts/stop.sh
diff --git a/apps/user_ldap/tests/integration/setup-scripts/createExplicitUsers.php b/apps/user_ldap/tests/integration/setup-scripts/createExplicitUsers.php
index ac21d48fd16..bb784d60f7b 100644
--- a/apps/user_ldap/tests/integration/setup-scripts/createExplicitUsers.php
+++ b/apps/user_ldap/tests/integration/setup-scripts/createExplicitUsers.php
@@ -30,7 +30,7 @@ if (true) {
 	}
 }
 
-$users = ['alice'];
+$users = ['alice', 'boris'];
 
 foreach ($users as $uid) {
 	$newDN = 'uid=' . $uid . ',' . $ouDN;