Skip to content

Commit 951103f

Browse files
SchmarviniusSchmarvinius
authored
Add scan check upon download (#762)
1 parent 15e5527 commit 951103f

10 files changed

Lines changed: 220 additions & 21 deletions

File tree

‎cds-feature-attachments/src/main/java/com/sap/cds/feature/attachments/configuration/Registration.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ public void eventHandlers(CdsRuntimeConfigurer configurer) {
133133
new EndTransactionMalwareScanRunner(null, null, malwareScanner, runtime);
134134
configurer.eventHandler(
135135
new ReadAttachmentsHandler(
136-
attachmentService, new AttachmentStatusValidator(), scanRunner));
136+
attachmentService, new AttachmentStatusValidator(), scanRunner, persistenceService));
137137
} else {
138138
logger.debug(
139139
"No application service is available. Application service event handlers will not be registered.");

‎cds-feature-attachments/src/main/java/com/sap/cds/feature/attachments/handler/applicationservice/ReadAttachmentsHandler.java‎

Lines changed: 64 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@
1818
import com.sap.cds.feature.attachments.service.AttachmentService;
1919
import com.sap.cds.feature.attachments.service.malware.AsyncMalwareScanExecutor;
2020
import com.sap.cds.ql.CQL;
21+
import com.sap.cds.ql.Update;
2122
import com.sap.cds.ql.cqn.CqnSelect;
23+
import com.sap.cds.ql.cqn.CqnUpdate;
2224
import com.sap.cds.ql.cqn.Path;
2325
import com.sap.cds.reflect.CdsAssociationType;
2426
import com.sap.cds.reflect.CdsElementDefinition;
@@ -32,7 +34,10 @@
3234
import com.sap.cds.services.handler.annotations.Before;
3335
import com.sap.cds.services.handler.annotations.HandlerOrder;
3436
import com.sap.cds.services.handler.annotations.ServiceName;
37+
import com.sap.cds.services.persistence.PersistenceService;
3538
import java.io.InputStream;
39+
import java.time.Duration;
40+
import java.time.Instant;
3641
import java.util.ArrayList;
3742
import java.util.List;
3843
import java.util.Map;
@@ -45,29 +50,45 @@
4550

4651
/**
4752
* The class {@link ReadAttachmentsHandler} is an event handler that is responsible for reading
48-
* attachments for entities. In the before read event, it modifies the CQN to include the content ID
49-
* and status. In the after read event, it adds a proxy for the stream of the attachments service to
50-
* the data. Only if the data are read the proxy forwards the request to the attachment service to
51-
* read the attachment. This is needed to have a filled stream in the data to enable the OData V4
52-
* adapter to enrich the data that a link to the content can be shown on the UI.
53+
* attachments for entities. In the before read event, it modifies the CQN to include the content
54+
* ID, status and scanned-at timestamp. In the after read event, it adds a proxy for the stream of
55+
* the attachments service to the data. Only if the data are read the proxy forwards the request to
56+
* the attachment service to read the attachment. This is needed to have a filled stream in the data
57+
* to enable the OData V4 adapter to enrich the data that a link to the content can be shown on the
58+
* UI.
59+
*
60+
* <p>Additionally, this handler implements rescan-on-download: if an attachment's last scan is
61+
* older than {@link #RESCAN_THRESHOLD}, the handler transitions the attachment status to {@code
62+
* SCANNING}, triggers an asynchronous malware rescan, and rejects the current download with a "not
63+
* scanned" error. The client must retry the download after the rescan completes.
5364
*/
5465
@ServiceName(value = "*", type = ApplicationService.class)
5566
public class ReadAttachmentsHandler implements EventHandler {
5667

5768
private static final Logger logger = LoggerFactory.getLogger(ReadAttachmentsHandler.class);
5869

70+
/**
71+
* The duration after which a previously scanned attachment is considered stale and must be
72+
* rescanned on download, as recommended by the SAP Malware Scanning Service FAQ.
73+
*/
74+
static final Duration RESCAN_THRESHOLD = Duration.ofDays(3);
75+
5976
private final AttachmentService attachmentService;
6077
private final AttachmentStatusValidator statusValidator;
6178
private final AsyncMalwareScanExecutor scanExecutor;
79+
private final PersistenceService persistenceService;
6280

6381
public ReadAttachmentsHandler(
6482
AttachmentService attachmentService,
6583
AttachmentStatusValidator statusValidator,
66-
AsyncMalwareScanExecutor scanExecutor) {
84+
AsyncMalwareScanExecutor scanExecutor,
85+
PersistenceService persistenceService) {
6786
this.attachmentService =
6887
requireNonNull(attachmentService, "attachmentService must not be null");
6988
this.statusValidator = requireNonNull(statusValidator, "statusValidator must not be null");
7089
this.scanExecutor = requireNonNull(scanExecutor, "scanExecutor must not be null");
90+
this.persistenceService =
91+
requireNonNull(persistenceService, "persistenceService must not be null");
7192
}
7293

7394
@Before
@@ -153,9 +174,10 @@ private void verifyStatus(Path path, Attachments attachment) {
153174
"In verify status for content id {} and status {}",
154175
attachment.getContentId(),
155176
currentStatus);
156-
if (StatusCode.UNSCANNED.equals(currentStatus)
157-
|| StatusCode.SCANNING.equals(currentStatus)
158-
|| currentStatus == null) {
177+
if (needsScan(currentStatus, attachment.getScannedAt())) {
178+
if (StatusCode.CLEAN.equals(currentStatus)) {
179+
transitionToScanning(path.target().entity(), attachment);
180+
}
159181
logger.debug(
160182
"Scanning content with ID {} for malware, has current status {}",
161183
attachment.getContentId(),
@@ -166,6 +188,39 @@ private void verifyStatus(Path path, Attachments attachment) {
166188
}
167189
}
168190

191+
private boolean needsScan(String status, Instant scannedAt) {
192+
if (StatusCode.UNSCANNED.equals(status)
193+
|| StatusCode.SCANNING.equals(status)
194+
|| status == null) {
195+
return true;
196+
}
197+
return StatusCode.CLEAN.equals(status) && isScanStale(scannedAt);
198+
}
199+
200+
private boolean isScanStale(Instant scannedAt) {
201+
return scannedAt == null || Instant.now().isAfter(scannedAt.plus(RESCAN_THRESHOLD));
202+
}
203+
204+
private void transitionToScanning(CdsEntity entity, Attachments attachment) {
205+
logger.debug(
206+
"Attachment {} has stale scan (scannedAt={}), transitioning to SCANNING for rescan.",
207+
attachment.getContentId(),
208+
attachment.getScannedAt());
209+
210+
Attachments updateData = Attachments.create();
211+
updateData.setStatus(StatusCode.SCANNING);
212+
213+
// Filter by contentId because primary keys are unavailable during content-only reads
214+
// (areKeysEmpty returns true). This is consistent with DefaultAttachmentMalwareScanner.
215+
CqnUpdate update =
216+
Update.entity(entity)
217+
.data(updateData)
218+
.where(entry -> entry.get(Attachments.CONTENT_ID).eq(attachment.getContentId()));
219+
persistenceService.run(update);
220+
221+
attachment.setStatus(StatusCode.SCANNING);
222+
}
223+
169224
private boolean areKeysEmpty(Map<String, Object> keys) {
170225
return keys.values().stream().allMatch(Objects::isNull);
171226
}

‎cds-feature-attachments/src/main/java/com/sap/cds/feature/attachments/handler/applicationservice/modifyevents/CreateAttachmentEvent.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,9 @@ public InputStream processEvent(
6666
eventContext.getChangeSetContext().register(createListener);
6767
path.target().values().put(Attachments.CONTENT_ID, result.contentId());
6868
path.target().values().put(Attachments.STATUS, result.status());
69+
if (nonNull(result.scannedAt())) {
70+
path.target().values().put(Attachments.SCANNED_AT, result.scannedAt());
71+
}
6972
return result.isInternalStored() ? content : null;
7073
}
7174

‎cds-feature-attachments/src/main/java/com/sap/cds/feature/attachments/handler/applicationservice/readhelper/BeforeReadItemsModifier.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@
1515
import org.slf4j.LoggerFactory;
1616

1717
/**
18-
* The class {@link BeforeReadItemsModifier} is a modifier that adds the content id field and status
19-
* code to the select items.
18+
* The class {@link BeforeReadItemsModifier} is a modifier that adds the content id field, status
19+
* code and scanned-at timestamp to the select items.
2020
*/
2121
public class BeforeReadItemsModifier implements Modifier {
2222

@@ -74,9 +74,10 @@ private List<CqnSelectListItem> processExpandedEntities(List<CqnSelectListItem>
7474
private void enhanceWithNewFieldForMediaAssociation(
7575
String association, List<CqnSelectListItem> list, List<CqnSelectListItem> listToEnhance) {
7676
if (isMediaAssociationAndNeedNewContentIdField(association, list)) {
77-
logger.debug("Adding document id and status code to select items");
77+
logger.debug("Adding document id, status code and scanned-at timestamp to select items");
7878
listToEnhance.add(CQL.get(Attachments.CONTENT_ID));
7979
listToEnhance.add(CQL.get(Attachments.STATUS));
80+
listToEnhance.add(CQL.get(Attachments.SCANNED_AT));
8081
}
8182
}
8283

‎cds-feature-attachments/src/main/java/com/sap/cds/feature/attachments/service/AttachmentsServiceImpl.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,8 @@ public AttachmentModificationResult createAttachment(CreateAttachmentInput input
5656
return new AttachmentModificationResult(
5757
Boolean.TRUE.equals(createContext.getIsInternalStored()),
5858
createContext.getContentId(),
59-
createContext.getData().getStatus());
59+
createContext.getData().getStatus(),
60+
createContext.getData().getScannedAt());
6061
}
6162

6263
@Override

‎cds-feature-attachments/src/main/java/com/sap/cds/feature/attachments/service/model/service/AttachmentModificationResult.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,15 @@
33
*/
44
package com.sap.cds.feature.attachments.service.model.service;
55

6+
import java.time.Instant;
7+
68
/**
79
* This record is used to store the result of the attachment modification.
810
*
911
* @param isInternalStored Indicates if the attachment is stored internally (in DB) or externally.
1012
* @param contentId The content id of the attachment.
1113
* @param status The status of the attachment.
14+
* @param scannedAt The timestamp when the attachment was last scanned for malware.
1215
*/
1316
public record AttachmentModificationResult(
14-
boolean isInternalStored, String contentId, String status) {}
17+
boolean isInternalStored, String contentId, String status, Instant scannedAt) {}

‎cds-feature-attachments/src/test/java/com/sap/cds/feature/attachments/handler/applicationservice/ReadAttachmentsHandlerTest.java‎

Lines changed: 102 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,14 @@
3636
import com.sap.cds.services.handler.annotations.Before;
3737
import com.sap.cds.services.handler.annotations.HandlerOrder;
3838
import com.sap.cds.services.handler.annotations.ServiceName;
39+
import com.sap.cds.services.persistence.PersistenceService;
3940
import com.sap.cds.services.runtime.CdsRuntime;
4041
import java.io.ByteArrayInputStream;
4142
import java.io.IOException;
4243
import java.io.InputStream;
4344
import java.nio.charset.StandardCharsets;
45+
import java.time.Instant;
46+
import java.time.temporal.ChronoUnit;
4447
import java.util.List;
4548
import java.util.UUID;
4649
import org.junit.jupiter.api.BeforeAll;
@@ -61,6 +64,7 @@ class ReadAttachmentsHandlerTest {
6164
private AttachmentStatusValidator attachmentStatusValidator;
6265
private CdsReadEventContext readEventContext;
6366
private AsyncMalwareScanExecutor asyncMalwareScanExecutor;
67+
private PersistenceService persistenceService;
6468

6569
@BeforeAll
6670
static void classSetup() {
@@ -72,9 +76,13 @@ void setup() {
7276
attachmentService = mock(AttachmentService.class);
7377
attachmentStatusValidator = mock(AttachmentStatusValidator.class);
7478
asyncMalwareScanExecutor = mock(AsyncMalwareScanExecutor.class);
79+
persistenceService = mock(PersistenceService.class);
7580
cut =
7681
new ReadAttachmentsHandler(
77-
attachmentService, attachmentStatusValidator, asyncMalwareScanExecutor);
82+
attachmentService,
83+
attachmentStatusValidator,
84+
asyncMalwareScanExecutor,
85+
persistenceService);
7886

7987
readEventContext = mock(CdsReadEventContext.class);
8088
}
@@ -164,6 +172,7 @@ void setAttachmentServiceCalled() throws IOException {
164172
attachment.setContentId("some ID");
165173
attachment.setContent(null);
166174
attachment.setStatus(StatusCode.CLEAN);
175+
attachment.setScannedAt(Instant.now());
167176

168177
cut.processAfter(readEventContext, List.of(attachment));
169178

@@ -253,6 +262,98 @@ void scannerNotCalledForInfectedAttachments() {
253262
verifyNoInteractions(asyncMalwareScanExecutor);
254263
}
255264

265+
@Test
266+
void scannerCalledForStaleCleanAttachment() {
267+
mockEventContext(Attachment_.CDS_NAME, mock(CqnSelect.class));
268+
var attachment = Attachments.create();
269+
attachment.setContentId("some ID");
270+
attachment.setContent(mock(InputStream.class));
271+
attachment.setStatus(StatusCode.CLEAN);
272+
attachment.setScannedAt(Instant.now().minus(4, ChronoUnit.DAYS));
273+
doThrow(AttachmentStatusException.class)
274+
.when(attachmentStatusValidator)
275+
.verifyStatus(StatusCode.SCANNING);
276+
277+
List<CdsData> attachments = List.of(attachment);
278+
assertThrows(
279+
AttachmentStatusException.class, () -> cut.processAfter(readEventContext, attachments));
280+
281+
verify(persistenceService).run(any(com.sap.cds.ql.cqn.CqnUpdate.class));
282+
verify(asyncMalwareScanExecutor)
283+
.scanAsync(readEventContext.getTarget(), attachment.getContentId());
284+
assertThat(attachment.getStatus()).isEqualTo(StatusCode.SCANNING);
285+
}
286+
287+
@Test
288+
void scannerCalledForCleanAttachmentWithNullScannedAt() {
289+
mockEventContext(Attachment_.CDS_NAME, mock(CqnSelect.class));
290+
var attachment = Attachments.create();
291+
attachment.setContentId("some ID");
292+
attachment.setContent(mock(InputStream.class));
293+
attachment.setStatus(StatusCode.CLEAN);
294+
attachment.setScannedAt(null);
295+
doThrow(AttachmentStatusException.class)
296+
.when(attachmentStatusValidator)
297+
.verifyStatus(StatusCode.SCANNING);
298+
299+
List<CdsData> attachments = List.of(attachment);
300+
assertThrows(
301+
AttachmentStatusException.class, () -> cut.processAfter(readEventContext, attachments));
302+
303+
verify(persistenceService).run(any(com.sap.cds.ql.cqn.CqnUpdate.class));
304+
verify(asyncMalwareScanExecutor)
305+
.scanAsync(readEventContext.getTarget(), attachment.getContentId());
306+
assertThat(attachment.getStatus()).isEqualTo(StatusCode.SCANNING);
307+
}
308+
309+
@Test
310+
void scannerNotCalledForFreshCleanAttachment() {
311+
mockEventContext(Attachment_.CDS_NAME, mock(CqnSelect.class));
312+
var attachment = Attachments.create();
313+
attachment.setContentId("some ID");
314+
attachment.setContent(mock(InputStream.class));
315+
attachment.setStatus(StatusCode.CLEAN);
316+
attachment.setScannedAt(Instant.now().minus(1, ChronoUnit.DAYS));
317+
318+
cut.processAfter(readEventContext, List.of(attachment));
319+
320+
verifyNoInteractions(asyncMalwareScanExecutor);
321+
verifyNoInteractions(persistenceService);
322+
assertThat(attachment.getStatus()).isEqualTo(StatusCode.CLEAN);
323+
}
324+
325+
@Test
326+
void scannerNotCalledForCleanAttachmentScannedExactlyAtThreshold() {
327+
mockEventContext(Attachment_.CDS_NAME, mock(CqnSelect.class));
328+
var attachment = Attachments.create();
329+
attachment.setContentId("some ID");
330+
attachment.setContent(mock(InputStream.class));
331+
attachment.setStatus(StatusCode.CLEAN);
332+
attachment.setScannedAt(
333+
Instant.now().minus(ReadAttachmentsHandler.RESCAN_THRESHOLD).plusSeconds(60));
334+
335+
cut.processAfter(readEventContext, List.of(attachment));
336+
337+
verifyNoInteractions(asyncMalwareScanExecutor);
338+
verifyNoInteractions(persistenceService);
339+
}
340+
341+
@Test
342+
void persistenceServiceNotCalledForUnscannedAttachments() {
343+
mockEventContext(Attachment_.CDS_NAME, mock(CqnSelect.class));
344+
var attachment = Attachments.create();
345+
attachment.setContentId("some ID");
346+
attachment.setContent(mock(InputStream.class));
347+
attachment.setStatus(StatusCode.UNSCANNED);
348+
349+
cut.processAfter(readEventContext, List.of(attachment));
350+
351+
verify(asyncMalwareScanExecutor)
352+
.scanAsync(readEventContext.getTarget(), attachment.getContentId());
353+
verify(attachmentStatusValidator).verifyStatus(StatusCode.UNSCANNED);
354+
verifyNoInteractions(persistenceService);
355+
}
356+
256357
@Test
257358
void attachmentServiceNotCalledIfNoMediaType() {
258359
var eventItem = EventItems.create();

‎cds-feature-attachments/src/test/java/com/sap/cds/feature/attachments/handler/applicationservice/modifyevents/CreateAttachmentEventTest.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ void storageCalledWithAllFieldsFilledFromExistingData() {
9292
when(target.values()).thenReturn(attachment);
9393
when(target.keys()).thenReturn(Map.of("ID", attachment.getId(), "up__ID", "test"));
9494
when(attachmentService.createAttachment(any()))
95-
.thenReturn(new AttachmentModificationResult(false, "id", "test"));
95+
.thenReturn(new AttachmentModificationResult(false, "id", "test", null));
9696
var existingData = Attachments.create();
9797
existingData.setFileName("some file name");
9898
existingData.setMimeType("some mime type");
@@ -116,7 +116,7 @@ void resultFromServiceStoredInPath() {
116116
var attachment = Attachments.create();
117117
attachment.setId("test");
118118
var attachmentServiceResult =
119-
new AttachmentModificationResult(false, "some document id", "test");
119+
new AttachmentModificationResult(false, "some document id", "test", null);
120120
when(attachmentService.createAttachment(any())).thenReturn(attachmentServiceResult);
121121
when(target.values()).thenReturn(attachment);
122122

@@ -134,7 +134,7 @@ void changesetIstRegistered() {
134134
var listener = mock(ChangeSetListener.class);
135135
when(listenerProvider.provideListener(contentId, runtime)).thenReturn(listener);
136136
when(attachmentService.createAttachment(any()))
137-
.thenReturn(new AttachmentModificationResult(false, contentId, "test"));
137+
.thenReturn(new AttachmentModificationResult(false, contentId, "test", null));
138138

139139
cut.processEvent(path, null, Attachments.create(), eventContext);
140140

@@ -154,7 +154,7 @@ void contentIsReturnedIfNotExternalStored(boolean isExternalStored) throws IOExc
154154
}
155155
when(target.values()).thenReturn(attachment);
156156
when(attachmentService.createAttachment(any()))
157-
.thenReturn(new AttachmentModificationResult(isExternalStored, "id", "test"));
157+
.thenReturn(new AttachmentModificationResult(isExternalStored, "id", "test", null));
158158

159159
var result =
160160
cut.processEvent(path, attachment.getContent(), Attachments.create(), eventContext);
@@ -174,7 +174,7 @@ private Attachments prepareAndExecuteEventWithData() {
174174
when(target.values()).thenReturn(attachment);
175175
when(target.keys()).thenReturn(Map.of("ID", attachment.getId()));
176176
when(attachmentService.createAttachment(any()))
177-
.thenReturn(new AttachmentModificationResult(false, "id", "test"));
177+
.thenReturn(new AttachmentModificationResult(false, "id", "test", null));
178178

179179
cut.processEvent(path, attachment.getContent(), Attachments.create(), eventContext);
180180
return attachment;

0 commit comments

Comments
 (0)