Refuse a patch path naming an attribute the schema does not declare

doc/changelog.rst

@@ -73,6 +73,9 @@ Changed
   when applied. It used to remove the entries equal to that ``value``, and to report no change
   when the ``value`` was a list or described an entry only in part. Set
   :attr:`~scim2_models.ScimPolicy.remove_value_as_filter` to keep reading it.
+- A PATCH reaching an extension attribute takes the extended resource type, as in
+  ``PatchOp[User[EnterpriseUser]]``. ``PatchOp[User]`` used to carry such an operation to the
+  endpoint, and now refuses a path its type parameter leaves out.
 
 Removed
 ^^^^^^^
@@ -107,6 +110,9 @@ Fixed
 - A PATCH operation carrying no ``path`` accepts a resource as its ``value``, and checks the
   attributes it names against the model. They used to go through unexamined, so a client naming
   an attribute it had misspelled was answered success.
+- A PATCH operation whose ``path`` names an attribute the resource schema does not declare is
+  refused with ``invalidPath``. It used to pass, so a client that misspelled an attribute was
+  answered success without anything being written. :issue:`164`
 - A refused PATCH ``add`` on a multi-valued attribute leaves the attribute as it was. The entry
   used to be appended before being validated, and outlived the failure.
 - A PATCH operation carrying no ``path`` marks the attributes it assigns as set, so

doc/explanation/policies.rst

@@ -46,13 +46,13 @@ stand in for one.
 What a policy leaves alone
 --------------------------
 
-**PATCH paths stay strict.** An operation whose ``path`` names an attribute no model declares
-raises :class:`~scim2_models.PathNotFoundException`, whatever the policy says. Path resolution and
-unknown attributes are two separate mechanisms, and making them uniform would take a third. The
-default that would come out of it is the wrong one: a server would answer 200 to a modification it
-never applied, where :rfc:`RFC7644 §3.5.2 <7644#section-3.5.2>` asks for an error. Inside the body
-of a resource the trade is different, since dropping one unknown attribute still lands everything
-the peer and the model both knew.
+**PATCH paths stay strict.** An operation whose ``path`` names an attribute no model declares is
+refused with ``invalidPath``, whatever the policy says. Path resolution and unknown attributes are
+two separate mechanisms, and making them uniform would take a third. The default that would come
+out of it is the wrong one: a server would answer 200 to a modification it never applied, where
+:rfc:`RFC7644 §3.5.2 <7644#section-3.5.2>` asks for an error. Inside the body of a resource the
+trade is different, since dropping one unknown attribute still lands everything the peer and the
+model both knew.
 
 **Building a model in Python stays strict.** ``User(bogus=1)`` and ``user.bogus = 1`` raise under
 every policy. Pydantic only offers a hook for extra keys during validation, so a keyword argument

scim2_models/messages/patch_op.py

@@ -22,6 +22,7 @@
 from ..exceptions import InvalidValueException
 from ..exceptions import MutabilityException
 from ..exceptions import NoTargetException
+from ..exceptions import PathNotFoundException
 from ..path import Path
 from ..path import ScimFilter
 from ..path import attribute_host
@@ -500,6 +501,11 @@ def validate_operations(self, info: ValidationInfo) -> Self:
             # targets, as a constraint on a complex attribute governs everything
             # written under it: "meta" is read-only where "meta.version" is not.
             if (resolved := operation.path.resolve()) is None:
+                if operation.path.model is None:
+                    raise PathNotFoundException(
+                        path=str(operation.path),
+                        detail=f"path '{operation.path}' is not declared by the resource schema",
+                    ).as_pydantic_error()
                 continue
             operation._validate_mutability(resolved.model, resolved.field_name)
             operation._validate_required_attribute(

tests/test_models.py

@@ -82,7 +82,18 @@ def _error_summary(exc: ValidationError) -> list[tuple[str, tuple]]:
 }
 
 
+SAMPLE_MODEL_OVERRIDES = {
+    "rfc7644-3.5.2.1-patch_op-add_members.json": PatchOp[Group],
+    "rfc7644-3.5.2.2-patch_op-remove_all_members.json": PatchOp[Group],
+    "rfc7644-3.5.2.2-patch_op-remove_and_add_one_member.json": PatchOp[Group],
+    "rfc7644-3.5.2.2-patch_op-remove_one_member.json": PatchOp[Group],
+    "rfc7644-3.5.2.3-patch_op-replace_all_members.json": PatchOp[Group],
+}
+
+
 def sample_model(sample: str) -> type:
+    if sample in SAMPLE_MODEL_OVERRIDES:
+        return SAMPLE_MODEL_OVERRIDES[sample]
     return SAMPLE_MODELS[sample.removesuffix(".json").split("-")[2]]
 
 

tests/test_patch_op_extensions.py

@@ -8,7 +8,6 @@
 from scim2_models import InvalidPathException
 from scim2_models import PatchOp
 from scim2_models import PatchOperation
-from scim2_models import PathNotFoundException
 from scim2_models import User
 from scim2_models.resources.enterprise_user import EnterpriseUser
 from scim2_models.resources.resource import Resource
@@ -26,9 +25,9 @@ def test_patch_operation_extension_simple_attribute():
         }
     )
 
-    patch1 = PatchOp[User](
+    patch1 = PatchOp[User[EnterpriseUser]](
         operations=[
-            PatchOperation[User](
+            PatchOperation[User[EnterpriseUser]](
                 op=PatchOperation.Op.replace_,
                 path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber",
                 value="54321",
@@ -39,9 +38,9 @@ def test_patch_operation_extension_simple_attribute():
     assert result is True
     assert user[EnterpriseUser].employee_number == "54321"
 
-    patch2 = PatchOp[User](
+    patch2 = PatchOp[User[EnterpriseUser]](
         operations=[
-            PatchOperation[User](
+            PatchOperation[User[EnterpriseUser]](
                 op=PatchOperation.Op.add,
                 path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:organization",
                 value="ACME Corp",
@@ -52,9 +51,9 @@ def test_patch_operation_extension_simple_attribute():
     assert result is True
     assert user[EnterpriseUser].organization == "ACME Corp"
 
-    patch3 = PatchOp[User](
+    patch3 = PatchOp[User[EnterpriseUser]](
         operations=[
-            PatchOperation[User](
+            PatchOperation[User[EnterpriseUser]](
                 op=PatchOperation.Op.remove,
                 path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:costCenter",
             )
@@ -77,9 +76,9 @@ def test_patch_operation_extension_complex_attribute():
         }
     )
 
-    patch1 = PatchOp[User](
+    patch1 = PatchOp[User[EnterpriseUser]](
         operations=[
-            PatchOperation[User](
+            PatchOperation[User[EnterpriseUser]](
                 op=PatchOperation.Op.replace_,
                 path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager.value",
                 value="new-manager-456",
@@ -91,9 +90,9 @@ def test_patch_operation_extension_complex_attribute():
     assert user[EnterpriseUser].manager.value == "new-manager-456"
     assert user[EnterpriseUser].manager.display_name == "John Smith"
 
-    patch2 = PatchOp[User](
+    patch2 = PatchOp[User[EnterpriseUser]](
         operations=[
-            PatchOperation[User](
+            PatchOperation[User[EnterpriseUser]](
                 op=PatchOperation.Op.replace_,
                 path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager",
                 value={
@@ -109,9 +108,9 @@ def test_patch_operation_extension_complex_attribute():
     assert user[EnterpriseUser].manager.value == "super-manager-789"
     assert user[EnterpriseUser].manager.display_name == "Alice Johnson"
 
-    patch3 = PatchOp[User](
+    patch3 = PatchOp[User[EnterpriseUser]](
         operations=[
-            PatchOperation[User](
+            PatchOperation[User[EnterpriseUser]](
                 op=PatchOperation.Op.remove,
                 path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager",
             )
@@ -139,9 +138,9 @@ def test_patch_operation_extension_mutability_handled_by_model():
 
     # This operation would fail during model validation for mutability,
     # but patch method assumes operations are already validated
-    patch = PatchOp[User](
+    patch = PatchOp[User[EnterpriseUser]](
         operations=[
-            PatchOperation[User](
+            PatchOperation[User[EnterpriseUser]](
                 op=PatchOperation.Op.replace_,
                 path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber",
                 value="12345",
@@ -154,38 +153,30 @@ def test_patch_operation_extension_mutability_handled_by_model():
 
 
 def test_patch_operation_extension_invalid_path_error():
-    """Test invalidPath error for non-existent extension attributes.
-
-    :rfc:`RFC7644 §3.5.2 <7644#section-3.5.2>`: invalidPath errors occur when
-    the path references an attribute that doesn't exist in the schema.
-    """
-    user = User[EnterpriseUser].model_validate({"userName": "test.user"})
-
-    patch1 = PatchOp[User](
-        operations=[
-            PatchOperation[User](
-                op=PatchOperation.Op.add,
-                path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:invalidAttribute",
-                value="test",
-            )
-        ]
-    )
-    with pytest.raises(InvalidPathException):
-        patch1.patch(user)
-    assert user[EnterpriseUser] is None
+    """An attribute the extension does not declare is refused, and so is its sub-attribute."""
+    with pytest.raises(ValidationError) as raised:
+        PatchOp[User[EnterpriseUser]](
+            operations=[
+                PatchOperation[User[EnterpriseUser]](
+                    op=PatchOperation.Op.add,
+                    path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:invalidAttribute",
+                    value="test",
+                )
+            ]
+        )
+    assert raised.value.errors()[0]["type"] == "scim_invalidPath"
 
-    patch2 = PatchOp[User](
-        operations=[
-            PatchOperation[User](
-                op=PatchOperation.Op.add,
-                path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager.invalidField",
-                value="test",
-            )
-        ]
-    )
-    with pytest.raises(InvalidPathException):
-        patch2.patch(user)
-    assert user[EnterpriseUser] is None
+    with pytest.raises(ValidationError) as raised:
+        PatchOp[User[EnterpriseUser]](
+            operations=[
+                PatchOperation[User[EnterpriseUser]](
+                    op=PatchOperation.Op.add,
+                    path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager.invalidField",
+                    value="test",
+                )
+            ]
+        )
+    assert raised.value.errors()[0]["type"] == "scim_invalidPath"
 
 
 def test_urn_parsing_errors():
@@ -230,17 +221,16 @@ class TestResourceTypeVar(Resource):
 
         typevar_field: T = None
 
-    user = TestResourceTypeVar()
-    patch = PatchOp[TestResourceTypeVar](
-        operations=[
-            PatchOperation[TestResourceTypeVar](
-                op=PatchOperation.Op.add, path="typevarField.subfield", value="test"
-            )
-        ]
-    )
+    with pytest.raises(ValidationError) as raised:
+        PatchOp[TestResourceTypeVar](
+            operations=[
+                PatchOperation[TestResourceTypeVar](
+                    op=PatchOperation.Op.add, path="typevarField.subfield", value="test"
+                )
+            ]
+        )
 
-    with pytest.raises(PathNotFoundException):
-        patch.patch(user)
+    assert raised.value.errors()[0]["type"] == "scim_invalidPath"
 
 
 def test_add_creates_the_parent_of_a_complex_attribute():
@@ -270,9 +260,9 @@ def test_patch_extension_schema_path_without_attribute():
     )
     user[EnterpriseUser] = EnterpriseUser()
 
-    patch = PatchOp[User](
+    patch = PatchOp[User[EnterpriseUser]](
         operations=[
-            PatchOperation[User](
+            PatchOperation[User[EnterpriseUser]](
                 op=PatchOperation.Op.add,
                 path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User",
                 value={
@@ -350,9 +340,9 @@ def test_patch_delete_extension_root():
     assert user[EnterpriseUser].employee_number == "12345"
     assert user[EnterpriseUser].cost_center == "Engineering"
 
-    patch = PatchOp[User](
+    patch = PatchOp[User[EnterpriseUser]](
         operations=[
-            PatchOperation[User](
+            PatchOperation[User[EnterpriseUser]](
                 op=PatchOperation.Op.remove,
                 path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User",
             )