Skip to content

Commit 5ab1815

Browse files
devondragonclaude
andcommitted
Fix code review issues and improve security
- Fix NPE in getUserByPasswordResetToken by adding null checks - Normalize emails to lowercase for consistent database lookups - Fix MailService to use consistent constructor injection - Add @ToString.Exclude to password fields to prevent logging sensitive data - Add password match validation with custom annotation @PasswordMatches - Update configuration metadata with correct types and missing properties - Add comprehensive tests for password validation These changes improve null safety, prevent sensitive data exposure in logs, ensure data consistency, and provide better IDE support through corrected configuration metadata. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent e851ead commit 5ab1815

9 files changed

Lines changed: 319 additions & 68 deletions

File tree

build.gradle

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ dependencies {
6969
testImplementation 'org.springframework.boot:spring-boot-starter-thymeleaf'
7070
testImplementation 'org.springframework.security:spring-security-test'
7171
testImplementation 'org.springframework.retry:spring-retry'
72+
testImplementation 'jakarta.validation:jakarta.validation-api:3.1.1'
7273
testImplementation 'com.h2database:h2:2.3.232'
7374

7475
// Runtime dependencies moved to test scope for library

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import jakarta.validation.constraints.NotBlank;
44
import jakarta.validation.constraints.Size;
55
import lombok.Data;
6+
import lombok.ToString;
67

78
/**
89
* A new password dto. This object is used for password form actions (change password, forgot password token, save
@@ -12,10 +13,12 @@
1213
public class PasswordDto {
1314

1415
/** The old password. */
16+
@ToString.Exclude
1517
@NotBlank(message = "Current password is required")
1618
private String oldPassword;
1719

1820
/** The new password. */
21+
@ToString.Exclude
1922
@NotBlank(message = "New password is required")
2023
@Size(min = 8, max = 128, message = "Password must be between 8 and 128 characters")
2124
private String newPassword;

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,18 @@
11
package com.digitalsanctuary.spring.user.dto;
22

3+
import com.digitalsanctuary.spring.user.validation.PasswordMatches;
34
import jakarta.validation.constraints.Email;
45
import jakarta.validation.constraints.NotBlank;
56
import jakarta.validation.constraints.Size;
67
import lombok.Data;
8+
import lombok.ToString;
79

810
/**
911
* A user dto. This object is used for handling user related form data (registration, forms passing in email addresses,
1012
* etc...).
1113
*/
1214
@Data
15+
@PasswordMatches
1316
public class UserDto {
1417

1518
/** The first name. */
@@ -23,11 +26,13 @@ public class UserDto {
2326
private String lastName;
2427

2528
/** The password. */
29+
@ToString.Exclude
2630
@NotBlank(message = "Password is required")
2731
@Size(min = 8, max = 128, message = "Password must be between 8 and 128 characters")
2832
private String password;
2933

3034
/** The matching password. */
35+
@ToString.Exclude
3136
@NotBlank(message = "Password confirmation is required")
3237
private String matchingPassword;
3338

src/main/java/com/digitalsanctuary/spring/user/mail/MailService.java

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

33
import java.util.Map;
4-
import org.springframework.beans.factory.annotation.Autowired;
54
import org.springframework.beans.factory.annotation.Value;
65
import org.springframework.mail.MailException;
76
import org.springframework.mail.javamail.JavaMailSender;
@@ -24,23 +23,24 @@
2423
public class MailService {
2524

2625
/** The mail sender. */
27-
private JavaMailSender mailSender;
26+
private final JavaMailSender mailSender;
27+
28+
/** The mail content builder. */
29+
private final MailContentBuilder mailContentBuilder;
2830

2931
/** The from address. */
3032
@Value("${user.mail.fromAddress}")
3133
private String fromAddress;
3234

33-
/** The mail content builder. */
34-
@Autowired
35-
MailContentBuilder mailContentBuilder;
36-
3735
/**
3836
* Instantiates a new mail service.
3937
*
4038
* @param mailSender the mail sender
39+
* @param mailContentBuilder the mail content builder
4140
*/
42-
public MailService(JavaMailSender mailSender) {
41+
public MailService(JavaMailSender mailSender, MailContentBuilder mailContentBuilder) {
4342
this.mailSender = mailSender;
43+
this.mailContentBuilder = mailContentBuilder;
4444
}
4545

4646
/**

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

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ public User registerNewUserAccount(final UserDto newUserDto) {
223223
user.setFirstName(newUserDto.getFirstName());
224224
user.setLastName(newUserDto.getLastName());
225225
user.setPassword(passwordEncoder.encode(newUserDto.getPassword()));
226-
user.setEmail(newUserDto.getEmail());
226+
user.setEmail(newUserDto.getEmail().toLowerCase());
227227
user.setRoles(Arrays.asList(roleRepository.findByName(USER_ROLE_NAME)));
228228

229229
// If we are not sending a verification email
@@ -295,7 +295,10 @@ public void deleteOrDisableUser(final User user) {
295295
* @return the user
296296
*/
297297
public User findUserByEmail(final String email) {
298-
return userRepository.findByEmail(email);
298+
if (email == null) {
299+
return null;
300+
}
301+
return userRepository.findByEmail(email.toLowerCase());
299302
}
300303

301304
/**
@@ -315,7 +318,14 @@ public PasswordResetToken getPasswordResetToken(final String token) {
315318
* @return the user by password reset token
316319
*/
317320
public Optional<User> getUserByPasswordResetToken(final String token) {
318-
return Optional.ofNullable(passwordTokenRepository.findByToken(token).getUser());
321+
if (token == null) {
322+
return Optional.empty();
323+
}
324+
PasswordResetToken passwordResetToken = passwordTokenRepository.findByToken(token);
325+
if (passwordResetToken == null) {
326+
return Optional.empty();
327+
}
328+
return Optional.ofNullable(passwordResetToken.getUser());
319329
}
320330

321331
/**
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
package com.digitalsanctuary.spring.user.validation;
2+
3+
import java.lang.annotation.Documented;
4+
import java.lang.annotation.ElementType;
5+
import java.lang.annotation.Retention;
6+
import java.lang.annotation.RetentionPolicy;
7+
import java.lang.annotation.Target;
8+
import jakarta.validation.Constraint;
9+
import jakarta.validation.Payload;
10+
11+
/**
12+
* Validation annotation to verify that the password and matchingPassword fields match.
13+
* This is a class-level constraint that should be applied to DTOs containing password confirmation fields.
14+
*/
15+
@Target({ElementType.TYPE, ElementType.ANNOTATION_TYPE})
16+
@Retention(RetentionPolicy.RUNTIME)
17+
@Constraint(validatedBy = PasswordMatchesValidator.class)
18+
@Documented
19+
public @interface PasswordMatches {
20+
21+
/**
22+
* The error message to display when passwords do not match.
23+
*
24+
* @return the error message
25+
*/
26+
String message() default "Passwords do not match";
27+
28+
/**
29+
* Validation groups.
30+
*
31+
* @return the groups
32+
*/
33+
Class<?>[] groups() default {};
34+
35+
/**
36+
* Additional payload data.
37+
*
38+
* @return the payload
39+
*/
40+
Class<? extends Payload>[] payload() default {};
41+
}
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
package com.digitalsanctuary.spring.user.validation;
2+
3+
import com.digitalsanctuary.spring.user.dto.UserDto;
4+
import jakarta.validation.ConstraintValidator;
5+
import jakarta.validation.ConstraintValidatorContext;
6+
7+
/**
8+
* Validator implementation for the PasswordMatches constraint.
9+
* Validates that the password and matchingPassword fields in a UserDto are equal.
10+
*/
11+
public class PasswordMatchesValidator implements ConstraintValidator<PasswordMatches, Object> {
12+
13+
/**
14+
* Initializes the validator.
15+
*
16+
* @param constraintAnnotation the annotation instance
17+
*/
18+
@Override
19+
public void initialize(PasswordMatches constraintAnnotation) {
20+
// No initialization needed
21+
}
22+
23+
/**
24+
* Validates that the password and matchingPassword fields match.
25+
*
26+
* @param obj the object to validate
27+
* @param context the constraint validator context
28+
* @return true if the passwords match or if either field is null (to allow other validators to handle null checks),
29+
* false otherwise
30+
*/
31+
@Override
32+
public boolean isValid(Object obj, ConstraintValidatorContext context) {
33+
if (!(obj instanceof UserDto)) {
34+
// If not a UserDto, consider it valid (this validator doesn't apply)
35+
return true;
36+
}
37+
38+
UserDto user = (UserDto) obj;
39+
40+
// If either password is null, let the @NotBlank validators handle it
41+
if (user.getPassword() == null || user.getMatchingPassword() == null) {
42+
return true;
43+
}
44+
45+
return user.getPassword().equals(user.getMatchingPassword());
46+
}
47+
}

0 commit comments

Comments
 (0)