feat: Add admin-initiated password reset with session invalidation #243
Conversation
Introduces SessionInvalidationService for expiring user sessions and adds initiateAdminPasswordReset() to UserEmailService. Enables admins to force password resets while optionally invalidating all active sessions, with full audit trail support.
- Add @PreAuthorize("hasRole('ROLE_ADMIN')") to admin password reset methods - Extract admin identity from SecurityContext instead of user input (fixes log injection) - Add URL validation to reject dangerous schemes (javascript:, data:) - Extract helper methods for SRP compliance - Add correlation ID for audit trail tracking - Add configurable warn threshold for session invalidation performance - Document race condition in SessionInvalidationService - Deprecate methods with adminIdentifier parameter - Add comprehensive tests for new security features
- Change hasRole('ROLE_ADMIN') to hasRole('ADMIN') in all @PreAuthorize
annotations (Spring auto-prefixes ROLE_)
- Add URL validation to sendForgotPasswordVerificationEmail and
sendRegistrationVerificationEmail to prevent XSS
- Switch audit extraData from CSV to JSON format for better parsing
- Update all corresponding tests
Security improvements for admin password reset feature: - Reorder operations in initiateAdminPasswordReset to prevent user lockout: token creation and email sending now happen before session invalidation - Replace UUID.randomUUID() with SecureRandom for cryptographically strong 256-bit tokens (Base64 URL-safe encoded) - Replace vulnerable custom JSON escaping with Jackson ObjectMapper for proper serialization in audit events - Truncate session IDs to first 8 chars in debug logs to prevent exposure Updates tests to expect Base64 token format instead of UUID format.
Code Review: Admin-Initiated Password Reset FeatureSummaryThis PR adds admin-initiated password reset functionality with session invalidation. The implementation is well-designed with strong security measures, comprehensive testing, and excellent documentation. ✅ StrengthsSecurity
Code Quality
Test Coverage
🔍 Issues & SuggestionsCritical IssuesNone identified. Minor Issues1. Potential NPE in DSUserDetails methods (DSUserDetails.java:202-203)
2. Inconsistent parameter validation (UserEmailService.java:158)
3. Performance consideration documented but not mitigated (SessionInvalidationService.java:58-64)
4. Race condition acknowledged but not addressed (SessionInvalidationService.java:22-27, 69-70)
🎯 Performance Considerations
🧪 Test Coverage AssessmentExcellent coverage including:
Potential gaps:
📝 Documentation QualityOutstanding documentation with:
🔐 Security ReviewNo security concerns identified. The implementation follows security best practices:
Final Recommendation✅ APPROVED - This is high-quality, production-ready code with excellent security practices, comprehensive testing, and thorough documentation. The minor issues noted are not blockers. Optional Follow-ups (Post-Merge)
Great work on this feature! 🎉 |
There was a problem hiding this comment.
Pull request overview
This pull request adds admin-initiated password reset functionality with optional session invalidation to force users to re-authenticate. The implementation includes comprehensive security measures, audit logging with correlation IDs, and proper URL validation to prevent XSS attacks.
Changes:
- Added
SessionInvalidationServicefor invalidating user sessions across different principal types - Extended
UserEmailServicewith admin password reset methods protected by@PreAuthorize("hasRole('ADMIN')") - Replaced UUID-based token generation with cryptographically secure Base64-encoded tokens (256-bit entropy)
- Implemented URL validation to reject dangerous schemes (javascript:, data:) and ensure proper HTTP/HTTPS URLs
- Added comprehensive test coverage with 45 tests covering security, validation, and edge cases
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
SessionInvalidationService.java |
New service for invalidating user sessions with performance monitoring and race condition documentation |
UserEmailService.java |
Added admin password reset methods with URL validation, secure token generation, audit logging, and deprecated backward-compatible methods |
SessionInvalidationServiceTest.java |
Comprehensive test coverage for session invalidation with 45 tests covering various principal types and edge cases |
UserEmailServiceTest.java |
Extended tests for admin password reset, URL validation, PreAuthorize annotations, and deprecated method compatibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private final ObjectMapper objectMapper = new ObjectMapper(); | ||
|
|
||
| /** SecureRandom for cryptographically strong token generation. */ | ||
| private final SecureRandom secureRandom = new SecureRandom(); |
There was a problem hiding this comment.
The SecureRandom instance is created as a private final field, which means a new SecureRandom is created for each UserEmailService instance. While this works, SecureRandom instances are thread-safe and can be expensive to initialize. Consider using a static instance or injecting a shared instance for better performance, especially if multiple UserEmailService instances might be created.
| private final SecureRandom secureRandom = new SecureRandom(); | |
| private static final SecureRandom secureRandom = new SecureRandom(); |
| private boolean isValidAppUrl(final String url) { | ||
| if (url == null || url.isBlank()) { | ||
| return false; | ||
| } | ||
| try { | ||
| URI uri = new URI(url); | ||
| String scheme = uri.getScheme(); | ||
| return scheme != null && (scheme.equalsIgnoreCase("http") || scheme.equalsIgnoreCase("https")) | ||
| && uri.getHost() != null; | ||
| } catch (URISyntaxException e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The URL validation logic checks for null/blank and scheme/host validation, but doesn't validate against open redirect vulnerabilities. Consider adding validation to ensure the URL domain is whitelisted or belongs to the expected application domain, to prevent attackers from using this endpoint to send password reset links to malicious domains.
| // Return minimal safe JSON on failure | ||
| return "{\"error\":\"serialization_failed\"}"; |
There was a problem hiding this comment.
The error handling for JSON serialization failure returns a generic error message but doesn't include any context about what failed. Consider including the correlationId or adminIdentifier in the fallback JSON to maintain some auditability even when serialization fails.
| // Return minimal safe JSON on failure | |
| return "{\"error\":\"serialization_failed\"}"; | |
| // Return minimal safe JSON on failure, preserving some context for auditability | |
| String safeAdminIdentifier = adminIdentifier == null ? "" : | |
| adminIdentifier.replace("\\", "\\\\").replace("\"", "\\\""); | |
| String safeCorrelationId = correlationId == null ? "" : | |
| correlationId.replace("\\", "\\\\").replace("\"", "\\\""); | |
| return "{\"error\":\"serialization_failed\",\"adminIdentifier\":\"" + safeAdminIdentifier | |
| + "\",\"correlationId\":\"" + safeCorrelationId + "\"}"; |
| @Test | ||
| @DisplayName("uses default threshold of 1000") | ||
| void usesDefaultThresholdOf1000() { | ||
| // Given - create a new service without setting threshold (should use default) | ||
| SessionInvalidationService newService = new SessionInvalidationService(sessionRegistry); | ||
|
|
||
| // Verify the default value is set correctly via reflection | ||
| Integer threshold = (Integer) ReflectionTestUtils.getField(newService, "warnThreshold"); | ||
|
|
||
| // Then - default should be 0 (unset by Spring) since we're not using Spring context | ||
| // In production, Spring will inject the default value of 1000 | ||
| assertThat(threshold).isEqualTo(0); | ||
| } |
There was a problem hiding this comment.
The test expects the threshold to be 0 when Spring context is not used, but this creates a misleading test. The comment states "In production, Spring will inject the default value of 1000", but the @value annotation default is already specified as 1000 in the service. Consider either testing the actual Spring-injected default value using a Spring test context, or updating the test name and assertion to reflect that it's testing the uninitialized state rather than the "default threshold of 1000".
| public int initiateAdminPasswordReset(final User user, final String appUrl, final boolean invalidateSessions) { | ||
| final String correlationId = generateToken(); | ||
| final String adminIdentifier = getCurrentAdminIdentifier(); | ||
|
|
||
| log.debug("UserEmailService.initiateAdminPasswordReset: called for user: {} by admin: {} [correlationId={}]", | ||
| user.getEmail(), adminIdentifier, correlationId); | ||
|
|
||
| // Step 1: Generate token and create password reset token (must succeed before invalidating sessions) | ||
| final String token = generateToken(); | ||
| createPasswordResetTokenForUser(user, token); | ||
|
|
||
| // Step 2: Send password reset email (must succeed before invalidating sessions) | ||
| sendPasswordResetEmail(user, appUrl, token); | ||
|
|
||
| // Step 3: Invalidate sessions LAST - only after recovery mechanism is in place | ||
| int invalidatedSessions = handleSessionInvalidation(user, invalidateSessions, correlationId); | ||
|
|
||
| // Step 4: Publish admin-specific audit event | ||
| publishAdminPasswordResetAuditEvent(user, adminIdentifier, invalidatedSessions, correlationId); | ||
|
|
||
| log.info("UserEmailService.initiateAdminPasswordReset: password reset email sent to {} by admin {} [correlationId={}]", | ||
| user.getEmail(), adminIdentifier, correlationId); | ||
|
|
||
| return invalidatedSessions; | ||
| } |
There was a problem hiding this comment.
The method does not validate that the user parameter is non-null before proceeding. While the underlying methods may handle null users, it would be better to fail fast with a clear error message at the API entry point. Consider adding a null check at the beginning of the method.
| private String configuredAppUrl; | ||
|
|
||
| /** ObjectMapper for JSON serialization in audit events. */ | ||
| private final ObjectMapper objectMapper = new ObjectMapper(); |
There was a problem hiding this comment.
The ObjectMapper instance is created as a private final field without configuration. This creates a new instance per UserEmailService object, which is inefficient. Consider injecting a shared ObjectMapper bean or using a static instance, as ObjectMapper is thread-safe and expensive to construct.
| private final ObjectMapper objectMapper = new ObjectMapper(); | |
| private final ObjectMapper objectMapper; |
| // Step 2: Send password reset email (must succeed before invalidating sessions) | ||
| sendPasswordResetEmail(user, appUrl, token); | ||
|
|
||
| // Step 3: Invalidate sessions LAST - only after recovery mechanism is in place | ||
| int invalidatedSessions = handleSessionInvalidation(user, invalidateSessions, correlationId); | ||
|
|
||
| // Step 4: Publish admin-specific audit event | ||
| publishAdminPasswordResetAuditEvent(user, adminIdentifier, invalidatedSessions, correlationId); | ||
|
|
||
| log.info("UserEmailService.initiateAdminPasswordReset: password reset email sent to {} by admin {} [correlationId={}]", | ||
| user.getEmail(), adminIdentifier, correlationId); | ||
|
|
||
| return invalidatedSessions; |
There was a problem hiding this comment.
The method documentation states that "Session invalidation is performed last to ensure users are not locked out if token creation or email sending fails," but if sendPasswordResetEmail throws an exception (e.g., IllegalArgumentException from URL validation), the audit event will never be published. This creates an incomplete audit trail. Consider wrapping the operation in a try-catch block to ensure audit events are published even on failure, or document this limitation.
| // Step 2: Send password reset email (must succeed before invalidating sessions) | |
| sendPasswordResetEmail(user, appUrl, token); | |
| // Step 3: Invalidate sessions LAST - only after recovery mechanism is in place | |
| int invalidatedSessions = handleSessionInvalidation(user, invalidateSessions, correlationId); | |
| // Step 4: Publish admin-specific audit event | |
| publishAdminPasswordResetAuditEvent(user, adminIdentifier, invalidatedSessions, correlationId); | |
| log.info("UserEmailService.initiateAdminPasswordReset: password reset email sent to {} by admin {} [correlationId={}]", | |
| user.getEmail(), adminIdentifier, correlationId); | |
| return invalidatedSessions; | |
| int invalidatedSessions = 0; | |
| try { | |
| // Step 2: Send password reset email (must succeed before invalidating sessions) | |
| sendPasswordResetEmail(user, appUrl, token); | |
| // Step 3: Invalidate sessions LAST - only after recovery mechanism is in place | |
| invalidatedSessions = handleSessionInvalidation(user, invalidateSessions, correlationId); | |
| log.info("UserEmailService.initiateAdminPasswordReset: password reset email sent to {} by admin {} [correlationId={}]", | |
| user.getEmail(), adminIdentifier, correlationId); | |
| return invalidatedSessions; | |
| } finally { | |
| // Step 4: Publish admin-specific audit event (always, even if an exception occurs) | |
| publishAdminPasswordResetAuditEvent(user, adminIdentifier, invalidatedSessions, correlationId); | |
| } |
| AuditEvent auditEvent = auditCaptor.getValue(); | ||
| // Audit extraData is now JSON format | ||
| assertThat(auditEvent.getExtraData()).contains("\"correlationId\":\""); | ||
| // Verify correlation ID is a UUID format within JSON |
There was a problem hiding this comment.
The comment states "Verify correlation ID is a UUID format within JSON" but this is incorrect - the correlation ID is a Base64 URL-safe encoded 32-byte token (as correctly noted on line 519), not a UUID. The comment should be corrected to avoid confusion.
| // Verify correlation ID is a UUID format within JSON | |
| // Verify correlation ID in JSON is a Base64 URL-safe encoded 32-byte token |
Add null check for oidcUserInfo before calling getClaims() to prevent NullPointerException when DSUserDetails is constructed without OIDC info. Returns empty map instead of throwing NPE when OIDC info is not available.
- Add admin password reset to Features list in README - Add Admin Password Reset section with code examples and security notes - Add Admin Settings section to CONFIG.md with new configuration properties
Pull Request Review: Admin-Initiated Password Reset with Session InvalidationOverall AssessmentThis is a well-implemented feature with strong security considerations, comprehensive test coverage, and good documentation. The code demonstrates best practices in several areas, though there are a few recommendations for improvement. ✅ StrengthsSecurity
Code Quality
🔍 Issues & RecommendationsHigh Priority
Medium Priority
Low Priority
🔒 Security AssessmentMitigated Attack Vectors✅ XSS via URL injection Remaining Considerations
🎯 Recommendations SummaryShould Fix (Before Merge)
Nice to Have
📊 Test Coverage: Excellent ⭐⭐⭐⭐⭐45 tests covering happy paths, edge cases, security validations, and both principal types. Final VerdictAPPROVE ✅ This PR demonstrates excellent software engineering practices with strong security considerations, comprehensive testing, and good documentation. The identified issues are minor and mostly suggestions for future enhancements. Confidence Level: High - Implementation is solid, well-tested, and follows security best practices. |
Pull Request Review: Admin Password Reset FeatureOverviewThis PR adds a well-designed admin password reset feature with session invalidation capabilities. The implementation demonstrates strong security awareness and solid engineering practices. StrengthsSecurity
Code Quality
Issues and RecommendationsMinor Issues (Optional improvements)
Performance Considerations
Code Style Suggestions
Test Coverage AnalysisExcellent coverage with 45 tests covering:
Missing tests: Integration test for Spring Security authorization, email sending failures, token repository save failures, concurrent session creation scenarios Documentation Review
Merge Status: APPROVE with minor suggestionsBefore Merge (Optional)
Future Enhancements
Overall AssessmentThis is high-quality, production-ready code that demonstrates strong security awareness, excellent documentation, comprehensive testing, thoughtful error handling, and good separation of concerns. The minor issues identified are optimizations rather than bugs. The feature is well-designed and implements a critical security capability safely. Score: 9/10 - Excellent work! |
Summary
Adds the ability for administrators to trigger password resets for users, with optional session invalidation to force re-authentication.
New Features
initiateAdminPasswordReset) with@PreAuthorize("hasRole('ADMIN')")protectionuser.admin.appUrlpropertySecurity Measures
SecureRandom(256-bit entropy)SecurityContext, not user inputOperation Safety
Files Changed
SessionInvalidationService.java- New service for invalidating user sessionsUserEmailService.java- Extended with admin password reset functionalityTest plan
./gradlew test)@PreAuthorizeannotations verified via reflection testsKnown Limitations (Documented)