From 7fa219e8348f88fd4f8305f5cf03cea0299ba6e0 Mon Sep 17 00:00:00 2001 From: yemkareems Date: Mon, 28 Oct 2024 11:22:36 +0530 Subject: fix: encrypt and store password, decrypt and retrieve the same Signed-off-by: yemkareems --- .../Authentication/LoginCredentials/Store.php | 10 +++++++++- lib/private/Server.php | 3 ++- .../Authentication/LoginCredentials/StoreTest.php | 21 +++++++++++++++++++-- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/lib/private/Authentication/LoginCredentials/Store.php b/lib/private/Authentication/LoginCredentials/Store.php index 1e6f52ae2b9..52fa0c599af 100644 --- a/lib/private/Authentication/LoginCredentials/Store.php +++ b/lib/private/Authentication/LoginCredentials/Store.php @@ -28,6 +28,7 @@ namespace OC\Authentication\LoginCredentials; use OC\Authentication\Exceptions\PasswordlessTokenException; use OC\Authentication\Token\IProvider; +use OC\Security\Crypto; use OCP\Authentication\Exceptions\CredentialsUnavailableException; use OCP\Authentication\Exceptions\InvalidTokenException; use OCP\Authentication\LoginCredentials\ICredentials; @@ -47,12 +48,17 @@ class Store implements IStore { /** @var IProvider|null */ private $tokenProvider; + /** @var Crypto|null */ + private $crypto; + public function __construct(ISession $session, LoggerInterface $logger, - ?IProvider $tokenProvider = null) { + ?IProvider $tokenProvider = null, + ?Crypto $crypto = null) { $this->session = $session; $this->logger = $logger; $this->tokenProvider = $tokenProvider; + $this->crypto = $crypto; Util::connectHook('OC_User', 'post_login', $this, 'authenticate'); } @@ -63,6 +69,7 @@ class Store implements IStore { * @param array $params */ public function authenticate(array $params) { + $params['password'] = $this->crypto->encrypt((string)$params['password']); $this->session->set('login_credentials', json_encode($params)); } @@ -109,6 +116,7 @@ class Store implements IStore { if ($trySession && $this->session->exists('login_credentials')) { /** @var array $creds */ $creds = json_decode($this->session->get('login_credentials'), true); + $creds['password'] = $this->crypto->decrypt($creds['password']); return new Credentials( $creds['uid'], $creds['loginName'] ?? $this->session->get('loginname') ?? $creds['uid'], // Pre 20 didn't have a loginName property, hence fall back to the session value and then to the UID diff --git a/lib/private/Server.php b/lib/private/Server.php index ad1f188881a..4ec252e85bb 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -524,7 +524,8 @@ class Server extends ServerContainer implements IServerContainer { $tokenProvider = null; } $logger = $c->get(LoggerInterface::class); - return new Store($session, $logger, $tokenProvider); + $crypto = $c->get(Crypto::class); + return new Store($session, $logger, $tokenProvider, $crypto); }); $this->registerAlias(IStore::class, Store::class); $this->registerAlias(IProvider::class, Authentication\Token\Manager::class); diff --git a/tests/lib/Authentication/LoginCredentials/StoreTest.php b/tests/lib/Authentication/LoginCredentials/StoreTest.php index 80d64d5466f..3e2ee7ea66c 100644 --- a/tests/lib/Authentication/LoginCredentials/StoreTest.php +++ b/tests/lib/Authentication/LoginCredentials/StoreTest.php @@ -30,8 +30,10 @@ use OC\Authentication\LoginCredentials\Credentials; use OC\Authentication\LoginCredentials\Store; use OC\Authentication\Token\IProvider; use OC\Authentication\Token\IToken; +use OC\Security\Crypto; use OCP\Authentication\Exceptions\CredentialsUnavailableException; use OCP\ISession; +use OCP\Security\ICrypto; use OCP\Session\Exceptions\SessionNotAvailableException; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -46,6 +48,8 @@ class StoreTest extends TestCase { /** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */ private $logger; + /** @var ICrypto|\PHPUnit\Framework\MockObject\MockObject */ + private $crypto; /** @var Store */ private $store; @@ -56,20 +60,24 @@ class StoreTest extends TestCase { $this->session = $this->createMock(ISession::class); $this->tokenProvider = $this->createMock(IProvider::class); $this->logger = $this->createMock(LoggerInterface::class); + $this->crypto = $this->createMock(Crypto::class); - $this->store = new Store($this->session, $this->logger, $this->tokenProvider); + $this->store = new Store($this->session, $this->logger, $this->tokenProvider, $this->crypto); } public function testAuthenticate() { $params = [ 'run' => true, 'uid' => 'user123', - 'password' => 123456, + 'password' => '123456', ]; $this->session->expects($this->once()) ->method('set') ->with($this->equalTo('login_credentials'), $this->equalTo(json_encode($params))); + $this->crypto->expects($this->once()) + ->method('encrypt') + ->willReturn($params['password']); $this->store->authenticate($params); } @@ -156,6 +164,9 @@ class StoreTest extends TestCase { ->method('exists') ->with($this->equalTo('login_credentials')) ->willReturn(true); + $this->crypto->expects($this->once()) + ->method('decrypt') + ->willReturn($password); $this->session->expects($this->exactly(2)) ->method('get') ->willReturnMap([ @@ -193,6 +204,9 @@ class StoreTest extends TestCase { ->method('exists') ->with($this->equalTo('login_credentials')) ->willReturn(true); + $this->crypto->expects($this->once()) + ->method('decrypt') + ->willReturn($password); $this->session->expects($this->exactly(2)) ->method('get') ->willReturnMap([ @@ -231,6 +245,9 @@ class StoreTest extends TestCase { ->method('exists') ->with($this->equalTo('login_credentials')) ->willReturn(true); + $this->crypto->expects($this->once()) + ->method('decrypt') + ->willReturn($password); $this->session->expects($this->once()) ->method('get') ->with($this->equalTo('login_credentials')) -- cgit v1.2.3 From e4c22e02e0a7c0df6f9403761b665c82ced439b7 Mon Sep 17 00:00:00 2001 From: yemkareems Date: Mon, 28 Oct 2024 15:04:11 +0530 Subject: fix: crypto type made not nullable and tests run using ICrypto Signed-off-by: yemkareems --- lib/private/Authentication/LoginCredentials/Store.php | 6 +++--- lib/private/Server.php | 2 +- tests/lib/Authentication/LoginCredentials/StoreTest.php | 7 +++---- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/private/Authentication/LoginCredentials/Store.php b/lib/private/Authentication/LoginCredentials/Store.php index 52fa0c599af..47794827a93 100644 --- a/lib/private/Authentication/LoginCredentials/Store.php +++ b/lib/private/Authentication/LoginCredentials/Store.php @@ -48,13 +48,13 @@ class Store implements IStore { /** @var IProvider|null */ private $tokenProvider; - /** @var Crypto|null */ + /** @var Crypto */ private $crypto; public function __construct(ISession $session, LoggerInterface $logger, - ?IProvider $tokenProvider = null, - ?Crypto $crypto = null) { + Crypto $crypto, + ?IProvider $tokenProvider = null) { $this->session = $session; $this->logger = $logger; $this->tokenProvider = $tokenProvider; diff --git a/lib/private/Server.php b/lib/private/Server.php index 4ec252e85bb..0106b717998 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -525,7 +525,7 @@ class Server extends ServerContainer implements IServerContainer { } $logger = $c->get(LoggerInterface::class); $crypto = $c->get(Crypto::class); - return new Store($session, $logger, $tokenProvider, $crypto); + return new Store($session, $logger, $crypto, $tokenProvider); }); $this->registerAlias(IStore::class, Store::class); $this->registerAlias(IProvider::class, Authentication\Token\Manager::class); diff --git a/tests/lib/Authentication/LoginCredentials/StoreTest.php b/tests/lib/Authentication/LoginCredentials/StoreTest.php index 3e2ee7ea66c..ba3dbf78886 100644 --- a/tests/lib/Authentication/LoginCredentials/StoreTest.php +++ b/tests/lib/Authentication/LoginCredentials/StoreTest.php @@ -30,7 +30,6 @@ use OC\Authentication\LoginCredentials\Credentials; use OC\Authentication\LoginCredentials\Store; use OC\Authentication\Token\IProvider; use OC\Authentication\Token\IToken; -use OC\Security\Crypto; use OCP\Authentication\Exceptions\CredentialsUnavailableException; use OCP\ISession; use OCP\Security\ICrypto; @@ -60,9 +59,9 @@ class StoreTest extends TestCase { $this->session = $this->createMock(ISession::class); $this->tokenProvider = $this->createMock(IProvider::class); $this->logger = $this->createMock(LoggerInterface::class); - $this->crypto = $this->createMock(Crypto::class); + $this->crypto = $this->createMock(ICrypto::class); - $this->store = new Store($this->session, $this->logger, $this->tokenProvider, $this->crypto); + $this->store = new Store($this->session, $this->logger, $this->crypto, $this->tokenProvider); } public function testAuthenticate() { @@ -77,7 +76,7 @@ class StoreTest extends TestCase { ->with($this->equalTo('login_credentials'), $this->equalTo(json_encode($params))); $this->crypto->expects($this->once()) ->method('encrypt') - ->willReturn($params['password']); + ->willReturn('123456'); $this->store->authenticate($params); } -- cgit v1.2.3 From 13765c0dbf577f16fff7d9b6d4b8348f283d070c Mon Sep 17 00:00:00 2001 From: yemkareems Date: Mon, 28 Oct 2024 15:49:05 +0530 Subject: fix: use Icrypto in place of Cypto Signed-off-by: yemkareems --- lib/private/Authentication/LoginCredentials/Store.php | 6 +++--- lib/private/Server.php | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/private/Authentication/LoginCredentials/Store.php b/lib/private/Authentication/LoginCredentials/Store.php index 47794827a93..de04dc8aa39 100644 --- a/lib/private/Authentication/LoginCredentials/Store.php +++ b/lib/private/Authentication/LoginCredentials/Store.php @@ -28,12 +28,12 @@ namespace OC\Authentication\LoginCredentials; use OC\Authentication\Exceptions\PasswordlessTokenException; use OC\Authentication\Token\IProvider; -use OC\Security\Crypto; use OCP\Authentication\Exceptions\CredentialsUnavailableException; use OCP\Authentication\Exceptions\InvalidTokenException; use OCP\Authentication\LoginCredentials\ICredentials; use OCP\Authentication\LoginCredentials\IStore; use OCP\ISession; +use OCP\Security\ICrypto; use OCP\Session\Exceptions\SessionNotAvailableException; use OCP\Util; use Psr\Log\LoggerInterface; @@ -48,12 +48,12 @@ class Store implements IStore { /** @var IProvider|null */ private $tokenProvider; - /** @var Crypto */ + /** @var ICrypto */ private $crypto; public function __construct(ISession $session, LoggerInterface $logger, - Crypto $crypto, + ICrypto $crypto, ?IProvider $tokenProvider = null) { $this->session = $session; $this->logger = $logger; diff --git a/lib/private/Server.php b/lib/private/Server.php index 0106b717998..38d3f55e82e 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -524,7 +524,7 @@ class Server extends ServerContainer implements IServerContainer { $tokenProvider = null; } $logger = $c->get(LoggerInterface::class); - $crypto = $c->get(Crypto::class); + $crypto = $c->get(ICrypto::class); return new Store($session, $logger, $crypto, $tokenProvider); }); $this->registerAlias(IStore::class, Store::class); -- cgit v1.2.3 From 91cafe42b7b1fd7e65d9ded72bd93e1ed63d62a0 Mon Sep 17 00:00:00 2001 From: yemkareems Date: Mon, 28 Oct 2024 16:32:57 +0530 Subject: fix: crypto made inline for constructor and decrypt error handled in exception Signed-off-by: yemkareems --- lib/private/Authentication/LoginCredentials/Store.php | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/private/Authentication/LoginCredentials/Store.php b/lib/private/Authentication/LoginCredentials/Store.php index de04dc8aa39..83a69eb4937 100644 --- a/lib/private/Authentication/LoginCredentials/Store.php +++ b/lib/private/Authentication/LoginCredentials/Store.php @@ -26,6 +26,7 @@ declare(strict_types=1); */ namespace OC\Authentication\LoginCredentials; +use Exception; use OC\Authentication\Exceptions\PasswordlessTokenException; use OC\Authentication\Token\IProvider; use OCP\Authentication\Exceptions\CredentialsUnavailableException; @@ -48,17 +49,13 @@ class Store implements IStore { /** @var IProvider|null */ private $tokenProvider; - /** @var ICrypto */ - private $crypto; - public function __construct(ISession $session, LoggerInterface $logger, - ICrypto $crypto, + private readonly ICrypto $crypto, ?IProvider $tokenProvider = null) { $this->session = $session; $this->logger = $logger; $this->tokenProvider = $tokenProvider; - $this->crypto = $crypto; Util::connectHook('OC_User', 'post_login', $this, 'authenticate'); } @@ -116,7 +113,11 @@ class Store implements IStore { if ($trySession && $this->session->exists('login_credentials')) { /** @var array $creds */ $creds = json_decode($this->session->get('login_credentials'), true); - $creds['password'] = $this->crypto->decrypt($creds['password']); + try { + $creds['password'] = $this->crypto->decrypt($creds['password']); + } catch (Exception $e) { + //decryption failed, continue with old password as it is + } return new Credentials( $creds['uid'], $creds['loginName'] ?? $this->session->get('loginname') ?? $creds['uid'], // Pre 20 didn't have a loginName property, hence fall back to the session value and then to the UID -- cgit v1.2.3 From 64d7677888d43cdf14167188122cd21dc4118284 Mon Sep 17 00:00:00 2001 From: yemkareems Date: Mon, 28 Oct 2024 16:43:24 +0530 Subject: fix: crypto made inline for constructor and decrypt error handled in exception Signed-off-by: yemkareems [skip ci] --- lib/private/Authentication/LoginCredentials/Store.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/private/Authentication/LoginCredentials/Store.php b/lib/private/Authentication/LoginCredentials/Store.php index 83a69eb4937..c422927ee63 100644 --- a/lib/private/Authentication/LoginCredentials/Store.php +++ b/lib/private/Authentication/LoginCredentials/Store.php @@ -49,10 +49,12 @@ class Store implements IStore { /** @var IProvider|null */ private $tokenProvider; - public function __construct(ISession $session, + public function __construct( + ISession $session, LoggerInterface $logger, private readonly ICrypto $crypto, - ?IProvider $tokenProvider = null) { + ?IProvider $tokenProvider = null, + ) { $this->session = $session; $this->logger = $logger; $this->tokenProvider = $tokenProvider; -- cgit v1.2.3 From de1460194424a2f43e36576d4b78930eb0ef372a Mon Sep 17 00:00:00 2001 From: yemkareems Date: Mon, 28 Oct 2024 18:47:05 +0530 Subject: fix: crypto added manually as expected Signed-off-by: yemkareems --- tests/lib/Authentication/LoginCredentials/StoreTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/Authentication/LoginCredentials/StoreTest.php b/tests/lib/Authentication/LoginCredentials/StoreTest.php index ba3dbf78886..8c1dd2272b2 100644 --- a/tests/lib/Authentication/LoginCredentials/StoreTest.php +++ b/tests/lib/Authentication/LoginCredentials/StoreTest.php @@ -89,7 +89,7 @@ class StoreTest extends TestCase { } public function testGetLoginCredentialsNoTokenProvider() { - $this->store = new Store($this->session, $this->logger, null); + $this->store = new Store($this->session, $this->logger, $this->crypto, null); $this->expectException(CredentialsUnavailableException::class); -- cgit v1.2.3 From b56692d002eb6e4d6acc876238495cd3a87c2c02 Mon Sep 17 00:00:00 2001 From: yemkareems Date: Mon, 28 Oct 2024 19:09:04 +0530 Subject: fix: readonly removed Signed-off-by: yemkareems --- lib/private/Authentication/LoginCredentials/Store.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Authentication/LoginCredentials/Store.php b/lib/private/Authentication/LoginCredentials/Store.php index c422927ee63..b354611be26 100644 --- a/lib/private/Authentication/LoginCredentials/Store.php +++ b/lib/private/Authentication/LoginCredentials/Store.php @@ -52,7 +52,7 @@ class Store implements IStore { public function __construct( ISession $session, LoggerInterface $logger, - private readonly ICrypto $crypto, + private ICrypto $crypto, ?IProvider $tokenProvider = null, ) { $this->session = $session; -- cgit v1.2.3