Merge pull request #20505 from nextcloud/fix/noid/system-creds

do not advertise nulled userId for for systemwide credentials
nextcloud · Apr 16, 2020 · d55f418 · d55f418
2 parents 8971403 + 5437844
commit d55f418
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 9 deletions.
diff --git a/lib/private/Security/CredentialsManager.php b/lib/private/Security/CredentialsManager.php
@@ -53,15 +53,15 @@ public function __construct(ICrypto $crypto, IDBConnection $dbConnection) {
 	/**
 	 * Store a set of credentials
 	 *
-	 * @param string|null $userId Null for system-wide credentials
+	 * @param string $userId empty string for system-wide credentials
 	 * @param string $identifier
 	 * @param mixed $credentials
 	 */
 	public function store($userId, $identifier, $credentials) {
 		$value = $this->crypto->encrypt(json_encode($credentials));
 
 		$this->dbConnection->setValues(self::DB_TABLE, [
-			'user' => $userId,
+			'user' => (string)$userId,
 			'identifier' => $identifier,
 		], [
 			'credentials' => $value,
@@ -71,15 +71,15 @@ public function store($userId, $identifier, $credentials) {
 	/**
 	 * Retrieve a set of credentials
 	 *
-	 * @param string|null $userId Null for system-wide credentials
+	 * @param string $userId empty string for system-wide credentials
 	 * @param string $identifier
 	 * @return mixed
 	 */
 	public function retrieve($userId, $identifier) {
 		$qb = $this->dbConnection->getQueryBuilder();
 		$qb->select('credentials')
 			->from(self::DB_TABLE)
-			->where($qb->expr()->eq('user', $qb->createNamedParameter($userId)))
+			->where($qb->expr()->eq('user', $qb->createNamedParameter((string)$userId)))
 			->andWhere($qb->expr()->eq('identifier', $qb->createNamedParameter($identifier)))
 		;
 		$result = $qb->execute()->fetch();
@@ -95,14 +95,14 @@ public function retrieve($userId, $identifier) {
 	/**
 	 * Delete a set of credentials
 	 *
-	 * @param string|null $userId Null for system-wide credentials
+	 * @param string $userId empty string for system-wide credentials
 	 * @param string $identifier
 	 * @return int rows removed
 	 */
 	public function delete($userId, $identifier) {
 		$qb = $this->dbConnection->getQueryBuilder();
 		$qb->delete(self::DB_TABLE)
-			->where($qb->expr()->eq('user', $qb->createNamedParameter($userId)))
+			->where($qb->expr()->eq('user', $qb->createNamedParameter((string)$userId)))
 			->andWhere($qb->expr()->eq('identifier', $qb->createNamedParameter($identifier)))
 		;
 		return $qb->execute();

diff --git a/lib/public/Security/ICredentialsManager.php b/lib/public/Security/ICredentialsManager.php
@@ -33,7 +33,7 @@ interface ICredentialsManager {
 	/**
 	 * Store a set of credentials
 	 *
-	 * @param string|null $userId Null for system-wide credentials
+	 * @param string $userId empty string for system-wide credentials
 	 * @param string $identifier
 	 * @param mixed $credentials
 	 * @since 8.2.0
@@ -43,7 +43,7 @@ public function store($userId, $identifier, $credentials);
 	/**
 	 * Retrieve a set of credentials
 	 *
-	 * @param string|null $userId Null for system-wide credentials
+	 * @param string $userId empty string for system-wide credentials
 	 * @param string $identifier
 	 * @return mixed
 	 * @since 8.2.0
@@ -53,7 +53,7 @@ public function retrieve($userId, $identifier);
 	/**
 	 * Delete a set of credentials
 	 *
-	 * @param string|null $userId Null for system-wide credentials
+	 * @param string $userId empty string for system-wide credentials
 	 * @param string $identifier
 	 * @return int rows removed
 	 * @since 8.2.0

diff --git a/tests/lib/Security/CredentialsManagerTest.php b/tests/lib/Security/CredentialsManagerTest.php
@@ -27,6 +27,9 @@
 use OCP\ILogger;
 use OCP\Security\ICrypto;
 
+/**
+ * @group DB
+ */
 class CredentialsManagerTest extends \Test\TestCase {
 
 	/** @var ICrypto */
@@ -106,4 +109,34 @@ public function testRetrieve() {
 
 		$this->manager->retrieve($userId, $identifier);
 	}
+
+	/**
+	 * @dataProvider credentialsProvider
+	 */
+	public function testWithDB($userId, $identifier) {
+		$credentialsManager = \OC::$server->getCredentialsManager();
+
+		$secrets = 'Open Sesame';
+
+		$credentialsManager->store($userId, $identifier, $secrets);
+		$received = $credentialsManager->retrieve($userId, $identifier);
+
+		$this->assertSame($secrets, $received);
+
+		$removedRows = $credentialsManager->delete($userId, $identifier);
+		$this->assertSame(1, $removedRows);
+	}
+
+	public function credentialsProvider() {
+		return [
+			[
+				'alice',
+				'privateCredentials'
+			],
+			[
+				'',
+				'systemCredentials',
+			],
+		];
+	}
 }