Skip to content

Commit 34632cb

Browse files
lapenna-bjsslapenna-bjss
authored
CCM-15801: Reject duplicate trust requests (#298)
* CCM-15801: Reject duplicate trust requests * CCM-15801: Fix linting issues * CCM-15801: Update handler * CCM-15801: Update test timeouts * CCM-15801: Update test timeouts * CCM-15801: add queue purge in beforeAll for component tests * CCM-15801: Update test timeouts * CCM-15801: Resolve comments
1 parent d596d08 commit 34632cb

13 files changed

Lines changed: 366 additions & 101 deletions

File tree

‎infrastructure/terraform/components/dl/module_lambda_mesh_download.tf‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -37,16 +37,17 @@ module "mesh_download" {
3737
log_subscription_role_arn = local.acct.log_subscription_role_arn
3838

3939
lambda_env_vars = {
40-
DOWNLOAD_METRIC_NAME = "mesh-download-successful-downloads"
41-
DUPLICATE_DOWNLOAD_METRIC_NAME = "mesh-duplicate-downloads"
42-
DOWNLOAD_METRIC_NAMESPACE = "dl-mesh-download"
43-
ENVIRONMENT = var.environment
44-
EVENT_PUBLISHER_DLQ_URL = module.sqs_event_publisher_errors.sqs_queue_url
45-
EVENT_PUBLISHER_EVENT_BUS_ARN = aws_cloudwatch_event_bus.main.arn
46-
PII_BUCKET = module.s3bucket_pii_data.bucket
47-
SSM_MESH_PREFIX = local.ssm_mesh_prefix
48-
SSM_SENDERS_PREFIX = local.ssm_senders_prefix
49-
USE_MESH_MOCK = var.enable_mock_mesh ? "true" : "false"
40+
DOWNLOAD_METRIC_NAME = "mesh-download-successful-downloads"
41+
INTERNAL_DUPLICATE_DOWNLOAD_METRIC_NAME = "mesh-internal-duplicate-downloads"
42+
TRUST_DUPLICATE_DOWNLOAD_METRIC_NAME = "mesh-trust-duplicate-downloads"
43+
DOWNLOAD_METRIC_NAMESPACE = "dl-mesh-download"
44+
ENVIRONMENT = var.environment
45+
EVENT_PUBLISHER_DLQ_URL = module.sqs_event_publisher_errors.sqs_queue_url
46+
EVENT_PUBLISHER_EVENT_BUS_ARN = aws_cloudwatch_event_bus.main.arn
47+
PII_BUCKET = module.s3bucket_pii_data.bucket
48+
SSM_MESH_PREFIX = local.ssm_mesh_prefix
49+
SSM_SENDERS_PREFIX = local.ssm_senders_prefix
50+
USE_MESH_MOCK = var.enable_mock_mesh ? "true" : "false"
5051
}
5152

5253
}

‎lambdas/mesh-download/mesh_download/__tests__/test_document_store.py‎

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,12 @@
22
import pytest
33
from unittest.mock import Mock
44
from botocore.exceptions import ClientError
5-
from mesh_download.document_store import DocumentStore, IntermediaryBodyStoreError, DocumentAlreadyExistsError
5+
from mesh_download.document_store import (
6+
DocumentStore,
7+
IntermediaryBodyStoreError,
8+
DocumentAlreadyExistsError,
9+
DocumentAlreadyExistsInternalRetryError,
10+
)
611

712

813
def make_client_error(code):
@@ -36,11 +41,12 @@ def test_store_document_success(self):
3641
content=b'test content'
3742
)
3843

39-
assert result == 'document-reference/SENDER-001/ref-123_mesh-456'
44+
assert result == 'document-reference/SENDER-001/ref-123'
4045
mock_s3_client.put_object.assert_called_once_with(
4146
Bucket='test-pii-bucket',
42-
Key='document-reference/SENDER-001/ref-123_mesh-456',
47+
Key='document-reference/SENDER-001/ref-123',
4348
Body=b'test content',
49+
Metadata={'mesh_message_id': 'mesh-456'},
4450
IfNoneMatch='*'
4551
)
4652

@@ -84,21 +90,56 @@ def test_store_document_raises_error_on_non_200_response(self):
8490
content=b'test content'
8591
)
8692

87-
def test_store_document_precondition_failed_raises_document_already_exists(self):
88-
"""Raises DocumentAlreadyExistsError when S3 returns PreconditionFailed (object already exists)"""
93+
def test_store_document_precondition_failed_same_mesh_message_id_raises_internal_retry(self):
94+
"""Raises DocumentAlreadyExistsInternalRetryError when stored meshMessageId matches incoming (internal retry)"""
8995
mock_s3_client = Mock()
9096
mock_s3_client.put_object.side_effect = make_client_error('PreconditionFailed')
97+
mock_s3_client.head_object.return_value = {
98+
'Metadata': {'mesh_message_id': 'mesh-456'}
99+
}
91100

92101
config = Mock()
93102
config.s3_client = mock_s3_client
94103
config.transactional_data_bucket = 'test-pii-bucket'
95104

96105
store = DocumentStore(config)
97106

98-
with pytest.raises(DocumentAlreadyExistsError, match='document-reference/SENDER-001/ref-123_mesh-456'):
107+
with pytest.raises(DocumentAlreadyExistsInternalRetryError, match='document-reference/SENDER-001/ref-123'):
99108
store.store_document(
100109
sender_id='SENDER-001',
101110
message_reference='ref-123',
102111
mesh_message_id='mesh-456',
103112
content=b'test content'
104113
)
114+
115+
mock_s3_client.head_object.assert_called_once_with(
116+
Bucket='test-pii-bucket',
117+
Key='document-reference/SENDER-001/ref-123'
118+
)
119+
120+
def test_store_document_precondition_failed_different_mesh_message_id_raises_trust_duplicate(self):
121+
"""Raises DocumentAlreadyExistsError when stored meshMessageId differs from incoming (trust duplicate)"""
122+
mock_s3_client = Mock()
123+
mock_s3_client.put_object.side_effect = make_client_error('PreconditionFailed')
124+
mock_s3_client.head_object.return_value = {
125+
'Metadata': {'mesh_message_id': 'original-mesh-id'}
126+
}
127+
128+
config = Mock()
129+
config.s3_client = mock_s3_client
130+
config.transactional_data_bucket = 'test-pii-bucket'
131+
132+
store = DocumentStore(config)
133+
134+
with pytest.raises(DocumentAlreadyExistsError, match='document-reference/SENDER-001/ref-123'):
135+
store.store_document(
136+
sender_id='SENDER-001',
137+
message_reference='ref-123',
138+
mesh_message_id='new-mesh-id',
139+
content=b'test content'
140+
)
141+
142+
mock_s3_client.head_object.assert_called_once_with(
143+
Bucket='test-pii-bucket',
144+
Key='document-reference/SENDER-001/ref-123'
145+
)

‎lambdas/mesh-download/mesh_download/__tests__/test_processor.py‎

Lines changed: 89 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
from datetime import datetime, timezone
1010
from pydantic import ValidationError
1111
from mesh_download.errors import MeshMessageNotFound
12-
from mesh_download.document_store import DocumentAlreadyExistsError
12+
from mesh_download.document_store import DocumentAlreadyExistsError, DocumentAlreadyExistsInternalRetryError
1313

1414

1515
def setup_mocks():
@@ -20,7 +20,8 @@ def setup_mocks():
2020
# Set up default config attributes
2121
config.mesh_client = Mock()
2222
config.download_metric = Mock()
23-
config.duplicate_download_metric = Mock()
23+
config.internal_duplicate_download_metric = Mock()
24+
config.trust_duplicate_download_metric = Mock()
2425
config.s3_client = Mock()
2526
config.environment = 'development'
2627
config.transactional_data_bucket = 'test-pii-bucket'
@@ -157,7 +158,8 @@ def test_processor_initialization_calls_mesh_handshake(self):
157158
log=log,
158159
mesh_client=config.mesh_client,
159160
download_metric=config.download_metric,
160-
duplicate_download_metric=config.duplicate_download_metric,
161+
internal_duplicate_download_metric=config.internal_duplicate_download_metric,
162+
trust_duplicate_download_metric=config.trust_duplicate_download_metric,
161163
document_store=document_store,
162164
event_publisher=event_publisher
163165
)
@@ -183,7 +185,8 @@ def test_process_sqs_message_success(self, mock_datetime):
183185
log=log,
184186
mesh_client=config.mesh_client,
185187
download_metric=config.download_metric,
186-
duplicate_download_metric=config.duplicate_download_metric,
188+
internal_duplicate_download_metric=config.internal_duplicate_download_metric,
189+
trust_duplicate_download_metric=config.trust_duplicate_download_metric,
187190
document_store=document_store,
188191
event_publisher=event_publisher
189192
)
@@ -260,7 +263,8 @@ def test_process_sqs_message_invalid_fhir_content(self, mock_datetime):
260263
log=log,
261264
mesh_client=config.mesh_client,
262265
download_metric=config.download_metric,
263-
duplicate_download_metric=config.duplicate_download_metric,
266+
internal_duplicate_download_metric=config.internal_duplicate_download_metric,
267+
trust_duplicate_download_metric=config.trust_duplicate_download_metric,
264268
document_store=document_store,
265269
event_publisher=event_publisher
266270
)
@@ -326,7 +330,8 @@ def test_process_sqs_message_validation_failure(self):
326330
log=log,
327331
mesh_client=config.mesh_client,
328332
download_metric=config.download_metric,
329-
duplicate_download_metric=config.duplicate_download_metric,
333+
internal_duplicate_download_metric=config.internal_duplicate_download_metric,
334+
trust_duplicate_download_metric=config.trust_duplicate_download_metric,
330335
document_store=document_store,
331336
event_publisher=event_publisher
332337
)
@@ -351,7 +356,8 @@ def test_process_sqs_message_missing_mesh_message_id(self):
351356
log=log,
352357
mesh_client=config.mesh_client,
353358
download_metric=config.download_metric,
354-
duplicate_download_metric=config.duplicate_download_metric,
359+
internal_duplicate_download_metric=config.internal_duplicate_download_metric,
360+
trust_duplicate_download_metric=config.trust_duplicate_download_metric,
355361
document_store=document_store,
356362
event_publisher=event_publisher
357363
)
@@ -379,7 +385,8 @@ def test_download_and_store_message_not_found(self):
379385
log=log,
380386
mesh_client=config.mesh_client,
381387
download_metric=config.download_metric,
382-
duplicate_download_metric=config.duplicate_download_metric,
388+
internal_duplicate_download_metric=config.internal_duplicate_download_metric,
389+
trust_duplicate_download_metric=config.trust_duplicate_download_metric,
383390
document_store=document_store,
384391
event_publisher=event_publisher
385392
)
@@ -410,7 +417,8 @@ def test_document_store_failure_prevents_ack_and_raises(self):
410417
log=log,
411418
mesh_client=config.mesh_client,
412419
download_metric=config.download_metric,
413-
duplicate_download_metric=config.duplicate_download_metric,
420+
internal_duplicate_download_metric=config.internal_duplicate_download_metric,
421+
trust_duplicate_download_metric=config.trust_duplicate_download_metric,
414422
document_store=document_store,
415423
event_publisher=event_publisher
416424
)
@@ -446,7 +454,8 @@ def test_bucket_selection_with_mesh_mock_enabled(self, mock_datetime):
446454
log=log,
447455
mesh_client=config.mesh_client,
448456
download_metric=config.download_metric,
449-
duplicate_download_metric=config.duplicate_download_metric,
457+
internal_duplicate_download_metric=config.internal_duplicate_download_metric,
458+
trust_duplicate_download_metric=config.trust_duplicate_download_metric,
450459
document_store=document_store,
451460
event_publisher=event_publisher
452461
)
@@ -486,7 +495,8 @@ def test_bucket_selection_with_mesh_mock_disabled(self, mock_datetime):
486495
log=log,
487496
mesh_client=config.mesh_client,
488497
download_metric=config.download_metric,
489-
duplicate_download_metric=config.duplicate_download_metric,
498+
internal_duplicate_download_metric=config.internal_duplicate_download_metric,
499+
trust_duplicate_download_metric=config.trust_duplicate_download_metric,
490500
document_store=document_store,
491501
event_publisher=event_publisher
492502
)
@@ -505,23 +515,27 @@ def test_bucket_selection_with_mesh_mock_disabled(self, mock_datetime):
505515
assert message_uri.startswith('s3://test-pii-bucket/')
506516

507517
def test_duplicate_delivery_skips_publish_and_acknowledge(self):
508-
"""When S3 signals the object already exists, processor logs a warning, skips publishing and metric, but still acknowledges"""
518+
"""
519+
Internal retry: the object already exists with the same meshMessageId.
520+
Skip publish, record internal metric, acknowledge
521+
"""
509522
from mesh_download.processor import MeshDownloadProcessor
510523

511524
config, log, event_publisher, document_store = setup_mocks()
512525
bound_logger = Mock()
513526
log.bind.return_value = bound_logger
514527

515-
document_store.store_document.side_effect = DocumentAlreadyExistsError(
516-
"Document already exists for key: document-reference/TEST-SENDER/ref-001_mesh-123"
528+
document_store.store_document.side_effect = DocumentAlreadyExistsInternalRetryError(
529+
"Internal retry for key: document-reference/TEST-SENDER/ref-001"
517530
)
518531

519532
processor = MeshDownloadProcessor(
520533
config=config,
521534
log=log,
522535
mesh_client=config.mesh_client,
523536
download_metric=config.download_metric,
524-
duplicate_download_metric=config.duplicate_download_metric,
537+
internal_duplicate_download_metric=config.internal_duplicate_download_metric,
538+
trust_duplicate_download_metric=config.trust_duplicate_download_metric,
525539
document_store=document_store,
526540
event_publisher=event_publisher
527541
)
@@ -530,17 +544,71 @@ def test_duplicate_delivery_skips_publish_and_acknowledge(self):
530544
config.mesh_client.retrieve_message.return_value = mesh_message
531545
sqs_record = create_sqs_record()
532546

533-
# Should complete without raising
534547
outcome = processor.process_sqs_message(sqs_record)
535548

536549
assert outcome == 'skipped'
537550
bound_logger.warning.assert_called_once()
538-
warning_msg = bound_logger.warning.call_args[0][0]
539-
assert "already stored" in warning_msg
540-
config.duplicate_download_metric.record.assert_called_once()
541-
551+
assert "Internal retry" in bound_logger.warning.call_args[0][0]
552+
config.internal_duplicate_download_metric.record.assert_called_once_with(1)
553+
config.trust_duplicate_download_metric.record.assert_not_called()
542554
event_publisher.send_events.assert_not_called()
543555
config.download_metric.record.assert_not_called()
544-
545556
# Acknowledge should still be called to remove message from MESH inbox
546557
mesh_message.acknowledge.assert_called_once()
558+
559+
@patch('mesh_download.processor.datetime')
560+
def test_trust_duplicate_publishes_invalid_event_and_acknowledges(self, mock_datetime):
561+
"""
562+
Trust duplicate: the object already exists with a different meshMessageId.
563+
Publish invalid event with DL_CLIV_004, record trust metric, acknowledge
564+
"""
565+
from mesh_download.processor import MeshDownloadProcessor
566+
567+
config, log, event_publisher, document_store = setup_mocks()
568+
bound_logger = Mock()
569+
log.bind.return_value = bound_logger
570+
571+
fixed_time = datetime(2025, 11, 19, 15, 30, 45, tzinfo=timezone.utc)
572+
mock_datetime.now.return_value = fixed_time
573+
574+
document_store.store_document.side_effect = DocumentAlreadyExistsError(
575+
"Trust duplicate for key: document-reference/TEST-SENDER/ref-001"
576+
)
577+
event_publisher.send_events.return_value = []
578+
579+
processor = MeshDownloadProcessor(
580+
config=config,
581+
log=log,
582+
mesh_client=config.mesh_client,
583+
download_metric=config.download_metric,
584+
internal_duplicate_download_metric=config.internal_duplicate_download_metric,
585+
trust_duplicate_download_metric=config.trust_duplicate_download_metric,
586+
document_store=document_store,
587+
event_publisher=event_publisher
588+
)
589+
590+
mesh_message = create_mesh_message()
591+
config.mesh_client.retrieve_message.return_value = mesh_message
592+
sqs_record = create_sqs_record()
593+
594+
outcome = processor.process_sqs_message(sqs_record)
595+
596+
assert outcome == 'duplicate'
597+
bound_logger.warning.assert_called_once()
598+
assert "Trust duplicate" in bound_logger.warning.call_args[0][0]
599+
config.trust_duplicate_download_metric.record.assert_called_once_with(1)
600+
config.internal_duplicate_download_metric.record.assert_not_called()
601+
config.download_metric.record.assert_not_called()
602+
603+
event_publisher.send_events.assert_called_once()
604+
published_event = event_publisher.send_events.call_args[0][0][0]
605+
assert published_event['type'] == 'uk.nhs.notify.digital.letters.mesh.inbox.message.invalid.v1'
606+
assert published_event['time'] == '2025-11-19T15:30:45+00:00'
607+
608+
event_data = published_event['data']
609+
assert event_data['senderId'] == 'TEST-SENDER'
610+
assert event_data['meshMessageId'] == 'test-message-123'
611+
assert event_data['messageReference'] == 'ref-001'
612+
assert event_data['failureCode'] == 'DL_CLIV_004'
613+
614+
mesh_message.acknowledge.assert_called_once()

‎lambdas/mesh-download/mesh_download/config.py‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@
99
"ssm_mesh_prefix": "SSM_MESH_PREFIX",
1010
"environment": "ENVIRONMENT",
1111
"download_metric_name": "DOWNLOAD_METRIC_NAME",
12-
"duplicate_download_metric_name": "DUPLICATE_DOWNLOAD_METRIC_NAME",
12+
"internal_duplicate_download_metric_name": "INTERNAL_DUPLICATE_DOWNLOAD_METRIC_NAME",
13+
"trust_duplicate_download_metric_name": "TRUST_DUPLICATE_DOWNLOAD_METRIC_NAME",
1314
"download_metric_namespace": "DOWNLOAD_METRIC_NAMESPACE",
1415
"event_publisher_event_bus_arn": "EVENT_PUBLISHER_EVENT_BUS_ARN",
1516
"event_publisher_dlq_url": "EVENT_PUBLISHER_DLQ_URL",
@@ -29,14 +30,16 @@ def __init__(self, ssm=None, s3_client=None):
2930
super().__init__(ssm=ssm, s3_client=s3_client)
3031

3132
self.download_metric = None
32-
self.duplicate_download_metric = None
33+
self.internal_duplicate_download_metric = None
34+
self.trust_duplicate_download_metric = None
3335

3436
def __enter__(self):
3537
super().__enter__()
3638

3739
# Build download metric
3840
self.download_metric = self.build_download_metric()
39-
self.duplicate_download_metric = self.build_duplicate_download_metric()
41+
self.internal_duplicate_download_metric = self.build_internal_duplicate_download_metric()
42+
self.trust_duplicate_download_metric = self.build_trust_duplicate_download_metric()
4043

4144
return self
4245

@@ -50,12 +53,22 @@ def build_download_metric(self):
5053
dimensions={"Environment": self.environment}
5154
)
5255

53-
def build_duplicate_download_metric(self):
56+
def build_internal_duplicate_download_metric(self):
5457
"""
55-
Returns a custom metric to record messages that were attempted to be downloaded more than once
58+
Custom metric for messages retried internally
5659
"""
5760
return Metric(
58-
name=self.duplicate_download_metric_name,
61+
name=self.internal_duplicate_download_metric_name,
62+
namespace=self.download_metric_namespace,
63+
dimensions={"Environment": self.environment}
64+
)
65+
66+
def build_trust_duplicate_download_metric(self):
67+
"""
68+
Custom metric for duplicate messages received from a Trust
69+
"""
70+
return Metric(
71+
name=self.trust_duplicate_download_metric_name,
5972
namespace=self.download_metric_namespace,
6073
dimensions={"Environment": self.environment}
6174
)

0 commit comments

Comments
 (0)