Skip to content

Commit 451d6f5

Browse files
devondragonclaude
andcommitted
Fix high-priority Spring User Framework issues
- Fix jar artifact naming from ds-spring-ai-client to ds-spring-user-framework - Move transitive runtime dependencies to test scope (devtools, mariadb, postgresql) - Fix JPA equals/hashCode anti-patterns in Role and Privilege entities - Fix audit log writer concurrency with synchronized methods - Fix AuthorityServiceTest by adding IDs to test Privilege objects 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent d183b4d commit 451d6f5

6 files changed

Lines changed: 161 additions & 13 deletions

File tree

FIXES.md

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
# Spring User Framework - Issues to Fix
2+
3+
## High-Impact Issues (Priority 1)
4+
5+
### 1. Fix jar artifact naming mismatch ✅ COMPLETED
6+
- **Issue**: Jar task sets archiveBaseName to 'ds-spring-ai-client' (copy/paste error)
7+
- **Fix**: Change to 'ds-spring-user-framework' in build.gradle line 109
8+
- **Status**: Fixed - changed archiveBaseName to correct value
9+
10+
### 2. Remove transitive runtime dependencies ✅ COMPLETED
11+
- **Issue**: Library declares runtimeOnly dependencies that surprise consumers
12+
- **Fix**: Move spring-boot-devtools, mariadb-java-client, postgresql to testRuntimeOnly
13+
- **Status**: Fixed - moved all runtime dependencies to test scope
14+
15+
### 3. Fix JPA equals/hashCode anti-patterns ✅ COMPLETED
16+
- **Issue**: Role and Privilege use @Data without excluding relationships (causes stack overflows)
17+
- **Fix**: Add @EqualsAndHashCode.Exclude to collection fields, base on id only
18+
- **Status**: Fixed - replaced @Data with explicit @EqualsAndHashCode(onlyExplicitlyIncluded = true) and @EqualsAndHashCode.Include on id fields
19+
20+
### 4. Fix audit log writer concurrency ✅ COMPLETED
21+
- **Issue**: FileAuditLogWriter and scheduler access shared BufferedWriter without synchronization
22+
- **Fix**: Add synchronized blocks to protect concurrent access
23+
- **Status**: Fixed - added synchronized keyword to writeLog(), flushWriter(), setup(), and cleanup() methods
24+
25+
### 5. Fix registration email base URL
26+
- **Issue**: UserAPI.publishRegistrationEvent uses request.getContextPath() (broken links)
27+
- **Fix**: Use UserUtils.getAppUrl(request) like other flows
28+
29+
### 6. Configure security remember-me properly
30+
- **Issue**: Uses random key per startup, invalidates on restart
31+
- **Fix**: Make opt-in with explicit key configuration
32+
33+
### 7. Remove @Async from event classes
34+
- **Issue**: @Async on POJOs has no effect (false impression)
35+
- **Fix**: Remove from AuditEvent and OnRegistrationCompleteEvent classes
36+
37+
## Security & API Issues (Priority 2)
38+
39+
### 8. Add DTO validation annotations
40+
- **Issue**: UserDto and PasswordDto lack bean validation
41+
- **Fix**: Add @NotBlank, @Email, password constraints, create @ControllerAdvice
42+
43+
### 9. Fix CSRF property typo
44+
- **Issue**: Property name contains odd "d" - 'disableCSRFdURIs'
45+
- **Fix**: Rename to 'disableCSRFURIs'
46+
47+
### 10. Improve error message handling
48+
- **Issue**: CustomOAuth2AuthenticationEntryPoint exposes exception details
49+
- **Fix**: Use generic user messages, log details internally
50+
51+
### 11. Enhance IP detection
52+
- **Issue**: Only honors X-Forwarded-For header
53+
- **Fix**: Support X-Real-IP, CF-Connecting-IP, True-Client-IP
54+
55+
## Web/Security Config (Priority 3)
56+
57+
### 12. Fix property injection robustness
58+
- **Issue**: Empty property yields list with empty string
59+
- **Fix**: Filter empty strings from unprotectedURIs list
60+
61+
### 13. Configure role hierarchy for method security
62+
- **Issue**: Method security doesn't use role hierarchy automatically
63+
- **Fix**: Create MethodSecurityExpressionHandler bean with hierarchy
64+
65+
### 14. Replace System.out.println with SLF4J
66+
- **Issue**: Using stdout instead of proper logging
67+
- **Fix**: Update CustomOAuth2AuthenticationEntryPoint and TimeLogger
68+
69+
## Persistence & Domain (Priority 3)
70+
71+
### 15. Clean up User.roles type handling
72+
- **Issue**: Mixed List/Set setters, defensive copying
73+
- **Fix**: Standardize collection handling for JPA dirty checking
74+
75+
## Email & Templates (Priority 3)
76+
77+
### 16. Improve MailService error handling
78+
- **Issue**: Exceptions only logged and swallowed
79+
- **Fix**: Add Spring Retry mechanism or queue
80+
81+
### 17. Document Thymeleaf dependency
82+
- **Issue**: Relies on optional TemplateEngine bean
83+
- **Fix**: Document requirement prominently
84+
85+
## Audit Issues (Priority 4)
86+
87+
### 18. Improve audit log defaults
88+
- **Issue**: Default path /opt/app/logs unlikely to be writable
89+
- **Fix**: Use temp directory or auto-create with graceful failure
90+
91+
### 19. Document conditional flushing
92+
- **Issue**: Complex conditional expression hard to understand
93+
- **Fix**: Add clear documentation
94+
95+
## Build & Publishing (Priority 4)
96+
97+
### 20. Fix group coordinate mismatch
98+
- **Issue**: group = 'com.digitalsanctuary.springuser' vs publishing 'com.digitalsanctuary'
99+
- **Fix**: Align group with publishing coordinates
100+
101+
### 21. Dependency management consistency
102+
- **Issue**: Mixed explicit versions and BOM usage
103+
- **Fix**: Prefer Boot BOM for all Spring dependencies
104+
105+
### 22. Simplify test task configuration
106+
- **Issue**: Overriding test task unusual for library
107+
- **Fix**: Make testAll optional, restore standard test task
108+
109+
## UX & Behavior (Priority 4)
110+
111+
### 23. Document registration verification flow
112+
- **Issue**: Auto-enable vs email verification unclear
113+
- **Fix**: Add clear documentation
114+
115+
### 24. Make post-auth redirects configurable
116+
- **Issue**: Forces alwaysUseDefaultTargetUrl(true), surprising UX
117+
- **Fix**: Add configuration property
118+
119+
### 25. Make global model injection opt-in
120+
- **Issue**: Adds user to all MVC views by default
121+
- **Fix**: Make opt-in for REST-only apps
122+
123+
## Documentation
124+
125+
### 26. Create comprehensive getting started guide
126+
- **Fix**: Document required dependencies, minimal properties, examples
127+
128+
## Notes
129+
- All issues have been validated against the codebase
130+
- Fixes should include appropriate tests
131+
- Run ./gradlew check after each fix to ensure no regressions

build.gradle

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,7 @@ dependencies {
4444
compileOnly 'org.thymeleaf.extras:thymeleaf-extras-springsecurity6:3.1.3.RELEASE'
4545
compileOnly 'nz.net.ultraq.thymeleaf:thymeleaf-layout-dialect:3.4.0'
4646

47-
// Other dependencies
48-
runtimeOnly 'org.springframework.boot:spring-boot-devtools'
49-
runtimeOnly 'org.mariadb.jdbc:mariadb-java-client:3.5.5'
50-
runtimeOnly 'org.postgresql:postgresql'
47+
// Other dependencies (moved to test scope for library)
5148
implementation 'org.passay:passay:1.6.6'
5249
implementation 'com.google.guava:guava:33.4.8-jre'
5350
compileOnly 'org.springframework.boot:spring-boot-starter-actuator'
@@ -73,6 +70,11 @@ dependencies {
7370
testImplementation 'org.springframework.security:spring-security-test'
7471
testImplementation 'com.h2database:h2:2.3.232'
7572

73+
// Runtime dependencies moved to test scope for library
74+
testRuntimeOnly 'org.springframework.boot:spring-boot-devtools'
75+
testRuntimeOnly 'org.mariadb.jdbc:mariadb-java-client:3.5.5'
76+
testRuntimeOnly 'org.postgresql:postgresql'
77+
7678
// Additional test dependencies for improved testing
7779
testImplementation 'org.testcontainers:testcontainers:1.21.3'
7880
testImplementation 'org.testcontainers:mariadb:1.21.3'
@@ -106,7 +108,7 @@ test {
106108

107109
tasks.named('jar') {
108110
enabled = true
109-
archiveBaseName.set('ds-spring-ai-client')
111+
archiveBaseName.set('ds-spring-user-framework')
110112
archiveClassifier.set('')
111113
}
112114

src/main/java/com/digitalsanctuary/spring/user/audit/FileAuditLogWriter.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ public class FileAuditLogWriter implements AuditLogWriter {
3232
*/
3333
@PostConstruct
3434
@Override
35-
public void setup() {
35+
public synchronized void setup() {
3636
log.info("FileAuditLogWriter.setup: Entering...");
3737
if (!validateConfig()) {
3838
return;
@@ -46,7 +46,7 @@ public void setup() {
4646
*/
4747
@PreDestroy
4848
@Override
49-
public void cleanup() {
49+
public synchronized void cleanup() {
5050
log.info("FileAuditLogWriter.cleanup: Closing log file.");
5151
closeLogFile();
5252
}
@@ -58,7 +58,7 @@ public void cleanup() {
5858
* @param event the audit event to write
5959
*/
6060
@Override
61-
public void writeLog(AuditEvent event) {
61+
public synchronized void writeLog(AuditEvent event) {
6262
if (bufferedWriter == null) {
6363
log.error("FileAuditLogWriter.writeLog: BufferedWriter is not initialized.");
6464
return;
@@ -84,7 +84,7 @@ public void writeLog(AuditEvent event) {
8484
* Flushes the buffered writer to ensure all data is written to the log file. This method is called by the {@link FileAuditLogFlushScheduler} to
8585
* ensure that the buffer is flushed periodically to balance performance with data integrity.
8686
*/
87-
public void flushWriter() {
87+
public synchronized void flushWriter() {
8888
if (bufferedWriter != null) {
8989
try {
9090
bufferedWriter.flush();

src/main/java/com/digitalsanctuary/spring/user/persistence/model/Privilege.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,19 +6,25 @@
66
import jakarta.persistence.GenerationType;
77
import jakarta.persistence.Id;
88
import jakarta.persistence.ManyToMany;
9-
import lombok.Data;
9+
import lombok.EqualsAndHashCode;
10+
import lombok.Getter;
11+
import lombok.Setter;
1012
import lombok.ToString;
1113

1214
/**
1315
* The Privilege Entity. Part of the basic User ->> Role ->> Privilege structure.
1416
*/
15-
@Data
17+
@Getter
18+
@Setter
19+
@EqualsAndHashCode(onlyExplicitlyIncluded = true)
20+
@ToString
1621
@Entity
1722
public class Privilege {
1823

1924
/** The id. */
2025
@Id
2126
@GeneratedValue(strategy = GenerationType.AUTO)
27+
@EqualsAndHashCode.Include
2228
private Long id;
2329

2430
/** The name. */

src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,18 +11,24 @@
1111
import jakarta.persistence.JoinColumn;
1212
import jakarta.persistence.JoinTable;
1313
import jakarta.persistence.ManyToMany;
14-
import lombok.Data;
14+
import lombok.EqualsAndHashCode;
15+
import lombok.Getter;
16+
import lombok.Setter;
1517
import lombok.ToString;
1618

1719
/**
1820
* The Role Entity. Part of the basic User ->> Role ->> Privilege structure.
1921
*/
20-
@Data
22+
@Getter
23+
@Setter
24+
@EqualsAndHashCode(onlyExplicitlyIncluded = true)
25+
@ToString
2126
@Entity
2227
public class Role {
2328
/** The id. */
2429
@Id
2530
@GeneratedValue(strategy = GenerationType.AUTO)
31+
@EqualsAndHashCode.Include
2632
@ToString.Include
2733
private Long id;
2834

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,8 +258,11 @@ void getAuthoritiesFromRoles_privilegeWithEmptyName_throwsException() {
258258
void getAuthoritiesFromRoles_mixedCasePrivileges_preservesCase() {
259259
// Given
260260
Privilege lowerPrivilege = new Privilege("read_privilege");
261+
lowerPrivilege.setId(1L);
261262
Privilege upperPrivilege = new Privilege("WRITE_PRIVILEGE");
263+
upperPrivilege.setId(2L);
262264
Privilege mixedPrivilege = new Privilege("Delete_Privilege");
265+
mixedPrivilege.setId(3L);
263266

264267
Role testRole = RoleTestDataBuilder.aRole()
265268
.withName("ROLE_TEST")

0 commit comments

Comments
 (0)