From 2bbccbd3b5ad905ca59751260c2912e22229ff35 Mon Sep 17 00:00:00 2001 From: Yuri Kuznetsov Date: Sat, 18 Nov 2023 10:29:31 +0200 Subject: [PATCH] ref --- .../TwoFactor/Email/EmailLogin.php | 17 ++--- .../TwoFactor/Email/EmailUserSetup.php | 16 ++--- .../Authentication/TwoFactor/Email/Util.php | 71 +++++++++---------- .../Authentication/TwoFactor/Sms/SmsLogin.php | 16 ++--- .../TwoFactor/Sms/SmsUserSetup.php | 16 ++--- .../Authentication/TwoFactor/Sms/Util.php | 2 +- .../TwoFactor/Totp/TotpLogin.php | 19 ++--- .../TwoFactor/Totp/TotpUserSetup.php | 33 ++++----- .../Authentication/TwoFactor/Totp/Util.php | 9 ++- .../Authentication/TwoFactor/UserSetup.php | 3 + .../TwoFactor/UserSetupFactory.php | 15 ++-- .../Espo/Tools/UserSecurity/Service.php | 4 +- 12 files changed, 96 insertions(+), 125 deletions(-) diff --git a/application/Espo/Core/Authentication/TwoFactor/Email/EmailLogin.php b/application/Espo/Core/Authentication/TwoFactor/Email/EmailLogin.php index 920b8e3cf6..1c903866bd 100644 --- a/application/Espo/Core/Authentication/TwoFactor/Email/EmailLogin.php +++ b/application/Espo/Core/Authentication/TwoFactor/Email/EmailLogin.php @@ -30,17 +30,13 @@ namespace Espo\Core\Authentication\TwoFactor\Email; use Espo\ORM\EntityManager; - use Espo\Entities\User; use Espo\Entities\UserData; - use Espo\Repositories\UserData as UserDataRepository; - use Espo\Core\Authentication\TwoFactor\Login; use Espo\Core\Authentication\Result; use Espo\Core\Authentication\Result\Data as ResultData; use Espo\Core\Authentication\Result\FailReason; - use Espo\Core\Api\Request; use RuntimeException; @@ -49,14 +45,10 @@ class EmailLogin implements Login { public const NAME = 'Email'; - private EntityManager $entityManager; - private Util $util; - - public function __construct(EntityManager $entityManager, Util $util) - { - $this->entityManager = $entityManager; - $this->util = $util; - } + public function __construct( + private EntityManager $entityManager, + private Util $util + ) {} public function login(Result $result, Request $request): Result { @@ -69,6 +61,7 @@ class EmailLogin implements Login } if (!$code) { + // @todo Catch errors, return fail with a corresponding reason. Introduce internal exceptions. $this->util->sendCode($user); return Result::secondStepRequired($user, $this->getResultData()); diff --git a/application/Espo/Core/Authentication/TwoFactor/Email/EmailUserSetup.php b/application/Espo/Core/Authentication/TwoFactor/Email/EmailUserSetup.php index f562a168c1..15eb4d7dde 100644 --- a/application/Espo/Core/Authentication/TwoFactor/Email/EmailUserSetup.php +++ b/application/Espo/Core/Authentication/TwoFactor/Email/EmailUserSetup.php @@ -29,22 +29,20 @@ namespace Espo\Core\Authentication\TwoFactor\Email; +use Espo\Core\Exceptions\BadRequest; use Espo\Entities\User; - use Espo\Core\Authentication\TwoFactor\UserSetup; -use Espo\Core\Exceptions\Error; - use stdClass; +/** + * @noinspection PhpUnused + */ class EmailUserSetup implements UserSetup { - private $util; - public function __construct(Util $util) - { - $this->util = $util; - } + public function __construct(private Util $util) + {} public function getData(User $user): stdClass { @@ -58,7 +56,7 @@ class EmailUserSetup implements UserSetup $code = $payloadData->code ?? null; if ($code === null) { - throw new Error("No code."); + throw new BadRequest("No code."); } $codeModified = str_replace(' ', '', trim($code)); diff --git a/application/Espo/Core/Authentication/TwoFactor/Email/Util.php b/application/Espo/Core/Authentication/TwoFactor/Email/Util.php index e4165fe57c..612935415b 100644 --- a/application/Espo/Core/Authentication/TwoFactor/Email/Util.php +++ b/application/Espo/Core/Authentication/TwoFactor/Email/Util.php @@ -31,24 +31,23 @@ namespace Espo\Core\Authentication\TwoFactor\Email; use Espo\Core\Exceptions\Error; use Espo\Core\Exceptions\Forbidden; - +use Espo\Core\Mail\Exceptions\SendingError; use Espo\Core\Utils\Config; use Espo\Core\Mail\EmailSender; use Espo\Core\Mail\EmailFactory; use Espo\Core\Utils\TemplateFileManager; use Espo\Core\Htmlizer\HtmlizerFactory; use Espo\Core\Field\DateTime; - use Espo\ORM\EntityManager; use Espo\ORM\Query\Part\Condition as Cond; - use Espo\Entities\User; use Espo\Entities\Email; use Espo\Entities\TwoFactorCode; use Espo\Entities\UserData; - use Espo\Repositories\UserData as UserDataRepository; +use RuntimeException; + use const STR_PAD_LEFT; class Util @@ -58,7 +57,7 @@ class Util */ private const CODE_LIFETIME_PERIOD = '10 minutes'; - /* + /** * A max number of attempts to try a single code. */ private const CODE_ATTEMPTS_COUNT = 5; @@ -78,37 +77,18 @@ class Util */ private const CODE_LIMIT_PERIOD = '10 minutes'; - /** - * @var EntityManager - */ - private $entityManager; - - private $config; - - private $emailSender; - - private $templateFileManager; - - private $htmlizerFactory; - - private $emailFactory; - public function __construct( - EntityManager $entityManager, - Config $config, - EmailSender $emailSender, - TemplateFileManager $templateFileManager, - HtmlizerFactory $htmlizerFactory, - EmailFactory $emailFactory - ) { - $this->entityManager = $entityManager; - $this->config = $config; - $this->emailSender = $emailSender; - $this->templateFileManager = $templateFileManager; - $this->htmlizerFactory = $htmlizerFactory; - $this->emailFactory = $emailFactory; - } + private EntityManager $entityManager, + private Config $config, + private EmailSender $emailSender, + private TemplateFileManager $templateFileManager, + private HtmlizerFactory $htmlizerFactory, + private EmailFactory $emailFactory + ) {} + /** + * @throws Forbidden + */ public function storeEmailAddress(User $user, string $emailAddress): void { $this->checkEmailAddressIsUsers($user, $emailAddress); @@ -116,7 +96,7 @@ class Util $userData = $this->getUserDataRepository()->getByUserId($user->getId()); if (!$userData) { - throw new Error("UserData not found."); + throw new RuntimeException("UserData not found."); } $userData->set('auth2FAEmailAddress', $emailAddress); @@ -156,6 +136,11 @@ class Util return true; } + /** + * @throws SendingError + * @throws Forbidden + * @throws Error + */ public function sendCode(User $user, ?string $emailAddress = null): void { if ($emailAddress === null) { @@ -201,12 +186,15 @@ class Util ->findOne(); } + /** + * @throws Error + */ private function getEmailAddress(User $user): string { $userData = $this->getUserDataRepository()->getByUserId($user->getId()); if (!$userData) { - throw new Error("UserData not found."); + throw new RuntimeException("UserData not found."); } $emailAddress = $userData->get('auth2FAEmailAddress'); @@ -223,6 +211,9 @@ class Util return $user->getEmailAddressGroup()->getPrimaryAddress(); } + /** + * @throws Forbidden + */ private function checkEmailAddressIsUsers(User $user, string $emailAddress): void { $userAddressList = array_map( @@ -237,6 +228,9 @@ class Util } } + /** + * @throws Forbidden + */ private function checkCodeLimit(User $user): void { $limit = $this->config->get('auth2FAEmailCodeLimit') ?? self::CODE_LIMIT; @@ -269,6 +263,7 @@ class Util $max = pow(10, $codeLength) - 1; + /** @noinspection PhpUnhandledExceptionInspection */ return str_pad( (string) random_int(0, $max), $codeLength, @@ -308,7 +303,7 @@ class Util ->in(TwoFactorCode::ENTITY_TYPE) ->where([ 'userId' => $user->getId(), - 'method' => 'Email', + 'method' => EmailLogin::NAME, ]) ->set([ 'isActive' => false, @@ -325,7 +320,7 @@ class Util $this->entityManager->createEntity(TwoFactorCode::ENTITY_TYPE, [ 'code' => $code, 'userId' => $user->getId(), - 'method' => 'Email', + 'method' => EmailLogin::NAME, 'attemptsLeft' => $this->getCodeAttemptsCount(), ]); } diff --git a/application/Espo/Core/Authentication/TwoFactor/Sms/SmsLogin.php b/application/Espo/Core/Authentication/TwoFactor/Sms/SmsLogin.php index 3342b3548a..fc2b436028 100644 --- a/application/Espo/Core/Authentication/TwoFactor/Sms/SmsLogin.php +++ b/application/Espo/Core/Authentication/TwoFactor/Sms/SmsLogin.php @@ -30,12 +30,9 @@ namespace Espo\Core\Authentication\TwoFactor\Sms; use Espo\ORM\EntityManager; - use Espo\Entities\User; use Espo\Entities\UserData; - use Espo\Repositories\UserData as UserDataRepository; - use Espo\Core\Authentication\TwoFactor\Login; use Espo\Core\Authentication\Result; use Espo\Core\Authentication\Result\Data as ResultData; @@ -48,14 +45,10 @@ class SmsLogin implements Login { public const NAME = 'Sms'; - private EntityManager $entityManager; - private Util $util; - - public function __construct(EntityManager $entityManager, Util $util) - { - $this->entityManager = $entityManager; - $this->util = $util; - } + public function __construct( + private EntityManager $entityManager, + private Util $util + ) {} public function login(Result $result, Request $request): Result { @@ -74,6 +67,7 @@ class SmsLogin implements Login throw new RuntimeException("No user."); } + // @todo Catch errors, return fail with a corresponding reason. Introduce internal exceptions. $this->util->sendCode($user); return Result::secondStepRequired($user, $this->getResultData()); diff --git a/application/Espo/Core/Authentication/TwoFactor/Sms/SmsUserSetup.php b/application/Espo/Core/Authentication/TwoFactor/Sms/SmsUserSetup.php index 20bfc4ded4..2614800b57 100644 --- a/application/Espo/Core/Authentication/TwoFactor/Sms/SmsUserSetup.php +++ b/application/Espo/Core/Authentication/TwoFactor/Sms/SmsUserSetup.php @@ -29,21 +29,19 @@ namespace Espo\Core\Authentication\TwoFactor\Sms; +use Espo\Core\Exceptions\BadRequest; use Espo\Entities\User; - use Espo\Core\Authentication\TwoFactor\UserSetup; -use Espo\Core\Exceptions\Error; use stdClass; +/** + * @noinspection PhpUnused + */ class SmsUserSetup implements UserSetup { - private $util; - - public function __construct(Util $util) - { - $this->util = $util; - } + public function __construct(private Util $util) + {} public function getData(User $user): stdClass { @@ -57,7 +55,7 @@ class SmsUserSetup implements UserSetup $code = $payloadData->code ?? null; if ($code === null) { - throw new Error("No code."); + throw new BadRequest("No code."); } $codeModified = str_replace(' ', '', trim($code)); diff --git a/application/Espo/Core/Authentication/TwoFactor/Sms/Util.php b/application/Espo/Core/Authentication/TwoFactor/Sms/Util.php index 8e2c2ec418..e157ce41c5 100644 --- a/application/Espo/Core/Authentication/TwoFactor/Sms/Util.php +++ b/application/Espo/Core/Authentication/TwoFactor/Sms/Util.php @@ -50,7 +50,7 @@ use const STR_PAD_LEFT; class Util { - private const METHOD = 'Sms'; + private const METHOD = SmsLogin::NAME; /** * A lifetime of a code. diff --git a/application/Espo/Core/Authentication/TwoFactor/Totp/TotpLogin.php b/application/Espo/Core/Authentication/TwoFactor/Totp/TotpLogin.php index d81880f02c..a830a586e2 100644 --- a/application/Espo/Core/Authentication/TwoFactor/Totp/TotpLogin.php +++ b/application/Espo/Core/Authentication/TwoFactor/Totp/TotpLogin.php @@ -30,33 +30,28 @@ namespace Espo\Core\Authentication\TwoFactor\Totp; use Espo\ORM\EntityManager; - use Espo\Entities\User; use Espo\Entities\UserData; - use Espo\Repositories\UserData as UserDataRepository; - use Espo\Core\Authentication\TwoFactor\Login; use Espo\Core\Authentication\Result; use Espo\Core\Authentication\Result\Data as ResultData; use Espo\Core\Authentication\Result\FailReason; - use Espo\Core\Api\Request; use RuntimeException; +/** + * @noinspection PhpUnused + */ class TotpLogin implements Login { public const NAME = 'Totp'; - private EntityManager $entityManager; - private Util $totp; - - public function __construct(EntityManager $entityManager, Util $totp) - { - $this->entityManager = $entityManager; - $this->totp = $totp; - } + public function __construct( + private EntityManager $entityManager, + private Util $totp + ) {} public function login(Result $result, Request $request): Result { diff --git a/application/Espo/Core/Authentication/TwoFactor/Totp/TotpUserSetup.php b/application/Espo/Core/Authentication/TwoFactor/Totp/TotpUserSetup.php index bdf944b442..de10c7ca33 100644 --- a/application/Espo/Core/Authentication/TwoFactor/Totp/TotpUserSetup.php +++ b/application/Espo/Core/Authentication/TwoFactor/Totp/TotpUserSetup.php @@ -29,34 +29,27 @@ namespace Espo\Core\Authentication\TwoFactor\Totp; +use Espo\Core\Exceptions\BadRequest; use Espo\Entities\UserData; use Espo\Entities\User; - use Espo\Repositories\UserData as UserDataRepository; - use Espo\ORM\EntityManager; - use Espo\Core\Authentication\TwoFactor\UserSetup; use Espo\Core\Utils\Config; -use Espo\Core\Exceptions\Error; - +use RuntimeException; use stdClass; +/** + * @noinspection PhpUnused + */ class TotpUserSetup implements UserSetup { - private $totp; - - private $config; - - private $entityManager; - - public function __construct(Util $totp, Config $config, EntityManager $entityManager) - { - $this->totp = $totp; - $this->config = $config; - $this->entityManager = $entityManager; - } + public function __construct( + private Util $totp, + private Config $config, + private EntityManager $entityManager + ) {} public function getData(User $user): stdClass { @@ -79,7 +72,7 @@ class TotpUserSetup implements UserSetup $code = $payloadData->code ?? null; if ($code === null) { - throw new Error("No code."); + throw new BadRequest("No code."); } $codeModified = str_replace(' ', '', trim($code)); @@ -91,7 +84,7 @@ class TotpUserSetup implements UserSetup $userData = $this->getUserDataRepository()->getByUserId($user->getId()); if (!$userData) { - throw new Error("User not found."); + throw new RuntimeException("User not found."); } $secret = $userData->get('auth2FATotpSecret'); @@ -104,7 +97,7 @@ class TotpUserSetup implements UserSetup $userData = $this->getUserDataRepository()->getByUserId($user->getId()); if (!$userData) { - throw new Error(); + throw new RuntimeException(); } $userData->set('auth2FATotpSecret', $secret); diff --git a/application/Espo/Core/Authentication/TwoFactor/Totp/Util.php b/application/Espo/Core/Authentication/TwoFactor/Totp/Util.php index 12914fef0f..fee892797c 100644 --- a/application/Espo/Core/Authentication/TwoFactor/Totp/Util.php +++ b/application/Espo/Core/Authentication/TwoFactor/Totp/Util.php @@ -30,6 +30,8 @@ namespace Espo\Core\Authentication\TwoFactor\Totp; use RobThree\Auth\TwoFactorAuth; +use RobThree\Auth\TwoFactorAuthException; +use RuntimeException; class Util { @@ -44,6 +46,11 @@ class Util { $impl = new TwoFactorAuth(); - return $impl->createSecret(); + try { + return $impl->createSecret(); + } + catch (TwoFactorAuthException $e) { + throw new RuntimeException($e->getMessage()); + } } } diff --git a/application/Espo/Core/Authentication/TwoFactor/UserSetup.php b/application/Espo/Core/Authentication/TwoFactor/UserSetup.php index 98ea39fc9f..d796110012 100644 --- a/application/Espo/Core/Authentication/TwoFactor/UserSetup.php +++ b/application/Espo/Core/Authentication/TwoFactor/UserSetup.php @@ -29,6 +29,7 @@ namespace Espo\Core\Authentication\TwoFactor; +use Espo\Core\Exceptions\BadRequest; use Espo\Entities\User; use stdClass; @@ -45,6 +46,8 @@ interface UserSetup /** * Verify input data before making 2FA enabled for a user. + * + * @throws BadRequest */ public function verifyData(User $user, stdClass $payloadData): bool; } diff --git a/application/Espo/Core/Authentication/TwoFactor/UserSetupFactory.php b/application/Espo/Core/Authentication/TwoFactor/UserSetupFactory.php index c97ab8236e..4ad8dffa84 100644 --- a/application/Espo/Core/Authentication/TwoFactor/UserSetupFactory.php +++ b/application/Espo/Core/Authentication/TwoFactor/UserSetupFactory.php @@ -32,18 +32,15 @@ namespace Espo\Core\Authentication\TwoFactor; use Espo\Core\InjectableFactory; use Espo\Core\Utils\Metadata; -use Espo\Core\Exceptions\Error; +use RuntimeException; class UserSetupFactory { - private InjectableFactory $injectableFactory; - private Metadata $metadata; - public function __construct(InjectableFactory $injectableFactory, Metadata $metadata) - { - $this->injectableFactory = $injectableFactory; - $this->metadata = $metadata; - } + public function __construct( + private InjectableFactory $injectableFactory, + private Metadata $metadata + ) {} public function create(string $method): UserSetup { @@ -51,7 +48,7 @@ class UserSetupFactory $className = $this->metadata->get(['app', 'authentication2FAMethods', $method, 'userSetupClassName']); if (!$className) { - throw new Error("No user-setup class for '{$method}'."); + throw new RuntimeException("No user-setup class for '$method'."); } return $this->injectableFactory->create($className); diff --git a/application/Espo/Tools/UserSecurity/Service.php b/application/Espo/Tools/UserSecurity/Service.php index 905cb9d65c..2a97facfca 100644 --- a/application/Espo/Tools/UserSecurity/Service.php +++ b/application/Espo/Tools/UserSecurity/Service.php @@ -29,7 +29,6 @@ namespace Espo\Tools\UserSecurity; -use Espo\Core\Exceptions\Error; use Espo\Core\Exceptions\Forbidden; use Espo\Core\Exceptions\NotFound; use Espo\Core\Exceptions\BadRequest; @@ -95,7 +94,6 @@ class Service /** * @throws BadRequest - * @throws Error * @throws Forbidden * @throws NotFound */ @@ -171,9 +169,9 @@ class Service } /** - * @throws Error * @throws Forbidden * @throws NotFound + * @throws BadRequest */ public function update(string $id, stdClass $data): stdClass {