Skip to content

Commit dd5e557

Browse files
devondragondevondragon
committed
fix(webauthn): address PR #259 review feedback
- Use @Modifying @query for true single-statement bulk DELETE in deleteByUserEntity(), replacing the Spring Data derived method that issued N+1 SQL under the hood - Add throw new IllegalStateException("Utility class") to WebAuthnTransportUtils constructor (consistent with UserUtils) - Use Collectors.toUnmodifiableList/Set() in WebAuthnTransportUtils to match declared Javadoc contract - Add WebAuthnTransportUtilsTest covering null/empty inputs, whitespace trimming, multiple values, and unmodifiability - Add shouldTrimLabelBeforeStoring test to verify the renameCredential trim fix actually reaches the repository layer
1 parent 5044a84 commit dd5e557

4 files changed

Lines changed: 151 additions & 6 deletions

File tree

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

Lines changed: 8 additions & 3 deletions
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;
@@ -69,9 +71,12 @@ public interface WebAuthnCredentialRepository extends JpaRepository<WebAuthnCred
6971
Optional<WebAuthnCredential> findByIdWithUser(@Param("credentialId") String credentialId);
7072

7173
/**
72-
* Delete all credentials for a WebAuthn user entity in a single batch operation.
74+
* Delete all credentials for a WebAuthn user entity in a single bulk DELETE statement.
7375
*
7476
* @param userEntity the WebAuthn user entity whose credentials should be deleted
7577
*/
76-
void deleteByUserEntity(WebAuthnUserEntity userEntity);
78+
@Transactional
79+
@Modifying
80+
@Query("DELETE FROM WebAuthnCredential c WHERE c.userEntity = :userEntity")
81+
void deleteByUserEntity(@Param("userEntity") WebAuthnUserEntity userEntity);
7782
}

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
public final class WebAuthnTransportUtils {
2626

2727
private WebAuthnTransportUtils() {
28-
// utility class
28+
throw new IllegalStateException("Utility class");
2929
}
3030

3131
/**
@@ -39,7 +39,7 @@ public static List<String> parseTransportStrings(String transports) {
3939
return Collections.emptyList();
4040
}
4141
return Arrays.stream(transports.split(",")).map(String::trim).filter(s -> !s.isEmpty())
42-
.collect(Collectors.toList());
42+
.collect(Collectors.toUnmodifiableList());
4343
}
4444

4545
/**
@@ -63,6 +63,6 @@ public static Set<AuthenticatorTransport> parseTransports(String transports) {
6363
log.warn("Unknown AuthenticatorTransport '{}', skipping", value);
6464
return null;
6565
}
66-
}).filter(Objects::nonNull).collect(Collectors.toSet());
66+
}).filter(Objects::nonNull).collect(Collectors.toUnmodifiableSet());
6767
}
6868
}

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,19 @@ void shouldRenameCredentialSuccessfully() {
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() {

‎src/test/java/com/digitalsanctuary/spring/user/util/WebAuthnTransportUtilsTest.java‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
package com.digitalsanctuary.spring.user.util;
2+
3+
import static org.assertj.core.api.Assertions.assertThat;
4+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
5+
import java.lang.reflect.Constructor;
6+
import java.lang.reflect.InvocationTargetException;
7+
import java.util.List;
8+
import java.util.Set;
9+
import org.junit.jupiter.api.DisplayName;
10+
import org.junit.jupiter.api.Nested;
11+
import org.junit.jupiter.api.Test;
12+
import org.junit.jupiter.params.ParameterizedTest;
13+
import org.junit.jupiter.params.provider.NullAndEmptySource;
14+
import org.springframework.security.web.webauthn.api.AuthenticatorTransport;
15+
16+
@DisplayName("WebAuthnTransportUtils Tests")
17+
class WebAuthnTransportUtilsTest {
18+
19+
@Nested
20+
@DisplayName("Utility Class Construction")
21+
class ConstructionTests {
22+
23+
@Test
24+
@DisplayName("should throw IllegalStateException when instantiated via reflection")
25+
void shouldThrowOnInstantiation() throws Exception {
26+
Constructor<WebAuthnTransportUtils> constructor = WebAuthnTransportUtils.class.getDeclaredConstructor();
27+
constructor.setAccessible(true);
28+
assertThatThrownBy(() -> {
29+
try {
30+
constructor.newInstance();
31+
} catch (InvocationTargetException e) {
32+
throw e.getCause();
33+
}
34+
}).isInstanceOf(IllegalStateException.class).hasMessage("Utility class");
35+
}
36+
}
37+
38+
@Nested
39+
@DisplayName("parseTransportStrings")
40+
class ParseTransportStringsTests {
41+
42+
@ParameterizedTest
43+
@NullAndEmptySource
44+
@DisplayName("should return empty list for null or empty input")
45+
void shouldReturnEmptyForNullOrEmpty(String input) {
46+
assertThat(WebAuthnTransportUtils.parseTransportStrings(input)).isEmpty();
47+
}
48+
49+
@Test
50+
@DisplayName("should parse single transport")
51+
void shouldParseSingleTransport() {
52+
assertThat(WebAuthnTransportUtils.parseTransportStrings("usb")).containsExactly("usb");
53+
}
54+
55+
@Test
56+
@DisplayName("should parse multiple transports")
57+
void shouldParseMultipleTransports() {
58+
List<String> result = WebAuthnTransportUtils.parseTransportStrings("usb,nfc,ble");
59+
assertThat(result).containsExactlyInAnyOrder("usb", "nfc", "ble");
60+
}
61+
62+
@Test
63+
@DisplayName("should trim whitespace from each transport")
64+
void shouldTrimWhitespace() {
65+
List<String> result = WebAuthnTransportUtils.parseTransportStrings(" usb , nfc ");
66+
assertThat(result).containsExactlyInAnyOrder("usb", "nfc");
67+
}
68+
69+
@Test
70+
@DisplayName("should filter empty entries from leading/trailing commas")
71+
void shouldFilterEmptyEntries() {
72+
assertThat(WebAuthnTransportUtils.parseTransportStrings(",usb,")).containsExactly("usb");
73+
}
74+
75+
@Test
76+
@DisplayName("should return unmodifiable list")
77+
void shouldReturnUnmodifiableList() {
78+
List<String> result = WebAuthnTransportUtils.parseTransportStrings("usb");
79+
assertThatThrownBy(() -> result.add("nfc")).isInstanceOf(UnsupportedOperationException.class);
80+
}
81+
}
82+
83+
@Nested
84+
@DisplayName("parseTransports")
85+
class ParseTransportsTests {
86+
87+
@ParameterizedTest
88+
@NullAndEmptySource
89+
@DisplayName("should return empty set for null or empty input")
90+
void shouldReturnEmptyForNullOrEmpty(String input) {
91+
assertThat(WebAuthnTransportUtils.parseTransports(input)).isEmpty();
92+
}
93+
94+
@Test
95+
@DisplayName("should parse single transport value")
96+
void shouldParseSingleTransport() {
97+
String value = AuthenticatorTransport.USB.getValue();
98+
Set<AuthenticatorTransport> result = WebAuthnTransportUtils.parseTransports(value);
99+
assertThat(result).containsExactly(AuthenticatorTransport.USB);
100+
}
101+
102+
@Test
103+
@DisplayName("should parse multiple transport values")
104+
void shouldParseMultipleTransports() {
105+
String input = AuthenticatorTransport.USB.getValue() + "," + AuthenticatorTransport.NFC.getValue();
106+
Set<AuthenticatorTransport> result = WebAuthnTransportUtils.parseTransports(input);
107+
assertThat(result).containsExactlyInAnyOrder(AuthenticatorTransport.USB, AuthenticatorTransport.NFC);
108+
}
109+
110+
@Test
111+
@DisplayName("should trim whitespace from each transport value")
112+
void shouldTrimWhitespace() {
113+
String input = " " + AuthenticatorTransport.USB.getValue() + " ";
114+
Set<AuthenticatorTransport> result = WebAuthnTransportUtils.parseTransports(input);
115+
assertThat(result).containsExactly(AuthenticatorTransport.USB);
116+
}
117+
118+
@Test
119+
@DisplayName("should return unmodifiable set")
120+
void shouldReturnUnmodifiableSet() {
121+
String validValue = AuthenticatorTransport.USB.getValue();
122+
Set<AuthenticatorTransport> result = WebAuthnTransportUtils.parseTransports(validValue);
123+
assertThatThrownBy(() -> result.add(AuthenticatorTransport.NFC))
124+
.isInstanceOf(UnsupportedOperationException.class);
125+
}
126+
}
127+
}

0 commit comments

Comments
 (0)