From f6cb45203755d85b33684b1dab1a91b5b05e8c9a Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 15 Apr 2020 16:44:28 +0200 Subject: [PATCH 1/2] add DB tests for credentials manager these are actually expected to FAIL, because NULL as a userid is not allowed in the schema, but documented to be used on the source Signed-off-by: Arthur Schiwon --- tests/lib/Security/CredentialsManagerTest.php | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/lib/Security/CredentialsManagerTest.php b/tests/lib/Security/CredentialsManagerTest.php index 8b58542f8c39f..b5d4116b2930f 100644 --- 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' + ], + [ + null, + 'systemCredentials' + ] + ]; + } } From 5437844b7ec24d6011e8f1e4a0df5f727d259ea5 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 15 Apr 2020 19:34:23 +0200 Subject: [PATCH 2/2] fix credentialsManager documentation and ensure userId to be used as string Signed-off-by: Arthur Schiwon --- lib/private/Security/CredentialsManager.php | 12 ++++++------ lib/public/Security/ICredentialsManager.php | 6 +++--- tests/lib/Security/CredentialsManagerTest.php | 6 +++--- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/private/Security/CredentialsManager.php b/lib/private/Security/CredentialsManager.php index 770919dacd779..d187acdf02b93 100644 --- a/lib/private/Security/CredentialsManager.php +++ b/lib/private/Security/CredentialsManager.php @@ -53,7 +53,7 @@ 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 */ @@ -61,7 +61,7 @@ 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,7 +71,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 */ @@ -79,7 +79,7 @@ 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 index b1daad30c9f16..0b34d9a28ca98 100644 --- 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 index b5d4116b2930f..9c1a0cb9291c0 100644 --- a/tests/lib/Security/CredentialsManagerTest.php +++ b/tests/lib/Security/CredentialsManagerTest.php @@ -134,9 +134,9 @@ public function credentialsProvider() { 'privateCredentials' ], [ - null, - 'systemCredentials' - ] + '', + 'systemCredentials', + ], ]; } }