IBX-11959: Exposed configured password requirements to the frontend - #132
IBX-11959: Exposed configured password requirements to the frontend#132tbialcz wants to merge 3 commits into
Conversation
b5738e0 to
94174f7
Compare
konradoboza
left a comment
konradoboza
left a comment
There was a problem hiding this comment.
But it would be nice to wire the ibexa/core PR to make sure the CI lits green.
94174f7 to
2e387f3
Compare
| private function isEnabled(string $constraintKey, array $constraints, array $fieldSettings): bool | ||
| { | ||
| return match ($constraintKey) { | ||
| 'minLength' => (int)($constraints['minLength'] ?? 0) > 0, |
There was a problem hiding this comment.
does it make sense to use const defined before here?
PasswordRequirement::MIN_LENGTH?
There was a problem hiding this comment.
isn't it a different thing? the match checks core's config keys (minLength) and PasswordRequirement::MIN_LENGTH is the frontend identifier (min_length)
| // 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]), |
There was a problem hiding this comment.
I propose to make more strict check here, 0 is actually empty and it may interfere. Shouldn't it be !isset() instead?
There was a problem hiding this comment.
i used !empty() because it behaves the same way as the core code. If requireNewPassword is false, the rule should be disabled. If we use isset(), requireNewPassword: false would still be treated as enabled, which would be incorrect.
There was a problem hiding this comment.
My point was, it works now, but if there is a new password rule like minSpecialChars = 0 it would be incorrectly resolved as not enabled. If it's ported directly from core then it's fine, but imo it could be reiterated on this step
There was a problem hiding this comment.
Ok, it follows the current core behavior, but a rule like minSpecialChars = 0 would need its own match case anyway. I’ll add a comment to make that clear
66311b9 to
ec2b2c1
Compare
|
5 participants
Related PRs:
Description:
Backend for showing password requirements on the "Set new password" screen:
ibexa_password_requirements()Twig function — returns the rules enabled in the user content type's password validator config,code= requirement identifier, so the frontend can mark unmet rules after submit,PasswordResetControllerpassescontent_typeto the view (user is not logged in there).Validation logic is unchanged.
For frontend:
{% for requirement in ibexa_password_requirements(content_type) %} <li data-requirement="{{ requirement.identifier }}"> {{ requirement.translationKey|trans(requirement.parameters, 'ibexa_password_requirements') }} </li> {% endfor %}On pages with a logged-in user call it without arguments. After submit, failed rules are in
form.new_password.first.vars.errors— matcherror.cause.codeagainstrequirement.identifier.For QA:
abc) — one error per unmet rule, password not changed.Requirements list on the page will be QA-able with the storefront PR.