Skip to content

Commit

Permalink
s3 expires implementation (#3232)
Browse files Browse the repository at this point in the history
* implementation to gracefully handle invalid timestamps returned in the Expires header
  • Loading branch information
alexgromero committed Sep 10, 2024
1 parent 466315c commit 4c2771a
Show file tree
Hide file tree
Showing 8 changed files with 300 additions and 6 deletions.
5 changes: 5 additions & 0 deletions .changes/next-release/enhancement-s3-94466.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"type": "enhancement",
"category": "``s3``",
"description": "Adds logic to gracefully handle invalid timestamps returned in the Expires header."
}
8 changes: 8 additions & 0 deletions botocore/data/s3/2006-03-01/service-2.sdk-extras.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"version": 1.0,
"merge": {
"shapes": {
"Expires":{"type":"timestamp"}
}
}
}
3 changes: 3 additions & 0 deletions botocore/docs/bcdoc/restdoc.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,9 @@ def get_section(self, name):
"""Retrieve a section"""
return self._structure[name]

def has_section(self, name):
return name in self._structure

def delete_section(self, name):
"""Delete a section"""
del self._structure[name]
Expand Down
8 changes: 8 additions & 0 deletions botocore/endpoint.py
Original file line number Diff line number Diff line change
Expand Up @@ -302,10 +302,18 @@ def _do_get_response(self, request, operation_model, context):
history_recorder.record('HTTP_RESPONSE', http_response_record_dict)

protocol = operation_model.metadata['protocol']
customized_response_dict = {}
self._event_emitter.emit(
f"before-parse.{service_id}.{operation_model.name}",
operation_model=operation_model,
response_dict=response_dict,
customized_response_dict=customized_response_dict,
)
parser = self._response_parser_factory.create_parser(protocol)
parsed_response = parser.parse(
response_dict, operation_model.output_shape
)
parsed_response.update(customized_response_dict)
# Do a second parsing pass to pick up on any modeled error fields
# NOTE: Ideally, we would push this down into the parser classes but
# they currently have no reference to the operation or service model
Expand Down
66 changes: 66 additions & 0 deletions botocore/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -1176,6 +1176,69 @@ def remove_content_type_header_for_presigning(request, **kwargs):
del request.headers['Content-Type']


def handle_expires_header(
operation_model, response_dict, customized_response_dict, **kwargs
):
if _has_expires_shape(operation_model.output_shape):
if expires_value := response_dict.get('headers', {}).get('Expires'):
customized_response_dict['ExpiresString'] = expires_value
try:
utils.parse_timestamp(expires_value)
except (ValueError, RuntimeError):
logger.warning(
f'Failed to parse the "Expires" member as a timestamp: {expires_value}. '
f'The unparsed value is available in the response under "ExpiresString".'
)
del response_dict['headers']['Expires']


def _has_expires_shape(shape):
if not shape:
return False
return any(
member_shape.name == 'Expires'
and member_shape.serialization.get('name') == 'Expires'
for member_shape in shape.members.values()
)


def document_expires_shape(section, event_name, **kwargs):
# Updates the documentation for S3 operations that include the 'Expires' member
# in their response structure. Documents a synthetic member 'ExpiresString' and
# includes a deprecation notice for 'Expires'.
if 'response-example' in event_name:
if not section.has_section('structure-value'):
return
parent = section.get_section('structure-value')
if not parent.has_section('Expires'):
return
param_line = parent.get_section('Expires')
param_line.add_new_section('ExpiresString')
new_param_line = param_line.get_section('ExpiresString')
new_param_line.write("'ExpiresString': 'string',")
new_param_line.style.new_line()
elif 'response-params' in event_name:
if not section.has_section('Expires'):
return
param_section = section.get_section('Expires')
# Add a deprecation notice for the "Expires" param
doc_section = param_section.get_section('param-documentation')
doc_section.style.start_note()
doc_section.write(
'This member has been deprecated. Please use ``ExpiresString`` instead.'
)
doc_section.style.end_note()
# Document the "ExpiresString" param
new_param_section = param_section.add_new_section('ExpiresString')
new_param_section.style.new_paragraph()
new_param_section.write('- **ExpiresString** *(string) --*')
new_param_section.style.indent()
new_param_section.style.new_paragraph()
new_param_section.write(
'The raw, unparsed value of the ``Expires`` field.'
)


# This is a list of (event_name, handler).
# When a Session is created, everything in this list will be
# automatically registered with that Session.
Expand Down Expand Up @@ -1205,6 +1268,7 @@ def remove_content_type_header_for_presigning(request, **kwargs):
('after-call.ec2.GetConsoleOutput', decode_console_output),
('after-call.cloudformation.GetTemplate', json_decode_template_body),
('after-call.s3.GetBucketLocation', parse_get_bucket_location),
('before-parse.s3.*', handle_expires_header),
('before-parameter-build', generate_idempotent_uuid),
('before-parameter-build.s3', validate_bucket_name),
('before-parameter-build.s3', remove_bucket_from_url_paths_from_model),
Expand All @@ -1231,6 +1295,8 @@ def remove_content_type_header_for_presigning(request, **kwargs):
('before-parameter-build.s3-control', remove_accid_host_prefix_from_model),
('docs.*.s3.CopyObject.complete-section', document_copy_source_form),
('docs.*.s3.UploadPartCopy.complete-section', document_copy_source_form),
('docs.response-example.s3.*.complete-section', document_expires_shape),
('docs.response-params.s3.*.complete-section', document_expires_shape),
('before-endpoint-resolution.s3', customize_endpoint_resolver_builtins),
('before-call', add_recursion_detection_header),
('before-call.s3', add_expect_header),
Expand Down
36 changes: 36 additions & 0 deletions tests/functional/test_s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import re

import pytest
from dateutil.tz import tzutc

import botocore.session
from botocore import UNSIGNED
Expand Down Expand Up @@ -1340,6 +1341,41 @@ def test_500_error_with_non_xml_body(self):
self.assertEqual(len(http_stubber.requests), 2)


class TestS3ExpiresHeaderResponse(BaseS3OperationTest):
def test_valid_expires_value_in_response(self):
expires_value = "Thu, 01 Jan 1970 00:00:00 GMT"
mock_headers = {'expires': expires_value}
s3 = self.session.create_client("s3")
with ClientHTTPStubber(s3) as http_stubber:
http_stubber.add_response(headers=mock_headers)
response = s3.get_object(Bucket='mybucket', Key='mykey')
self.assertEqual(
response.get('Expires'),
datetime.datetime(1970, 1, 1, tzinfo=tzutc()),
)
self.assertEqual(response.get('ExpiresString'), expires_value)

def test_invalid_expires_value_in_response(self):
expires_value = "Invalid Date"
mock_headers = {'expires': expires_value}
warning_msg = 'Failed to parse the "Expires" member as a timestamp'
s3 = self.session.create_client("s3")
with self.assertLogs('botocore.handlers', level='WARNING') as log:
with ClientHTTPStubber(s3) as http_stubber:
http_stubber.add_response(headers=mock_headers)
response = s3.get_object(Bucket='mybucket', Key='mykey')
self.assertNotIn(
'expires',
response.get('ResponseMetadata').get('HTTPHeaders'),
)
self.assertNotIn('Expires', response)
self.assertEqual(response.get('ExpiresString'), expires_value)
self.assertTrue(
any(warning_msg in entry for entry in log.output),
f'Expected warning message not found in logs. Logs: {log.output}',
)


class TestWriteGetObjectResponse(BaseS3ClientConfigurationTest):
def create_stubbed_s3_client(self, **kwargs):
client = self.create_s3_client(**kwargs)
Expand Down
16 changes: 10 additions & 6 deletions tests/unit/test_endpoint.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,16 @@ def setUp(self):
def tearDown(self):
self.factory_patch.stop()

def get_emitter_responses(self, num_retries=0, sleep_time=0):
def get_emitter_responses(self, num_retries=0, sleep_time=0, num_events=4):
emitter_responses = []
# We emit the following events:
# 1. request-created
# 2. before-send
# 3. before-parse (may not be emitted if certain errors are thrown)
# 4. response-received
response_request_emitter_responses = [
[(None, None)], # emit() response for request-created
[(None, None)], # emit() response for before-send
[(None, None)], # emit() response for response-received
]
[(None, None)] # emit() response for each emitted event
] * num_events
for _ in range(num_retries):
emitter_responses.extend(response_request_emitter_responses)
# emit() response for retry for sleep time
Expand Down Expand Up @@ -239,6 +242,7 @@ def test_retry_events_can_alter_behavior(self):
expected_events=[
'request-created.ec2.DescribeInstances',
'before-send.ec2.DescribeInstances',
'before-parse.ec2.DescribeInstances',
'response-received.ec2.DescribeInstances',
'needs-retry.ec2.DescribeInstances',
]
Expand All @@ -247,7 +251,7 @@ def test_retry_events_can_alter_behavior(self):

def test_retry_on_socket_errors(self):
self.event_emitter.emit.side_effect = self.get_emitter_responses(
num_retries=1
num_retries=1, num_events=3
)
self.http_session.send.side_effect = HTTPClientError(error='wrapped')
with self.assertRaises(HTTPClientError):
Expand Down
164 changes: 164 additions & 0 deletions tests/unit/test_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import copy
import io
import json
import logging
import os

import pytest
Expand Down Expand Up @@ -1780,3 +1781,166 @@ def test_remove_bucket_from_url_paths_from_model(
)
assert model.http['requestUri'] == request_uri_after
assert model.http['authPath'] == auth_path


@pytest.fixture()
def operation_model_mock():
operation_model = mock.Mock()
operation_model.output_shape = mock.Mock()
operation_model.output_shape.members = {'Expires': mock.Mock()}
operation_model.output_shape.members['Expires'].name = 'Expires'
operation_model.output_shape.members['Expires'].serialization = {
'name': 'Expires'
}
return operation_model


@pytest.mark.parametrize(
"expires, expect_expires_header",
[
# Valid expires values
("Thu, 01 Jan 2015 00:00:00 GMT", True),
("10/21/2018", True),
("01 dec 2100", True),
("2023-11-02 08:43:04 -0400", True),
("Sun, 22 Oct 23 00:45:02 UTC", True),
# Invalid expires values
("Invalid Date", False),
("access plus 1 month", False),
("Expires: Thu, 9 Sep 2013 14:19:41 GMT", False),
("{ts '2023-10-10 09:27:14'}", False),
(-33702800404003370280040400, False),
],
)
def test_handle_expires_header(
expires, expect_expires_header, operation_model_mock
):
response_dict = {
'headers': {
'Expires': expires,
}
}
customized_response_dict = {}
handlers.handle_expires_header(
operation_model_mock, response_dict, customized_response_dict
)
assert customized_response_dict.get('ExpiresString') == expires
assert ('Expires' in response_dict['headers']) == expect_expires_header


def test_handle_expires_header_logs_warning(operation_model_mock, caplog):
response_dict = {
'headers': {
'Expires': 'Invalid Date',
}
}
with caplog.at_level(logging.WARNING):
handlers.handle_expires_header(operation_model_mock, response_dict, {})
assert len(caplog.records) == 1
assert 'Failed to parse the "Expires" member as a timestamp' in caplog.text


def test_handle_expires_header_does_not_log_warning(
operation_model_mock, caplog
):
response_dict = {
'headers': {
'Expires': 'Thu, 01 Jan 2015 00:00:00 GMT',
}
}
with caplog.at_level(logging.WARNING):
handlers.handle_expires_header(operation_model_mock, response_dict, {})
assert len(caplog.records) == 0


@pytest.fixture()
def document_expires_mocks():
return {
'section': mock.Mock(),
'parent': mock.Mock(),
'param_line': mock.Mock(),
'param_section': mock.Mock(),
'doc_section': mock.Mock(),
'new_param_line': mock.Mock(),
'new_param_section': mock.Mock(),
'response_example_event': 'docs.response-example.s3.TestOperation.complete-section',
'response_params_event': 'docs.response-params.s3.TestOperation.complete-section',
}


def test_document_response_example_with_expires(document_expires_mocks):
mocks = document_expires_mocks
mocks['section'].has_section.return_value = True
mocks['section'].get_section.return_value = mocks['parent']
mocks['parent'].has_section.return_value = True
mocks['parent'].get_section.return_value = mocks['param_line']
mocks['param_line'].has_section.return_value = True
mocks['param_line'].get_section.return_value = mocks['new_param_line']
handlers.document_expires_shape(
mocks['section'], mocks['response_example_event']
)
mocks['param_line'].add_new_section.assert_called_once_with(
'ExpiresString'
)
mocks['new_param_line'].write.assert_called_once_with(
"'ExpiresString': 'string',"
)
mocks['new_param_line'].style.new_line.assert_called_once()


def test_document_response_example_without_expires(document_expires_mocks):
mocks = document_expires_mocks
mocks['section'].has_section.return_value = True
mocks['section'].get_section.return_value = mocks['parent']
mocks['parent'].has_section.return_value = False
handlers.document_expires_shape(
mocks['section'], mocks['response_example_event']
)
mocks['parent'].add_new_section.assert_not_called()
mocks['parent'].get_section.assert_not_called()
mocks['new_param_line'].write.assert_not_called()


def test_document_response_params_with_expires(document_expires_mocks):
mocks = document_expires_mocks
mocks['section'].has_section.return_value = True
mocks['section'].get_section.return_value = mocks['param_section']
mocks['param_section'].get_section.side_effect = [
mocks['doc_section'],
]
mocks['param_section'].add_new_section.side_effect = [
mocks['new_param_section'],
]
mocks['doc_section'].style = mock.Mock()
mocks['new_param_section'].style = mock.Mock()
handlers.document_expires_shape(
mocks['section'], mocks['response_params_event']
)
mocks['param_section'].get_section.assert_any_call('param-documentation')
mocks['doc_section'].style.start_note.assert_called_once()
mocks['doc_section'].write.assert_called_once_with(
'This member has been deprecated. Please use ``ExpiresString`` instead.'
)
mocks['doc_section'].style.end_note.assert_called_once()
mocks['param_section'].add_new_section.assert_called_once_with(
'ExpiresString'
)
mocks['new_param_section'].style.new_paragraph.assert_any_call()
mocks['new_param_section'].write.assert_any_call(
'- **ExpiresString** *(string) --*'
)
mocks['new_param_section'].style.indent.assert_called_once()
mocks['new_param_section'].write.assert_any_call(
'The raw, unparsed value of the ``Expires`` field.'
)


def test_document_response_params_without_expires(document_expires_mocks):
mocks = document_expires_mocks
mocks['section'].has_section.return_value = False
handlers.document_expires_shape(
mocks['section'], mocks['response_params_event']
)
mocks['section'].get_section.assert_not_called()
mocks['param_section'].add_new_section.assert_not_called()
mocks['doc_section'].write.assert_not_called()

0 comments on commit 4c2771a

Please sign in to comment.