From 3a201143cb7b4033f11a2050120d4bd744c160eb Mon Sep 17 00:00:00 2001 From: Denys Sokolov Date: Fri, 24 Nov 2023 23:16:26 +0100 Subject: [PATCH] CC-26451: Password reset improvement (#10628) CC-26451 Fixed Insecure Password Reset Workflow --- architecture-baseline.json | 21 ----- .../PasswordResetExpirationChecker.php | 71 ++++++++++++++++ ...asswordResetExpirationCheckerInterface.php | 27 ++++++ .../Customer/Business/Customer/Customer.php | 31 ++++--- .../Business/CustomerBusinessFactory.php | 13 +++ .../Communication/Form/AddressForm.php | 10 +-- .../Communication/Form/CustomerForm.php | 21 ++--- .../Communication/Form/CustomerUpdateForm.php | 8 +- src/Spryker/Zed/Customer/CustomerConfig.php | 43 ++++++++++ .../Business/Facade/RestorePasswordTest.php | 82 +++++++++++++++++-- 10 files changed, 269 insertions(+), 58 deletions(-) create mode 100644 src/Spryker/Zed/Customer/Business/Customer/Checker/PasswordResetExpirationChecker.php create mode 100644 src/Spryker/Zed/Customer/Business/Customer/Checker/PasswordResetExpirationCheckerInterface.php diff --git a/architecture-baseline.json b/architecture-baseline.json index fecb64b0..df19640f 100644 --- a/architecture-baseline.json +++ b/architecture-baseline.json @@ -1,25 +1,4 @@ [ - { - "fileName": "src/Spryker/Zed/Customer/Communication/Form/AddressForm.php", - "description": "Method `Spryker\\Zed\\Customer\\Communication\\Form\\AddressForm::getBlockPrefix()` must have return type.", - "rule": "ExternalMethodExtensionReturnTypeRule", - "ruleset": "Spryker", - "priority": "1" - }, - { - "fileName": "src/Spryker/Zed/Customer/Communication/Form/CustomerForm.php", - "description": "Method `Spryker\\Zed\\Customer\\Communication\\Form\\CustomerForm::getBlockPrefix()` must have return type.", - "rule": "ExternalMethodExtensionReturnTypeRule", - "ruleset": "Spryker", - "priority": "1" - }, - { - "fileName": "src/Spryker/Zed/Customer/Communication/Form/CustomerUpdateForm.php", - "description": "Method `Spryker\\Zed\\Customer\\Communication\\Form\\CustomerUpdateForm::getBlockPrefix()` must have return type.", - "rule": "ExternalMethodExtensionReturnTypeRule", - "ruleset": "Spryker", - "priority": "1" - }, { "fileName": "src/Spryker/Zed/Customer/Dependency/Facade/CustomerToCountryBridge.php", "description": "Bridges: Method `getPreferredCountryByName()` must have `public function getCollection(CriteriaTransfer): CollectionTransfer` signature.", diff --git a/src/Spryker/Zed/Customer/Business/Customer/Checker/PasswordResetExpirationChecker.php b/src/Spryker/Zed/Customer/Business/Customer/Checker/PasswordResetExpirationChecker.php new file mode 100644 index 00000000..20692101 --- /dev/null +++ b/src/Spryker/Zed/Customer/Business/Customer/Checker/PasswordResetExpirationChecker.php @@ -0,0 +1,71 @@ +customerConfig = $customerConfig; + } + + /** + * @param \Orm\Zed\Customer\Persistence\SpyCustomer$customerEntity + * @param \Generated\Shared\Transfer\CustomerResponseTransfer $customerResponseTransfer + * + * @return \Generated\Shared\Transfer\CustomerResponseTransfer + */ + public function checkPasswordResetExpiration( + SpyCustomer $customerEntity, + CustomerResponseTransfer $customerResponseTransfer + ): CustomerResponseTransfer { + if (!$this->customerConfig->isCustomerPasswordResetExpirationEnabled()) { + return $customerResponseTransfer + ->setIsSuccess(true); + } + + /** @var \DateTime|string|null $restorePasswordDate */ + $restorePasswordDate = $customerEntity->getRestorePasswordDate(); + + if (!$restorePasswordDate) { + return $customerResponseTransfer; + } + + if (is_string($restorePasswordDate)) { + $restorePasswordDate = new DateTime($restorePasswordDate); + } + + $expirationDate = clone $restorePasswordDate; + $expirationDate->modify($this->customerConfig->getCustomerPasswordResetExpirationPeriod()); + $now = new DateTime(); + + if ($now < $expirationDate) { + return $customerResponseTransfer; + } + + $customerErrorTransfer = (new CustomerErrorTransfer())->setMessage(CustomerConfig::GLOSSARY_KEY_CONFIRM_EMAIL_LINK_INVALID_OR_USED); + + return $customerResponseTransfer + ->setIsSuccess(false) + ->addError($customerErrorTransfer); + } +} diff --git a/src/Spryker/Zed/Customer/Business/Customer/Checker/PasswordResetExpirationCheckerInterface.php b/src/Spryker/Zed/Customer/Business/Customer/Checker/PasswordResetExpirationCheckerInterface.php new file mode 100644 index 00000000..59b95db0 --- /dev/null +++ b/src/Spryker/Zed/Customer/Business/Customer/Checker/PasswordResetExpirationCheckerInterface.php @@ -0,0 +1,27 @@ + $postCustomerRegistrationPlugins */ public function __construct( @@ -137,6 +138,7 @@ public function __construct( CustomerToLocaleInterface $localeFacade, CustomerExpanderInterface $customerExpander, CustomerPasswordPolicyValidatorInterface $customerPasswordPolicyValidator, + PasswordResetExpirationCheckerInterface $passwordResetExpirationChecker, array $postCustomerRegistrationPlugins = [] ) { $this->queryContainer = $queryContainer; @@ -149,6 +151,7 @@ public function __construct( $this->customerPasswordPolicyValidator = $customerPasswordPolicyValidator; $this->postCustomerRegistrationPlugins = $postCustomerRegistrationPlugins; $this->localeFacade = $localeFacade; + $this->passwordResetExpirationChecker = $passwordResetExpirationChecker; } /** @@ -434,7 +437,7 @@ public function confirmCustomerRegistration(CustomerTransfer $customerTransfer): if (!$customerEntity) { return $customerResponseTransfer ->setIsSuccess(false) - ->addError((new CustomerErrorTransfer())->setMessage(static::GLOSSARY_KEY_CONFIRM_EMAIL_LINK_INVALID_OR_USED)); + ->addError((new CustomerErrorTransfer())->setMessage(CustomerConfig::GLOSSARY_KEY_CONFIRM_EMAIL_LINK_INVALID_OR_USED)); } $customerEntity->setRegistered(new DateTime()); @@ -505,6 +508,14 @@ public function restorePassword(CustomerTransfer $customerTransfer) return $customerResponseTransfer; } + $customerResponseTransfer = $this + ->passwordResetExpirationChecker + ->checkPasswordResetExpiration($customerEntity, $customerResponseTransfer); + + if (!$customerResponseTransfer->getIsSuccess()) { + return $customerResponseTransfer; + } + $customerEntity->setRestorePasswordDate(null); $customerEntity->setRestorePasswordKey(null); @@ -887,11 +898,7 @@ protected function getEncodedPassword($currentPassword) */ protected function getPasswordEncoder(): PasswordEncoderInterface { - if (class_exists(NativePasswordEncoder::class)) { - return new NativePasswordEncoder(null, null, static::BCRYPT_FACTOR); - } - - return new BCryptPasswordEncoder(static::BCRYPT_FACTOR); + return new NativePasswordEncoder(null, null, static::BCRYPT_FACTOR); } /** diff --git a/src/Spryker/Zed/Customer/Business/CustomerBusinessFactory.php b/src/Spryker/Zed/Customer/Business/CustomerBusinessFactory.php index 0bee256f..7d283a67 100644 --- a/src/Spryker/Zed/Customer/Business/CustomerBusinessFactory.php +++ b/src/Spryker/Zed/Customer/Business/CustomerBusinessFactory.php @@ -14,6 +14,8 @@ use Spryker\Zed\Customer\Business\Checkout\CustomerOrderSaverInterface; use Spryker\Zed\Customer\Business\Checkout\CustomerOrderSaverWithMultiShippingAddress; use Spryker\Zed\Customer\Business\Customer\Address; +use Spryker\Zed\Customer\Business\Customer\Checker\PasswordResetExpirationChecker; +use Spryker\Zed\Customer\Business\Customer\Checker\PasswordResetExpirationCheckerInterface; use Spryker\Zed\Customer\Business\Customer\Customer; use Spryker\Zed\Customer\Business\Customer\CustomerReader; use Spryker\Zed\Customer\Business\Customer\CustomerReaderInterface; @@ -66,12 +68,23 @@ public function createCustomer() $this->getLocaleFacade(), $this->createCustomerExpander(), $this->createCustomerPasswordPolicyValidator(), + $this->createPasswordResetExpirationChecker(), $this->getPostCustomerRegistrationPlugins(), ); return $customer; } + /** + * @return \Spryker\Zed\Customer\Business\Customer\Checker\PasswordResetExpirationCheckerInterface + */ + public function createPasswordResetExpirationChecker(): PasswordResetExpirationCheckerInterface + { + return new PasswordResetExpirationChecker( + $this->getConfig(), + ); + } + /** * @return \Spryker\Zed\Customer\Business\Customer\CustomerReaderInterface */ diff --git a/src/Spryker/Zed/Customer/Communication/Form/AddressForm.php b/src/Spryker/Zed/Customer/Communication/Form/AddressForm.php index 0fe4e1d7..ea2a288e 100644 --- a/src/Spryker/Zed/Customer/Communication/Form/AddressForm.php +++ b/src/Spryker/Zed/Customer/Communication/Form/AddressForm.php @@ -181,7 +181,7 @@ protected function addFkCustomerField(FormBuilderInterface $builder) /** * @param \Symfony\Component\Form\FormBuilderInterface $builder - * @param array $choices + * @param array $choices * * @return $this */ @@ -331,8 +331,8 @@ protected function addZipCodeField(FormBuilderInterface $builder) /** * @param \Symfony\Component\Form\FormBuilderInterface $builder - * @param array $choices - * @param array $preferredChoices + * @param array $choices + * @param array $preferredChoices * * @return $this */ @@ -444,7 +444,7 @@ protected function createLastNameRegexConstraint(): Regex /** * @return string */ - public function getBlockPrefix() + public function getBlockPrefix(): string { return 'customer_address'; } @@ -454,7 +454,7 @@ public function getBlockPrefix() * * @return string */ - public function getName() + public function getName(): string { return $this->getBlockPrefix(); } diff --git a/src/Spryker/Zed/Customer/Communication/Form/CustomerForm.php b/src/Spryker/Zed/Customer/Communication/Form/CustomerForm.php index 3dfaa8ed..7560076a 100644 --- a/src/Spryker/Zed/Customer/Communication/Form/CustomerForm.php +++ b/src/Spryker/Zed/Customer/Communication/Form/CustomerForm.php @@ -19,6 +19,7 @@ use Symfony\Component\Form\Extension\Core\Type\TextType; use Symfony\Component\Form\FormBuilderInterface; use Symfony\Component\OptionsResolver\OptionsResolver; +use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\Constraints\Callback; use Symfony\Component\Validator\Constraints\Email; use Symfony\Component\Validator\Constraints\Length; @@ -118,7 +119,7 @@ class CustomerForm extends AbstractType /** * @return string */ - public function getBlockPrefix() + public function getBlockPrefix(): string { return 'customer'; } @@ -188,7 +189,7 @@ protected function addEmailField(FormBuilderInterface $builder) /** * @param \Symfony\Component\Form\FormBuilderInterface $builder - * @param array $choices + * @param array $choices * * @return $this */ @@ -246,7 +247,7 @@ protected function addLastNameField(FormBuilderInterface $builder) /** * @param \Symfony\Component\Form\FormBuilderInterface $builder - * @param array $choices + * @param array $choices * * @return $this */ @@ -319,7 +320,7 @@ protected function addPhoneField(FormBuilderInterface $builder) /** * @param \Symfony\Component\Form\FormBuilderInterface $builder - * @param array $choices + * @param array $choices * * @return $this */ @@ -351,7 +352,7 @@ protected function addStoreField(FormBuilderInterface $builder, array $choices) /** * @param \Symfony\Component\Form\FormBuilderInterface $builder - * @param array $choices + * @param array $choices * * @return $this */ @@ -394,9 +395,9 @@ protected function addDateOfBirthField(FormBuilderInterface $builder) } /** - * @return array + * @return list<\Symfony\Component\Validator\Constraint> */ - protected function createEmailConstraints() + protected function createEmailConstraints(): array { $emailConstraints = [ new NotBlank(), @@ -456,7 +457,7 @@ protected function createLastNameRegexConstraint(): Regex /** * @return \Symfony\Component\Form\CallbackTransformer */ - protected function createDateTimeModelTransformer() + protected function createDateTimeModelTransformer(): CallbackTransformer { return new CallbackTransformer( function ($dateAsString) { @@ -473,7 +474,7 @@ function ($dateAsObject) { /** * @return \Symfony\Component\Form\CallbackTransformer */ - protected function createLocaleModelTransformer() + protected function createLocaleModelTransformer(): CallbackTransformer { return new CallbackTransformer( function ($localeAsObject) { @@ -494,7 +495,7 @@ function ($localeAsInt) { * * @return string */ - public function getName() + public function getName(): string { return $this->getBlockPrefix(); } diff --git a/src/Spryker/Zed/Customer/Communication/Form/CustomerUpdateForm.php b/src/Spryker/Zed/Customer/Communication/Form/CustomerUpdateForm.php index 6d25fc1f..c11a79c6 100644 --- a/src/Spryker/Zed/Customer/Communication/Form/CustomerUpdateForm.php +++ b/src/Spryker/Zed/Customer/Communication/Form/CustomerUpdateForm.php @@ -120,7 +120,7 @@ protected function addEmailField(FormBuilderInterface $builder) /** * @param \Symfony\Component\Form\FormBuilderInterface $builder - * @param array $choices + * @param array $choices * * @return $this */ @@ -138,7 +138,7 @@ protected function addDefaultBillingAddressField(FormBuilderInterface $builder, /** * @param \Symfony\Component\Form\FormBuilderInterface $builder - * @param array $choices + * @param array $choices * * @return $this */ @@ -157,7 +157,7 @@ protected function addDefaultShippingAddressField(FormBuilderInterface $builder, /** * @return string */ - public function getBlockPrefix() + public function getBlockPrefix(): string { return 'customer'; } @@ -167,7 +167,7 @@ public function getBlockPrefix() * * @return string */ - public function getName() + public function getName(): string { return $this->getBlockPrefix(); } diff --git a/src/Spryker/Zed/Customer/CustomerConfig.php b/src/Spryker/Zed/Customer/CustomerConfig.php index ea5c4b3d..52342291 100644 --- a/src/Spryker/Zed/Customer/CustomerConfig.php +++ b/src/Spryker/Zed/Customer/CustomerConfig.php @@ -40,6 +40,11 @@ class CustomerConfig extends AbstractBundleConfig */ public const CUSTOMER_REGISTRATION_WITH_CONFIRMATION_MAIL_TYPE = 'customer registration confirmation mail'; + /** + * @var string + */ + public const GLOSSARY_KEY_CONFIRM_EMAIL_LINK_INVALID_OR_USED = 'customer.error.confirm_email_link.invalid_or_used'; + /** * Specification: * - Regular expression to validate Customer First Name field. @@ -97,6 +102,16 @@ class CustomerConfig extends AbstractBundleConfig */ protected const PASSWORD_RESTORE_TOKEN_URL_WITHOUT_STORE = '%s/password/restore?token=%s'; + /** + * @var string + */ + protected const CUSTOMER_PASSWORD_EXPIRATION_PERIOD = '+2 hours'; + + /** + * @var bool + */ + protected const PASSWORD_RESET_EXPIRATION_IS_ENABLED = false; + /** * @api * @@ -107,6 +122,34 @@ public function getHostYves() return $this->get(CustomerConstants::BASE_URL_YVES); } + /** + * Specification: + * - Toggles the password reset expiration. + * - It is enabled by default. + * + * @api + * + * @return bool + */ + public function isCustomerPasswordResetExpirationEnabled(): bool + { + return static::PASSWORD_RESET_EXPIRATION_IS_ENABLED; + } + + /** + * Specification: + * - Returns a time string that must be compatible with https://www.php.net/manual/en/datetime.modify.php that is used to check if the password reset has been expired. + * - The default is 2h hours. + * + * @api + * + * @return string + */ + public function getCustomerPasswordResetExpirationPeriod(): string + { + return static::CUSTOMER_PASSWORD_EXPIRATION_PERIOD; + } + /** * @api * diff --git a/tests/SprykerTest/Zed/Customer/Business/Facade/RestorePasswordTest.php b/tests/SprykerTest/Zed/Customer/Business/Facade/RestorePasswordTest.php index 738b61f8..e41d9e69 100644 --- a/tests/SprykerTest/Zed/Customer/Business/Facade/RestorePasswordTest.php +++ b/tests/SprykerTest/Zed/Customer/Business/Facade/RestorePasswordTest.php @@ -7,6 +7,9 @@ namespace SprykerTest\Zed\Customer\Business\Facade; +use DateTime; +use Generated\Shared\Transfer\CustomerTransfer; + /** * Auto-generated group annotations * @@ -28,15 +31,82 @@ class RestorePasswordTest extends AbstractCustomerFacadeTest /** * @return void */ - public function testRestorePassword(): void + public function testRestoringANonExpiredResetToken(): void { - $customerTransfer = $this->tester->createTestCustomerTransfer(); - $customerResponseTransfer = $this->tester->getFacade()->registerCustomer($customerTransfer); - $customerTransfer = $this->tester->getFacade()->confirmRegistration($customerResponseTransfer->getCustomerTransfer()); - $this->tester->getFacade()->sendPasswordRestoreMail($customerTransfer); - $customerTransfer = $this->tester->getFacade()->getCustomer($customerTransfer); + // Arrange + $customerTransfer = $this->tester->haveCustomer([ + CustomerTransfer::RESTORE_PASSWORD_KEY => '7a96da42437d4b12a05bc48ca7c99548', + CustomerTransfer::RESTORE_PASSWORD_DATE => (new DateTime())->format('Y-m-d H:i:s'), + CustomerTransfer::PASSWORD => 'changeme123', + ]); + + // Act + $customerResponseTransfer = $this->tester->getFacade()->restorePassword($customerTransfer); + + // Assert + $this->assertTrue($customerResponseTransfer->getIsSuccess()); + $this->assertCount(0, $customerResponseTransfer->getErrors()); + } + + /** + * @return void + */ + public function testRestoringAnExpiredResetTokenIsEnabled(): void + { + $this->tester->mockConfigMethod( + 'isCustomerPasswordResetExpirationEnabled', + true, + ); + + // Arrange + $this->tester->mockConfigMethod( + 'getCustomerPasswordResetExpirationPeriod', + '+2', + ); + + $customerTransfer = $this->tester->haveCustomer([ + CustomerTransfer::RESTORE_PASSWORD_KEY => '51408d2551d148e78612b37d7d3fcbfc', + CustomerTransfer::RESTORE_PASSWORD_DATE => '2000-10-10 12:00:00', + CustomerTransfer::PASSWORD => 'changeme123', + ]); + + // Act + $customerResponseTransfer = $this->tester->getFacade()->restorePassword($customerTransfer); + + // Assert + $this->assertFalse($customerResponseTransfer->getIsSuccess()); + $this->assertCount(1, $customerResponseTransfer->getErrors()); + $this->assertEquals( + 'customer.error.confirm_email_link.invalid_or_used', + $customerResponseTransfer->getErrors()[0]->getMessage(), + ); + } + + /** + * This test is the same as before but it disables the expiration check. + * + * @return void + */ + public function testRestoringAnExpiredResetTokenIsDisabled(): void + { + // Arrange + $this->tester->mockConfigMethod( + 'isCustomerPasswordResetExpirationEnabled', + false, + ); + + $customerTransfer = $this->tester->haveCustomer([ + CustomerTransfer::RESTORE_PASSWORD_KEY => '51408d2551d148e78612b37d7d3fcbfc', + CustomerTransfer::RESTORE_PASSWORD_DATE => '2000-10-10 12:00:00', + CustomerTransfer::PASSWORD => 'changeme123', + ]); + + // Act $customerResponseTransfer = $this->tester->getFacade()->restorePassword($customerTransfer); + + // Assert $this->assertTrue($customerResponseTransfer->getIsSuccess()); + $this->assertCount(0, $customerResponseTransfer->getErrors()); } /**