feat: add passkey authentication#286
Conversation
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
|
Important Review skippedToo many files! This PR contains 195 files, which is 45 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (195)
You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end WebAuthn/passkey support: new env and DI, MongoDB mappings and repositories, domain entities/DTOs/factories/validators, command handlers and processors, OpenAPI/GraphQL and API wiring, rate-limit updates, extensive tests, and documentation. ChangesPasskey authentication end-to-end
Estimated code review effort Possibly related PRs
Suggested labels Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
28 new issues
|
Signed-off-by: Vadym <kostiukdsfv@gmail.com>
There was a problem hiding this comment.
7 issues found across 93 files
Confidence score: 3/5
- There is a concrete integration risk in
.github/openapi-spec/spec.yaml: passkey request schemas are under-specified versus backend validation, which can break client generation and runtime requests due to a mismatched API contract. - Configuration issues in
.env.load_test(variable expansion order) and.env.test(missing local test origin in passkey allowlist) can cause avoidable startup/test failures, so this is not fully low-risk yet. - Most remaining findings are lower impact (PHPDoc typing in
src/User/Application/Processor/PasskeySignUpCompleteProcessor.phpandsrc/User/Application/Processor/PasskeySignInCompleteProcessor.php, plus import-order style insrc/User/Application/Passkey/PasskeyJsonCodec.php), though the TOCTOU note insrc/User/Application/Passkey/PasskeyCredentialStore.phpcould still surface as duplicate-request errors under concurrency. - Pay close attention to
.github/openapi-spec/spec.yaml,.env.load_test,.env.test, andsrc/User/Application/Passkey/PasskeyCredentialStore.php- contract drift, env misconfiguration, and concurrent passkey registration behavior are the main merge risks.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/User/Application/Processor/PasskeySignUpCompleteProcessor.php">
<violation number="1" location="src/User/Application/Processor/PasskeySignUpCompleteProcessor.php:35">
P3: PHPDoc type `array<string, scalar|array|null>` for `$context` is too narrow — `$context['request']` is a `Request` object. Use `array<string, mixed>` (as in `SignInProcessor`) to avoid misleading static analysis.</violation>
</file>
<file name="src/User/Application/Processor/PasskeySignInCompleteProcessor.php">
<violation number="1" location="src/User/Application/Processor/PasskeySignInCompleteProcessor.php:35">
P3: The PHPDoc type `array<string, scalar|array|null>` for `$context` is incorrect — `$context['request']` is a Symfony `Request` object. The existing `SignInProcessor` correctly uses `array<string,mixed>` for this parameter.</violation>
</file>
<file name="src/User/Application/Passkey/PasskeyJsonCodec.php">
<violation number="1" location="src/User/Application/Passkey/PasskeyJsonCodec.php:7">
P3: `use function` statement should appear after class imports per PSR-12 and the project's existing convention. Other files (e.g., `OAuthProviderCollectionFactory.php`) place `use function` after class `use` statements.</violation>
</file>
<file name="src/User/Application/Passkey/PasskeyCredentialStore.php">
<violation number="1" location="src/User/Application/Passkey/PasskeyCredentialStore.php:39">
P2: TOCTOU race condition: `existsByCredentialId` check and `save` are not atomic. A concurrent request could pass the same check before either saves. The MongoDB unique index prevents data corruption, but when the race occurs, the user gets an unhandled MongoDB duplicate-key exception instead of the intended `ConflictHttpException`. Consider wrapping the `save` in a try/catch for the duplicate-key exception and re-throwing as `ConflictHttpException`.</violation>
</file>
<file name=".env.load_test">
<violation number="1" location=".env.load_test:28">
P2: Don't reference `LOAD_TEST_API_PORT` before it is defined in this env file. Dotenv expands in file order, so this can resolve incorrectly during load-test startup.</violation>
</file>
<file name=".github/openapi-spec/spec.yaml">
<violation number="1" location=".github/openapi-spec/spec.yaml:2982">
P1: The new passkey request schemas are under-specified in OpenAPI (empty/missing required fields) compared to backend validation, causing a broken API contract for clients.</violation>
</file>
<file name=".env.test">
<violation number="1" location=".env.test:31">
P2: Include the test browser origin in the passkey allowlist; otherwise WebAuthn requests from the standard local test origin are rejected.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Browser Client
participant API as API Platform Router
participant Auth as Security / Auth Middleware
participant Proc as Passkey Processor
participant Srv as Passkey Application Service
participant Codec as PasskeyJsonCodec
participant WA as web-auth/webauthn-lib
participant DB as MongoDB (passkey_* collections)
participant UserDB as MongoDB (user collection)
Note over Client,UserDB: NEW: Passkey Sign-Up Flow
Client->>API: POST /api/passkeys/signup/options (email, initials, displayName)
API->>Auth: PUBLIC_ACCESS check
Auth-->>API: pass
API->>Proc: PasskeySignUpOptionsProcessor::process()
Proc->>Srv: PasskeyRegistrationService::startSignup()
Srv->>UserDB: find by email (assert available)
UserDB-->>Srv: null
Srv->>Srv: PasskeyOptionsFactory::createSignupOptions()
Srv->>Codec: encodeOptions(PublicKeyCredentialCreationOptions)
Codec->>WA: serializer serialize
WA-->>Codec: JSON string
Codec-->>Srv: options JSON
Srv->>DB: save PasskeyChallenge (signup, challenge, options, email, userId)
DB-->>Srv: saved
Srv-->>Proc: PasskeyOptionsResult
Proc-->>API: { challenge_id, public_key }
API-->>Client: 200 JSON
Note over Client,UserDB: Client creates credential via navigator.credentials.create()
Client->>API: POST /api/passkeys/signup/complete (challengeId, credential JSON, label, rememberMe)
API->>Auth: PUBLIC_ACCESS check
Auth-->>API: pass
API->>Proc: PasskeySignUpCompleteProcessor::process()
Proc->>Srv: PasskeyRegistrationService::completeSignup()
Srv->>DB: claimActive(challengeId, signup, now) atomic update consumedAt
alt atomic claim succeeds
DB-->>Srv: PasskeyChallenge (consumed)
Srv->>DB: find by email (assert still available)
UserDB-->>Srv: null
Srv->>Codec: decodeCredential(credential JSON)
Codec->>WA: deserialize to PublicKeyCredential
WA-->>Codec: PublicKeyCredential
Codec-->>Srv: PublicKeyCredential
Srv->>WA: AuthenticatorAttestationResponseValidator::check()
WA-->>Srv: CredentialRecord (verified)
Srv->>DB: save User (generated random password hash)
UserDB-->>Srv: User
Srv->>DB: save PasskeyCredential (userId, credentialId, record, label)
DB-->>Srv: saved
Srv->>DB: delete consumed challenge
Srv->>Srv: PasskeySessionIssuer::issue() via IssuedSessionFactory
DB->>DB: create AuthSession, AuthRefreshToken
Srv-->>Proc: PasskeyAuthenticationResult (tokens)
Proc->>Proc: set auth cookie
Proc-->>API: { access_token, refresh_token }
API-->>Client: 200 JSON + Set-Cookie
else stale / consumed / expired challenge
DB-->>Srv: null
Srv-->>Proc: UnauthorizedHttpException
Proc-->>API: 401
API-->>Client: 401
end
Note over Client,UserDB: NEW: Authenticated Passkey Registration
Client->>API: POST /api/passkeys/register/options ({})
API->>Auth: ROLE_USER check
Auth-->>API: pass
API->>Proc: PasskeyRegistrationOptionsProcessor::process()
Proc->>UserDB: get current user identity
UserDB-->>Proc: userId, email
Proc->>Srv: PasskeyRegistrationService::startRegistration()
Srv->>UserDB: findById(userId)
UserDB-->>Srv: User
Srv->>DB: find by userId (existing credentials)
DB-->>Srv: list of PasskeyCredential
Srv->>Srv: create options with excludeCredentials
Srv->>DB: save PasskeyChallenge (registration)
DB-->>Srv: saved
Srv-->>Proc: PasskeyOptionsResult
Proc-->>API: { challenge_id, public_key }
API-->>Client: 200 JSON
Client->>API: POST /api/passkeys/register/complete (challengeId, credential JSON, label)
API->>Auth: ROLE_USER check
Auth-->>API: pass
API->>Proc: PasskeyRegistrationCompleteProcessor::process()
Proc->>Srv: completeRegistration()
Srv->>DB: claimActive(challengeId, registration, now)
alt valid challenge
DB-->>Srv: PasskeyChallenge (consumed, belongs to current user)
Srv->>WA: verify attestation
WA-->>Srv: CredentialRecord
Srv->>DB: check existsByCredentialId (prevents duplicates)
alt unique
DB-->>Srv: false
Srv->>DB: save PasskeyCredential
DB-->>Srv: saved
Srv->>DB: delete challenge
Srv-->>Proc: PasskeyCredential
Proc-->>API: { credential_id }
API-->>Client: 200 JSON
else duplicate
DB-->>Srv: true
Srv-->>Proc: ConflictHttpException
Proc-->>API: 409
API-->>Client: 409
end
else invalid challenge
DB-->>Srv: null
Srv-->>Proc: 401
Proc-->>API: 401
API-->>Client: 401
end
Note over Client,UserDB: NEW: Passkey Sign-In Flow
Client->>API: POST /api/passkeys/signin/options (email, rememberMe)
API->>Auth: PUBLIC_ACCESS check
Auth-->>API: pass
API->>Proc: PasskeySignInOptionsProcessor::process()
Proc->>Srv: PasskeyAuthenticationService::start()
Srv->>UserDB: find by email
alt user exists
UserDB-->>Srv: User
Srv->>DB: find by userId (credentials)
DB-->>Srv: list of PasskeyCredential
else unknown user
UserDB-->>Srv: null
Srv->>Srv: empty credential list (no user enumeration leak)
end
Srv->>Srv: create request options (allowCredentials from existing)
Srv->>DB: save PasskeyChallenge (authentication, email, rememberMe, userId)
DB-->>Srv: saved
Srv-->>Proc: PasskeyOptionsResult
Proc-->>API: { challenge_id, public_key }
API-->>Client: 200 JSON
Note over Client,UserDB: Client gets credential via navigator.credentials.get()
Client->>API: POST /api/passkeys/signin/complete (challengeId, credential JSON)
API->>Auth: PUBLIC_ACCESS check
Auth-->>API: pass
API->>Proc: PasskeySignInCompleteProcessor::process()
Proc->>Srv: PasskeyAuthenticationService::complete()
Srv->>DB: claimActive(challengeId, authentication, now)
alt valid challenge
DB-->>Srv: PasskeyChallenge (consumed, has userId)
Srv->>Codec: extractCredentialId from credential JSON
Srv->>DB: findByCredentialId(encodedId)
alt credential found and owned by challenge userId
DB-->>Srv: PasskeyCredential
Srv->>WA: AuthenticatorAssertionResponseValidator::check()
WA-->>Srv: CredentialRecord (verified)
Srv->>DB: markUsed (update credential record and counter)
DB-->>Srv: updated
Srv->>UserDB: findById(userId)
UserDB-->>Srv: User
Srv->>DB: delete consumed challenge
Srv->>Srv: issue session tokens
DB->>DB: create AuthSession, AuthRefreshToken
Srv-->>Proc: PasskeyAuthenticationResult
Proc->>Proc: set auth cookie
Proc-->>API: { access_token, refresh_token }
API-->>Client: 200 JSON + Set-Cookie
else credential mismatch / not found
DB-->>Srv: null
Srv-->>Proc: 401
Proc-->>API: 401
API-->>Client: 401
end
else stale/consumed challenge
DB-->>Srv: null
Srv-->>Proc: 401
Proc-->>API: 401
API-->>Client: 401
end
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
=============================================
Coverage 100.00% 100.00%
- Complexity 0 3553 +3553
=============================================
Files 551 627 +76
Lines 9541 11782 +2241
=============================================
+ Hits 9541 11782 +2241
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.env.load_test (1)
28-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
LOAD_TEST_API_PORTbeforePASSKEY_ALLOWED_ORIGINSto ensure the variable is defined before interpolation.In Symfony Dotenv, variable expansion depends on definition order. Since
PASSKEY_ALLOWED_ORIGINSon line 28 references${LOAD_TEST_API_PORT}, which is not defined until line 35, the substitution will fail and leave the literal string${LOAD_TEST_API_PORT}in the value, breaking WebAuthn origin validation during load tests.Proposed patch
AUTH_STANDARD_COOKIE_MAX_AGE=900 AUTH_REMEMBER_ME_COOKIE_MAX_AGE=2592000 +LOAD_TEST_API_PORT=18081 PASSKEY_RP_ID=localhost PASSKEY_RP_NAME="VilnaCRM User Service" PASSKEY_ALLOWED_ORIGINS="http://localhost,https://localhost,http://localhost:${LOAD_TEST_API_PORT}" PASSKEY_TIMEOUT_SECONDS=300 PASSKEY_CHALLENGE_TTL_SECONDS=300 REQUEST_BODY_MAX_SIZE_BYTES=65536 CONFIRMATION_TOKEN_LENGTH=32 PASSWORD_RESET_TOKEN_LENGTH=32 PASSWORD_RESET_TOKEN_EXPIRATION_HOURS=1 -LOAD_TEST_API_PORT=18081🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.load_test around lines 28 - 35, PASSKEY_ALLOWED_ORIGINS references ${LOAD_TEST_API_PORT} but LOAD_TEST_API_PORT is defined after it, so move the LOAD_TEST_API_PORT declaration above the PASSKEY_ALLOWED_ORIGINS line to ensure Symfony Dotenv expands the variable; update the file so LOAD_TEST_API_PORT=18081 appears before PASSKEY_ALLOWED_ORIGINS="http://localhost,https://localhost,http://localhost:${LOAD_TEST_API_PORT}" and keep the rest of the variables unchanged.
♻️ Duplicate comments (1)
src/User/Application/Passkey/PasskeyEncoding.php (1)
7-15:⚠️ Potential issue | 🟡 MinorFix import grouping (already flagged by linter).
The class import
InvalidArgumentExceptionat line 10 splits the function import group, violating PSR-12. Group all function imports together before class imports.📦 Proposed fix for import ordering
namespace App\User\Application\Passkey; use function base64_decode; use function base64_encode; - -use InvalidArgumentException; - use function rtrim; use function str_repeat; use function strlen; use function strtr; + +use InvalidArgumentException;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/User/Application/Passkey/PasskeyEncoding.php` around lines 7 - 15, The import order violates PSR-12 because the class InvalidArgumentException is inserted between function imports; in PasskeyEncoding.php move InvalidArgumentException so all function imports (base64_decode, base64_encode, rtrim, str_repeat, strlen, strtr) are grouped together first and then add the class import InvalidArgumentException after them, ensuring function use statements are contiguous and class imports follow.
🟡 Minor comments (3)
specs/passkey-authentication/prd.md-92-106 (1)
92-106:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsolidate duplicate NFR sections to avoid ambiguity.
Line 92 (
## Nonfunctional Requirements) and Line 100 (## Non-Functional Requirements) split one concept into two headings with inconsistent naming. Please merge them into a single NFR section so implementation and test traceability stays clear.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/passkey-authentication/prd.md` around lines 92 - 106, Merge the two duplicate headings "## Nonfunctional Requirements" and "## Non-Functional Requirements" into a single unified section titled consistently (e.g., "## Non-Functional Requirements"), and consolidate all bullets from both headers under that single heading so there is one authoritative NFR list; remove the redundant header and ensure the combined content preserves all items (Domain layer remains framework-free, YAML/XML config items, MongoDB TTL requirement, configurable WebAuthn params, CI checks, Security/Privacy/Compatibility/Observability/Maintainability bullets) and consistent naming for traceability.tests/Unit/User/Application/Passkey/PasskeyStoreTest.php-44-49 (1)
44-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace hardcoded test data with Faker.
This test uses hardcoded IDs (
'user-id','credential-id','passkey-id') and other test values. As per coding guidelines, all test data must be generated using Faker, and hardcoded IDs are not permitted in tests.🎲 Proposed fix using Faker
public function testRegisterRejectsDuplicateCredentialId(): void { + $credentialId = $this->faker->uuid(); + $this->credentialRepository->expects($this->once()) ->method('existsByCredentialId') - ->with('credential-id') + ->with($credentialId) ->willReturn(true); $this->credentialRepository->expects($this->never())->method('save'); $this->expectException(ConflictHttpException::class); $this->expectExceptionMessage('Passkey credential is already registered.'); $this->createStore()->register( - 'user-id', - new VerifiedPasskeyCredential('credential-id', '{"record":true}'), - 'Laptop', + $this->faker->uuid(), + new VerifiedPasskeyCredential($credentialId, json_encode(['record' => true], JSON_THROW_ON_ERROR)), + $this->faker->words(2, true), new DateTimeImmutable() ); }Apply the same pattern to
testFindByUserIdDelegatesToRepository().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/User/Application/Passkey/PasskeyStoreTest.php` around lines 44 - 49, In testFindByUserIdDelegatesToRepository() replace hardcoded values used in the createStore()->register(...) call (e.g. 'user-id', 'credential-id', 'Laptop', any 'passkey-id' usages and static DateTime values) with Faker-generated data obtained from the test's faker instance (e.g. $this->faker->uuid(), $this->faker->uuid(), $this->faker->word(), $this->faker->dateTimeImmutable()) and use those variables when constructing the VerifiedPasskeyCredential and calling register(), ensuring all test IDs and strings come from Faker rather than being hardcoded.tests/Unit/User/Domain/Entity/PasskeyCredentialTest.php-16-23 (1)
16-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace hardcoded test data with Faker.
The test uses hardcoded IDs (
'passkey-id','user-id','credential-id'), JSON strings, and labels. As per coding guidelines, tests must use Faker for all test data generation and never use hardcoded IDs or other test values.🎲 Proposed fix using Faker
public function testCredentialStoresMetadataAndSerializedRecord(): void { $createdAt = new DateTimeImmutable(); + $passkeyId = $this->faker->uuid(); + $userId = $this->faker->uuid(); + $credentialId = $this->faker->uuid(); + $credentialRecord = json_encode(['record' => true], JSON_THROW_ON_ERROR); + $label = $this->faker->words(2, true); + $credential = new PasskeyCredential( - 'passkey-id', - 'user-id', - 'credential-id', - '{"record":true}', - 'Work laptop', + $passkeyId, + $userId, + $credentialId, + $credentialRecord, + $label, $createdAt ); - self::assertSame('passkey-id', $credential->getId()); - self::assertSame('user-id', $credential->getUserId()); - self::assertSame('credential-id', $credential->getCredentialId()); - self::assertSame('{"record":true}', $credential->getCredentialRecord()); - self::assertSame('Work laptop', $credential->getLabel()); + self::assertSame($passkeyId, $credential->getId()); + self::assertSame($userId, $credential->getUserId()); + self::assertSame($credentialId, $credential->getCredentialId()); + self::assertSame($credentialRecord, $credential->getCredentialRecord()); + self::assertSame($label, $credential->getLabel()); self::assertSame($createdAt, $credential->getCreatedAt()); self::assertNull($credential->getLastUsedAt()); }Apply the same pattern to
testMarkUsedUpdatesCredentialRecordAndLastUsedAt().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/User/Domain/Entity/PasskeyCredentialTest.php` around lines 16 - 23, Replace hardcoded test values in the PasskeyCredential instantiation with Faker-generated values: use $this->faker (e.g., ->uuid() for IDs) for 'passkey-id', 'user-id', and 'credential-id'; generate the JSON payload by encoding an array built from Faker values (instead of the literal '{"record":true}'); and replace the fixed label 'Work laptop' with a Faker string (e.g., ->sentence() or ->word()). Apply this same Faker pattern inside the test method testMarkUsedUpdatesCredentialRecordAndLastUsedAt() to ensure all IDs, JSON payloads, and labels are generated by Faker rather than hardcoded.
🧹 Nitpick comments (10)
tests/Unit/User/Application/Passkey/PasskeyEncodingTest.php (1)
11-25: ⚡ Quick winAdd test coverage for unpadded base64url input and round-trip encoding.
The current tests only cover already-padded input and invalid input. Base64url typically omits padding (
=characters), so the primary use case—decoding unpadded base64url values—is untested. Additionally, there are no tests for theencode()method or round-trip behavior.🧪 Suggested additional test cases
public function testDecodeHandlesUnpaddedBase64UrlValue(): void { // 'dGVzdA' is base64url for 'test' without padding (main use case) self::assertSame('test', (new PasskeyEncoding())->decode('dGVzdA')); } public function testEncodeProducesUrlSafeBase64WithoutPadding(): void { $encoded = (new PasskeyEncoding())->encode('test'); self::assertSame('dGVzdA', $encoded); self::assertStringNotContainsString('=', $encoded); self::assertStringNotContainsString('+', $encoded); self::assertStringNotContainsString('/', $encoded); } public function testRoundTripEncodingAndDecoding(): void { $encoding = new PasskeyEncoding(); $original = 'Hello, WebAuthn!'; $encoded = $encoding->encode($original); $decoded = $encoding->decode($encoded); self::assertSame($original, $decoded); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/User/Application/Passkey/PasskeyEncodingTest.php` around lines 11 - 25, Add tests to PasskeyEncodingTest to cover decoding unpadded base64url input, encoding behavior, and round-trip consistency: add a test method (e.g., testDecodeHandlesUnpaddedBase64UrlValue) that asserts (new PasskeyEncoding())->decode('dGVzdA') === 'test'; add a test (e.g., testEncodeProducesUrlSafeBase64WithoutPadding) that asserts (new PasskeyEncoding())->encode('test') returns 'dGVzdA' and contains no '=', '+' or '/'; and add a round-trip test (e.g., testRoundTripEncodingAndDecoding) that encodes a string with PasskeyEncoding::encode and then decodes it with PasskeyEncoding::decode and asserts the result equals the original.tests/Unit/User/Domain/Entity/PasskeyChallengeTest.php (1)
14-74: ⚡ Quick winPrefer Faker for test data generation.
The test uses hardcoded values for emails (
'person@example.com'), IDs ('challenge-id','user-id'), and other test data throughout. Using Faker would improve maintainability and eliminate magic values.Example refactor using Faker
+use Faker\Factory; + final class PasskeyChallengeTest extends UnitTestCase { + private function faker(): \Faker\Generator + { + return Factory::create(); + } + public function testChallengeStoresCeremonyContext(): void { + $faker = $this->faker(); $createdAt = new DateTimeImmutable(); - $expiresAt = $createdAt->modify('+5 minutes'); + $ttlMinutes = 5; + $expiresAt = $createdAt->modify(sprintf('+%d minutes', $ttlMinutes));Apply similar changes for email addresses, names, and IDs using
$faker->email(),$faker->name(),$faker->uuid(), etc.As per coding guidelines: "Always use Faker for all test data generation; never use hardcoded emails, passwords, tokens, or IDs in tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/User/Domain/Entity/PasskeyChallengeTest.php` around lines 14 - 74, Replace hardcoded test data in testChallengeStoresCeremonyContext and createRememberedSignupChallenge with Faker-generated values: initialize a Faker instance in the test (e.g., in setUp) and use $faker->uuid() for IDs, $faker->email() for email, $faker->name() for display name, and appropriate faker calls for initials and challenge/options; update createRememberedSignupChallenge to accept or capture these generated values and assert against them (use variables like $id, $email, $displayName, $userId when constructing PasskeyChallenge and in the assertions), keeping the existing methods and PasskeyChallenge/PasskeyChallengeContext usage intact.tests/Unit/User/Application/Passkey/PasskeyUserCreatorTest.php (1)
25-72: ⚡ Quick winPrefer Faker for test data generation.
The test uses hardcoded emails (
'new@example.com'), IDs ('challenge-id','018f33bb-1111-7222-8333-111111111111'), and other literal values. Using Faker would eliminate these magic values and improve test maintainability.As per coding guidelines: "Always use Faker for all test data generation; never use hardcoded emails, passwords, tokens, or IDs in tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/User/Application/Passkey/PasskeyUserCreatorTest.php` around lines 25 - 72, The test uses hardcoded literals in createSignupChallenge() (like 'new@example.com', 'challenge-id', UUID string, etc.); replace those magic values with Faker-generated data by instantiating or using an existing Faker\Generator in the test and using it to generate the email, uuid, challenge id/string and any other token values before constructing the PasskeyChallenge and PasskeyChallengeContext; update createSignupChallenge() to accept or reference Faker and pass the generated values into the PasskeyChallenge and PasskeyChallengeContext constructors so all emails, IDs and tokens come from Faker rather than hardcoded literals.tests/Unit/User/Application/Passkey/PasskeyDtoTest.php (1)
21-108: ⚡ Quick winPrefer Faker for test data generation.
The test methods use hardcoded emails, IDs, tokens, and other literal values throughout. Using Faker would eliminate magic values and improve maintainability.
As per coding guidelines: "Always use Faker for all test data generation; never use hardcoded emails, passwords, tokens, or IDs in tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/User/Application/Passkey/PasskeyDtoTest.php` around lines 21 - 108, Tests in PasskeyDtoTest use hardcoded literal values; replace them with Faker-generated data to comply with the guideline. Update testSignUpOptionsExposeInputValues, testSignUpCompleteExposesCredentialAndRememberMe, testSignInOptionsExposeEmailAndRememberMe, testCompleteDtosExposeCredentialValues, testResultDtosExposeValues and createChallenge to use Faker for emails, IDs, tokens, labels, challenge strings and option arrays (e.g., replace 'person@example.com', 'credential-id', 'challenge-id', 'access-token', 'refresh-token', 'Security key', options array values, and challenge JSON) — instantiate Faker in setUp (or use existing $this->faker) and use its methods (email, word, uuid, text) when constructing PasskeySignUpOptionsDto, PasskeySignUpCompleteDto, PasskeySignInOptionsDto, PasskeySignInCompleteDto, PasskeyRegistrationCompleteDto, PasskeyOptionsResult, PasskeyAuthenticationResult, VerifiedPasskeyCredential and in createChallenge to produce non-hardcoded created values.src/User/Application/Passkey/PasskeyRegistrationService.php (1)
22-23: 💤 Low valueInconsistent ID-generation abstraction.
PasskeyOptionsFactory(and the test support) usesApp\User\Application\Factory\IdFactoryInterfaceto generate identifiers, while this service depends directly on Symfony'sUuidFactoryto seeduserId. Routing user-id generation through the sameIdFactoryInterfacekeeps the abstraction consistent and makes unit testing easier (single mock). Not a correctness issue — just dependency-direction hygiene.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/User/Application/Passkey/PasskeyRegistrationService.php` around lines 22 - 23, PasskeyRegistrationService currently injects Symfony's UuidFactory for seeding userId while PasskeyOptionsFactory and tests use App\User\Application\Factory\IdFactoryInterface; change the service to depend on IdFactoryInterface instead of UuidFactory, update the constructor signature and property (replace UuidFactory $uuidFactory with IdFactoryInterface $idFactory), and use $idFactory->createId() (or the existing IdFactoryInterface method) to generate the userId wherever $this->uuidFactory was used (keeping PasskeyUserCreator and userCreator usage unchanged) so ID generation is routed through the same abstraction for consistency and easier testing.src/User/Application/Passkey/PasskeyCredentialVerifier.php (1)
47-88: ⚡ Quick winDRY: extract the wrap-and-throw helper for attestation/assertion.
verifyAttestationandverifyAssertionshare an identical try/catch shape (rethrowBadRequestHttpException, wrap any otherThrowableasUnauthorizedHttpException('Bearer', 'Invalid passkey credential.', …)). Extracting that into a small private helper will reduce duplication and keep the error mapping in one place.♻️ Proposed refactor
- public function verifyAttestation( - PasskeyChallenge $challenge, - array $credential - ): VerifiedPasskeyCredential { - try { - return $this->verifiedCredentialFactory->create( - $this->attestationVerifier->verify($challenge, $credential) - ); - } catch (BadRequestHttpException $exception) { - throw $exception; - } catch (Throwable $exception) { - throw new UnauthorizedHttpException( - 'Bearer', - 'Invalid passkey credential.', - $exception - ); - } - } + public function verifyAttestation( + PasskeyChallenge $challenge, + array $credential + ): VerifiedPasskeyCredential { + return $this->wrapVerification( + fn () => $this->attestationVerifier->verify($challenge, $credential) + ); + } @@ - public function verifyAssertion( - PasskeyChallenge $challenge, - array $credential, - PasskeyCredential $storedCredential - ): VerifiedPasskeyCredential { - try { - return $this->verifiedCredentialFactory->create( - $this->assertionVerifier->verify($challenge, $storedCredential, $credential) - ); - } catch (BadRequestHttpException $exception) { - throw $exception; - } catch (Throwable $exception) { - throw new UnauthorizedHttpException( - 'Bearer', - 'Invalid passkey credential.', - $exception - ); - } - } + public function verifyAssertion( + PasskeyChallenge $challenge, + array $credential, + PasskeyCredential $storedCredential + ): VerifiedPasskeyCredential { + return $this->wrapVerification( + fn () => $this->assertionVerifier->verify($challenge, $storedCredential, $credential) + ); + } + + private function wrapVerification(callable $verify): VerifiedPasskeyCredential + { + try { + return $this->verifiedCredentialFactory->create($verify()); + } catch (BadRequestHttpException $exception) { + throw $exception; + } catch (Throwable $exception) { + throw new UnauthorizedHttpException( + 'Bearer', + 'Invalid passkey credential.', + $exception + ); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/User/Application/Passkey/PasskeyCredentialVerifier.php` around lines 47 - 88, verifyAttestation and verifyAssertion duplicate the same try/catch logic; extract that error-wrapping into a private helper (e.g. wrapVerificationCall or handleVerificationExceptions) that accepts a callable which executes the verifier call and returns the VerifiedPasskeyCredential, rethrows BadRequestHttpException, and wraps any other Throwable into new UnauthorizedHttpException('Bearer','Invalid passkey credential.', $exception); replace the try/catch blocks in verifyAttestation and verifyAssertion to call this helper with the appropriate lambda that calls $this->attestationVerifier->verify(...) and $this->assertionVerifier->verify(...), then pass the verifier result into $this->verifiedCredentialFactory->create(...) inside the callable or after the helper returns.src/User/Infrastructure/Repository/MongoDBPasskeyCredentialRepository.php (1)
50-54: 💤 Low valueConsider using a count query for existence check.
existsByCredentialIdhydrates a fullPasskeyCredentialdocument just to check presence. For a hot path (every sign-in/sign-up complete), a projection orcountDocuments(['credentialId' => $id], ['limit' => 1])would avoid the unmarshalling cost. Not strictly required since the field is uniquely indexed and lookups are O(1), but it's a small win.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/User/Infrastructure/Repository/MongoDBPasskeyCredentialRepository.php` around lines 50 - 54, The existsByCredentialId method currently calls findByCredentialId and hydrates a full PasskeyCredential just to test presence; change it to perform a lightweight existence check (e.g., use the MongoDB collection's countDocuments/filter with ['credentialId' => $credentialId] and limit 1 or use findOne with a projection that only returns _id) and return a boolean based on count>0 or non-null result; update the method in MongoDBPasskeyCredentialRepository (replace the call to findByCredentialId) so the hot-path sign-in checks avoid unmarshalling the full PasskeyCredential.tests/Unit/User/Application/Passkey/PasskeyCredentialVerifierTest.php (1)
54-341: 🏗️ Heavy liftReplace hardcoded passkey test data with Faker-generated values.
The verifier tests rely heavily on fixed emails/IDs/tokens/challenges, which conflicts with the repository test-data rule.
As per coding guidelines
tests/**/*.php: “Always use Faker for all test data generation; never use hardcoded emails, passwords, tokens, or IDs in tests.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/User/Application/Passkey/PasskeyCredentialVerifierTest.php` around lines 54 - 341, Tests use fixed literals (emails, user IDs, challenge strings, hostnames, credential IDs) in helper methods like createCreationOptions, createConfiguration, createChallenge, createStoredCredential, createCredentialRecord and createPublicKeyCredential; replace those hardcoded values with Faker-generated values by adding/using a Faker instance (e.g. $this->faker or injecting Faker in the test setup) and call appropriate methods (safeEmail/uuid/uuidV4/text/word) to produce the email, user-id, challenge strings, host, credential ids, and device name so all helpers return dynamic, non-hardcoded data while keeping the same structure and types expected by the tests.tests/Unit/User/Application/Passkey/PasskeyRegistrationServiceTestSupport.php (1)
69-137: ⚡ Quick winApply Faker in this test support helper as well.
This helper centralizes hardcoded test identities/metadata and propagates them into multiple tests. Converting these fixtures to Faker will keep policy compliance and reduce brittle coupling.
As per coding guidelines
tests/**/*.php: “Always use Faker for all test data generation; never use hardcoded emails, passwords, tokens, or IDs in tests.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/User/Application/Passkey/PasskeyRegistrationServiceTestSupport.php` around lines 69 - 137, Replace hardcoded literals in this test helper with Faker-generated values: instantiate Faker in the helper (e.g., in the class constructor or as a private $faker property) and use it to produce the passkey id, raw-credential-id (encoded via PasskeyEncoding), challenge id, device name, IP, browser/user-agent, access-token and refresh-token, and host/application values used in createPasskeyCredential, assertRegistrationOptionsStarted, completeSignup, assertSignupCompleted, createConfiguration and any expectations; store those generated values as properties on the helper so assertion methods (assertRegistrationOptionsStarted, assertSignupCompleted) compare against the same generated values instead of fixed strings, and update createOptionsFactory/createConfiguration to use faker-generated host/title/urls and timeout values.tests/Unit/User/Application/Passkey/PasskeyJsonCodecTest.php (1)
39-290: 🏗️ Heavy liftSwitch this test fixture setup to Faker-based values.
The suite uses multiple hardcoded emails/IDs/challenges/origins. Please generate these through Faker to align with repo testing policy.
As per coding guidelines
tests/**/*.php: “Always use Faker for all test data generation; never use hardcoded emails, passwords, tokens, or IDs in tests.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/User/Application/Passkey/PasskeyJsonCodecTest.php` around lines 39 - 290, Tests use hardcoded emails, IDs, challenges and origins; replace those literals with Faker-generated values by adding a Faker\Generator instance (e.g. $this->faker) in the test class setup and use it in methods that currently return constants: createPasskeyCredential (id, user-id, name), createClientDataJson (origin and challenge), createAssertionPayload (credential id/rawId), createConfiguration (rpId and origin) and createRequestOptions (challenge); update assertCreationOptionsRoundTrip to use the faker-generated email when building registration options and ensure any encoding calls (encoding->encode('...')) use the corresponding faker strings so all test fixtures are created from Faker instead of hardcoded values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/openapi-spec/spec.yaml:
- Around line 2994-3002: The OpenAPI schema places the rememberMe boolean on
EmptyResponse.PasskeySignUpCompleteDto but it belongs on
EmptyResponse.PasskeySignUpOptionsDto; move the rememberMe property definition
(default: false, type: boolean) from EmptyResponse.PasskeySignUpCompleteDto into
EmptyResponse.PasskeySignUpOptionsDto and remove it from
PasskeySignUpCompleteDto so the /api/passkeys/signup/options contract and
generated SDKs match the intended request/response shape.
In `@config/api_platform/resources/EmptyResponse.yaml`:
- Around line 83-102: The OpenAPI config and processor leak email-existence by
returning 409 for passkey_signup_options_http; change the behavior so
/passkeys/signup/options (passkey_signup_options_http) does not return 409:
update the EmptyResponse.yaml responses (remove or replace the 409 entry with a
generic 200/202) and modify
App\User\Application\Processor\PasskeySignUpOptionsProcessor to stop performing
an email-uniqueness rejection there (defer duplicate-account checks to
/passkeys/signup/complete), ensuring the options endpoint always returns a
generic success payload and only the completion endpoint enforces/returns
duplicate-account errors.
In `@config/services.yaml`:
- Line 380: The current regex entry "{ pattern:
'#^/api/passkeys/(signup|signin)#', methods: ['POST'] }" is too broad and may
match future prefixed routes; replace it with explicit exact-path entries for
the two endpoints used for public POST access (e.g., exact-match patterns for
"/api/passkeys/signup" and "/api/passkeys/signin" or use anchored regexes that
include the end-of-string like "^/api/passkeys/signup$" and
"^/api/passkeys/signin$") so only those two POST routes are whitelisted; update
the service YAML entry that contains the pattern string to add two precise rules
instead of the single broad alternate-group pattern.
In `@deptrac.yaml`:
- Line 12: The Application collector regex in deptrac.yaml was widened by adding
"Passkey" to the Application type list (the regex string containing
Transformer|Command|...|Passkey), which weakens architecture enforcement; revert
the change by removing "Passkey" from that regex and instead move any new
Passkey-related classes into the appropriate existing Application subdirectories
(e.g., Processor/Resolver/Factory) or create a narrowly scoped collector with a
clear intent if truly needed, updating references to the Application collector
accordingly.
In `@src/User/Application/DTO/PasskeyRegistrationCompleteDto.php`:
- Around line 17-40: Change the DTO PasskeyRegistrationCompleteDto to expose its
data as public properties instead of private fields with getters: replace the
private constructor properties (challengeId, credential, label) with public
properties and remove the accessor methods challengeIdValue(),
credentialValue(), and labelValue(); then update any callers/processors that
used those methods to read the fields directly (e.g. $dto->challengeId,
$dto->credential, $dto->label) and ensure validation for these fields is
provided in config/validator/validation.yaml as per the application-layer DTO
guideline.
In `@src/User/Application/DTO/PasskeySignInCompleteDto.php`:
- Around line 17-34: Replace the private DTO properties and their accessor
methods with public properties so the Application DTO exposes data directly:
change the constructor signature to accept and assign public properties
$challengeId and $credential (remove private and the methods challengeIdValue()
and credentialValue()), then update any callers/processors to read
$dto->challengeId and $dto->credential instead of the getters; ensure validation
for these fields is moved to and configured in config/validator/validation.yaml
per the Application layer DTO guideline.
In `@src/User/Application/DTO/PasskeySignInOptionsDto.php`:
- Around line 10-37: Change the PasskeySignInOptionsDto to use public properties
per project DTO conventions: make $email and $rememberMe public (remove private
visibility), remove or stop using the emailValue(), isRememberMe() and
setRememberMe() accessor methods and instead rely on the public properties (or
update constructor to no longer encapsulate $email as a private parameter),
ensuring default values remain (email = '' and rememberMe = false) so YAML
validation can target the public properties; update any callers that used
emailValue(), isRememberMe() or setRememberMe() to access the public $email and
$rememberMe directly.
In `@src/User/Application/DTO/PasskeySignUpCompleteDto.php`:
- Around line 12-55: The DTO PasskeySignUpCompleteDto currently declares its
properties as private with accessor methods; change them to public properties so
the application layer DTO exposes fields directly and can be validated via YAML:
update the constructor property promotion from "private string $challengeId",
"private array $credential", "private string $label" and the "private bool
$rememberMe" field to public visibility, and remove or replace the accessor
methods (challengeIdValue, credentialValue, labelValue, isRememberMe) so callers
use $dto->challengeId, $dto->credential, $dto->label, $dto->rememberMe; if you
keep setRememberMe(), have it assign the public $rememberMe property instead of
a private one. Ensure the `@psalm-api/docblocks` on credential remain accurate.
In `@src/User/Application/DTO/PasskeySignUpOptionsDto.php`:
- Around line 15-35: Change the DTO to expose public properties instead of
private properties with accessors: replace the private constructor properties in
PasskeySignUpOptionsDto with public properties (email, initials, displayName)
and remove the emailValue(), initialsValue(), and displayNameValue() accessor
methods; then update any callers to read $dto->email, $dto->initials, and
$dto->displayName (and rely on YAML validation in
config/validator/validation.yaml) so the DTO conforms to the application-layer
guideline.
In `@src/User/Application/Passkey/PasskeyCredentialStore.php`:
- Around line 64-82: The pre-check using
credentialRepository->existsByCredentialId in PasskeyCredentialStore is not
atomic and can race; remove reliance on that separate check and instead handle
unique-index violations at save-time by catching the repository/DB unique
constraint exception thrown by credentialRepository->save (or underlying
persistence layer) and rethrowing a ConflictHttpException('Passkey credential is
already registered.'); ensure the new flow still constructs the
PasskeyCredential object and calls save, but wraps save in a try/catch that maps
the specific unique constraint exception to ConflictHttpException while letting
other exceptions bubble up.
In `@src/User/Application/Passkey/PasskeyVerifiedCredentialFactory.php`:
- Around line 1-28: Move the PasskeyVerifiedCredentialFactory class file into a
Factory directory and update its namespace and any references: relocate
PasskeyVerifiedCredentialFactory.php from the Passkey folder into a Factory
folder, change the class namespace from App\User\Application\Passkey to
App\User\Application\Factory, and update all callers/imports that reference
PasskeyVerifiedCredentialFactory to the new namespace; ensure the file name
remains PasskeyVerifiedCredentialFactory.php and that autoloading
(composer/PSR-4) still resolves the new namespace.
In `@src/User/Domain/Entity/PasskeyChallenge.php`:
- Around line 103-106: isExpired in PasskeyChallenge treats a challenge as valid
when $now equals $this->expiresAt; change the comparison in
PasskeyChallenge::isExpired to treat the boundary as expired (use >= rather than
>) so a challenge with $now === $expiresAt returns true (expired) to prevent the
acceptance window at the exact deadline.
In
`@tests/Unit/User/Application/Passkey/PasskeyAuthenticationServiceTestSupport.php`:
- Around line 55-63: In the PasskeyAuthenticationServiceTestSupport::complete
method replace the hardcoded test literals with Faker-generated values: use
$this->faker->uuid instead of 'challenge-id', $this->faker->ipv4 instead of
'203.0.113.10', and $this->faker->userAgent instead of 'Test Browser'; update
the createService()->complete call to pass those faker variables so tests follow
the repo convention of generating IDs, IPs and user agents dynamically.
In `@tests/Unit/User/Application/Passkey/PasskeyConfigurationTest.php`:
- Around line 16-96: Tests in PasskeyConfigurationTest use hardcoded RP
IDs/names/origins and dates; replace those literals with Faker-generated values
(seed the Faker generator for deterministic assertions) and use the Faker
strings when constructing PasskeyConfiguration and when asserting
getAllowedOrigins(), getRpId(), getRpName(), getTimeoutMilliseconds(), and
challengeExpiresAt(DateTimeImmutable). Instantiate a Faker\Generator at the
start of the test class (or in setUp), seed it (e.g., $faker->seed(1234)),
generate rpId, rpName, allowed origins (trim/normalize commas as current
production code expects) and a createdAt DateTimeImmutable via Faker, then use
those generated values in the constructors and assertions for the
PasskeyConfiguration methods (getAllowedOrigins, getRpId, getRpName,
getTimeoutMilliseconds, challengeExpiresAt) so tests follow the repository
policy.
In `@tests/Unit/User/Application/Passkey/PasskeyOptionsFactoryTest.php`:
- Around line 40-119: Replace hardcoded literals in the tests with
Faker-generated values: assign $this->faker->safeEmail(), $this->faker->name(),
$this->faker->lexify() or $this->faker->bothify()/->uuid() (for initials,
user-id, challenge ids, credential raw id, passkey id) to local variables at the
top of each test and use those variables when calling createSignupOptions,
createAuthenticationOptions, createRegistrationOptions and when building
credentials via createCredential and encoding->encode; ensure assertions compare
against the same local variables (e.g., assertSame($email,
$publicKey['user']['name']) and assertSame($challengeId,
$result->getChallenge()->getId())) so no hardcoded strings remain and
duplicate-literal S1192 warnings are avoided.
In `@tests/Unit/User/Application/Passkey/PasskeyServiceTestObjects.php`:
- Around line 23-114: The test helpers (methods createAuthenticationContext,
createCredential, createSignupChallenge, createIncompleteSignupChallenge,
createRegistrationChallenge, createUser) use hardcoded emails, IDs, tokens and
other identity-like values; replace those literals with values generated by
Faker (e.g., $faker->email, $faker->uuid, $faker->userName, $faker->word,
$faker->password) either by injecting a Faker\Generator into this test object or
constructing one in the class constructor, and use the generated values for
challenge ids, emails, user ids, display names, device names, raw credential ids
and passwords while only keeping fixed values when a test assertion explicitly
requires them. Ensure you update all occurrences in the listed methods so tests
no longer contain hardcoded identity data.
In `@tests/Unit/User/Application/Processor/PasskeyProcessorTest.php`:
- Around line 300-342: Replace hardcoded test data in createOptionsResult(),
createIdentityResolver(), and createCredential() with Faker-generated values:
instantiate a Faker\Generator (or use $this->faker if available) and generate
email, uuids, challenge id, credential id, rpId, and human-readable name; use
the generated uuid string with UuidTransformer->transformFromString(...) for the
user id and use Faker data for JSON fields (e.g., json_encode or Faker->json)
and timestamps (DateTimeImmutable from now) so createOptionsResult(),
createIdentityResolver(), and createCredential() consume only Faker data instead
of fixed literals.
- Around line 67-83: Replace hardcoded test values in
testSignUpOptionsProcessorReturnsChallengeOptions with Faker-generated data:
create a fake email, country code and full name (use the same Faker instance
used elsewhere in tests or add one to this test class) and use those variables
in the registrationService expectation (.with(...)), when constructing the
PasskeySignUpOptionsDto, and when asserting behavior; ensure the helper method
createOptionsResult() still matches the generated values or is fed the generated
inputs so the mocked startSignup call and PasskeySignUpOptionsProcessor.process
remain consistent.
In `@tests/Unit/User/Infrastructure/Repository/MongoDBPasskeyRepositoryTest.php`:
- Around line 190-215: Replace hardcoded literals in createChallenge() and
createCredential() with Faker-generated values: use Faker to produce a realistic
email for PasskeyChallengeContext, unique IDs for 'challenge-id', 'user-id', and
'credential-id', a random challenge/token string for the challenge field, and a
device/name for the PasskeyCredential's device label; maintain the same
constructors (PasskeyChallenge, PasskeyChallengeContext, PasskeyCredential) and
DateTimeImmutable usage but pass Faker values instead of fixed strings so tests
no longer rely on hardcoded data.
---
Outside diff comments:
In @.env.load_test:
- Around line 28-35: PASSKEY_ALLOWED_ORIGINS references ${LOAD_TEST_API_PORT}
but LOAD_TEST_API_PORT is defined after it, so move the LOAD_TEST_API_PORT
declaration above the PASSKEY_ALLOWED_ORIGINS line to ensure Symfony Dotenv
expands the variable; update the file so LOAD_TEST_API_PORT=18081 appears before
PASSKEY_ALLOWED_ORIGINS="http://localhost,https://localhost,http://localhost:${LOAD_TEST_API_PORT}"
and keep the rest of the variables unchanged.
---
Minor comments:
In `@specs/passkey-authentication/prd.md`:
- Around line 92-106: Merge the two duplicate headings "## Nonfunctional
Requirements" and "## Non-Functional Requirements" into a single unified section
titled consistently (e.g., "## Non-Functional Requirements"), and consolidate
all bullets from both headers under that single heading so there is one
authoritative NFR list; remove the redundant header and ensure the combined
content preserves all items (Domain layer remains framework-free, YAML/XML
config items, MongoDB TTL requirement, configurable WebAuthn params, CI checks,
Security/Privacy/Compatibility/Observability/Maintainability bullets) and
consistent naming for traceability.
In `@tests/Unit/User/Application/Passkey/PasskeyStoreTest.php`:
- Around line 44-49: In testFindByUserIdDelegatesToRepository() replace
hardcoded values used in the createStore()->register(...) call (e.g. 'user-id',
'credential-id', 'Laptop', any 'passkey-id' usages and static DateTime values)
with Faker-generated data obtained from the test's faker instance (e.g.
$this->faker->uuid(), $this->faker->uuid(), $this->faker->word(),
$this->faker->dateTimeImmutable()) and use those variables when constructing the
VerifiedPasskeyCredential and calling register(), ensuring all test IDs and
strings come from Faker rather than being hardcoded.
In `@tests/Unit/User/Domain/Entity/PasskeyCredentialTest.php`:
- Around line 16-23: Replace hardcoded test values in the PasskeyCredential
instantiation with Faker-generated values: use $this->faker (e.g., ->uuid() for
IDs) for 'passkey-id', 'user-id', and 'credential-id'; generate the JSON payload
by encoding an array built from Faker values (instead of the literal
'{"record":true}'); and replace the fixed label 'Work laptop' with a Faker
string (e.g., ->sentence() or ->word()). Apply this same Faker pattern inside
the test method testMarkUsedUpdatesCredentialRecordAndLastUsedAt() to ensure all
IDs, JSON payloads, and labels are generated by Faker rather than hardcoded.
---
Duplicate comments:
In `@src/User/Application/Passkey/PasskeyEncoding.php`:
- Around line 7-15: The import order violates PSR-12 because the class
InvalidArgumentException is inserted between function imports; in
PasskeyEncoding.php move InvalidArgumentException so all function imports
(base64_decode, base64_encode, rtrim, str_repeat, strlen, strtr) are grouped
together first and then add the class import InvalidArgumentException after
them, ensuring function use statements are contiguous and class imports follow.
---
Nitpick comments:
In `@src/User/Application/Passkey/PasskeyCredentialVerifier.php`:
- Around line 47-88: verifyAttestation and verifyAssertion duplicate the same
try/catch logic; extract that error-wrapping into a private helper (e.g.
wrapVerificationCall or handleVerificationExceptions) that accepts a callable
which executes the verifier call and returns the VerifiedPasskeyCredential,
rethrows BadRequestHttpException, and wraps any other Throwable into new
UnauthorizedHttpException('Bearer','Invalid passkey credential.', $exception);
replace the try/catch blocks in verifyAttestation and verifyAssertion to call
this helper with the appropriate lambda that calls
$this->attestationVerifier->verify(...) and
$this->assertionVerifier->verify(...), then pass the verifier result into
$this->verifiedCredentialFactory->create(...) inside the callable or after the
helper returns.
In `@src/User/Application/Passkey/PasskeyRegistrationService.php`:
- Around line 22-23: PasskeyRegistrationService currently injects Symfony's
UuidFactory for seeding userId while PasskeyOptionsFactory and tests use
App\User\Application\Factory\IdFactoryInterface; change the service to depend on
IdFactoryInterface instead of UuidFactory, update the constructor signature and
property (replace UuidFactory $uuidFactory with IdFactoryInterface $idFactory),
and use $idFactory->createId() (or the existing IdFactoryInterface method) to
generate the userId wherever $this->uuidFactory was used (keeping
PasskeyUserCreator and userCreator usage unchanged) so ID generation is routed
through the same abstraction for consistency and easier testing.
In `@src/User/Infrastructure/Repository/MongoDBPasskeyCredentialRepository.php`:
- Around line 50-54: The existsByCredentialId method currently calls
findByCredentialId and hydrates a full PasskeyCredential just to test presence;
change it to perform a lightweight existence check (e.g., use the MongoDB
collection's countDocuments/filter with ['credentialId' => $credentialId] and
limit 1 or use findOne with a projection that only returns _id) and return a
boolean based on count>0 or non-null result; update the method in
MongoDBPasskeyCredentialRepository (replace the call to findByCredentialId) so
the hot-path sign-in checks avoid unmarshalling the full PasskeyCredential.
In `@tests/Unit/User/Application/Passkey/PasskeyCredentialVerifierTest.php`:
- Around line 54-341: Tests use fixed literals (emails, user IDs, challenge
strings, hostnames, credential IDs) in helper methods like
createCreationOptions, createConfiguration, createChallenge,
createStoredCredential, createCredentialRecord and createPublicKeyCredential;
replace those hardcoded values with Faker-generated values by adding/using a
Faker instance (e.g. $this->faker or injecting Faker in the test setup) and call
appropriate methods (safeEmail/uuid/uuidV4/text/word) to produce the email,
user-id, challenge strings, host, credential ids, and device name so all helpers
return dynamic, non-hardcoded data while keeping the same structure and types
expected by the tests.
In `@tests/Unit/User/Application/Passkey/PasskeyDtoTest.php`:
- Around line 21-108: Tests in PasskeyDtoTest use hardcoded literal values;
replace them with Faker-generated data to comply with the guideline. Update
testSignUpOptionsExposeInputValues,
testSignUpCompleteExposesCredentialAndRememberMe,
testSignInOptionsExposeEmailAndRememberMe,
testCompleteDtosExposeCredentialValues, testResultDtosExposeValues and
createChallenge to use Faker for emails, IDs, tokens, labels, challenge strings
and option arrays (e.g., replace 'person@example.com', 'credential-id',
'challenge-id', 'access-token', 'refresh-token', 'Security key', options array
values, and challenge JSON) — instantiate Faker in setUp (or use existing
$this->faker) and use its methods (email, word, uuid, text) when constructing
PasskeySignUpOptionsDto, PasskeySignUpCompleteDto, PasskeySignInOptionsDto,
PasskeySignInCompleteDto, PasskeyRegistrationCompleteDto, PasskeyOptionsResult,
PasskeyAuthenticationResult, VerifiedPasskeyCredential and in createChallenge to
produce non-hardcoded created values.
In `@tests/Unit/User/Application/Passkey/PasskeyEncodingTest.php`:
- Around line 11-25: Add tests to PasskeyEncodingTest to cover decoding unpadded
base64url input, encoding behavior, and round-trip consistency: add a test
method (e.g., testDecodeHandlesUnpaddedBase64UrlValue) that asserts (new
PasskeyEncoding())->decode('dGVzdA') === 'test'; add a test (e.g.,
testEncodeProducesUrlSafeBase64WithoutPadding) that asserts (new
PasskeyEncoding())->encode('test') returns 'dGVzdA' and contains no '=', '+' or
'/'; and add a round-trip test (e.g., testRoundTripEncodingAndDecoding) that
encodes a string with PasskeyEncoding::encode and then decodes it with
PasskeyEncoding::decode and asserts the result equals the original.
In `@tests/Unit/User/Application/Passkey/PasskeyJsonCodecTest.php`:
- Around line 39-290: Tests use hardcoded emails, IDs, challenges and origins;
replace those literals with Faker-generated values by adding a Faker\Generator
instance (e.g. $this->faker) in the test class setup and use it in methods that
currently return constants: createPasskeyCredential (id, user-id, name),
createClientDataJson (origin and challenge), createAssertionPayload (credential
id/rawId), createConfiguration (rpId and origin) and createRequestOptions
(challenge); update assertCreationOptionsRoundTrip to use the faker-generated
email when building registration options and ensure any encoding calls
(encoding->encode('...')) use the corresponding faker strings so all test
fixtures are created from Faker instead of hardcoded values.
In
`@tests/Unit/User/Application/Passkey/PasskeyRegistrationServiceTestSupport.php`:
- Around line 69-137: Replace hardcoded literals in this test helper with
Faker-generated values: instantiate Faker in the helper (e.g., in the class
constructor or as a private $faker property) and use it to produce the passkey
id, raw-credential-id (encoded via PasskeyEncoding), challenge id, device name,
IP, browser/user-agent, access-token and refresh-token, and host/application
values used in createPasskeyCredential, assertRegistrationOptionsStarted,
completeSignup, assertSignupCompleted, createConfiguration and any expectations;
store those generated values as properties on the helper so assertion methods
(assertRegistrationOptionsStarted, assertSignupCompleted) compare against the
same generated values instead of fixed strings, and update
createOptionsFactory/createConfiguration to use faker-generated host/title/urls
and timeout values.
In `@tests/Unit/User/Application/Passkey/PasskeyUserCreatorTest.php`:
- Around line 25-72: The test uses hardcoded literals in createSignupChallenge()
(like 'new@example.com', 'challenge-id', UUID string, etc.); replace those magic
values with Faker-generated data by instantiating or using an existing
Faker\Generator in the test and using it to generate the email, uuid, challenge
id/string and any other token values before constructing the PasskeyChallenge
and PasskeyChallengeContext; update createSignupChallenge() to accept or
reference Faker and pass the generated values into the PasskeyChallenge and
PasskeyChallengeContext constructors so all emails, IDs and tokens come from
Faker rather than hardcoded literals.
In `@tests/Unit/User/Domain/Entity/PasskeyChallengeTest.php`:
- Around line 14-74: Replace hardcoded test data in
testChallengeStoresCeremonyContext and createRememberedSignupChallenge with
Faker-generated values: initialize a Faker instance in the test (e.g., in setUp)
and use $faker->uuid() for IDs, $faker->email() for email, $faker->name() for
display name, and appropriate faker calls for initials and challenge/options;
update createRememberedSignupChallenge to accept or capture these generated
values and assert against them (use variables like $id, $email, $displayName,
$userId when constructing PasskeyChallenge and in the assertions), keeping the
existing methods and PasskeyChallenge/PasskeyChallengeContext usage intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b28f2f51-78cd-4300-a3ad-b1102281c498
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (92)
.env.env.load_test.env.test.github/openapi-spec/spec.yamlcomposer.jsonconfig/api_platform/resources/EmptyResponse.yamlconfig/doctrine/User/PasskeyChallenge.mongodb.xmlconfig/doctrine/User/PasskeyCredential.mongodb.xmlconfig/packages/security.yamlconfig/services.yamlconfig/services_test.yamlconfig/validator/validation.yamldeptrac.yamldocs/advanced-configuration.mddocs/main.mddocs/passkey-authentication.mdspecs/passkey-authentication/architecture.mdspecs/passkey-authentication/epics.mdspecs/passkey-authentication/implementation-readiness.mdspecs/passkey-authentication/prd.mdspecs/passkey-authentication/product-brief.mdspecs/passkey-authentication/research.mdspecs/passkey-authentication/run-summary.mdsrc/Shared/Application/Resolver/RateLimit/ApiRateLimitAuthTargetResolver.phpsrc/Shared/Application/Resolver/RateLimit/ApiRateLimitRequestResolver.phpsrc/User/Application/DTO/PasskeyAuthenticationResult.phpsrc/User/Application/DTO/PasskeyOptionsResult.phpsrc/User/Application/DTO/PasskeyRegistrationCompleteDto.phpsrc/User/Application/DTO/PasskeyRegistrationOptionsDto.phpsrc/User/Application/DTO/PasskeySignInCompleteDto.phpsrc/User/Application/DTO/PasskeySignInOptionsDto.phpsrc/User/Application/DTO/PasskeySignUpCompleteDto.phpsrc/User/Application/DTO/PasskeySignUpOptionsDto.phpsrc/User/Application/DTO/VerifiedPasskeyCredential.phpsrc/User/Application/Passkey/PasskeyAssertionCredentialRecordVerifier.phpsrc/User/Application/Passkey/PasskeyAttestationCredentialRecordVerifier.phpsrc/User/Application/Passkey/PasskeyAuthenticationService.phpsrc/User/Application/Passkey/PasskeyAuthenticationServiceInterface.phpsrc/User/Application/Passkey/PasskeyChallengeStore.phpsrc/User/Application/Passkey/PasskeyConfiguration.phpsrc/User/Application/Passkey/PasskeyCredentialResponseResolver.phpsrc/User/Application/Passkey/PasskeyCredentialStore.phpsrc/User/Application/Passkey/PasskeyCredentialVerifier.phpsrc/User/Application/Passkey/PasskeyCredentialVerifierInterface.phpsrc/User/Application/Passkey/PasskeyEncoding.phpsrc/User/Application/Passkey/PasskeyJsonCodec.phpsrc/User/Application/Passkey/PasskeyJsonCodecInterface.phpsrc/User/Application/Passkey/PasskeyOptionsFactory.phpsrc/User/Application/Passkey/PasskeyPublicKeyOptionsFactory.phpsrc/User/Application/Passkey/PasskeyRegistrationService.phpsrc/User/Application/Passkey/PasskeyRegistrationServiceInterface.phpsrc/User/Application/Passkey/PasskeyResponseFactory.phpsrc/User/Application/Passkey/PasskeySessionIssuer.phpsrc/User/Application/Passkey/PasskeyUserCreator.phpsrc/User/Application/Passkey/PasskeyUserResolver.phpsrc/User/Application/Passkey/PasskeyVerifiedCredentialFactory.phpsrc/User/Application/Passkey/PasskeyWebauthnFactory.phpsrc/User/Application/Passkey/PasskeyWebauthnFactoryInterface.phpsrc/User/Application/Processor/PasskeyRegistrationCompleteProcessor.phpsrc/User/Application/Processor/PasskeyRegistrationOptionsProcessor.phpsrc/User/Application/Processor/PasskeySignInCompleteProcessor.phpsrc/User/Application/Processor/PasskeySignInOptionsProcessor.phpsrc/User/Application/Processor/PasskeySignUpCompleteProcessor.phpsrc/User/Application/Processor/PasskeySignUpOptionsProcessor.phpsrc/User/Domain/Entity/PasskeyChallenge.phpsrc/User/Domain/Entity/PasskeyCredential.phpsrc/User/Domain/Repository/PasskeyChallengeRepositoryInterface.phpsrc/User/Domain/Repository/PasskeyCredentialRepositoryInterface.phpsrc/User/Domain/ValueObject/PasskeyChallengeContext.phpsrc/User/Infrastructure/Repository/MongoDBPasskeyChallengeRepository.phpsrc/User/Infrastructure/Repository/MongoDBPasskeyCredentialRepository.phptests/Behat/Support/TestClientIpHttpRequestContextResolver.phptests/Behat/UserContext/UserRequestContext.phptests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitAuthTargetResolverTest.phptests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitRequestResolverLimitersTest.phptests/Unit/User/Application/Passkey/PasskeyAuthenticationServiceTest.phptests/Unit/User/Application/Passkey/PasskeyAuthenticationServiceTestSupport.phptests/Unit/User/Application/Passkey/PasskeyConfigurationTest.phptests/Unit/User/Application/Passkey/PasskeyCredentialVerifierTest.phptests/Unit/User/Application/Passkey/PasskeyDtoTest.phptests/Unit/User/Application/Passkey/PasskeyEncodingTest.phptests/Unit/User/Application/Passkey/PasskeyJsonCodecTest.phptests/Unit/User/Application/Passkey/PasskeyOptionsFactoryTest.phptests/Unit/User/Application/Passkey/PasskeyRegistrationServiceTest.phptests/Unit/User/Application/Passkey/PasskeyRegistrationServiceTestSupport.phptests/Unit/User/Application/Passkey/PasskeyServiceTestObjects.phptests/Unit/User/Application/Passkey/PasskeyStoreTest.phptests/Unit/User/Application/Passkey/PasskeyUserCreatorTest.phptests/Unit/User/Application/Processor/PasskeyProcessorTest.phptests/Unit/User/Domain/Entity/PasskeyChallengeTest.phptests/Unit/User/Domain/Entity/PasskeyCredentialTest.phptests/Unit/User/Infrastructure/Repository/MongoDBPasskeyRepositoryTest.php
Signed-off-by: Vadym <kostiukdsfv@gmail.com>
38aaa15 to
8125902
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
.github/openapi-spec/spec.yaml (1)
3041-3042:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
rememberMeplacement is inconsistent between sign-in and sign-up flows.The OpenAPI schema places
rememberMeon different DTOs depending on the flow:
- Sign-in flow:
rememberMeis onPasskeySignInOptionsDto(options request)- Sign-up flow:
rememberMeis onPasskeySignUpCompleteDto(completion request)This inconsistency forces API consumers to remember different patterns for each flow. For consistency and developer experience, both flows should follow the same pattern.
The previous review suggested moving
rememberMefromPasskeySignUpCompleteDtotoPasskeySignUpOptionsDtoto align with the sign-in pattern. However, verify with the team which pattern is preferred before making the change, as moving it would also require updating the backend processor and tests.Also applies to: 3068-3069
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/openapi-spec/spec.yaml around lines 3041 - 3042, The OpenAPI spec is inconsistent: rememberMe is on PasskeySignInOptionsDto but on PasskeySignUpCompleteDto for sign-up; to fix, decide with the team to standardize on one pattern (recommended: options DTO like PasskeySignUpOptionsDto to mirror PasskeySignInOptionsDto), then move the rememberMe property from PasskeySignUpCompleteDto to PasskeySignUpOptionsDto in the spec, and update all related references; also update the backend handler/processor that consumes sign-up requests (the sign-up completion flow), plus any affected tests and fixtures to accept rememberMe on the sign-up options DTO (or conversely, if the team prefers completion DTO, move it from PasskeySignInOptionsDto to PasskeySignUpCompleteDto and update backend/tests accordingly).
🧹 Nitpick comments (5)
tests/Unit/User/Application/Passkey/PasskeyRegistrationServiceTest.php (1)
284-292: 💤 Low valuePrefix unused callback parameters with underscore.
The callback parameters
$idand$purposematch theclaimActivesignature but aren't used in the callback body. Prefix them with underscores to indicate they're intentionally unused and silence the static analysis warning.Proposed fix
->willReturnCallback(static function ( - string $id, - string $purpose, + string $_id, + string $_purpose, DateTimeImmutable $consumedAt ) use ($challenge): PasskeyChallenge { $challenge->consume($consumedAt);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/User/Application/Passkey/PasskeyRegistrationServiceTest.php` around lines 284 - 292, In PasskeyRegistrationServiceTest update the anonymous callback used in the willReturnCallback to mark the unused parameters as intentionally unused by prefixing $id and $purpose with underscores (e.g. change parameters in the static function signature to $_id and $_purpose while keeping DateTimeImmutable $consumedAt unchanged); this will silence static analysis warnings while preserving the callback behavior in the claimActive mock setup that consumes $consumedAt and returns $challenge..env (1)
84-84: ⚡ Quick winRemove unnecessary quotes from PASSKEY_ALLOWED_ORIGINS.
The value contains no spaces, so quotes are unnecessary and may cause environment parsers to include them literally in the value, breaking origin validation. Line 83 correctly uses quotes because the value contains a space.
🔧 Proposed fix
-PASSKEY_ALLOWED_ORIGINS="http://localhost,https://localhost,http://localhost:8080" +PASSKEY_ALLOWED_ORIGINS=http://localhost,https://localhost,http://localhost:8080🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env at line 84, The PASSKEY_ALLOWED_ORIGINS environment variable is wrapped in unnecessary quotes which may be read literally by env parsers and break origin validation; update the .env entry for PASSKEY_ALLOWED_ORIGINS by removing the surrounding double quotes so the value is unquoted (PASSKEY_ALLOWED_ORIGINS=http://localhost,https://localhost,http://localhost:8080), ensuring it matches how other no-space variables are defined..env.test (1)
31-31: ⚡ Quick winRemove unnecessary quotes from PASSKEY_ALLOWED_ORIGINS.
The value contains no spaces, so quotes are unnecessary and may cause the parsed value to include literal quotes, breaking origin validation.
🔧 Proposed fix
-PASSKEY_ALLOWED_ORIGINS="http://localhost,https://localhost,http://localhost:8080,http://localhost:8081" +PASSKEY_ALLOWED_ORIGINS=http://localhost,https://localhost,http://localhost:8080,http://localhost:8081🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.test at line 31, The PASSKEY_ALLOWED_ORIGINS value in the .env.test file is wrapped in double quotes which can lead to literal quotes being included when parsed and break origin validation; update the PASSKEY_ALLOWED_ORIGINS entry by removing the surrounding quotes so the value is an unquoted comma-separated list (refer to the PASSKEY_ALLOWED_ORIGINS variable to locate the line) and ensure no spaces or extra characters remain..env.load_test (1)
29-29: ⚡ Quick winRemove unnecessary quotes from PASSKEY_ALLOWED_ORIGINS.
The value contains no spaces, so quotes are unnecessary and may cause the parsed value to include literal quotes, breaking origin validation.
🔧 Proposed fix
-PASSKEY_ALLOWED_ORIGINS="http://localhost,https://localhost,http://localhost:${LOAD_TEST_API_PORT}" +PASSKEY_ALLOWED_ORIGINS=http://localhost,https://localhost,http://localhost:${LOAD_TEST_API_PORT}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.load_test at line 29, The PASSKEY_ALLOWED_ORIGINS value is wrapped in quotes which can become literal characters when loaded; remove the surrounding double quotes so the environment entry for PASSKEY_ALLOWED_ORIGINS is an unquoted comma-separated list (e.g. PASSKEY_ALLOWED_ORIGINS=http://localhost,https://localhost,http://localhost:${LOAD_TEST_API_PORT}), ensure there are no extra spaces, save the change and re-run the origin validation to confirm it parses correctly.specs/passkey-authentication/product-brief.md (1)
21-21: 💤 Low valueConsider using "Non-Goals" with hyphen for consistency with product brief conventions.
Most product briefs use "Non-Goals" with a hyphen. While "Non Goals" is not incorrect, the hyphenated form is more standard in technical product documentation.
📝 Suggested style fix
-## Non Goals +## Non-Goals🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/passkey-authentication/product-brief.md` at line 21, Replace the header text "Non Goals" with the hyphenated form "Non-Goals" to match product brief conventions and maintain consistency; locate the heading "Non Goals" in the document (the section header) and update it to "Non-Goals".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/passkey-authentication/epics.md`:
- Around line 13-14: The acronym "BMALPH" is inconsistent with "BMAD" in the
same story; update the text so both occurrences use the same acronym (replace
"BMALPH" with "BMAD" in the two bullet lines) and run a quick grep/scan for any
other "BMALPH" occurrences to ensure all references (e.g., the lines containing
"BMALPH doctor passes." and "BMALPH implementation transition succeeds.") are
aligned to "BMAD".
In `@tests/Unit/User/Application/Passkey/PasskeyAuthenticationServiceTest.php`:
- Around line 64-398: This test file uses many hardcoded literals (emails,
UUIDs, challenge/session IDs, IPs, browser strings) in methods like
testStartUsesExistingUserCredentialsWhenUserExists,
testStartCreatesOptionsWithoutCredentialDescriptorsForUnknownEmail,
testCompleteVerifiesCredentialUpdatesRecordAndIssuesSession and helper methods
expectExistingUserCredentials, expectAuthenticationOptionsChallenge,
expectAuthenticationCompleted, expectSessionIssue, etc.; replace those hardcoded
values with Faker-generated values (e.g. $this->faker->email,
$this->faker->uuid, $this->faker->ipv4, $this->faker->userAgent or custom
string) when creating User, PasskeyChallenge, PasskeyCredential, challenge IDs
and session IDs and when setting expectations on mocks (idFactory->create,
sessionFactory->create, repositories and publishers) so tests consume dynamic
fake data while keeping assertions that compare against the generated variables
(store faker outputs in local variables and use them in both setup and
assertions).
In `@tests/Unit/User/Application/Passkey/PasskeyDtoTest.php`:
- Around line 113-116: The test contains an unrelated assertInstanceOf for
PasskeyRegistrationOptionsDto that doesn't exercise the optionsResult under
test; remove the redundant assertion (the new PasskeyRegistrationOptionsDto()
check) or replace it with an assertion that examines the actual variable under
test (e.g. assertInstanceOf(PasskeyRegistrationOptionsDto::class,
$optionsResult) or assertions against its properties); update the test method
that references $optionsResult so it verifies the expected behavior/state
instead of instantiating a fresh DTO.
In `@tests/Unit/User/Application/Passkey/PasskeyRegistrationServiceTest.php`:
- Line 86: The tests in PasskeyRegistrationServiceTest use hardcoded emails and
IDs (e.g., the startSignup call with 'new@example.com' and hardcoded UUIDs like
'018f33bb-1111-7222-8333-111111111111'); replace these literals with
Faker-generated values (use $this->faker->safeEmail(), $this->faker->uuid(), or
Str::uuid()->toString() where appropriate), assign them to variables at the top
of each test (e.g., $email, $userId, $ownerId) and reuse those variables in
setup, the call to startSignup and all assertions/mocks (look for methods
startSignup, finishSignup, and any occurrences of 'person@example.com',
'user-id', 'owner-id', 'missing-user-id' in this test class) so every test uses
consistent, generated test data instead of hardcoded strings.
In `@tests/Unit/User/Application/Passkey/PasskeyStoreTest.php`:
- Around line 58-60: Replace all hardcoded test identifiers and the email in
PasskeyStoreTest with Faker-generated values: use Faker to create unique IDs for
'user-id' and 'credential-id' (e.g., $this->faker->uuid or unique()->uuid),
generate the email instead of 'person@example.com' (e.g.,
$this->faker->safeEmail), and replace hardcoded device label 'Laptop' with a
Faker string (e.g., $this->faker->word or userAgent). Update the test
instantiation that uses new VerifiedPasskeyCredential('credential-id', ...) and
any other places in the test class where IDs/tokens/emails are hardcoded to
reference the Faker variables (ensure Faker is available on the test class,
e.g., via setUp or a trait). Ensure all occurrences flagged in the review
(IDs/tokens/emails in the test) are replaced consistently.
In `@tests/Unit/User/Application/Passkey/PasskeyUserCreatorTest.php`:
- Around line 35-183: The tests use hardcoded fixtures (emails, UUIDs, challenge
IDs/tokens, event IDs, and password literals) across helpers like
createSignupChallenge, createExpectedUser, createUserFactoryExpectingSignup,
createPasswordHasherExpectingRandomPassword, createEventIdFactory and
createRegisteredEventFactory; replace those literals by generating values once
with $this->faker (e.g. in setUp or class properties) and reuse those variables
in all helper methods so assertions/mocks still match (keep the same UUID
string, email, event-id, and hashed-password values but sourced from faker), and
ensure the password-hasher callback still validates a 64-char hex string while
the hasher mock returns the faker-derived hashed-password.
---
Duplicate comments:
In @.github/openapi-spec/spec.yaml:
- Around line 3041-3042: The OpenAPI spec is inconsistent: rememberMe is on
PasskeySignInOptionsDto but on PasskeySignUpCompleteDto for sign-up; to fix,
decide with the team to standardize on one pattern (recommended: options DTO
like PasskeySignUpOptionsDto to mirror PasskeySignInOptionsDto), then move the
rememberMe property from PasskeySignUpCompleteDto to PasskeySignUpOptionsDto in
the spec, and update all related references; also update the backend
handler/processor that consumes sign-up requests (the sign-up completion flow),
plus any affected tests and fixtures to accept rememberMe on the sign-up options
DTO (or conversely, if the team prefers completion DTO, move it from
PasskeySignInOptionsDto to PasskeySignUpCompleteDto and update backend/tests
accordingly).
---
Nitpick comments:
In @.env:
- Line 84: The PASSKEY_ALLOWED_ORIGINS environment variable is wrapped in
unnecessary quotes which may be read literally by env parsers and break origin
validation; update the .env entry for PASSKEY_ALLOWED_ORIGINS by removing the
surrounding double quotes so the value is unquoted
(PASSKEY_ALLOWED_ORIGINS=http://localhost,https://localhost,http://localhost:8080),
ensuring it matches how other no-space variables are defined.
In @.env.load_test:
- Line 29: The PASSKEY_ALLOWED_ORIGINS value is wrapped in quotes which can
become literal characters when loaded; remove the surrounding double quotes so
the environment entry for PASSKEY_ALLOWED_ORIGINS is an unquoted comma-separated
list (e.g.
PASSKEY_ALLOWED_ORIGINS=http://localhost,https://localhost,http://localhost:${LOAD_TEST_API_PORT}),
ensure there are no extra spaces, save the change and re-run the origin
validation to confirm it parses correctly.
In @.env.test:
- Line 31: The PASSKEY_ALLOWED_ORIGINS value in the .env.test file is wrapped in
double quotes which can lead to literal quotes being included when parsed and
break origin validation; update the PASSKEY_ALLOWED_ORIGINS entry by removing
the surrounding quotes so the value is an unquoted comma-separated list (refer
to the PASSKEY_ALLOWED_ORIGINS variable to locate the line) and ensure no spaces
or extra characters remain.
In `@specs/passkey-authentication/product-brief.md`:
- Line 21: Replace the header text "Non Goals" with the hyphenated form
"Non-Goals" to match product brief conventions and maintain consistency; locate
the heading "Non Goals" in the document (the section header) and update it to
"Non-Goals".
In `@tests/Unit/User/Application/Passkey/PasskeyRegistrationServiceTest.php`:
- Around line 284-292: In PasskeyRegistrationServiceTest update the anonymous
callback used in the willReturnCallback to mark the unused parameters as
intentionally unused by prefixing $id and $purpose with underscores (e.g. change
parameters in the static function signature to $_id and $_purpose while keeping
DateTimeImmutable $consumedAt unchanged); this will silence static analysis
warnings while preserving the callback behavior in the claimActive mock setup
that consumes $consumedAt and returns $challenge.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 538942b4-774d-4d75-9bf5-07847e9fc0e8
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (92)
.env.env.load_test.env.test.github/openapi-spec/spec.yamlcomposer.jsonconfig/api_platform/resources/EmptyResponse.yamlconfig/doctrine/User/PasskeyChallenge.mongodb.xmlconfig/doctrine/User/PasskeyCredential.mongodb.xmlconfig/packages/security.yamlconfig/services.yamlconfig/services_test.yamlconfig/validator/validation.yamldeptrac.yamldocs/advanced-configuration.mddocs/main.mddocs/passkey-authentication.mdspecs/passkey-authentication/architecture.mdspecs/passkey-authentication/epics.mdspecs/passkey-authentication/implementation-readiness.mdspecs/passkey-authentication/prd.mdspecs/passkey-authentication/product-brief.mdspecs/passkey-authentication/research.mdspecs/passkey-authentication/run-summary.mdsrc/Shared/Application/Resolver/RateLimit/ApiRateLimitAuthTargetResolver.phpsrc/Shared/Application/Resolver/RateLimit/ApiRateLimitRequestResolver.phpsrc/User/Application/DTO/PasskeyAuthenticationResult.phpsrc/User/Application/DTO/PasskeyOptionsResult.phpsrc/User/Application/DTO/PasskeyRegistrationCompleteDto.phpsrc/User/Application/DTO/PasskeyRegistrationOptionsDto.phpsrc/User/Application/DTO/PasskeySignInCompleteDto.phpsrc/User/Application/DTO/PasskeySignInOptionsDto.phpsrc/User/Application/DTO/PasskeySignUpCompleteDto.phpsrc/User/Application/DTO/PasskeySignUpOptionsDto.phpsrc/User/Application/DTO/VerifiedPasskeyCredential.phpsrc/User/Application/Factory/PasskeyVerifiedCredentialFactory.phpsrc/User/Application/Passkey/PasskeyAssertionCredentialRecordVerifier.phpsrc/User/Application/Passkey/PasskeyAttestationCredentialRecordVerifier.phpsrc/User/Application/Passkey/PasskeyAuthenticationService.phpsrc/User/Application/Passkey/PasskeyAuthenticationServiceInterface.phpsrc/User/Application/Passkey/PasskeyChallengeStore.phpsrc/User/Application/Passkey/PasskeyConfiguration.phpsrc/User/Application/Passkey/PasskeyCredentialResponseResolver.phpsrc/User/Application/Passkey/PasskeyCredentialStore.phpsrc/User/Application/Passkey/PasskeyCredentialVerifier.phpsrc/User/Application/Passkey/PasskeyCredentialVerifierInterface.phpsrc/User/Application/Passkey/PasskeyEncoding.phpsrc/User/Application/Passkey/PasskeyJsonCodec.phpsrc/User/Application/Passkey/PasskeyJsonCodecInterface.phpsrc/User/Application/Passkey/PasskeyOptionsFactory.phpsrc/User/Application/Passkey/PasskeyPublicKeyOptionsFactory.phpsrc/User/Application/Passkey/PasskeyRegistrationService.phpsrc/User/Application/Passkey/PasskeyRegistrationServiceInterface.phpsrc/User/Application/Passkey/PasskeyResponseFactory.phpsrc/User/Application/Passkey/PasskeySessionIssuer.phpsrc/User/Application/Passkey/PasskeyUserCreator.phpsrc/User/Application/Passkey/PasskeyUserResolver.phpsrc/User/Application/Passkey/PasskeyWebauthnFactory.phpsrc/User/Application/Passkey/PasskeyWebauthnFactoryInterface.phpsrc/User/Application/Processor/PasskeyRegistrationCompleteProcessor.phpsrc/User/Application/Processor/PasskeyRegistrationOptionsProcessor.phpsrc/User/Application/Processor/PasskeySignInCompleteProcessor.phpsrc/User/Application/Processor/PasskeySignInOptionsProcessor.phpsrc/User/Application/Processor/PasskeySignUpCompleteProcessor.phpsrc/User/Application/Processor/PasskeySignUpOptionsProcessor.phpsrc/User/Domain/Entity/PasskeyChallenge.phpsrc/User/Domain/Entity/PasskeyCredential.phpsrc/User/Domain/Repository/PasskeyChallengeRepositoryInterface.phpsrc/User/Domain/Repository/PasskeyCredentialRepositoryInterface.phpsrc/User/Domain/ValueObject/PasskeyChallengeContext.phpsrc/User/Infrastructure/Repository/MongoDBPasskeyChallengeRepository.phpsrc/User/Infrastructure/Repository/MongoDBPasskeyCredentialRepository.phptests/Behat/Support/TestClientIpHttpRequestContextResolver.phptests/Behat/UserContext/UserRequestContext.phptests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitAuthTargetResolverTest.phptests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitRequestResolverLimitersTest.phptests/Unit/User/Application/Passkey/PasskeyAuthenticationServiceTest.phptests/Unit/User/Application/Passkey/PasskeyAuthenticationServiceTestSupport.phptests/Unit/User/Application/Passkey/PasskeyConfigurationTest.phptests/Unit/User/Application/Passkey/PasskeyCredentialVerifierTest.phptests/Unit/User/Application/Passkey/PasskeyDtoTest.phptests/Unit/User/Application/Passkey/PasskeyEncodingTest.phptests/Unit/User/Application/Passkey/PasskeyJsonCodecTest.phptests/Unit/User/Application/Passkey/PasskeyOptionsFactoryTest.phptests/Unit/User/Application/Passkey/PasskeyRegistrationServiceTest.phptests/Unit/User/Application/Passkey/PasskeyRegistrationServiceTestSupport.phptests/Unit/User/Application/Passkey/PasskeyServiceTestObjects.phptests/Unit/User/Application/Passkey/PasskeyStoreTest.phptests/Unit/User/Application/Passkey/PasskeyUserCreatorTest.phptests/Unit/User/Application/Processor/PasskeyProcessorTest.phptests/Unit/User/Domain/Entity/PasskeyChallengeTest.phptests/Unit/User/Domain/Entity/PasskeyCredentialTest.phptests/Unit/User/Infrastructure/Repository/MongoDBPasskeyRepositoryTest.php
✅ Files skipped from review due to trivial changes (17)
- src/User/Application/DTO/PasskeyAuthenticationResult.php
- specs/passkey-authentication/implementation-readiness.md
- src/User/Application/Factory/PasskeyVerifiedCredentialFactory.php
- src/User/Application/DTO/PasskeyRegistrationOptionsDto.php
- src/User/Application/Passkey/PasskeyResponseFactory.php
- src/User/Domain/Repository/PasskeyChallengeRepositoryInterface.php
- config/doctrine/User/PasskeyCredential.mongodb.xml
- docs/main.md
- docs/advanced-configuration.md
- src/User/Application/Passkey/PasskeyChallengeStore.php
- docs/passkey-authentication.md
- specs/passkey-authentication/prd.md
- src/User/Application/DTO/VerifiedPasskeyCredential.php
- tests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitRequestResolverLimitersTest.php
- specs/passkey-authentication/research.md
- src/User/Application/Passkey/PasskeyJsonCodecInterface.php
- specs/passkey-authentication/architecture.md
🚧 Files skipped from review as they are similar to previous changes (49)
- composer.json
- src/User/Application/Passkey/PasskeyWebauthnFactoryInterface.php
- src/User/Application/DTO/PasskeyOptionsResult.php
- src/Shared/Application/Resolver/RateLimit/ApiRateLimitAuthTargetResolver.php
- src/Shared/Application/Resolver/RateLimit/ApiRateLimitRequestResolver.php
- src/User/Application/Passkey/PasskeySessionIssuer.php
- config/doctrine/User/PasskeyChallenge.mongodb.xml
- src/User/Application/Passkey/PasskeyUserResolver.php
- config/packages/security.yaml
- src/User/Application/Passkey/PasskeyAuthenticationServiceInterface.php
- src/User/Application/Passkey/PasskeyAttestationCredentialRecordVerifier.php
- src/User/Domain/Entity/PasskeyCredential.php
- src/User/Application/Passkey/PasskeyAuthenticationService.php
- src/User/Infrastructure/Repository/MongoDBPasskeyCredentialRepository.php
- config/services_test.yaml
- src/User/Application/Passkey/PasskeyAssertionCredentialRecordVerifier.php
- deptrac.yaml
- src/User/Application/Passkey/PasskeyCredentialResponseResolver.php
- tests/Behat/UserContext/UserRequestContext.php
- config/services.yaml
- tests/Behat/Support/TestClientIpHttpRequestContextResolver.php
- src/User/Domain/ValueObject/PasskeyChallengeContext.php
- tests/Unit/User/Application/Passkey/PasskeyAuthenticationServiceTestSupport.php
- tests/Unit/User/Domain/Entity/PasskeyCredentialTest.php
- src/User/Application/Passkey/PasskeyWebauthnFactory.php
- config/api_platform/resources/EmptyResponse.yaml
- src/User/Domain/Entity/PasskeyChallenge.php
- src/User/Application/Passkey/PasskeyConfiguration.php
- src/User/Infrastructure/Repository/MongoDBPasskeyChallengeRepository.php
- tests/Unit/User/Domain/Entity/PasskeyChallengeTest.php
- src/User/Application/Passkey/PasskeyCredentialVerifierInterface.php
- tests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitAuthTargetResolverTest.php
- src/User/Application/Passkey/PasskeyRegistrationService.php
- src/User/Domain/Repository/PasskeyCredentialRepositoryInterface.php
- tests/Unit/User/Application/Passkey/PasskeyRegistrationServiceTestSupport.php
- src/User/Application/Passkey/PasskeyCredentialVerifier.php
- tests/Unit/User/Application/Processor/PasskeyProcessorTest.php
- src/User/Application/Passkey/PasskeyPublicKeyOptionsFactory.php
- src/User/Application/Passkey/PasskeyOptionsFactory.php
- src/User/Application/Passkey/PasskeyJsonCodec.php
- src/User/Application/Passkey/PasskeyRegistrationServiceInterface.php
- tests/Unit/User/Infrastructure/Repository/MongoDBPasskeyRepositoryTest.php
- src/User/Application/Passkey/PasskeyUserCreator.php
- tests/Unit/User/Application/Passkey/PasskeyServiceTestObjects.php
- tests/Unit/User/Application/Passkey/PasskeyCredentialVerifierTest.php
- src/User/Application/Passkey/PasskeyCredentialStore.php
- tests/Unit/User/Application/Passkey/PasskeyConfigurationTest.php
- tests/Unit/User/Application/Passkey/PasskeyOptionsFactoryTest.php
- tests/Unit/User/Application/Passkey/PasskeyJsonCodecTest.php
2361672
|
Automated re-review requested after all review threads were resolved and CI is green. @coderabbitai review |
|
✅ Actions performedReview triggered.
|
Code Review: FAIL
Review OutputVerification Output |
Code Review: FAIL
Review OutputVerification Output |
Code Review: PASS
Review Output |
BMAD FR/NFR Review Gate: FAIL
Review OutputVerification Output |
BMAD FR/NFR Review Gate: FAIL
Review OutputVerification Output |
BMAD FR/NFR Review Gate: PASS
Review OutputVerification Output |
|
@coderabbitai review @cubic-dev-ai review Current head is |
|
✅ Action performedReview finished.
|
@Kravalg I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
0 issues found across 0 files (changes from recent commits).
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
|
Current head Status summary:
Remaining branch-protection blocker: |
Description
Adds passkey-based authentication support for sign-up, authenticated passkey registration, and sign-in using WebAuthn.
This includes:
/api/passkeys/*web-auth/webauthn-libRelated Issue
Closes #221
Motivation and Context
Users need a phishing-resistant authentication option that can support sign-up and sign-in without relying only on passwords. The implementation keeps passkey flows in the existing User bounded context and reuses existing session/token issuance patterns.
Performance Changes
PasskeyCredentialhas a uniquecredential_idindex for constant-time assertion credential lookup.PasskeyCredentialhas auser_idindex for authenticated user passkey listing and duplicate checks.PasskeyChallengehas a compound(purpose, user_id)index for challenge cleanup and lookup paths.PasskeyChallengehas a TTL index onexpires_atwithexpireAfterSeconds=0so MongoDB removes expired challenge records.PASSKEY_CHALLENGE_TTL_SECONDSandPASSKEY_TIMEOUT_SECONDS.How Has This Been Tested?
Local Docker verification:
phpmd src: passed.phpmd tests: passed.phpinsightssource: Code 100, Complexity 97.6, Architecture 100, Style 100.phpinsights analyse tests: Code 100, Complexity 97.9, Architecture 100, Style 100.psalm --show-info=false --no-progress: passed.psalm --taint-analysis --show-info=false --no-progress: passed.deptrac analyse --config-file=deptrac.yaml --report-uncovered --fail-on-uncovered: passed.bin/console lint:container: passed.git diff --check: passed.Note: literal
make cicould not run locally because another checkout owns the hardcoded development port8081. Equivalent Docker commands were run with isolated dependency ports, and Behat was run through an internal one-off FrankenPHP server withAPP_ENV=testandAPP_DEBUG=0.Screenshots (if appropriate):
N/A
Types of changes
Checklist:
Summary by cubic
Adds WebAuthn passkey sign‑up, authenticated passkey enrollment, and sign‑in over REST and GraphQL with strict validation, hardened rate limits, and a production‑readiness gate. Updates docs, schemas, CI, and tests; implements #221.
New Features
/api/passkeys/(signup|signin|register)/(options|complete)and GraphQL passkey mutations; options returnchallengeId+publicKey, completions return tokens/pendingSessionIdorcredentialId. OpenAPI/GraphQL specs updated and mutation descriptions normalized; public signup/signin routes allowed.web-auth/webauthn-libhandles attestation/assertion;PasskeyChallengeis TTL and single‑use;PasskeyCredentialis unique and discoverable‑only; public suffix list bundled.503until enabled) via env flags; docs and planning evidence added; Schemathesis covers passkey option routes; K6 load covers option routes; workflows pin Actions and restrict permissions; CI asserts test‑ and production‑mode readiness.Migration
PASSKEY_*envs (RP_ID,RP_NAME,ALLOWED_ORIGINS,TIMEOUT_SECONDS,CHALLENGE_TTL_SECONDS,PRODUCTION_TRAFFIC_ENABLED,PRODUCTION_MONITORING_READY).make passkey-test-readinessandmake passkey-production-readinessbefore enabling production traffic.Written for commit e9acebc. Summary will update on new commits.
Summary by CodeRabbit
New Features
Configuration
Documentation
Tests
BMAD Planning Evidence
specs/passkey-authentication/