IBX-11973: Kept blank login fields distinguishable on authentication failure - #143
Open
tischsoic wants to merge 2 commits into
Open
IBX-11973: Kept blank login fields distinguishable on authentication failure#143tischsoic wants to merge 2 commits into
tischsoic wants to merge 2 commits into
Conversation
…failure The failure handler flattens every BadCredentialsException to a single "Bad credentials." message so that an unknown user cannot be told apart from a wrong password. That also hid which submitted field was left empty, which the login screen needs to mark the field invalid. Blank fields are read from the submitted request rather than from the authenticator's message, so both can be reported at once — Symfony stops at the username. The message and the flattening of everything else are unchanged, so templates that only render the message are unaffected. Co-Authored-By: Claude <noreply@anthropic.com>
ibexa-workflow-automation-1
Bot
requested review from
Steveb-p,
ViniTou,
alongosz,
barw4,
bnowak,
ciastektk,
konradoboza,
mikadamczyk,
tbialcz and
wiewiurdp
and removed request for
a team
September 8, 2026 06:01
konradoboza
reviewed
Sep 8, 2026
Contributor
There was a problem hiding this comment.
While the change itself looks kinda ok we miss the test coverage within DefaultAuthenticationFailureHandlerTest. Feel free to use https://github.com/ibexa/internal-ai/pull/3 if needed.
Adds the cases the failure handler now distinguishes: each field blank on its own, both at once, and the asymmetry it inherits from the authenticator — a whitespace-only login counts as blank, a whitespace-only password does not. The existing message test now posts filled credentials, so it keeps covering the plain path rather than silently taking the new one. Co-Authored-By: Claude <noreply@anthropic.com>
|
konradoboza
approved these changes
Sep 8, 2026
bnowak
approved these changes
Sep 8, 2026
| * @param list<self::FIELD_*> $blankFields | ||
| */ | ||
| public function __construct( | ||
| array $blankFields, |
Contributor
There was a problem hiding this comment.
To simplify it, that could be public readonly constructor promoted property (to reduce code and get rid of its getter).
tbialcz
approved these changes
Sep 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related PRs:
Description:
Because admin-ui never gets told which field was blank — ibexa/user throws that information away before the template runs.
DefaultAuthenticationFailureHandlerreplaces everyBadCredentialsExceptionwith one carrying a flat"Bad credentials.", so an unknown user cannot be told apart from a wrong password. That also hides which submitted field was left empty, which the login screen needs in order to mark that field invalid instead of showing a banner.Blank fields are now carried on a
BlankCredentialsException(aBadCredentialsExceptionsubclass) with the same message and code as before — only the extra data is new, so anything that renderserror.messageis unaffected. Enumeration protection is untouched: the flattening ofUserNotFoundException/AccountStatusExceptionstill happens, and knowing that the field you submitted was empty says nothing about whether an account exists.Which fields are blank is read from the request rather than from the authenticator's exception message, for two reasons: the message is a
sprintfover the configurableusername_parameter, andFormLoginAuthenticatorthrows on the username first, so it can never report both fields at once. Reading the request reports both.The exception is stored in the session, and
AuthenticationException::__serialize()only carries token/code/message/file/line — hence the__serialize()/__unserialize()overrides, without which the field list is silently lost on the redirect.For QA:
Back office
/admin/login(needs ibexa/admin-ui#2058 for the visible part):"Bad credentials."banner, neither field marked.Without ibexa/admin-ui#2058 nothing changes visibly anywhere, including the storefront and customer portal login screens, which render the message only.
Documentation: