Skip to content

Commit 8d94dab

Browse files
devondragondevondragon
authored
Merge pull request #259 from devondragon/webauthn-fixes
fix(webauthn): address PR #258 review feedback
2 parents 750863e + dd5e557 commit 8d94dab

11 files changed

Lines changed: 246 additions & 57 deletions

‎src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListener.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ public void onUserPreDelete(UserPreDeleteEvent event) {
4242
}
4343

4444
private void deleteUserEntityAndCredentials(WebAuthnUserEntity userEntity) {
45-
credentialRepository.findByUserEntity(userEntity).forEach(credentialRepository::delete);
45+
credentialRepository.deleteByUserEntity(userEntity);
4646
userEntityRepository.delete(userEntity);
4747
log.info("Deleted WebAuthn data for user entity {}", userEntity.getName());
4848
}

‎src/main/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnCredentialQueryRepository.java‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
package com.digitalsanctuary.spring.user.persistence.repository;
22

3-
import java.util.Arrays;
4-
import java.util.Collections;
53
import java.util.List;
64
import java.util.Optional;
75
import java.util.stream.Collectors;
@@ -11,6 +9,7 @@
119
import org.springframework.transaction.annotation.Transactional;
1210
import com.digitalsanctuary.spring.user.dto.WebAuthnCredentialInfo;
1311
import com.digitalsanctuary.spring.user.persistence.model.WebAuthnCredential;
12+
import com.digitalsanctuary.spring.user.util.WebAuthnTransportUtils;
1413
import lombok.RequiredArgsConstructor;
1514
import lombok.extern.slf4j.Slf4j;
1615

@@ -59,6 +58,12 @@ public long countCredentials(Long userId) {
5958
/**
6059
* Lock all credentials for a user and return the count.
6160
*
61+
* <p>
62+
* <b>Note:</b> This method uses {@code Propagation.MANDATORY}, so callers must already be within
63+
* an active transaction. Calling this method without a surrounding transaction will throw
64+
* {@link org.springframework.transaction.IllegalTransactionStateException}.
65+
* </p>
66+
*
6267
* @param userId the user ID
6368
* @return count of locked credential rows
6469
*/
@@ -139,18 +144,10 @@ private Optional<WebAuthnCredential> findCredentialForUser(String credentialId,
139144
* @return the DTO
140145
*/
141146
private WebAuthnCredentialInfo toCredentialInfo(WebAuthnCredential entity) {
142-
List<String> transportList = parseTransportList(entity.getAuthenticatorTransports());
147+
List<String> transportList = WebAuthnTransportUtils.parseTransportStrings(entity.getAuthenticatorTransports());
143148
return WebAuthnCredentialInfo.builder().id(entity.getCredentialId()).label(entity.getLabel())
144149
.created(entity.getCreated()).lastUsed(entity.getLastUsed())
145150
.transports(transportList).backupEligible(entity.isBackupEligible())
146151
.backupState(entity.isBackupState()).build();
147152
}
148-
149-
private List<String> parseTransportList(String transports) {
150-
if (transports == null || transports.isEmpty()) {
151-
return Collections.emptyList();
152-
}
153-
return Arrays.stream(transports.split(",")).map(String::trim).filter(s -> !s.isEmpty())
154-
.collect(Collectors.toList());
155-
}
156153
}

‎src/main/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnCredentialRepository.java‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,12 @@
22

33
import java.util.List;
44
import java.util.Optional;
5-
import org.springframework.data.jpa.repository.Lock;
65
import org.springframework.data.jpa.repository.JpaRepository;
6+
import org.springframework.data.jpa.repository.Lock;
7+
import org.springframework.data.jpa.repository.Modifying;
78
import org.springframework.data.jpa.repository.Query;
89
import org.springframework.data.repository.query.Param;
10+
import org.springframework.transaction.annotation.Transactional;
911
import jakarta.persistence.LockModeType;
1012
import com.digitalsanctuary.spring.user.persistence.model.WebAuthnCredential;
1113
import com.digitalsanctuary.spring.user.persistence.model.WebAuthnUserEntity;
@@ -67,4 +69,14 @@ public interface WebAuthnCredentialRepository extends JpaRepository<WebAuthnCred
6769
*/
6870
@Query("SELECT c FROM WebAuthnCredential c JOIN FETCH c.userEntity ue JOIN FETCH ue.user WHERE c.credentialId = :credentialId")
6971
Optional<WebAuthnCredential> findByIdWithUser(@Param("credentialId") String credentialId);
72+
73+
/**
74+
* Delete all credentials for a WebAuthn user entity in a single bulk DELETE statement.
75+
*
76+
* @param userEntity the WebAuthn user entity whose credentials should be deleted
77+
*/
78+
@Transactional
79+
@Modifying
80+
@Query("DELETE FROM WebAuthnCredential c WHERE c.userEntity = :userEntity")
81+
void deleteByUserEntity(@Param("userEntity") WebAuthnUserEntity userEntity);
7082
}

‎src/main/java/com/digitalsanctuary/spring/user/persistence/repository/WebAuthnUserEntityBridge.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@
3636
@Slf4j
3737
public class WebAuthnUserEntityBridge implements PublicKeyCredentialUserEntityRepository {
3838

39+
private static final SecureRandom SECURE_RANDOM = new SecureRandom();
40+
3941
private final WebAuthnUserEntityRepository webAuthnUserEntityRepository;
4042
private final UserRepository userRepository;
4143

@@ -115,7 +117,7 @@ public void delete(Bytes id) {
115117
@Transactional
116118
public PublicKeyCredentialUserEntity createUserEntity(User user) {
117119
byte[] randomHandle = new byte[64];
118-
new SecureRandom().nextBytes(randomHandle);
120+
SECURE_RANDOM.nextBytes(randomHandle);
119121
Bytes userId = new Bytes(randomHandle);
120122
String idStr = Base64.getUrlEncoder().withoutPadding().encodeToString(userId.getBytes());
121123
String displayName = user.getFullName();

‎src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnRepositoryConfig.java‎

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
package com.digitalsanctuary.spring.user.security;
22

3-
import java.util.Arrays;
43
import java.util.Base64;
5-
import java.util.Collections;
64
import java.util.List;
75
import java.util.Set;
86
import java.util.stream.Collectors;
@@ -21,6 +19,7 @@
2119
import com.digitalsanctuary.spring.user.persistence.model.WebAuthnUserEntity;
2220
import com.digitalsanctuary.spring.user.persistence.repository.WebAuthnCredentialRepository;
2321
import com.digitalsanctuary.spring.user.persistence.repository.WebAuthnUserEntityRepository;
22+
import com.digitalsanctuary.spring.user.util.WebAuthnTransportUtils;
2423
import lombok.extern.slf4j.Slf4j;
2524

2625
/**
@@ -148,7 +147,7 @@ private CredentialRecord toCredentialRecord(WebAuthnCredential entity) {
148147
.signatureCount(entity.getSignatureCount()).uvInitialized(entity.isUvInitialized())
149148
.backupEligible(entity.isBackupEligible()).backupState(entity.isBackupState())
150149
.credentialType(credType)
151-
.transports(parseTransports(entity.getAuthenticatorTransports()))
150+
.transports(WebAuthnTransportUtils.parseTransports(entity.getAuthenticatorTransports()))
152151
.attestationObject(
153152
entity.getAttestationObject() != null ? new Bytes(entity.getAttestationObject()) : null)
154153
.attestationClientDataJSON(entity.getAttestationClientDataJson() != null
@@ -157,23 +156,6 @@ private CredentialRecord toCredentialRecord(WebAuthnCredential entity) {
157156
.created(entity.getCreated()).lastUsed(entity.getLastUsed()).label(entity.getLabel()).build();
158157
}
159158

160-
/**
161-
* Parse comma-separated transport string to a Set of AuthenticatorTransport.
162-
*/
163-
private Set<AuthenticatorTransport> parseTransports(String transports) {
164-
if (transports == null || transports.isEmpty()) {
165-
return Collections.emptySet();
166-
}
167-
return Arrays.stream(transports.split(",")).map(String::trim).filter(s -> !s.isEmpty()).map(value -> {
168-
try {
169-
return AuthenticatorTransport.valueOf(value);
170-
} catch (IllegalArgumentException e) {
171-
log.warn("Unknown AuthenticatorTransport '{}', skipping", value);
172-
return null;
173-
}
174-
}).filter(java.util.Objects::nonNull).collect(Collectors.toSet());
175-
}
176-
177159
/**
178160
* Convert a Set of AuthenticatorTransport to a comma-separated string.
179161
*/

‎src/main/java/com/digitalsanctuary/spring/user/service/WebAuthnCredentialManagementService.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public boolean hasCredentials(User user) {
7171
public void renameCredential(String credentialId, String newLabel, User user) {
7272
validateLabel(newLabel);
7373

74-
int updated = credentialQueryRepository.renameCredential(credentialId, newLabel, user.getId());
74+
int updated = credentialQueryRepository.renameCredential(credentialId, newLabel.trim(), user.getId());
7575

7676
if (updated == 0) {
7777
throw new WebAuthnException("Credential not found or access denied");

‎src/main/java/com/digitalsanctuary/spring/user/util/WebAuthnTransportUtils.java‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
package com.digitalsanctuary.spring.user.util;
2+
3+
import java.util.Arrays;
4+
import java.util.Collections;
5+
import java.util.List;
6+
import java.util.Objects;
7+
import java.util.Set;
8+
import java.util.stream.Collectors;
9+
import org.springframework.security.web.webauthn.api.AuthenticatorTransport;
10+
import lombok.extern.slf4j.Slf4j;
11+
12+
/**
13+
* Shared utility methods for parsing WebAuthn authenticator transport strings.
14+
*
15+
* <p>
16+
* Transport values are stored as comma-separated strings in the database (e.g. {@code "internal,hybrid"}).
17+
* This class provides two levels of parsing:
18+
* </p>
19+
* <ul>
20+
* <li>{@link #parseTransportStrings(String)} - split into trimmed, non-empty strings (for DTOs)</li>
21+
* <li>{@link #parseTransports(String)} - map to {@link AuthenticatorTransport} enum values (for Spring Security)</li>
22+
* </ul>
23+
*/
24+
@Slf4j
25+
public final class WebAuthnTransportUtils {
26+
27+
private WebAuthnTransportUtils() {
28+
throw new IllegalStateException("Utility class");
29+
}
30+
31+
/**
32+
* Parse a comma-separated transport string into a list of trimmed, non-empty strings.
33+
*
34+
* @param transports the comma-separated transport string, may be {@code null} or empty
35+
* @return an unmodifiable list of transport name strings, never {@code null}
36+
*/
37+
public static List<String> parseTransportStrings(String transports) {
38+
if (transports == null || transports.isEmpty()) {
39+
return Collections.emptyList();
40+
}
41+
return Arrays.stream(transports.split(",")).map(String::trim).filter(s -> !s.isEmpty())
42+
.collect(Collectors.toUnmodifiableList());
43+
}
44+
45+
/**
46+
* Parse a comma-separated transport string into a set of {@link AuthenticatorTransport} enum values.
47+
*
48+
* <p>
49+
* Unknown transport values are logged at WARN level and skipped.
50+
* </p>
51+
*
52+
* @param transports the comma-separated transport string, may be {@code null} or empty
53+
* @return an unmodifiable set of transport enum values, never {@code null}
54+
*/
55+
public static Set<AuthenticatorTransport> parseTransports(String transports) {
56+
if (transports == null || transports.isEmpty()) {
57+
return Collections.emptySet();
58+
}
59+
return parseTransportStrings(transports).stream().map(value -> {
60+
try {
61+
return AuthenticatorTransport.valueOf(value);
62+
} catch (IllegalArgumentException e) {
63+
log.warn("Unknown AuthenticatorTransport '{}', skipping", value);
64+
return null;
65+
}
66+
}).filter(Objects::nonNull).collect(Collectors.toUnmodifiableSet());
67+
}
68+
}

‎src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ class RenameCredentialTests {
154154

155155
@Test
156156
@DisplayName("should rename credential successfully")
157-
void shouldRenameSuccessfully() /* no checked exception */ {
157+
void shouldRenameSuccessfully() {
158158
// Given
159159
WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop");
160160

@@ -169,7 +169,7 @@ void shouldRenameSuccessfully() /* no checked exception */ {
169169

170170
@Test
171171
@DisplayName("should throw when rename fails")
172-
void shouldThrowOnFailure() /* no checked exception */ {
172+
void shouldThrowOnFailure() {
173173
// Given
174174
WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("New Name");
175175
doThrow(new WebAuthnException("Credential not found or access denied")).when(credentialManagementService)
@@ -182,7 +182,7 @@ void shouldThrowOnFailure() /* no checked exception */ {
182182

183183
@Test
184184
@DisplayName("should throw not found when user not found")
185-
void shouldThrowNotFoundWhenUserNotFound() /* no checked exception */ {
185+
void shouldThrowNotFoundWhenUserNotFound() {
186186
// Given
187187
WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("New Name");
188188
when(userService.findUserByEmail(testUser.getEmail())).thenReturn(null);
@@ -200,7 +200,7 @@ class DeleteCredentialTests {
200200

201201
@Test
202202
@DisplayName("should delete credential successfully")
203-
void shouldDeleteSuccessfully() /* no checked exception */ {
203+
void shouldDeleteSuccessfully() {
204204
// When
205205
ResponseEntity<GenericResponse> response = api.deleteCredential("cred-1", userDetails);
206206

@@ -212,7 +212,7 @@ void shouldDeleteSuccessfully() /* no checked exception */ {
212212

213213
@Test
214214
@DisplayName("should throw when delete fails")
215-
void shouldThrowOnFailure() /* no checked exception */ {
215+
void shouldThrowOnFailure() {
216216
// Given
217217
doThrow(new WebAuthnException("Cannot delete last passkey")).when(credentialManagementService).deleteCredential(eq("cred-1"),
218218
any(User.class));
@@ -224,7 +224,7 @@ void shouldThrowOnFailure() /* no checked exception */ {
224224

225225
@Test
226226
@DisplayName("should throw not found when user not found")
227-
void shouldThrowNotFoundWhenUserNotFound() /* no checked exception */ {
227+
void shouldThrowNotFoundWhenUserNotFound() {
228228
// Given
229229
when(userService.findUserByEmail(testUser.getEmail())).thenReturn(null);
230230

‎src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListenerTest.java‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
import static org.mockito.Mockito.never;
44
import static org.mockito.Mockito.verify;
55
import static org.mockito.Mockito.when;
6-
import java.util.List;
76
import java.util.Optional;
87
import org.junit.jupiter.api.BeforeEach;
98
import org.junit.jupiter.api.DisplayName;
@@ -13,7 +12,6 @@
1312
import org.mockito.Mock;
1413
import com.digitalsanctuary.spring.user.event.UserPreDeleteEvent;
1514
import com.digitalsanctuary.spring.user.persistence.model.User;
16-
import com.digitalsanctuary.spring.user.persistence.model.WebAuthnCredential;
1715
import com.digitalsanctuary.spring.user.persistence.model.WebAuthnUserEntity;
1816
import com.digitalsanctuary.spring.user.persistence.repository.WebAuthnCredentialRepository;
1917
import com.digitalsanctuary.spring.user.persistence.repository.WebAuthnUserEntityRepository;
@@ -53,25 +51,15 @@ void shouldDeleteWebAuthnDataForUser() {
5351
userEntity.setName(testUser.getEmail());
5452
userEntity.setUser(testUser);
5553

56-
WebAuthnCredential cred1 = new WebAuthnCredential();
57-
cred1.setCredentialId("cred-1");
58-
cred1.setUserEntity(userEntity);
59-
60-
WebAuthnCredential cred2 = new WebAuthnCredential();
61-
cred2.setCredentialId("cred-2");
62-
cred2.setUserEntity(userEntity);
63-
6454
when(userEntityRepository.findByUserId(testUser.getId())).thenReturn(Optional.of(userEntity));
65-
when(credentialRepository.findByUserEntity(userEntity)).thenReturn(List.of(cred1, cred2));
6655

6756
UserPreDeleteEvent event = new UserPreDeleteEvent(this, testUser);
6857

6958
// When
7059
listener.onUserPreDelete(event);
7160

7261
// Then
73-
verify(credentialRepository).delete(cred1);
74-
verify(credentialRepository).delete(cred2);
62+
verify(credentialRepository).deleteByUserEntity(userEntity);
7563
verify(userEntityRepository).delete(userEntity);
7664
}
7765

‎src/test/java/com/digitalsanctuary/spring/user/service/WebAuthnCredentialManagementServiceTest.java‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ class RenameCredentialTests {
114114

115115
@Test
116116
@DisplayName("should rename credential successfully")
117-
void shouldRenameCredentialSuccessfully() /* no checked exception */ {
117+
void shouldRenameCredentialSuccessfully() {
118118
// Given
119119
when(credentialQueryRepository.renameCredential("cred-123", "Work Laptop", testUser.getId())).thenReturn(1);
120120

@@ -125,6 +125,19 @@ void shouldRenameCredentialSuccessfully() /* no checked exception */ {
125125
verify(credentialQueryRepository).renameCredential("cred-123", "Work Laptop", testUser.getId());
126126
}
127127

128+
@Test
129+
@DisplayName("should trim whitespace from label before storing")
130+
void shouldTrimLabelBeforeStoring() {
131+
// Given
132+
when(credentialQueryRepository.renameCredential("cred-123", "Work Laptop", testUser.getId())).thenReturn(1);
133+
134+
// When
135+
service.renameCredential("cred-123", " Work Laptop ", testUser);
136+
137+
// Then — trimmed value reaches the repository, not the padded original
138+
verify(credentialQueryRepository).renameCredential("cred-123", "Work Laptop", testUser.getId());
139+
}
140+
128141
@Test
129142
@DisplayName("should throw when credential not found")
130143
void shouldThrowWhenCredentialNotFound() {
@@ -184,7 +197,7 @@ class DeleteCredentialTests {
184197

185198
@Test
186199
@DisplayName("should delete credential when user has multiple passkeys")
187-
void shouldDeleteWhenMultiplePasskeys() /* no checked exception */ {
200+
void shouldDeleteWhenMultiplePasskeys() {
188201
// Given
189202
when(credentialQueryRepository.lockAndCountCredentials(testUser.getId())).thenReturn(2L);
190203
when(credentialQueryRepository.deleteCredential("cred-123", testUser.getId())).thenReturn(1);
@@ -198,7 +211,7 @@ void shouldDeleteWhenMultiplePasskeys() /* no checked exception */ {
198211

199212
@Test
200213
@DisplayName("should delete last credential when user has a password")
201-
void shouldDeleteLastCredentialWhenUserHasPassword() /* no checked exception */ {
214+
void shouldDeleteLastCredentialWhenUserHasPassword() {
202215
// Given - user has password set (from TestFixtures)
203216
when(credentialQueryRepository.lockAndCountCredentials(testUser.getId())).thenReturn(1L);
204217
when(credentialQueryRepository.deleteCredential("cred-123", testUser.getId())).thenReturn(1);

0 commit comments

Comments
 (0)