From af3be1b7e3df9db7a5166838e38b6fb4cc190c62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89loi=20Rivard?= Date: Thu, 2 Apr 2026 18:19:36 +0200 Subject: [PATCH] fix: enforce immutable mutability checks at runtime in PATCH operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move immutable field validation from parse-time to runtime in patch(), where the resource instance is available. This aligns with RFC 7644 §3.5.2 which allows adding a value to an immutable attribute only if it had no previous value, and tolerates no-op operations (remove on unset fields, replace with identical value). --- doc/changelog.rst | 7 +++ scim2_models/messages/patch_op.py | 72 ++++++++++++++++++----- tests/test_patch_op_replace.py | 22 +++---- tests/test_patch_op_validation.py | 95 ++++++++++++++++++++++++++++--- 4 files changed, 165 insertions(+), 31 deletions(-) diff --git a/doc/changelog.rst b/doc/changelog.rst index 85635e0..60c5fd8 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -1,6 +1,13 @@ Changelog ========= +[0.6.8] - Unreleased +-------------------- + +Fixed +^^^^^ +- PATCH operations on :attr:`~scim2_models.Mutability.immutable` fields are now validated at runtime per :rfc:`RFC 7644 §3.5.2 <7644#section-3.5.2>`: ``add`` is only allowed when the field has no previous value, ``replace`` is only allowed with the same value, and ``remove`` is only allowed on unset fields. + [0.6.7] - 2026-04-02 -------------------- diff --git a/scim2_models/messages/patch_op.py b/scim2_models/messages/patch_op.py index aeaf9c3..01898f7 100644 --- a/scim2_models/messages/patch_op.py +++ b/scim2_models/messages/patch_op.py @@ -52,28 +52,23 @@ class Op(str, Enum): def _validate_mutability( self, resource_class: type[Resource[Any]], field_name: str ) -> None: - """Validate mutability constraints.""" - # RFC 7644 Section 3.5.2: "Servers should be tolerant of schema extensions" + """Validate mutability constraints at parse-time. + + Only :attr:`~scim2_models.Mutability.read_only` is validated here. + :attr:`~scim2_models.Mutability.immutable` validation requires access + to the resource instance and is enforced at runtime in + :meth:`PatchOp._check_immutable`. + """ if field_name not in resource_class.model_fields: return mutability = resource_class.get_field_annotation(field_name, Mutability) - # RFC 7643 Section 7: "Attributes with mutability 'readOnly' SHALL NOT be modified" - if mutability == Mutability.read_only and self.op in ( - PatchOperation.Op.add, - PatchOperation.Op.replace_, - ): + if mutability == Mutability.read_only: raise MutabilityException( attribute=field_name, mutability="readOnly", operation=self.op.value ).as_pydantic_error() - # RFC 7643 Section 7: "Attributes with mutability 'immutable' SHALL NOT be updated" - if mutability == Mutability.immutable and self.op == PatchOperation.Op.replace_: - raise MutabilityException( - attribute=field_name, mutability="immutable", operation=self.op.value - ).as_pydantic_error() - def _validate_required_attribute( self, resource_class: type[Resource[Any]], field_name: str ) -> None: @@ -284,7 +279,12 @@ def _apply_operation( """Apply a single patch operation to a resource. :return: :data:`True` if the resource was modified, else :data:`False`. + :raises MutabilityException: If the operation would modify an + immutable attribute. """ + if operation.path is not None: + self._check_immutable(resource, operation) + if operation.op in (PatchOperation.Op.add, PatchOperation.Op.replace_): return self._apply_add_replace(resource, operation) if operation.op == PatchOperation.Op.remove: @@ -292,6 +292,52 @@ def _apply_operation( raise InvalidValueException(detail=f"unsupported operation: {operation.op}") + def _check_immutable( + self, resource: Resource[Any], operation: PatchOperation[ResourceT] + ) -> None: + """Validate immutable constraints at runtime. + + :rfc:`RFC 7644 §3.5.2 <7644#section-3.5.2>`: + + *"A client MUST NOT modify an attribute that has mutability + "readOnly" or "immutable". However, a client MAY "add" a value + to an "immutable" attribute if the attribute had no previous + value."* + + An operation is considered a no-op (and thus allowed) when it would + not effectively change the resource state: ``remove`` on an unset + field, or ``replace`` with the current value. + """ + resource_class = type(resource) + assert operation.path is not None + field_name = operation.path.parts[0] if operation.path.parts else None + if field_name is None or field_name not in resource_class.model_fields: + return + + mutability = resource_class.get_field_annotation(field_name, Mutability) + if mutability != Mutability.immutable: + return + + current_value = getattr(resource, field_name, None) + + if operation.op == PatchOperation.Op.add and current_value is None: + return + + if operation.op == PatchOperation.Op.remove and current_value is None: + return + + if ( + operation.op == PatchOperation.Op.replace_ + and operation.value == current_value + ): + return + + raise MutabilityException( + attribute=field_name, + mutability="immutable", + operation=operation.op.value, + ) + def _apply_add_replace( self, resource: Resource[Any], operation: PatchOperation[ResourceT] ) -> bool: diff --git a/tests/test_patch_op_replace.py b/tests/test_patch_op_replace.py index 082cc64..c421a92 100644 --- a/tests/test_patch_op_replace.py +++ b/tests/test_patch_op_replace.py @@ -1,9 +1,9 @@ from typing import Annotated import pytest -from pydantic import ValidationError from scim2_models import URN +from scim2_models import MutabilityException from scim2_models import PatchOp from scim2_models import PatchOperation from scim2_models import User @@ -205,21 +205,23 @@ def test_replace_operation_with_non_dict_value_no_path(): def test_immutable_field(): - """Test that replace operations on immutable fields raise validation errors.""" + """Test that replace operations on immutable fields raise mutability errors.""" class Dummy(Resource): __schema__ = URN("urn:test:TestResource") immutable: Annotated[str, Mutability.immutable] - with pytest.raises(ValidationError, match="mutability"): - PatchOp[Dummy]( - operations=[ - PatchOperation[Dummy]( - op=PatchOperation.Op.replace_, path="immutable", value="new_value" - ) - ] - ) + resource = Dummy.model_construct(immutable="original") + patch = PatchOp[Dummy]( + operations=[ + PatchOperation[Dummy]( + op=PatchOperation.Op.replace_, path="immutable", value="new_value" + ) + ] + ) + with pytest.raises(MutabilityException): + patch.patch(resource) def test_primary_auto_exclusion_on_add(): diff --git a/tests/test_patch_op_validation.py b/tests/test_patch_op_validation.py index d79dcc8..69bd319 100644 --- a/tests/test_patch_op_validation.py +++ b/tests/test_patch_op_validation.py @@ -1,3 +1,4 @@ +from typing import Annotated from typing import TypeVar import pytest @@ -6,6 +7,8 @@ from scim2_models import Group from scim2_models import InvalidPathException from scim2_models import InvalidValueException +from scim2_models import Mutability +from scim2_models import MutabilityException from scim2_models import PatchOp from scim2_models import PatchOperation from scim2_models import User @@ -13,6 +16,10 @@ from scim2_models.resources.resource import Resource +class ImmutableFieldResource(Resource): + locked: Annotated[str | None, Mutability.immutable] = None + + def test_patch_op_add_invalid_extension_path(): user = User(user_name="john") patch_op = PatchOp[User]( @@ -257,9 +264,8 @@ def test_validate_mutability_readonly_error(): ) -def test_validate_mutability_immutable_error(): - """Test mutability validation error for immutable attributes.""" - # Test replace operation on immutable field within groups complex attribute +def test_validate_mutability_readonly_replace_via_complex_path(): + """Replacing a readOnly complex attribute path is rejected.""" with pytest.raises(ValidationError, match="mutability"): PatchOp[User].model_validate( { @@ -275,9 +281,83 @@ def test_validate_mutability_immutable_error(): ) +def test_patch_remove_on_immutable_field_with_value_is_rejected(): + """Removing an existing immutable attribute via PATCH is rejected.""" + resource = ImmutableFieldResource.model_construct(locked="existing") + patch_op = PatchOp[ImmutableFieldResource].model_validate( + {"operations": [{"op": "remove", "path": "locked"}]}, + context={"scim": Context.RESOURCE_PATCH_REQUEST}, + ) + with pytest.raises(MutabilityException): + patch_op.patch(resource) + + +def test_patch_remove_on_immutable_field_without_value_is_allowed(): + """Removing an unset immutable attribute is a no-op and is allowed.""" + resource = ImmutableFieldResource.model_construct() + patch_op = PatchOp[ImmutableFieldResource].model_validate( + {"operations": [{"op": "remove", "path": "locked"}]}, + context={"scim": Context.RESOURCE_PATCH_REQUEST}, + ) + patch_op.patch(resource) + assert resource.locked is None + + +def test_patch_add_on_immutable_field_with_existing_value_is_rejected(): + """Adding to an immutable attribute that already has a value is rejected.""" + resource = ImmutableFieldResource.model_construct(locked="existing") + patch_op = PatchOp[ImmutableFieldResource].model_validate( + {"operations": [{"op": "add", "path": "locked", "value": "new"}]}, + context={"scim": Context.RESOURCE_PATCH_REQUEST}, + ) + with pytest.raises(MutabilityException): + patch_op.patch(resource) + + +def test_patch_add_on_immutable_field_without_value_is_allowed(): + """Adding to an immutable attribute with no previous value is allowed per RFC 7644.""" + resource = ImmutableFieldResource.model_construct() + patch_op = PatchOp[ImmutableFieldResource].model_validate( + {"operations": [{"op": "add", "path": "locked", "value": "initial"}]}, + context={"scim": Context.RESOURCE_PATCH_REQUEST}, + ) + patch_op.patch(resource) + assert resource.locked == "initial" + + +def test_patch_replace_on_immutable_field_with_different_value_is_rejected(): + """Replacing an immutable attribute with a different value is rejected.""" + resource = ImmutableFieldResource.model_construct(locked="existing") + patch_op = PatchOp[ImmutableFieldResource].model_validate( + {"operations": [{"op": "replace", "path": "locked", "value": "other"}]}, + context={"scim": Context.RESOURCE_PATCH_REQUEST}, + ) + with pytest.raises(MutabilityException): + patch_op.patch(resource) + + +def test_patch_replace_on_immutable_field_with_same_value_is_allowed(): + """Replacing an immutable attribute with its current value is a no-op and is allowed.""" + resource = ImmutableFieldResource.model_construct(locked="existing") + patch_op = PatchOp[ImmutableFieldResource].model_validate( + {"operations": [{"op": "replace", "path": "locked", "value": "existing"}]}, + context={"scim": Context.RESOURCE_PATCH_REQUEST}, + ) + patch_op.patch(resource) + assert resource.locked == "existing" + + +def test_patch_remove_on_readonly_field_is_rejected(): + """Removing a readOnly attribute via PATCH is rejected per RFC 7643 §7.""" + with pytest.raises(ValidationError, match="mutability"): + PatchOp[User].model_validate( + {"operations": [{"op": "remove", "path": "id"}]}, + context={"scim": Context.RESOURCE_PATCH_REQUEST}, + ) + + def test_patch_validation_allows_unknown_fields(): - """Test that patch validation allows unknown fields in operations.""" - # This should not raise an error even though 'unknownField' doesn't exist on User + """Patch operations on unknown fields pass without mutability checks.""" patch_op = PatchOp[User].model_validate( { "operations": [ @@ -290,9 +370,8 @@ def test_patch_validation_allows_unknown_fields(): assert patch_op.operations[0].path == "unknownField" -def test_non_replace_operations_on_immutable_fields_allowed(): - """Test that non-replace operations on immutable fields are allowed.""" - # Test with non-immutable fields since groups.value is immutable +def test_patch_operations_on_readwrite_fields_allowed(): + """All patch operations are allowed on readWrite fields.""" patch_op = PatchOp[User].model_validate( { "operations": [