Skip to content

Commit

Permalink
fix HTTP HEAD method operation detection in ASF
Browse files Browse the repository at this point in the history
Co-authored-by: Thomas Rausch <thomas@thrau.at>
  • Loading branch information
alexrashed and thrau committed May 3, 2022
1 parent 65468b7 commit fb3f218
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 6 deletions.
30 changes: 25 additions & 5 deletions localstack/aws/protocol/op_router.py
Expand Up @@ -115,7 +115,25 @@ def matches(self, query_args: MultiDict, headers: Headers) -> bool:
return True


class _RequestMatchingRule(Rule):
class _StrictMethodRule(Rule):
"""
Small extension to Werkzeug's Rule class which reverts unwanted assumptions made by Werkzeug.
Reverted assumptions:
- Werkzeug automatically matches HEAD requests to the corresponding GET request (i.e. Werkzeug's rule automatically
adds the HEAD HTTP method to a rule which should only match GET requests). This is implemented to simplify
implementing an app compliant with HTTP (where a HEAD request needs to return the headers of a corresponding GET
request), but it is unwanted for our strict rule matching in here.
"""

def __init__(self, string: str, method: str, **kwargs) -> None:
super().__init__(string=string, methods=[method], **kwargs)

# Make sure Werkzeug's Rule does not add any other methods
# (f.e. the HEAD method even though the rule should only match GET)
self.methods = {method.upper()}


class _RequestMatchingRule(_StrictMethodRule):
"""
A Werkzeug Rule extension which initially acts as a normal rule (i.e. matches a path and method).
Expand All @@ -125,8 +143,10 @@ class _RequestMatchingRule(Rule):
The result of `match_request` is only meaningful if this wrapping rule also matches.
"""

def __init__(self, string: str, *args, operations: List[_HttpOperation], **kwargs) -> None:
super().__init__(string, *args, **kwargs)
def __init__(
self, string: str, operations: List[_HttpOperation], method: str, **kwargs
) -> None:
super().__init__(string=string, method=method, **kwargs)
# Create a rule which checks all required arguments (not only the path and method)
rules = [_RequiredArgsRule(op) for op in operations]
# Sort the rules descending based on their rule score
Expand Down Expand Up @@ -221,11 +241,11 @@ def _create_service_map(service: ServiceModel) -> Map:
# if there is only a single operation for a (path, method) combination,
# the default Werkzeug rule can be used directly (this is the case for most rules)
op = ops[0]
rules.append(Rule(rule_string, methods=[method], endpoint=op.operation)) # type: ignore
rules.append(_StrictMethodRule(string=rule_string, method=method, endpoint=op.operation)) # type: ignore
else:
# if there is an ambiguity with only the (path, method) combination,
# a custom rule - which can use additional request metadata - needs to be used
rules.append(_RequestMatchingRule(rule_string, methods=[method], operations=ops))
rules.append(_RequestMatchingRule(string=rule_string, method=method, operations=ops))

return Map(rules=rules, merge_slashes=False, converters={"path": GreedyPathConverter})

Expand Down
10 changes: 10 additions & 0 deletions tests/unit/aws/protocol/test_op_router.py
Expand Up @@ -47,3 +47,13 @@ def test_greedy_path_converter():
assert matcher.match("/some-route//foo/bar/bar") == (None, {"p": "/foo/bar"})
with pytest.raises(NotFound):
matcher.match("/some-route//foo/baz")


def test_s3_head_request():
router = RestServiceOperationRouter(load_service("s3"))

op, _ = router.match(Request("GET", "/my-bucket/my-key/"))
assert op.name == "GetObject"

op, _ = router.match(Request("HEAD", "/my-bucket/my-key/"))
assert op.name == "HeadObject"
9 changes: 8 additions & 1 deletion tests/unit/aws/protocol/test_parser.py
Expand Up @@ -889,7 +889,7 @@ def test_restjson_operation_detection_with_subpath():
)


def test_s3_operation_detection():
def test_s3_get_operation_detection():
"""
Test if the S3 operation detection works for ambiguous operations. GetObject is the worst, because it is
overloaded with the exact same requestURI by another non-deprecated function where the only distinction is the
Expand All @@ -900,6 +900,13 @@ def test_s3_operation_detection():
)


def test_s3_head_operation_detection():
"""Test if the S3 operation detection works for HEAD operations."""
_botocore_parser_integration_test(
service="s3", action="HeadObject", Bucket="test-bucket", Key="foo/bar/test.json"
)


def test_s3_put_object_keys_with_slashes():
_botocore_parser_integration_test(
service="s3",
Expand Down

0 comments on commit fb3f218

Please sign in to comment.