Merge pull request #32581 from owncloud/stable10-stop-setting-empty-p…

…asswords [stable10] Throw an exception when attempting to set an empty password
owncloud · Sep 5, 2018 · 2c51fcb · 2c51fcb
2 parents 2743d40 + 41a393b
commit 2c51fcb
Show file tree

Hide file tree

Showing 6 changed files with 74 additions and 6 deletions.
diff --git a/lib/private/User/Database.php b/lib/private/User/Database.php
@@ -11,6 +11,7 @@
  * @author Joas Schilling <coding@schilljs.com>
  * @author Jörn Friedrich Dreyer <jfd@butonic.de>
  * @author Lukas Reschke <lukas@statuscode.ch>
+ * @author Matthew Setter <matthew@matthewsetter.com>
  * @author Michael Gapczynski <GapczynskiM@gmail.com>
  * @author Morris Jobke <hey@morrisjobke.de>
  * @author nishiki <nishiki@yaegashi.fr>
@@ -116,14 +117,20 @@ public function deleteUser($uid) {
 	}
 
 	/**
-	 * Set password
-	 * @param string $uid The username
+	 * Change a user's password
+	 *
+	 * @param string $uid The user's username
 	 * @param string $password The new password
 	 * @return bool
 	 *
-	 * Change the password of a user
+	 * @throws \OC\DatabaseException
+	 * @throws \InvalidArgumentException
 	 */
 	public function setPassword($uid, $password) {
+		if (Util::isEmptyString($password)) {
+			throw new \InvalidArgumentException('Password cannot be empty');
+		}
+
 		if ($this->userExists($uid)) {
 			$query = \OC_DB::prepare('UPDATE `*PREFIX*users` SET `password` = ? WHERE `uid` = ?');
 			$result = $query->execute([\OC::$server->getHasher()->hash($password), $uid]);

diff --git a/lib/private/User/User.php b/lib/private/User/User.php
@@ -48,6 +48,7 @@
 
 class User implements IUser {
 	use EventEmitterTrait;
+
 	/** @var Account */
 	private $account;
 
@@ -251,13 +252,18 @@ public function delete() {
 	}
 
 	/**
-	 * Set the password of the user
+	 * Set the user's password
 	 *
 	 * @param string $password
 	 * @param string $recoveryPassword for the encryption app to reset encryption keys
 	 * @return bool
+	 * @throws \InvalidArgumentException
 	 */
 	public function setPassword($password, $recoveryPassword = null) {
+		if (\OCP\Util::isEmptyString($password)) {
+			throw new \InvalidArgumentException('Password cannot be empty');
+		}
+
 		return $this->emittingCall(function () use (&$password, &$recoveryPassword) {
 			if ($this->emitter) {
 				$this->emitter->emit('\OC\User', 'preSetPassword', [$this, $password, $recoveryPassword]);

diff --git a/lib/public/Util.php b/lib/public/Util.php
@@ -654,6 +654,18 @@ public static function isValidFileName($file) {
 		return \OC_Util::isValidFileName($file);
 	}
 
+	/**
+	 * Determine if the string is, strictly-speaking, empty or not
+	 *
+	 * @param $value
+	 * @return bool
+	 * @since 10.0.1
+	 */
+	public static function isEmptyString($value) {
+		$value = \trim(\strval($value));
+		return ($value === '' || $value === '0');
+	}
+
 	/**
 	 * Generates a cryptographic secure pseudo-random string
 	 * @param int $length of the random string

diff --git a/tests/lib/TestCase.php b/tests/lib/TestCase.php
@@ -525,6 +525,18 @@ public function getCurrentUser() {
 		return $processUser['name'];
 	}
 
+	/**
+	 * @return array A list of items equivalent to an empty values
+	 */
+	public function getEmptyValues() {
+		return [
+			[''],
+			[0],
+			[null],
+			[false],
+		];
+	}
+
 	/**
 	 * @param string $string
 	 * @return bool|resource

diff --git a/tests/lib/User/DatabaseTest.php b/tests/lib/User/DatabaseTest.php
@@ -3,6 +3,7 @@
 * ownCloud
 *
 * @author Robin Appelman
+* @author Matthew Setter <matthew@matthewsetter.com>
 * @copyright Copyright (c) 2012 Robin Appelman icewind@owncloud.com
 *
 * This library is free software; you can redistribute it and/or
@@ -21,6 +22,7 @@
 */
 
 namespace Test\User;
+use Test\Traits\PasswordTrait;
 
 /**
  * Class DatabaseTest
@@ -39,7 +41,7 @@ public function getUser($prefix = 'test_') {
 
 	protected function setUp() {
 		parent::setUp();
-		$this->backend=new \OC\User\Database();
+		$this->backend = new \OC\User\Database();
 	}
 
 	protected function tearDown() {
@@ -52,6 +54,20 @@ protected function tearDown() {
 		parent::tearDown();
 	}
 
+	/**
+	 * Tests that a \OC\User\ArgumentNotSetException is thrown when the supplied password is empty.
+	 *
+	 * @param string $password
+	 * @dataProvider getEmptyValues
+	 * @expectedException \InvalidArgumentException
+	 * @expectedExceptionMessage Password cannot be empty
+	 */
+	public function testCannotSetEmptyPassword($password) {
+		$user1 = $this->getUser();
+		$this->backend->createUser($this->getUser(), 'pw');
+		$this->backend->setPassword($user1, $password);
+	}
+
 	public function testCreateUserInvalidatesCache() {
 		$user1 = $this->getUser();
 		$this->assertFalse($this->backend->userExists($user1));

diff --git a/tests/lib/User/UserTest.php b/tests/lib/User/UserTest.php
@@ -24,6 +24,7 @@
 use Symfony\Component\EventDispatcher\EventDispatcher;
 use Symfony\Component\EventDispatcher\GenericEvent;
 use Test\TestCase;
+use Test\Traits\PasswordTrait;
 
 /**
  * Class UserTest
@@ -33,7 +34,6 @@
  * @package Test\User
  */
 class UserTest extends TestCase {
-
 	/** @var AccountMapper | \PHPUnit_Framework_MockObject_MockObject */
 	private $accountMapper;
 	/** @var Account */
@@ -134,6 +134,21 @@ public function testSetPassword() {
 		$this->assertEquals('bar', $calledEvents['user.aftersetpassword']->getArgument('password'));
 		$this->assertEquals('', $calledEvents['user.aftersetpassword']->getArgument('recoveryPassword'));
 	}
+
+	/**
+	 * @param string $password
+	 * @dataProvider getEmptyValues
+	 * @expectedException \InvalidArgumentException
+	 * @expectedExceptionMessage Password cannot be empty
+	 * @throws \InvalidArgumentException
+	 */
+	public function testSetEmptyPasswordNotPermitted($password) {
+		(new User(
+			$this->createMock(Account::class),
+			$this->accountMapper
+		))->setPassword($password, 'bar');
+	}
+
 	public function testSetPasswordNotSupported() {
 		$this->config->expects($this->never())
 			->method('deleteUserValue')