2.3.0: extract the passkey assertion and known-device abstracts - #13
Merged
Merged
Conversation
…erifier Every consumer wrote the verify half of a WebAuthn assertion out by hand — the integration guide even shipped a ~40-line skeleton to copy, and the plugin has two near-identical copies of it (77 lines each, 20 of them actually different). What the copies share is not boilerplate: it is the ordering the ceremony depends on. The abstract runs it — consume the pending options, refuse a response that is not an assertion, look the credential up by raw id, run the library check, write the updated source and lastUsedAt back — and leaves the subclass the four firewall-specific things: the options session key, the credential's owner, the result DTO, and the flush. `commit()` is a hook rather than something the caller does afterwards on purpose. The sign-count write has to be atomic with the check, or two concurrent assertions replaying the same authenticator response both pass before either one's counter lands. That invariant now lives in one place instead of being restated in each copy. Additive: `PasskeyAssertionVerifierInterface` is untouched, so an existing hand-written implementation keeps working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The "have I seen this device for this user?" step behind login notifications was written per consumer, twice in the plugin alone (45 lines each, 8 of them different). Answering and remembering must stay one step: split them and two concurrent sign-ins from the same device both read "unknown", so the user gets two "new device" mails for one login. The abstract keeps that shape, including the recovery it depends on — the unique key on the consumer's (user, fingerprint) columns is what makes the losing insert fail, and the subclass says which exception means conflict (`isConcurrentInsertConflict`) so the bundle stays free of a Doctrine dependency. Same split `AbstractSessionTracker` already uses for the same reason. `KnownDeviceRecordInterface` types the row, matching how every other persisted record the bundle touches is contracted: record data here, the user association on the consumer's entity. 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.
Item 4 of the review notes from
3brs/sylius-enterprise-security-plugin— the two abstracts every consumer currently writes for itself. Additive: no existing interface changes, nothing a consumer has today stops working.Why these two, and not "less duplication"
The plugin has both pairs twice (customer + admin), 45 and 77 lines each, differing in 8 and 20 lines. What the copies share is not boilerplate — each one carries an invariant that has to survive being edited:
AbstractPasskeyAssertionVerifier— the sign-count write has to be atomic with the WebAuthn check, or two concurrent assertions replaying the same authenticator response both pass before either one's counter lands. That is whycommit()is a hook inside the verifier rather than a flush the caller does afterwards. The integration guide previously shipped the whole ~40-line ceremony as a skeleton to copy, so the invariant was restated in every consumer.AbstractNewDeviceDetector(+KnownDeviceRecordInterface) — "is this device new?" and "remember it" must not be separable, or two concurrent sign-ins from the same device both read unknown and the user gets two "new device" mails for one login. The insert race is settled by the unique key on the consumer's(user, fingerprint)columns; the subclass names the exception that means conflict, so no Doctrine dependency enters the bundle — the same splitAbstractSessionTrackeralready uses.Shape
Both follow the existing
AbstractSessionTrackerpattern: orchestration in the bundle, framework-coupled primitives (repository lookup, record factory, persist / flush / detach, conflict detection) as abstract hooks.docs/interface-implementations.mdnow shows the subclass form for both instead of a skeleton to copy.Verified against a real consumer
The plugin's four classes were rewritten onto these abstracts locally (−135/+117 lines, both duplicated bodies gone): its ECS, PHPStan,
lint:containerand 586 PHPUnit tests pass, and the 12 pre-existing tests covering those four classes pass unchanged — the strongest evidence available that the extraction preserves behaviour. That plugin PR follows once this is tagged; it needs~2.3.0to resolve.Checks
phpunit399 tests / 1484 assertions (9 new),phpstan analyseclean,ecs checkclean,composer validate --strictOK.Not in scope
Webauthn\PublicKeyCredentialSourceis deprecated upstream since webauthn-lib 5.3 in favour ofCredentialRecord. The abstract keeps the plugin's current usage verbatim — migrating touches the shape of the persistedcredentialSource, so it belongs in its own change.🤖 Generated with Claude Code