Skip to content

Commit fa7ff91

Browse files
azmeukazmeuk
authored
fix: mutability and required checks resolve the field name (#157)
The readOnly, immutable and required constraints of a PATCH operation were looked up with `parts[0]`, the attribute name as the client spelled it, against `model_fields`, whose keys are Python field names. The guard was skipped whenever the two differed, so removing `userName` succeeded on a required attribute, and RFC7643 §2.1 making attribute names case-insensitive, a path spelled `GROUPS` wrote to a readOnly one. The three checks now resolve the name through `_find_field_name` and only give up when nothing matches. Error messages keep the client's spelling, which is what a SCIM peer understands.
1 parent 735f468 commit fa7ff91

3 files changed

Lines changed: 106 additions & 17 deletions

File tree

‎doc/changelog.rst‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,11 @@ Fixed
6666
- :meth:`~scim2_models.Resource.from_schema` no longer crashes on ``reference`` attributes missing the optional ``referenceTypes``, and reads them as :class:`~scim2_models.URI` references.
6767
- Looking a model up by schema no longer crashes when the model list mixes resources with messages such as :class:`~scim2_models.ListResponse`.
6868
- Check recursively extensions' replace constraints.
69+
- The ``readOnly``, ``immutable`` and ``required`` constraints of a PATCH operation are checked
70+
against the attribute its path resolves to, instead of against a literal field name match.
71+
They used to be skipped whenever the two differed, so ``userName`` could be removed and a
72+
path spelled ``GROUPS`` could write to a ``readOnly`` attribute, :rfc:`7643` §2.1 making
73+
attribute names case-insensitive.
6974

7075
Removed
7176
^^^^^^^

‎scim2_models/messages/patch_op.py‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,27 @@
2222
from ..path import URN
2323
from ..path import Path
2424
from ..resources.resource import Resource
25+
from ..utils import _find_field_name
2526
from .message import Message
2627
from .message import _get_resource_class
2728

2829
ResourceT = TypeVar("ResourceT", bound=Resource[Any])
2930

3031

32+
def _resolved_field(
33+
resource_class: type[Resource[Any]], attr_name: str | None
34+
) -> str | None:
35+
"""Return the Python field a SCIM attribute name designates.
36+
37+
Attribute names are case-insensitive per :rfc:`RFC7643 §2.1 <7643#section-2.1>`
38+
and differ from the field names of the model, so the constraint checks
39+
resolve the name instead of matching it against ``model_fields``.
40+
"""
41+
if attr_name is None:
42+
return None
43+
return _find_field_name(resource_class, attr_name)
44+
45+
3146
class PatchOperation(ComplexAttribute, Generic[ResourceT]):
3247
class Op(str, Enum):
3348
replace_ = "replace"
@@ -50,7 +65,7 @@ class Op(str, Enum):
5065
describing the target of the operation."""
5166

5267
def _validate_mutability(
53-
self, resource_class: type[Resource[Any]], field_name: str
68+
self, resource_class: type[Resource[Any]], field_name: str | None
5469
) -> None:
5570
"""Validate mutability constraints at parse-time.
5671
@@ -59,29 +74,29 @@ def _validate_mutability(
5974
to the resource instance and is enforced at runtime in
6075
:meth:`PatchOp._check_immutable`.
6176
"""
62-
if field_name not in resource_class.model_fields:
77+
if (field := _resolved_field(resource_class, field_name)) is None:
6378
return
6479

65-
mutability = resource_class.get_field_annotation(field_name, Mutability)
80+
mutability = resource_class.get_field_annotation(field, Mutability)
6681

6782
if mutability == Mutability.read_only:
6883
raise MutabilityException(
6984
attribute=field_name, mutability="readOnly", operation=self.op.value
7085
).as_pydantic_error()
7186

7287
def _validate_required_attribute(
73-
self, resource_class: type[Resource[Any]], field_name: str
88+
self, resource_class: type[Resource[Any]], field_name: str | None
7489
) -> None:
7590
"""Validate required attribute constraints for remove operations."""
7691
# RFC 7644 Section 3.5.2.3: Only validate for remove operations
7792
if self.op != PatchOperation.Op.remove:
7893
return
7994

8095
# RFC 7644 Section 3.5.2: "Servers should be tolerant of schema extensions"
81-
if field_name not in resource_class.model_fields:
96+
if (field := _resolved_field(resource_class, field_name)) is None:
8297
return
8398

84-
required = resource_class.get_field_annotation(field_name, Required)
99+
required = resource_class.get_field_annotation(field, Required)
85100

86101
# RFC 7643 Section 7: "Required attributes SHALL NOT be removed"
87102
if required == Required.true:
@@ -238,8 +253,8 @@ def validate_operations(self, info: ValidationInfo) -> Self:
238253
continue
239254

240255
field_name = operation.path.parts[0] if operation.path.parts else None
241-
operation._validate_mutability(resource_class, field_name) # type: ignore[arg-type]
242-
operation._validate_required_attribute(resource_class, field_name) # type: ignore[arg-type]
256+
operation._validate_mutability(resource_class, field_name)
257+
operation._validate_required_attribute(resource_class, field_name)
243258

244259
return self
245260

@@ -311,14 +326,14 @@ def _check_immutable(
311326
resource_class = type(resource)
312327
assert operation.path is not None
313328
field_name = operation.path.parts[0] if operation.path.parts else None
314-
if field_name is None or field_name not in resource_class.model_fields:
329+
if (field := _resolved_field(resource_class, field_name)) is None:
315330
return
316331

317-
mutability = resource_class.get_field_annotation(field_name, Mutability)
332+
mutability = resource_class.get_field_annotation(field, Mutability)
318333
if mutability != Mutability.immutable:
319334
return
320335

321-
current_value = getattr(resource, field_name, None)
336+
current_value = getattr(resource, field, None)
322337

323338
if operation.op == PatchOperation.Op.add and current_value is None:
324339
return

‎tests/test_patch_op_validation.py‎

Lines changed: 75 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -121,22 +121,22 @@ def test_validate_patchop_case_insensitivity():
121121
{
122122
"schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"],
123123
"operations": [
124-
{"op": "Replace", "path": "userName", "value": "Rivard"},
125-
{"op": "ADD", "path": "userName", "value": "Rivard"},
126-
{"op": "ReMove", "path": "userName", "value": "Rivard"},
124+
{"op": "Replace", "path": "displayName", "value": "Rivard"},
125+
{"op": "ADD", "path": "displayName", "value": "Rivard"},
126+
{"op": "ReMove", "path": "displayName", "value": "Rivard"},
127127
],
128128
},
129129
) == PatchOp[User](
130130
schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"],
131131
operations=[
132132
PatchOperation[User](
133-
op=PatchOperation.Op.replace_, path="userName", value="Rivard"
133+
op=PatchOperation.Op.replace_, path="displayName", value="Rivard"
134134
),
135135
PatchOperation[User](
136-
op=PatchOperation.Op.add, path="userName", value="Rivard"
136+
op=PatchOperation.Op.add, path="displayName", value="Rivard"
137137
),
138138
PatchOperation[User](
139-
op=PatchOperation.Op.remove, path="userName", value="Rivard"
139+
op=PatchOperation.Op.remove, path="displayName", value="Rivard"
140140
),
141141
],
142142
)
@@ -683,3 +683,72 @@ def test_patch_op_operations_attribute_required_in_patch_context():
683683
context={"scim": Context.RESOURCE_PATCH_REQUEST},
684684
)
685685
assert len(patch_op.operations) == 1
686+
687+
688+
@pytest.mark.parametrize(
689+
"path",
690+
[
691+
"groups",
692+
"GROUPS",
693+
"Groups",
694+
"urn:ietf:params:scim:schemas:core:2.0:User:groups",
695+
],
696+
)
697+
def test_a_read_only_attribute_is_protected_whatever_its_spelling(path):
698+
"""Attribute names are case-insensitive per :rfc:`RFC7643 §2.1 <7643#section-2.1>`.
699+
700+
``groups`` is readOnly, so every spelling designating it must be refused.
701+
"""
702+
with pytest.raises(ValidationError, match="mutability"):
703+
PatchOp[User](
704+
operations=[
705+
PatchOperation[User](
706+
op=PatchOperation.Op.replace_,
707+
path=path,
708+
value=[{"value": "group-id"}],
709+
)
710+
]
711+
)
712+
713+
714+
@pytest.mark.parametrize(
715+
"path",
716+
["userName", "USERNAME", "urn:ietf:params:scim:schemas:core:2.0:User:userName"],
717+
)
718+
def test_a_required_attribute_cannot_be_removed_whatever_its_spelling(path):
719+
"""``userName`` is required, and its SCIM name differs from its field name."""
720+
with pytest.raises(ValidationError, match="required attribute cannot be removed"):
721+
PatchOp[User](
722+
operations=[PatchOperation[User](op=PatchOperation.Op.remove, path=path)]
723+
)
724+
725+
726+
def test_an_immutable_attribute_is_protected_whatever_its_spelling():
727+
"""The runtime check reads the current value, so it resolves the name too."""
728+
resource = ImmutableFieldResource(locked="original")
729+
patch = PatchOp[ImmutableFieldResource](
730+
operations=[
731+
PatchOperation[ImmutableFieldResource](
732+
op=PatchOperation.Op.replace_, path="LOCKED", value="hijacked"
733+
)
734+
]
735+
)
736+
737+
with pytest.raises(MutabilityException):
738+
patch.patch(resource)
739+
assert resource.locked == "original"
740+
741+
742+
def test_an_operation_on_the_resource_root_has_no_attribute_to_check():
743+
"""An empty path designates the resource itself, so no constraint applies."""
744+
resource = User(user_name="bjensen")
745+
patch = PatchOp[User](
746+
operations=[
747+
PatchOperation[User](
748+
op=PatchOperation.Op.add, path="", value={"displayName": "Barbara"}
749+
)
750+
]
751+
)
752+
753+
assert patch.patch(resource)
754+
assert resource.display_name == "Barbara"

0 commit comments

Comments
 (0)