From ee1750ff55318cd6f377d16e854657a135ba6519 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Sat, 21 Feb 2026 17:35:55 -0700 Subject: [PATCH] fix(webauthn): apply PR #256 review fixes Apply all 14 review fixes that were missing from the squash merge: - Safe-parse AuthenticatorTransport enums, default label, trim-before-length - Fix TOCTOU race in last-credential protection with in-transaction recount - Add ON DELETE CASCADE to WebAuthn foreign keys in schema - Add WebAuthnPreDeleteEventListener for cleanup on user deletion - Add WebAuthnAuthenticationToken for passkey session distinction - Change transports to List in credential response DTO - Add @NotBlank on path variable parameters in management API - Add @ConditionalOnProperty to WebAuthnManagementAPIAdvice - Change WebAuthnException to extend RuntimeException - Use SecureRandom for user handle generation - Use @Transactional(propagation = MANDATORY) on credential operations - Update all tests for new behavior --- db-scripts/mariadb-schema.sql | 4 +- .../user/api/WebAuthnManagementAPI.java | 9 +- .../user/api/WebAuthnManagementAPIAdvice.java | 2 + .../user/dto/WebAuthnCredentialInfo.java | 3 +- .../user/exceptions/WebAuthnException.java | 4 +- .../WebAuthnPreDeleteEventListener.java | 49 ++++++++++ .../WebAuthnCredentialQueryRepository.java | 15 ++- .../repository/WebAuthnUserEntityBridge.java | 26 +++--- .../WebAuthnAuthenticationSuccessHandler.java | 3 +- .../security/WebAuthnAuthenticationToken.java | 49 ++++++++++ .../security/WebAuthnRepositoryConfig.java | 26 ++++-- .../WebAuthnCredentialManagementService.java | 8 +- .../user/api/WebAuthnManagementAPITest.java | 12 +-- .../WebAuthnPreDeleteEventListenerTest.java | 93 +++++++++++++++++++ .../WebAuthnUserEntityBridgeTest.java | 30 ++++++ ...AuthnAuthenticationSuccessHandlerTest.java | 2 +- ...bAuthnCredentialManagementServiceTest.java | 6 +- 17 files changed, 295 insertions(+), 46 deletions(-) create mode 100644 src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListener.java create mode 100644 src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationToken.java create mode 100644 src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListenerTest.java diff --git a/db-scripts/mariadb-schema.sql b/db-scripts/mariadb-schema.sql index d6501e5..5b8f4be 100644 --- a/db-scripts/mariadb-schema.sql +++ b/db-scripts/mariadb-schema.sql @@ -106,7 +106,7 @@ CREATE TABLE `user_entities` ( PRIMARY KEY (`id`), UNIQUE KEY `UK_user_entities_name` (`name`), KEY `FK_user_entities_user` (`user_account_id`), - CONSTRAINT `FK_user_entities_user` FOREIGN KEY (`user_account_id`) REFERENCES `user_account` (`id`) + CONSTRAINT `FK_user_entities_user` FOREIGN KEY (`user_account_id`) REFERENCES `user_account` (`id`) ON DELETE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; CREATE TABLE `user_credentials` ( @@ -126,5 +126,5 @@ CREATE TABLE `user_credentials` ( `label` VARCHAR(64) NOT NULL, PRIMARY KEY (`credential_id`), KEY `FK_user_credentials_entity` (`user_entity_user_id`), - CONSTRAINT `FK_user_credentials_entity` FOREIGN KEY (`user_entity_user_id`) REFERENCES `user_entities` (`id`) + CONSTRAINT `FK_user_credentials_entity` FOREIGN KEY (`user_entity_user_id`) REFERENCES `user_entities` (`id`) ON DELETE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; diff --git a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java index 6030fa6..24fa845 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java @@ -13,7 +13,6 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; import com.digitalsanctuary.spring.user.dto.WebAuthnCredentialInfo; -import com.digitalsanctuary.spring.user.exceptions.WebAuthnException; import com.digitalsanctuary.spring.user.exceptions.WebAuthnUserNotFoundException; import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.service.UserService; @@ -96,9 +95,9 @@ public ResponseEntity hasCredentials(@AuthenticationPrincipal UserDetai * @return ResponseEntity with success message or error */ @PutMapping("/credentials/{id}/label") - public ResponseEntity renameCredential(@PathVariable @Size(max = 512) String id, + public ResponseEntity renameCredential(@PathVariable @NotBlank @Size(max = 512) String id, @RequestBody @Valid RenameCredentialRequest request, - @AuthenticationPrincipal UserDetails userDetails) throws WebAuthnException { + @AuthenticationPrincipal UserDetails userDetails) { User user = findAuthenticatedUser(userDetails); credentialManagementService.renameCredential(id, request.label(), user); return ResponseEntity.ok(new GenericResponse("Passkey renamed successfully")); @@ -121,8 +120,8 @@ public ResponseEntity renameCredential(@PathVariable @Size(max * @return ResponseEntity with success message or error */ @DeleteMapping("/credentials/{id}") - public ResponseEntity deleteCredential(@PathVariable @Size(max = 512) String id, - @AuthenticationPrincipal UserDetails userDetails) throws WebAuthnException { + public ResponseEntity deleteCredential(@PathVariable @NotBlank @Size(max = 512) String id, + @AuthenticationPrincipal UserDetails userDetails) { User user = findAuthenticatedUser(userDetails); credentialManagementService.deleteCredential(id, user); return ResponseEntity.ok(new GenericResponse("Passkey deleted successfully")); diff --git a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdvice.java b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdvice.java index a34fe18..5c16304 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdvice.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdvice.java @@ -1,5 +1,6 @@ package com.digitalsanctuary.spring.user.api; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.MethodArgumentNotValidException; @@ -15,6 +16,7 @@ * Centralized exception handling for WebAuthn credential management endpoints. */ @RestControllerAdvice(assignableTypes = WebAuthnManagementAPI.class) +@ConditionalOnProperty(name = "user.webauthn.enabled", havingValue = "true", matchIfMissing = false) @Slf4j public class WebAuthnManagementAPIAdvice { diff --git a/src/main/java/com/digitalsanctuary/spring/user/dto/WebAuthnCredentialInfo.java b/src/main/java/com/digitalsanctuary/spring/user/dto/WebAuthnCredentialInfo.java index 924621c..cc166cb 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/dto/WebAuthnCredentialInfo.java +++ b/src/main/java/com/digitalsanctuary/spring/user/dto/WebAuthnCredentialInfo.java @@ -1,6 +1,7 @@ package com.digitalsanctuary.spring.user.dto; import java.time.Instant; +import java.util.List; import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Data; @@ -28,7 +29,7 @@ public class WebAuthnCredentialInfo { private Instant lastUsed; /** Supported transports (usb, nfc, ble, internal). */ - private String transports; + private List transports; /** Whether credential is backup-eligible (synced passkey). */ private Boolean backupEligible; diff --git a/src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnException.java b/src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnException.java index ac3f1b4..5183969 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnException.java +++ b/src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnException.java @@ -4,7 +4,7 @@ * Exception thrown for WebAuthn-related errors. * *

- * This is a checked exception used to signal WebAuthn-specific business logic errors such as: + * This is an unchecked exception used to signal WebAuthn-specific business logic errors such as: *

*
    *
  • Attempting to delete the last passkey when user has no password
    • @@ -13,7 +13,7 @@ *
  • User not found during credential operations
    • *
*/ -public class WebAuthnException extends Exception { +public class WebAuthnException extends RuntimeException { /** Serial Version UID. */ private static final long serialVersionUID = 1L; diff --git a/src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListener.java b/src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListener.java new file mode 100644 index 0000000..ed68f7f --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListener.java @@ -0,0 +1,49 @@ +package com.digitalsanctuary.spring.user.listener; + +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.context.event.EventListener; +import org.springframework.stereotype.Component; +import com.digitalsanctuary.spring.user.event.UserPreDeleteEvent; +import com.digitalsanctuary.spring.user.persistence.model.WebAuthnUserEntity; +import com.digitalsanctuary.spring.user.persistence.repository.WebAuthnCredentialRepository; +import com.digitalsanctuary.spring.user.persistence.repository.WebAuthnUserEntityRepository; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; + +/** + * Listens for {@link UserPreDeleteEvent} and cleans up all WebAuthn data for the user being deleted. + * + *

+ * Deletes all credentials associated with the user's WebAuthn entity, then deletes the entity itself. + * This listener is synchronous so that failures cause the enclosing transaction to roll back, preventing + * orphaned user accounts. + *

+ */ +@Slf4j +@Component +@ConditionalOnProperty(name = "user.webauthn.enabled", havingValue = "true", matchIfMissing = false) +@RequiredArgsConstructor +public class WebAuthnPreDeleteEventListener { + + private final WebAuthnCredentialRepository credentialRepository; + private final WebAuthnUserEntityRepository userEntityRepository; + + /** + * Handle user pre-delete by removing all WebAuthn credentials and the user entity. + * + * @param event the user pre-delete event + */ + @EventListener + public void onUserPreDelete(UserPreDeleteEvent event) { + Long userId = event.getUserId(); + log.debug("Cleaning up WebAuthn data for user {}", userId); + + userEntityRepository.findByUserId(userId).ifPresent(this::deleteUserEntityAndCredentials); + } + + private void deleteUserEntityAndCredentials(WebAuthnUserEntity userEntity) { + credentialRepository.findByUserEntity(userEntity).forEach(credentialRepository::delete); + userEntityRepository.delete(userEntity); + log.info("Deleted WebAuthn data for user entity {}", userEntity.getName()); + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnCredentialQueryRepository.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnCredentialQueryRepository.java index 09a4b3c..1d97182 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnCredentialQueryRepository.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnCredentialQueryRepository.java @@ -1,10 +1,13 @@ package com.digitalsanctuary.spring.user.persistence.repository; +import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Optional; import java.util.stream.Collectors; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.stereotype.Repository; +import org.springframework.transaction.annotation.Propagation; import org.springframework.transaction.annotation.Transactional; import com.digitalsanctuary.spring.user.dto.WebAuthnCredentialInfo; import com.digitalsanctuary.spring.user.persistence.model.WebAuthnCredential; @@ -59,6 +62,7 @@ public long countCredentials(Long userId) { * @param userId the user ID * @return count of locked credential rows */ + @Transactional(propagation = Propagation.MANDATORY) public long lockAndCountCredentials(Long userId) { return credentialRepository.lockCredentialIdsByUserEntityUserId(userId).size(); } @@ -135,9 +139,18 @@ private Optional findCredentialForUser(String credentialId, * @return the DTO */ private WebAuthnCredentialInfo toCredentialInfo(WebAuthnCredential entity) { + List transportList = parseTransportList(entity.getAuthenticatorTransports()); return WebAuthnCredentialInfo.builder().id(entity.getCredentialId()).label(entity.getLabel()) .created(entity.getCreated()).lastUsed(entity.getLastUsed()) - .transports(entity.getAuthenticatorTransports()).backupEligible(entity.isBackupEligible()) + .transports(transportList).backupEligible(entity.isBackupEligible()) .backupState(entity.isBackupState()).build(); } + + private List parseTransportList(String transports) { + if (transports == null || transports.isEmpty()) { + return Collections.emptyList(); + } + return Arrays.stream(transports.split(",")).map(String::trim).filter(s -> !s.isEmpty()) + .collect(Collectors.toList()); + } } diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnUserEntityBridge.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnUserEntityBridge.java index e0be162..77ec6ce 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnUserEntityBridge.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnUserEntityBridge.java @@ -1,10 +1,11 @@ package com.digitalsanctuary.spring.user.persistence.repository; -import java.nio.ByteBuffer; +import java.security.SecureRandom; import java.util.Base64; import java.util.Optional; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.context.annotation.Primary; +import org.springframework.dao.DataIntegrityViolationException; import org.springframework.security.web.webauthn.api.Bytes; import org.springframework.security.web.webauthn.api.ImmutablePublicKeyCredentialUserEntity; import org.springframework.security.web.webauthn.api.PublicKeyCredentialUserEntity; @@ -45,6 +46,7 @@ public PublicKeyCredentialUserEntity findById(Bytes id) { } @Override + @Transactional public PublicKeyCredentialUserEntity findByUsername(String username) { // Handle edge cases that can occur during login if (username == null || username.isEmpty() || "anonymousUser".equals(username)) { @@ -65,8 +67,13 @@ public PublicKeyCredentialUserEntity findByUsername(String username) { return null; } - // Create WebAuthn user entity for this application user - return createUserEntity(user); + // Create WebAuthn user entity for this application user, handling concurrent creation + try { + return createUserEntity(user); + } catch (DataIntegrityViolationException e) { + log.debug("Concurrent WebAuthn user entity creation for {}, retrying lookup", username); + return webAuthnUserEntityRepository.findByName(username).map(this::toSpringSecurityEntity).orElse(null); + } } @Override @@ -107,7 +114,9 @@ public void delete(Bytes id) { */ @Transactional public PublicKeyCredentialUserEntity createUserEntity(User user) { - Bytes userId = new Bytes(longToBytes(user.getId())); + byte[] randomHandle = new byte[64]; + new SecureRandom().nextBytes(randomHandle); + Bytes userId = new Bytes(randomHandle); String idStr = Base64.getUrlEncoder().withoutPadding().encodeToString(userId.getBytes()); String displayName = user.getFullName(); @@ -148,13 +157,4 @@ private PublicKeyCredentialUserEntity toSpringSecurityEntity(WebAuthnUserEntity .displayName(entity.getDisplayName()).build(); } - /** - * Convert Long ID to byte array for WebAuthn user ID. - * - * @param value the Long value to convert - * @return byte array representation of the Long - */ - private byte[] longToBytes(Long value) { - return ByteBuffer.allocate(Long.BYTES).putLong(value).array(); - } } diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandler.java b/src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandler.java index 26d3e83..fc1642c 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandler.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandler.java @@ -1,7 +1,6 @@ package com.digitalsanctuary.spring.user.security; import java.io.IOException; -import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; @@ -73,7 +72,7 @@ public void onAuthenticationSuccess(HttpServletRequest request, HttpServletRespo UserDetails userDetails = userDetailsService.loadUserByUsername(username); // Create new authentication with DSUserDetails as principal, preserving authorities - Authentication convertedAuth = UsernamePasswordAuthenticationToken.authenticated(userDetails, null, + Authentication convertedAuth = new WebAuthnAuthenticationToken(userDetails, authentication.getAuthorities()); // Update SecurityContext with the converted authentication diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationToken.java b/src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationToken.java new file mode 100644 index 0000000..a3fded2 --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationToken.java @@ -0,0 +1,49 @@ +package com.digitalsanctuary.spring.user.security; + +import java.util.Collection; +import org.springframework.security.authentication.AbstractAuthenticationToken; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.userdetails.UserDetails; + +/** + * Authentication token representing a successful WebAuthn/passkey authentication. + * + *

+ * This token replaces the use of {@link org.springframework.security.authentication.UsernamePasswordAuthenticationToken} + * for WebAuthn logins, making it possible to distinguish passkey-based sessions from password-based sessions + * in security logic and audit trails. + *

+ * + *

+ * The principal is the application's {@link UserDetails} (typically {@code DSUserDetails}), and + * {@link #getCredentials()} returns {@code null} because no password is involved in WebAuthn authentication. + *

+ */ +public class WebAuthnAuthenticationToken extends AbstractAuthenticationToken { + + private static final long serialVersionUID = 1L; + + private final UserDetails principal; + + /** + * Creates a new WebAuthn authentication token. + * + * @param principal the authenticated user details + * @param authorities the granted authorities + */ + public WebAuthnAuthenticationToken(UserDetails principal, Collection authorities) { + super(authorities); + this.principal = principal; + setAuthenticated(true); + } + + @Override + public Object getCredentials() { + return null; + } + + @Override + public Object getPrincipal() { + return principal; + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnRepositoryConfig.java b/src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnRepositoryConfig.java index 3bc1955..8720d97 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnRepositoryConfig.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnRepositoryConfig.java @@ -102,7 +102,7 @@ public void save(CredentialRecord record) { : null); entity.setCreated(record.getCreated()); entity.setLastUsed(record.getLastUsed()); - entity.setLabel(record.getLabel()); + entity.setLabel(record.getLabel() != null ? record.getLabel() : "Passkey"); credentialRepository.save(entity); } @@ -131,15 +131,23 @@ public void delete(Bytes credentialId) { * Convert a JPA entity to Spring Security's CredentialRecord. */ private CredentialRecord toCredentialRecord(WebAuthnCredential entity) { + PublicKeyCredentialType credType = null; + if (entity.getPublicKeyCredentialType() != null) { + try { + credType = PublicKeyCredentialType.valueOf(entity.getPublicKeyCredentialType()); + } catch (IllegalArgumentException e) { + log.warn("Unknown PublicKeyCredentialType '{}' for credential {}, defaulting to null", + entity.getPublicKeyCredentialType(), entity.getCredentialId()); + } + } + return ImmutableCredentialRecord.builder() .credentialId(new Bytes(Base64.getUrlDecoder().decode(entity.getCredentialId()))) .userEntityUserId(new Bytes(Base64.getUrlDecoder().decode(entity.getUserEntity().getId()))) .publicKey(new ImmutablePublicKeyCose(entity.getPublicKey())) .signatureCount(entity.getSignatureCount()).uvInitialized(entity.isUvInitialized()) .backupEligible(entity.isBackupEligible()).backupState(entity.isBackupState()) - .credentialType(entity.getPublicKeyCredentialType() != null - ? PublicKeyCredentialType.valueOf(entity.getPublicKeyCredentialType()) - : null) + .credentialType(credType) .transports(parseTransports(entity.getAuthenticatorTransports())) .attestationObject( entity.getAttestationObject() != null ? new Bytes(entity.getAttestationObject()) : null) @@ -156,8 +164,14 @@ private Set parseTransports(String transports) { if (transports == null || transports.isEmpty()) { return Collections.emptySet(); } - return Arrays.stream(transports.split(",")).map(String::trim).filter(s -> !s.isEmpty()) - .map(AuthenticatorTransport::valueOf).collect(Collectors.toSet()); + return Arrays.stream(transports.split(",")).map(String::trim).filter(s -> !s.isEmpty()).map(value -> { + try { + return AuthenticatorTransport.valueOf(value); + } catch (IllegalArgumentException e) { + log.warn("Unknown AuthenticatorTransport '{}', skipping", value); + return null; + } + }).filter(java.util.Objects::nonNull).collect(Collectors.toSet()); } /** diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/WebAuthnCredentialManagementService.java b/src/main/java/com/digitalsanctuary/spring/user/service/WebAuthnCredentialManagementService.java index 94b4db0..4724a2c 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/WebAuthnCredentialManagementService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/WebAuthnCredentialManagementService.java @@ -68,7 +68,7 @@ public boolean hasCredentials(User user) { * @throws WebAuthnException if the credential is not found, access is denied, or the label is invalid */ @Transactional - public void renameCredential(String credentialId, String newLabel, User user) throws WebAuthnException { + public void renameCredential(String credentialId, String newLabel, User user) { validateLabel(newLabel); int updated = credentialQueryRepository.renameCredential(credentialId, newLabel, user.getId()); @@ -101,7 +101,7 @@ public void renameCredential(String credentialId, String newLabel, User user) th * @throws WebAuthnException if the credential is not found, access is denied, or deletion would lock out the user */ @Transactional - public void deleteCredential(String credentialId, User user) throws WebAuthnException { + public void deleteCredential(String credentialId, User user) { // Lock all user credentials before checking count and deleting to avoid TOCTOU races. long enabledCount = credentialQueryRepository.lockAndCountCredentials(user.getId()); @@ -133,11 +133,11 @@ public void deleteCredential(String credentialId, User user) throws WebAuthnExce * @param label the label to validate * @throws WebAuthnException if the label is invalid */ - private void validateLabel(String label) throws WebAuthnException { + private void validateLabel(String label) { if (label == null || label.trim().isEmpty()) { throw new WebAuthnException("Credential label cannot be empty"); } - if (label.length() > 64) { + if (label.trim().length() > 64) { throw new WebAuthnException("Credential label too long (max 64 characters)"); } } diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java index b6d8854..6bac0bf 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java @@ -154,7 +154,7 @@ class RenameCredentialTests { @Test @DisplayName("should rename credential successfully") - void shouldRenameSuccessfully() throws WebAuthnException { + void shouldRenameSuccessfully() /* no checked exception */ { // Given WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop"); @@ -169,7 +169,7 @@ void shouldRenameSuccessfully() throws WebAuthnException { @Test @DisplayName("should throw when rename fails") - void shouldThrowOnFailure() throws WebAuthnException { + void shouldThrowOnFailure() /* no checked exception */ { // Given WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("New Name"); doThrow(new WebAuthnException("Credential not found or access denied")).when(credentialManagementService) @@ -182,7 +182,7 @@ void shouldThrowOnFailure() throws WebAuthnException { @Test @DisplayName("should throw not found when user not found") - void shouldThrowNotFoundWhenUserNotFound() throws WebAuthnException { + void shouldThrowNotFoundWhenUserNotFound() /* no checked exception */ { // Given WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("New Name"); when(userService.findUserByEmail(testUser.getEmail())).thenReturn(null); @@ -200,7 +200,7 @@ class DeleteCredentialTests { @Test @DisplayName("should delete credential successfully") - void shouldDeleteSuccessfully() throws WebAuthnException { + void shouldDeleteSuccessfully() /* no checked exception */ { // When ResponseEntity response = api.deleteCredential("cred-1", userDetails); @@ -212,7 +212,7 @@ void shouldDeleteSuccessfully() throws WebAuthnException { @Test @DisplayName("should throw when delete fails") - void shouldThrowOnFailure() throws WebAuthnException { + void shouldThrowOnFailure() /* no checked exception */ { // Given doThrow(new WebAuthnException("Cannot delete last passkey")).when(credentialManagementService).deleteCredential(eq("cred-1"), any(User.class)); @@ -224,7 +224,7 @@ void shouldThrowOnFailure() throws WebAuthnException { @Test @DisplayName("should throw not found when user not found") - void shouldThrowNotFoundWhenUserNotFound() throws WebAuthnException { + void shouldThrowNotFoundWhenUserNotFound() /* no checked exception */ { // Given when(userService.findUserByEmail(testUser.getEmail())).thenReturn(null); diff --git a/src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListenerTest.java b/src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListenerTest.java new file mode 100644 index 0000000..9d28f56 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListenerTest.java @@ -0,0 +1,93 @@ +package com.digitalsanctuary.spring.user.listener; + +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import java.util.List; +import java.util.Optional; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import com.digitalsanctuary.spring.user.event.UserPreDeleteEvent; +import com.digitalsanctuary.spring.user.persistence.model.User; +import com.digitalsanctuary.spring.user.persistence.model.WebAuthnCredential; +import com.digitalsanctuary.spring.user.persistence.model.WebAuthnUserEntity; +import com.digitalsanctuary.spring.user.persistence.repository.WebAuthnCredentialRepository; +import com.digitalsanctuary.spring.user.persistence.repository.WebAuthnUserEntityRepository; +import com.digitalsanctuary.spring.user.test.annotations.ServiceTest; +import com.digitalsanctuary.spring.user.test.fixtures.TestFixtures; + +@ServiceTest +@DisplayName("WebAuthnPreDeleteEventListener Tests") +class WebAuthnPreDeleteEventListenerTest { + + @Mock + private WebAuthnCredentialRepository credentialRepository; + + @Mock + private WebAuthnUserEntityRepository userEntityRepository; + + @InjectMocks + private WebAuthnPreDeleteEventListener listener; + + private User testUser; + + @BeforeEach + void setUp() { + testUser = TestFixtures.Users.standardUser(); + } + + @Nested + @DisplayName("User Pre-Delete Handling") + class UserPreDeleteTests { + + @Test + @DisplayName("should delete credentials and user entity when user has WebAuthn data") + void shouldDeleteWebAuthnDataForUser() { + // Given + WebAuthnUserEntity userEntity = new WebAuthnUserEntity(); + userEntity.setId("entity-id"); + userEntity.setName(testUser.getEmail()); + userEntity.setUser(testUser); + + WebAuthnCredential cred1 = new WebAuthnCredential(); + cred1.setCredentialId("cred-1"); + cred1.setUserEntity(userEntity); + + WebAuthnCredential cred2 = new WebAuthnCredential(); + cred2.setCredentialId("cred-2"); + cred2.setUserEntity(userEntity); + + when(userEntityRepository.findByUserId(testUser.getId())).thenReturn(Optional.of(userEntity)); + when(credentialRepository.findByUserEntity(userEntity)).thenReturn(List.of(cred1, cred2)); + + UserPreDeleteEvent event = new UserPreDeleteEvent(this, testUser); + + // When + listener.onUserPreDelete(event); + + // Then + verify(credentialRepository).delete(cred1); + verify(credentialRepository).delete(cred2); + verify(userEntityRepository).delete(userEntity); + } + + @Test + @DisplayName("should do nothing when user has no WebAuthn data") + void shouldDoNothingWhenNoWebAuthnData() { + // Given + when(userEntityRepository.findByUserId(testUser.getId())).thenReturn(Optional.empty()); + + UserPreDeleteEvent event = new UserPreDeleteEvent(this, testUser); + + // When + listener.onUserPreDelete(event); + + // Then + verify(userEntityRepository, never()).delete(org.mockito.ArgumentMatchers.any(WebAuthnUserEntity.class)); + } + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnUserEntityBridgeTest.java b/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnUserEntityBridgeTest.java index 4fdfa58..136ba48 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnUserEntityBridgeTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnUserEntityBridgeTest.java @@ -11,6 +11,7 @@ import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; import org.mockito.Mock; +import org.springframework.dao.DataIntegrityViolationException; import org.springframework.security.web.webauthn.api.PublicKeyCredentialUserEntity; import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.persistence.model.WebAuthnUserEntity; @@ -85,6 +86,25 @@ void shouldReturnEmptyWhenNoUser() { assertThat(result).isEmpty(); } + @Test + @DisplayName("should retry lookup on concurrent creation conflict") + void shouldRetryOnDataIntegrityViolation() { + // Given - no existing entity, user exists, but save throws due to concurrent creation + when(webAuthnUserEntityRepository.findByName(testUser.getEmail())) + .thenReturn(Optional.empty()) // first call during findByUsername + .thenReturn(Optional.of(createJpaEntity())); // retry call after conflict + when(userRepository.findByEmail(testUser.getEmail())).thenReturn(testUser); + when(webAuthnUserEntityRepository.save(any(WebAuthnUserEntity.class))) + .thenThrow(new DataIntegrityViolationException("Duplicate entry")); + + // When + PublicKeyCredentialUserEntity result = bridge.findByUsername(testUser.getEmail()); + + // Then - should return entity from retry lookup + assertThat(result).isNotNull(); + assertThat(result.getName()).isEqualTo(testUser.getEmail()); + } + @Test @DisplayName("should return existing entity when found by name") void shouldReturnExistingEntity() { @@ -105,6 +125,14 @@ void shouldReturnExistingEntity() { } } + private WebAuthnUserEntity createJpaEntity() { + WebAuthnUserEntity jpaEntity = new WebAuthnUserEntity(); + jpaEntity.setId("AAAAAAAAAAE"); + jpaEntity.setName(testUser.getEmail()); + jpaEntity.setDisplayName(testUser.getFullName()); + return jpaEntity; + } + @Nested @DisplayName("Create User Entity") class CreateUserEntityTests { @@ -123,6 +151,8 @@ void shouldCreateEntityWithCorrectFields() { assertThat(entity.getName()).isEqualTo(testUser.getEmail()); assertThat(entity.getDisplayName()).isEqualTo(testUser.getFullName()); assertThat(entity.getId()).isNotNull(); + // Random handle should be 64 bytes + assertThat(entity.getId().getBytes()).hasSize(64); // Verify the JPA entity was saved with the user link ArgumentCaptor captor = ArgumentCaptor.forClass(WebAuthnUserEntity.class); diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandlerTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandlerTest.java index e25ad6b..c36bb6d 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandlerTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandlerTest.java @@ -81,7 +81,7 @@ void shouldConvertWebAuthnPrincipalToDSUserDetails() throws Exception { authCaptor.capture()); Authentication convertedAuth = authCaptor.getValue(); - assertThat(convertedAuth).isInstanceOf(UsernamePasswordAuthenticationToken.class); + assertThat(convertedAuth).isInstanceOf(WebAuthnAuthenticationToken.class); assertThat(convertedAuth.getPrincipal()).isInstanceOf(DSUserDetails.class); assertThat(((DSUserDetails) convertedAuth.getPrincipal()).getUser()).isEqualTo(testUser); } diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/WebAuthnCredentialManagementServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/WebAuthnCredentialManagementServiceTest.java index e12743a..ac8c09e 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/WebAuthnCredentialManagementServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/WebAuthnCredentialManagementServiceTest.java @@ -114,7 +114,7 @@ class RenameCredentialTests { @Test @DisplayName("should rename credential successfully") - void shouldRenameCredentialSuccessfully() throws WebAuthnException { + void shouldRenameCredentialSuccessfully() /* no checked exception */ { // Given when(credentialQueryRepository.renameCredential("cred-123", "Work Laptop", testUser.getId())).thenReturn(1); @@ -184,7 +184,7 @@ class DeleteCredentialTests { @Test @DisplayName("should delete credential when user has multiple passkeys") - void shouldDeleteWhenMultiplePasskeys() throws WebAuthnException { + void shouldDeleteWhenMultiplePasskeys() /* no checked exception */ { // Given when(credentialQueryRepository.lockAndCountCredentials(testUser.getId())).thenReturn(2L); when(credentialQueryRepository.deleteCredential("cred-123", testUser.getId())).thenReturn(1); @@ -198,7 +198,7 @@ void shouldDeleteWhenMultiplePasskeys() throws WebAuthnException { @Test @DisplayName("should delete last credential when user has a password") - void shouldDeleteLastCredentialWhenUserHasPassword() throws WebAuthnException { + void shouldDeleteLastCredentialWhenUserHasPassword() /* no checked exception */ { // Given - user has password set (from TestFixtures) when(credentialQueryRepository.lockAndCountCredentials(testUser.getId())).thenReturn(1L); when(credentialQueryRepository.deleteCredential("cred-123", testUser.getId())).thenReturn(1);