fix(webauthn): apply PR #256 review fixes

[devondragon]

Summary

PR #256 was squash-merged before the review fixes were committed. This PR applies all 14 review fixes that were missing:

• Safety: safe-parse enums, default label, trim-before-length, TOCTOU race fix
• Data integrity: ON DELETE CASCADE, WebAuthnPreDeleteEventListener for cleanup on user deletion
• API quality: transports as List<String>, @NotBlank on path vars, @ConditionalOnProperty on advice
• Design: WebAuthnException → RuntimeException, WebAuthnAuthenticationToken, SecureRandom user handle, @Transactional(MANDATORY)

New files:

• WebAuthnPreDeleteEventListener + test
• WebAuthnAuthenticationToken

Test plan

• ./gradlew test passes
• All 17 changed files apply cleanly on top of main

[claude]

PR #258 Review

Overall this is a solid set of fixes. The changes are well-scoped, the new files follow the project's conventions, and the test additions cover the key new behaviors. A few issues worth addressing below.

---

Issues

1. SecureRandom instantiated per call (performance)

In WebAuthnUserEntityBridge.createUserEntity(), a new SecureRandom() is constructed on every call. SecureRandom construction involves OS entropy gathering and is non-trivial. Since WebAuthnUserEntityBridge is a Spring singleton, this should be a private static final SecureRandom SECURE_RANDOM = new SecureRandom() field. SecureRandom is thread-safe, so a shared instance is fine.

---

2. N+1 delete in WebAuthnPreDeleteEventListener (performance)

credentialRepository.findByUserEntity(userEntity).forEach(credentialRepository::delete);

This issues one DELETE per credential. Prefer credentialRepository.deleteAll(...) for a single batch, or add a deleteByUserEntity(WebAuthnUserEntity) repository method to avoid loading entities at all. With DB-level CASCADE already in place, deleting the userEntity directly and letting the DB cascade handle credentials would make the credential loop unnecessary.

---

3. Label trimmed for validation but not for storage (data integrity)

validateLabel now correctly trims before checking length, but renameCredential passes the raw newLabel to the repository without trimming it first. A label like " my key " passes validation but is stored with surrounding spaces. The label should be trimmed before the update call.

---

4. Redundant /* no checked exception */ test comments (style)

Seven test methods now read:

void shouldRenameSuccessfully() /* no checked exception */ {

The comment is noise — the absence of a throws clause speaks for itself. Just removing throws WebAuthnException is sufficient.

---

5. PostgreSQL schema not updated

Only db-scripts/mariadb-schema.sql received the ON DELETE CASCADE additions. If a PostgreSQL schema file exists or is added later, it will need the same treatment.

---

Minor observations

@Transactional(propagation = MANDATORY) on lockAndCountCredentials — correct, but the existing Javadoc does not mention this requirement. Callers need to know they must be within an active transaction.

parseTransportList duplication — WebAuthnCredentialQueryRepository and WebAuthnRepositoryConfig both contain nearly identical split/trim/filter transport parsing logic. The return types differ (List<String> vs Set<AuthenticatorTransport>), but a shared utility or a cross-reference comment would reduce future drift.

WebAuthnAuthenticationToken — clean design. Calling setAuthenticated(true) in the constructor follows the same pattern as UsernamePasswordAuthenticationToken.authenticated() internally. The Javadoc explaining why getCredentials() returns null is helpful.

TOCTOU retry in findByUsername — reasonable. If the retry lookup also returns empty the method returns null; this edge case appears acceptable for the registration flow.

---

What is working well

• Safe enum parsing (PublicKeyCredentialType, AuthenticatorTransport) with warn-and-skip is correct for stored data that could have been written by an older schema version.
• @NotBlank on path variables closes a real gap — a blank credential ID would previously have been a silent no-op at the framework level.
• The TOCTOU lock (Propagation.MANDATORY + pessimistic lock on credential count before delete) is correctly structured.
• WebAuthnAuthenticationToken makes passkey sessions distinguishable from password sessions — good for audit logging and future conditional security rules.
• WebAuthnPreDeleteEventListener test coverage handles both the happy path and the no-WebAuthn-data path cleanly.

[Copilot]

Pull request overview

Applies the missing post-review fixes from PR #256 to harden and refine the WebAuthn/passkey feature (safety, data integrity, API quality, and security design adjustments) in the Spring User Framework.

Changes:

• Convert WebAuthnException to an unchecked exception and update impacted services/controllers/tests accordingly.
• Improve WebAuthn persistence and API ergonomics (default label, safer enum parsing, transports as List<String>, validation on path vars, conditional advice).
• Add cleanup behavior and schema integrity improvements (pre-delete listener + test, ON DELETE CASCADE, TOCTOU locking).

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/main/java/com/digitalsanctuary/spring/user/service/WebAuthnCredentialManagementService.java	Removes checked exception usage; strengthens label validation and deletion locking logic.
src/main/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnUserEntityBridge.java	Adds transactional lookup and attempts to handle concurrent entity creation; switches user handle to random bytes.
src/main/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnCredentialQueryRepository.java	Adds mandatory-transaction locking and returns transports as a parsed list.
src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnRepositoryConfig.java	Defaults missing labels and safely parses enums/transports with logging.
src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationToken.java	Introduces a dedicated token type for WebAuthn-authenticated sessions.
src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandler.java	Switches converted authentication to the new WebAuthn token.
src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java	Adds @NotBlank path var validation and removes checked exception declaration.
src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdvice.java	Makes advice conditional on WebAuthn being enabled.
src/main/java/com/digitalsanctuary/spring/user/dto/WebAuthnCredentialInfo.java	Changes transports field from String to List<String>.
src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListener.java	Adds a pre-delete listener to clean up WebAuthn data.
src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnException.java	Changes exception type to RuntimeException and updates docs accordingly.
src/test/java/com/digitalsanctuary/spring/user/service/WebAuthnCredentialManagementServiceTest.java	Updates tests to match unchecked exception behavior.
src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java	Updates tests to match unchecked exception behavior.
src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandlerTest.java	Updates expected authentication token type to WebAuthnAuthenticationToken.
src/test/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnUserEntityBridgeTest.java	Adds coverage for concurrent-creation retry path and random handle sizing.
src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListenerTest.java	Adds tests for user pre-delete cleanup behavior.
db-scripts/mariadb-schema.sql	Adds ON DELETE CASCADE to WebAuthn-related foreign keys.

Comments suppressed due to low confidence (1)

src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListener.java:46

• Now that the schema adds ON DELETE CASCADE from user_credentials → user_entities, deleting credentials one-by-one before deleting the user entity is redundant and adds extra queries. If the project intends to rely on the DB cascade for cleanup, this method can delete only the WebAuthnUserEntity (or use a bulk delete) to reduce round-trips; otherwise, consider documenting why manual deletes are still needed.

	private void deleteUserEntityAndCredentials(WebAuthnUserEntity userEntity) {
		credentialRepository.findByUserEntity(userEntity).forEach(credentialRepository::delete);
		userEntityRepository.delete(userEntity);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

validateLabel checks label.trim().length() but renameCredential persists the original newLabel unchanged. This allows inputs like 64 chars plus trailing spaces to pass validation but exceed the DB column length (VARCHAR(64)), causing truncation or a persistence error. Consider normalizing once (e.g., trim) and validating/persisting the normalized value so the stored label can never exceed 64 characters.

[Copilot]

The concurrent-creation handling is unlikely to work reliably with JPA: webAuthnUserEntityRepository.save(entity) typically does not flush immediately, so a unique-constraint conflict may surface at transaction commit rather than inside this try/catch, bypassing the retry path. To make the retry deterministic, force the insert to flush within the try block (e.g., via saveAndFlush/flush) and consider running the retry lookup in a clean/new persistence context/transaction after a constraint violation.