Skip to content

Commit 080489e

Browse files
kruscheclaude
andcommitted
Fix security, resource leaks, and code quality issues from deep review
P1 fixes: - Add @PreAuthorize("isAuthenticated()") to DataExportController - Reserve anonymizedAt for full anonymization only (not set in soft deletion where name/files are preserved for retention) - Explicitly delete ApplicationReviewer records before deleting expired rejected applications to avoid reliance on DB cascade behavior P2 fixes: - Prevent duplicate export processing in multi-instance deployments via atomic claimForProcessing() JPQL update - Fix resource leak: wrap InputStream in try-with-resources in addUserFile during data export ZIP creation - Change UserDeletionPreviewDto boolean fields to Boolean wrappers so false values are not omitted by @JsonInclude(NON_EMPTY) - Collect user IDs before processing deferred deletions to safely handle entityManager.clear() within the loop - Move application activity check into findInactiveStudentCandidates JPQL query to eliminate N+1 queries in disableInactiveUsers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 71dea50 commit 080489e

File tree

7 files changed

+50
-27
lines changed

7 files changed

+50
-27
lines changed

server/src/main/java/de/tum/cit/aet/thesis/controller/DataExportController.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
@RestController
2525
@RequestMapping("/v2/data-exports")
26+
@org.springframework.security.access.prepost.PreAuthorize("isAuthenticated()")
2627
public class DataExportController {
2728
private final DataExportService dataExportService;
2829
private final ObjectProvider<CurrentUserProvider> currentUserProviderProvider;

server/src/main/java/de/tum/cit/aet/thesis/dto/UserDeletionPreviewDto.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@
66

77
@JsonInclude(JsonInclude.Include.NON_EMPTY)
88
public record UserDeletionPreviewDto(
9-
boolean canBeFullyDeleted,
10-
boolean hasActiveTheses,
9+
Boolean canBeFullyDeleted,
10+
Boolean hasActiveTheses,
1111
int retentionBlockedThesisCount,
1212
Instant earliestFullDeletionDate,
13-
boolean isResearchGroupHead,
13+
Boolean isResearchGroupHead,
1414
String message
1515
) {
1616
}

server/src/main/java/de/tum/cit/aet/thesis/repository/DataExportRepository.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,11 @@
44
import de.tum.cit.aet.thesis.entity.DataExport;
55
import de.tum.cit.aet.thesis.entity.User;
66
import org.springframework.data.jpa.repository.JpaRepository;
7+
import org.springframework.data.jpa.repository.Modifying;
78
import org.springframework.data.jpa.repository.Query;
89
import org.springframework.data.repository.query.Param;
910
import org.springframework.stereotype.Repository;
11+
import org.springframework.transaction.annotation.Transactional;
1012

1113
import java.time.Instant;
1214
import java.util.List;
@@ -18,6 +20,11 @@ public interface DataExportRepository extends JpaRepository<DataExport, UUID> {
1820

1921
List<DataExport> findAllByStateIn(List<DataExportState> states);
2022

23+
@Modifying
24+
@Transactional
25+
@Query("UPDATE DataExport e SET e.state = 'IN_CREATION' WHERE e.id = :id AND e.state = :expectedState")
26+
int claimForProcessing(@Param("id") UUID id, @Param("expectedState") DataExportState expectedState);
27+
2128
@Query("""
2229
SELECT e FROM DataExport e
2330
WHERE e.creationFinishedAt < :cutoff

server/src/main/java/de/tum/cit/aet/thesis/repository/UserRepository.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,10 @@ AND NOT EXISTS (
100100
de.tum.cit.aet.thesis.constants.ThesisState.DROPPED_OUT
101101
)
102102
)
103+
AND NOT EXISTS (
104+
SELECT 1 FROM Application a
105+
WHERE a.user.id = u.id AND a.createdAt >= :cutoff
106+
)
103107
""")
104108
List<User> findInactiveStudentCandidates(@Param("cutoff") Instant cutoff);
105109
}

server/src/main/java/de/tum/cit/aet/thesis/service/DataExportService.java

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,13 @@ public void processAllPendingExports() {
175175
List.of(DataExportState.REQUESTED));
176176

177177
for (DataExport export : pending) {
178+
// Atomically claim this export to prevent duplicate processing
179+
// in multi-instance deployments.
180+
int updated = dataExportRepository.claimForProcessing(export.getId(), DataExportState.REQUESTED);
181+
if (updated == 0) {
182+
continue; // Another instance already claimed it
183+
}
184+
178185
try {
179186
createDataExport(export);
180187
} catch (Exception e) {
@@ -188,7 +195,6 @@ public void processAllPendingExports() {
188195

189196
private void createDataExport(DataExport export) throws IOException {
190197
export.setState(DataExportState.IN_CREATION);
191-
dataExportRepository.save(export);
192198

193199
User user = export.getUser();
194200
String filename = String.format("export_%s_%d.zip", user.getId(), System.currentTimeMillis());
@@ -392,7 +398,9 @@ private void addUserFile(ZipOutputStream zos, String filename, String entryPrefi
392398
extension = filename.substring(dotIndex);
393399
}
394400
zos.putNextEntry(new ZipEntry(entryPrefix + extension));
395-
resource.getInputStream().transferTo(zos);
401+
try (java.io.InputStream is = resource.getInputStream()) {
402+
is.transferTo(zos);
403+
}
396404
zos.closeEntry();
397405
}
398406
} catch (Exception e) {

server/src/main/java/de/tum/cit/aet/thesis/service/DataRetentionService.java

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import de.tum.cit.aet.thesis.entity.User;
44
import de.tum.cit.aet.thesis.repository.ApplicationRepository;
5+
import de.tum.cit.aet.thesis.repository.ApplicationReviewerRepository;
56
import de.tum.cit.aet.thesis.repository.UserRepository;
67
import org.slf4j.Logger;
78
import org.slf4j.LoggerFactory;
@@ -19,19 +20,22 @@ public class DataRetentionService {
1920
private static final Logger log = LoggerFactory.getLogger(DataRetentionService.class);
2021

2122
private final ApplicationRepository applicationRepository;
23+
private final ApplicationReviewerRepository applicationReviewerRepository;
2224
private final UserRepository userRepository;
2325
private final DataExportService dataExportService;
2426
private final UserDeletionService userDeletionService;
2527
private final int retentionDays;
2628
private final int inactiveUserDays;
2729

2830
public DataRetentionService(ApplicationRepository applicationRepository,
31+
ApplicationReviewerRepository applicationReviewerRepository,
2932
UserRepository userRepository,
3033
DataExportService dataExportService,
3134
UserDeletionService userDeletionService,
3235
@Value("${thesis-management.data-retention.rejected-application-retention-days}") int retentionDays,
3336
@Value("${thesis-management.data-retention.inactive-user-days}") int inactiveUserDays) {
3437
this.applicationRepository = applicationRepository;
38+
this.applicationReviewerRepository = applicationReviewerRepository;
3539
this.userRepository = userRepository;
3640
this.dataExportService = dataExportService;
3741
this.userDeletionService = userDeletionService;
@@ -51,11 +55,7 @@ public void runNightlyCleanup() {
5155
public int disableInactiveUsers() {
5256
Instant cutoff = Instant.now().minus(inactiveUserDays, ChronoUnit.DAYS);
5357

54-
List<User> candidates = userRepository.findInactiveStudentCandidates(cutoff);
55-
56-
List<User> toDisable = candidates.stream()
57-
.filter(user -> hasNoRecentActivity(user, cutoff))
58-
.toList();
58+
List<User> toDisable = userRepository.findInactiveStudentCandidates(cutoff);
5959

6060
if (toDisable.isEmpty()) {
6161
return 0;
@@ -71,13 +71,6 @@ public int disableInactiveUsers() {
7171
return toDisable.size();
7272
}
7373

74-
private boolean hasNoRecentActivity(User user, Instant cutoff) {
75-
boolean hasRecentApplication = applicationRepository.findAllByUser(user).stream()
76-
.anyMatch(app -> app.getCreatedAt().isAfter(cutoff));
77-
78-
return !hasRecentApplication;
79-
}
80-
8174
public int deleteExpiredRejectedApplications() {
8275
Instant cutoffDate = Instant.now().minus(retentionDays, ChronoUnit.DAYS);
8376

@@ -92,6 +85,7 @@ public int deleteExpiredRejectedApplications() {
9285

9386
for (UUID id : expiredIds) {
9487
try {
88+
applicationReviewerRepository.deleteByApplicationId(id);
9589
applicationRepository.deleteApplicationById(id);
9690
totalDeleted++;
9791
} catch (Exception e) {

server/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.java

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -169,32 +169,40 @@ public UserDeletionResultDto deleteOrAnonymizeUser(UUID userId) {
169169
}
170170

171171
public void processDeferredDeletions() {
172-
List<User> pendingUsers = userRepository.findAllByDeletionScheduledForIsNotNull();
172+
// Collect IDs first because anonymizeUser() clears the persistence context,
173+
// which would detach entities loaded in the same session.
174+
List<UUID> pendingUserIds = userRepository.findAllByDeletionScheduledForIsNotNull()
175+
.stream().map(User::getId).toList();
176+
177+
for (UUID userId : pendingUserIds) {
178+
User user = userRepository.findById(userId).orElse(null);
179+
if (user == null) {
180+
continue;
181+
}
173182

174-
for (User user : pendingUsers) {
175-
List<ThesisRole> retentionBlocked = getRetentionBlockedThesisRoles(user.getId());
183+
List<ThesisRole> retentionBlocked = getRetentionBlockedThesisRoles(userId);
176184
if (retentionBlocked.isEmpty()) {
177-
log.info("Retention expired for user {}, performing full cleanup", user.getId());
185+
log.info("Retention expired for user {}, performing full cleanup", userId);
178186

179187
// Collect file paths before DB changes
180188
List<String> exportFilePaths = collectExportFilePaths(user);
181189
List<String> userFilePaths = collectUserFilePaths(user);
182190

183191
// Delete all remaining related data
184192
deleteDataExportRecords(user);
185-
List<Application> remainingApps = applicationRepository.findAllByUserId(user.getId());
193+
List<Application> remainingApps = applicationRepository.findAllByUserId(userId);
186194
for (Application app : remainingApps) {
187195
applicationReviewerRepository.deleteByApplicationId(app.getId());
188196
}
189-
applicationRepository.deleteAllByUserId(user.getId());
190-
topicRoleRepository.deleteAllByIdUserId(user.getId());
191-
thesisRoleRepository.deleteAllByIdUserId(user.getId());
197+
applicationRepository.deleteAllByUserId(userId);
198+
topicRoleRepository.deleteAllByIdUserId(userId);
199+
thesisRoleRepository.deleteAllByIdUserId(userId);
192200

193201
// Fully anonymize the tombstone (clear name + file references)
194202
// anonymizeUser clears the persistence context and re-fetches,
195203
// so we must set deletionScheduledFor via a separate query.
196204
anonymizeUser(user);
197-
userRepository.clearDeletionScheduledFor(user.getId());
205+
userRepository.clearDeletionScheduledFor(userId);
198206

199207
// Delete files after DB operations succeeded
200208
deleteFilePaths(userFilePaths);
@@ -239,8 +247,9 @@ private UserDeletionResultDto performSoftDeletion(User user, List<ThesisRole> re
239247

240248
// Deactivate the account but keep name and thesis-related files intact
241249
// so thesis records remain searchable during the legal retention period.
250+
// Note: anonymizedAt is NOT set here because the user is not fully anonymized
251+
// (name, matriculation number, and thesis files are preserved for retention).
242252
user.setDisabled(true);
243-
user.setAnonymizedAt(now);
244253
user.setDeletionRequestedAt(now);
245254
user.setDeletionScheduledFor(earliestDeletion);
246255

0 commit comments

Comments
 (0)