Add Password Policy Functionality #217

Edamijueda · 2025-09-22T20:49:17Z

PR for issue #158

Implemented PasswordPolicyService and add it to userRegistration flow.

Edamijueda · 2025-09-22T20:50:44Z

@devondragon review is needed for this PR. Thank you.

Copilot

Pull Request Overview

This PR adds comprehensive password policy functionality to the Spring User Framework, implementing configurable password validation rules including length, character requirements, similarity checks, dictionary-based rejection, and password history tracking.

Implemented PasswordPolicyService with configurable validation rules using Passay library
Added password history tracking to prevent password reuse
Integrated password policy validation into user registration flow

Reviewed Changes

Copilot reviewed 10 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
PasswordPolicyService.java	Core service implementing password validation with configurable rules
PasswordHistoryEntry.java	Entity for tracking password history
PasswordHistoryRepository.java	Repository for password history operations
UserService.java	Added password history tracking integration
UserAPI.java	Integrated password policy validation in registration endpoint
dsspringuserconfig.properties	Added password policy configuration properties
dsspringusermessages.properties	Added password validation error messages
additional-spring-configuration-metadata.json	Added metadata for password policy properties
UserServiceTest.java	Updated test setup with password history repository
PasswordPolicyServiceTest.java	Comprehensive test suite for password policy service

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

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

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

src/main/resources/META-INF/additional-spring-configuration-metadata.json

Edamijueda · 2025-09-22T22:01:45Z

@devondragon I have made a new commit that apply all request changes.

Copilot

Pull Request Overview

Copilot reviewed 10 out of 12 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

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

The changeUserPassword method lacks integration with the new password policy validation and password history saving. This method should validate the new password and save it to history.

	public void changeUserPassword(final User user, final String password) {

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-22T23:55:30Z

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

+    @Value("classpath:common_passwords.txt")
+    private Resource commonPasswordsResource;


The hardcoded classpath reference to 'common_passwords.txt' may cause runtime failures if the file doesn't exist. This should be made configurable or the initialization should handle missing files gracefully.

Copilot · 2025-09-22T23:55:31Z

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

+        // when(messages.getMessage(anyString(), any(), any(Locale.class)))
+        // .thenAnswer(inv -> inv.getArgument(0, String.class));


Remove commented-out code to improve code cleanliness.

Suggested change

// when(messages.getMessage(anyString(), any(), any(Locale.class)))

// .thenAnswer(inv -> inv.getArgument(0, String.class));

- 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 <[email protected]>

- Add historyCount field with @value annotation for configuration - Implement cleanUpPasswordHistory() method to remove old password entries - Automatically clean up entries beyond configured history count after saving - Add password history tracking to changeUserPassword() method - Prevent database bloat by limiting stored password history entries This ensures password history doesn't grow unbounded and maintains only the configured number of recent passwords for reuse checking. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

- Remove unnecessary quotes around special-chars property value - Fix escaping for backslash and quotes in allowed special characters - Ensures proper parsing of special characters configuration The previous format with surrounding quotes could cause parsing issues and prevent certain special characters from being recognized. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

- Correct validate() method call to include both password and username parameters - Previously passing null for username prevented similarity check from executing - Now properly tests password-to-username similarity validation This ensures the similarity check test actually validates the intended behavior. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

- Add @column annotations with proper constraints for passwordHash and entryDate - Set passwordHash max length to 255 characters (sufficient for bcrypt hashes) - Mark both passwordHash and entryDate as non-nullable fields - Add database indexes on user_id and entry_date for query performance - Improve query performance for findByUserOrderByEntryDateDesc operations These changes ensure data integrity at the database level and optimize password history lookups which are performed on every password change. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

- Add mock for PasswordPolicyService dependency - Ensures UserAPI unit tests can run with password policy integration - Prevents NullPointerException when UserAPI references passwordPolicyService Required for testing UserAPI with the new password policy validation feature. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

- Create PasswordPolicyServiceIntegrationTest to verify property loading - Test that special characters are correctly parsed from configuration - Validate all configured special characters work as expected - Ensure property file escaping is handled properly - Test with actual Spring context and property loading This integration test ensures the configuration properties are correctly formatted and that the password policy works with real application context, catching any property parsing issues that unit tests might miss. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

- Fix password history off-by-one error in cleanUpPasswordHistory() - Keep historyCount + 1 entries to ensure proper password reuse prevention - With historyCount=3, now keeps [current, prev1, prev2, prev3] - Prevents immediate reuse of the Nth previous password - Fix UserAPIUnitTest NPE from unstubbed PasswordPolicyService mock - Add stub to return empty list (no validation errors) for all registration tests - Add Collections import for the stub return value - Ensures tests don't fail with NullPointerException on .isEmpty() call These changes address critical issues identified in code review: 1. Password history was only preventing reuse of historyCount-1 passwords 2. Unit tests were failing due to unstubbed mock returning null 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

Copilot

Pull Request Overview

Copilot reviewed 12 out of 14 changed files in this pull request and generated 3 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-23T01:33:59Z

src/main/resources/config/dsspringuserconfig.properties

+# Require at least one special character
+user.security.password.require-special=true
+# Allowed special characters
+user.security.password.special-chars=~`!@#$%^&*()_-+={}[]|\\:;"'<>,.?/


The backslash in the special characters string should be escaped properly. Consider using a double backslash (\\) to ensure proper parsing, or document the expected behavior for this configuration value.

Suggested change

user.security.password.special-chars=~`!@#$%^&*()_-+={}[]|\\:;"'<>,.?/

user.security.password.special-chars=~`!@#$%^&*()_-+={}[]|\\\\:;"'<>,.?/

Copilot · 2025-09-23T01:34:00Z

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

+                log.error("CRITICAL: Failed to load common passwords file. " +
+                    "Common password checking is DISABLED. This is a security risk!", e);


In production environments, failing to load the common passwords dictionary is a significant security risk. Consider failing fast (throwing an exception) or using a fallback mechanism instead of silently continuing with reduced security.

Copilot · 2025-09-23T01:34:00Z

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

+		// Keep historyCount + 1 entries: the current password plus historyCount previous passwords
+		// This ensures we actually prevent reuse of the last historyCount passwords
+		int maxEntries = historyCount + 1;


The logic for calculating maxEntries may be confusing. If historyCount=3 means 'prevent reuse of last 3 passwords', then keeping historyCount + 1 entries means keeping 4 total entries (current + 3 previous). Consider clarifying this behavior in the configuration documentation or adjusting the logic to match user expectations.

Suggested change

// Keep historyCount + 1 entries: the current password plus historyCount previous passwords

// This ensures we actually prevent reuse of the last historyCount passwords

int maxEntries = historyCount + 1;

// Keep the current password plus the last 'historyCount' previous passwords.

// This ensures we prevent reuse of the last 'historyCount' previous passwords (excluding the current password).

int maxEntries = historyCount + 1; // current password + historyCount previous passwords

Add Password Policy Functionality

9c9fcd4

devondragon self-assigned this Sep 22, 2025

devondragon requested a review from Copilot September 22, 2025 21:02

devondragon added the enhancement New feature or request label Sep 22, 2025

devondragon added this to SpringUserFramework Sep 22, 2025

github-project-automation bot moved this to In progress in SpringUserFramework Sep 22, 2025

Copilot AI reviewed Sep 22, 2025

View reviewed changes

fix: apply requested changes

64e9d2c

devondragon requested a review from Copilot September 22, 2025 23:54

Copilot AI reviewed Sep 22, 2025

View reviewed changes

devondragon mentioned this pull request Sep 23, 2025

Enhance Password Memory Security - Use char[] Instead of String #218

Closed

4 tasks

devondragon and others added 8 commits September 22, 2025 19:21

devondragon requested a review from Copilot September 23, 2025 01:33

Copilot AI reviewed Sep 23, 2025

View reviewed changes

devondragon merged commit 8fc188c into devondragon:main Sep 23, 2025

github-project-automation bot moved this from In progress to Done in SpringUserFramework Sep 23, 2025

Copilot AI mentioned this pull request Oct 26, 2025

Enhance password memory security using char[] with explicit clearing #228

Closed

		@Value("classpath:common_passwords.txt")
		private Resource commonPasswordsResource;

		// when(messages.getMessage(anyString(), any(), any(Locale.class)))
		// .thenAnswer(inv -> inv.getArgument(0, String.class));

	user.security.password.special-chars=~`!@#$%^&*()_-+={}[]\|\\:;"'<>,.?/
	user.security.password.special-chars=~`!@#$%^&*()_-+={}[]\|\\\\:;"'<>,.?/

		log.error("CRITICAL: Failed to load common passwords file. " +
		"Common password checking is DISABLED. This is a security risk!", e);

Add Password Policy Functionality #217

Add Password Policy Functionality #217

Uh oh!

Conversation

Edamijueda commented Sep 22, 2025

Uh oh!

Edamijueda commented Sep 22, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Edamijueda commented Sep 22, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants