From dfab5fa9f6bf647fe074ee894c19554a52f56b5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89loi=20Rivard?= Date: Sun, 27 Sep 2026 14:32:16 +0200 Subject: [PATCH 1/7] fix: keep a null PATCH value when dumping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SCIM dumps drop null values, so a replace that clears its target was sent without a value, and the server rejected it as malformed. The value is now kept when it was set, except on remove, which only uses the path (RFC 7644 §3.5.2.2). --- doc/changelog.rst | 6 ++++- scim2_models/messages/patch_op.py | 34 ++++++++++++++++++--------- scim2_models/path/access.py | 13 +++++------ tests/test_patch_op_validation.py | 38 ++++++++++++++++++++++++++++++- 4 files changed, 71 insertions(+), 20 deletions(-) diff --git a/doc/changelog.rst b/doc/changelog.rst index 051b5f26..b859cade 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -12,6 +12,11 @@ Changed - In a model built from a schema, an attribute named after a member of the model, such as ``copy``, is held as ``copy_``. Its SCIM name is unchanged. +Fixed +^^^^^ +- A :class:`~scim2_models.PatchOperation` with a null value keeps it when dumped. A ``replace`` + that clears its target used to be sent without a value. + Security ^^^^^^^^ - A published schema can no longer break the models built from it, nor make @@ -21,7 +26,6 @@ Security - :func:`~scim2_models.get_model_by_payload` matches no model when ``schemas`` is not a list of strings. - [0.8.2] - 2026-09-25 -------------------- diff --git a/scim2_models/messages/patch_op.py b/scim2_models/messages/patch_op.py index e3d2a40c..513707e7 100644 --- a/scim2_models/messages/patch_op.py +++ b/scim2_models/messages/patch_op.py @@ -11,8 +11,11 @@ from pydantic import BaseModel as PydanticBaseModel from pydantic import Field +from pydantic import SerializationInfo +from pydantic import SerializerFunctionWrapHandler from pydantic import ValidationInfo from pydantic import field_validator +from pydantic import model_serializer from pydantic import model_validator from ..annotations import Mutability @@ -316,6 +319,24 @@ def _validate_operation_requirements(self, info: ValidationInfo) -> Self: value: Any | None = None + @model_serializer(mode="wrap") + def _scim_serializer( + self, handler: SerializerFunctionWrapHandler, info: SerializationInfo + ) -> dict[str, Any]: + """Keep a null value the operation was given. + + SCIM dumps drop null values, so a replace that clears its target would + be sent without a value. + """ + serialized = super()._scim_serializer(handler, info) + if ( + self.op != PatchOperation.Op.remove + and "value" in self.model_fields_set + and self.value is None + ): + serialized["value"] = None + return serialized + @field_validator("op", mode="before") @classmethod def _normalize_op(cls, v: Any) -> Any: @@ -376,12 +397,7 @@ def __class_getitem__(cls, item: Any) -> Any: @model_validator(mode="after") def _validate_operations(self, info: ValidationInfo) -> Self: - """Validate operations against resource type metadata if available. - - When PatchOp is used with a specific resource type (e.g., - PatchOp[User]), this validator will automatically check mutability and - required constraints. - """ + """Reject the errors the operations have on any resource of the type.""" # RFC 7644: The body of an HTTP PATCH request MUST contain the attribute "Operations" scim_ctx = info.context.get("scim") if info.context else None if scim_ctx == Context.RESOURCE_PATCH_REQUEST and self.operations is None: @@ -534,11 +550,7 @@ def patch(self, resource: ResourceT, scim_policy: ScimPolicy | None = None) -> b def _apply_operation( self, resource: Resource[Any], operation: PatchOperation[ResourceT] ) -> bool: - """Apply a single patch operation, and say whether the resource changed. - - An operation modifying an immutable attribute raises - MutabilityException. - """ + """Apply one operation as RFC7644 §3.5.2 defines it, then check the result.""" if operation.path is not None: self._check_immutable(resource, operation) diff --git a/scim2_models/path/access.py b/scim2_models/path/access.py index aa1c8406..5c2c54dd 100644 --- a/scim2_models/path/access.py +++ b/scim2_models/path/access.py @@ -260,13 +260,12 @@ def _set_selected( ) -> bool: """Apply a value to every entry matched by a value selection. - A replacement selection matching nothing raises NoTargetException, per - RFC7644 §3.5.2.3. That failure is defined for ``replace`` only: §3.5.2.1 - says nothing of a selection that matches nothing for ``add``, so the - operation is a no-op instead. Errata 8097 - (https://errata.rfc-editor.org/eid8097/) asks for value selections in - ``add`` to be clarified at all, implementations differing on whether they - are allowed. + A selection matching nothing raises NoTargetException. RFC7644 §3.5.2.3 + requires it for replace, and Table 9 of §3.12 defines noTarget for a filter + that "yields no match", which applies to add too. + + Without a sub-attribute, each matched entry is replaced in place, so it + stays the same object. Entries have no identity other than the object. """ host, field_name, matched, sub_attr = selection diff --git a/tests/test_patch_op_validation.py b/tests/test_patch_op_validation.py index 1a0a1dc0..f70267e7 100644 --- a/tests/test_patch_op_validation.py +++ b/tests/test_patch_op_validation.py @@ -804,7 +804,7 @@ def test_an_operation_without_path_refuses_an_undeclared_attribute(): def test_an_operation_without_path_may_unassign_an_extension(): - """An extension is no attribute of the resource, so none of it is required.""" + """An extension is not an attribute of the resource, so it is not required.""" patch = PatchOp[User[ConstrainedExtension]].model_validate( { "Operations": [ @@ -1109,3 +1109,39 @@ def test_a_path_designating_the_resource_itself_is_accepted(): user = User(user_name="bjensen") patch.patch(user) assert user.nick_name == "Babs" + + +def test_a_replace_unassigning_its_target_keeps_its_null_value_when_dumped(): + """A replace dumped without its value would be read back as missing its value.""" + patch = PatchOp[User]( + operations=[PatchOperation[User](op="replace", path="title", value=None)] + ) + + payload = patch.model_dump(scim_ctx=Context.RESOURCE_PATCH_REQUEST) + + assert payload["Operations"] == [{"op": "replace", "path": "title", "value": None}] + user = User(user_name="bjensen", title="CEO") + PatchOp[User].model_validate( + payload, scim_ctx=Context.RESOURCE_PATCH_REQUEST + ).patch(user) + assert user.title is None + + +def test_an_operation_given_no_value_is_dumped_without_one(): + """Only a null value the operation was given is kept in the dump.""" + operation = PatchOperation[User](op="replace", path="title") + + assert operation.model_dump(scim_ctx=Context.RESOURCE_PATCH_REQUEST) == { + "op": "replace", + "path": "title", + } + + +def test_a_remove_given_a_null_value_is_dumped_without_one(): + """RFC7644 §3.5.2.2 reads a remove from its path only, so a null value is left out.""" + operation = PatchOperation[User](op="remove", path="title", value=None) + + assert operation.model_dump(scim_ctx=Context.RESOURCE_PATCH_REQUEST) == { + "op": "remove", + "path": "title", + } From be0940362dda96c9fc8f9a5c739443946542c28e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89loi=20Rivard?= Date: Sun, 27 Sep 2026 14:32:41 +0200 Subject: [PATCH 2/7] fix: don't create a container to hold a null value Setting null under an unset complex attribute or extension created an empty object to hold it. The resource ended up with an empty object and was reported as modified. Nothing is created for null anymore. --- doc/changelog.rst | 2 ++ scim2_models/path/access.py | 4 +++- tests/test_patch_op_replace.py | 21 +++++++++++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/doc/changelog.rst b/doc/changelog.rst index b859cade..83669599 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -16,6 +16,8 @@ Fixed ^^^^^ - A :class:`~scim2_models.PatchOperation` with a null value keeps it when dumped. A ``replace`` that clears its target used to be sent without a value. +- Setting a null value under an unset complex attribute or extension no longer creates an empty + one, and no longer reports the resource as modified. Security ^^^^^^^^ diff --git a/scim2_models/path/access.py b/scim2_models/path/access.py index 5c2c54dd..f2c6654f 100644 --- a/scim2_models/path/access.py +++ b/scim2_models/path/access.py @@ -347,7 +347,9 @@ def _set_value( if (selection := _select(path, resource)) is not None: return _set_selected(path, selection, value, is_add=is_add) - target = _walk(path, resource, create=True) + # Nothing is created to hold a null value: it would only unassign what the + # new container holds, which is nothing. + target = _walk(path, resource, create=value is not None) if target is None: return False if isinstance(target, _Root): diff --git a/tests/test_patch_op_replace.py b/tests/test_patch_op_replace.py index af278dbd..06ef7c75 100644 --- a/tests/test_patch_op_replace.py +++ b/tests/test_patch_op_replace.py @@ -3,6 +3,7 @@ import pytest from scim2_models import URN +from scim2_models import EnterpriseUser from scim2_models import MutabilityException from scim2_models import PatchOp from scim2_models import PatchOperation @@ -395,3 +396,23 @@ def test_replace_a_subattribute_of_every_entry(): "new@example.com", "new@example.com", ] + + +@pytest.mark.parametrize( + "path", + [ + "name.givenName", + "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:costCenter", + "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager.value", + ], +) +def test_unassigning_under_an_unassigned_container_leaves_the_resource(path): + """Nothing is created to hold a null value.""" + user = User[EnterpriseUser](user_name="bjensen") + patch = PatchOp[User[EnterpriseUser]].model_validate( + {"Operations": [{"op": "replace", "path": path, "value": None}]} + ) + + assert patch.patch(user) is False + assert user.name is None + assert user[EnterpriseUser] is None From c3cfc2bfdd7b9eccff575fd44172891c91ad5a3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89loi=20Rivard?= Date: Sun, 27 Sep 2026 14:33:10 +0200 Subject: [PATCH 3/7] fix: return invalidValue for an empty or unknown PATCH operation A PatchOp with no operation, or with an op other than add, remove and replace, raised a pydantic error without a scimType, so a server could not turn it into a SCIM error. Both now raise invalidValue. --- doc/changelog.rst | 3 +++ scim2_models/messages/patch_op.py | 19 ++++++++++++++++--- tests/test_patch_op_validation.py | 19 +++++++++++++++++++ 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/doc/changelog.rst b/doc/changelog.rst index 83669599..a9c9367c 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -18,6 +18,9 @@ Fixed that clears its target used to be sent without a value. - Setting a null value under an unset complex attribute or extension no longer creates an empty one, and no longer reports the resource as modified. +- A :class:`~scim2_models.PatchOp` with no operation, or with an operation other than ``add``, + ``remove`` and ``replace``, fails with ``invalidValue`` instead of a validation error without + ``scimType``. Security ^^^^^^^^ diff --git a/scim2_models/messages/patch_op.py b/scim2_models/messages/patch_op.py index 513707e7..909d6aad 100644 --- a/scim2_models/messages/patch_op.py +++ b/scim2_models/messages/patch_op.py @@ -350,8 +350,16 @@ def _normalize_op(cls, v: Any) -> Any: Microsoft Entra ID emits the values of op as Add, Replace, and Remove. """ if isinstance(v, str): - return v.lower() - return v + v = v.lower() + try: + return cls.Op(v) + except ValueError: + # RFC7644 §3.5.2 defines no scimType for an unknown operation, and + # §3.12 defines invalidValue for a value that does not fit the + # operation. + raise InvalidValueException( + detail=f"{v!r} is not a PATCH operation: add, remove or replace" + ).as_pydantic_error() from None class PatchOp(_ResourceParameterized, Message, Generic[ResourceT]): @@ -390,7 +398,7 @@ def __class_getitem__(cls, item: Any) -> Any: __schema__ = URN("urn:ietf:params:scim:api:messages:2.0:PatchOp") operations: Annotated[list[PatchOperation[ResourceT]] | None, Required.true] = ( - Field(None, serialization_alias="Operations", min_length=1) + Field(None, serialization_alias="Operations") ) """The body of an HTTP PATCH request MUST contain the attribute "Operations", whose value is an array of one or more PATCH operations.""" @@ -405,6 +413,11 @@ def _validate_operations(self, info: ValidationInfo) -> Self: detail="operations attribute is required" ).as_pydantic_error() + if self.operations == []: + raise InvalidValueException( + detail="operations holds one or more operations" + ).as_pydantic_error() + resource_class = _get_resource_class(self) if resource_class is None or not self.operations: return self diff --git a/tests/test_patch_op_validation.py b/tests/test_patch_op_validation.py index f70267e7..876ef020 100644 --- a/tests/test_patch_op_validation.py +++ b/tests/test_patch_op_validation.py @@ -1145,3 +1145,22 @@ def test_a_remove_given_a_null_value_is_dumped_without_one(): "op": "remove", "path": "title", } + + +def test_a_patch_without_any_operation_is_refused(): + """RFC7644 §3.5.2 requires "Operations" to hold at least one operation.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User].model_validate({"Operations": []}) + + assert raised.value.errors()[0]["type"] == "scim_invalidValue" + + +@pytest.mark.parametrize("op", ["move", "copy", 1]) +def test_an_unknown_operation_is_refused(op): + """RFC7644 §3.5.2 defines add, remove and replace, and §3.12 gives invalidValue for anything else.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User].model_validate( + {"Operations": [{"op": op, "path": "nickName", "value": "Babs"}]} + ) + + assert raised.value.errors()[0]["type"] == "scim_invalidValue" From a137bd41a5720c2ef415a9491480a46718e9e6f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89loi=20Rivard?= Date: Sun, 27 Sep 2026 14:34:12 +0200 Subject: [PATCH 4/7] fix: return noTarget when a PATCH add filter matches nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An add whose filter matched no entry did nothing and reported success. Microsoft Entra ID sends such requests, for example add emails[type eq "work"].value, and expects the entry to be created, so its data was silently lost. The operation is now rejected with noTarget, like replace (RFC 7644 §3.12). --- doc/changelog.rst | 4 ++++ doc/explanation/patch.rst | 14 +++++++---- scim2_models/path/access.py | 8 ++----- scim2_models/path/path.py | 4 ++-- tests/test_patch_op_add.py | 37 ++++++++++++++++++++++++++++++ tests/test_path_value_selection.py | 21 ++++++++++------- 6 files changed, 67 insertions(+), 21 deletions(-) diff --git a/doc/changelog.rst b/doc/changelog.rst index a9c9367c..2b00626c 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -11,6 +11,10 @@ Changed :class:`str` and f-strings give their value, ``readOnly`` rather than ``Mutability.read_only``. - In a model built from a schema, an attribute named after a member of the model, such as ``copy``, is held as ``copy_``. Its SCIM name is unchanged. +- A PATCH ``add`` whose path filter matches no entry, such as ``emails[type eq "work"].value`` + on a user without a work email, now fails with ``noTarget`` instead of silently doing nothing. + :meth:`Path.set ` raises :class:`~scim2_models.NoTargetException` in + that case when strict. Fixed ^^^^^ diff --git a/doc/explanation/patch.rst b/doc/explanation/patch.rst index 7761ce56..fc1856f7 100644 --- a/doc/explanation/patch.rst +++ b/doc/explanation/patch.rst @@ -100,16 +100,20 @@ nothing changed: >>> patch.patch(user) False -``add`` behaves the same way. :rfc:`RFC7644 §3.5.2.1 <7644#section-3.5.2.1>` does not say what a -selection matching nothing means for it, so the operation is a no-op and not a failure. +``add`` fails like ``replace``. :rfc:`RFC7644 §3.5.2.1 <7644#section-3.5.2.1>` does not say what +a selection matching nothing means for it, and Table 9 of :rfc:`RFC7644 §3.12 <7644#section-3.12>` +defines ``noTarget`` for a filter that "yields no match". The filter selects the entries to +write. It does not describe an entry to create. So the operation fails instead of silently doing +nothing. `Errata 8097 `_ asks the RFC to say whether ``add`` accepts a value selection at all, since implementations differ on it. .. note:: - Microsoft Entra ID sends ``add`` operations whose selection matches nothing, and expects the - selected entry to be created. scim2-models does not create it, so an integration serving that - client handles the case before applying the operation. + Microsoft Entra ID sends ``add`` operations whose selection matches nothing, such as + ``emails[type eq "work"].value`` for a user without a work email, and expects the selected + entry to be created. scim2-models answers ``noTarget``, so an integration serving that client + creates the entry before applying the operation. What a remove selects --------------------- diff --git a/scim2_models/path/access.py b/scim2_models/path/access.py index f2c6654f..577ac14c 100644 --- a/scim2_models/path/access.py +++ b/scim2_models/path/access.py @@ -255,9 +255,7 @@ def _get_value(path: "Path[Any]", resource: BaseModel) -> Any: return values if target.multivalued else values[0] -def _set_selected( - path: "Path[Any]", selection: "_Selection", value: Any, *, is_add: bool = False -) -> bool: +def _set_selected(path: "Path[Any]", selection: "_Selection", value: Any) -> bool: """Apply a value to every entry matched by a value selection. A selection matching nothing raises NoTargetException. RFC7644 §3.5.2.3 @@ -270,8 +268,6 @@ def _set_selected( host, field_name, matched, sub_attr = selection if not matched: - if is_add: - return False raise NoTargetException( detail=f"no value of '{field_name}' matches the path filter" ) @@ -345,7 +341,7 @@ def _set_value( ) -> bool: """Write a value where a path designates on a resource.""" if (selection := _select(path, resource)) is not None: - return _set_selected(path, selection, value, is_add=is_add) + return _set_selected(path, selection, value) # Nothing is created to hold a null value: it would only unassign what the # new container holds, which is nothing. diff --git a/scim2_models/path/path.py b/scim2_models/path/path.py index 446c628d..d467af6e 100644 --- a/scim2_models/path/path.py +++ b/scim2_models/path/path.py @@ -389,8 +389,8 @@ def set( :raises InvalidPathException: If strict and the path does not exist or is invalid. :raises InvalidFilterException: If strict and a value selection does not apply to the attribute it selects from. - :raises NoTargetException: If strict, ``is_add`` is false and a value - selection matches nothing. + :raises NoTargetException: If strict and a value selection matches + nothing. """ try: return _set_value(self, resource, value, is_add=is_add) diff --git a/tests/test_patch_op_add.py b/tests/test_patch_op_add.py index a13142f1..1de69746 100644 --- a/tests/test_patch_op_add.py +++ b/tests/test_patch_op_add.py @@ -3,6 +3,7 @@ from scim2_models import Group from scim2_models import GroupMember +from scim2_models import NoTargetException from scim2_models import PatchOp from scim2_models import PatchOperation from scim2_models import User @@ -376,3 +377,39 @@ def test_an_operation_without_a_path_marks_the_attributes_it_wrote(): assert user.display_name == "Barbara" assert "display_name" in user.model_fields_set assert user.model_dump(exclude_unset=True)["displayName"] == "Barbara" + + +def test_add_through_a_filter_matching_nothing_has_no_target(): + """Entra expects the entry to be created, but the filter only selects entries, so noTarget.""" + user = User( + user_name="bjensen", emails=[{"value": "b@example.com", "type": "home"}] + ) + patch = PatchOp[User]( + operations=[ + { + "op": "add", + "path": 'emails[type eq "work"].value', + "value": "w@example.com", + } + ] + ) + with pytest.raises(NoTargetException): + patch.patch(user) + assert [email.value for email in user.emails] == ["b@example.com"] + + +def test_add_an_entry_through_a_filter_on_an_unassigned_attribute_has_no_target(): + """An unassigned attribute holds no entry for the filter to match.""" + user = User(user_name="bjensen") + patch = PatchOp[User]( + operations=[ + { + "op": "add", + "path": 'emails[type eq "work"]', + "value": {"value": "w@example.com"}, + } + ] + ) + with pytest.raises(NoTargetException): + patch.patch(user) + assert user.emails is None diff --git a/tests/test_path_value_selection.py b/tests/test_path_value_selection.py index 72d28134..292ac395 100644 --- a/tests/test_path_value_selection.py +++ b/tests/test_path_value_selection.py @@ -347,22 +347,27 @@ def test_a_selection_on_a_multivalued_attribute_of_an_unset_extension(): assert not path.delete(user) -def test_adding_to_a_selection_that_matches_nothing_changes_nothing(user): - """§3.5.2.1 does not define this case, and errata 8097 leaves it open. +def test_adding_to_a_selection_that_matches_nothing_has_no_target(user): + """A filter yielding no match is noTarget per §3.12, and no entry is created.""" + before = user.model_dump() + with pytest.raises(NoTargetException): + Path[User]('emails[type eq "other"].value').set( + user, "other@example.com", is_add=True + ) + assert user.model_dump() == before - Microsoft Entra ID emits exactly this operation expecting the entry to be - created, which is not what the published text says, so the operation is a - no-op rather than a failure. - """ + +def test_adding_to_a_selection_that_matches_nothing_leniently_changes_nothing(user): + """A non-strict write ignores the missing target and creates no entry.""" before = user.model_dump() assert not Path[User]('emails[type eq "other"].value').set( - user, "other@example.com", is_add=True + user, "other@example.com", is_add=True, strict=False ) assert user.model_dump() == before def test_replacing_a_selection_that_matches_nothing_has_no_target(user): - """§3.5.2.3 is the only operation the RFC requires ``noTarget`` for.""" + """§3.5.2.3 requires noTarget when the filter matches no value.""" with pytest.raises(NoTargetException): Path[User]('emails[type eq "other"].value').set(user, "x") From dd7309f0cfd6246d52b8be5d052fea74f5642cd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89loi=20Rivard?= Date: Sun, 27 Sep 2026 14:35:51 +0200 Subject: [PATCH 5/7] fix: rework how PATCH operations are applied and checked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each operation is now split into attribute writes. A value that targets a resource or a complex attribute is written key by key, so the attributes it leaves out are kept (RFC 7644 §3.5.2.3), and a filter writes into the matching entries in place. The resource is then compared before and after the operation. This checks read-only, immutable and required attributes at every level, reports a change only when something changed, and fixes primary values. Only the attributes the operation touches are copied, so large multi-valued attributes stay cheap. --- doc/changelog.rst | 36 + doc/explanation/patch.rst | 99 ++- doc/explanation/policies.rst | 13 + doc/explanation/scim-contexts.rst | 11 +- doc/how-to/build-a-patch.rst | 5 +- scim2_models/messages/patch_op.py | 999 +++++++++++---------- scim2_models/path/access.py | 48 +- scim2_models/policy.py | 8 +- tests/test_patch_op_add.py | 75 +- tests/test_patch_op_build.py | 2 +- tests/test_patch_op_extensions.py | 224 ++++- tests/test_patch_op_replace.py | 416 ++++++++- tests/test_patch_op_validation.py | 1288 +++++++++++++++++++++++++--- tests/test_path_value_selection.py | 5 +- tests/test_policy.py | 17 +- 15 files changed, 2520 insertions(+), 726 deletions(-) diff --git a/doc/changelog.rst b/doc/changelog.rst index 2b00626c..5166ca40 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -15,6 +15,10 @@ Changed on a user without a work email, now fails with ``noTarget`` instead of silently doing nothing. :meth:`Path.set ` raises :class:`~scim2_models.NoTargetException` in that case when strict. +- A PATCH ``add`` or ``replace`` on a filtered path, such as ``emails[type eq "work"]``, merges + its value into the matching entries instead of replacing them + (:rfc:`RFC7644 §3.5.2.3 <7644#section-3.5.2.3>`). The entries are updated in place, so the + immutable sub-attributes of a group member cannot be changed this way. Fixed ^^^^^ @@ -25,6 +29,38 @@ Fixed - A :class:`~scim2_models.PatchOp` with no operation, or with an operation other than ``add``, ``remove`` and ``replace``, fails with ``invalidValue`` instead of a validation error without ``scimType``. +- A PATCH ``add`` or ``replace`` on a complex attribute keeps the sub-attributes its value leaves + out, instead of replacing the whole attribute (:rfc:`RFC7644 §3.5.2.3 <7644#section-3.5.2.3>`). +- A PATCH ``add`` without a path adds to the multi-valued attributes in its value, like an + ``add`` with a path, instead of replacing their values. +- A key of a PATCH value can be an attribute path, such as ``name.givenName`` or + ``urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber``, as Microsoft + Entra ID and its SCIM Validator send. It used to be rejected as an undeclared attribute. +- Undeclared attributes and sub-attributes in a PATCH value follow + :attr:`ScimPolicy.unknown `. +- PATCH checks immutable attributes at every level and in every operation, including operations + with no path, an empty path or a schema URN, and the removal of an extension. For example, the + ``value`` of a group member can be added and removed, but not changed + (:rfc:`RFC7643 §4.2 <7643#section-4.2>`). Setting a first value with ``replace``, or writing + back the current value, is accepted. +- PATCH checks read-only attributes at every level, such as the ``displayName`` of the + enterprise ``manager``. A path to one is rejected. A value that contains one is + rejected only if it changes it, so an attribute sent back as it was read is accepted. Okta, for + example, sends back the ``id`` of a group it renames. +- PATCH rejects a change to a required attribute only when it leaves the attribute unset + (:rfc:`RFC7644 §3.5.2.2 <7644#section-3.5.2.2>`). Removing some values of a required + multi-valued attribute, removing a sub-attribute of a required complex attribute, or adding an + empty list is now accepted. Required sub-attributes and extensions are checked too. +- A PATCH ``replace`` without a value fails with ``invalidValue`` instead of clearing its + target. So does an operation whose path targets the resource or an extension with a value that + is not an object, which used to be ignored. +- A PATCH ``replace`` that sets several ``primary`` entries fails with ``invalidValue``, like + ``add`` already did, instead of keeping one of them at random. +- :meth:`PatchOp.patch ` raises + :class:`~scim2_models.InvalidValueException` when the attribute rejects a value, instead of a + pydantic :class:`~pydantic.ValidationError` without ``scimType``. +- :meth:`PatchOp.patch ` no longer reports a resource as modified + when an operation writes a complex or multi-valued value it already has. Security ^^^^^^^^ diff --git a/doc/explanation/patch.rst b/doc/explanation/patch.rst index fc1856f7..f48c0882 100644 --- a/doc/explanation/patch.rst +++ b/doc/explanation/patch.rst @@ -13,19 +13,82 @@ Validation and application Validating a PATCH message in :attr:`~scim2_models.Context.RESOURCE_PATCH_REQUEST` rejects what the message alone settles: a missing operation value, a ``remove`` without a path or carrying a -value, a read-only target, and an operation that would unassign a required attribute. +value, a path to a read-only attribute, and an operation that removes a required +attribute or sets it to an empty value. :meth:`~scim2_models.PatchOp.patch` then applies the message to the stored resource. Operations run in their listed order. This is where an immutable value can be compared with the value it -replaces, and where the method reports whether any operation changed the resource. Splitting the -two keeps a parsed :class:`~scim2_models.PatchOp` useful before the resource is loaded. +replaces, and where the method reports whether any operation changed the resource. It also +rejects an operation that unassigns a required attribute in another way, such as removing its +last entry through a filter. Read-only attributes in the value are checked there too. A client +that sends back the ``id`` or ``meta`` it read is accepted, since +:rfc:`RFC7643 §3.1 <7643#section-3.1>` says to ignore them. A client that changes them gets a +``mutability`` error. Splitting the two keeps a parsed :class:`~scim2_models.PatchOp` useful +before the resource is loaded. Operation outcomes ------------------ ``add`` appends a value to a multi-valued attribute rather than replacing its list. ``replace`` -replaces its target, creating an unassigned single-valued complex parent when necessary. -``remove`` removes a selected list entry or unassigns the targeted attribute. +replaces the value of a simple or multi-valued target. ``remove`` removes a selected list entry +or unassigns the targeted attribute. + +A path to a sub-attribute of an absent complex attribute, such as ``name.givenName`` on a user +without a name, creates that attribute. + +A complex attribute is merged rather than replaced. ``add`` and ``replace`` set the +sub-attributes in their value and keep the others, as +:rfc:`RFC7644 §3.5.2.3 <7644#section-3.5.2.3>` requires: + +.. doctest:: + + >>> from scim2_models import PatchOp, PatchOperation, User + + >>> user = User(user_name="bjensen", name={"family_name": "Jensen", "given_name": "Barbara"}) + >>> patch = PatchOp[User]( + ... operations=[ + ... PatchOperation( + ... op=PatchOperation.Op.replace_, path="name", value={"givenName": "Babs"} + ... ) + ... ] + ... ) + >>> patch.patch(user) + True + >>> user.name.given_name, user.name.family_name + ('Babs', 'Jensen') + +Operations without a path +------------------------- + +The value of an ``add`` or ``replace`` without a path holds the attributes to write. Each +attribute is handled like an operation with that attribute as its path. A key can also be an +attribute path, such as ``name.givenName`` or the full URN of an extension attribute. Microsoft +Entra ID and its SCIM Validator send such keys. :rfc:`RFC7643 §2.1 <7643#section-2.1>` forbids +dots and colons in attribute names, so such a key cannot be confused with a name: + +.. doctest:: + + >>> from scim2_models import EnterpriseUser, PatchOp, PatchOperation, User + + >>> user = User[EnterpriseUser](user_name="bjensen", name={"family_name": "Jensen"}) + >>> patch = PatchOp[User[EnterpriseUser]]( + ... operations=[ + ... PatchOperation( + ... op=PatchOperation.Op.replace_, + ... value={ + ... "name.givenName": "Barbara", + ... "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber": "42", + ... }, + ... ) + ... ] + ... ) + >>> patch.patch(user) + True + >>> user.name.given_name, user.name.family_name, user[EnterpriseUser].employee_number + ('Barbara', 'Jensen', '42') + +A key with a filter is not read as a path, and neither is a key that matches no declared +attribute. Both follow :attr:`ScimPolicy.unknown `. What a path selects ------------------- @@ -60,6 +123,32 @@ of a multi-valued attribute an operation applies to: >>> [email.value for email in user.emails] ['new@example.com', 'home@example.com'] +When a filter selects whole entries, ``add`` and ``replace`` merge their value into each selected +entry, as they do for a complex attribute. :rfc:`RFC7644 §3.5.2.3 <7644#section-3.5.2.3>` says +"all matching record values" are replaced, and keeps the sub-attributes the value does not +specify: + +.. doctest:: + + >>> patch = PatchOp[User]( + ... operations=[ + ... PatchOperation( + ... op=PatchOperation.Op.replace_, + ... path='emails[type eq "home"]', + ... value={"display": "Home"}, + ... ) + ... ] + ... ) + >>> patch.patch(user) + True + >>> user.emails[1].type.value, user.emails[1].value, user.emails[1].display + ('home', 'home@example.com', 'Home') + +Each selected entry is updated in place, not replaced by a new one. So a client can change the +``display`` of a group member without repeating its immutable ``value``. A different ``value`` is +rejected with ``mutability``. To unassign a sub-attribute, give it a null value, or remove it with +a path such as ``emails[type eq "home"].display``. + A selection matching nothing ---------------------------- diff --git a/doc/explanation/policies.rst b/doc/explanation/policies.rst index d314f2e6..296a71ab 100644 --- a/doc/explanation/policies.rst +++ b/doc/explanation/policies.rst @@ -43,6 +43,19 @@ Telling the two apart would take a notion of role, and a model has none — a re by the client that wrote it as readily as by the server that received it, so the context cannot stand in for one. +Tolerances that need no setting +------------------------------- + +A setting is worth its cost when a tolerance could confuse one payload with another. Some cases +the specification does not cover carry no such risk, and scim2-models accepts them without a +setting. + +A key of a PATCH value that is an attribute path, such as ``name.givenName``, is read as that +path. Microsoft Entra ID and its SCIM Validator send such keys. A strict reading would reject them +as unknown attributes. But :rfc:`RFC7643 §2.1 <7643#section-2.1>` forbids dots and colons in +attribute names, so the key cannot mean anything else. A setting that rejects it by default would +only break that client. :doc:`patch` describes how such a key is read. + What a policy leaves alone -------------------------- diff --git a/doc/explanation/scim-contexts.rst b/doc/explanation/scim-contexts.rst index 7fa15002..f9287df4 100644 --- a/doc/explanation/scim-contexts.rst +++ b/doc/explanation/scim-contexts.rst @@ -17,12 +17,13 @@ SCIM declares rules on attributes rather than on whole Python models: ``required`` Requires a value in creation and replacement requests. A partial PATCH request does not have - to repeat every required attribute. + to repeat every required attribute, but it cannot leave one unassigned. ``mutability`` Controls whether a client may write an attribute. ``readOnly`` is removed from creation and - replacement payloads, while a PATCH targeting it is rejected. ``immutable`` needs the current - resource value, so replacement and PATCH enforce it as the change is applied. + replacement payloads. A PATCH path to it is rejected. A PATCH value that contains it + is rejected only if it changes the current value. ``immutable`` needs the current resource + value, so replacement and PATCH enforce it as the change is applied. ``returned`` Controls response projection. ``always`` cannot be excluded, ``never`` cannot be returned, @@ -55,8 +56,8 @@ Where rules are enforced Creation and replacement validation check required values. Response serialization applies ``returned`` and attribute projection. The replacement workflow calls :meth:`~scim2_models.Resource.replace` after parsing so it can compare immutable values with the -stored resource. PATCH first validates the operation message, then checks immutable values while -applying it. +stored resource. PATCH first validates the operation message, then checks immutable, read-only and +required values on the result of each operation. :doc:`../how-to/validate-and-serialize` presents the validation and serialization procedure for each resource operation. diff --git a/doc/how-to/build-a-patch.rst b/doc/how-to/build-a-patch.rst index 34c4ddd9..11eb83c6 100644 --- a/doc/how-to/build-a-patch.rst +++ b/doc/how-to/build-a-patch.rst @@ -20,8 +20,9 @@ Pass the state the peer holds first, then the state it should hold: [{'op': 'replace', 'path': 'name.givenName', 'value': 'Babs'}] A complex attribute is compared sub-attribute by sub-attribute, and each one gets its own path. -Targeting ``name`` as a whole would replace it entirely and drop what the operation does not -carry. +Per :rfc:`RFC7644 §3.5.2.3 <7644#section-3.5.2.3>`, a ``replace`` on ``name`` keeps the +sub-attributes it does not carry. But some peers replace the whole attribute and drop them. One +path per sub-attribute gives the same result on every peer. Leave alone what the wanted state does not name ----------------------------------------------- diff --git a/scim2_models/messages/patch_op.py b/scim2_models/messages/patch_op.py index 909d6aad..3b77b7d2 100644 --- a/scim2_models/messages/patch_op.py +++ b/scim2_models/messages/patch_op.py @@ -1,3 +1,4 @@ +import copy from collections.abc import Iterator from enum import StrEnum from inspect import isclass @@ -9,10 +10,10 @@ from typing import cast from typing import get_origin -from pydantic import BaseModel as PydanticBaseModel from pydantic import Field from pydantic import SerializationInfo from pydantic import SerializerFunctionWrapHandler +from pydantic import ValidationError from pydantic import ValidationInfo from pydantic import field_validator from pydantic import model_serializer @@ -23,20 +24,21 @@ from ..attributes import ComplexAttribute from ..base import BaseModel from ..context import Context +from ..exceptions import InvalidPathException from ..exceptions import InvalidValueException from ..exceptions import MutabilityException from ..exceptions import NoTargetException -from ..exceptions import PathNotFoundException +from ..exceptions import SCIMException from ..path import Path from ..path import ScimFilter -from ..path import attribute_host +from ..path.access import _select +from ..path.access import _set_values from ..policy import ScimPolicy from ..policy import _effective_policy from ..policy import _policy from ..resources.resource import Resource from ..urn import URN from ..utils import UNION_TYPES -from ..utils import _find_field_name from .message import Message from .message import _get_resource_class from .message import _ResourceParameterized @@ -44,170 +46,6 @@ ResourceT = TypeVar("ResourceT", bound=Resource[Any]) -def _commit(resource: Any, working: Any) -> None: - """Write a patched copy back onto the resource the caller holds. - - Assignment is bypassed on purpose: ``working`` was built by the very passes - ``validate_assignment`` would run again. - """ - resource.__dict__.clear() - resource.__dict__.update(working.__dict__) - resource.__pydantic_fields_set__.clear() - resource.__pydantic_fields_set__.update(working.__pydantic_fields_set__) - resource.__pydantic_private__ = working.__pydantic_private__ - - -def _targeted_attributes(value: Any) -> dict[str, Any]: - """Return the attributes an operation without a path writes. - - RFC7644 §3.5.2.3 has the ``value`` name them when the path is omitted. A - client building its payload in Python passes a resource, where a server - parses a mapping, and both name the same attributes. Anything else names - none. - """ - if isinstance(value, BaseModel): - # Dumped out of context on purpose: a payload dumped in the PATCH - # context already leaves read-only attributes out, and an operation - # naming one is to be refused rather than quietly trimmed. - return value.model_dump(exclude_unset=True) - return value if isinstance(value, dict) else {} - - -def _resolved_field(resource_class: type[BaseModel], attr_name: str) -> str | None: - """Return the Python field a SCIM attribute name designates. - - Attribute names are case-insensitive per RFC7643 §2.1 and differ from the - field names of the model, so the constraint checks resolve the name instead - of matching it against ``model_fields``. - """ - return _find_field_name(resource_class, attr_name) - - -_ENVELOPE_FIELDS = frozenset({"schemas"}) -"""Fields that carry the payload rather than the state it describes.""" - - -def _asserted_sub_attributes(entries: Any) -> set[str]: - """Return the sub-attributes the entries of a wanted state name.""" - asserted: set[str] = set() - for entry in entries or []: - if isinstance(entry, BaseModel): - asserted |= entry.model_fields_set - return asserted - - -def _projection(entries: Any, asserted: set[str]) -> list[Any]: - """Reduce the entries of a multi-valued attribute to what is worth comparing. - - RFC7643 §2.4 gives no significance to the order of a multi-valued - attribute, so the projections are sorted before comparison. - """ - projected = [ - tuple(sorted((name, getattr(entry, name, None)) for name in asserted)) - if isinstance(entry, BaseModel) - else entry - for entry in entries or [] - ] - return sorted(projected, key=repr) - - -def _operation( - path: str, old: Any, new: Any, mutability: Mutability | None -) -> tuple["PatchOperation.Op", str, Any]: - """Return the operation writing *new* where the current state holds *old*. - - Called once a difference is established. RFC7644 §3.5.2.3 has a service - provider treat a ``replace`` on an unset target as an ``add``, so a single - operation covers both. An immutable attribute is the exception: RFC7644 - §3.5.2 lets a client add a value to one that had none, and nothing else. - """ - if mutability == Mutability.immutable: - if old is not None: - raise MutabilityException( - attribute=path, mutability="immutable", operation="replace" - ) - return PatchOperation.Op.add, path, new - - if new is None or new == []: - return PatchOperation.Op.remove, path, None - - return PatchOperation.Op.replace_, path, new - - -def _diff_multi_valued( - path: str, old: Any, new: Any, mutability: Mutability | None -) -> Iterator[tuple["PatchOperation.Op", str, Any]]: - """Diff a multi-valued attribute, which is replaced as a whole. - - Only the sub-attributes the wanted entries name take part in the - comparison, so the sub-attributes the peer alone maintains do not read as a - difference. When the collection does change it is replaced entirely: - RFC7643 §2.4 gives the entries no identity, so an entry that changed cannot - be told from a removed one and an added one. - """ - asserted = _asserted_sub_attributes(new) - if _projection(old, asserted) == _projection(new, asserted): - return - - yield _operation(path, old, new, mutability) - - -def _diff_sub_object( - prefix: str, - path: str, - old: Any, - new: Any, - mutability: Mutability | None, -) -> Iterator[tuple["PatchOperation.Op", str, Any]]: - """Diff a complex attribute or an extension, one sub-attribute at a time.""" - if new is not None: - yield from _diff(old, new, prefix) - return - - if old is not None: - yield _operation(path, old, None, mutability) - - -def _diff( - before: Any, after: Any, prefix: str = "" -) -> Iterator[tuple["PatchOperation.Op", str, Any]]: - """Yield the operations turning *before* into *after*. - - Only the attributes *after* names are candidates: what a wanted state never - mentions is left to the peer. Attributes are visited in declaration order, - so a diff is reproducible. - """ - model = type(after) - info = model.__scim_info__ - for field_name in model.model_fields: - if field_name not in after.model_fields_set: - continue - - if field_name in _ENVELOPE_FIELDS: - continue - - mutability = model.get_field_annotation(field_name, Mutability) - if mutability == Mutability.read_only: - continue - - old = getattr(before, field_name, None) if before is not None else None - new = getattr(after, field_name, None) - path = f"{prefix}{model._scim_name(field_name)}" - - if model.get_field_multiplicity(field_name): - yield from _diff_multi_valued(path, old, new, mutability) - - elif field_name in info.extensions: - urn = info.attribute_urns[field_name] - yield from _diff_sub_object(f"{urn}:", urn, old, new, mutability) - - elif field_name in info.complex_fields: - yield from _diff_sub_object(f"{path}.", path, old, new, mutability) - - elif old != new: - yield _operation(path, old, new, mutability) - - class PatchOperation(ComplexAttribute, Generic[ResourceT]): class Op(StrEnum): replace_ = "replace" @@ -229,96 +67,30 @@ class Op(StrEnum): """The "path" attribute value is a String containing an attribute path describing the target of the operation.""" - def _validate_mutability( - self, resource_class: type[BaseModel], field_name: str - ) -> None: - """Validate mutability constraints at parse-time. - - Only scim2_models.Mutability.read_only is validated here. - scim2_models.Mutability.immutable validation requires access to the - resource instance and is enforced at runtime in - PatchOp._check_immutable. - """ - mutability = resource_class.get_field_annotation(field_name, Mutability) - - if mutability == Mutability.read_only: - raise MutabilityException( - attribute=field_name, mutability="readOnly", operation=self.op.value - ).as_pydantic_error() + value: Any | None = None - def _validate_required_attribute( - self, - resource_class: type[BaseModel], - field_name: str, - written: Any = None, - ) -> None: - """Refuse an operation that would leave a required attribute unassigned. + @model_validator(mode="after") + def _validate_request_operation(self, info: ValidationInfo) -> Self: + """Reject an operation that lacks a member RFC7644 §3.5.2 requires. - ``written`` is the value the operation writes to that attribute, which - an operation without a path takes from its ``value``. + A remove needs a path. An add needs a value, and null is not one. A + replace needs a value, and null unassigns the target. """ - # RFC7643 §2.5 makes a null value, an empty array and an unassigned - # attribute equivalent in state, so writing one of those unassigns the - # attribute as surely as a remove does. - if self.op == PatchOperation.Op.remove: - detail = "required attribute cannot be removed" - elif self.op in ( - PatchOperation.Op.replace_, - PatchOperation.Op.add, - ) and written in (None, []): - detail = "required attribute cannot be unassigned" - else: - return - - required = resource_class.get_field_annotation(field_name, Required) - - # RFC7644 §3.5.2.2 has a server answer "mutability" when a required - # attribute is removed or becomes unassigned. - if required == Required.true: - raise MutabilityException( - detail=detail, attribute=field_name, operation=self.op.value - ).as_pydantic_error() - - @model_validator(mode="after") - def _validate_operation_requirements(self, info: ValidationInfo) -> Self: - """Validate operation requirements according to RFC 7644.""" - # Only validate in PATCH request context - scim_ctx = info.context.get("scim") if info.context else None - if scim_ctx != Context.RESOURCE_PATCH_REQUEST: + if not _in_patch_request(info): return self - - # RFC 7644 Section 3.5.2.2: "If 'path' is unspecified, the operation - # fails with HTTP status code 400 and a 'scimType' error of 'noTarget'" - if self.path is None and self.op == PatchOperation.Op.remove: + if self.op == PatchOperation.Op.remove and self.path is None: raise NoTargetException( detail="Remove operation requires a path" ).as_pydantic_error() - - # RFC 7644 Section 3.5.2.2 defines a remove by its path alone: the four - # target locations it lists all read off "path", and a selection is - # spelled as a filter there. An operation carrying a value is thus - # incompatible with the schema of the attribute it targets, which - # Section 3.5.2 answers with an error. - if ( - self.op == PatchOperation.Op.remove - and self.value is not None - and _policy(info).remove_value_as_filter != ScimPolicy.RemoveValue.apply + if (self.op == PatchOperation.Op.add and self.value is None) or ( + self.op == PatchOperation.Op.replace_ + and "value" not in self.model_fields_set ): raise InvalidValueException( - detail="a remove operation carries no value, " - "a filter in the path selects what to remove" + detail=f"value is required for {self.op.value} operations" ).as_pydantic_error() - - # RFC 7644 Section 3.5.2.1: "Value is required for add operations" - if self.op == PatchOperation.Op.add and self.value is None: - raise InvalidValueException( - detail="value is required for add operations" - ).as_pydantic_error() - return self - value: Any | None = None - @model_serializer(mode="wrap") def _scim_serializer( self, handler: SerializerFunctionWrapHandler, info: SerializationInfo @@ -406,62 +178,25 @@ def __class_getitem__(cls, item: Any) -> Any: @model_validator(mode="after") def _validate_operations(self, info: ValidationInfo) -> Self: """Reject the errors the operations have on any resource of the type.""" - # RFC 7644: The body of an HTTP PATCH request MUST contain the attribute "Operations" - scim_ctx = info.context.get("scim") if info.context else None - if scim_ctx == Context.RESOURCE_PATCH_REQUEST and self.operations is None: - raise InvalidValueException( - detail="operations attribute is required" - ).as_pydantic_error() - - if self.operations == []: + if self.operations is None: + if _in_patch_request(info): + raise InvalidValueException( + detail="operations attribute is required" + ).as_pydantic_error() + return self + if not self.operations: raise InvalidValueException( detail="operations holds one or more operations" ).as_pydantic_error() - resource_class = _get_resource_class(self) - if resource_class is None or not self.operations: - return self - - for operation in self.operations: - if operation.path is None: - # §3.5.2.1 and §3.5.2.3: "If the path parameter is omitted, the - # target is assumed to be the resource itself", the value naming - # the attributes to write. Each of them is a target of its own, - # and answers to §3.5.2 as a named path does. - for attr_name, written in _targeted_attributes(operation.value).items(): - field_name = _resolved_field(resource_class, attr_name) - if field_name is None: - # §3.5.2 has an operation that is not compatible with an - # attribute's schema return an error, and §3.12 defines - # invalidValue for a value "not compatible with [...] the - # resource schema". There is no path here to call invalid. - raise InvalidValueException( - detail=f"attribute '{attr_name}' is not declared by the resource schema" - ).as_pydantic_error() - operation._validate_mutability(resource_class, field_name) - operation._validate_required_attribute( - resource_class, field_name, written - ) - continue - - # The attribute a qualified path applies to is declared by the - # extension the URN designates, not by the resource, so the checks - # resolve the path instead of reading its first segment. They read - # the attribute the path applies to rather than the sub-attribute it - # 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( - resolved.model, resolved.field_name, operation.value - ) - + model = _get_resource_class(self) + try: + for operation in self.operations: + _check_operation( + model, operation, _policy(info), in_request=_in_patch_request(info) + ) + except SCIMException as exc: + raise exc.as_pydantic_error() from exc return self @classmethod @@ -503,16 +238,18 @@ def build_from( # index of a generic as a type, not as a value. model = type(after) operation_class: Any = PatchOperation.__class_getitem__(model) - path_class = Path.__class_getitem__(model) operations = [ - operation_class(op=op, path=path_class(path), value=value) - for op, path, value in _diff(before, after) + operation_class(op=op, path=path, value=value) + for op, path, value in _differences(before, after, "") ] if not operations: return None patch_class = PatchOp.__class_getitem__(model) - return cast("PatchOp[ResourceT]", patch_class(operations=operations)) + patch = cast("PatchOp[ResourceT]", patch_class(operations=operations)) + # Apply to a copy, so that a patch the peer would reject fails here. + patch.patch(before.model_copy(deep=True)) + return patch def patch(self, resource: ResourceT, scim_policy: ScimPolicy | None = None) -> bool: """Apply all PATCH operations to the given SCIM resource in sequence. @@ -530,255 +267,483 @@ def patch(self, resource: ResourceT, scim_policy: ScimPolicy | None = None) -> b The operations are applied as a whole: when one fails, the resource is left as it was. The resource object itself is kept, but the values it - holds are replaced, so a reference taken on one of them beforehand no - longer reflects the resource. + holds are then copies of the ones it held, so a reference taken on one + of them beforehand no longer reflects the resource. :param resource: The SCIM resource to patch. This object is modified in-place. :param scim_policy: The :class:`~scim2_models.ScimPolicy` the patch is applied under. Defaults to the strict reading of the specification. :return: True if the resource was modified by any operation, False otherwise. - :raises InvalidValueException: If multiple values are marked as primary in a single - operation, or if multiple primary values already exist before the patch. + :raises InvalidValueException: If a value is not compatible with the type of + the attribute it is written to, if multiple values are marked as primary + in a single operation, or if multiple primary values already exist + before the patch. + :raises MutabilityException: If an operation changes an immutable value, + changes a read-only value through its value, or leaves a required + attribute unassigned. + :raises NoTargetException: If the path filter of an ``add`` or + ``replace`` matches no value. """ - if not self.operations: - return False - - modified = False - # §3.5.2 has a failing operation leave the resource as it was, and an - # operation only fails once tried: a filter selecting nothing is known - # from the state, not from the payload. - working = resource.model_copy(deep=True) - - # The policy is made ambient for the whole application: the passes it - # governs below are revalidations that start from no call of ours. - with _effective_policy(scim_policy): - # RFC 7644 Section 3.5.2: "Apply each operation in sequence" - for operation in self.operations: - if self._apply_operation(working, operation): - modified = True + snapshot = resource.model_copy(deep=True) + try: + # The policy is made ambient for the whole application: the passes + # it governs below are revalidations that start from no call of ours. + with _effective_policy(scim_policy): + modified = [ + _apply_operation(resource, operation, _effective_policy()) + for operation in self.operations or [] + ] + except Exception: + _restore(resource, snapshot) + raise + return any(modified) - _commit(resource, working) - return modified - def _apply_operation( - self, resource: Resource[Any], operation: PatchOperation[ResourceT] - ) -> bool: - """Apply one operation as RFC7644 §3.5.2 defines it, then check the result.""" - if operation.path is not None: - self._check_immutable(resource, operation) +def _in_patch_request(info: ValidationInfo) -> bool: + """Whether a validation reads the payload of a PATCH request.""" + return (info.context or {}).get("scim") == Context.RESOURCE_PATCH_REQUEST - if operation.op in (PatchOperation.Op.add, PatchOperation.Op.replace_): - return self._apply_add_replace(resource, operation) - if operation.op == PatchOperation.Op.remove: - return self._apply_remove(resource, operation) - raise InvalidValueException(detail=f"unsupported operation: {operation.op}") +def _is_model(type_: Any) -> bool: + """Whether a type is a model, whose values hold attributes.""" + return isclass(type_) and issubclass(type_, BaseModel) - def _check_immutable( - self, resource: Resource[Any], operation: PatchOperation[ResourceT] - ) -> None: - """Validate immutable constraints at runtime. - RFC 7644 §3.5.2: +def _as_payload(value: Any) -> Any: + """Return the payload a value built in Python would be sent as.""" + if isinstance(value, BaseModel): + return value.model_dump(scim_ctx=Context.DEFAULT) + return value + + +def _check_operation( + model: type[Resource[Any]], + operation: PatchOperation[Any], + policy: ScimPolicy, + *, + in_request: bool, +) -> None: + """Reject the errors an operation has on any resource. + + The path must target a declared attribute that is not read-only, and must + not remove or unassign a required attribute. The attributes in a value are + checked against the resource when the patch is applied. + """ + path = Path.__class_getitem__(model)(operation.path or "") + removal = operation.op == PatchOperation.Op.remove + if removal and in_request: + _removal_path(path, operation.value, policy) + + if path.model is None: + binding = path.resolve() + if binding is None: + raise InvalidPathException( + path=str(path), detail=f"'{path}' is not an attribute of the schema" + ) + mutabilities = ( + binding.model.get_field_annotation(binding.field_name, Mutability), + binding.get_annotation(Mutability), + ) + if Mutability.read_only in mutabilities: + raise MutabilityException( + attribute=str(path), detail=f"'{path}' is read-only" + ) - *"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."* + if not removal: + for write_path, value in _writes(path, operation.value, policy): + if value is None or (value == [] and operation.op != PatchOperation.Op.add): + _refuse_unassigning(write_path, "unassigned") + return - 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. - """ - assert operation.path is not None - if (resolved := operation.path.resolve()) is None: - return - field_name = resolved.field_name + if operation.path is not None: + _refuse_unassigning(path, "removed") - mutability = resolved.model.get_field_annotation(field_name, Mutability) - if mutability != Mutability.immutable: - return - host = attribute_host(resource, resolved) - current_value = getattr(host, field_name, None) +def _refuse_unassigning(path: Path[Any], verb: str) -> None: + """Reject an operation that unassigns a required attribute (RFC7644 §3.5.2.2). - if operation.op == PatchOperation.Op.add and current_value is None: - return + A sub-attribute is only required when its parent is assigned, and a + selection may leave values behind, so both are checked when the patch is + applied. + """ + binding = path.resolve() + if binding is None or binding.sub_field_name or path.value_filter is not None: + return + if ( + binding.model.get_field_annotation(binding.field_name, Required) + == Required.true + ): + raise MutabilityException( + attribute=str(path), detail=f"required attribute cannot be {verb}" + ) - if operation.op == PatchOperation.Op.remove and current_value is None: - return - if ( - operation.op == PatchOperation.Op.replace_ - and operation.value == current_value - ): - return +def _removal_path(path: Path[Any], value: Any, policy: ScimPolicy) -> Path[Any] | None: + """Return the path a remove deletes, or None when its value selects nothing. - raise MutabilityException( - attribute=field_name, - mutability="immutable", - operation=operation.op.value, + RFC7644 §3.5.2.2 reads the target of a remove from its path only. Under the + apply policy, the value lists the entries to remove by their sub- + attributes, as Microsoft Entra sends it. + """ + if value is None: + return path + if policy.remove_value_as_filter == ScimPolicy.RemoveValue.forbid: + raise InvalidValueException( + detail="a remove operation carries no value, " + "a filter in the path selects what to remove" ) - - def _apply_add_replace( - self, resource: Resource[Any], operation: PatchOperation[ResourceT] - ) -> bool: - """Apply an add or replace operation.""" - before_state = self._capture_primary_state(resource) - - value = operation.value - if operation.path is None and isinstance(value, BaseModel): - # Path("").set writes the attributes a mapping names, so a resource - # given as a value is dumped to the payload it stands for, and to - # the very attributes the operation was checked against. - value = _targeted_attributes(value) - - path = operation.path if operation.path is not None else Path("") - modified = path.set( - resource, # type: ignore[arg-type] - value, - is_add=operation.op == PatchOperation.Op.add, + if path.model is not None: + raise InvalidPathException( + path=str(path), + detail="a remove selecting values by its value needs " + "a path to the attribute holding them", ) - if modified: - self._normalize_primary_after_patch(resource, before_state) - - return modified - - def _capture_primary_state(self, resource: Resource[Any]) -> dict[str, set[int]]: - """Capture indices of elements with primary=True for each multi-valued attribute.""" - state: dict[str, set[int]] = {} - for field_name in type(resource).model_fields: - if not resource.get_field_multiplicity(field_name): - continue - - field_value = getattr(resource, field_name, None) - if not field_value: - continue - - element_type = resource.get_field_root_type(field_name) - if ( - not element_type - or not isclass(element_type) - or not issubclass(element_type, PydanticBaseModel) - or "primary" not in element_type.model_fields - ): - continue - - primary_indices = { - i - for i, item in enumerate(field_value) - if getattr(item, "primary", None) is True - } - state[field_name] = primary_indices - - return state - - def _normalize_primary_after_patch( - self, resource: Resource[Any], before_state: dict[str, set[int]] - ) -> None: - """Normalize primary attributes after a patch operation. - - Per RFC 7644 §3.5.2: a PATCH operation that sets a value's "primary" - sub-attribute to "true" SHALL cause the server to automatically set - "primary" to "false" for any other values. - """ - for field_name in type(resource).model_fields: - if not resource.get_field_multiplicity(field_name): - continue - - field_value = getattr(resource, field_name, None) - if not field_value: - continue - - element_type = resource.get_field_root_type(field_name) - if ( - not element_type - or not isclass(element_type) - or not issubclass(element_type, PydanticBaseModel) - or "primary" not in element_type.model_fields - ): - continue - - current_primary_indices = { - i - for i, item in enumerate(field_value) - if getattr(item, "primary", None) is True - } - - if len(current_primary_indices) <= 1: - continue - - before_primaries = before_state.get(field_name, set()) - new_primaries = current_primary_indices - before_primaries - - if len(new_primaries) > 1: - raise InvalidValueException( - detail=f"Multiple values marked as primary in field '{field_name}'" - ) + entries = value if isinstance(value, list) else [value] + if path.value_filter is not None or not all( + isinstance(entry, dict) and entry for entry in entries + ): + raise InvalidValueException( + detail="the value of a remove must be non-empty objects, " + "on a path without a filter" + ) + if not entries: + return None + selection = " or ".join( + " and ".join( + f"{name} eq {ScimFilter.quote(item)}" for name, item in entry.items() + ) + for entry in entries + ) + return type(path)(f"{path}[{selection}]") - if not new_primaries: - raise InvalidValueException( - detail=f"Multiple primary values already exist in field '{field_name}'" - ) - keep_index = next(iter(new_primaries)) - for i in current_primary_indices: - if i != keep_index: - field_value[i].primary = False +def _apply_operation( + resource: Resource[Any], operation: PatchOperation[Any], policy: ScimPolicy +) -> bool: + """Apply one operation as RFC7644 §3.5.2 defines it, then check the result.""" + if not isinstance(operation.op, PatchOperation.Op): + raise InvalidValueException(detail=f"{operation.op!r} is not a PATCH operation") + path = Path.__class_getitem__(type(resource))(operation.path or "") - def _apply_remove( - self, resource: Resource[Any], operation: PatchOperation[ResourceT] - ) -> bool: - """Apply a remove operation.""" + removal = None + if operation.op == PatchOperation.Op.remove: if operation.path is None: raise NoTargetException(detail="Remove operation requires a path") + removal = _removal_path(path, operation.value, policy) + writes = [] + touched = [] if removal is None else [removal] + else: + writes = list(_writes(path, operation.value, policy)) + touched = [path, *(write_path for write_path, _ in writes)] + + memo: dict[int, Any] = {} + before = _snapshot(resource, touched, memo) + try: + if operation.op == PatchOperation.Op.remove: + if removal is not None: + removal.delete(resource) + else: + _write(resource, path, writes, is_add=operation.op == PatchOperation.Op.add) + except ValidationError as exc: + raise InvalidValueException(detail=str(exc)) from exc + _settle(before, resource, memo) + return not all( + _same(getattr(before, name), getattr(resource, name)) + for name in type(resource).model_fields + ) - # Checked again here, a PatchOp built in Python reaching no validator. - if operation.value is not None: - if ( - _effective_policy().remove_value_as_filter - != ScimPolicy.RemoveValue.apply - ): - raise InvalidValueException( - detail="a remove operation carries no value, " - "a filter in the path selects what to remove" - ) - return self._remove_selected_values( - resource, operation.path, operation.value + +def _write( + resource: Resource[Any], + path: Path[Any], + writes: list[tuple[Path[Any], Any]], + *, + is_add: bool, +) -> None: + """Write the value of an add or a replace. + + The path filter is resolved even when the value writes nothing, so a filter + that matches nothing is always reported. + """ + if not writes: + selection = _select(path, resource) + if selection is not None and not selection.matched: + raise NoTargetException( + detail=f"no value of '{selection.field_name}' matches the path filter" ) + _set_values(resource, writes, is_add=is_add) - return operation.path.delete(resource) # type: ignore[arg-type] - def _remove_selected_values( - self, resource: Resource[Any], path: Path[ResourceT], value: Any - ) -> bool: - """Remove the entries the value of a remove operation selects. +def _writes( + path: Path[Any], value: Any, policy: ScimPolicy +) -> Iterator[tuple[Path[Any], Any]]: + """Split the value of an add or a replace into single writes. - Microsoft Entra puts the selection in ``value`` where RFC7644 §3.5.2.2 - puts it in ``path``. Each entry becomes a filter on the sub-attributes - it names, which is the path the operation should have carried. - """ - if path.value_filter is not None: - raise InvalidValueException( - detail="a remove operation carrying a value cannot also " - "select values in its path" - ) + A value for a model or a complex attribute is written key by key, so the + attributes it leaves out are kept, as RFC7644 §3.5.2.3 requires. A key can + be an attribute path. Any other value is written at the path. + """ + value = _as_payload(value) + prefix = _key_prefix(path) + if prefix is None or (path.model is None and not isinstance(value, dict)): + yield path, value + return + if not isinstance(value, dict): + raise InvalidValueException( + detail=f"the value written to '{path}' must be an object" + ) - entries = value if isinstance(value, list) else [value] - if not all(isinstance(entry, dict) and entry for entry in entries): - raise InvalidValueException( - detail="the value of a remove operation names the " - "sub-attributes selecting what to remove" + for key, item in value.items(): + if key.lower() == "schemas": + continue + key_path = _declared(path, prefix + key, policy) + if key_path is not None and key_path.model is not None and item is None: + yield key_path, None + elif key_path is not None: + yield from _writes(key_path, item, policy) + + +def _key_prefix(path: Path[Any]) -> str | None: + """Return the path the keys of a value are relative to. + + The keys of a value for a model are attribute names or paths. The keys of a + value for a complex attribute, or for the entries a filter selects, are + sub-attribute names. Any other target takes a plain value, and gets None. + """ + if path.model is not None: + return "" if path.model is path.models[0] else f"{path}:" + + binding = path.resolve() + if ( + binding is None + or binding.sub_field_name is not None + or not _is_model(binding.field_type) + or (binding.is_multivalued and path.value_filter is None) + ): + return None + return f"{path._as_value_path() or path}." + + +def _declared(parent: Path[Any], text: str, policy: ScimPolicy) -> Path[Any] | None: + """Return the path a key of a value stands for. + + The key must match a declared attribute, without a filter. Otherwise it is + dropped under a tolerant policy, or rejected with invalidValue, which + RFC7644 §3.12 defines for a value that does not fit the schema. + """ + try: + key_path = type(parent)(text) + except InvalidPathException: + key_path = None + if ( + key_path is not None + and key_path.value_filter == parent.value_filter + and (key_path.model or key_path.resolve()) + ): + return key_path + if policy.unknown != ScimPolicy.Unknown.forbid: + return None + raise InvalidValueException( + detail=f"'{text}' is not declared by the resource schema" + ) + + +def _snapshot( + resource: Resource[Any], paths: list[Path[Any]], memo: dict[int, Any] +) -> Resource[Any]: + """Copy the attributes an operation may change, and share the others. + + The memo maps each object to its copy, which pairs each entry of a multi- + valued attribute with its earlier state. When no path reaches into the + entries of a multi-valued attribute, through a filter or a sub-attribute, + entries can only be added or removed, so they are shared instead of copied. + """ + reached: dict[str, bool] = {} + for path in paths: + binding = None if path.model is not None else path.resolve() + model = path.model if binding is None else binding.model + if model is None or model is type(resource): + if binding is not None: + reaches = binding.sub_field_name is not None or bool(path.value_filter) + name = binding.field_name + reached[name] = reached.get(name, False) or reaches + else: + reached[model.__name__] = True + + before = copy.copy(resource) + for name, reaches in reached.items(): + value = getattr(resource, name) + if isinstance(value, list) and not reaches: + memo.update({id(entry): entry for entry in value}) + before.__dict__[name] = list(value) + else: + before.__dict__[name] = copy.deepcopy(value, memo) + return before + + +def _settle(before: Any, after: Any, memo: dict[int, Any]) -> None: + """Compare the state after an operation with the state before it. + + Each attribute is compared with its earlier value, and each entry of a + multi-valued attribute with its earlier state, as the memo pairs them. Per + RFC7644 §3.5.2, a read-only attribute cannot change, an immutable one + cannot change once assigned, and a required one cannot become unassigned. + Removing an extension or a complex attribute may remove its read-only and + required attributes, but not its assigned immutable ones. A value marked + primary unmarks the others. + """ + if before is after: + return + removed = after is None + model = type(before if removed else after) + for field_name in model.model_fields: + old = getattr(before, field_name, None) + new = getattr(after, field_name, None) + if _same(old, new): + continue + + name = model._scim_name(field_name) + mutability = model.get_field_annotation(field_name, Mutability) + if mutability == Mutability.immutable and _assigned(old): + raise MutabilityException(attribute=name, detail=f"'{name}' is immutable") + if not removed and mutability == Mutability.read_only: + raise MutabilityException(attribute=name, detail=f"'{name}' is read-only") + if ( + not removed + and model.get_field_annotation(field_name, Required) == Required.true + and _assigned(old) + and not _assigned(new) + ): + raise MutabilityException( + attribute=name, detail=f"required attribute '{name}' became unassigned" ) - removed = False - for entry in entries: - conditions = " and ".join( - f"{name} eq {ScimFilter.quote(item)}" for name, item in entry.items() + for old_value, new_value in _counterparts(old, new, memo): + _settle(old_value, new_value, memo) + if isinstance(new, list): + _settle_primary(new, memo) + + +def _counterparts(old: Any, new: Any, memo: dict[int, Any]) -> list[tuple[Any, Any]]: + """Pair the objects an attribute holds with their earlier state. + + Entries of a multi-valued attribute have no identity other than the object, + so a removed entry is not visited, and an added entry has no pair. + """ + if isinstance(new, list): + return [ + (memo.get(id(entry)), entry) + for entry in new + if isinstance(entry, BaseModel) + ] + if isinstance(old, BaseModel) or isinstance(new, BaseModel): + return [(old, new)] + return [] + + +def _settle_primary(entries: list[Any], memo: dict[int, Any]) -> None: + """Keep the value just marked primary as the only primary one. + + Per RFC7644 §3.5.2, a value marked primary unmarks the others, and per + RFC7643 §2.4, only one value can be primary. + """ + primary = [ + entry + for entry in entries + if "primary" in getattr(type(entry), "model_fields", {}) + and entry.primary is True + ] + marked = [ + entry + for entry in primary + if getattr(memo.get(id(entry)), "primary", None) is not True + ] + if len(marked) > 1: + raise InvalidValueException(detail="Multiple values marked as primary") + if not marked and len(primary) > 1: + raise InvalidValueException(detail="Multiple primary values already exist") + for entry in primary: + if marked and entry is not marked[0]: + entry.primary = False + + +def _same(old: Any, new: Any) -> bool: + """Whether a value is unchanged, the order of entries aside (RFC7643 §2.4).""" + if old == new: + return True + if not (isinstance(old, list) and isinstance(new, list)) or len(old) != len(new): + return False + return bool(_comparable(old) == _comparable(new)) + + +def _comparable(value: Any, include: set[str] | None = None) -> Any: + """Reduce a value to a form that compares equal regardless of entry order.""" + if isinstance(value, list): + return sorted((_comparable(item, include) for item in value), key=repr) + if isinstance(value, BaseModel): + return value.model_dump(include=include) + return value + + +def _assigned(value: Any) -> bool: + """Whether a value is assigned, as RFC7643 §2.5 defines it.""" + if isinstance(value, BaseModel): + return any(_assigned(getattr(value, name)) for name in type(value).model_fields) + return value is not None and value != [] + + +def _restore(resource: BaseModel, snapshot: BaseModel) -> None: + """Restore a resource from a copy, keeping the same object.""" + for attribute in ("__dict__", "__pydantic_fields_set__", "__pydantic_private__"): + object.__setattr__(resource, attribute, getattr(snapshot, attribute)) + + +def _differences( + before: Any, after: BaseModel, prefix: str +) -> Iterator[tuple[PatchOperation.Op, str, Any]]: + """Yield the operations that set the attributes of the wanted state. + + A complex attribute or an extension is compared attribute by attribute. A + multi-valued attribute is compared on the sub-attributes its wanted entries + name. + """ + model = type(after) + for field_name in model.model_fields: + mutability = model.get_field_annotation(field_name, Mutability) + if ( + field_name not in after.model_fields_set + or field_name == "schemas" + or mutability == Mutability.read_only + ): + continue + + old = getattr(before, field_name, None) + new = getattr(after, field_name) + path = f"{prefix}{model._scim_name(field_name)}" + include = _named_sub_attributes(new) + if isinstance(new, BaseModel): + separator = ":" if field_name in model.__scim_info__.extensions else "." + yield from _differences(old, new, path + separator) + elif not _assigned(new): + if _assigned(old): + yield PatchOperation.Op.remove, path, None + elif _comparable(old, include) != _comparable(new, include): + op = ( + PatchOperation.Op.add + if mutability == Mutability.immutable and old is None + else PatchOperation.Op.replace_ ) - # Subscripted through the call the syntax stands for: mypy reads - # the index of a generic as a type, not as a value. - selection = Path.__class_getitem__(type(resource))(f"{path}[{conditions}]") - removed = selection.delete(resource) or removed - return removed + yield op, path, new + + +def _named_sub_attributes(value: Any) -> set[str] | None: + """Return the sub-attributes the entries of a wanted collection name.""" + if not isinstance(value, list): + return None + return { + name + for entry in value + if isinstance(entry, BaseModel) + for name in entry.model_fields_set + } diff --git a/scim2_models/path/access.py b/scim2_models/path/access.py index 577ac14c..bef940b1 100644 --- a/scim2_models/path/access.py +++ b/scim2_models/path/access.py @@ -273,19 +273,23 @@ def _set_selected(path: "Path[Any]", selection: "_Selection", value: Any) -> boo ) if sub_attr is None: - # Without a sub-attribute the matched entries are replaced wholesale. - current = getattr(host, field_name) - replacement = list(current) new_value = _as_entry(type(host), field_name, value) - modified = False - for index, item in enumerate(replacement): - if any(item is candidate for candidate in matched): - if not _values_match(item, new_value): - replacement[index] = new_value - modified = True - if modified: - setattr(host, field_name, replacement) - return modified + changed = [item for item in matched if not _values_match(item, new_value)] + if not changed: + return False + if isinstance(new_value, BaseModel): + for item in changed: + item.__dict__.update(new_value.__dict__) + object.__setattr__( + item, "__pydantic_fields_set__", set(new_value.model_fields_set) + ) + return True + replacement = [ + new_value if any(item is entry for entry in changed) else item + for item in getattr(host, field_name) + ] + setattr(host, field_name, replacement) + return True modified = False for item in matched: @@ -343,8 +347,6 @@ def _set_value( if (selection := _select(path, resource)) is not None: return _set_selected(path, selection, value) - # Nothing is created to hold a null value: it would only unassign what the - # new container holds, which is nothing. target = _walk(path, resource, create=value is not None) if target is None: return False @@ -358,6 +360,22 @@ def _set_value( return any(changed) +def _set_values( + resource: BaseModel, writes: list[tuple["Path[Any]", Any]], *, is_add: bool = False +) -> None: + """Write several values, resolving every selection before writing anything. + + A value may write the sub-attributes a filter compares. Resolving first + keeps the rest of the value writing to the same entries. + """ + selections = [_select(path, resource) for path, _ in writes] + for (path, value), selection in zip(writes, selections, strict=True): + if selection is None: + _set_value(path, resource, value, is_add=is_add) + else: + _set_selected(path, selection, value) + + def _merge(path: "Path[Any]", obj: BaseModel, value: Any, *, explicit: bool) -> bool: """Write the attributes a mapping names onto the object the path designates. @@ -388,7 +406,7 @@ def _set_field_value(obj: BaseModel, field_name: str, value: Any, is_add: bool) """Set or add a value to a field.""" is_multivalued = obj.get_field_multiplicity(field_name) - if is_add and is_multivalued: + if is_add and is_multivalued and value is not None: current_list = getattr(obj, field_name) or [] entries = [ _as_entry(type(obj), field_name, item) diff --git a/scim2_models/policy.py b/scim2_models/policy.py index 49e4bd1e..beed1df6 100644 --- a/scim2_models/policy.py +++ b/scim2_models/policy.py @@ -24,7 +24,9 @@ class ScimPolicy(BaseModel): on the wire, symmetric, and defined by the specification; the second is local, and nothing on the wire announces it. - Every setting defaults to the strict reading of the specification. + Every setting defaults to the strict reading of the specification. A + tolerance that cannot confuse one payload with another, such as a PATCH + value key that is an attribute path, needs no setting. >>> from scim2_models import ScimPolicy >>> ScimPolicy().unknown is ScimPolicy.Unknown.forbid @@ -77,7 +79,9 @@ class Unknown(StrEnum): """The payload is accepted and the attribute is dumped back. The original spelling is preserved. No attribute characteristic is - declared for it, so no context filters it out. + declared for it, so no context filters it out. In the value of a PATCH + operation, the attribute has no field to be written to and is dropped + as ``ignore`` drops it. """ class RemoveValue(StrEnum): diff --git a/tests/test_patch_op_add.py b/tests/test_patch_op_add.py index 1de69746..313ba162 100644 --- a/tests/test_patch_op_add.py +++ b/tests/test_patch_op_add.py @@ -3,6 +3,7 @@ from scim2_models import Group from scim2_models import GroupMember +from scim2_models import InvalidValueException from scim2_models import NoTargetException from scim2_models import PatchOp from scim2_models import PatchOperation @@ -305,18 +306,6 @@ def test_add_operation_no_path_with_invalid_attribute(): assert user.nick_name == "Test" -def test_add_operation_with_non_dict_value_no_path(): - """Test add operation with no path and non-dict value should return False.""" - user = User() - patch = PatchOp[User]( - operations=[ - PatchOperation[User](op=PatchOperation.Op.add, value="invalid_value") - ] - ) - result = patch.patch(user) - assert result is False - - def test_add_a_subattribute_to_every_entry(): """An unfiltered path designates the sub-attribute of each entry.""" user = User( @@ -351,7 +340,7 @@ def test_a_rejected_addition_leaves_the_attribute_untouched(): ) ] ) - with pytest.raises(ValidationError): + with pytest.raises(InvalidValueException): patch.patch(user) assert user.emails == [User.Emails(value="bjensen@example.com")] @@ -379,6 +368,66 @@ def test_an_operation_without_a_path_marks_the_attributes_it_wrote(): assert user.model_dump(exclude_unset=True)["displayName"] == "Barbara" +@pytest.mark.parametrize( + "path", [None, "", "urn:ietf:params:scim:schemas:core:2.0:User"] +) +def test_an_add_with_a_multi_valued_attribute_in_its_value_adds_to_it(path): + """RFC7644 §3.5.2.1 adds an email to the resource in its example of an add without a path.""" + user = User(user_name="bjensen", emails=[{"value": "bjensen@example.com"}]) + operation = {"op": "add", "value": {"emails": [{"value": "babs@example.com"}]}} + if path is not None: + operation["path"] = path + + PatchOp[User].model_validate({"Operations": [operation]}).patch(user) + + assert [email.value for email in user.emails] == [ + "bjensen@example.com", + "babs@example.com", + ] + + +def test_an_add_with_an_empty_list_in_its_value_leaves_the_attribute(): + """An add of an empty list adds nothing, with or without a path.""" + user = User(user_name="bjensen", emails=[{"value": "bjensen@example.com"}]) + patch = PatchOp[User].model_validate( + {"Operations": [{"op": "add", "value": {"emails": []}}]} + ) + + assert patch.patch(user) is False + assert [email.value for email in user.emails] == ["bjensen@example.com"] + + +def test_an_add_with_schemas_in_its_value_leaves_them_to_the_resource(): + """The schemas of a resource follow the attributes it holds, not the payload writing them.""" + user = User(user_name="bjensen") + patch = PatchOp[User].model_validate( + { + "Operations": [ + { + "op": "add", + "value": {"schemas": ["urn:example:foo"], "nickName": "Babs"}, + } + ] + } + ) + + patch.patch(user) + + assert user.schemas == ["urn:ietf:params:scim:schemas:core:2.0:User"] + assert user.nick_name == "Babs" + + +def test_an_add_setting_a_multi_valued_attribute_to_null_unassigns_it(): + """RFC7643 §2.5 makes null the state of an attribute holding no value.""" + user = User(user_name="bjensen", emails=[{"value": "bjensen@example.com"}]) + patch = PatchOp[User].model_validate( + {"Operations": [{"op": "add", "value": {"emails": None}}]} + ) + + assert patch.patch(user) + assert user.emails is None + + def test_add_through_a_filter_matching_nothing_has_no_target(): """Entra expects the entry to be created, but the filter only selects entries, so noTarget.""" user = User( diff --git a/tests/test_patch_op_build.py b/tests/test_patch_op_build.py index db78232c..b2ca0cd3 100644 --- a/tests/test_patch_op_build.py +++ b/tests/test_patch_op_build.py @@ -63,7 +63,7 @@ def test_an_attribute_the_wanted_state_does_not_name_is_left_alone(): def test_a_changed_sub_attribute_is_targeted_by_its_own_path(): - """Targeting name as a whole would replace it entirely and drop the sub-attributes the operation does not carry.""" + """A peer that replaces name as a whole would drop the sub-attributes the operation leaves out.""" before = User(name=Name(given_name="Barbara", family_name="Jensen")) after = User(name=Name(given_name="Babs", family_name="Jensen")) diff --git a/tests/test_patch_op_extensions.py b/tests/test_patch_op_extensions.py index e3aba465..8dd18009 100644 --- a/tests/test_patch_op_extensions.py +++ b/tests/test_patch_op_extensions.py @@ -5,10 +5,12 @@ from scim2_models import URN from scim2_models import Group -from scim2_models import InvalidPathException +from scim2_models import MutabilityException from scim2_models import PatchOp from scim2_models import PatchOperation +from scim2_models import ScimPolicy from scim2_models import User +from scim2_models.context import Context from scim2_models.resources.enterprise_user import EnterpriseUser from scim2_models.resources.resource import Resource @@ -97,7 +99,7 @@ def test_patch_operation_extension_complex_attribute(): path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager", value={ "value": "super-manager-789", - "displayName": "Alice Johnson", + "displayName": "John Smith", "$ref": "https://example.com/Users/super-manager-789", }, ) @@ -106,7 +108,7 @@ def test_patch_operation_extension_complex_attribute(): result = patch2.patch(user) assert result is True assert user[EnterpriseUser].manager.value == "super-manager-789" - assert user[EnterpriseUser].manager.display_name == "Alice Johnson" + assert user[EnterpriseUser].manager.display_name == "John Smith" patch3 = PatchOp[User[EnterpriseUser]]( operations=[ @@ -304,24 +306,6 @@ def test_patch_main_schema_path_without_attribute(): assert user.title == "Manager" -def test_patch_schema_path_with_invalid_value_type(): - """Test PATCH with schema URN path and invalid value type (non-dict).""" - user = User(user_name="test") - - patch = PatchOp[User]( - operations=[ - PatchOperation[User]( - op=PatchOperation.Op.add, - path="urn:ietf:params:scim:schemas:core:2.0:User", - value="invalid string value", - ) - ] - ) - - with pytest.raises(InvalidPathException): - patch.patch(user) - - def test_patch_delete_extension_root(): """Test PATCH remove operation targeting the root of an extension.""" user = User[EnterpriseUser].model_validate( @@ -352,3 +336,201 @@ def test_patch_delete_extension_root(): result = patch.patch(user) assert result is True assert user[EnterpriseUser] is None + + +MANAGER = "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager" + + +def _managed_user(): + return User[EnterpriseUser].model_validate( + { + "userName": "jane.doe", + MANAGER.rsplit(":", 1)[0]: { + "manager": {"value": "manager-123", "displayName": "John Smith"} + }, + } + ) + + +def test_a_path_to_a_read_only_sub_attribute_is_refused(): + """RFC7644 §3.5.2 forbids changing a read-only attribute, and this path points to one.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User[EnterpriseUser]].model_validate( + { + "Operations": [ + { + "op": "replace", + "path": f"{MANAGER}.displayName", + "value": "Alice", + } + ] + } + ) + + assert raised.value.errors()[0]["type"] == "scim_mutability" + + +@pytest.mark.parametrize( + ("path", "value"), + [ + (MANAGER, {"value": "manager-456", "displayName": "Alice"}), + (None, {MANAGER.rsplit(":", 1)[0]: {"manager": {"displayName": "Alice"}}}), + ], +) +def test_a_value_changing_a_read_only_sub_attribute_is_refused(path, value): + """A read-only sub-attribute in a value is rejected when it would change.""" + user = _managed_user() + operation = {"op": "replace", "value": value} + if path is not None: + operation["path"] = path + patch = PatchOp[User[EnterpriseUser]].model_validate({"Operations": [operation]}) + + with pytest.raises(MutabilityException): + patch.patch(user) + + assert user[EnterpriseUser].manager.display_name == "John Smith" + + +def test_a_complex_attribute_holding_a_read_only_sub_attribute_may_be_removed(): + """Removing an attribute also removes its read-only sub-attributes.""" + user = _managed_user() + patch = PatchOp[User[EnterpriseUser]].model_validate( + {"Operations": [{"op": "remove", "path": MANAGER}]} + ) + + assert patch.patch(user) + assert user[EnterpriseUser].manager is None + + +def test_a_new_complex_value_cannot_assign_a_read_only_sub_attribute(): + """A client cannot set a read-only sub-attribute, even on an attribute that had no value.""" + user = User[EnterpriseUser](user_name="jane.doe") + patch = PatchOp[User[EnterpriseUser]].model_validate( + { + "Operations": [ + { + "op": "add", + "path": MANAGER, + "value": {"value": "manager-123", "displayName": "John Smith"}, + } + ] + } + ) + + with pytest.raises(MutabilityException): + patch.patch(user) + + +ENTERPRISE_URN = "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User" + + +def _keyed_patch(op, value, **kwargs): + return PatchOp[User[EnterpriseUser]].model_validate( + {"Operations": [{"op": op, "value": value}]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + **kwargs, + ) + + +def _keyed_user(): + user = User[EnterpriseUser](user_name="bjensen", name={"familyName": "Jensen"}) + user[EnterpriseUser] = EnterpriseUser( + manager={"value": "m1", "displayName": "Boss"} + ) + return user + + +@pytest.mark.parametrize("op", ["add", "replace"]) +def test_a_dotted_key_writes_its_sub_attribute(op): + """Entra uses paths as keys in a value without a path, and no attribute name looks like a path.""" + user = _keyed_user() + + assert _keyed_patch(op, {"name.givenName": "Barbara"}).patch(user) + assert user.name.given_name == "Barbara" + assert user.name.family_name == "Jensen" + + +@pytest.mark.parametrize("op", ["add", "replace"]) +def test_a_urn_qualified_key_writes_its_extension_attribute(op): + """The Microsoft SCIM Validator uses the full URN of extension attributes as keys.""" + user = _keyed_user() + + assert _keyed_patch(op, {f"{ENTERPRISE_URN}:employeeNumber": "42"}).patch(user) + assert user[EnterpriseUser].employee_number == "42" + + +def test_a_urn_qualified_key_merges_into_its_complex_attribute(): + """A complex attribute written through a URN key keeps the sub-attributes its value leaves out.""" + user = _keyed_user() + + assert _keyed_patch( + "replace", {f"{ENTERPRISE_URN}:manager": {"value": "m2"}} + ).patch(user) + assert user[EnterpriseUser].manager.value == "m2" + assert user[EnterpriseUser].manager.display_name == "Boss" + + +def test_a_dotted_key_inside_an_extension_is_read_from_the_extension(): + """A key of the value an extension takes is a path relative to that extension.""" + user = _keyed_user() + + assert _keyed_patch("replace", {ENTERPRISE_URN: {"manager.value": "m2"}}).patch( + user + ) + assert user[EnterpriseUser].manager.value == "m2" + + +def test_a_key_qualified_by_the_resource_urn_writes_the_core_attribute(): + """The core schema URN qualifies the attributes of the resource as well.""" + user = _keyed_user() + + assert _keyed_patch( + "replace", {"urn:ietf:params:scim:schemas:core:2.0:User:nickName": "Babs"} + ).patch(user) + assert user.nick_name == "Babs" + + +@pytest.mark.parametrize( + "key", + [ + pytest.param("name.nickName", id="undeclared sub-attribute"), + pytest.param("urn:example:2.0:Unknown:attr", id="undeclared extension"), + pytest.param('emails[type eq "work"].value', id="filter"), + pytest.param("display name", id="malformed"), + ], +) +def test_a_key_spelling_no_declared_attribute_path_is_refused(key): + """A key that is neither an attribute name nor a declared path is undeclared.""" + with pytest.raises(ValidationError) as raised: + _keyed_patch("replace", {key: "x"}) + + assert raised.value.errors()[0]["type"] == "scim_invalidValue" + + +def test_a_key_spelling_no_declared_attribute_path_is_dropped_under_a_tolerant_policy(): + """The unknown policy handles such a key like any undeclared attribute.""" + user = _keyed_user() + tolerant = ScimPolicy(unknown=ScimPolicy.Unknown.ignore) + patch = _keyed_patch("replace", {"name.nickName": "x"}, scim_policy=tolerant) + + assert not patch.patch(user, scim_policy=tolerant) + + +def test_a_dotted_key_cannot_change_a_read_only_sub_attribute(): + """A path used as a key has the same constraints as in the path field.""" + user = _keyed_user() + patch = _keyed_patch("replace", {f"{ENTERPRISE_URN}:manager.displayName": "Other"}) + + with pytest.raises(MutabilityException): + patch.patch(user) + assert user[EnterpriseUser].manager.display_name == "Boss" + + +def test_a_qualified_key_cannot_unassign_a_required_attribute(): + """Unassigning a required attribute is rejected however the key spells it.""" + with pytest.raises(ValidationError) as raised: + _keyed_patch( + "replace", {"urn:ietf:params:scim:schemas:core:2.0:User:userName": None} + ) + + assert raised.value.errors()[0]["type"] == "scim_mutability" diff --git a/tests/test_patch_op_replace.py b/tests/test_patch_op_replace.py index 06ef7c75..9eb570ee 100644 --- a/tests/test_patch_op_replace.py +++ b/tests/test_patch_op_replace.py @@ -1,12 +1,19 @@ from typing import Annotated import pytest +from pydantic import ValidationError from scim2_models import URN from scim2_models import EnterpriseUser +from scim2_models import Group +from scim2_models import InvalidFilterException +from scim2_models import InvalidValueException from scim2_models import MutabilityException +from scim2_models import Name +from scim2_models import NoTargetException from scim2_models import PatchOp from scim2_models import PatchOperation +from scim2_models import ScimPolicy from scim2_models import User from scim2_models.annotations import Mutability from scim2_models.resources.resource import Resource @@ -192,19 +199,6 @@ def test_replace_operation_no_path_same_attributes(): assert user.display_name == "Display" -def test_replace_operation_with_non_dict_value_no_path(): - """Test replace operation with no path and non-dict value should return False.""" - user = User(nick_name="Test") - patch = PatchOp[User]( - operations=[ - PatchOperation[User](op=PatchOperation.Op.replace_, value="invalid_value") - ] - ) - result = patch.patch(user) - assert result is False - assert user.nick_name == "Test" - - def test_immutable_field(): """Test that replace operations on immutable fields raise mutability errors.""" @@ -416,3 +410,399 @@ def test_unassigning_under_an_unassigned_container_leaves_the_resource(path): assert patch.patch(user) is False assert user.name is None assert user[EnterpriseUser] is None + + +def _jensen(): + return User( + user_name="bjensen", name=Name(given_name="Barbara", family_name="Jensen") + ) + + +COMPLEX_WRITES = [ + pytest.param("name", {"givenName": "Babs"}, id="attribute path"), + pytest.param( + "urn:ietf:params:scim:schemas:core:2.0:User:name", + {"givenName": "Babs"}, + id="qualified path", + ), + pytest.param(None, {"name": {"givenName": "Babs"}}, id="no path"), + pytest.param("", {"name": {"givenName": "Babs"}}, id="empty path"), +] + + +@pytest.mark.parametrize("op", ["add", "replace"]) +@pytest.mark.parametrize(("path", "value"), COMPLEX_WRITES) +def test_a_partial_complex_value_leaves_the_other_sub_attributes(op, path, value): + """RFC7644 §3.5.2.3 keeps the sub-attributes the value does not specify.""" + user = _jensen() + operation = {"op": op, "value": value} + if path is not None: + operation["path"] = path + + assert PatchOp[User].model_validate({"Operations": [operation]}).patch(user) + + assert user.name == Name(given_name="Babs", family_name="Jensen") + + +def test_a_null_sub_attribute_in_a_complex_value_unassigns_it_alone(): + """A null value unassigns only the sub-attribute it is given to.""" + user = _jensen() + patch = PatchOp[User].model_validate( + { + "Operations": [ + {"op": "replace", "path": "name", "value": {"givenName": None}} + ] + } + ) + + patch.patch(user) + + assert user.name == Name(family_name="Jensen") + + +def test_a_complex_value_on_an_unassigned_attribute_assigns_it(): + """Per RFC7644 §3.5.2.3, a replace on a missing attribute adds it.""" + user = User(user_name="bjensen") + patch = PatchOp[User].model_validate( + { + "Operations": [ + {"op": "replace", "path": "name", "value": {"givenName": "Babs"}} + ] + } + ) + + assert patch.patch(user) + assert user.name == Name(given_name="Babs") + + +def test_an_undeclared_sub_attribute_in_a_complex_value_is_refused(): + """RFC7644 §3.12 defines invalidValue for a value that does not fit the resource schema.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User].model_validate( + {"Operations": [{"op": "add", "path": "name", "value": {"bogus": 1}}]} + ) + + assert raised.value.errors()[0]["type"] == "scim_invalidValue" + + +def test_a_tolerant_policy_drops_an_undeclared_sub_attribute_alone(): + """The sub-attributes the value leaves out are kept when the undeclared one is dropped.""" + user = _jensen() + tolerant = ScimPolicy(unknown=ScimPolicy.Unknown.ignore) + patch = PatchOp[User].model_validate( + {"Operations": [{"op": "add", "path": "name", "value": {"bogus": 1}}]}, + scim_policy=tolerant, + ) + + assert patch.patch(user, scim_policy=tolerant) is False + assert user.name == Name(given_name="Barbara", family_name="Jensen") + + +def _user_with_work_email(): + return User( + user_name="bjensen", + emails=[ + {"value": "bjensen@example.com", "type": "work", "display": "Work"}, + {"value": "babs@example.com", "type": "home"}, + ], + ) + + +def test_a_replace_selecting_entries_merges_the_value_into_them(): + """RFC7644 §3.5.2.3 keeps the sub-attributes a value does not specify.""" + user = _user_with_work_email() + patch = PatchOp[User].model_validate( + { + "Operations": [ + { + "op": "replace", + "path": 'emails[type eq "work"]', + "value": {"value": "barbara@example.com"}, + } + ] + } + ) + + assert patch.patch(user) + + assert user.emails[0].value == "barbara@example.com" + assert user.emails[0].type == "work" + assert user.emails[0].display == "Work" + assert user.emails[1].value == "babs@example.com" + + +def test_a_replace_selecting_entries_unassigns_a_sub_attribute_given_null(): + """Per RFC7643 §2.5, a null value unassigns the sub-attribute it is given to.""" + user = _user_with_work_email() + patch = PatchOp[User].model_validate( + { + "Operations": [ + { + "op": "replace", + "path": 'emails[type eq "work"]', + "value": {"display": None}, + } + ] + } + ) + + assert patch.patch(user) + + assert user.emails[0].display is None + assert user.emails[0].value == "bjensen@example.com" + + +def test_a_replace_selecting_a_member_changes_it_without_repeating_its_value(): + """A client does not have to repeat the immutable value its filter used to select the member.""" + group = Group( + display_name="Tour Guides", + members=[{"value": "2819c223", "display": "Babs Jensen"}], + ) + patch = PatchOp[Group].model_validate( + { + "Operations": [ + { + "op": "replace", + "path": 'members[value eq "2819c223"]', + "value": {"display": "Barbara Jensen"}, + } + ] + } + ) + + assert patch.patch(group) + assert group.members[0].value == "2819c223" + assert group.members[0].display == "Barbara Jensen" + + +def test_an_entry_replaced_through_a_selection_keeps_its_identity(): + """Entries are only told apart by identity, so a replaced entry stays the same object.""" + user = _user_with_work_email() + entry = user.emails[0] + patch = PatchOp[User].model_validate( + { + "Operations": [ + { + "op": "replace", + "path": 'emails[type eq "work"]', + "value": {"value": "barbara@example.com"}, + } + ] + } + ) + + patch.patch(user) + + assert user.emails[0] is entry + + +def test_an_add_selecting_entries_merges_the_value_into_them(): + """An add writes the sub-attributes in its value and leaves the others.""" + user = _user_with_work_email() + patch = PatchOp[User].model_validate( + { + "Operations": [ + { + "op": "add", + "path": 'emails[type eq "work"]', + "value": {"display": "Office"}, + } + ] + } + ) + + assert patch.patch(user) + + assert user.emails[0].value == "bjensen@example.com" + assert user.emails[0].type == "work" + assert user.emails[0].display == "Office" + + +def test_an_add_selecting_entries_with_the_values_they_hold_changes_nothing(): + """Per RFC7644 §3.5.2.1, adding a value already held changes nothing.""" + user = _user_with_work_email() + patch = PatchOp[User].model_validate( + { + "Operations": [ + { + "op": "add", + "path": 'emails[type eq "work"]', + "value": {"display": "Work"}, + } + ] + } + ) + + assert patch.patch(user) is False + + +def test_a_replace_selecting_a_member_cannot_change_its_immutable_value(): + """RFC7643 §4.2 makes member sub-attributes immutable, even when the whole entry is written.""" + group = Group(display_name="Tour Guides", members=[{"value": "2819c223"}]) + patch = PatchOp[Group].model_validate( + { + "Operations": [ + { + "op": "replace", + "path": 'members[value eq "2819c223"]', + "value": {"value": "c3a26dd3"}, + } + ] + } + ) + + with pytest.raises(MutabilityException): + patch.patch(group) + + +class Tagged(Resource): + __schema__ = URN("urn:example:2.0:Tagged") + + tags: list[str] | None = None + + +def test_a_replace_selecting_simple_values_replaces_them(): + """The values of a multi-valued attribute without sub-attributes are selected by their value.""" + resource = Tagged(tags=["red", "blue"]) + patch = PatchOp[Tagged].model_validate( + { + "Operations": [ + {"op": "replace", "path": 'tags[value eq "red"]', "value": "green"} + ] + } + ) + + assert patch.patch(resource) + assert resource.tags == ["green", "blue"] + + +def test_a_replace_selecting_entries_refuses_a_value_that_is_no_entry(): + """RFC7644 §3.12 returns invalidValue for a value that does not fit the attribute type.""" + user = _user_with_work_email() + patch = PatchOp[User].model_validate( + { + "Operations": [ + {"op": "replace", "path": 'emails[type eq "work"]', "value": "x"} + ] + } + ) + + with pytest.raises(InvalidValueException): + patch.patch(user) + + +def test_a_replace_selecting_simple_values_with_the_one_they_hold_changes_nothing(): + """Writing the value already held modifies nothing.""" + resource = Tagged(tags=["red", "blue"]) + patch = PatchOp[Tagged].model_validate( + { + "Operations": [ + {"op": "replace", "path": 'tags[value eq "red"]', "value": "red"} + ] + } + ) + + assert patch.patch(resource) is False + + +@pytest.mark.parametrize("path", [None, "emails"]) +def test_a_replace_writing_the_entries_held_reports_no_change(path): + """A patch leaving the resource as it was reports no modification.""" + user = _user_with_work_email() + entries = [ + {"value": "bjensen@example.com", "type": "work", "display": "Work"}, + {"value": "babs@example.com", "type": "home"}, + ] + operation = ( + {"op": "replace", "value": {"emails": entries}} + if path is None + else {"op": "replace", "path": path, "value": entries} + ) + + assert ( + PatchOp[User].model_validate({"Operations": [operation]}).patch(user) is False + ) + + +def test_a_replace_bringing_two_primary_entries_is_refused(): + """RFC7643 §2.4 allows primary to be true on one value at most.""" + user = User( + user_name="bjensen", + emails=[{"value": "bjensen@example.com", "primary": True}], + ) + patch = PatchOp[User].model_validate( + { + "Operations": [ + { + "op": "replace", + "path": "emails", + "value": [ + {"value": "babs@example.com", "primary": True}, + {"value": "barbara@example.com", "primary": True}, + ], + } + ] + } + ) + + with pytest.raises(InvalidValueException): + patch.patch(user) + + assert [email.value for email in user.emails] == ["bjensen@example.com"] + + +def test_a_replace_bringing_one_primary_entry_keeps_it(): + """A replaced list holding a single primary entry keeps it primary.""" + user = User( + user_name="bjensen", + emails=[{"value": "bjensen@example.com", "primary": True}], + ) + patch = PatchOp[User].model_validate( + { + "Operations": [ + { + "op": "replace", + "path": "emails", + "value": [ + {"value": "babs@example.com"}, + {"value": "barbara@example.com", "primary": True}, + ], + } + ] + } + ) + + assert patch.patch(user) + assert [email.primary for email in user.emails] == [None, True] + + +@pytest.mark.parametrize("op", ["add", "replace"]) +@pytest.mark.parametrize( + "value", [{}, {"whatever": "x"}], ids=["empty", "only-undeclared"] +) +def test_a_value_writing_nothing_still_needs_a_target(op, value): + """The filter is resolved even when the value writes nothing to the entries.""" + user = User( + user_name="bjensen", emails=[{"value": "b@example.com", "type": "home"}] + ) + patch = PatchOp[User].model_validate( + {"Operations": [{"op": op, "path": 'emails[type eq "work"]', "value": value}]}, + scim_policy=ScimPolicy(unknown=ScimPolicy.Unknown.ignore), + ) + + with pytest.raises(NoTargetException): + patch.patch(user, scim_policy=ScimPolicy(unknown=ScimPolicy.Unknown.ignore)) + + +@pytest.mark.parametrize("op", ["add", "replace"]) +def test_a_value_writing_nothing_still_needs_a_valid_filter(op): + """A filter on an undeclared sub-attribute is rejected, whatever the value.""" + user = User( + user_name="bjensen", emails=[{"value": "b@example.com", "type": "home"}] + ) + patch = PatchOp[User].model_validate( + {"Operations": [{"op": op, "path": 'emails[whatever eq "x"]', "value": {}}]} + ) + + with pytest.raises(InvalidFilterException): + patch.patch(user) diff --git a/tests/test_patch_op_validation.py b/tests/test_patch_op_validation.py index 876ef020..f078840c 100644 --- a/tests/test_patch_op_validation.py +++ b/tests/test_patch_op_validation.py @@ -1,3 +1,5 @@ +from datetime import UTC +from datetime import datetime from typing import Annotated from typing import TypeVar @@ -5,6 +7,7 @@ from pydantic import ValidationError from scim2_models import URN +from scim2_models import ComplexAttribute from scim2_models import Extension from scim2_models import Group from scim2_models import InvalidFilterException @@ -12,9 +15,13 @@ from scim2_models import InvalidValueException from scim2_models import Mutability from scim2_models import MutabilityException +from scim2_models import NoTargetException from scim2_models import PatchOp from scim2_models import PatchOperation +from scim2_models import Path +from scim2_models import PathNotFoundException from scim2_models import Required +from scim2_models import ScimPolicy from scim2_models import User from scim2_models.base import Context from scim2_models.resources.resource import Resource @@ -24,6 +31,24 @@ class ImmutableFieldResource(Resource): locked: Annotated[str | None, Mutability.immutable] = None +class ConstrainedComplex(ComplexAttribute): + label: str | None = None + + +class ConstrainedResource(Resource): + __schema__ = URN("urn:example:2.0:ConstrainedResource") + + read_only_attr: Annotated[str | None, Mutability.read_only] = None + immutable_attr: Annotated[str | None, Mutability.immutable] = None + immutable_complex: Annotated[ConstrainedComplex | None, Mutability.immutable] = None + immutable_date: Annotated[datetime | None, Mutability.immutable] = None + immutable_list: Annotated[list[ConstrainedComplex] | None, Mutability.immutable] = ( + None + ) + required_attr: Annotated[str | None, Required.true] = None + plain_attr: str | None = None + + class ConstrainedExtension(Extension): __schema__ = URN("urn:example:2.0:Constrained") @@ -152,7 +177,7 @@ def test_path_required_for_remove_operations(): { "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], "operations": [ - {"op": "replace", "value": "foobar"}, + {"op": "replace", "value": {"nickName": "foobar"}}, ], }, context={"scim": Context.RESOURCE_PATCH_REQUEST}, @@ -161,7 +186,7 @@ def test_path_required_for_remove_operations(): { "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], "operations": [ - {"op": "add", "value": "foobar"}, + {"op": "add", "value": {"nickName": "foobar"}}, ], }, context={"scim": Context.RESOURCE_PATCH_REQUEST}, @@ -186,15 +211,6 @@ def test_value_required_for_add_operations(): :rfc:`RFC7644 §3.5.2.1 <7644#section-3.5.2.1>`: "The operation MUST contain a 'value' member whose content specifies the value to be added." """ - PatchOp[User].model_validate( - { - "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], - "operations": [ - {"op": "replace", "path": "nickName"}, - ], - }, - context={"scim": Context.RESOURCE_PATCH_REQUEST}, - ) with pytest.raises(ValidationError): PatchOp[User].model_validate( { @@ -406,45 +422,6 @@ def test_remove_operation_on_non_required_field_allowed(): ) -def test_patch_operations_with_none_path_skipped(): - """Test that patch operations with None path are skipped during validation.""" - # Create a patch operation with None path (bypassing normal validation) - patch_op = PatchOp[User]( - operations=[ - PatchOperation.model_construct( - op=PatchOperation.Op.add, path=None, value="test" - ) - ] - ) - - # The _validate_operations method should skip operations with None path - # This should not raise an error - user = User(user_name="test") - result = patch_op.patch(user) - assert result is False # Should return False because operation was skipped - - -def test_patch_operation_with_schema_only_urn_path(): - """Test patch operation with URN path that contains only schema.""" - # Test edge case where extract_field_name returns None for schema-only URNs - user = User(user_name="test") - - # This URN resolves to just the schema without an attribute name - patch = PatchOp[User]( - operations=[ - PatchOperation[User]( - op=PatchOperation.Op.add, - path="urn:ietf:params:scim:schemas:core:2.0:User", - value="test", - ) - ] - ) - - # This should trigger the path where extract_field_name returns None - with pytest.raises(InvalidPathException): - patch.patch(user) - - def test_add_remove_operations_on_group_members_allowed(): """Test that add/remove operations work on group collections.""" # Test operations on group collection (not the immutable value field) @@ -475,7 +452,7 @@ def test_patch_error_handling_no_operations(): def test_patch_error_handling_type_mismatch(): - """Test error handling when patch value type doesn't match field type.""" + """RFC7644 §3.12 returns invalidValue for a value that does not fit the attribute type.""" user = User(user_name="test") # Try to set active (boolean) to a string @@ -487,9 +464,11 @@ def test_patch_error_handling_type_mismatch(): ] ) - with pytest.raises(ValidationError): + with pytest.raises(InvalidValueException) as raised: patch.patch(user) + assert raised.value.scim_type == "invalidValue" + T = TypeVar("T", bound=Resource) UserT = TypeVar("UserT", bound=User) @@ -729,32 +708,79 @@ def test_a_replace_may_unassign_an_optional_attribute(): assert user.display_name is None -def test_an_operation_without_path_cannot_write_a_read_only_attribute(): - """:rfc:`RFC7644 §3.5.2.3 <7644#section-3.5.2.3>` has the value name the attributes to write when the path is omitted, so each of them answers to §3.5.2 like a named path.""" - with pytest.raises(ValidationError, match="mutability"): - PatchOp[User].model_validate( - { - "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], - "Operations": [ - {"op": "replace", "value": {"id": "chosen-by-the-client"}} - ], - }, - scim_ctx=Context.RESOURCE_PATCH_REQUEST, - ) +def test_an_operation_without_path_cannot_change_a_read_only_attribute(): + """RFC7644 §3.5.2 forbids changing a read-only attribute, here set in the value.""" + patch = PatchOp[User].model_validate( + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [{"op": "replace", "value": {"id": "chosen-by-the-client"}}], + }, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + user = User(id="srv-1", user_name="bjensen") + with pytest.raises(MutabilityException): + patch.patch(user) + assert user.id == "srv-1" -def test_an_operation_without_path_cannot_write_a_read_only_complex_attribute(): - """Naming a complex attribute is enough to refuse it, without descending into its sub-attributes.""" - with pytest.raises(ValidationError, match="mutability"): - PatchOp[User].model_validate( - { - "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], - "Operations": [ - {"op": "replace", "value": {"meta": {"resourceType": "Group"}}} - ], - }, - scim_ctx=Context.RESOURCE_PATCH_REQUEST, - ) + +def test_an_operation_without_path_cannot_assign_a_read_only_complex_attribute(): + """A client cannot set a read-only complex attribute that has no value.""" + patch = PatchOp[User].model_validate( + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + {"op": "replace", "value": {"meta": {"resourceType": "Group"}}} + ], + }, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + user = User(id="srv-1", user_name="bjensen") + + with pytest.raises(MutabilityException): + patch.patch(user) + assert user.meta is None + + +def test_an_operation_without_path_ignores_a_read_only_attribute_sent_back_unchanged(): + """Okta sends back the id of a group it renames, and RFC7643 §3.1 says to ignore it.""" + patch = PatchOp[Group].model_validate( + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + {"op": "replace", "value": {"id": "grp-1", "displayName": "Admins"}} + ], + }, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + group = Group(id="grp-1", display_name="Staff") + + assert patch.patch(group) + assert group.id == "grp-1" + assert group.display_name == "Admins" + + +def test_an_operation_without_path_ignores_a_resource_sent_back_as_it_was_read(): + """The read-only meta of a dumped resource compares equal once parsed, whatever its spelling.""" + user = User( + id="srv-1", + user_name="bjensen", + meta={ + "resourceType": "User", + "created": datetime(2020, 1, 1, tzinfo=UTC), + "version": 'W/"1"', + }, + ) + value = user.model_dump(scim_ctx=Context.RESOURCE_QUERY_RESPONSE) + value["nickName"] = "Babs" + patch = PatchOp[User].model_validate( + {"Operations": [{"op": "replace", "value": value}]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + assert patch.patch(user) + assert user.nick_name == "Babs" + assert user.meta.version == 'W/"1"' def test_an_operation_without_path_cannot_unassign_a_required_attribute(): @@ -771,17 +797,32 @@ def test_an_operation_without_path_cannot_unassign_a_required_attribute(): ) -def test_an_add_without_path_cannot_write_a_read_only_attribute(): - """:rfc:`RFC7644 §3.5.2.1 <7644#section-3.5.2.1>` gives ``add`` the same omitted path as ``replace``.""" - with pytest.raises(ValidationError, match="mutability"): +def test_an_add_without_path_cannot_change_a_read_only_attribute(): + """RFC7644 §3.5.2.1 lets add omit the path, as replace does.""" + patch = PatchOp[User].model_validate( + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [{"op": "add", "value": {"id": "chosen-by-the-client"}}], + }, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + user = User(id="srv-1", user_name="bjensen") + + with pytest.raises(MutabilityException): + patch.patch(user) + assert user.id == "srv-1" + + +def test_a_path_to_a_read_only_attribute_is_refused_even_unchanged(): + """A path to a read-only attribute targets it on purpose, whatever the value.""" + with pytest.raises(ValidationError) as raised: PatchOp[User].model_validate( - { - "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], - "Operations": [{"op": "add", "value": {"id": "chosen-by-the-client"}}], - }, + {"Operations": [{"op": "replace", "path": "id", "value": "srv-1"}]}, scim_ctx=Context.RESOURCE_PATCH_REQUEST, ) + assert raised.value.errors()[0]["type"] == "scim_mutability" + def test_an_operation_without_path_refuses_an_undeclared_attribute(): """§3.5.2 has an operation incompatible with an attribute's schema return an error. @@ -891,15 +932,19 @@ def test_an_operation_without_path_takes_a_model_as_value(): def test_a_model_value_answers_to_the_same_constraints_as_a_dict_one(): """The form the value takes says nothing about what the operation may write.""" - with pytest.raises(ValidationError, match="mutability"): - PatchOp[User]( - operations=[ - PatchOperation[User]( - op=PatchOperation.Op.replace_, - value=User(id="chosen-by-the-client"), - ) - ] - ) + patch = PatchOp[User]( + operations=[ + PatchOperation[User]( + op=PatchOperation.Op.replace_, + value=User(id="chosen-by-the-client"), + ) + ] + ) + user = User(id="srv-1", user_name="bjensen") + + with pytest.raises(MutabilityException): + patch.patch(user) + assert user.id == "srv-1" def test_a_model_value_carries_the_attributes_its_payload_would(): @@ -1111,56 +1156,1045 @@ def test_a_path_designating_the_resource_itself_is_accepted(): assert user.nick_name == "Babs" -def test_a_replace_unassigning_its_target_keeps_its_null_value_when_dumped(): - """A replace dumped without its value would be read back as missing its value.""" - patch = PatchOp[User]( - operations=[PatchOperation[User](op="replace", path="title", value=None)] +CONSTRAINED_RESOURCE_URN = "urn:example:2.0:ConstrainedResource" +CONSTRAINED_EXTENSION_URN = "urn:example:2.0:Constrained" +CORE_USER_URN = "urn:ietf:params:scim:schemas:core:2.0:User" + + +def _constrained_resource(): + return ConstrainedResource( + read_only_attr="original", + immutable_attr="original", + immutable_complex=ConstrainedComplex(label="original"), + immutable_date=datetime(2020, 1, 1, tzinfo=UTC), + immutable_list=[ConstrainedComplex(label="original")], + required_attr="original", ) - payload = patch.model_dump(scim_ctx=Context.RESOURCE_PATCH_REQUEST) - assert payload["Operations"] == [{"op": "replace", "path": "title", "value": None}] - user = User(user_name="bjensen", title="CEO") - PatchOp[User].model_validate( - payload, scim_ctx=Context.RESOURCE_PATCH_REQUEST - ).patch(user) - assert user.title is None +def _operation(op, path, value): + operation = {"op": op, "value": value} + if path is not None: + operation["path"] = path + return operation -def test_an_operation_given_no_value_is_dumped_without_one(): - """Only a null value the operation was given is kept in the dump.""" - operation = PatchOperation[User](op="replace", path="title") +RESOURCE_PATHS = [ + pytest.param(None, id="no path"), + pytest.param("", id="empty path"), + pytest.param(CONSTRAINED_RESOURCE_URN, id="schema URN"), + pytest.param(CONSTRAINED_RESOURCE_URN.upper(), id="schema URN in capitals"), +] - assert operation.model_dump(scim_ctx=Context.RESOURCE_PATCH_REQUEST) == { - "op": "replace", - "path": "title", - } +USER_PATHS = [ + pytest.param(None, id="no path"), + pytest.param("", id="empty path"), + pytest.param(CORE_USER_URN, id="schema URN"), + pytest.param(CORE_USER_URN.upper(), id="schema URN in capitals"), +] +VALIDATION_VIOLATIONS = [ + pytest.param("replace", "requiredAttr", None, id="required unassigned"), +] -def test_a_remove_given_a_null_value_is_dumped_without_one(): - """RFC7644 §3.5.2.2 reads a remove from its path only, so a null value is left out.""" - operation = PatchOperation[User](op="remove", path="title", value=None) +APPLICATION_VIOLATIONS = [ + pytest.param("replace", "readOnlyAttr", "hijacked", id="read-only replaced"), + pytest.param("add", "readOnlyAttr", "hijacked", id="read-only added"), + pytest.param("replace", "immutableAttr", "hijacked", id="immutable replaced"), + pytest.param("add", "immutableAttr", "hijacked", id="immutable added over a value"), +] - assert operation.model_dump(scim_ctx=Context.RESOURCE_PATCH_REQUEST) == { - "op": "remove", - "path": "title", - } +def _assert_constraints_kept(holder): + assert holder.read_only_attr == "original" + assert holder.immutable_attr == "original" + assert holder.required_attr == "original" -def test_a_patch_without_any_operation_is_refused(): - """RFC7644 §3.5.2 requires "Operations" to hold at least one operation.""" - with pytest.raises(ValidationError) as raised: - PatchOp[User].model_validate({"Operations": []}) - assert raised.value.errors()[0]["type"] == "scim_invalidValue" +def _assert_refused_for_mutability(raised): + assert raised.value.errors()[0]["type"] == "scim_mutability" -@pytest.mark.parametrize("op", ["move", "copy", 1]) -def test_an_unknown_operation_is_refused(op): - """RFC7644 §3.5.2 defines add, remove and replace, and §3.12 gives invalidValue for anything else.""" +@pytest.mark.parametrize("path", RESOURCE_PATHS) +@pytest.mark.parametrize(("op", "attribute", "value"), VALIDATION_VIOLATIONS) +def test_a_path_designating_the_resource_refuses_the_value_at_validation( + path, op, attribute, value +): + """When the path targets the resource, the value holds the attributes (RFC7644 §3.5.2.3).""" with pytest.raises(ValidationError) as raised: - PatchOp[User].model_validate( - {"Operations": [{"op": op, "path": "nickName", "value": "Babs"}]} + PatchOp[ConstrainedResource].model_validate( + {"Operations": [_operation(op, path, {attribute: value})]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + _assert_refused_for_mutability(raised) + + +@pytest.mark.parametrize("path", RESOURCE_PATHS) +@pytest.mark.parametrize(("op", "attribute", "value"), APPLICATION_VIOLATIONS) +def test_a_path_designating_the_resource_refuses_a_constrained_change_when_applied( + path, op, attribute, value +): + """Only the resource tells whether the value changes a read-only or immutable attribute, so this is checked when the patch is applied.""" + resource = _constrained_resource() + patch = PatchOp[ConstrainedResource].model_validate( + {"Operations": [_operation(op, path, {attribute: value})]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + with pytest.raises(MutabilityException): + patch.patch(resource) + + _assert_constraints_kept(resource) + + +@pytest.mark.parametrize("path", USER_PATHS) +@pytest.mark.parametrize( + "value", + [ + pytest.param({"id": "other-id"}, id="id"), + pytest.param({"groups": [{"value": "admins"}]}, id="groups"), + pytest.param({"meta": {"version": 'W/"999"'}}, id="meta"), + ], +) +def test_a_path_designating_the_resource_cannot_rewrite_its_identity(path, value): + """The read-only attributes of a user are rejected, whatever path targets the user.""" + user = User(id="srv-1", user_name="bjensen") + patch = PatchOp[User].model_validate( + {"Operations": [_operation("replace", path, value)]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + with pytest.raises(MutabilityException): + patch.patch(user) + assert user.id == "srv-1" + assert user.groups is None + assert user.meta is None + + +@pytest.mark.parametrize("path", RESOURCE_PATHS) +@pytest.mark.parametrize( + ("op", "attribute", "value"), + [ + pytest.param("replace", "plainAttr", "written", id="plain replaced"), + pytest.param("replace", "immutableAttr", "original", id="immutable kept"), + pytest.param("replace", "readOnlyAttr", "original", id="read-only kept"), + pytest.param("add", "readOnlyAttr", "original", id="read-only added unchanged"), + ], +) +def test_a_path_designating_the_resource_writes_what_the_schema_allows( + path, op, attribute, value +): + """The checks reject what the constraints forbid, and nothing else.""" + resource = _constrained_resource() + + PatchOp[ConstrainedResource].model_validate( + {"Operations": [_operation(op, path, {attribute: value})]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ).patch(resource) + + assert resource.model_dump(by_alias=True)[attribute] == value + + +IMMUTABLE_VALUES_AS_SPELLED = [ + pytest.param("immutableAttr", "original", id="string"), + pytest.param("immutableComplex", {"LABEL": "original"}, id="complex"), + pytest.param("immutableDate", "2020-01-01T00:00:00Z", id="datetime"), + pytest.param("immutableList", [{"label": "original"}], id="multi-valued"), +] + + +def test_an_immutable_multi_valued_attribute_takes_back_its_entries_in_another_order(): + """RFC7643 §2.4 gives no significance to the order of a multi-valued attribute.""" + resource = _constrained_resource() + resource.immutable_list = [ + ConstrainedComplex(label="first"), + ConstrainedComplex(label="second"), + ] + patch = PatchOp[ConstrainedResource].model_validate( + { + "Operations": [ + _operation( + "replace", + None, + {"immutableList": [{"label": "second"}, {"label": "first"}]}, + ) + ] + }, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + patch.patch(resource) + + assert {entry.label for entry in resource.immutable_list} == {"first", "second"} + + +@pytest.mark.parametrize( + "value", + [ + pytest.param([{"label": "original"}, {"label": "other"}], id="entry added"), + pytest.param([{"label": "other"}], id="entry replaced"), + ], +) +def test_an_immutable_multi_valued_attribute_refuses_other_entries(value): + """Comparing entries regardless of order still detects a different collection.""" + resource = _constrained_resource() + patch = PatchOp[ConstrainedResource].model_validate( + {"Operations": [_operation("replace", None, {"immutableList": value})]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + with pytest.raises(MutabilityException): + patch.patch(resource) + + assert resource.immutable_list == [ConstrainedComplex(label="original")] + + +@pytest.mark.parametrize("op", ["add", "replace"]) +@pytest.mark.parametrize(("attribute", "value"), IMMUTABLE_VALUES_AS_SPELLED) +@pytest.mark.parametrize("path", RESOURCE_PATHS) +def test_an_immutable_attribute_takes_back_the_value_it_holds_through_the_value( + path, attribute, value, op +): + """A client that sends the whole state sends the current value of an immutable attribute as JSON.""" + resource = _constrained_resource() + patch = PatchOp[ConstrainedResource].model_validate( + {"Operations": [_operation(op, path, {attribute: value})]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + patch.patch(resource) + + assert resource.model_dump() == _constrained_resource().model_dump() + + +@pytest.mark.parametrize("op", ["add", "replace"]) +@pytest.mark.parametrize(("attribute", "value"), IMMUTABLE_VALUES_AS_SPELLED) +def test_an_immutable_attribute_takes_back_the_value_it_holds_through_an_attribute_path( + attribute, value, op +): + """RFC7644 §3.5.2.1 makes no change when the target already holds the value.""" + resource = _constrained_resource() + patch = PatchOp[ConstrainedResource].model_validate( + {"Operations": [_operation(op, attribute, value)]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + patch.patch(resource) + + assert resource.model_dump() == _constrained_resource().model_dump() + + +def test_an_immutable_attribute_refuses_a_value_its_type_does_not_accept(): + """A value the attribute rejects is reported as invalid before any mutability check.""" + resource = _constrained_resource() + patch = PatchOp[ConstrainedResource].model_validate( + {"Operations": [_operation("replace", None, {"immutableDate": "not a date"})]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + with pytest.raises(InvalidValueException): + patch.patch(resource) + + assert resource.immutable_date == datetime(2020, 1, 1, tzinfo=UTC) + + +@pytest.mark.parametrize("op", ["add", "replace"]) +@pytest.mark.parametrize( + "path", [*RESOURCE_PATHS, pytest.param("immutableAttr", id="attribute path")] +) +def test_an_immutable_attribute_without_value_takes_its_first_value(path, op): + """§3.5.2 lets a client assign an immutable attribute with no value, and §3.5.2.3 treats a replace on it as an add.""" + resource = ConstrainedResource(required_attr="original") + value = "first" if path == "immutableAttr" else {"immutableAttr": "first"} + + PatchOp[ConstrainedResource].model_validate( + {"Operations": [_operation(op, path, value)]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ).patch(resource) + + assert resource.immutable_attr == "first" + + +def test_a_path_designating_the_resource_refuses_an_undeclared_attribute(): + """An attribute the schema URN does not declare is incompatible with the resource schema.""" + with pytest.raises(ValidationError) as raised: + PatchOp[ConstrainedResource].model_validate( + { + "Operations": [ + _operation("replace", CONSTRAINED_RESOURCE_URN, {"whatever": "x"}) + ] + }, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + assert raised.value.errors()[0]["type"] == "scim_invalidValue" + + +@pytest.mark.parametrize("path", ["", CORE_USER_URN]) +def test_a_path_designating_the_resource_writes_a_resource_given_as_value(path): + """A resource given as the value writes the attributes it was checked against.""" + user = User(user_name="bjensen") + patch = PatchOp[User]( + operations=[ + PatchOperation[User]( + op=PatchOperation.Op.replace_, path=path, value=User(nick_name="Babs") + ) + ] + ) + + assert patch.patch(user) + assert user.nick_name == "Babs" + + +@pytest.mark.parametrize( + "path", + [ + pytest.param("", id="empty path"), + pytest.param(CORE_USER_URN, id="resource schema URN"), + pytest.param(CONSTRAINED_EXTENSION_URN, id="extension schema URN"), + ], +) +def test_a_remove_selecting_by_value_needs_a_path_to_an_attribute(path): + """Under the apply policy the value of a remove selects entries of the attribute in its path.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User[ConstrainedExtension]].model_validate( + {"Operations": [_operation("remove", path, [{"plainAttr": "x"}])]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + scim_policy=ScimPolicy(remove_value_as_filter=ScimPolicy.RemoveValue.apply), + ) + + assert raised.value.errors()[0]["type"] == "scim_invalidPath" + + +def test_a_remove_on_the_schema_urn_of_an_extension_unassigns_it(): + """An extension is not an attribute of the resource, so it is not required.""" + user = User[ConstrainedExtension](user_name="bjensen") + user[ConstrainedExtension] = ConstrainedExtension( + required_attr="original", plain_attr="original" + ) + + PatchOp[User[ConstrainedExtension]].model_validate( + {"Operations": [{"op": "remove", "path": CONSTRAINED_EXTENSION_URN}]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ).patch(user) + + assert user[ConstrainedExtension] is None + + +EXTENSION_TARGETS = [ + pytest.param(None, CONSTRAINED_EXTENSION_URN, id="no path"), + pytest.param( + None, CONSTRAINED_EXTENSION_URN.upper(), id="no path, key in capitals" + ), + pytest.param("", CONSTRAINED_EXTENSION_URN, id="empty path"), + pytest.param(CORE_USER_URN, CONSTRAINED_EXTENSION_URN, id="resource schema URN"), + pytest.param(CONSTRAINED_EXTENSION_URN, None, id="extension schema URN"), + pytest.param( + CONSTRAINED_EXTENSION_URN.upper(), None, id="extension schema URN in capitals" + ), +] + + +def _extension_value(key, attribute, value): + written = {attribute: value} + return {key: written} if key is not None else written + + +@pytest.mark.parametrize(("path", "key"), EXTENSION_TARGETS) +@pytest.mark.parametrize(("op", "attribute", "value"), VALIDATION_VIOLATIONS) +def test_an_extension_in_the_value_refuses_its_constrained_attributes_at_validation( + path, key, op, attribute, value +): + """The attributes of an extension in the value follow the constraints the extension declares.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User[ConstrainedExtension]].model_validate( + { + "Operations": [ + _operation(op, path, _extension_value(key, attribute, value)) + ] + }, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + _assert_refused_for_mutability(raised) + + +@pytest.mark.parametrize(("path", "key"), EXTENSION_TARGETS) +@pytest.mark.parametrize(("op", "attribute", "value"), APPLICATION_VIOLATIONS) +def test_an_extension_in_the_value_refuses_a_constrained_change_when_applied( + path, key, op, attribute, value +): + """A read-only or immutable attribute of an extension in the value is protected once it has a value.""" + user = _constrained_user() + patch = PatchOp[User[ConstrainedExtension]].model_validate( + {"Operations": [_operation(op, path, _extension_value(key, attribute, value))]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + with pytest.raises(MutabilityException): + patch.patch(user) + + _assert_constraints_kept(user[ConstrainedExtension]) + + +@pytest.mark.parametrize(("path", "key"), EXTENSION_TARGETS) +def test_an_extension_in_the_value_refuses_an_attribute_it_does_not_declare(path, key): + """An attribute outside the extension schema is incompatible with it.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User[ConstrainedExtension]].model_validate( + { + "Operations": [ + _operation("replace", path, _extension_value(key, "whatever", "x")) + ] + }, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + assert raised.value.errors()[0]["type"] == "scim_invalidValue" + + +def _user_with_hijacked_extension(): + user = User[ConstrainedExtension]() + user[ConstrainedExtension] = ConstrainedExtension(read_only_attr="hijacked") + return user + + +@pytest.mark.parametrize( + ("path", "value"), + [ + pytest.param( + CONSTRAINED_EXTENSION_URN, + ConstrainedExtension(read_only_attr="hijacked"), + id="extension on its schema URN", + ), + pytest.param(None, _user_with_hijacked_extension(), id="resource without path"), + ], +) +def test_an_extension_given_as_a_model_answers_for_its_own_constraints(path, value): + """A value built in Python is checked like the same value in a payload.""" + user = _constrained_user() + patch = PatchOp[User[ConstrainedExtension]]( + operations=[ + PatchOperation[User[ConstrainedExtension]]( + op=PatchOperation.Op.replace_, path=path, value=value + ) + ] + ) + + with pytest.raises(MutabilityException): + patch.patch(user) + _assert_constraints_kept(user[ConstrainedExtension]) + + +def _unvalidated_patch_writing_an_undeclared_attribute(): + return PatchOp[ConstrainedResource].model_construct( + operations=[ + PatchOperation[ConstrainedResource].model_construct( + op=PatchOperation.Op.replace_, path=None, value={"whatever": "x"} + ) + ] + ) + + +def test_an_unvalidated_patch_refuses_an_undeclared_attribute_when_applied(): + """The policy passed to patch decides what happens to undeclared attributes.""" + patch = _unvalidated_patch_writing_an_undeclared_attribute() + + with pytest.raises(InvalidValueException): + patch.patch(_constrained_resource()) + + +def test_an_unvalidated_patch_drops_an_undeclared_attribute_under_a_tolerant_policy(): + """A tolerant policy drops the attribute, and the operation writes nothing.""" + patch = _unvalidated_patch_writing_an_undeclared_attribute() + tolerant = ScimPolicy(unknown=ScimPolicy.Unknown.ignore) + + assert patch.patch(_constrained_resource(), scim_policy=tolerant) is False + + +def test_an_unvalidated_patch_leaves_an_undeclared_path_to_the_write(): + """An undeclared path has no immutability constraint, and fails when the write finds no field.""" + resource = _constrained_resource() + patch = PatchOp[ConstrainedResource].model_construct( + operations=[ + PatchOperation[ConstrainedResource].model_construct( + op=PatchOperation.Op.replace_, + path=Path[ConstrainedResource]("whatever"), + value="x", + ) + ] + ) + + with pytest.raises(PathNotFoundException): + patch.patch(resource) + + +def test_a_remove_without_path_carrying_a_value_has_no_target_outside_a_request(): + """RFC7644 §3.5.2.2 returns noTarget for a remove without a path, whatever its value.""" + patch = PatchOp[User].model_validate( + {"Operations": [{"op": "remove", "value": {"nickName": "x"}}]} + ) + + with pytest.raises(NoTargetException): + patch.patch(User(user_name="bjensen")) + + +def test_a_remove_carrying_a_value_on_a_schema_urn_is_refused_by_default_outside_a_request(): + """Without the apply policy, the value of a remove is rejected, as on any path.""" + patch = PatchOp[User].model_validate( + { + "Operations": [ + {"op": "remove", "path": CORE_USER_URN, "value": [{"nickName": "x"}]} + ] + } + ) + + with pytest.raises(InvalidValueException): + patch.patch(User(user_name="bjensen")) + + +@pytest.mark.parametrize( + "unknown", [ScimPolicy.Unknown.ignore, ScimPolicy.Unknown.keep], ids=str +) +@pytest.mark.parametrize(("path", "key"), EXTENSION_TARGETS) +def test_a_tolerant_policy_writes_the_declared_attributes_beside_an_undeclared_one( + path, key, unknown +): + """The unknown policy applies to the value of a patch as it does to a resource.""" + policy = ScimPolicy(unknown=unknown) + user = _constrained_user() + value = _extension_value(key, "plainAttr", "written") + if key is not None: + value[key]["whatever"] = "x" + else: + value["whatever"] = "x" + + PatchOp[User[ConstrainedExtension]].model_validate( + {"Operations": [_operation("replace", path, value)]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + scim_policy=policy, + ).patch(user, scim_policy=policy) + + dumped = user.model_dump(scim_ctx=Context.RESOURCE_QUERY_RESPONSE) + assert dumped[CONSTRAINED_EXTENSION_URN]["plainAttr"] == "written" + assert "whatever" not in dumped[CONSTRAINED_EXTENSION_URN] + + +@pytest.mark.parametrize( + "unknown", [ScimPolicy.Unknown.ignore, ScimPolicy.Unknown.keep], ids=str +) +@pytest.mark.parametrize("path", RESOURCE_PATHS) +def test_a_tolerant_policy_drops_an_undeclared_attribute_of_the_resource(path, unknown): + """An undeclared attribute has no field to write to.""" + policy = ScimPolicy(unknown=unknown) + resource = _constrained_resource() + + PatchOp[ConstrainedResource].model_validate( + {"Operations": [_operation("replace", path, {"plainAttr": "w", "zzz": "x"})]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + scim_policy=policy, + ).patch(resource, scim_policy=policy) + + assert resource.plain_attr == "w" + assert "zzz" not in resource.model_dump() + + +EXTENSION_UNASSIGNMENTS = [ + pytest.param( + {"op": "remove", "path": CONSTRAINED_EXTENSION_URN}, id="removed by URN" + ), + pytest.param( + {"op": "replace", "value": {CONSTRAINED_EXTENSION_URN: None}}, + id="null key without path", + ), + pytest.param( + {"op": "replace", "path": "", "value": {CONSTRAINED_EXTENSION_URN: None}}, + id="null key on the empty path", + ), + pytest.param( + { + "op": "replace", + "path": CORE_USER_URN, + "value": {CONSTRAINED_EXTENSION_URN: None}, + }, + id="null key on the resource URN", + ), +] + + +@pytest.mark.parametrize("operation", EXTENSION_UNASSIGNMENTS) +def test_an_extension_holding_an_immutable_value_cannot_be_unassigned(operation): + """Unassigning an extension changes its immutable attributes, which RFC7644 §3.5.2 forbids once they hold a value.""" + user = _constrained_user() + patch = PatchOp[User[ConstrainedExtension]].model_validate( + {"Operations": [operation]}, scim_ctx=Context.RESOURCE_PATCH_REQUEST + ) + + with pytest.raises(MutabilityException): + patch.patch(user) + + _assert_constraints_kept(user[ConstrainedExtension]) + + +@pytest.mark.parametrize("operation", EXTENSION_UNASSIGNMENTS) +def test_an_extension_without_immutable_value_can_be_unassigned(operation): + """Unassigning an extension does not change an immutable attribute that has no value yet.""" + user = User[ConstrainedExtension](user_name="bjensen") + user[ConstrainedExtension] = ConstrainedExtension( + read_only_attr="original", required_attr="original" + ) + + PatchOp[User[ConstrainedExtension]].model_validate( + {"Operations": [operation]}, scim_ctx=Context.RESOURCE_PATCH_REQUEST + ).patch(user) + + assert user[ConstrainedExtension] is None + + +@pytest.mark.parametrize("path", ["", CORE_USER_URN]) +def test_a_remove_designating_the_resource_itself_is_refused(path): + """A remove on the resource root has no attribute to unassign.""" + patch = PatchOp[User].model_validate( + {"Operations": [{"op": "remove", "path": path}]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + with pytest.raises(InvalidPathException): + patch.patch(User(user_name="bjensen")) + + +def test_building_the_unassignment_of_an_extension_holding_an_immutable_value_is_refused(): + """The peer would reject the patch, so building it fails.""" + after = User[ConstrainedExtension](user_name="bjensen") + after.ConstrainedExtension = None + + with pytest.raises(MutabilityException): + PatchOp.build_from(_constrained_user(), after) + + +def test_the_unassignment_built_for_an_extension_without_immutable_value_applies(): + """A patch built from two states is one the resource accepts.""" + before = User[ConstrainedExtension](user_name="bjensen") + before[ConstrainedExtension] = ConstrainedExtension(plain_attr="original") + after = User[ConstrainedExtension](user_name="bjensen") + after.ConstrainedExtension = None + + patch = PatchOp.build_from(before, after) + + assert patch.patch(before) + assert before[ConstrainedExtension] is None + + +def test_a_replace_unassigning_its_target_keeps_its_null_value_when_dumped(): + """A replace dumped without its value would be read back as missing its value.""" + patch = PatchOp[User]( + operations=[PatchOperation[User](op="replace", path="title", value=None)] + ) + + payload = patch.model_dump(scim_ctx=Context.RESOURCE_PATCH_REQUEST) + + assert payload["Operations"] == [{"op": "replace", "path": "title", "value": None}] + user = User(user_name="bjensen", title="CEO") + PatchOp[User].model_validate( + payload, scim_ctx=Context.RESOURCE_PATCH_REQUEST + ).patch(user) + assert user.title is None + + +def test_an_operation_given_no_value_is_dumped_without_one(): + """Only a null value the operation was given is kept in the dump.""" + operation = PatchOperation[User](op="replace", path="title") + + assert operation.model_dump(scim_ctx=Context.RESOURCE_PATCH_REQUEST) == { + "op": "replace", + "path": "title", + } + + +def test_a_remove_given_a_null_value_is_dumped_without_one(): + """RFC7644 §3.5.2.2 reads a remove from its path only, so a null value is left out.""" + operation = PatchOperation[User](op="remove", path="title", value=None) + + assert operation.model_dump(scim_ctx=Context.RESOURCE_PATCH_REQUEST) == { + "op": "remove", + "path": "title", + } + + +@pytest.mark.parametrize("path", [None, "nickName"]) +def test_a_replace_needs_a_value(path): + """RFC7644 §3.5.2.3 replaces the target with the value, and a missing one would read as null.""" + operation = {"op": "replace"} if path is None else {"op": "replace", "path": path} + + with pytest.raises(ValidationError) as raised: + PatchOp[User].model_validate( + {"Operations": [operation]}, scim_ctx=Context.RESOURCE_PATCH_REQUEST + ) + + assert raised.value.errors()[0]["type"] == "scim_invalidValue" + + +@pytest.mark.parametrize("op", ["add", "replace"]) +@pytest.mark.parametrize("value", ["Babs", ["Babs"], None]) +@pytest.mark.parametrize("path", [*USER_PATHS, CONSTRAINED_EXTENSION_URN]) +def test_a_path_designating_a_model_needs_an_object_value(op, value, path): + """RFC7644 §3.5.2.1 and §3.5.2.3 require that value to be a set of attributes.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User[ConstrainedExtension]].model_validate( + {"Operations": [_operation(op, path, value)]} + ) + + assert raised.value.errors()[0]["type"] == "scim_invalidValue" + + +@pytest.mark.parametrize("path", [None, CONSTRAINED_EXTENSION_URN]) +def test_an_unvalidated_non_object_value_is_refused_when_applied(path): + """A patch built in Python reaches no validator, and is refused when applied.""" + patch = PatchOp[User[ConstrainedExtension]].model_construct( + operations=[ + PatchOperation[User[ConstrainedExtension]].model_construct( + op=PatchOperation.Op.add, + path=None if path is None else Path[User[ConstrainedExtension]](path), + value="Babs", + ) + ] + ) + + with pytest.raises(InvalidValueException): + patch.patch(User[ConstrainedExtension](user_name="bjensen")) + + +@pytest.mark.parametrize("path", USER_PATHS) +def test_an_extension_in_a_value_needs_an_object(path): + """An extension is a container of attributes, not a value of its own.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User[ConstrainedExtension]].model_validate( + {"Operations": [_operation("add", path, {CONSTRAINED_EXTENSION_URN: "x"})]} + ) + + assert raised.value.errors()[0]["type"] == "scim_invalidValue" + + +def _group_with_member(): + return Group( + display_name="Tour Guides", members=[{"value": "2819c223", "display": "Babs"}] + ) + + +@pytest.mark.parametrize( + "path", ['members[value eq "2819c223"].value', "members.value"] +) +def test_an_immutable_sub_attribute_of_an_entry_cannot_change(path): + """RFC7643 §4.2 lets members be added and removed, and makes their sub-attributes immutable.""" + group = _group_with_member() + patch = PatchOp[Group].model_validate( + {"Operations": [{"op": "replace", "path": path, "value": "c3a26dd3"}]} + ) + + with pytest.raises(MutabilityException): + patch.patch(group) + + assert group.members[0].value == "2819c223" + + +def test_a_mutable_sub_attribute_of_an_entry_may_change(): + """Only the sub-attributes declared immutable must keep their value.""" + group = _group_with_member() + patch = PatchOp[Group].model_validate( + { + "Operations": [ + { + "op": "replace", + "path": 'members[value eq "2819c223"].display', + "value": "Barbara", + } + ] + } + ) + + assert patch.patch(group) + assert group.members[0].display == "Barbara" + + +def test_an_entry_holding_immutable_sub_attributes_may_be_removed(): + """Removing a member changes none of the sub-attributes of the ones left.""" + group = Group( + display_name="Tour Guides", + members=[{"value": "2819c223"}, {"value": "c3a26dd3"}], + ) + patch = PatchOp[Group].model_validate( + {"Operations": [{"op": "remove", "path": 'members[value eq "2819c223"]'}]} + ) + + assert patch.patch(group) + assert [member.value for member in group.members] == ["c3a26dd3"] + + +def test_an_entry_kept_through_an_assignment_is_the_same_object(): + """Entries are told apart by identity, which assigning a list must keep.""" + group = _group_with_member() + member = group.members[0] + + group.members = [*group.members, Group.Members(value="c3a26dd3")] + + assert group.members[0] is member + + +@pytest.mark.parametrize("op", ["add", "replace"]) +def test_an_immutable_complex_takes_back_the_sub_attribute_it_holds(op): + """Writing the value a sub-attribute holds modifies nothing.""" + resource = _constrained_resource() + patch = PatchOp[ConstrainedResource].model_validate( + {"Operations": [_operation(op, "immutableComplex.label", "original")]} + ) + + assert patch.patch(resource) is False + + +def test_an_immutable_complex_refuses_a_new_sub_attribute_value(): + """A complex attribute holding a value is immutable down to its sub-attributes.""" + resource = _constrained_resource() + patch = PatchOp[ConstrainedResource].model_validate( + {"Operations": [_operation("replace", "immutableComplex.label", "changed")]} + ) + + with pytest.raises(MutabilityException): + patch.patch(resource) + + +def test_a_remove_selecting_nothing_in_an_immutable_attribute_is_accepted(): + """Per RFC7644 §3.5.2.2, a remove that selects nothing changes nothing.""" + resource = _constrained_resource() + patch = PatchOp[ConstrainedResource].model_validate( + {"Operations": [{"op": "remove", "path": 'immutableList[label eq "none"]'}]} + ) + + assert patch.patch(resource) is False + + +class StampedEntry(ComplexAttribute): + value: str | None = None + stamp: Annotated[str | None, Mutability.read_only] = None + + +class StampedResource(Resource): + __schema__ = URN("urn:example:2.0:StampedResource") + + entries: list[StampedEntry] | None = None + + +def _stamped(): + return StampedResource(entries=[StampedEntry(value="a", stamp="server")]) + + +def _replace_entry(value): + return PatchOp[StampedResource].model_validate( + { + "Operations": [ + {"op": "replace", "path": 'entries[value eq "a"]', "value": value} + ] + } + ) + + +def test_an_entry_written_back_with_its_read_only_sub_attribute_is_accepted(): + """An entry sent as it was read changes none of its read-only sub-attributes.""" + resource = _stamped() + + assert _replace_entry({"value": "a", "stamp": "server"}).patch(resource) is False + + +def test_an_entry_cannot_change_its_read_only_sub_attribute(): + """RFC7644 §3.5.2 forbids changing a read-only attribute, including a sub-attribute of an entry.""" + resource = _stamped() + + with pytest.raises(MutabilityException): + _replace_entry({"value": "a", "stamp": "client"}).patch(resource) + + +def test_an_added_entry_cannot_assign_a_read_only_sub_attribute(): + """A client cannot set a read-only sub-attribute, even on an entry it adds.""" + resource = _stamped() + patch = PatchOp[StampedResource].model_validate( + { + "Operations": [ + { + "op": "add", + "path": "entries", + "value": [{"value": "b", "stamp": "client"}], + } + ] + } + ) + + with pytest.raises(MutabilityException): + patch.patch(resource) + + +class Tag(ComplexAttribute): + value: str | None = None + + +class Parts(ComplexAttribute): + first: str | None = None + last: str | None = None + + +class Contact(ComplexAttribute): + value: Annotated[str | None, Required.true] = None + label: str | None = None + + +class RequiringResource(Resource): + __schema__ = URN("urn:example:2.0:RequiringResource") + + tags: Annotated[list[Tag] | None, Required.true] = None + parts: Annotated[Parts | None, Required.true] = None + contact: Contact | None = None + + +def _requiring(): + return RequiringResource( + tags=[Tag(value="a"), Tag(value="b")], + parts=Parts(first="Barbara", last="Jensen"), + contact=Contact(value="555", label="work"), + ) + + +def _apply(operation): + patch = PatchOp[RequiringResource].model_validate({"Operations": [operation]}) + resource = _requiring() + return patch.patch(resource), resource + + +@pytest.mark.parametrize( + "operation", + [ + pytest.param( + {"op": "remove", "path": 'tags[value eq "a"]'}, id="some entries removed" + ), + pytest.param( + {"op": "remove", "path": 'tags[value eq "none"]'}, id="no entry selected" + ), + pytest.param( + {"op": "remove", "path": "parts.first"}, id="a sub-attribute removed" + ), + pytest.param({"op": "add", "path": "tags", "value": []}, id="no value added"), + pytest.param( + {"op": "remove", "path": "contact"}, + id="a required sub-attribute removed with its parent", + ), + ], +) +def test_a_required_attribute_left_assigned_is_accepted(operation): + """RFC7644 §3.5.2.2 only rejects a required attribute that is removed or becomes unassigned.""" + _, resource = _apply(operation) + + assert resource.tags + assert resource.parts + + +@pytest.mark.parametrize( + "operation", + [ + pytest.param( + {"op": "remove", "path": 'tags[value eq "a" or value eq "b"]'}, + id="every entry removed", + ), + pytest.param( + {"op": "replace", "path": "parts", "value": {"first": None, "last": None}}, + id="every sub-attribute unassigned", + ), + pytest.param( + {"op": "remove", "path": "contact.value"}, + id="required sub-attribute removed", + ), + ], +) +def test_a_required_attribute_becoming_unassigned_is_refused_when_applied(operation): + """Only the result of the patch shows that a required attribute became unassigned.""" + patch = PatchOp[RequiringResource].model_validate({"Operations": [operation]}) + resource = _requiring() + + with pytest.raises(MutabilityException): + patch.patch(resource) + + assert resource == _requiring() + + +def test_a_required_extension_cannot_be_removed(): + """Per RFC7643 §6, a resource includes the extensions its resource type requires.""" + model = User[Annotated[ConstrainedExtension, Required.true]] + user = model(user_name="bjensen") + user[ConstrainedExtension] = ConstrainedExtension(plain_attr="x") + patch = PatchOp[model].model_validate( + {"Operations": [{"op": "remove", "path": CONSTRAINED_EXTENSION_URN}]} + ) + + with pytest.raises(MutabilityException): + patch.patch(user) + + +def test_a_patch_without_any_operation_is_refused(): + """RFC7644 §3.5.2 requires "Operations" to hold at least one operation.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User].model_validate({"Operations": []}) + + assert raised.value.errors()[0]["type"] == "scim_invalidValue" + + +@pytest.mark.parametrize("op", ["move", "copy", 1]) +def test_an_unknown_operation_is_refused(op): + """RFC7644 §3.5.2 defines add, remove and replace, and §3.12 gives invalidValue for anything else.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User].model_validate( + {"Operations": [{"op": op, "path": "nickName", "value": "Babs"}]} + ) + + assert raised.value.errors()[0]["type"] == "scim_invalidValue" + + +def test_an_add_with_a_null_value_is_refused(): + """RFC7644 §3.5.2.1 requires an add to carry a value, and null is not one.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User].model_validate( + {"Operations": [{"op": "add", "path": "nickName", "value": None}]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + assert raised.value.errors()[0]["type"] == "scim_invalidValue" + + +@pytest.mark.parametrize("op", ["add", "replace"]) +def test_an_empty_value_on_a_read_only_attribute_is_refused(op): + """The path points to a read-only attribute, whatever the value writes under it.""" + with pytest.raises(ValidationError) as raised: + PatchOp[Group].model_validate( + {"Operations": [{"op": op, "path": "meta", "value": {}}]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + ) + + assert raised.value.errors()[0]["type"] == "scim_mutability" + + +@pytest.mark.parametrize( + "policy", + [ + ScimPolicy(unknown=ScimPolicy.Unknown.ignore), + ScimPolicy(unknown=ScimPolicy.Unknown.keep), + ], + ids=["ignore", "keep"], +) +def test_a_remove_value_on_an_undeclared_path_is_refused_under_a_tolerant_policy( + policy, +): + """The value of a remove is rejected before the path is dropped.""" + with pytest.raises(ValidationError) as raised: + PatchOp[User].model_validate( + {"Operations": [{"op": "remove", "path": "whatever", "value": "x"}]}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + scim_policy=policy, ) assert raised.value.errors()[0]["type"] == "scim_invalidValue" diff --git a/tests/test_path_value_selection.py b/tests/test_path_value_selection.py index 292ac395..eb123da9 100644 --- a/tests/test_path_value_selection.py +++ b/tests/test_path_value_selection.py @@ -163,11 +163,14 @@ def test_writing_the_same_value_changes_nothing(user): def test_writing_replaces_the_matching_values_wholesale(user): - """§3.5.2.3: all matching record values are replaced.""" + """Path.set assigns, so unlike PATCH it unassigns the sub-attributes the value leaves out.""" + entry = user.emails[1] assert Path[User]('emails[type eq "home"]').set( user, {"type": "other", "value": "other@example.com"} ) assert [email.type for email in user.emails] == ["work", "other", "work"] + assert user.emails[1].primary is None + assert user.emails[1] is entry def test_writing_the_same_whole_value_changes_nothing(user): diff --git a/tests/test_policy.py b/tests/test_policy.py index 44e0c76e..aa11ec4e 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -465,8 +465,8 @@ def test_a_pathless_patch_operation_keeps_the_unknown_attributes_it_merges_over( assert user.unknown_attributes == {"unknownAttr": "x"} -def test_replacing_a_complex_attribute_drops_the_unknowns_it_carried(): - """The whole attribute is replaced, so what was unknown in it goes with the rest.""" +def test_replacing_a_complex_attribute_keeps_the_unknowns_it_carried(): + """RFC7644 §3.5.2.3 keeps the sub-attributes a replace does not specify, unknown ones included.""" with KEEP: user = User.model_validate( unknown_payload(name={"familyName": "Jensen", "bogusSub": 1}) @@ -476,8 +476,9 @@ def test_replacing_a_complex_attribute_drops_the_unknowns_it_carried(): ) PatchOp[User](operations=[operation]).patch(user) - assert user.name.family_name is None - assert user.name.unknown_attributes == {} + assert user.name.given_name == "Barbara" + assert user.name.family_name == "Jensen" + assert user.name.unknown_attributes == {"bogusSub": 1} # A remove operation carrying a value @@ -589,6 +590,14 @@ def test_a_selection_that_matches_nothing_is_a_success(): assert len(group.members) == 2 +def test_a_selection_listing_no_entry_removes_nothing(): + """An empty list selects no member, so none is removed.""" + group = group_with_members() + + assert entra_remove([]).patch(group, scim_policy=APPLY) is False + assert len(group.members) == 2 + + def test_a_path_that_already_selects_refuses_a_value_even_under_apply(): """Two selections are two intentions, and nothing says which one to honour.""" patch_op = PatchOp[Group]( From 9457ffe7870684bd4ee1d7746798650a56af7cf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89loi=20Rivard?= Date: Sun, 27 Sep 2026 14:36:10 +0200 Subject: [PATCH 6/7] fix: apply the unknown policy to PATCH paths A path naming an undeclared attribute always failed with invalidPath, while the same attribute in an operation value was dropped under ignore and keep. The policy now applies to paths too: forbid returns invalidPath, and ignore and keep turn the operation into a no-op. An undeclared attribute inside a filter still returns invalidFilter. --- doc/changelog.rst | 4 + doc/explanation/patch.rst | 7 +- doc/explanation/policies.rst | 15 ++-- doc/how-to/tolerate-a-nonconformant-peer.rst | 4 + scim2_models/messages/patch_op.py | 13 +++ scim2_models/policy.py | 9 +- tests/test_policy.py | 87 ++++++++++++++++++++ 7 files changed, 126 insertions(+), 13 deletions(-) diff --git a/doc/changelog.rst b/doc/changelog.rst index 5166ca40..a1cd4ea4 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -61,6 +61,10 @@ Fixed pydantic :class:`~pydantic.ValidationError` without ``scimType``. - :meth:`PatchOp.patch ` no longer reports a resource as modified when an operation writes a complex or multi-valued value it already has. +- A PATCH path to an undeclared attribute follows + :attr:`ScimPolicy.unknown `, like a value does. With ``ignore`` + or ``keep``, the operation changes nothing instead of failing the whole patch with + ``invalidPath``. An undeclared sub-attribute in a filter still fails with ``invalidFilter``. Security ^^^^^^^^ diff --git a/doc/explanation/patch.rst b/doc/explanation/patch.rst index f48c0882..156ebe0c 100644 --- a/doc/explanation/patch.rst +++ b/doc/explanation/patch.rst @@ -257,9 +257,12 @@ Rejected paths Which ``scimType`` a rejected path answers follows Table 9 of :rfc:`RFC7644 §3.12 <7644#section-3.12>`. ``invalidPath`` covers "the ``path`` attribute was invalid or malformed (see Figure 7)". It answers a path the grammar refuses, and an attribute the -model does not declare. Table 9 lists ``invalidFilter`` as applying to a "PATCH (Path Filter)", +model does not declare, unless :attr:`ScimPolicy.unknown ` drops +it. With ``ignore`` or ``keep``, an operation on an undeclared attribute changes +nothing. Table 9 lists ``invalidFilter`` as applying to a "PATCH (Path Filter)", so it answers what goes wrong between the brackets. That covers an unknown sub-attribute, a -comparison the attribute cannot take, and a selection over an attribute holding a single value. +comparison the attribute cannot take, and a selection over an attribute holding a single value, +under any policy. Building a patch rather than applying one ----------------------------------------- diff --git a/doc/explanation/policies.rst b/doc/explanation/policies.rst index 296a71ab..759304df 100644 --- a/doc/explanation/policies.rst +++ b/doc/explanation/policies.rst @@ -59,13 +59,14 @@ only break that client. :doc:`patch` describes how such a key is read. What a policy leaves alone -------------------------- -**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. +**PATCH filters stay strict.** Under :attr:`~scim2_models.ScimPolicy.Unknown.ignore` or +:attr:`~scim2_models.ScimPolicy.Unknown.keep`, a PATCH operation on an attribute no model +declares changes nothing, like an undeclared attribute in its value. A filter that +compares such an attribute is still rejected with ``invalidFilter``, and a malformed path with +``invalidPath``. In both cases, there is no attribute the policy could drop. Dropping an +operation has a cost: the server returns 200 for a change it never applied, where +:rfc:`RFC7644 §3.5.2 <7644#section-3.5.2>` asks for an error. A tolerant policy already accepts +that cost for the body of a resource, and the strict default keeps the error. **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/doc/how-to/tolerate-a-nonconformant-peer.rst b/doc/how-to/tolerate-a-nonconformant-peer.rst index 3a983dd4..3adbdf0b 100644 --- a/doc/how-to/tolerate-a-nonconformant-peer.rst +++ b/doc/how-to/tolerate-a-nonconformant-peer.rst @@ -45,6 +45,10 @@ from: An unmodelled extension takes the same route, since a payload names one with a root key whose name is a URN. +Under that policy, a PATCH operation on an undeclared attribute changes +nothing, and the other operations still apply. Pass the policy to +:meth:`PatchOp.patch ` as well as to the validation of the message. + Write unknown attributes back ----------------------------- diff --git a/scim2_models/messages/patch_op.py b/scim2_models/messages/patch_op.py index 3b77b7d2..d27daf55 100644 --- a/scim2_models/messages/patch_op.py +++ b/scim2_models/messages/patch_op.py @@ -316,6 +316,15 @@ def _as_payload(value: Any) -> Any: return value +def _dropped(path: Path[Any], policy: ScimPolicy) -> bool: + """Whether the policy drops the undeclared attribute of a path.""" + return ( + path.model is None + and path.resolve() is None + and policy.unknown != ScimPolicy.Unknown.forbid + ) + + def _check_operation( model: type[Resource[Any]], operation: PatchOperation[Any], @@ -333,6 +342,8 @@ def _check_operation( removal = operation.op == PatchOperation.Op.remove if removal and in_request: _removal_path(path, operation.value, policy) + if _dropped(path, policy): + return if path.model is None: binding = path.resolve() @@ -425,6 +436,8 @@ def _apply_operation( if not isinstance(operation.op, PatchOperation.Op): raise InvalidValueException(detail=f"{operation.op!r} is not a PATCH operation") path = Path.__class_getitem__(type(resource))(operation.path or "") + if _dropped(path, policy): + return False removal = None if operation.op == PatchOperation.Op.remove: diff --git a/scim2_models/policy.py b/scim2_models/policy.py index beed1df6..2efd7963 100644 --- a/scim2_models/policy.py +++ b/scim2_models/policy.py @@ -72,16 +72,17 @@ class Unknown(StrEnum): It stays readable on :attr:`~scim2_models.BaseModel.unknown_attributes`, and no dump - restores it. + restores it. A PATCH operation on such an attribute changes + nothing. """ keep = "keep" """The payload is accepted and the attribute is dumped back. The original spelling is preserved. No attribute characteristic is - declared for it, so no context filters it out. In the value of a PATCH - operation, the attribute has no field to be written to and is dropped - as ``ignore`` drops it. + declared for it, so no context filters it out. In the value or the + path of a PATCH operation, it has no field to write to, so it is + dropped as with ``ignore``. """ class RemoveValue(StrEnum): diff --git a/tests/test_policy.py b/tests/test_policy.py index aa11ec4e..073475ca 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -19,6 +19,7 @@ from scim2_models import CreationResponseContext from scim2_models import EnterpriseUser from scim2_models import Group +from scim2_models import InvalidFilterException from scim2_models import InvalidValueException from scim2_models import PatchOp from scim2_models import PatchOperation @@ -388,6 +389,92 @@ def test_the_keys_microsoft_entra_adds_to_a_patch_are_ignored(): assert patch_op.operations[0].unknown_attributes == {"name": "addMember"} +# Unknown attributes in a PATCH path + + +def _pathed_patch(operations, policy): + return PatchOp[User].model_validate( + {"Operations": operations}, + scim_ctx=Context.RESOURCE_PATCH_REQUEST, + scim_policy=policy, + ) + + +def _pathed_user(): + return User( + user_name="bjensen", emails=[{"value": "b@example.com", "type": "work"}] + ) + + +UNDECLARED_PATHS = [ + pytest.param("unknownAttr", id="attribute"), + pytest.param("name.unknownAttr", id="sub-attribute"), + pytest.param("urn:example:2.0:Unmodelled:attr", id="extension"), + pytest.param( + 'emails[type eq "work"].unknownAttr', id="sub-attribute of a selection" + ), +] + + +def test_an_undeclared_path_is_refused_by_default(): + """The interoperability profile asks a service provider to reject what it does not define.""" + with pytest.raises(ValidationError) as raised: + _pathed_patch([{"op": "replace", "path": "unknownAttr", "value": "x"}], None) + + assert raised.value.errors()[0]["type"] == "scim_invalidPath" + + +@pytest.mark.parametrize( + "policy", [IGNORE, ScimPolicy(unknown=ScimPolicy.Unknown.keep)] +) +@pytest.mark.parametrize("op", ["add", "replace", "remove"]) +@pytest.mark.parametrize("path", UNDECLARED_PATHS) +def test_an_undeclared_path_is_dropped_under_a_tolerant_policy(policy, op, path): + """An attribute the policy drops from a value is also dropped from a path, since it has no field to write to.""" + operation = {"op": op, "path": path} + if op != "remove": + operation["value"] = "x" + user = _pathed_user() + before = user.model_dump() + + assert not _pathed_patch([operation], policy).patch(user, scim_policy=policy) + assert user.model_dump() == before + + +def test_the_operations_beside_an_undeclared_path_are_applied(): + """Dropping one operation leaves the others of the patch to apply.""" + user = _pathed_user() + patch = _pathed_patch( + [ + {"op": "replace", "path": "unknownAttr", "value": "x"}, + {"op": "replace", "path": "nickName", "value": "Babs"}, + ], + IGNORE, + ) + + assert patch.patch(user, scim_policy=IGNORE) + assert user.nick_name == "Babs" + + +def test_a_filter_comparing_an_undeclared_sub_attribute_is_invalid_under_any_policy(): + """The policy applies to attribute names, not to filter expressions.""" + patch = _pathed_patch( + [{"op": "replace", "path": 'emails[unknownAttr eq "x"].value', "value": "x"}], + IGNORE, + ) + + with pytest.raises(InvalidFilterException): + patch.patch(_pathed_user(), scim_policy=IGNORE) + + +def test_a_malformed_path_is_invalid_under_any_policy(): + """A path the grammar rejects has no attribute the policy could drop.""" + with pytest.raises(ValidationError) as raised: + _pathed_patch([{"op": "replace", "path": "unknown attr", "value": "x"}], IGNORE) + + assert raised.value.errors()[0]["type"] == "scim_invalidPath" + + # Unknown attributes, carried back From 772deb1dc24982f5014e73de5e656c65fd6703f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89loi=20Rivard?= Date: Sun, 27 Sep 2026 14:36:29 +0200 Subject: [PATCH 7/7] feat: let a policy create the entry a PATCH filter describes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Microsoft Entra ID sends add or replace on a filter that matches nothing, such as emails[type eq "work"].value on a user without a work email, and expects the entry to be created. By default scim2-models still returns noTarget, as RFC 7644 §3.5.2.3 requires for replace. With ScimPolicy.unmatched_path_filter set to create, the entry the filter describes (eq comparisons joined by and) is created, then the value is written into it. A value that would make the entry stop matching the filter is rejected. --- doc/changelog.rst | 6 + doc/explanation/patch.rst | 23 +- doc/how-to/tolerate-a-nonconformant-peer.rst | 32 +++ scim2_models/messages/patch_op.py | 73 +++++- scim2_models/policy.py | 23 ++ tests/test_policy.py | 225 +++++++++++++++++++ 6 files changed, 372 insertions(+), 10 deletions(-) diff --git a/doc/changelog.rst b/doc/changelog.rst index a1cd4ea4..e2e06d20 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -4,6 +4,12 @@ Changelog [Unreleased] ------------ +Added +^^^^^ +- :attr:`ScimPolicy.unmatched_path_filter ` can + make a PATCH ``add`` or ``replace`` create the entry its path filter describes when no entry + matches, as Microsoft Entra ID expects. By default, the operation still fails with ``noTarget``. + Changed ^^^^^^^ - Python 3.11 is now the minimum supported version. diff --git a/doc/explanation/patch.rst b/doc/explanation/patch.rst index 156ebe0c..549bd824 100644 --- a/doc/explanation/patch.rst +++ b/doc/explanation/patch.rst @@ -194,15 +194,22 @@ a selection matching nothing means for it, and Table 9 of :rfc:`RFC7644 §3.12 < defines ``noTarget`` for a filter that "yields no match". The filter selects the entries to write. It does not describe an entry to create. So the operation fails instead of silently doing nothing. -`Errata 8097 `_ asks the RFC to say whether ``add`` -accepts a value selection at all, since implementations differ on it. -.. note:: - - Microsoft Entra ID sends ``add`` operations whose selection matches nothing, such as - ``emails[type eq "work"].value`` for a user without a work email, and expects the selected - entry to be created. scim2-models answers ``noTarget``, so an integration serving that client - creates the entry before applying the operation. +Implementations differ here. UnboundID's SCIM 2 SDK, Apache SCIMple and scim-patch create the +entry on ``add``. SCIM-SDK returns ``noTarget``, and so does WSO2 Charon unless the attribute is +unassigned. `Errata 8097 `_ describes the creation +Microsoft Entra ID expects. It is held, with no corrected text. The editor of :rfc:`7644` +`replied `_ that +such an implementation is "going outside the spec at the cost of their interoperability". + +Entra sends such operations to fill an attribute it has not set yet. It sends ``add`` by +default, and ``replace`` when its ``aadOptscim062020`` flag is set. For example, it targets +``emails[type eq "work"].value`` on a user without a work email, and expects a new entry +``{"type": "work", "value": ...}``. +:attr:`ScimPolicy.UnmatchedPathFilter.create ` +creates that entry for both operations. It only works with ``eq`` comparisons joined by ``and``. +The new entry must still match the filter after the value is written, so that later operations +with the same filter find it. See :doc:`../how-to/tolerate-a-nonconformant-peer`. What a remove selects --------------------- diff --git a/doc/how-to/tolerate-a-nonconformant-peer.rst b/doc/how-to/tolerate-a-nonconformant-peer.rst index 3adbdf0b..6bd52eed 100644 --- a/doc/how-to/tolerate-a-nonconformant-peer.rst +++ b/doc/how-to/tolerate-a-nonconformant-peer.rst @@ -115,6 +115,38 @@ which :rfc:`RFC7644 §3.5.2.2 <7644#section-3.5.2.2>` asks for a membership that Entra documents this form as non-conformant, and its ``aadOptscim062020`` tenant flag makes it send a filter path instead. Setting that flag is the other way out. +Create the entry an Entra filter describes +------------------------------------------ + +Microsoft Entra ID fills an attribute it has not set yet through a filter, such as +``emails[type eq "work"].value`` on a user without a work email. The filter matches nothing, and +scim2-models returns ``noTarget``. Set +:attr:`~scim2_models.ScimPolicy.UnmatchedPathFilter.create` to add the entry the filter describes +instead: + +.. doctest:: + + >>> patch = PatchOp[User]( + ... operations=[ + ... PatchOperation( + ... op=PatchOperation.Op.add, + ... path='emails[type eq "work"].value', + ... value="bjensen@example.com", + ... ) + ... ] + ... ) + >>> creating = ScimPolicy(unmatched_path_filter=ScimPolicy.UnmatchedPathFilter.create) + >>> user = User(user_name="bjensen") + >>> patch.patch(user, scim_policy=creating) + True + >>> [(email.type.value, email.value) for email in user.emails] + [('work', 'bjensen@example.com')] + +The setting covers ``add`` and ``replace``, since Entra sends ``replace`` once its +``aadOptscim062020`` tenant flag is set. It only works with ``eq`` comparisons joined by ``and``. +Any other filter still returns ``noTarget``, and so does a filter on an attribute without +sub-attributes. :doc:`../explanation/patch` explains why this is not the default. + State a policy once per request ------------------------------- diff --git a/scim2_models/messages/patch_op.py b/scim2_models/messages/patch_op.py index d27daf55..6817dfc0 100644 --- a/scim2_models/messages/patch_op.py +++ b/scim2_models/messages/patch_op.py @@ -1,5 +1,6 @@ import copy from collections.abc import Iterator +from dataclasses import replace from enum import StrEnum from inspect import isclass from typing import Annotated @@ -29,6 +30,11 @@ from ..exceptions import MutabilityException from ..exceptions import NoTargetException from ..exceptions import SCIMException +from ..path import CompareOperator +from ..path import Comparison +from ..path import FilterNode +from ..path import LogicalExpr +from ..path import LogicalOperator from ..path import Path from ..path import ScimFilter from ..path.access import _select @@ -282,7 +288,9 @@ def patch(self, resource: ResourceT, scim_policy: ScimPolicy | None = None) -> b changes a read-only value through its value, or leaves a required attribute unassigned. :raises NoTargetException: If the path filter of an ``add`` or - ``replace`` matches no value. + ``replace`` matches no value, and + :attr:`~scim2_models.ScimPolicy.unmatched_path_filter` creates no + entry for it. """ snapshot = resource.model_copy(deep=True) try: @@ -456,7 +464,7 @@ def _apply_operation( if operation.op == PatchOperation.Op.remove: if removal is not None: removal.delete(resource) - else: + elif not _create_described_entry(resource, path, operation.value, policy): _write(resource, path, writes, is_add=operation.op == PatchOperation.Op.add) except ValidationError as exc: raise InvalidValueException(detail=str(exc)) from exc @@ -562,6 +570,67 @@ def _declared(parent: Path[Any], text: str, policy: ScimPolicy) -> Path[Any] | N ) +def _create_described_entry( + resource: Resource[Any], path: Path[Any], value: Any, policy: ScimPolicy +) -> bool: + """Add the entry an unmatched path filter describes, under the create policy. + + The entry gets the values the eq comparisons of the filter compare, then + the value of the operation. It must still match the filter, so that later + operations find it. + """ + value_path = path._as_value_path() + if ( + value_path is None + or policy.unmatched_path_filter != ScimPolicy.UnmatchedPathFilter.create + ): + return False + + selection = type(path)(str(replace(value_path, sub_attr=None))) + described = _described_entry(value_path.val_filter) + binding = selection.resolve() + written = ( + {value_path.sub_attr: value} if value_path.sub_attr else _as_payload(value) + ) + if ( + selection.get(resource) is not None + or described is None + or binding is None + or not _is_model(binding.field_type) + or not isinstance(written, dict) + ): + return False + + type(path)(str(value_path.attr_path)).set( + resource, {**described, **written}, is_add=True + ) + if selection.get(resource) is None: + raise InvalidValueException( + detail=f"the value contradicts the filter of '{path}'" + ) + return True + + +def _described_entry(val_filter: FilterNode) -> dict[str, Any] | None: + """Return the sub-attribute values set by eq comparisons joined by and.""" + terms = ( + val_filter.terms + if isinstance(val_filter, LogicalExpr) and val_filter.op == LogicalOperator.and_ + else (val_filter,) + ) + comparisons = [ + term + for term in terms + if isinstance(term, Comparison) + and term.op == CompareOperator.eq + and term.attr_path.sub_attr is None + and term.attr_path.uri is None + ] + if len(comparisons) != len(terms): + return None + return {comparison.attr_path.attr: comparison.value for comparison in comparisons} + + def _snapshot( resource: Resource[Any], paths: list[Path[Any]], memo: dict[int, Any] ) -> Resource[Any]: diff --git a/scim2_models/policy.py b/scim2_models/policy.py index 2efd7963..fc004ba8 100644 --- a/scim2_models/policy.py +++ b/scim2_models/policy.py @@ -98,12 +98,35 @@ class RemoveValue(StrEnum): apply = "apply" """The ``value`` selects what to remove, as Microsoft Entra sends it.""" + class UnmatchedPathFilter(StrEnum): + """What becomes of a PATCH operation whose path filter matches no entry.""" + + forbid = "forbid" + """The ``add`` or ``replace`` operation is rejected with ``noTarget``. + + :rfc:`RFC7644 §3.5.2.3 <7644#section-3.5.2.3>` requires it for + ``replace``, and Table 9 of :rfc:`RFC7644 §3.12 <7644#section-3.12>` + defines ``noTarget`` for a filter that "yields no match". + """ + + create = "create" + """The entry the filter describes is added, as Microsoft Entra expects. + + Only ``eq`` comparisons on sub-attributes, joined by ``and``, describe + an entry. The entry gets the compared values, then the operation + writes its value. Any other filter, and any ``remove``, is still + rejected with ``noTarget``. + """ + unknown: Unknown = Unknown.forbid """What becomes of an attribute no model declares.""" remove_value_as_filter: RemoveValue = RemoveValue.forbid """What becomes of a PATCH ``remove`` operation carrying a ``value``.""" + unmatched_path_filter: UnmatchedPathFilter = UnmatchedPathFilter.forbid + """What becomes of a PATCH operation whose path filter matches no entry.""" + def __enter__(self) -> "ScimPolicy": """Make this policy the one every call in the block runs under.""" _AMBIENT_POLICIES.set(_AMBIENT_POLICIES.get() + (self,)) diff --git a/tests/test_policy.py b/tests/test_policy.py index 073475ca..88b5de5e 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -2,6 +2,7 @@ import asyncio import threading +from typing import Annotated from typing import Any import pytest @@ -18,11 +19,16 @@ from scim2_models import CreationRequestContext from scim2_models import CreationResponseContext from scim2_models import EnterpriseUser +from scim2_models import Extension from scim2_models import Group from scim2_models import InvalidFilterException from scim2_models import InvalidValueException +from scim2_models import Mutability +from scim2_models import MutabilityException +from scim2_models import NoTargetException from scim2_models import PatchOp from scim2_models import PatchOperation +from scim2_models import Path from scim2_models import Resource from scim2_models import ScimPolicy from scim2_models import ScimProvider @@ -715,3 +721,222 @@ def test_an_ambient_policy_reaches_a_remove_carrying_a_value(): entra_remove([{"value": "s-foobar"}]).patch(group) assert [member.value for member in group.members] == ["autre"] + + +# A path filter matching nothing + + +CREATE = ScimPolicy(unmatched_path_filter=ScimPolicy.UnmatchedPathFilter.create) + + +class Tag(ComplexAttribute): + kind: str | None = None + stamp: Annotated[str | None, Mutability.read_only] = None + + +class Tagged(Resource): + __schema__ = URN("urn:example:2.0:Tagged") + + tags: list[Tag] | None = None + sealed: Annotated[list[Tag] | None, Mutability.immutable] = None + labels: list[str] | None = None + + +class Tagging(Extension): + __schema__ = URN("urn:example:2.0:Tagging") + + tags: list[Tag] | None = None + + +def unmatched_patch(model: Any, *operations: dict[str, Any]) -> "PatchOp[Any]": + """Build a patch from raw operations on the given resource type.""" + return PatchOp[model].model_validate({"Operations": list(operations)}) + + +def test_an_add_whose_filter_matches_nothing_has_no_target_by_default(): + """Table 9 of RFC7644 §3.12 defines noTarget for a filter that yields no match.""" + patch = unmatched_patch( + User, + {"op": "add", "path": 'emails[type eq "work"].value', "value": "w@example.com"}, + ) + + with pytest.raises(NoTargetException): + patch.patch(User(user_name="bjensen")) + + +@pytest.mark.parametrize("op", ["add", "replace"]) +def test_the_entry_a_filter_describes_is_created_under_create(op): + """Microsoft Entra fills an absent work email through a filter, with add or replace.""" + user = User(user_name="bjensen") + patch = unmatched_patch( + User, + {"op": op, "path": 'emails[type eq "work"].value', "value": "w@example.com"}, + ) + + assert patch.patch(user, scim_policy=CREATE) + + assert [(email.type, email.value) for email in user.emails] == [ + ("work", "w@example.com") + ] + + +def test_the_operations_with_the_same_filter_reach_the_created_entry(): + """Entra spreads one entry over several operations that share a filter.""" + user = User(user_name="bjensen") + patch = unmatched_patch( + User, + {"op": "add", "path": 'emails[type eq "work"].value', "value": "w@example.com"}, + {"op": "add", "path": 'emails[type eq "work"].display', "value": "Work"}, + ) + + assert patch.patch(user, scim_policy=CREATE) + + assert len(user.emails) == 1 + assert user.emails[0].display == "Work" + + +def test_a_value_is_merged_into_the_created_entry(): + """The created entry also takes the sub-attributes in the value.""" + user = User(user_name="bjensen") + patch = unmatched_patch( + User, + { + "op": "add", + "path": 'emails[type eq "work"]', + "value": {"value": "w@example.com"}, + }, + ) + + assert patch.patch(user, scim_policy=CREATE) + + assert (user.emails[0].type, user.emails[0].value) == ("work", "w@example.com") + + +def test_a_created_primary_entry_becomes_the_only_primary(): + """The filter literal is read as ScimFilter compares it, and primary stays unique.""" + user = User( + user_name="bjensen", + emails=[{"type": "home", "value": "h@example.com", "primary": True}], + ) + patch = unmatched_patch( + User, + { + "op": "replace", + "path": 'emails[type eq "work" and primary eq "True"].value', + "value": "w@example.com", + }, + ) + + assert patch.patch(user, scim_policy=CREATE) + + assert [(email.type, email.primary) for email in user.emails] == [ + ("home", False), + ("work", True), + ] + + +def test_a_value_contradicting_the_filter_is_refused(): + """The created entry must match its filter, for a later operation to reach it.""" + user = User(user_name="bjensen") + patch = unmatched_patch( + User, + { + "op": "add", + "path": 'emails[type eq "work"]', + "value": {"type": "home", "value": "h@example.com"}, + }, + ) + + with pytest.raises(InvalidValueException): + patch.patch(user, scim_policy=CREATE) + assert user.emails is None + + +@pytest.mark.parametrize( + "path", + [ + 'emails[type eq "work" or type eq "home"].value', + 'emails[type ne "work"].value', + 'emails[not (type eq "work")].value', + 'emails[display co "Work"].value', + "emails[type pr].value", + ], +) +def test_a_filter_describing_no_entry_still_has_no_target(path): + """Only eq comparisons joined by and describe the entry to create.""" + patch = unmatched_patch(User, {"op": "add", "path": path, "value": "w@example.com"}) + + with pytest.raises(NoTargetException): + patch.patch(User(user_name="bjensen"), scim_policy=CREATE) + + +def test_a_filter_over_simple_values_still_has_no_target(): + """A multi-valued attribute without sub-attributes has no entry to describe.""" + patch = unmatched_patch( + Tagged, {"op": "replace", "path": 'labels[value eq "red"]', "value": "green"} + ) + + with pytest.raises(NoTargetException): + patch.patch(Tagged(labels=["blue"]), scim_policy=CREATE) + + +def test_a_remove_whose_filter_matches_nothing_creates_nothing(): + """Per RFC7644 §3.5.2.2, a remove that selects nothing succeeds without change.""" + user = User(user_name="bjensen") + patch = unmatched_patch(User, {"op": "remove", "path": 'emails[type eq "work"]'}) + + assert patch.patch(user, scim_policy=CREATE) is False + assert user.emails is None + + +def test_a_created_entry_cannot_set_a_read_only_sub_attribute(): + """The created entry goes through the same checks as an add of that entry.""" + patch = unmatched_patch( + Tagged, {"op": "add", "path": 'tags[stamp eq "x"].kind', "value": "k"} + ) + + with pytest.raises(MutabilityException): + patch.patch(Tagged(), scim_policy=CREATE) + + +def test_no_entry_is_created_in_an_immutable_attribute_holding_values(): + """An immutable multi-valued attribute takes no new entry once assigned.""" + patch = unmatched_patch( + Tagged, {"op": "add", "path": 'sealed[kind eq "b"].kind', "value": "b"} + ) + + with pytest.raises(MutabilityException): + patch.patch(Tagged(sealed=[{"kind": "a"}]), scim_policy=CREATE) + + +def test_an_unassigned_extension_is_created_with_the_entry(): + """An extension attribute is selected into as a core one is.""" + resource = Tagged[Tagging]() + patch = unmatched_patch( + Tagged[Tagging], + {"op": "add", "path": 'urn:example:2.0:Tagging:tags[kind eq "b"]', "value": {}}, + ) + + assert patch.patch(resource, scim_policy=CREATE) + + assert resource[Tagging].tags == [Tag(kind="b")] + + +def test_an_ambient_policy_reaches_the_creation(): + """A server may state the policy once per request with a block.""" + user = User(user_name="bjensen") + patch = unmatched_patch( + User, + {"op": "add", "path": 'emails[type eq "work"].value', "value": "w@example.com"}, + ) + + with CREATE: + assert patch.patch(user) + + +def test_path_set_creates_nothing_under_create(): + """The policy only applies to PATCH, and Path.set still reports the missing target.""" + with CREATE, pytest.raises(NoTargetException): + Path[User]('emails[type eq "work"].value').set( + User(user_name="bjensen"), "w@example.com" + )