Skip to content

Commit e0a3fe7

Browse files
committed
fix: address PR review feedback for passwordless account support
- Guard passwordless registration when WebAuthn is unavailable (L1) - Invalidate sessions on password removal for security (L2) - Reject null password in registerNewUserAccount (L3) - Replace @Autowired field with ObjectProvider constructor injection (L4) - Remove unused role field from PasswordlessRegistrationDto (L5) - Change passkeysCount from int to long (L6) - Add webAuthnEnabled to AuthMethodsResponse (L7) - Verify savePasswordHistory in setInitialPassword test (L8)
1 parent bf8211b commit e0a3fe7

5 files changed

Lines changed: 29 additions & 13 deletions

File tree

src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
import java.util.Locale;
55
import java.util.Optional;
66
import jakarta.validation.Valid;
7-
import org.springframework.beans.factory.annotation.Autowired;
7+
import org.springframework.beans.factory.ObjectProvider;
88
import org.springframework.beans.factory.annotation.Value;
99
import org.springframework.context.ApplicationEventPublisher;
1010
import org.springframework.context.MessageSource;
@@ -66,9 +66,7 @@ public class UserAPI {
6666
private final MessageSource messages;
6767
private final ApplicationEventPublisher eventPublisher;
6868
private final PasswordPolicyService passwordPolicyService;
69-
70-
@Autowired(required = false)
71-
private WebAuthnCredentialManagementService webAuthnCredentialManagementService;
69+
private final ObjectProvider<WebAuthnCredentialManagementService> webAuthnCredentialManagementServiceProvider;
7270

7371
@Value("${user.security.registrationPendingURI}")
7472
private String registrationPendingURI;
@@ -360,18 +358,19 @@ public ResponseEntity<JSONResponse> getAuthMethods(@AuthenticationPrincipal DSUs
360358
return buildErrorResponse("User not found", 1, HttpStatus.BAD_REQUEST);
361359
}
362360

361+
WebAuthnCredentialManagementService webAuthnService = webAuthnCredentialManagementServiceProvider.getIfAvailable();
363362
boolean hasPasskeys = false;
364-
int passkeysCount = 0;
365-
if (webAuthnCredentialManagementService != null) {
366-
long count = webAuthnCredentialManagementService.getCredentialCount(user);
367-
hasPasskeys = count > 0;
368-
passkeysCount = (int) count;
363+
long passkeysCount = 0;
364+
if (webAuthnService != null) {
365+
passkeysCount = webAuthnService.getCredentialCount(user);
366+
hasPasskeys = passkeysCount > 0;
369367
}
370368

371369
AuthMethodsResponse authMethods = AuthMethodsResponse.builder()
372370
.hasPassword(userService.hasPassword(user))
373371
.hasPasskeys(hasPasskeys)
374372
.passkeysCount(passkeysCount)
373+
.webAuthnEnabled(webAuthnService != null)
375374
.provider(user.getProvider())
376375
.build();
377376

@@ -388,6 +387,9 @@ public ResponseEntity<JSONResponse> getAuthMethods(@AuthenticationPrincipal DSUs
388387
@PostMapping("/registration/passwordless")
389388
public ResponseEntity<JSONResponse> registerPasswordlessAccount(@Valid @RequestBody PasswordlessRegistrationDto dto,
390389
HttpServletRequest request) {
390+
if (webAuthnCredentialManagementServiceProvider.getIfAvailable() == null) {
391+
return buildErrorResponse("Passwordless registration is not available", 1, HttpStatus.BAD_REQUEST);
392+
}
391393
try {
392394
User registeredUser = userService.registerPasswordlessAccount(dto);
393395
publishRegistrationEvent(registeredUser, request);

src/main/java/com/digitalsanctuary/spring/user/dto/AuthMethodsResponse.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,10 @@ public class AuthMethodsResponse {
2424
private boolean hasPasskeys;
2525

2626
/** The number of passkeys registered. */
27-
private int passkeysCount;
27+
private long passkeysCount;
28+
29+
/** Whether WebAuthn is enabled on the server. */
30+
private boolean webAuthnEnabled;
2831

2932
/** The user's authentication provider. */
3033
private User.Provider provider;

src/main/java/com/digitalsanctuary/spring/user/dto/PasswordlessRegistrationDto.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,4 @@ public class PasswordlessRegistrationDto {
3333
@Size(max = 100, message = "Email must not exceed 100 characters")
3434
private String email;
3535

36-
/** The role. */
37-
private Integer role;
3836
}

src/main/java/com/digitalsanctuary/spring/user/service/UserService.java

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,8 @@ public String getValue() {
224224

225225
private final PasswordHistoryRepository passwordHistoryRepository;
226226

227+
private final SessionInvalidationService sessionInvalidationService;
228+
227229
/** The send registration verification email flag. */
228230
@Value("${user.registration.sendVerificationEmail:false}")
229231
private boolean sendRegistrationVerificationEmail;
@@ -249,8 +251,13 @@ public User registerNewUserAccount(final UserDto newUserDto) {
249251
TimeLogger timeLogger = new TimeLogger(log, "UserService.registerNewUserAccount");
250252
log.debug("UserService.registerNewUserAccount: called with userDto: {}", newUserDto);
251253

254+
if (newUserDto.getPassword() == null) {
255+
throw new IllegalArgumentException(
256+
"Password is required for standard registration. Use registerPasswordlessAccount() for passwordless accounts.");
257+
}
258+
252259
// Validate password match only if both are provided
253-
if (newUserDto.getPassword() != null && newUserDto.getMatchingPassword() != null
260+
if (newUserDto.getMatchingPassword() != null
254261
&& !newUserDto.getPassword().equals(newUserDto.getMatchingPassword())) {
255262
throw new IllegalArgumentException("Passwords do not match");
256263
}
@@ -495,6 +502,7 @@ public void removeUserPassword(User user) {
495502
user.setPassword(null);
496503
userRepository.save(user);
497504
passwordHistoryRepository.deleteByUser(user);
505+
sessionInvalidationService.invalidateUserSessions(user);
498506
log.info("Password removed for user: {}", user.getEmail());
499507
}
500508

src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import com.digitalsanctuary.spring.user.dto.UserDto;
4343
import com.digitalsanctuary.spring.user.event.UserPreDeleteEvent;
4444
import com.digitalsanctuary.spring.user.exceptions.UserAlreadyExistException;
45+
import com.digitalsanctuary.spring.user.persistence.model.PasswordHistoryEntry;
4546
import com.digitalsanctuary.spring.user.persistence.model.PasswordResetToken;
4647
import com.digitalsanctuary.spring.user.persistence.model.Role;
4748
import com.digitalsanctuary.spring.user.persistence.model.User;
@@ -87,6 +88,8 @@ public class UserServiceTest {
8788
private AuthorityService authorityService;
8889
@Mock
8990
private PasswordHistoryRepository passwordHistoryRepository;
91+
@Mock
92+
private SessionInvalidationService sessionInvalidationService;
9093
@InjectMocks
9194
private UserService userService;
9295
private User testUser;
@@ -691,6 +694,7 @@ void shouldRemovePasswordAndClearHistory() {
691694
assertThat(testUser.getPassword()).isNull();
692695
verify(userRepository).save(testUser);
693696
verify(passwordHistoryRepository).deleteByUser(testUser);
697+
verify(sessionInvalidationService).invalidateUserSessions(testUser);
694698
}
695699
}
696700

@@ -715,6 +719,7 @@ void shouldSetInitialPasswordWhenNoPassword() {
715719
assertThat(testUser.getPassword()).isEqualTo(encodedPassword);
716720
verify(passwordEncoder).encode(rawPassword);
717721
verify(userRepository).save(testUser);
722+
verify(passwordHistoryRepository).save(any(PasswordHistoryEntry.class));
718723
}
719724

720725
@Test

0 commit comments

Comments
 (0)