Better RFC support for OPTIONS and CORS preflight requests

Ensures that the Allow header is set for 405 responses from the proxy,
as per RFC 2616.

CORS preflight requests now require both the Origin and Access-
Control-Request-Method headers to get a successful (200) response. The
draft spec defines errors as a response with a non-200 status code.
This patch sets the CORS error response code to 401 (Not Authorized).
A later patch may choose to make this configurable.

There is some ambiguity between RFC 2616 and the CORS draft spec
around what to do when a CORS request is made but the cluster has no
CORS information about the requested resource. This patch chooses to
return an error in this case because it is what would be simplest for
CORS client apps.

Further improvements to the OPTIONS verb not included in this patch
 include support of more top-level resources (eg / or /v1/) or
sending the configured constraints in the reponse body.

Change-Id: I40be059e8bbf3737dafc4e6fefa7598d05669c60

swift/proxy/controllers/account.py

@@ -42,13 +42,17 @@ class AccountController(Controller):
     def __init__(self, app, account_name, **kwargs):
         Controller.__init__(self, app)
         self.account_name = unquote(account_name)
+        if not self.app.allow_account_management:
+            self.allowed_methods.remove('PUT')
+            self.allowed_methods.remove('DELETE')
 
     def GETorHEAD(self, req):
         """Handler for HTTP GET/HEAD requests."""
         partition, nodes = self.app.account_ring.get_nodes(self.account_name)
         shuffle(nodes)
-        resp = self.GETorHEAD_base(req, _('Account'), partition, nodes,
+        resp = self.GETorHEAD_base(
-                req.path_info.rstrip('/'), len(nodes))
+            req, _('Account'), partition, nodes, req.path_info.rstrip('/'),
+            len(nodes))
         if resp.status_int == HTTP_NOT_FOUND and self.app.account_autocreate:
             if len(self.account_name) > MAX_ACCOUNT_NAME_LENGTH:
                 resp = HTTPBadRequest(request=req)
@@ -66,15 +70,18 @@ class AccountController(Controller):
                 self.app.logger.warning('Could not autocreate account %r' %
                                         self.account_name)
                 return resp
-            resp = self.GETorHEAD_base(req, _('Account'), partition, nodes,
+            resp = self.GETorHEAD_base(
-                req.path_info.rstrip('/'), len(nodes))
+                req, _('Account'), partition, nodes, req.path_info.rstrip('/'),
+                len(nodes))
         return resp
 
     @public
     def PUT(self, req):
         """HTTP PUT request handler."""
         if not self.app.allow_account_management:
-            return HTTPMethodNotAllowed(request=req)
+            return HTTPMethodNotAllowed(
+                request=req,
+                headers={'Allow': ', '.join(self.allowed_methods)})
         error_response = check_metadata(req, 'account')
         if error_response:
             return error_response
@@ -91,8 +98,9 @@ class AccountController(Controller):
         self.transfer_headers(req.headers, headers)
         if self.app.memcache:
             self.app.memcache.delete('account%s' % req.path_info.rstrip('/'))
-        resp = self.make_requests(req, self.app.account_ring,
+        resp = self.make_requests(
-            account_partition, 'PUT', req.path_info, [headers] * len(accounts))
+            req, self.app.account_ring, account_partition, 'PUT',
+            req.path_info, [headers] * len(accounts))
         return resp
 
     @public
@@ -109,9 +117,9 @@ class AccountController(Controller):
         self.transfer_headers(req.headers, headers)
         if self.app.memcache:
             self.app.memcache.delete('account%s' % req.path_info.rstrip('/'))
-        resp = self.make_requests(req, self.app.account_ring,
+        resp = self.make_requests(
-            account_partition, 'POST', req.path_info,
+            req, self.app.account_ring, account_partition, 'POST',
-            [headers] * len(accounts))
+            req.path_info, [headers] * len(accounts))
         if resp.status_int == HTTP_NOT_FOUND and self.app.account_autocreate:
             if len(self.account_name) > MAX_ACCOUNT_NAME_LENGTH:
                 resp = HTTPBadRequest(request=req)
@@ -132,7 +140,9 @@ class AccountController(Controller):
     def DELETE(self, req):
         """HTTP DELETE request handler."""
         if not self.app.allow_account_management:
-            return HTTPMethodNotAllowed(request=req)
+            return HTTPMethodNotAllowed(
+                request=req,
+                headers={'Allow': ', '.join(self.allowed_methods)})
         account_partition, accounts = \
             self.app.account_ring.get_nodes(self.account_name)
         headers = {'X-Timestamp': normalize_timestamp(time.time()),
@@ -140,7 +150,7 @@ class AccountController(Controller):
                    'Connection': 'close'}
         if self.app.memcache:
             self.app.memcache.delete('account%s' % req.path_info.rstrip('/'))
-        resp = self.make_requests(req, self.app.account_ring,
+        resp = self.make_requests(
-            account_partition, 'DELETE', req.path_info,
+            req, self.app.account_ring, account_partition, 'DELETE',
-            [headers] * len(accounts))
+            req.path_info, [headers] * len(accounts))
         return resp

swift/proxy/controllers/base.py

@@ -26,6 +26,7 @@
 
 import time
 import functools
+import inspect
 
 from eventlet import spawn_n, GreenPile, Timeout
 from eventlet.queue import Queue, Empty, Full
@@ -38,7 +39,7 @@ from swift.common.exceptions import ChunkReadTimeout, ConnectionTimeout
 from swift.common.http import is_informational, is_success, is_redirection, \
     is_server_error, HTTP_OK, HTTP_PARTIAL_CONTENT, HTTP_MULTIPLE_CHOICES, \
     HTTP_BAD_REQUEST, HTTP_NOT_FOUND, HTTP_SERVICE_UNAVAILABLE, \
-    HTTP_INSUFFICIENT_STORAGE
+    HTTP_INSUFFICIENT_STORAGE, HTTP_UNAUTHORIZED
 from swift.common.swob import Request, Response, status_map
 
 
@@ -132,6 +133,11 @@ class Controller(object):
         self.account_name = None
         self.app = app
         self.trans_id = '-'
+        self.allowed_methods = set()
+        all_methods = inspect.getmembers(self, predicate=inspect.ismethod)
+        for name, m in all_methods:
+            if getattr(m, 'publicly_accessible', False):
+                self.allowed_methods.add(name)
 
     def transfer_headers(self, src_headers, dst_headers):
 
@@ -696,28 +702,41 @@ class Controller(object):
         :param req: swob.Request object
         :returns: swob.Response object
         """
+        headers = {'Allow': ', '.join(self.allowed_methods)}
+        resp = Response(status=200, request=req,
+                        headers=headers)
+        req_origin_value = req.headers.get('Origin', None)
+        if not req_origin_value:
+            # NOT a CORS request
+            return resp
+
+        # CORS preflight request
+        try:
             container_info = \
                 self.container_info(self.account_name, self.container_name)
+        except AttributeError:
+            container_info = {}
         cors = container_info.get('cors', {})
         allowed_origins = set()
         if cors.get('allow_origin'):
             allowed_origins.update(cors['allow_origin'].split(' '))
         if self.app.cors_allow_origin:
             allowed_origins.update(self.app.cors_allow_origin)
-        if not allowed_origins:
+        if (req_origin_value not in allowed_origins and
-            return Response(status=401, request=req)
+                '*' not in allowed_origins) or (
-        headers = {}
+                req.headers.get('Access-Control-Request-Method') not in
-        if req.headers.get('Origin') in allowed_origins \
+                self.allowed_methods):
-                or '*' in allowed_origins:
+            resp.status = HTTP_UNAUTHORIZED
-            headers['access-control-allow-origin'] = ' '.join(allowed_origins)
+            return resp  # CORS preflight request that isn't valid
-            headers['access-control-max-age'] = cors.get('max_age')
+        headers['access-control-allow-origin'] = req_origin_value
-            headers['access-control-allow-methods'] = \
+        if cors.get('max_age', None) is not None:
-                'GET, POST, PUT, DELETE, HEAD'
+            headers['access-control-max-age'] = '%d' % cors.get('max_age')
-            headers['access-control-allow-headers'] = \
+        headers['access-control-allow-methods'] = ', '.join(
-                cors.get('allow_headers')
+            self.allowed_methods)
-            return Response(status=200, headers=headers, request=req)
+        if cors.get('allow_headers'):
-        else:
+            headers['access-control-allow-headers'] = cors.get('allow_headers')
-            return Response(status=401, request=req)
+        resp.headers = headers
+        return resp
 
     @public
     def OPTIONS(self, req):

swift/proxy/server.py

@@ -213,7 +213,9 @@ class Application(object):
                 handler = getattr(controller, req.method)
                 getattr(handler, 'publicly_accessible')
             except AttributeError:
-                return HTTPMethodNotAllowed(request=req)
+                allowed_methods = getattr(controller, 'allowed_methods', set())
+                return HTTPMethodNotAllowed(
+                    request=req, headers={'Allow': ', '.join(allowed_methods)})
             if path_parts['version']:
                 req.path_info_pop()
             if 'swift.authorize' in req.environ:

test/unit/proxy/test_server.py

@@ -3514,7 +3514,8 @@ class TestObjectController(unittest.TestCase):
             req = Request.blank(
                 '/a/c/o.jpg',
                 {'REQUEST_METHOD': 'OPTIONS'},
-                headers={'Origin': 'http://foo.com'})
+                headers={'Origin': 'http://foo.com',
+                         'Access-Control-Request-Method': 'GET'})
             resp = controller.OPTIONS(req)
             self.assertEquals(401, resp.status_int)
 
@@ -3524,7 +3525,8 @@ class TestObjectController(unittest.TestCase):
             req = Request.blank(
                 '/a/c/o.jpg',
                 {'REQUEST_METHOD': 'OPTIONS'},
-                headers={'Origin': 'http://foo.com'})
+                headers={'Origin': 'http://foo.com',
+                         'Access-Control-Request-Method': 'GET'})
             resp = controller.OPTIONS(req)
             self.assertEquals(401, resp.status_int)
 
@@ -3540,24 +3542,39 @@ class TestObjectController(unittest.TestCase):
             req = Request.blank(
                 '/a/c/o.jpg',
                 {'REQUEST_METHOD': 'OPTIONS'},
-                headers={'Origin': 'https://foo.bar'})
+                headers={'Origin': 'https://foo.bar',
+                         'Access-Control-Request-Method': 'GET'})
             req.content_length = 0
             resp = controller.OPTIONS(req)
             self.assertEquals(200, resp.status_int)
             self.assertEquals(
-                set(['http://foo.bar:8080', 'https://foo.bar']),
+                'https://foo.bar',
-                set(resp.headers['access-control-allow-origin'].split()))
+                resp.headers['access-control-allow-origin'])
+            for verb in 'OPTIONS COPY GET POST PUT DELETE HEAD'.split():
+                self.assertTrue(
+                    verb in resp.headers['access-control-allow-methods'])
             self.assertEquals(
-                'GET, POST, PUT, DELETE, HEAD',
+                len(resp.headers['access-control-allow-methods'].split(', ')),
-                resp.headers['access-control-allow-methods'])
+                7)
             self.assertEquals('999', resp.headers['access-control-max-age'])
             self.assertEquals(
                 'x-foo',
                 resp.headers['access-control-allow-headers'])
-            req = Request.blank('/a/c/o.jpg', {'REQUEST_METHOD': 'OPTIONS'})
+            req = Request.blank(
+                '/a/c/o.jpg',
+                {'REQUEST_METHOD': 'OPTIONS'},
+                headers={'Origin': 'https://foo.bar'})
             req.content_length = 0
             resp = controller.OPTIONS(req)
             self.assertEquals(401, resp.status_int)
+            req = Request.blank('/a/c/o.jpg', {'REQUEST_METHOD': 'OPTIONS'})
+            req.content_length = 0
+            resp = controller.OPTIONS(req)
+            self.assertEquals(200, resp.status_int)
+            for verb in 'OPTIONS COPY GET POST PUT DELETE HEAD'.split():
+                self.assertTrue(
+                    verb in resp.headers['Allow'])
+            self.assertEquals(len(resp.headers['Allow'].split(', ')), 7)
             req = Request.blank(
                 '/a/c/o.jpg',
                 {'REQUEST_METHOD': 'OPTIONS'},
@@ -3567,11 +3584,43 @@ class TestObjectController(unittest.TestCase):
             req = Request.blank(
                 '/a/c/o.jpg',
                 {'REQUEST_METHOD': 'OPTIONS'},
-                headers={'Origin': 'http://foo.bar'})
+                headers={'Origin': 'http://foo.bar',
+                         'Access-Control-Request-Method': 'GET'})
             controller.app.cors_allow_origin = ['http://foo.bar', ]
             resp = controller.OPTIONS(req)
             self.assertEquals(200, resp.status_int)
 
+            def my_container_info_wildcard(*args):
+                return {
+                    'cors': {
+                        'allow_origin': '*',
+                        'allow_headers': 'x-foo',
+                        'max_age': 999,
+                    }
+                }
+            controller.container_info = my_container_info_wildcard
+            req = Request.blank(
+                '/a/c/o.jpg',
+                {'REQUEST_METHOD': 'OPTIONS'},
+                headers={'Origin': 'https://bar.baz',
+                         'Access-Control-Request-Method': 'GET'})
+            req.content_length = 0
+            resp = controller.OPTIONS(req)
+            self.assertEquals(200, resp.status_int)
+            self.assertEquals(
+                'https://bar.baz',
+                resp.headers['access-control-allow-origin'])
+            for verb in 'OPTIONS COPY GET POST PUT DELETE HEAD'.split():
+                self.assertTrue(
+                    verb in resp.headers['access-control-allow-methods'])
+            self.assertEquals(
+                len(resp.headers['access-control-allow-methods'].split(', ')),
+                7)
+            self.assertEquals('999', resp.headers['access-control-max-age'])
+            self.assertEquals(
+                'x-foo',
+                resp.headers['access-control-allow-headers'])
+
 
 class TestContainerController(unittest.TestCase):
     "Test swift.proxy_server.ContainerController"
@@ -4071,7 +4120,8 @@ class TestContainerController(unittest.TestCase):
             req = Request.blank(
                 '/a/c',
                 {'REQUEST_METHOD': 'OPTIONS'},
-                headers={'Origin': 'http://foo.com'})
+                headers={'Origin': 'http://foo.com',
+                         'Access-Control-Request-Method': 'GET'})
             resp = controller.OPTIONS(req)
             self.assertEquals(401, resp.status_int)
 
@@ -4081,7 +4131,8 @@ class TestContainerController(unittest.TestCase):
             req = Request.blank(
                 '/a/c',
                 {'REQUEST_METHOD': 'OPTIONS'},
-                headers={'Origin': 'http://foo.com'})
+                headers={'Origin': 'http://foo.com',
+                         'Access-Control-Request-Method': 'GET'})
             resp = controller.OPTIONS(req)
             self.assertEquals(401, resp.status_int)
 
@@ -4097,38 +4148,86 @@ class TestContainerController(unittest.TestCase):
             req = Request.blank(
                 '/a/c',
                 {'REQUEST_METHOD': 'OPTIONS'},
-                headers={'Origin': 'https://foo.bar'})
+                headers={'Origin': 'https://foo.bar',
+                         'Access-Control-Request-Method': 'GET'})
             req.content_length = 0
             resp = controller.OPTIONS(req)
             self.assertEquals(200, resp.status_int)
             self.assertEquals(
-                set(['http://foo.bar:8080', 'https://foo.bar']),
+                'https://foo.bar',
-                set(resp.headers['access-control-allow-origin'].split()))
+                resp.headers['access-control-allow-origin'])
+            for verb in 'OPTIONS GET POST PUT DELETE HEAD'.split():
+                self.assertTrue(
+                    verb in resp.headers['access-control-allow-methods'])
             self.assertEquals(
-                'GET, POST, PUT, DELETE, HEAD',
+                len(resp.headers['access-control-allow-methods'].split(', ')),
-                resp.headers['access-control-allow-methods'])
+                6)
             self.assertEquals('999', resp.headers['access-control-max-age'])
             self.assertEquals(
                 'x-foo',
                 resp.headers['access-control-allow-headers'])
-            req = Request.blank('/a/c', {'REQUEST_METHOD': 'OPTIONS'})
+            req = Request.blank(
+                '/a/c',
+                {'REQUEST_METHOD': 'OPTIONS'},
+                headers={'Origin': 'https://foo.bar'})
             req.content_length = 0
             resp = controller.OPTIONS(req)
             self.assertEquals(401, resp.status_int)
+            req = Request.blank('/a/c', {'REQUEST_METHOD': 'OPTIONS'})
+            req.content_length = 0
+            resp = controller.OPTIONS(req)
+            self.assertEquals(200, resp.status_int)
+            for verb in 'OPTIONS GET POST PUT DELETE HEAD'.split():
+                self.assertTrue(
+                    verb in resp.headers['Allow'])
+            self.assertEquals(len(resp.headers['Allow'].split(', ')), 6)
             req = Request.blank(
                 '/a/c',
                 {'REQUEST_METHOD': 'OPTIONS'},
-                headers={'Origin': 'http://foo.bar'})
+                headers={'Origin': 'http://foo.bar',
+                         'Access-Control-Request-Method': 'GET'})
             resp = controller.OPTIONS(req)
             self.assertEquals(401, resp.status_int)
             req = Request.blank(
                 '/a/c',
                 {'REQUEST_METHOD': 'OPTIONS'},
-                headers={'Origin': 'http://foo.bar'})
+                headers={'Origin': 'http://foo.bar',
+                         'Access-Control-Request-Method': 'GET'})
             controller.app.cors_allow_origin = ['http://foo.bar', ]
             resp = controller.OPTIONS(req)
             self.assertEquals(200, resp.status_int)
 
+            def my_container_info_wildcard(*args):
+                return {
+                    'cors': {
+                        'allow_origin': '*',
+                        'allow_headers': 'x-foo',
+                        'max_age': 999,
+                    }
+                }
+            controller.container_info = my_container_info_wildcard
+            req = Request.blank(
+                '/a/c/o.jpg',
+                {'REQUEST_METHOD': 'OPTIONS'},
+                headers={'Origin': 'https://bar.baz',
+                         'Access-Control-Request-Method': 'GET'})
+            req.content_length = 0
+            resp = controller.OPTIONS(req)
+            self.assertEquals(200, resp.status_int)
+            self.assertEquals(
+                'https://bar.baz',
+                resp.headers['access-control-allow-origin'])
+            for verb in 'OPTIONS GET POST PUT DELETE HEAD'.split():
+                self.assertTrue(
+                    verb in resp.headers['access-control-allow-methods'])
+            self.assertEquals(
+                len(resp.headers['access-control-allow-methods'].split(', ')),
+                6)
+            self.assertEquals('999', resp.headers['access-control-max-age'])
+            self.assertEquals(
+                'x-foo',
+                resp.headers['access-control-allow-headers'])
+
 
 class TestAccountController(unittest.TestCase):
 
@@ -4151,6 +4250,30 @@ class TestAccountController(unittest.TestCase):
             res = method(req)
             self.assertEquals(res.status_int, expected)
 
+    def test_OPTIONS(self):
+        with save_globals():
+            self.app.allow_account_management = False
+            controller = proxy_server.AccountController(self.app, 'account')
+            req = Request.blank('/account', {'REQUEST_METHOD': 'OPTIONS'})
+            req.content_length = 0
+            resp = controller.OPTIONS(req)
+            self.assertEquals(200, resp.status_int)
+            for verb in 'OPTIONS GET POST HEAD'.split():
+                self.assertTrue(
+                    verb in resp.headers['Allow'])
+            self.assertEquals(len(resp.headers['Allow'].split(', ')), 4)
+            self.app.allow_account_management = True
+            controller = proxy_server.AccountController(self.app, 'account')
+            req = Request.blank('/account', {'REQUEST_METHOD': 'OPTIONS'})
+            req.content_length = 0
+            resp = controller.OPTIONS(req)
+            self.assertEquals(200, resp.status_int)
+            print resp.headers['Allow']
+            for verb in 'OPTIONS GET POST PUT DELETE HEAD'.split():
+                self.assertTrue(
+                    verb in resp.headers['Allow'])
+            self.assertEquals(len(resp.headers['Allow'].split(', ')), 6)
+
     def test_GET(self):
         with save_globals():
             controller = proxy_server.AccountController(self.app, 'account')