2.2.2: settings fallback, OAuth flash key, issuer hook, English translations - #12
Merged
Merged
Conversation
…nown value `PolicyFactory::twoFactorMode()` passed the stored `two_factor_authentication.mode` straight to `TwoFactorMode::from()`, which throws a `\ValueError` on anything outside `disabled` / `allowed` / `enforced`. The value comes from a settings store the bundle does not own — a direct SQL write, a data migration, a restore from an older database or a consumer filling settings from its own code can all put something else there. The mode is read on every request of a signed-in user, including the settings page where the value could have been corrected, so one bad row meant an HTTP 500 the consumer could only clear in the database. `tryFrom() ?? DISABLED` fixes it. The fallback is `DISABLED` rather than `ALLOWED` because an unrecognised value must neither switch a second factor on by itself nor quietly stop enforcing one that was set to `enforced`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every one of the ten other `addFlashMessage()` calls in `AbstractOAuthCallbackController`
and `AbstractOAuthConfirmLinkController` passes a translation key. The one on a failed
`fetchUserInfo()` passed the exception's message — developer-facing English that the
consumer's flash rendering shows verbatim, and that for a failed profile fetch includes
the provider's raw response body (`GoogleOAuthProvider` builds it from the upstream
exception). The other two messages on that path, "Invalid OAuth state parameter." and
"Missing authorization code in … callback.", told the user nothing they could act on.
The user now gets `three_brs.ui.social_login.provider_error` and the exception goes to
the logger at `warning` under `{audit channel}.provider_error` — the point is not to
drop the information but to keep it where it is useful. `auditLog()` could not carry it:
it needs the `OAuthUserInfoInterface` that this path failed to produce.
Fixing it in the bundle rather than in a consumer: a plugin can override
`addFlashMessage()`, but a non-Sylius consumer would inherit the defect untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The class already reads two constructor values through overridable getters — `isRecoveryCodesEnabled()` and `getRecoveryCodesCount()`, both documented as the hook for a subclass that resolves them at runtime rather than at compile time. The TOTP issuer had no such hook, so a consumer wanting it per tenant, per brand, or from DB-backed settings had to reimplement all ~60 lines of `__invoke()` to change one argument. `__invoke()` now builds the provisioning URI from `getIssuer()`, and the test asserts both the constructor default and an override reaching the generator, so the call site cannot quietly go back to reading the property. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bundle emitted ~30 `three_brs.*` message ids and shipped no catalogue for any of
them, so every consumer had to define the whole set or watch raw ids surface to end
users. `src/Resources/translations/{validators,flashes,messages}.en.yaml` now covers
them; Symfony registers a bundle's translation directory itself, so they apply as soon
as the bundle is registered.
They are defaults, not fixtures. The app's `translations/` directory is loaded after
every bundle, and so is any bundle registered later — which is where
`3brs/sylius-enterprise-security-plugin` sits, so its own wording keeps winning.
English is all that ships; other locales stay the consumer's. The domain split follows
where the ids actually surface: `validators` for constraint messages (the domain the
Symfony validator uses), `flashes` for the flash bag (the bundle adds raw ids and picks
no domain, so the conventional catalogue is assumed), `messages` for the two
recovery-challenge ids handed to a template.
`TranslationCataloguesTest` holds the catalogues and `src/` to each other in both
directions, so neither a new id without wording nor wording for an id that no longer
exists gets through. It needs a YAML parser, hence `symfony/yaml` in require-dev; the
new `suggest` entry names what a consumer needs for the catalogues to load at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Works through the review notes from
3brs/sylius-enterprise-security-plugin(var/bundle-assignment.md), plus the translation catalogues the bundle never shipped. All four commits are backwards compatible — the plugin's~2.2.1pin picks them up oncomposer updatewith no edit.Item 4 of the assignment (extracting
AbstractNewDeviceDetector/AbstractPasskeyAssertionVerifier) is not here: it changes subclass constructors, so it belongs to 2.3.0 and to a separate PR.What's in it
Fall back to a disabled two-factor mode instead of throwing (
PolicyFactory::twoFactorMode())The only item with a live-application impact.
TwoFactorMode::from()threw a\ValueErroron any value outsidedisabled/allowed/enforced, and that value comes from a settings store the bundle does not own. The mode is read on every request of a signed-in user — including the settings page where it could have been corrected — so one bad row was an HTTP 500 clearable only in the database.tryFrom() ?? DISABLED; the fallback isDISABLEDso an unrecognised value neither switches a second factor on by itself nor quietly stops enforcing one.Flash a translation key instead of the OAuth provider's own message (
AbstractOAuthCallbackController)Ten of the eleven
addFlashMessage()calls on that path pass a translation key; this one passed the exception's message, which for a failed profile fetch quotes the provider's raw response body. The user now getsthree_brs.ui.social_login.provider_errorand the detail goes to the logger atwarningunder{audit channel}.provider_error. Fixed here rather than in the plugin: a plugin can overrideaddFlashMessage(), a non-Sylius consumer inherits the defect.Add
getIssuer()(AbstractTwoFactorSetupController)The class already exposes
isRecoveryCodesEnabled()/getRecoveryCodesCount()as runtime hooks. The TOTP issuer had none, so a per-tenant or per-brand issuer meant reimplementing ~60 lines of__invoke().Ship English translation catalogues (
src/Resources/translations/)The bundle emitted ~30
three_brs.*ids and shipped wording for none.validators.en.yaml/flashes.en.yaml/messages.en.yamlnow cover them, split by where each id actually surfaces. They are defaults: the app'stranslations/and any bundle registered later — including the plugin — still win.TranslationCataloguesTestholds the catalogues andsrc/to each other in both directions; it needs a YAML parser, hencesymfony/yamlinrequire-dev.Consumer follow-up
Documented in UPGRADE.md. Nothing is required, but a consumer that renders OAuth flashes untranslated needs
three_brs.ui.social_login.provider_errordefined, and ids that were previously left undefined now read as English instead of rawthree_brs.*strings.Checks
phpunit390 tests / 1456 assertions,phpstan analyseclean,ecs checkclean,composer validate --strictOK.🤖 Generated with Claude Code