Skip to content

S3 hardening #83

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattknatt merged 9 commits into from
Apr 23, 2026
Merged

S3 hardening #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
36fc43d
S3 hardening
LinusWestling Apr 22, 2026
a6d5018
rabbit feedback fixes
LinusWestling Apr 23, 2026
dd72b77
Fixed test errors and additional rabbit feedback about s3 handling
LinusWestling Apr 23, 2026
d11bf80
Merge branch 'main' into improvement/s3
LinusWestling Apr 23, 2026
be76d35
Align DocumentServiceTest with S3 retry executor flow
LinusWestling Apr 23, 2026
1df33d3
Fixed the bugg that appeared in the demo
LinusWestling Apr 23, 2026
73b951e
fixed app.css
LinusWestling Apr 23, 2026
5bab543
Merge branch 'main' into improvement/s3
mattknatt Apr 23, 2026
df18030
Remove redundant exception handling in S3 upload logic and update rel…
mattknatt Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion AUDIT.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ Table **`audit_events`** (entity `AuditEventEntity`):
`AuditWebMvcConfig` adds `AuditInterceptor` for:

- **Included:** `/ui/**`, `/api/**`
- **Excluded:** `/static/**`, `/app.css`, `/app.js`, `/error**`, `/login**`
- **Excluded:** `/static/**`, `/app.js`, `/error**`, `/login**`

So static assets, error pages, and login flows do not generate audit rows.

Expand Down
4 changes: 4 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@
<groupId>io.awspring.cloud</groupId>
<artifactId>spring-cloud-aws-starter-s3</artifactId>
</dependency>
<dependency>
<groupId>org.springframework.retry</groupId>
<artifactId>spring-retry</artifactId>
</dependency>
</dependencies>

<build>
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/org/example/projektarendehantering/ProjektArendehanteringApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.scheduling.annotation.EnableScheduling;

@SpringBootApplication
@EnableScheduling
public class ProjektArendehanteringApplication {

public static void main(String[] args) {
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/example/projektarendehantering/application/service/AuditService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.springframework.data.domain.Page;
import org.springframework.data.domain.Pageable;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Propagation;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;
Expand Down Expand Up @@ -67,7 +68,7 @@ public class AuditService {
"refresh_token"
);

@Transactional
@Transactional(propagation = Propagation.REQUIRES_NEW)
public void record(AuditEventEntity event) {
if (event == null) return;
if (event.getId() == null) {
Expand Down
95 changes: 38 additions & 57 deletions src/main/java/org/example/projektarendehantering/application/service/DocumentService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@
import org.springframework.beans.factory.annotation.Value;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;
import org.springframework.web.multipart.MultipartFile;

import org.apache.tika.Tika;
Expand All @@ -34,6 +32,8 @@ public class DocumentService {
private final DocumentRepository documentRepository;
private final CaseRepository caseRepository;
private final S3Template s3Template;
private final S3RetryExecutor s3RetryExecutor;
private final FailedS3DeletionService failedS3DeletionService;
private final DocumentMapper documentMapper;
private final AuditService auditService;

Expand Down Expand Up @@ -94,31 +94,14 @@ public DocumentDTO uploadDocument(Actor actor, UUID caseId, MultipartFile file)

String s3Key = UUID.randomUUID() + "-" + originalFilename;

try {
ObjectMetadata metadata = ObjectMetadata.builder()
.contentType(file.getContentType())
.build();
s3Template.upload(bucket, s3Key, new ByteArrayInputStream(fileBytes), metadata);
} catch (Exception e) {
log.error("S3 upload failed for bucket: {}, key: {}. Error: {}", bucket, s3Key, e.getMessage(), e);
throw new AppException("S3_UPLOAD_FAILED", "Failed to upload file to S3");
}
ObjectMetadata metadata = ObjectMetadata.builder()
.contentType(contentType)
.build();
s3RetryExecutor.execute("upload", context -> {
s3Template.upload(bucket, s3Key, new ByteArrayInputStream(fileBytes), metadata);
return null;
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

boolean syncActive = TransactionSynchronizationManager.isSynchronizationActive();
if (syncActive) {
TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
@Override
public void afterCompletion(int status) {
if (status != TransactionSynchronization.STATUS_COMMITTED) {
try {
s3Template.deleteObject(bucket, s3Key);
} catch (Exception cleanupEx) {
log.error("Failed to cleanup rolled-back S3 object {}", s3Key, cleanupEx);
}
}
}
});
}

DocumentEntity entity = DocumentEntity.builder()
.fileName(originalFilename)
Expand Down Expand Up @@ -161,12 +144,16 @@ public void afterCompletion(int status) {
}
return documentMapper.toDTO(saved);
} catch (RuntimeException ex) {
if (!syncActive) {
try {
try {
s3RetryExecutor.execute("delete", context -> {
s3Template.deleteObject(bucket, s3Key);
} catch (Exception cleanupEx) {
log.error("Failed to cleanup orphaned S3 object {}", s3Key, cleanupEx);
}
return null;
});
} catch (Exception cleanupEx) {
log.error("Failed to cleanup orphaned S3 object {}", s3Key, cleanupEx);
failedS3DeletionService.enqueue(bucket, s3Key, cleanupEx);
recordS3Audit(actor, caseEntity.getId(), "DOCUMENT_UPLOAD_COMPENSATION_QUEUED",
"Queued failed upload compensation cleanup for retry", s3Key);
}
throw ex;
}
Comment thread
LinusWestling marked this conversation as resolved.
Expand Down Expand Up @@ -195,7 +182,7 @@ public S3Resource downloadDocument(Actor actor, UUID documentId) {

validateAccess(actor, entity.getCaseEntity());

return s3Template.download(bucket, entity.getS3Key());
return s3RetryExecutor.execute("download", context -> s3Template.download(bucket, entity.getS3Key()));
}

@Transactional
Expand All @@ -208,37 +195,31 @@ public void deleteDocument(Actor actor, UUID documentId) {

validateAccess(actor, entity.getCaseEntity());

// Save S3 key before deletion
String s3Key = entity.getS3Key();

// 1. Delete DB entity inside the transaction
documentRepository.delete(entity);

// 2. Delete S3 object *after* successful commit (if in a transaction)
if (TransactionSynchronizationManager.isSynchronizationActive()) {
TransactionSynchronizationManager.registerSynchronization(
new TransactionSynchronization() {
@Override
public void afterCommit() {
try {
s3Template.deleteObject(bucket, s3Key);
} catch (Exception e) {
// DB is already committed — log but do not throw
log.error("Failed to delete S3 object {} after DB commit", s3Key, e);
}
}
}
);
} else {
// No active transaction (e.g. in unit test)
try {
try {
s3RetryExecutor.execute("delete", context -> {
s3Template.deleteObject(bucket, s3Key);
} catch (Exception e) {
log.error("Failed to delete S3 object {} (no active transaction)", s3Key, e);
}
return null;
});
} catch (Exception e) {
log.error("Failed to delete S3 object {} after DB delete", s3Key, e);
failedS3DeletionService.enqueue(bucket, s3Key, e);
}
}

private void recordS3Audit(Actor actor, UUID caseId, String eventName, String description, String s3Key) {
auditService.record(AuditEventEntity.builder()
.caseId(caseId)
.eventName(eventName)
.description(description)
.actorId(actor != null ? actor.userId() : null)
.actorRole(actor != null && actor.role() != null ? actor.role().name() : null)
.queryString("bucket=" + bucket + "&s3Key=" + s3Key)
.occurredAt(Instant.now())
.build());
}


@Transactional(readOnly = true)
public DocumentEntity getEntity(Actor actor, UUID documentId) {
Expand Down
10 changes: 6 additions & 4 deletions src/main/java/org/example/projektarendehantering/application/service/EmployeeMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import org.example.projektarendehantering.presentation.dto.EmployeeDTO;
import org.example.projektarendehantering.presentation.dto.EmployeeUpdateDTO;
import org.springframework.stereotype.Component;
import org.example.projektarendehantering.common.GithubUsernameNormalizer;

import java.nio.charset.StandardCharsets;
import java.util.UUID;
Expand All @@ -25,23 +26,24 @@ public EmployeeDTO toDTO(EmployeeEntity entity) {

public EmployeeEntity toEntity(EmployeeCreateDTO dto) {
if (dto == null) return null;
String normalizedGithubUsername = GithubUsernameNormalizer.normalize(dto.getGithubUsername());
UUID id = null;
if (dto.getGithubUsername() != null && !dto.getGithubUsername().isBlank()) {
id = UUID.nameUUIDFromBytes(dto.getGithubUsername().getBytes(StandardCharsets.UTF_8));
if (normalizedGithubUsername != null) {
id = UUID.nameUUIDFromBytes(normalizedGithubUsername.getBytes(StandardCharsets.UTF_8));
}

return EmployeeEntity.builder()
.id(id)
.displayName(dto.getDisplayName())
.githubUsername(dto.getGithubUsername())
.githubUsername(normalizedGithubUsername)
.role(dto.getRole())
.build();
}

public void updateEntity(EmployeeUpdateDTO dto, EmployeeEntity entity) {
if (dto == null || entity == null) return;
entity.setDisplayName(dto.getDisplayName());
entity.setGithubUsername(dto.getGithubUsername());
entity.setGithubUsername(GithubUsernameNormalizer.normalize(dto.getGithubUsername()));
entity.setRole(dto.getRole());
}
}
Expand Down
16 changes: 13 additions & 3 deletions src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import org.example.projektarendehantering.common.Actor;
import org.example.projektarendehantering.common.BadRequestException;
import org.example.projektarendehantering.common.GithubUsernameNormalizer;
import org.example.projektarendehantering.common.NotAuthorizedException;
import org.example.projektarendehantering.common.Role;
import org.example.projektarendehantering.infrastructure.persistence.EmployeeEntity;
Expand All @@ -15,6 +16,7 @@

import java.time.Instant;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.UUID;
import java.util.stream.Collectors;
Expand All @@ -29,9 +31,15 @@ public class EmployeeService {
@Transactional
public EmployeeDTO createEmployee(Actor actor, EmployeeCreateDTO dto) {
requireCanManageEmployees(actor);
String normalizedGithubUsername = GithubUsernameNormalizer.normalize(dto.getGithubUsername());
dto.setGithubUsername(normalizedGithubUsername);

if (employeeRepository.findByGithubUsername(dto.getGithubUsername()).isPresent()) {
throw new BadRequestException("EMPLOYEE_EXISTS", "Employee with username " + dto.getGithubUsername() + " already exists");
if (normalizedGithubUsername == null) {
throw new BadRequestException("EMPLOYEE_USERNAME_REQUIRED", "Github username is required");
}

if (employeeRepository.findByGithubUsername(normalizedGithubUsername).isPresent()) {
throw new BadRequestException("EMPLOYEE_EXISTS", "Employee with username " + normalizedGithubUsername + " already exists");
}

EmployeeEntity entity = employeeMapper.toEntity(dto);
Expand All @@ -48,10 +56,12 @@ public EmployeeDTO updateEmployee(Actor actor, UUID id, EmployeeUpdateDTO dto) {
EmployeeEntity entity = employeeRepository.findById(id)
.orElseThrow(() -> new BadRequestException("EMPLOYEE_NOT_FOUND", "Employee not found"));

if (!entity.getGithubUsername().equals(dto.getGithubUsername())) {
String normalizedGithubUsername = GithubUsernameNormalizer.normalize(dto.getGithubUsername());
if (!Objects.equals(entity.getGithubUsername(), normalizedGithubUsername)) {
throw new BadRequestException("EMPLOYEE_USERNAME_IMMUTABLE", "Github username cannot be changed");
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

dto.setGithubUsername(normalizedGithubUsername);
employeeMapper.updateEntity(dto, entity);
return employeeMapper.toDTO(employeeRepository.save(entity));
}
Expand Down
Loading