From 8b1916617c1745cbe7db8b3c4bf0291486810b3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Bia=C5=82czak?= Date: Fri, 31 Jul 2026 13:39:11 +0200 Subject: [PATCH 1/3] IBX-11959: Exposed configured password requirements to the frontend --- .../Controller/PasswordResetController.php | 1 + src/bundle/Resources/config/services.yaml | 3 + .../ibexa_password_requirements.en.xliff | 46 +++++ src/bundle/Twig/UserExtension.php | 7 +- src/bundle/Twig/UserRuntime.php | 18 +- .../Password/PasswordRequirement.php | 49 ++++++ .../PasswordRequirementsResolverInterface.php | 19 +++ .../Password/PasswordRequirementsResolver.php | 91 ++++++++++ .../Constraints/PasswordValidator.php | 40 ++++- tests/bundle/Twig/UserRuntimeTest.php | 97 +++++++++++ .../PasswordRequirementsResolverTest.php | 158 ++++++++++++++++++ .../Constraint/PasswordValidatorTest.php | 156 +++++++++++++++++ 12 files changed, 674 insertions(+), 11 deletions(-) create mode 100644 src/bundle/Resources/translations/ibexa_password_requirements.en.xliff create mode 100644 src/contracts/Password/PasswordRequirement.php create mode 100644 src/contracts/Password/PasswordRequirementsResolverInterface.php create mode 100644 src/lib/Password/PasswordRequirementsResolver.php create mode 100644 tests/bundle/Twig/UserRuntimeTest.php create mode 100644 tests/lib/Password/PasswordRequirementsResolverTest.php diff --git a/src/bundle/Controller/PasswordResetController.php b/src/bundle/Controller/PasswordResetController.php index 75ebc9e..aa01e74 100644 --- a/src/bundle/Controller/PasswordResetController.php +++ b/src/bundle/Controller/PasswordResetController.php @@ -172,6 +172,7 @@ public function userResetPasswordAction(Request $request, string $hashKey): Inva $view = new UserResetPasswordFormView(null, [ 'form_reset_user_password' => $form->createView(), + 'content_type' => $user->getContentType(), ]); $view->setResponse($response); diff --git a/src/bundle/Resources/config/services.yaml b/src/bundle/Resources/config/services.yaml index 6ba35ed..6b92684 100644 --- a/src/bundle/Resources/config/services.yaml +++ b/src/bundle/Resources/config/services.yaml @@ -74,3 +74,6 @@ services: Ibexa\User\Form\BaseSubmitHandler: ~ Ibexa\User\Form\SubmitHandler: '@Ibexa\User\Form\BaseSubmitHandler' + + Ibexa\User\Password\PasswordRequirementsResolver: ~ + Ibexa\Contracts\User\Password\PasswordRequirementsResolverInterface: '@Ibexa\User\Password\PasswordRequirementsResolver' diff --git a/src/bundle/Resources/translations/ibexa_password_requirements.en.xliff b/src/bundle/Resources/translations/ibexa_password_requirements.en.xliff new file mode 100644 index 0000000..9820b30 --- /dev/null +++ b/src/bundle/Resources/translations/ibexa_password_requirements.en.xliff @@ -0,0 +1,46 @@ + + + +
+ + The source node in most cases contains the sample message as written by the developer. If it looks like a dot-delimitted string such as "form.label.firstname", then the developer has not provided a default message. +
+ + + At least one lowercase letter + At least one lowercase letter + key: password_requirement.lower_case + + + At least %length% characters long + At least %length% characters long + key: password_requirement.min_length + + + Different from your current password + Different from your current password + key: password_requirement.new_password + + + At least one special character + At least one special character + key: password_requirement.non_alphanumeric + + + Not found in known data breaches + Not found in known data breaches + key: password_requirement.not_compromised + + + At least one number + At least one number + key: password_requirement.numeric + + + At least one uppercase letter + At least one uppercase letter + key: password_requirement.upper_case + + + + diff --git a/src/bundle/Twig/UserExtension.php b/src/bundle/Twig/UserExtension.php index 666bbd5..9d64abd 100644 --- a/src/bundle/Twig/UserExtension.php +++ b/src/bundle/Twig/UserExtension.php @@ -8,13 +8,14 @@ namespace Ibexa\Bundle\User\Twig; +use Override; use Twig\DeprecatedCallableInfo; use Twig\Extension\AbstractExtension; use Twig\TwigFunction; final class UserExtension extends AbstractExtension { - #[\Override] + #[Override] public function getFunctions(): array { return [ @@ -25,6 +26,10 @@ public function getFunctions(): array 'deprecation_info' => new DeprecatedCallableInfo('ibexa/user', '4.6', 'ibexa_current_user'), ] ), + new TwigFunction( + 'ibexa_password_requirements', + [UserRuntime::class, 'getPasswordRequirements'] + ), ]; } } diff --git a/src/bundle/Twig/UserRuntime.php b/src/bundle/Twig/UserRuntime.php index 9c01020..63ff06d 100644 --- a/src/bundle/Twig/UserRuntime.php +++ b/src/bundle/Twig/UserRuntime.php @@ -10,14 +10,17 @@ use Ibexa\Contracts\Core\Repository\PermissionResolver; use Ibexa\Contracts\Core\Repository\UserService; +use Ibexa\Contracts\Core\Repository\Values\ContentType\ContentType; use Ibexa\Contracts\Core\Repository\Values\User\User; +use Ibexa\Contracts\User\Password\PasswordRequirementsResolverInterface; use Twig\Extension\RuntimeExtensionInterface; final readonly class UserRuntime implements RuntimeExtensionInterface { public function __construct( private PermissionResolver $permissionResolver, - private UserService $userService + private UserService $userService, + private PasswordRequirementsResolverInterface $passwordRequirementsResolver ) { } @@ -27,4 +30,17 @@ public function getCurrentUser(): User $this->permissionResolver->getCurrentUserReference()->getUserId() ); } + + /** + * @param \Ibexa\Contracts\Core\Repository\Values\ContentType\ContentType|null $contentType required + * on anonymous pages (e.g. password reset); defaults to the current user's content type + * + * @return \Ibexa\Contracts\User\Password\PasswordRequirement[] + */ + public function getPasswordRequirements(?ContentType $contentType = null): array + { + return $this->passwordRequirementsResolver->getRequirements( + $contentType ?? $this->getCurrentUser()->getContentType() + ); + } } diff --git a/src/contracts/Password/PasswordRequirement.php b/src/contracts/Password/PasswordRequirement.php new file mode 100644 index 0000000..fb9e12e --- /dev/null +++ b/src/contracts/Password/PasswordRequirement.php @@ -0,0 +1,49 @@ + $parameters + */ + public function __construct( + private string $identifier, + private array $parameters = [] + ) { + } + + public function getIdentifier(): string + { + return $this->identifier; + } + + /** + * @return array + */ + public function getParameters(): array + { + return $this->parameters; + } + + public function getTranslationKey(): string + { + return self::TRANSLATION_KEY_PREFIX . $this->identifier; + } +} diff --git a/src/contracts/Password/PasswordRequirementsResolverInterface.php b/src/contracts/Password/PasswordRequirementsResolverInterface.php new file mode 100644 index 0000000..ac9ce17 --- /dev/null +++ b/src/contracts/Password/PasswordRequirementsResolverInterface.php @@ -0,0 +1,19 @@ +getFirstFieldDefinitionOfType(UserType::FIELD_TYPE_IDENTIFIER); + if ($fieldDefinition === null) { + return []; + } + + $constraints = $fieldDefinition->getValidatorConfiguration()['PasswordValueValidator'] ?? []; + $requirements = []; + + $minLength = (int)($constraints['minLength'] ?? 0); + if ($minLength > 0) { + $requirements[] = new PasswordRequirement(PasswordRequirement::MIN_LENGTH, ['%length%' => $minLength]); + } + + if (!empty($constraints['requireAtLeastOneUpperCaseCharacter'])) { + $requirements[] = new PasswordRequirement(PasswordRequirement::UPPER_CASE); + } + + if (!empty($constraints['requireAtLeastOneLowerCaseCharacter'])) { + $requirements[] = new PasswordRequirement(PasswordRequirement::LOWER_CASE); + } + + if (!empty($constraints['requireAtLeastOneNumericCharacter'])) { + $requirements[] = new PasswordRequirement(PasswordRequirement::NUMERIC); + } + + if (!empty($constraints['requireAtLeastOneNonAlphanumericCharacter'])) { + $requirements[] = new PasswordRequirement(PasswordRequirement::NON_ALPHANUMERIC); + } + + // A configured password TTL implies this rule, {@see \Ibexa\Core\FieldType\User\Type::isNewPasswordRequired()} + if ( + !empty($constraints['requireNewPassword']) + || (int)($fieldDefinition->getFieldSettings()[UserType::PASSWORD_TTL_SETTING] ?? 0) > 0 + ) { + $requirements[] = new PasswordRequirement(PasswordRequirement::NEW_PASSWORD); + } + + if (!empty($constraints['requireNotCompromisedPassword'])) { + $requirements[] = new PasswordRequirement(PasswordRequirement::NOT_COMPROMISED); + } + + return $requirements; + } + + /** + * @return \JMS\TranslationBundle\Model\Message[] + */ + public static function getTranslationMessages(): array + { + $descriptions = [ + PasswordRequirement::MIN_LENGTH => 'At least %length% characters long', + PasswordRequirement::UPPER_CASE => 'At least one uppercase letter', + PasswordRequirement::LOWER_CASE => 'At least one lowercase letter', + PasswordRequirement::NUMERIC => 'At least one number', + PasswordRequirement::NON_ALPHANUMERIC => 'At least one special character', + PasswordRequirement::NEW_PASSWORD => 'Different from your current password', + PasswordRequirement::NOT_COMPROMISED => 'Not found in known data breaches', + ]; + + $messages = []; + foreach ($descriptions as $identifier => $description) { + $messages[] = Message::create( + (new PasswordRequirement($identifier))->getTranslationKey(), + 'ibexa_password_requirements' + )->setDesc($description); + } + + return $messages; + } +} diff --git a/src/lib/Validator/Constraints/PasswordValidator.php b/src/lib/Validator/Constraints/PasswordValidator.php index 6b76c4b..5020717 100644 --- a/src/lib/Validator/Constraints/PasswordValidator.php +++ b/src/lib/Validator/Constraints/PasswordValidator.php @@ -8,14 +8,29 @@ namespace Ibexa\User\Validator\Constraints; -use Ibexa\ContentForms\Validator\ValidationErrorsProcessor; use Ibexa\Contracts\Core\Repository\UserService; +use Ibexa\Contracts\Core\Repository\Values\Translation\Plural; use Ibexa\Contracts\Core\Repository\Values\User\PasswordValidationContext; +use Ibexa\Contracts\User\Password\PasswordRequirement; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\ConstraintValidator; class PasswordValidator extends ConstraintValidator { + /** + * Message templates from {@see \Ibexa\Core\Repository\Validator\UserPasswordValidator} + * and {@see \Ibexa\Core\Repository\User\PasswordValidator}. + */ + private const array REQUIREMENT_CODE_MAP = [ + 'User password must be at least %length% characters long' => PasswordRequirement::MIN_LENGTH, + 'User password must include at least one upper case letter' => PasswordRequirement::UPPER_CASE, + 'User password must include at least one lower case letter' => PasswordRequirement::LOWER_CASE, + 'User password must include at least one number' => PasswordRequirement::NUMERIC, + 'User password must include at least one special character' => PasswordRequirement::NON_ALPHANUMERIC, + 'New password cannot be the same as old password' => PasswordRequirement::NEW_PASSWORD, + 'This password has been leaked in a data breach, it must not be used. Please use another password.' => PasswordRequirement::NOT_COMPROMISED, + ]; + public function __construct( private readonly UserService $userService ) { @@ -41,14 +56,21 @@ public function validate(mixed $value, Constraint $constraint): void $value, $passwordValidationContext ); - if (!empty($validationErrors)) { - $validationErrorsProcessor = $this->createValidationErrorsProcessor(); - $validationErrorsProcessor->processValidationErrors($validationErrors); - } - } - protected function createValidationErrorsProcessor(): ValidationErrorsProcessor - { - return new ValidationErrorsProcessor($this->context); + foreach ($validationErrors as $validationError) { + $message = $validationError->getTranslatableMessage(); + $messageTemplate = $message instanceof Plural ? $message->getPlural() : $message->getMessage(); + + $violationBuilder = $this->context + ->buildViolation($messageTemplate) + ->setParameters($message->getValues()); + + $code = self::REQUIREMENT_CODE_MAP[$messageTemplate] ?? null; + if ($code !== null) { + $violationBuilder->setCode($code); + } + + $violationBuilder->addViolation(); + } } } diff --git a/tests/bundle/Twig/UserRuntimeTest.php b/tests/bundle/Twig/UserRuntimeTest.php new file mode 100644 index 0000000..6f5913d --- /dev/null +++ b/tests/bundle/Twig/UserRuntimeTest.php @@ -0,0 +1,97 @@ +permissionResolver = $this->createMock(PermissionResolver::class); + $this->userService = $this->createMock(UserService::class); + $this->passwordRequirementsResolver = $this->createMock( + PasswordRequirementsResolverInterface::class + ); + + $this->runtime = new UserRuntime( + $this->permissionResolver, + $this->userService, + $this->passwordRequirementsResolver + ); + } + + public function testGetPasswordRequirementsFallsBackToCurrentUserContentType(): void + { + $contentType = $this->createMock(ContentType::class); + $requirements = [new PasswordRequirement(PasswordRequirement::MIN_LENGTH, ['%length%' => 10])]; + + $this->mockCurrentUserWithContentType($contentType); + $this->passwordRequirementsResolver + ->expects(self::once()) + ->method('getRequirements') + ->with($contentType) + ->willReturn($requirements); + + self::assertSame($requirements, $this->runtime->getPasswordRequirements()); + } + + public function testGetPasswordRequirementsForGivenContentType(): void + { + $contentType = $this->createMock(ContentType::class); + $requirements = [new PasswordRequirement(PasswordRequirement::UPPER_CASE)]; + + $this->userService + ->expects(self::never()) + ->method('loadUser'); + $this->passwordRequirementsResolver + ->expects(self::once()) + ->method('getRequirements') + ->with($contentType) + ->willReturn($requirements); + + self::assertSame($requirements, $this->runtime->getPasswordRequirements($contentType)); + } + + private function mockCurrentUserWithContentType(ContentType $contentType): void + { + $userReference = $this->createMock(UserReference::class); + $userReference->method('getUserId')->willReturn(self::CURRENT_USER_ID); + + $user = $this->createMock(User::class); + $user->method('getContentType')->willReturn($contentType); + + $this->permissionResolver + ->method('getCurrentUserReference') + ->willReturn($userReference); + $this->userService + ->method('loadUser') + ->with(self::CURRENT_USER_ID) + ->willReturn($user); + } +} diff --git a/tests/lib/Password/PasswordRequirementsResolverTest.php b/tests/lib/Password/PasswordRequirementsResolverTest.php new file mode 100644 index 0000000..1835113 --- /dev/null +++ b/tests/lib/Password/PasswordRequirementsResolverTest.php @@ -0,0 +1,158 @@ +resolver = new PasswordRequirementsResolver(); + } + + public function testContentTypeWithoutUserFieldDefinition(): void + { + $contentType = $this->createMock(ContentType::class); + $contentType + ->method('getFirstFieldDefinitionOfType') + ->with('ibexa_user') + ->willReturn(null); + + self::assertSame([], $this->resolver->getRequirements($contentType)); + } + + /** + * @dataProvider dataProviderForGetRequirements + * + * @param array $constraints + * @param array $fieldSettings + * @param string[] $expectedIdentifiers + */ + public function testGetRequirements( + array $constraints, + array $fieldSettings, + array $expectedIdentifiers + ): void { + $requirements = $this->resolver->getRequirements( + $this->createContentType($constraints, $fieldSettings) + ); + + self::assertSame( + $expectedIdentifiers, + array_map( + static fn (PasswordRequirement $requirement): string => $requirement->getIdentifier(), + $requirements + ) + ); + } + + /** + * @return array, + * 1: array, + * 2: string[], + * }> + */ + public function dataProviderForGetRequirements(): array + { + return [ + 'all rules disabled' => [ + [ + 'minLength' => null, + 'requireAtLeastOneUpperCaseCharacter' => null, + 'requireAtLeastOneLowerCaseCharacter' => null, + 'requireAtLeastOneNumericCharacter' => null, + 'requireAtLeastOneNonAlphanumericCharacter' => null, + 'requireNewPassword' => null, + 'requireNotCompromisedPassword' => false, + ], + [], + [], + ], + 'all rules enabled' => [ + [ + 'minLength' => 10, + 'requireAtLeastOneUpperCaseCharacter' => 1, + 'requireAtLeastOneLowerCaseCharacter' => 1, + 'requireAtLeastOneNumericCharacter' => 1, + 'requireAtLeastOneNonAlphanumericCharacter' => 1, + 'requireNewPassword' => 1, + 'requireNotCompromisedPassword' => true, + ], + [], + [ + PasswordRequirement::MIN_LENGTH, + PasswordRequirement::UPPER_CASE, + PasswordRequirement::LOWER_CASE, + PasswordRequirement::NUMERIC, + PasswordRequirement::NON_ALPHANUMERIC, + PasswordRequirement::NEW_PASSWORD, + PasswordRequirement::NOT_COMPROMISED, + ], + ], + 'zero min length is disabled' => [ + ['minLength' => 0], + [], + [], + ], + 'new password implied by password TTL' => [ + ['requireNewPassword' => null], + ['PasswordTTL' => 90], + [PasswordRequirement::NEW_PASSWORD], + ], + 'missing validator configuration' => [ + [], + [], + [], + ], + ]; + } + + public function testMinLengthRequirementCarriesParameters(): void + { + $requirements = $this->resolver->getRequirements( + $this->createContentType(['minLength' => 16], []) + ); + + self::assertCount(1, $requirements); + self::assertSame(PasswordRequirement::MIN_LENGTH, $requirements[0]->getIdentifier()); + self::assertSame(['%length%' => 16], $requirements[0]->getParameters()); + self::assertSame('password_requirement.min_length', $requirements[0]->getTranslationKey()); + } + + /** + * @param array $constraints + * @param array $fieldSettings + */ + private function createContentType(array $constraints, array $fieldSettings): ContentType + { + $fieldDefinition = $this->createMock(FieldDefinition::class); + $fieldDefinition + ->method('getValidatorConfiguration') + ->willReturn($constraints === [] ? [] : ['PasswordValueValidator' => $constraints]); + $fieldDefinition + ->method('getFieldSettings') + ->willReturn($fieldSettings); + + $contentType = $this->createMock(ContentType::class); + $contentType + ->method('getFirstFieldDefinitionOfType') + ->with('ibexa_user') + ->willReturn($fieldDefinition); + + return $contentType; + } +} diff --git a/tests/lib/Validator/Constraint/PasswordValidatorTest.php b/tests/lib/Validator/Constraint/PasswordValidatorTest.php index d2e611c..d59b962 100644 --- a/tests/lib/Validator/Constraint/PasswordValidatorTest.php +++ b/tests/lib/Validator/Constraint/PasswordValidatorTest.php @@ -12,7 +12,9 @@ use Ibexa\Contracts\Core\Repository\Values\ContentType\ContentType; use Ibexa\Contracts\Core\Repository\Values\User\PasswordValidationContext; use Ibexa\Contracts\Core\Repository\Values\User\User; +use Ibexa\Contracts\User\Password\PasswordRequirement; use Ibexa\Core\FieldType\ValidationError; +use Ibexa\Core\Repository\Validator\UserPasswordValidator; use Ibexa\User\Validator\Constraints\Password; use Ibexa\User\Validator\Constraints\PasswordValidator; use PHPUnit\Framework\MockObject\MockObject; @@ -131,6 +133,9 @@ public function testInvalid(): void ->method('setParameters') ->with(['%foo%' => $errorParameter]) ->willReturn($constraintViolationBuilder); + $constraintViolationBuilder + ->expects(self::never()) + ->method('setCode'); $constraintViolationBuilder ->expects(self::once()) ->method('addViolation'); @@ -140,6 +145,157 @@ public function testInvalid(): void ])); } + public function testPluralValidationErrorUsesPluralMessageTemplate(): void + { + $contentType = $this->createMock(ContentType::class); + + $this->userService + ->method('validatePassword') + ->willReturn([ + new ValidationError('singular error', 'plural error', ['%limit%' => 2]), + ]); + + $constraintViolationBuilder = $this->createMock(ConstraintViolationBuilderInterface::class); + $constraintViolationBuilder + ->expects(self::once()) + ->method('setParameters') + ->with(['%limit%' => 2]) + ->willReturn($constraintViolationBuilder); + $constraintViolationBuilder + ->expects(self::never()) + ->method('setCode'); + $constraintViolationBuilder + ->expects(self::once()) + ->method('addViolation'); + + $this->executionContext + ->expects(self::once()) + ->method('buildViolation') + ->with('plural error') + ->willReturn($constraintViolationBuilder); + + $this->validator->validate('pass', new Password([ + 'contentType' => $contentType, + ])); + } + + /** + * @dataProvider dataProviderForKnownValidationErrorsGetRequirementCode + */ + public function testKnownValidationErrorsGetRequirementCode( + string $errorMessage, + string $expectedCode + ): void { + $contentType = $this->createMock(ContentType::class); + + $this->userService + ->method('validatePassword') + ->willReturn([new ValidationError($errorMessage)]); + + $constraintViolationBuilder = $this->createMock(ConstraintViolationBuilderInterface::class); + $constraintViolationBuilder + ->method('setParameters') + ->willReturn($constraintViolationBuilder); + $constraintViolationBuilder + ->expects(self::once()) + ->method('setCode') + ->with($expectedCode) + ->willReturn($constraintViolationBuilder); + $constraintViolationBuilder + ->expects(self::once()) + ->method('addViolation'); + + $this->executionContext + ->expects(self::once()) + ->method('buildViolation') + ->with($errorMessage) + ->willReturn($constraintViolationBuilder); + + $this->validator->validate('pass', new Password([ + 'contentType' => $contentType, + ])); + } + + /** + * @return array + */ + public function dataProviderForKnownValidationErrorsGetRequirementCode(): array + { + return [ + 'min length' => [ + 'User password must be at least %length% characters long', + PasswordRequirement::MIN_LENGTH, + ], + 'upper case' => [ + 'User password must include at least one upper case letter', + PasswordRequirement::UPPER_CASE, + ], + 'lower case' => [ + 'User password must include at least one lower case letter', + PasswordRequirement::LOWER_CASE, + ], + 'numeric' => [ + 'User password must include at least one number', + PasswordRequirement::NUMERIC, + ], + 'non alphanumeric' => [ + 'User password must include at least one special character', + PasswordRequirement::NON_ALPHANUMERIC, + ], + 'new password' => [ + 'New password cannot be the same as old password', + PasswordRequirement::NEW_PASSWORD, + ], + 'not compromised' => [ + 'This password has been leaked in a data breach, it must not be used. Please use another password.', + PasswordRequirement::NOT_COMPROMISED, + ], + ]; + } + + /** + * Guards against core rewording validation messages, which would silently + * break the message template → requirement code mapping. + */ + public function testEveryCoreCharacterRuleErrorProducesRequirementCode(): void + { + $coreValidator = new UserPasswordValidator([ + 'minLength' => 10, + 'requireAtLeastOneUpperCaseCharacter' => 1, + 'requireAtLeastOneLowerCaseCharacter' => 1, + 'requireAtLeastOneNumericCharacter' => 1, + 'requireAtLeastOneNonAlphanumericCharacter' => 1, + 'requireNewPassword' => null, + 'requireNotCompromisedPassword' => false, + ]); + $validationErrors = $coreValidator->validate(''); + self::assertCount(5, $validationErrors); + + $this->userService + ->method('validatePassword') + ->willReturn($validationErrors); + + $constraintViolationBuilder = $this->createMock(ConstraintViolationBuilderInterface::class); + $constraintViolationBuilder + ->method('setParameters') + ->willReturn($constraintViolationBuilder); + $constraintViolationBuilder + ->expects(self::exactly(count($validationErrors))) + ->method('setCode') + ->willReturn($constraintViolationBuilder); + $constraintViolationBuilder + ->expects(self::exactly(count($validationErrors))) + ->method('addViolation'); + + $this->executionContext + ->method('buildViolation') + ->willReturn($constraintViolationBuilder); + + $this->validator->validate('pass', new Password([ + 'contentType' => $this->createMock(ContentType::class), + ])); + } + /** * @return array */ From 2e387f38ba7b64d6b01be0e0449c7303f29bd572 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Bia=C5=82czak?= Date: Mon, 3 Aug 2026 07:15:53 +0200 Subject: [PATCH 2/3] IBX-11959: Applied review remarks --- src/bundle/Resources/config/services.yaml | 1 - src/bundle/Twig/UserRuntime.php | 8 +- .../PasswordRequirementsResolverInterface.php | 19 ---- .../Password/PasswordRequirement.php | 2 +- .../Password/PasswordRequirementsResolver.php | 99 ++++++++++--------- .../Constraints/PasswordValidator.php | 5 +- tests/bundle/Twig/UserRuntimeTest.php | 74 +++++++++----- .../PasswordRequirementsResolverTest.php | 39 +++++++- .../Constraint/PasswordValidatorTest.php | 2 +- 9 files changed, 146 insertions(+), 103 deletions(-) delete mode 100644 src/contracts/Password/PasswordRequirementsResolverInterface.php rename src/{contracts => lib}/Password/PasswordRequirement.php (96%) diff --git a/src/bundle/Resources/config/services.yaml b/src/bundle/Resources/config/services.yaml index 6b92684..0cb2dfa 100644 --- a/src/bundle/Resources/config/services.yaml +++ b/src/bundle/Resources/config/services.yaml @@ -76,4 +76,3 @@ services: Ibexa\User\Form\SubmitHandler: '@Ibexa\User\Form\BaseSubmitHandler' Ibexa\User\Password\PasswordRequirementsResolver: ~ - Ibexa\Contracts\User\Password\PasswordRequirementsResolverInterface: '@Ibexa\User\Password\PasswordRequirementsResolver' diff --git a/src/bundle/Twig/UserRuntime.php b/src/bundle/Twig/UserRuntime.php index 63ff06d..5178bf5 100644 --- a/src/bundle/Twig/UserRuntime.php +++ b/src/bundle/Twig/UserRuntime.php @@ -12,7 +12,7 @@ use Ibexa\Contracts\Core\Repository\UserService; use Ibexa\Contracts\Core\Repository\Values\ContentType\ContentType; use Ibexa\Contracts\Core\Repository\Values\User\User; -use Ibexa\Contracts\User\Password\PasswordRequirementsResolverInterface; +use Ibexa\User\Password\PasswordRequirementsResolver; use Twig\Extension\RuntimeExtensionInterface; final readonly class UserRuntime implements RuntimeExtensionInterface @@ -20,7 +20,7 @@ public function __construct( private PermissionResolver $permissionResolver, private UserService $userService, - private PasswordRequirementsResolverInterface $passwordRequirementsResolver + private PasswordRequirementsResolver $passwordRequirementsResolver ) { } @@ -33,9 +33,9 @@ public function getCurrentUser(): User /** * @param \Ibexa\Contracts\Core\Repository\Values\ContentType\ContentType|null $contentType required - * on anonymous pages (e.g. password reset); defaults to the current user's content type + * on anonymous pages (e.g. password reset); defaults to the current user's content type * - * @return \Ibexa\Contracts\User\Password\PasswordRequirement[] + * @return \Ibexa\User\Password\PasswordRequirement[] */ public function getPasswordRequirements(?ContentType $contentType = null): array { diff --git a/src/contracts/Password/PasswordRequirementsResolverInterface.php b/src/contracts/Password/PasswordRequirementsResolverInterface.php deleted file mode 100644 index ac9ce17..0000000 --- a/src/contracts/Password/PasswordRequirementsResolverInterface.php +++ /dev/null @@ -1,19 +0,0 @@ - requirement identifier and its English label. Adding a rule here + * is all that is needed — translations are generated from this list. + */ + private const array RULES = [ + 'minLength' => [PasswordRequirement::MIN_LENGTH, 'At least %length% characters long'], + 'requireAtLeastOneUpperCaseCharacter' => [PasswordRequirement::UPPER_CASE, 'At least one uppercase letter'], + 'requireAtLeastOneLowerCaseCharacter' => [PasswordRequirement::LOWER_CASE, 'At least one lowercase letter'], + 'requireAtLeastOneNumericCharacter' => [PasswordRequirement::NUMERIC, 'At least one number'], + 'requireAtLeastOneNonAlphanumericCharacter' => [PasswordRequirement::NON_ALPHANUMERIC, 'At least one special character'], + 'requireNewPassword' => [PasswordRequirement::NEW_PASSWORD, 'Different from your current password'], + 'requireNotCompromisedPassword' => [PasswordRequirement::NOT_COMPROMISED, 'Not found in known data breaches'], + ]; + + /** + * @return \Ibexa\User\Password\PasswordRequirement[] + */ public function getRequirements(ContentType $contentType): array { $fieldDefinition = $contentType->getFirstFieldDefinitionOfType(UserType::FIELD_TYPE_IDENTIFIER); @@ -25,42 +41,43 @@ public function getRequirements(ContentType $contentType): array } $constraints = $fieldDefinition->getValidatorConfiguration()['PasswordValueValidator'] ?? []; - $requirements = []; - - $minLength = (int)($constraints['minLength'] ?? 0); - if ($minLength > 0) { - $requirements[] = new PasswordRequirement(PasswordRequirement::MIN_LENGTH, ['%length%' => $minLength]); - } - - if (!empty($constraints['requireAtLeastOneUpperCaseCharacter'])) { - $requirements[] = new PasswordRequirement(PasswordRequirement::UPPER_CASE); - } - - if (!empty($constraints['requireAtLeastOneLowerCaseCharacter'])) { - $requirements[] = new PasswordRequirement(PasswordRequirement::LOWER_CASE); - } - - if (!empty($constraints['requireAtLeastOneNumericCharacter'])) { - $requirements[] = new PasswordRequirement(PasswordRequirement::NUMERIC); - } + $fieldSettings = $fieldDefinition->getFieldSettings(); - if (!empty($constraints['requireAtLeastOneNonAlphanumericCharacter'])) { - $requirements[] = new PasswordRequirement(PasswordRequirement::NON_ALPHANUMERIC); + $requirements = []; + foreach (self::RULES as $constraintKey => [$identifier]) { + if ($this->isEnabled($constraintKey, $constraints, $fieldSettings)) { + $requirements[] = new PasswordRequirement($identifier, $this->getParameters($constraintKey, $constraints)); + } } - // A configured password TTL implies this rule, {@see \Ibexa\Core\FieldType\User\Type::isNewPasswordRequired()} - if ( - !empty($constraints['requireNewPassword']) - || (int)($fieldDefinition->getFieldSettings()[UserType::PASSWORD_TTL_SETTING] ?? 0) > 0 - ) { - $requirements[] = new PasswordRequirement(PasswordRequirement::NEW_PASSWORD); - } + return $requirements; + } - if (!empty($constraints['requireNotCompromisedPassword'])) { - $requirements[] = new PasswordRequirement(PasswordRequirement::NOT_COMPROMISED); - } + /** + * @param array $constraints + * @param array $fieldSettings + */ + private function isEnabled(string $constraintKey, array $constraints, array $fieldSettings): bool + { + return match ($constraintKey) { + 'minLength' => (int)($constraints['minLength'] ?? 0) > 0, + // A configured password TTL implies this rule, {@see \Ibexa\Core\FieldType\User\Type::isNewPasswordRequired()} + 'requireNewPassword' => !empty($constraints['requireNewPassword']) + || (int)($fieldSettings[UserType::PASSWORD_TTL_SETTING] ?? 0) > 0, + default => !empty($constraints[$constraintKey]), + }; + } - return $requirements; + /** + * @param array $constraints + * + * @return array + */ + private function getParameters(string $constraintKey, array $constraints): array + { + return $constraintKey === 'minLength' + ? ['%length%' => (int)$constraints['minLength']] + : []; } /** @@ -68,22 +85,12 @@ public function getRequirements(ContentType $contentType): array */ public static function getTranslationMessages(): array { - $descriptions = [ - PasswordRequirement::MIN_LENGTH => 'At least %length% characters long', - PasswordRequirement::UPPER_CASE => 'At least one uppercase letter', - PasswordRequirement::LOWER_CASE => 'At least one lowercase letter', - PasswordRequirement::NUMERIC => 'At least one number', - PasswordRequirement::NON_ALPHANUMERIC => 'At least one special character', - PasswordRequirement::NEW_PASSWORD => 'Different from your current password', - PasswordRequirement::NOT_COMPROMISED => 'Not found in known data breaches', - ]; - $messages = []; - foreach ($descriptions as $identifier => $description) { + foreach (self::RULES as [$identifier, $label]) { $messages[] = Message::create( (new PasswordRequirement($identifier))->getTranslationKey(), 'ibexa_password_requirements' - )->setDesc($description); + )->setDesc($label); } return $messages; diff --git a/src/lib/Validator/Constraints/PasswordValidator.php b/src/lib/Validator/Constraints/PasswordValidator.php index 5020717..2afac26 100644 --- a/src/lib/Validator/Constraints/PasswordValidator.php +++ b/src/lib/Validator/Constraints/PasswordValidator.php @@ -9,9 +9,8 @@ namespace Ibexa\User\Validator\Constraints; use Ibexa\Contracts\Core\Repository\UserService; -use Ibexa\Contracts\Core\Repository\Values\Translation\Plural; use Ibexa\Contracts\Core\Repository\Values\User\PasswordValidationContext; -use Ibexa\Contracts\User\Password\PasswordRequirement; +use Ibexa\User\Password\PasswordRequirement; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\ConstraintValidator; @@ -59,7 +58,7 @@ public function validate(mixed $value, Constraint $constraint): void foreach ($validationErrors as $validationError) { $message = $validationError->getTranslatableMessage(); - $messageTemplate = $message instanceof Plural ? $message->getPlural() : $message->getMessage(); + $messageTemplate = $message->getMessageTemplate(); $violationBuilder = $this->context ->buildViolation($messageTemplate) diff --git a/tests/bundle/Twig/UserRuntimeTest.php b/tests/bundle/Twig/UserRuntimeTest.php index 6f5913d..d9b6860 100644 --- a/tests/bundle/Twig/UserRuntimeTest.php +++ b/tests/bundle/Twig/UserRuntimeTest.php @@ -12,10 +12,11 @@ use Ibexa\Contracts\Core\Repository\PermissionResolver; use Ibexa\Contracts\Core\Repository\UserService; use Ibexa\Contracts\Core\Repository\Values\ContentType\ContentType; +use Ibexa\Contracts\Core\Repository\Values\ContentType\FieldDefinition; use Ibexa\Contracts\Core\Repository\Values\User\User; use Ibexa\Contracts\Core\Repository\Values\User\UserReference; -use Ibexa\Contracts\User\Password\PasswordRequirement; -use Ibexa\Contracts\User\Password\PasswordRequirementsResolverInterface; +use Ibexa\User\Password\PasswordRequirement; +use Ibexa\User\Password\PasswordRequirementsResolver; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -27,69 +28,88 @@ final class UserRuntimeTest extends TestCase private UserService&MockObject $userService; - private PasswordRequirementsResolverInterface&MockObject $passwordRequirementsResolver; - private UserRuntime $runtime; protected function setUp(): void { $this->permissionResolver = $this->createMock(PermissionResolver::class); $this->userService = $this->createMock(UserService::class); - $this->passwordRequirementsResolver = $this->createMock( - PasswordRequirementsResolverInterface::class - ); $this->runtime = new UserRuntime( $this->permissionResolver, $this->userService, - $this->passwordRequirementsResolver + new PasswordRequirementsResolver() ); } public function testGetPasswordRequirementsFallsBackToCurrentUserContentType(): void { - $contentType = $this->createMock(ContentType::class); - $requirements = [new PasswordRequirement(PasswordRequirement::MIN_LENGTH, ['%length%' => 10])]; + $this->mockCurrentUserWithContentType($this->createContentTypeWithMinLength(10)); - $this->mockCurrentUserWithContentType($contentType); - $this->passwordRequirementsResolver - ->expects(self::once()) - ->method('getRequirements') - ->with($contentType) - ->willReturn($requirements); + $requirements = $this->runtime->getPasswordRequirements(); - self::assertSame($requirements, $this->runtime->getPasswordRequirements()); + self::assertCount(1, $requirements); + self::assertSame(PasswordRequirement::MIN_LENGTH, $requirements[0]->getIdentifier()); + self::assertSame(['%length%' => 10], $requirements[0]->getParameters()); } public function testGetPasswordRequirementsForGivenContentType(): void { - $contentType = $this->createMock(ContentType::class); - $requirements = [new PasswordRequirement(PasswordRequirement::UPPER_CASE)]; - $this->userService ->expects(self::never()) ->method('loadUser'); - $this->passwordRequirementsResolver + + $requirements = $this->runtime->getPasswordRequirements( + $this->createContentTypeWithMinLength(16) + ); + + self::assertCount(1, $requirements); + self::assertSame(PasswordRequirement::MIN_LENGTH, $requirements[0]->getIdentifier()); + self::assertSame(['%length%' => 16], $requirements[0]->getParameters()); + } + + private function createContentTypeWithMinLength(int $minLength): ContentType + { + $fieldDefinition = $this->createMock(FieldDefinition::class); + $fieldDefinition + ->expects(self::once()) + ->method('getValidatorConfiguration') + ->willReturn(['PasswordValueValidator' => ['minLength' => $minLength]]); + $fieldDefinition + ->expects(self::once()) + ->method('getFieldSettings') + ->willReturn([]); + + $contentType = $this->createMock(ContentType::class); + $contentType ->expects(self::once()) - ->method('getRequirements') - ->with($contentType) - ->willReturn($requirements); + ->method('getFirstFieldDefinitionOfType') + ->with('ibexa_user') + ->willReturn($fieldDefinition); - self::assertSame($requirements, $this->runtime->getPasswordRequirements($contentType)); + return $contentType; } private function mockCurrentUserWithContentType(ContentType $contentType): void { $userReference = $this->createMock(UserReference::class); - $userReference->method('getUserId')->willReturn(self::CURRENT_USER_ID); + $userReference + ->expects(self::once()) + ->method('getUserId') + ->willReturn(self::CURRENT_USER_ID); $user = $this->createMock(User::class); - $user->method('getContentType')->willReturn($contentType); + $user + ->expects(self::once()) + ->method('getContentType') + ->willReturn($contentType); $this->permissionResolver + ->expects(self::once()) ->method('getCurrentUserReference') ->willReturn($userReference); $this->userService + ->expects(self::once()) ->method('loadUser') ->with(self::CURRENT_USER_ID) ->willReturn($user); diff --git a/tests/lib/Password/PasswordRequirementsResolverTest.php b/tests/lib/Password/PasswordRequirementsResolverTest.php index 1835113..de7e70f 100644 --- a/tests/lib/Password/PasswordRequirementsResolverTest.php +++ b/tests/lib/Password/PasswordRequirementsResolverTest.php @@ -8,9 +8,13 @@ namespace Ibexa\Tests\User\Password; +use Ibexa\Contracts\Core\Persistence\User\Handler as UserHandler; +use Ibexa\Contracts\Core\Repository\PasswordHashService; use Ibexa\Contracts\Core\Repository\Values\ContentType\ContentType; use Ibexa\Contracts\Core\Repository\Values\ContentType\FieldDefinition; -use Ibexa\Contracts\User\Password\PasswordRequirement; +use Ibexa\Core\FieldType\User\Type as UserType; +use Ibexa\Core\Repository\User\PasswordValidatorInterface; +use Ibexa\User\Password\PasswordRequirement; use Ibexa\User\Password\PasswordRequirementsResolver; use PHPUnit\Framework\TestCase; @@ -27,6 +31,7 @@ public function testContentTypeWithoutUserFieldDefinition(): void { $contentType = $this->createMock(ContentType::class); $contentType + ->expects(self::once()) ->method('getFirstFieldDefinitionOfType') ->with('ibexa_user') ->willReturn(null); @@ -121,6 +126,35 @@ public function dataProviderForGetRequirements(): array ]; } + /** + * Guards against core adding a new rule to the PasswordValueValidator schema + * that this resolver would silently not expose. + */ + public function testCoversEveryCoreValidatorSchemaRule(): void + { + $schema = (new UserType( + $this->createMock(UserHandler::class), + $this->createMock(PasswordHashService::class), + $this->createMock(PasswordValidatorInterface::class) + ))->getValidatorConfigurationSchema()['PasswordValueValidator']; + + $allRulesEnabled = array_map( + static fn (array $rule) => $rule['type'] === 'int' ? 1 : true, + $schema + ); + $allRulesEnabled['minLength'] = 10; + + $requirements = $this->resolver->getRequirements( + $this->createContentType($allRulesEnabled, []) + ); + + self::assertCount( + count($schema), + $requirements, + 'Every rule in the core PasswordValueValidator schema must produce a password requirement.' + ); + } + public function testMinLengthRequirementCarriesParameters(): void { $requirements = $this->resolver->getRequirements( @@ -141,14 +175,17 @@ private function createContentType(array $constraints, array $fieldSettings): Co { $fieldDefinition = $this->createMock(FieldDefinition::class); $fieldDefinition + ->expects(self::once()) ->method('getValidatorConfiguration') ->willReturn($constraints === [] ? [] : ['PasswordValueValidator' => $constraints]); $fieldDefinition + ->expects(self::once()) ->method('getFieldSettings') ->willReturn($fieldSettings); $contentType = $this->createMock(ContentType::class); $contentType + ->expects(self::once()) ->method('getFirstFieldDefinitionOfType') ->with('ibexa_user') ->willReturn($fieldDefinition); diff --git a/tests/lib/Validator/Constraint/PasswordValidatorTest.php b/tests/lib/Validator/Constraint/PasswordValidatorTest.php index d59b962..b55833b 100644 --- a/tests/lib/Validator/Constraint/PasswordValidatorTest.php +++ b/tests/lib/Validator/Constraint/PasswordValidatorTest.php @@ -12,9 +12,9 @@ use Ibexa\Contracts\Core\Repository\Values\ContentType\ContentType; use Ibexa\Contracts\Core\Repository\Values\User\PasswordValidationContext; use Ibexa\Contracts\Core\Repository\Values\User\User; -use Ibexa\Contracts\User\Password\PasswordRequirement; use Ibexa\Core\FieldType\ValidationError; use Ibexa\Core\Repository\Validator\UserPasswordValidator; +use Ibexa\User\Password\PasswordRequirement; use Ibexa\User\Validator\Constraints\Password; use Ibexa\User\Validator\Constraints\PasswordValidator; use PHPUnit\Framework\MockObject\MockObject; From ec2b2c1d4f4adc4e14b50ae81b837178d6bcb4df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Bia=C5=82czak?= Date: Wed, 5 Aug 2026 08:42:32 +0200 Subject: [PATCH 3/3] IBX-11959: Applied review remarks --- src/lib/Password/PasswordRequirementsResolver.php | 12 ++++++++---- .../Validator/Constraint/PasswordValidatorTest.php | 2 ++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/lib/Password/PasswordRequirementsResolver.php b/src/lib/Password/PasswordRequirementsResolver.php index 2332a6c..ff5f1fe 100644 --- a/src/lib/Password/PasswordRequirementsResolver.php +++ b/src/lib/Password/PasswordRequirementsResolver.php @@ -20,8 +20,11 @@ * schema => requirement identifier and its English label. Adding a rule here * is all that is needed — translations are generated from this list. */ + /** Constraint key in the core PasswordValueValidator schema, unlike {@see PasswordRequirement::MIN_LENGTH}. */ + private const string MIN_LENGTH_CONSTRAINT = 'minLength'; + private const array RULES = [ - 'minLength' => [PasswordRequirement::MIN_LENGTH, 'At least %length% characters long'], + self::MIN_LENGTH_CONSTRAINT => [PasswordRequirement::MIN_LENGTH, 'At least %length% characters long'], 'requireAtLeastOneUpperCaseCharacter' => [PasswordRequirement::UPPER_CASE, 'At least one uppercase letter'], 'requireAtLeastOneLowerCaseCharacter' => [PasswordRequirement::LOWER_CASE, 'At least one lowercase letter'], 'requireAtLeastOneNumericCharacter' => [PasswordRequirement::NUMERIC, 'At least one number'], @@ -60,10 +63,11 @@ public function getRequirements(ContentType $contentType): array private function isEnabled(string $constraintKey, array $constraints, array $fieldSettings): bool { return match ($constraintKey) { - 'minLength' => (int)($constraints['minLength'] ?? 0) > 0, + self::MIN_LENGTH_CONSTRAINT => (int)($constraints[self::MIN_LENGTH_CONSTRAINT] ?? 0) > 0, // A configured password TTL implies this rule, {@see \Ibexa\Core\FieldType\User\Type::isNewPasswordRequired()} 'requireNewPassword' => !empty($constraints['requireNewPassword']) || (int)($fieldSettings[UserType::PASSWORD_TTL_SETTING] ?? 0) > 0, + // Covers boolean on/off flags only; a numeric rule needs its own arm, like minLength above default => !empty($constraints[$constraintKey]), }; } @@ -75,8 +79,8 @@ private function isEnabled(string $constraintKey, array $constraints, array $fie */ private function getParameters(string $constraintKey, array $constraints): array { - return $constraintKey === 'minLength' - ? ['%length%' => (int)$constraints['minLength']] + return $constraintKey === self::MIN_LENGTH_CONSTRAINT + ? ['%length%' => (int)$constraints[self::MIN_LENGTH_CONSTRAINT]] : []; } diff --git a/tests/lib/Validator/Constraint/PasswordValidatorTest.php b/tests/lib/Validator/Constraint/PasswordValidatorTest.php index b55833b..b3f1882 100644 --- a/tests/lib/Validator/Constraint/PasswordValidatorTest.php +++ b/tests/lib/Validator/Constraint/PasswordValidatorTest.php @@ -194,6 +194,7 @@ public function testKnownValidationErrorsGetRequirementCode( $constraintViolationBuilder = $this->createMock(ConstraintViolationBuilderInterface::class); $constraintViolationBuilder + ->expects(self::once()) ->method('setParameters') ->willReturn($constraintViolationBuilder); $constraintViolationBuilder @@ -277,6 +278,7 @@ public function testEveryCoreCharacterRuleErrorProducesRequirementCode(): void $constraintViolationBuilder = $this->createMock(ConstraintViolationBuilderInterface::class); $constraintViolationBuilder + ->expects(self::exactly(count($validationErrors))) ->method('setParameters') ->willReturn($constraintViolationBuilder); $constraintViolationBuilder