Skip to content

Commit a87922e

Browse files
devondragonclaude
andcommitted
refactor: improve PasswordPolicyService structure and error handling
- Extract password validation logic into separate private methods for better maintainability - Add checkPasswordHistory() method for cleaner password history validation - Add checkPasswordSimilarity() method for username/email similarity checks - Add validateWithPassay() method to encapsulate Passay validation logic - Add buildPassayRules() and createSpecialCharacterRule() helper methods - Implement early returns for history and similarity checks to improve performance - Add Optional return types for null-safe error handling - Improve error logging with critical severity for missing common passwords file - Add debug logging for password rejection reasons These changes improve code readability, testability, and follow single responsibility principle by breaking down the large validate() method into focused helper methods. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 64e9d2c commit a87922e

1 file changed

Lines changed: 119 additions & 39 deletions

File tree

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

Lines changed: 119 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import java.util.ArrayList;
3636
import java.util.List;
3737
import java.util.Locale;
38+
import java.util.Optional;
3839

3940
/**
4041
* The PasswordPolicyService enforces configurable password validation rules
@@ -104,7 +105,12 @@ private void initCommonPasswords() {
104105
commonPasswordRule = new DictionaryRule(dictionary);
105106
log.info("Common password dictionary initialized successfully.");
106107
} catch (Exception e) {
107-
log.error("Failed to load common passwords file", e);
108+
// Log the error as severe since this is a security feature
109+
log.error("CRITICAL: Failed to load common passwords file. " +
110+
"Common password checking is DISABLED. This is a security risk!", e);
111+
// In production, you may want to fail fast, but for now we'll allow
112+
// the application to start with reduced security to avoid breaking tests
113+
// Consider using a profile-based approach for stricter production behavior
108114
}
109115
}
110116
}
@@ -125,6 +131,30 @@ public List<String> validate(User user, String password, String usernameOrEmail,
125131
}
126132

127133
log.debug("Validating password with configured policy.");
134+
135+
// Check password history first (early return if reused)
136+
Optional<String> historyError = checkPasswordHistory(user, password, locale);
137+
if (historyError.isPresent()) {
138+
return List.of(historyError.get());
139+
}
140+
141+
// Check similarity to username/email (early return if too similar)
142+
Optional<String> similarityError = checkPasswordSimilarity(password, usernameOrEmail, locale);
143+
if (similarityError.isPresent()) {
144+
return List.of(similarityError.get());
145+
}
146+
147+
// Build and apply Passay rules
148+
List<Rule> rules = buildPassayRules();
149+
return validateWithPassay(password, rules, locale);
150+
}
151+
152+
/**
153+
* Build the list of Passay rules based on configuration.
154+
*
155+
* @return list of Passay rules
156+
*/
157+
private List<Rule> buildPassayRules() {
128158
List<Rule> rules = new ArrayList<>();
129159

130160
// Length rule
@@ -141,57 +171,107 @@ public List<String> validate(User user, String password, String usernameOrEmail,
141171
rules.add(new CharacterRule(EnglishCharacterData.Digit, 1));
142172
}
143173
if (requireSpecial) {
144-
CharacterData specialCharacterData = new CharacterData() {
145-
@Override
146-
public String getErrorCode() {
147-
return EnglishCharacterData.Special.getErrorCode();
148-
149-
}
150-
151-
@Override
152-
public String getCharacters() {
153-
return specialChars;
154-
}
155-
};
156-
rules.add(new CharacterRule(specialCharacterData, 1));
174+
rules.add(createSpecialCharacterRule());
157175
}
158176

159177
// Common Passwords Dictionary Rule
160178
if (preventCommonPasswords && commonPasswordRule != null) {
161179
rules.add(commonPasswordRule);
162180
}
163181

164-
// Password History Check
165-
if (user != null && historyCount > 0) {
166-
List<String> oldHashes = passwordHistoryRepository.findRecentPasswordHashes(user,
167-
PageRequest.of(0, historyCount));
168-
for (String hash : oldHashes) {
169-
if (passwordEncoder.matches(password, hash)) {
170-
String msg = messages.getMessage("password.error.history.reuse", new Object[] { historyCount },
171-
locale);
172-
return List.of(msg);
173-
}
182+
return rules;
183+
}
184+
185+
/**
186+
* Create a special character rule with configured allowed characters.
187+
*
188+
* @return CharacterRule for special characters
189+
*/
190+
private CharacterRule createSpecialCharacterRule() {
191+
CharacterData specialCharacterData = new CharacterData() {
192+
@Override
193+
public String getErrorCode() {
194+
return EnglishCharacterData.Special.getErrorCode();
195+
}
196+
197+
@Override
198+
public String getCharacters() {
199+
return specialChars;
174200
}
201+
};
202+
return new CharacterRule(specialCharacterData, 1);
203+
}
204+
205+
/**
206+
* Check if the password has been used before by this user.
207+
*
208+
* @param user the user
209+
* @param password the password to check
210+
* @param locale the locale for error messages
211+
* @return Optional containing error message if password was reused, empty otherwise
212+
*/
213+
private Optional<String> checkPasswordHistory(User user, String password, Locale locale) {
214+
if (user == null || historyCount <= 0) {
215+
return Optional.empty();
175216
}
176217

177-
// Similarity Check
178-
if (StringUtils.hasText(usernameOrEmail) && similarityThreshold > 0) {
179-
int distance = LevenshteinDistance.getDefaultInstance().apply(password.toLowerCase(),
180-
usernameOrEmail.toLowerCase());
181-
int maxLength = Math.max(password.length(), usernameOrEmail.length());
182-
183-
if (maxLength > 0) {
184-
double similarityPercent = (100.0 * (maxLength - distance)) / maxLength;
185-
log.debug("Password similarity to username/email: {}%", similarityPercent);
186-
187-
if (similarityPercent >= similarityThreshold) {
188-
String msg = messages.getMessage("password.error.similarity",
189-
new Object[] { String.format("%.2f", similarityPercent) }, locale);
190-
return List.of(msg);
191-
}
218+
List<String> oldHashes = passwordHistoryRepository.findRecentPasswordHashes(user,
219+
PageRequest.of(0, historyCount));
220+
221+
for (String hash : oldHashes) {
222+
if (passwordEncoder.matches(password, hash)) {
223+
String msg = messages.getMessage("password.error.history.reuse",
224+
new Object[] { historyCount }, locale);
225+
log.debug("Password rejected: matches historical password");
226+
return Optional.of(msg);
192227
}
193228
}
194229

230+
return Optional.empty();
231+
}
232+
233+
/**
234+
* Check if the password is too similar to the username or email.
235+
*
236+
* @param password the password to check
237+
* @param usernameOrEmail the username or email to compare against
238+
* @param locale the locale for error messages
239+
* @return Optional containing error message if too similar, empty otherwise
240+
*/
241+
private Optional<String> checkPasswordSimilarity(String password, String usernameOrEmail, Locale locale) {
242+
if (!StringUtils.hasText(usernameOrEmail) || similarityThreshold <= 0) {
243+
return Optional.empty();
244+
}
245+
246+
int distance = LevenshteinDistance.getDefaultInstance().apply(
247+
password.toLowerCase(), usernameOrEmail.toLowerCase());
248+
int maxLength = Math.max(password.length(), usernameOrEmail.length());
249+
250+
if (maxLength == 0) {
251+
return Optional.empty();
252+
}
253+
254+
double similarityPercent = (100.0 * (maxLength - distance)) / maxLength;
255+
log.debug("Password similarity to username/email: {}%", similarityPercent);
256+
257+
if (similarityPercent >= similarityThreshold) {
258+
String msg = messages.getMessage("password.error.similarity",
259+
new Object[] { String.format("%.2f", similarityPercent) }, locale);
260+
return Optional.of(msg);
261+
}
262+
263+
return Optional.empty();
264+
}
265+
266+
/**
267+
* Validate password using Passay rules.
268+
*
269+
* @param password the password to validate
270+
* @param rules the Passay rules to apply
271+
* @param locale the locale for error messages
272+
* @return list of error messages if validation fails, empty if valid
273+
*/
274+
private List<String> validateWithPassay(String password, List<Rule> rules, Locale locale) {
195275
PasswordValidator validator = new PasswordValidator(
196276
(detail) -> messages.getMessage(detail.getErrorCode(), detail.getValues(), locale),
197277
rules);

0 commit comments

Comments
 (0)