Fix contradictory POST Allow header behavior (#495)

POST method was incorrectly included in Allow headers for all resources
even when it would return 405 Method Not Allowed. This created confusing
behavior where POST appeared as allowed but then failed.

Fixes #495

Author: jelmer

xandikos/tests/test_webdav.py

@@ -73,6 +73,9 @@ def start_response(code, headers):
     def lock(self, app, path):
         return self._method(app, "LOCK", path)
 
+    def options(self, app, path):
+        return self._method(app, "OPTIONS", path)
+
     def mkcol(self, app, path):
         environ = {
             "PATH_INFO": path,
@@ -245,13 +248,70 @@ def test_lock_not_allowed(self):
                 "Allow",
                 (
                     "COPY, DELETE, GET, HEAD, MKCOL, MOVE, OPTIONS, "
-                    "POST, PROPFIND, PROPPATCH, PUT, REPORT"
+                    "PROPFIND, PROPPATCH, PUT, REPORT"
                 ),
             ),
             headers,
         )
         self.assertEqual(b"", contents)
 
+    def test_post_allowed_on_collection(self):
+        """Test that POST is included in Allow header for collections."""
+        from ..webdav import Collection
+
+        class TestCollection(Collection):
+            resource_types = Collection.resource_types
+
+            def members(self):
+                return []
+
+            def get_member(self, name):
+                raise KeyError(name)
+
+            def delete_member(self, name, etag=None):
+                raise KeyError(name)
+
+            async def create_member(self, name, contents, content_type, requester=None):
+                return ("new_item", '"new_etag"')
+
+            def get_ctag(self):
+                return "test-ctag"
+
+            def destroy(self):
+                pass
+
+        app = self.makeApp({"/collection": TestCollection()}, [])
+        code, headers, contents = self.options(app, "/collection")
+        self.assertEqual("200 OK", code)
+
+        # Find the Allow header
+        allow_header = None
+        for header_name, header_value in headers:
+            if header_name == "Allow":
+                allow_header = header_value
+                break
+
+        self.assertIsNotNone(allow_header, "Allow header should be present")
+        self.assertIn("POST", allow_header, "POST should be allowed on collections")
+
+    def test_post_not_allowed_on_resource(self):
+        """Test that POST is NOT included in Allow header for regular resources."""
+        app = self.makeApp({"/resource": Resource()}, [])
+        code, headers, contents = self.options(app, "/resource")
+        self.assertEqual("200 OK", code)
+
+        # Find the Allow header
+        allow_header = None
+        for header_name, header_value in headers:
+            if header_name == "Allow":
+                allow_header = header_value
+                break
+
+        self.assertIsNotNone(allow_header, "Allow header should be present")
+        self.assertNotIn(
+            "POST", allow_header, "POST should NOT be allowed on regular resources"
+        )
+
     def test_mkcol_ok(self):
         class Backend:
             def create_collection(self, relpath):

xandikos/webdav.py

@@ -1828,7 +1828,7 @@ def name(self):
     async def handle(self, request, environ, app):
         raise NotImplementedError(self.handle)
 
-    def allow(self, request):
+    async def allow(self, request, environ, app):
         """Is this method allowed considering the specified request?"""
         return True
 
@@ -1851,14 +1851,23 @@ async def handle(self, request, environ, app):
 
 
 class PostMethod(Method):
+    async def allow(self, request, environ, app):
+        """POST is only allowed on collections (for RFC5995 add-member)."""
+        unused_href, path, r = app._get_resource_from_environ(request, environ)
+        if r is None:
+            return False
+        return COLLECTION_RESOURCE_TYPE in r.resource_types
+
     async def handle(self, request, environ, app):
         # see RFC5995
         new_contents = await _readBody(request)
         unused_href, path, r = app._get_resource_from_environ(request, environ)
         if r is None:
             return _send_not_found(request)
         if COLLECTION_RESOURCE_TYPE not in r.resource_types:
-            return _send_method_not_allowed(app._get_allowed_methods(request))
+            return _send_method_not_allowed(
+                await app._get_allowed_methods(request, environ)
+            )
         content_type, params = parse_type(request.content_type)
         try:
             (name, etag) = await r.create_member(
@@ -1912,7 +1921,9 @@ async def handle(self, request, environ, app):
                     description=e.description,
                 )
             except NotImplementedError:
-                return _send_method_not_allowed(app._get_allowed_methods(request))
+                return _send_method_not_allowed(
+                    await app._get_allowed_methods(request, environ)
+                )
             else:
                 return Response(status="204 No Content", headers=[("ETag", new_etag)])
         content_type = request.content_type
@@ -1922,7 +1933,9 @@ async def handle(self, request, environ, app):
             # RFC 4918 Section 9.7.1: Return 409 Conflict when intermediate collection is missing
             return Response(status=409, reason="Conflict")
         if COLLECTION_RESOURCE_TYPE not in r.resource_types:
-            return _send_method_not_allowed(app._get_allowed_methods(request))
+            return _send_method_not_allowed(
+                await app._get_allowed_methods(request, environ)
+            )
         try:
             (new_name, new_etag) = await r.create_member(
                 name,
@@ -2466,7 +2479,9 @@ async def handle(self, request, environ, app):
             raise UnsupportedMediaType(base_content_type)
         href, path, resource = app._get_resource_from_environ(request, environ)
         if resource is not None:
-            return _send_method_not_allowed(app._get_allowed_methods(request))
+            return _send_method_not_allowed(
+                await app._get_allowed_methods(request, environ)
+            )
         try:
             resource = app.backend.create_collection(path)
         except FileNotFoundError:
@@ -2506,7 +2521,7 @@ async def handle(self, request, environ, app):
                 return _send_not_found(request)
             dav_features = app._get_dav_features(r)
             headers.append(("DAV", ", ".join(dav_features)))
-            allowed_methods = app._get_allowed_methods(request)
+            allowed_methods = await app._get_allowed_methods(request, environ)
             headers.append(("Allow", ", ".join(allowed_methods)))
 
         # RFC7231 requires that if there is no response body,
@@ -2690,19 +2705,21 @@ def _get_dav_features(self, resource):
             "quota",
         ]
 
-    def _get_allowed_methods(self, request):
+    async def _get_allowed_methods(self, request, environ):
         """List of supported methods on this endpoint."""
         ret = []
         for name in sorted(self.methods.keys()):
-            if self.methods[name].allow(request):
+            if await self.methods[name].allow(request, environ, self):
                 ret.append(name)
         return ret
 
     async def _handle_request(self, request, environ, start_response=None):
         try:
             do = self.methods[request.method]
         except KeyError:
-            return _send_method_not_allowed(self._get_allowed_methods(request))
+            return _send_method_not_allowed(
+                await self._get_allowed_methods(request, environ)
+            )
         try:
             return await do.handle(request, environ, self)
         except BadRequestError as e: