diff --git a/doc/changelog.rst b/doc/changelog.rst index 3efb8a9..c677d7b 100644 --- a/doc/changelog.rst +++ b/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 diff --git a/doc/explanation/policies.rst b/doc/explanation/policies.rst index 8885585..d314f2e 100644 --- a/doc/explanation/policies.rst +++ b/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 diff --git a/scim2_models/messages/patch_op.py b/scim2_models/messages/patch_op.py index 0e606bc..0054c38 100644 --- a/scim2_models/messages/patch_op.py +++ b/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( diff --git a/tests/test_models.py b/tests/test_models.py index 7b82a21..76bd86a 100644 --- a/tests/test_models.py +++ b/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]] diff --git a/tests/test_patch_op_extensions.py b/tests/test_patch_op_extensions.py index e9a4cec..c37586f 100644 --- a/tests/test_patch_op_extensions.py +++ b/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", ) diff --git a/tests/test_patch_op_validation.py b/tests/test_patch_op_validation.py index babad09..32527c0 100644 --- a/tests/test_patch_op_validation.py +++ b/tests/test_patch_op_validation.py @@ -33,62 +33,36 @@ class ConstrainedExtension(Extension): plain_attr: str | None = None -def test_patch_op_add_invalid_extension_path(): - user = User(user_name="john") - patch_op = PatchOp[User]( - operations=[ - PatchOperation[User]( - op="add", - path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User", - value={"key": "value"}, - ) - ] - ) - with pytest.raises(InvalidPathException): - patch_op.patch(user) - - -def test_patch_op_replace_invalid_extension_path(): - user = User(user_name="john") - patch_op = PatchOp[User]( - operations=[ - PatchOperation[User]( - op="replace", - path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User.attr", - value="test", - ) - ] - ) - with pytest.raises(InvalidPathException): - patch_op.patch(user) +def test_a_path_naming_an_extension_the_resource_does_not_carry_is_refused(): + """A schema URN the type parameter leaves out designates no target.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User]( + operations=[ + PatchOperation[User]( + op="add", + path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User", + value={"employeeNumber": "12345"}, + ) + ] + ) + assert raised.value.errors()[0]["type"] == "scim_invalidPath" -def test_patch_op_remove_invalid_extension_path(): - user = User(user_name="john") - patch_op = PatchOp[User]( - operations=[ - PatchOperation[User]( - op="remove", - path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User.attr", - ) - ] - ) - with pytest.raises(InvalidPathException): - patch_op.patch(user) +def test_a_schema_urn_separated_from_its_attribute_by_a_dot_is_refused(): + """A URN carries its attribute behind a colon, and a dot names a sub-attribute of it.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User[ConstrainedExtension]]( + operations=[ + PatchOperation[User[ConstrainedExtension]]( + op="replace", + path="urn:example:2.0:Constrained.plainAttr", + value="test", + ) + ] + ) -def test_patch_op_remove_unknown_extension_attribute(): - user = User(user_name="john") - patch_op = PatchOp[User]( - operations=[ - PatchOperation[User]( - op="remove", - path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User.attr", - ) - ] - ) - with pytest.raises(InvalidPathException): - patch_op.patch(user) + assert raised.value.errors()[0]["type"] == "scim_invalidPath" def test_patch_op_without_type_parameter(): @@ -212,7 +186,7 @@ def test_value_required_for_add_operations(): { "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], "operations": [ - {"op": "replace", "path": "foobar"}, + {"op": "replace", "path": "nickName"}, ], }, context={"scim": Context.RESOURCE_PATCH_REQUEST}, @@ -222,7 +196,7 @@ def test_value_required_for_add_operations(): { "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], "operations": [ - {"op": "add", "path": "foobar"}, + {"op": "add", "path": "nickName"}, ], }, context={"scim": Context.RESOURCE_PATCH_REQUEST}, @@ -232,7 +206,7 @@ def test_value_required_for_add_operations(): { "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], "operations": [ - {"op": "remove", "path": "foobar"}, + {"op": "remove", "path": "nickName"}, ], }, context={"scim": Context.RESOURCE_PATCH_REQUEST}, @@ -399,21 +373,6 @@ def test_patch_remove_on_readonly_field_is_rejected(): ) -def test_patch_validation_allows_unknown_fields(): - """Patch operations on unknown fields pass without mutability checks.""" - patch_op = PatchOp[User].model_validate( - { - "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], - "operations": [ - {"op": "add", "path": "unknownField", "value": "some-value"}, - ], - }, - context={"scim": Context.RESOURCE_PATCH_REQUEST}, - ) - assert len(patch_op.operations) == 1 - assert patch_op.operations[0].path == "unknownField" - - def test_patch_operations_on_readwrite_fields_allowed(): """All patch operations are allowed on readWrite fields.""" patch_op = PatchOp[User].model_validate( @@ -429,21 +388,6 @@ def test_patch_operations_on_readwrite_fields_allowed(): assert len(patch_op.operations) == 2 -def test_remove_operation_on_unknown_field_validates(): - """Test remove operation on unknown field validates successfully.""" - patch_op = PatchOp[User].model_validate( - { - "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], - "operations": [ - {"op": "remove", "path": "unknownField"}, - ], - }, - context={"scim": Context.RESOURCE_PATCH_REQUEST}, - ) - assert len(patch_op.operations) == 1 - assert patch_op.operations[0].path == "unknownField" - - def test_remove_operation_on_non_required_field_allowed(): """Test remove operation on non-required field is allowed.""" # nickName is not required, so remove should be allowed @@ -581,26 +525,6 @@ def test_patch_op_with_typevar_bound_to_non_resource(): PatchOp[NonResourceT] -def test_create_parent_object_return_none(): - """Test _create_parent_object returns None when type resolution fails.""" - user = User() - - # Create a patch that will trigger _create_parent_object with complex path - patch = PatchOp[User]( - operations=[ - PatchOperation[User]( - op=PatchOperation.Op.add, - path="complexField.subField", # Non-existent complex field - value="test", - ) - ] - ) - - # Non-existent field returns invalidPath error - with pytest.raises(InvalidPathException): - patch.patch(user) - - def test_validate_required_field_removal(): """Test that removing required fields raises validation error.""" # Test removing schemas (required field) should raise validation error @@ -632,41 +556,33 @@ def test_patch_error_handling_invalid_operation(): patch.patch(user) -def test_remove_value_at_path_invalid_field(): - """Test removing value at path with invalid parent field name.""" - user = User(name={"familyName": "Test"}) - - # Create patch that attempts to remove from invalid parent field - patch = PatchOp[User]( - operations=[ - PatchOperation[User]( - op=PatchOperation.Op.remove, path="invalidParent.subField" - ) - ] - ) +def test_a_path_whose_parent_attribute_no_model_declares_is_refused(): + """A sub-attribute is looked up on the attribute holding it, which must exist first.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User]( + operations=[ + PatchOperation[User]( + op=PatchOperation.Op.remove, path="invalidParent.subField" + ) + ] + ) - # Non-existent field returns invalidPath error - with pytest.raises(InvalidPathException): - patch.patch(user) + assert raised.value.errors()[0]["type"] == "scim_invalidPath" def test_remove_an_attribute_no_model_declares(): - """Test removing specific value from invalid field name.""" - user = User() - - # Create patch that attempts to remove specific value from invalid field - patch = PatchOp[User]( - operations=[ - PatchOperation[User]( - op=PatchOperation.Op.remove, - path="invalidField", - ) - ] - ) + """A remove names its target in its path, and one outside the schema is refused.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User]( + operations=[ + PatchOperation[User]( + op=PatchOperation.Op.remove, + path="invalidField", + ) + ] + ) - # Non-existent field returns invalidPath error - with pytest.raises(InvalidPathException): - patch.patch(user) + assert raised.value.errors()[0]["type"] == "scim_invalidPath" def test_patch_op_operations_attribute_required_in_patch_context(): @@ -1010,20 +926,18 @@ def test_a_urn_that_merely_starts_like_a_schema_reaches_no_attribute(): ``…:2.0:User`` and writes a :attr:`~scim2_models.Mutability.read_only` attribute that every other spelling of it is refused. """ - user = User(user_name="bjensen", id="2819c223") - patch_op = PatchOp[User]( - operations=[ - PatchOperation[User]( - op=PatchOperation.Op.replace_, - path="urn:ietf:params:scim:schemas:core:2.0:UserId", - value="forged", - ) - ] - ) + with pytest.raises(ValidationError) as raised: + PatchOp[User]( + operations=[ + PatchOperation[User]( + op=PatchOperation.Op.replace_, + path="urn:ietf:params:scim:schemas:core:2.0:UserId", + value="forged", + ) + ] + ) - with pytest.raises(InvalidPathException): - patch_op.patch(user) - assert user.id == "2819c223" + assert raised.value.errors()[0]["type"] == "scim_invalidPath" def test_a_patch_path_naming_a_subattribute_of_a_scalar_answers_invalid_path(): @@ -1032,19 +946,16 @@ def test_a_patch_path_naming_a_subattribute_of_a_scalar_answers_invalid_path(): :rfc:`RFC7644 §3.12 <7644#section-3.12>` gives ``invalidPath`` for a path that is unknown, which a client may write without meaning to. """ - user = User(user_name="bjensen") - patch_op = PatchOp[User]( - operations=[ - PatchOperation[User]( - op=PatchOperation.Op.replace_, path="userName.foo", value="forged" - ) - ] - ) + with pytest.raises(ValidationError) as raised: + PatchOp[User]( + operations=[ + PatchOperation[User]( + op=PatchOperation.Op.replace_, path="userName.foo", value="forged" + ) + ] + ) - with pytest.raises(InvalidPathException) as raised: - patch_op.patch(user) - assert raised.value.to_error().scim_type == "invalidPath" - assert user.user_name == "bjensen" + assert raised.value.errors()[0]["type"] == "scim_invalidPath" def _constrained_user(): @@ -1149,3 +1060,48 @@ def test_patch_selection_naming_an_unknown_attribute_fails_the_operation(): with pytest.raises(InvalidFilterException, match="nonexistent"): patch.patch(user) + + +def test_a_path_naming_an_attribute_no_model_declares_is_refused(): + """A path outside the resource schema is refused, as the same mistake without a path is.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User].model_validate( + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [{"op": "add", "path": "nonexistent", "value": "x"}], + }, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + assert raised.value.errors()[0]["type"] == "scim_invalidPath" + + +def test_a_path_naming_a_subattribute_no_model_declares_is_refused(): + """A sub-attribute outside the schema of the attribute holding it is refused too.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User].model_validate( + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + {"op": "replace", "path": "name.nonexistent", "value": "x"} + ], + }, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + assert raised.value.errors()[0]["type"] == "scim_invalidPath" + + +def test_a_path_designating_the_resource_itself_is_accepted(): + """The resource root names no attribute, and answers to the value as a pathless operation.""" + patch = PatchOp[User].model_validate( + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [{"op": "add", "path": "", "value": {"nickName": "Babs"}}], + }, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + user = User(user_name="bjensen") + patch.patch(user) + assert user.nick_name == "Babs"