From ff04ef05cd5a8f9675cbc8f292fa666d03ae6644 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Wed, 22 May 2019 16:36:50 -0700 Subject: [PATCH] Rework private-request-method interface Instead of taking a X-Backend-Allow-Method that *must match* the REQUEST_METHOD, take a truish X-Backend-Allow-Private-Methods and expand the set of allowed methods. This allows us to also expose the full list of available private methods when returning a 405. Drive-By: make async-delete tests a little more robust: * check that end_marker and prefix are preserved on subsequent listings * check that objects with a leading slash are correctly handled Change-Id: I5542623f16e0b5a0d728a6706343809e50743f73 --- swift/cli/container_deleter.py | 2 +- swift/common/utils.py | 11 +++++++++++ swift/proxy/controllers/base.py | 11 +++++++++++ swift/proxy/controllers/container.py | 3 ++- swift/proxy/server.py | 11 ++++++----- test/unit/cli/test_container_deleter.py | 21 +++++++++++---------- test/unit/proxy/test_server.py | 10 +++++++++- 7 files changed, 51 insertions(+), 18 deletions(-) diff --git a/swift/cli/container_deleter.py b/swift/cli/container_deleter.py index 62b65c6dd0..71dd370a8a 100644 --- a/swift/cli/container_deleter.py +++ b/swift/cli/container_deleter.py @@ -113,7 +113,7 @@ def mark_for_deletion(swift, account, container, marker, end_marker, swift.make_request( 'UPDATE', swift.make_path('.expiring_objects', str(int(timestamp))), - headers={'X-Backend-Allow-Method': 'UPDATE', + headers={'X-Backend-Allow-Private-Methods': 'True', 'X-Backend-Storage-Policy-Index': '0', 'X-Timestamp': timestamp.internal}, acceptable_statuses=(2,), diff --git a/swift/common/utils.py b/swift/common/utils.py index 7e76964fbe..336f039adf 100644 --- a/swift/common/utils.py +++ b/swift/common/utils.py @@ -3740,6 +3740,17 @@ def public(func): return func +def private(func): + """ + Decorator to declare which methods are privately accessible as HTTP + requests with an ``X-Backend-Allow-Private-Methods: True`` override + + :param func: function to make private + """ + func.privately_accessible = True + return func + + def majority_size(n): return (n // 2) + 1 diff --git a/swift/proxy/controllers/base.py b/swift/proxy/controllers/base.py index 160ed4316f..06328def36 100644 --- a/swift/proxy/controllers/base.py +++ b/swift/proxy/controllers/base.py @@ -1517,6 +1517,7 @@ class Controller(object): self.app = app self.trans_id = '-' self._allowed_methods = None + self._private_methods = None @property def allowed_methods(self): @@ -1528,6 +1529,16 @@ class Controller(object): self._allowed_methods.add(name) return self._allowed_methods + @property + def private_methods(self): + if self._private_methods is None: + self._private_methods = set() + all_methods = inspect.getmembers(self, predicate=inspect.ismethod) + for name, m in all_methods: + if getattr(m, 'privately_accessible', False): + self._private_methods.add(name) + return self._private_methods + def _x_remove_headers(self): """ Returns a list of headers that must not be sent to the backend diff --git a/swift/proxy/controllers/container.py b/swift/proxy/controllers/container.py index 58f2a569d2..747d6b998f 100644 --- a/swift/proxy/controllers/container.py +++ b/swift/proxy/controllers/container.py @@ -18,7 +18,7 @@ import json from six.moves.urllib.parse import unquote -from swift.common.utils import public, csv_append, Timestamp, \ +from swift.common.utils import public, private, csv_append, Timestamp, \ config_true_value, ShardRange from swift.common.constraints import check_metadata, CONTAINER_LISTING_LIMIT from swift.common.http import HTTP_ACCEPTED, is_success @@ -356,6 +356,7 @@ class ContainerController(Controller): return HTTPNotFound(request=req) return resp + @private def UPDATE(self, req): """HTTP UPDATE request handler. diff --git a/swift/proxy/server.py b/swift/proxy/server.py index 082a6d2f79..7c8fbef502 100644 --- a/swift/proxy/server.py +++ b/swift/proxy/server.py @@ -507,13 +507,14 @@ class Application(object): controller.trans_id = req.environ['swift.trans_id'] self.logger.client_ip = get_remote_client(req) - allowed_methods = set(controller.allowed_methods) - if 'X-Backend-Allow-Method' in req.headers: - allowed_methods.add(req.headers['X-Backend-Allow-Method']) + allowed_methods = controller.allowed_methods + if config_true_value(req.headers.get( + 'X-Backend-Allow-Private-Methods', False)): + allowed_methods = set(allowed_methods).union( + controller.private_methods) if req.method not in allowed_methods: return HTTPMethodNotAllowed(request=req, headers={ - # Only advertise the *controller's* allowed_methods - 'Allow': ', '.join(controller.allowed_methods)}) + 'Allow': ', '.join(allowed_methods)}) handler = getattr(controller, req.method) old_authorize = None diff --git a/test/unit/cli/test_container_deleter.py b/test/unit/cli/test_container_deleter.py index 7c4aaa2bed..9aeff5df2d 100644 --- a/test/unit/cli/test_container_deleter.py +++ b/test/unit/cli/test_container_deleter.py @@ -90,7 +90,7 @@ class TestContainerDeleter(unittest.TestCase): uacct = acct = u'acct-\U0001f334' ucont = cont = u'cont-\N{SNOWMAN}' uobj1 = obj1 = u'obj-\N{GREEK CAPITAL LETTER ALPHA}' - uobj2 = obj2 = u'obj-\N{GREEK CAPITAL LETTER OMEGA}' + uobj2 = obj2 = u'/obj-\N{GREEK CAPITAL LETTER OMEGA}' if six.PY2: acct = acct.encode('utf8') cont = cont.encode('utf8') @@ -184,7 +184,7 @@ class TestContainerDeleter(unittest.TestCase): ts = '1558463777.42739' with FakeInternalClient([ swob.Response(json.dumps([ - {'name': 'obj1'}, + {'name': '/obj1'}, {'name': 'obj2'}, {'name': 'obj3'}, ])), @@ -208,14 +208,14 @@ class TestContainerDeleter(unittest.TestCase): ('GET', '/v1/account/container', 'format=json&marker=obj3&end_marker=&prefix=', {}, None), ('UPDATE', '/v1/.expiring_objects/' + ts.split('.')[0], '', { - 'X-Backend-Allow-Method': 'UPDATE', + 'X-Backend-Allow-Private-Methods': 'True', 'X-Backend-Storage-Policy-Index': '0', 'X-Timestamp': ts}, mock.ANY), ]) self.assertEqual( json.loads(swift.calls[-1].body), container_deleter.make_delete_jobs( - 'account', 'container', ['obj1', 'obj2', 'obj3'], + 'account', 'container', ['/obj1', 'obj2', 'obj3'], utils.Timestamp(ts) ) ) @@ -241,22 +241,23 @@ class TestContainerDeleter(unittest.TestCase): 'account', 'container', '', - '', - '', + 'end', + 'pre', timestamp=utils.Timestamp(ts), yield_time=0, )), [(5, 'obj5'), (6, 'obj6'), (6, None)]) self.assertEqual(swift.calls, [ ('GET', '/v1/account/container', - 'format=json&marker=&end_marker=&prefix=', {}, None), + 'format=json&marker=&end_marker=end&prefix=pre', {}, None), ('UPDATE', '/v1/.expiring_objects/' + ts.split('.')[0], '', { - 'X-Backend-Allow-Method': 'UPDATE', + 'X-Backend-Allow-Private-Methods': 'True', 'X-Backend-Storage-Policy-Index': '0', 'X-Timestamp': ts}, mock.ANY), ('GET', '/v1/account/container', - 'format=json&marker=obj6&end_marker=&prefix=', {}, None), + 'format=json&marker=obj6&end_marker=end&prefix=pre', + {}, None), ('UPDATE', '/v1/.expiring_objects/' + ts.split('.')[0], '', { - 'X-Backend-Allow-Method': 'UPDATE', + 'X-Backend-Allow-Private-Methods': 'True', 'X-Backend-Storage-Policy-Index': '0', 'X-Timestamp': ts}, mock.ANY), ]) diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index 6a4cf86c18..6732b2a491 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -684,11 +684,19 @@ class TestProxyServer(unittest.TestCase): # But with appropriate (internal-only) overrides, you can still use it resp = baseapp.handle_request( Request.blank('/v1/a/c', environ={'REQUEST_METHOD': 'UPDATE'}, - headers={'X-Backend-Allow-Method': 'UPDATE', + headers={'X-Backend-Allow-Private-Methods': 'True', 'X-Backend-Storage-Policy-Index': '0'})) # Now we actually make the requests, but there aren't any nodes self.assertEqual(resp.status, '503 Service Unavailable') + # Bad method with overrides advertises private methods + resp = baseapp.handle_request( + Request.blank('/v1/a/c', environ={'REQUEST_METHOD': 'BOGUS'}, + headers={'X-Backend-Allow-Private-Methods': '1'})) + self.assertEqual(resp.status, '405 Method Not Allowed') + self.assertEqual(sorted(resp.headers['Allow'].split(', ')), [ + 'DELETE', 'GET', 'HEAD', 'OPTIONS', 'POST', 'PUT', 'UPDATE']) + def test_calls_authorize_allow(self): called = [False]