From cfa2b20e8bebb48fb96b1ab9bb19ade464bb5f99 Mon Sep 17 00:00:00 2001 From: nuxsmin Date: Sun, 15 Jul 2018 20:39:11 +0200 Subject: [PATCH] * [ADD] Unit testing. Work in progress * [MOD] Code refactoring --- lib/SP/Core/Context/ContextInterface.php | 7 + lib/SP/Core/Context/SessionContext.php | 4 +- lib/SP/Core/Context/StatelessContext.php | 12 ++ lib/SP/Services/Account/AccountService.php | 8 +- lib/SP/Services/Api/ApiService.php | 3 +- .../Services/AuthToken/AuthTokenService.php | 10 +- .../Crypt/TemporaryMasterPassService.php | 11 +- lib/SP/Services/MailService.php | 6 +- lib/SP/Services/Service.php | 28 ++++ .../Services/Crypt/MasterPassServiceTest.php | 1 - .../Crypt/TemporaryMasterPassServiceTest.php | 127 ++++++++++++++++++ 11 files changed, 192 insertions(+), 25 deletions(-) create mode 100644 tests/Services/Crypt/TemporaryMasterPassServiceTest.php diff --git a/lib/SP/Core/Context/ContextInterface.php b/lib/SP/Core/Context/ContextInterface.php index 92603750..1b3f3b9e 100644 --- a/lib/SP/Core/Context/ContextInterface.php +++ b/lib/SP/Core/Context/ContextInterface.php @@ -182,4 +182,11 @@ interface ContextInterface * @return mixed */ public function getTrasientKey(string $key, $default = null); + + /** + * Sets a temporary master password + * + * @param string $password + */ + public function setTemporaryMasterPass(string $password); } \ No newline at end of file diff --git a/lib/SP/Core/Context/SessionContext.php b/lib/SP/Core/Context/SessionContext.php index 851ad7f4..06d6dad6 100644 --- a/lib/SP/Core/Context/SessionContext.php +++ b/lib/SP/Core/Context/SessionContext.php @@ -268,11 +268,11 @@ class SessionContext extends ContextBase } /** - * Establece la clave maestra temporal + * Sets a temporary master password * * @param string $password */ - public function setTemporaryMasterPass($password) + public function setTemporaryMasterPass(string $password) { $this->setContextKey('tempmasterpass', $password); } diff --git a/lib/SP/Core/Context/StatelessContext.php b/lib/SP/Core/Context/StatelessContext.php index cb38a2dd..9a78858e 100644 --- a/lib/SP/Core/Context/StatelessContext.php +++ b/lib/SP/Core/Context/StatelessContext.php @@ -257,4 +257,16 @@ class StatelessContext extends ContextBase { return null; } + + /** + * Sets a temporary master password + * + * @param string $password + * + * @throws ContextException + */ + public function setTemporaryMasterPass(string $password) + { + $this->setTrasientKey('_tempmasterpass', $password); + } } \ No newline at end of file diff --git a/lib/SP/Services/Account/AccountService.php b/lib/SP/Services/Account/AccountService.php index 98be6f53..063a8349 100644 --- a/lib/SP/Services/Account/AccountService.php +++ b/lib/SP/Services/Account/AccountService.php @@ -28,9 +28,7 @@ use Defuse\Crypto\Exception\CryptoException; use SP\Account\AccountRequest; use SP\Account\AccountSearchFilter; use SP\Account\AccountUtil; -use SP\Core\Context\SessionContext; use SP\Core\Crypt\Crypt; -use SP\Core\Crypt\Session as CryptSession; use SP\Core\Exceptions\QueryException; use SP\Core\Exceptions\SPException; use SP\DataModel\AccountData; @@ -220,11 +218,7 @@ class AccountService extends Service implements AccountServiceInterface { try { if ($masterPass === null) { - if ($this->context instanceof SessionContext) { - $masterPass = CryptSession::getSessionKey($this->context); - } else { - $masterPass = $this->context->getTrasientKey('_masterpass'); - } + $masterPass = $this->getMasterKeyFromContext(); } if (empty($masterPass)) { diff --git a/lib/SP/Services/Api/ApiService.php b/lib/SP/Services/Api/ApiService.php index 51645a69..ed2973c7 100644 --- a/lib/SP/Services/Api/ApiService.php +++ b/lib/SP/Services/Api/ApiService.php @@ -393,10 +393,11 @@ class ApiService extends Service /** * @return string + * @throws ServiceException */ public function getMasterPass() { - return $this->context->getTrasientKey('_masterpass'); + return $this->getMasterKeyFromContext(); } /** diff --git a/lib/SP/Services/AuthToken/AuthTokenService.php b/lib/SP/Services/AuthToken/AuthTokenService.php index 6cdefa47..f7f0bcad 100644 --- a/lib/SP/Services/AuthToken/AuthTokenService.php +++ b/lib/SP/Services/AuthToken/AuthTokenService.php @@ -27,7 +27,6 @@ namespace SP\Services\AuthToken; use SP\Core\Acl\Acl; use SP\Core\Acl\ActionsInterface; use SP\Core\Crypt\Hash; -use SP\Core\Crypt\Session as CryptSession; use SP\Core\Crypt\Vault; use SP\Core\Exceptions\SPException; use SP\DataModel\AuthTokenData; @@ -217,17 +216,12 @@ class AuthTokenService extends Service * @param string $key * * @return Vault + * @throws ServiceException * @throws \Defuse\Crypto\Exception\CryptoException */ private function getSecureData($token, $key) { - if (($mKey = $this->context->getTrasientKey('_masterpass')) !== null) { - return (new Vault()) - ->saveData($mKey, $key . $token); - } - - return (new Vault()) - ->saveData(CryptSession::getSessionKey($this->context), $key . $token); + return (new Vault())->saveData($this->getMasterKeyFromContext(), $key . $token); } /** diff --git a/lib/SP/Services/Crypt/TemporaryMasterPassService.php b/lib/SP/Services/Crypt/TemporaryMasterPassService.php index 510a8b49..1f45a91c 100644 --- a/lib/SP/Services/Crypt/TemporaryMasterPassService.php +++ b/lib/SP/Services/Crypt/TemporaryMasterPassService.php @@ -26,7 +26,6 @@ namespace SP\Services\Crypt; use SP\Core\Crypt\Crypt; use SP\Core\Crypt\Hash; -use SP\Core\Crypt\Session; use SP\Core\Events\Event; use SP\Core\Events\EventMessage; use SP\Core\Messages\MailMessage; @@ -86,7 +85,7 @@ class TemporaryMasterPassService extends Service $secureKey = Crypt::makeSecuredKey($randomKey); $configRequest = new ConfigRequest(); - $configRequest->add(self::PARAM_PASS, Crypt::encrypt(Session::getSessionKey($this->context), $secureKey, $randomKey)); + $configRequest->add(self::PARAM_PASS, Crypt::encrypt($this->getMasterKeyFromContext(), $secureKey, $randomKey)); $configRequest->add(self::PARAM_KEY, $secureKey); $configRequest->add(self::PARAM_HASH, Hash::hashKey($randomKey)); $configRequest->add(self::PARAM_TIME, time()); @@ -123,9 +122,7 @@ class TemporaryMasterPassService extends Service { try { $isValid = false; - $passTime = (int)$this->configService->getByParam(self::PARAM_TIME); $passMaxTime = (int)$this->configService->getByParam(self::PARAM_MAX_TIME); - $attempts = (int)$this->configService->getByParam(self::PARAM_ATTEMPTS); // Comprobar si el tiempo de validez o los intentos se han superado if ($passMaxTime === 0) { @@ -136,6 +133,9 @@ class TemporaryMasterPassService extends Service return $isValid; } + $passTime = (int)$this->configService->getByParam(self::PARAM_TIME); + $attempts = (int)$this->configService->getByParam(self::PARAM_ATTEMPTS); + if ((!empty($passTime) && time() > $passMaxTime) || $attempts >= self::MAX_ATTEMPTS ) { @@ -197,7 +197,8 @@ class TemporaryMasterPassService extends Service return $value->email; }, $this->dic->get(UserService::class)->getUserEmailForGroup($groupId)); - $this->dic->get(MailService::class)->sendBatch($mailMessage->getTitle(), $emails, $mailMessage); + $this->dic->get(MailService::class) + ->sendBatch($mailMessage->getTitle(), $emails, $mailMessage); } /** diff --git a/lib/SP/Services/MailService.php b/lib/SP/Services/MailService.php index aee26bc9..29f1cd2d 100644 --- a/lib/SP/Services/MailService.php +++ b/lib/SP/Services/MailService.php @@ -56,6 +56,7 @@ class MailService extends Service * * @param MailParams $mailParams * @param string $to + * * @throws ServiceException */ public function check(MailParams $mailParams, $to) @@ -104,6 +105,7 @@ class MailService extends Service /** * @param $action + * * @return string */ protected function getSubjectForAction($action) @@ -115,6 +117,7 @@ class MailService extends Service * @param string $subject * @param string $to * @param MailMessage $mailMessage + * * @throws ServiceException */ public function send($subject, $to, MailMessage $mailMessage) @@ -134,7 +137,7 @@ class MailService extends Service { try { $this->mailer->send(); - + $this->eventDispatcher->notifyEvent('send.mail', new Event($this, EventMessage::factory() ->addDescription(__u('Correo enviado')) @@ -155,6 +158,7 @@ class MailService extends Service * @param string $subject * @param array $to * @param MailMessage $mailMessage + * * @throws ServiceException */ public function sendBatch($subject, array $to, MailMessage $mailMessage) diff --git a/lib/SP/Services/Service.php b/lib/SP/Services/Service.php index 7f569d74..fff2b069 100644 --- a/lib/SP/Services/Service.php +++ b/lib/SP/Services/Service.php @@ -24,10 +24,13 @@ namespace SP\Services; +use Defuse\Crypto\Exception\CryptoException; use DI\Container; use Psr\Container\ContainerInterface; use SP\Config\Config; use SP\Core\Context\ContextInterface; +use SP\Core\Context\SessionContext; +use SP\Core\Crypt\Session; use SP\Core\Events\Event; use SP\Core\Events\EventDispatcher; use SP\Core\Events\EventMessage; @@ -115,4 +118,29 @@ abstract class Service throw new ServiceException(__u('No es posible iniciar una transacción')); } } + + /** + * @return string + * @throws ServiceException + */ + protected final function getMasterKeyFromContext(): String + { + try { + if ($this->context instanceof SessionContext) { + $key = Session::getSessionKey($this->context); + } else { + $key = $this->context->getTrasientKey('_masterpass'); + } + + if (empty($key)) { + throw new ServiceException(__u('Error ol obtener la clave maestra del contexto')); + } + + return $key; + } catch (CryptoException $e) { + debugLog($e->getMessage()); + + throw new ServiceException(__u('Error ol obtener la clave maestra del contexto')); + } + } } \ No newline at end of file diff --git a/tests/Services/Crypt/MasterPassServiceTest.php b/tests/Services/Crypt/MasterPassServiceTest.php index 678cd513..579aa9e4 100644 --- a/tests/Services/Crypt/MasterPassServiceTest.php +++ b/tests/Services/Crypt/MasterPassServiceTest.php @@ -72,7 +72,6 @@ class MasterPassServiceTest extends DatabaseTestCase self::$service = $dic->get(MasterPassService::class); self::$accountService = $dic->get(AccountService::class); self::$customFieldService = $dic->get(CustomFieldService::class); - } /** diff --git a/tests/Services/Crypt/TemporaryMasterPassServiceTest.php b/tests/Services/Crypt/TemporaryMasterPassServiceTest.php new file mode 100644 index 00000000..19a06446 --- /dev/null +++ b/tests/Services/Crypt/TemporaryMasterPassServiceTest.php @@ -0,0 +1,127 @@ +. + */ + +namespace SP\Tests\Services\Crypt; + +use Defuse\Crypto\Exception\CryptoException; +use PHPUnit\Framework\TestCase; +use SP\Core\Context\ContextInterface; +use SP\Services\Crypt\TemporaryMasterPassService; +use function SP\Tests\setupContext; + +/** + * Class TemporaryMasterPassServiceTest + * + * @package SP\Tests\Services\Crypt + */ +class TemporaryMasterPassServiceTest extends TestCase +{ + /** + * @var ContextInterface + */ + private $context; + /** + * @var TemporaryMasterPassService + */ + private $service; + + /** + * @throws \DI\NotFoundException + * @throws \SP\Core\Context\ContextException + * @throws \DI\DependencyException + */ + public function setUp() + { + $dic = setupContext(); + + // Inicializar el repositorio + $this->service = $dic->get(TemporaryMasterPassService::class); + + $this->context = $dic->get(ContextInterface::class); + } + + /** + * @throws \SP\Services\ServiceException + */ + public function testCreate() + { + $key = $this->service->create(); + + $this->assertNotEmpty($key); + $this->assertEquals($this->context->getTrasientKey('_tempmasterpass'), $key); + + return $key; + } + + /** + * @depends testCreate + * + * @param $key + * + * @throws \Defuse\Crypto\Exception\CryptoException + * @throws \SP\Repositories\NoSuchItemException + * @throws \SP\Services\ServiceException + */ + public function testGetUsingKey($key) + { + $this->assertEquals('12345678900', $this->service->getUsingKey($key)); + + $this->expectException(CryptoException::class); + + $this->service->getUsingKey('test123'); + } + + /** + * @depends testCreate + * + * @param $key + * + * @throws \SP\Services\ServiceException + */ + public function testCheckTempMasterPass($key) + { + $this->assertTrue($this->service->checkTempMasterPass($key)); + + for ($i = 1; $i <= 50; $i++) { + $this->assertFalse($this->service->checkTempMasterPass('test123')); + } + + // The 50's attempt should fails + $this->assertFalse($this->service->checkTempMasterPass($key)); + } + + /** + * @throws \SP\Services\ServiceException + */ + public function testExpiredKey() + { + $key = $this->service->create(10); + + print 'Sleeping for 12 seconds'; + + sleep(12); + + $this->assertFalse($this->service->checkTempMasterPass($key)); + } +}