Skip to content

Commit 02bef20

Browse files
azmeukazmeuk
committed
fix: refuse a patch path naming an attribute the schema does not declare
1 parent 1315d5d commit 02bef20

6 files changed

Lines changed: 196 additions & 227 deletions

File tree

‎doc/changelog.rst‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,9 @@ Changed
7373
when applied. It used to remove the entries equal to that ``value``, and to report no change
7474
when the ``value`` was a list or described an entry only in part. Set
7575
:attr:`~scim2_models.ScimPolicy.remove_value_as_filter` to keep reading it.
76+
- A PATCH reaching an extension attribute takes the extended resource type, as in
77+
``PatchOp[User[EnterpriseUser]]``. ``PatchOp[User]`` used to carry such an operation to the
78+
endpoint, and now refuses a path its type parameter leaves out.
7679

7780
Removed
7881
^^^^^^^
@@ -107,6 +110,9 @@ Fixed
107110
- A PATCH operation carrying no ``path`` accepts a resource as its ``value``, and checks the
108111
attributes it names against the model. They used to go through unexamined, so a client naming
109112
an attribute it had misspelled was answered success.
113+
- A PATCH operation whose ``path`` names an attribute the resource schema does not declare is
114+
refused with ``invalidPath``. It used to pass, so a client that misspelled an attribute was
115+
answered success without anything being written. :issue:`164`
110116
- A refused PATCH ``add`` on a multi-valued attribute leaves the attribute as it was. The entry
111117
used to be appended before being validated, and outlived the failure.
112118
- A PATCH operation carrying no ``path`` marks the attributes it assigns as set, so

‎doc/explanation/policies.rst‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,13 +46,13 @@ stand in for one.
4646
What a policy leaves alone
4747
--------------------------
4848

49-
**PATCH paths stay strict.** An operation whose ``path`` names an attribute no model declares
50-
raises :class:`~scim2_models.PathNotFoundException`, whatever the policy says. Path resolution and
51-
unknown attributes are two separate mechanisms, and making them uniform would take a third. The
52-
default that would come out of it is the wrong one: a server would answer 200 to a modification it
53-
never applied, where :rfc:`RFC7644 §3.5.2 <7644#section-3.5.2>` asks for an error. Inside the body
54-
of a resource the trade is different, since dropping one unknown attribute still lands everything
55-
the peer and the model both knew.
49+
**PATCH paths stay strict.** An operation whose ``path`` names an attribute no model declares is
50+
refused with ``invalidPath``, whatever the policy says. Path resolution and unknown attributes are
51+
two separate mechanisms, and making them uniform would take a third. The default that would come
52+
out of it is the wrong one: a server would answer 200 to a modification it never applied, where
53+
:rfc:`RFC7644 §3.5.2 <7644#section-3.5.2>` asks for an error. Inside the body of a resource the
54+
trade is different, since dropping one unknown attribute still lands everything the peer and the
55+
model both knew.
5656

5757
**Building a model in Python stays strict.** ``User(bogus=1)`` and ``user.bogus = 1`` raise under
5858
every policy. Pydantic only offers a hook for extra keys during validation, so a keyword argument

‎scim2_models/messages/patch_op.py‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
from ..exceptions import InvalidValueException
2323
from ..exceptions import MutabilityException
2424
from ..exceptions import NoTargetException
25+
from ..exceptions import PathNotFoundException
2526
from ..path import Path
2627
from ..path import ScimFilter
2728
from ..path import attribute_host
@@ -500,6 +501,11 @@ def validate_operations(self, info: ValidationInfo) -> Self:
500501
# targets, as a constraint on a complex attribute governs everything
501502
# written under it: "meta" is read-only where "meta.version" is not.
502503
if (resolved := operation.path.resolve()) is None:
504+
if operation.path.model is None:
505+
raise PathNotFoundException(
506+
path=str(operation.path),
507+
detail=f"path '{operation.path}' is not declared by the resource schema",
508+
).as_pydantic_error()
503509
continue
504510
operation._validate_mutability(resolved.model, resolved.field_name)
505511
operation._validate_required_attribute(

‎tests/test_models.py‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,18 @@ def _error_summary(exc: ValidationError) -> list[tuple[str, tuple]]:
8282
}
8383

8484

85+
SAMPLE_MODEL_OVERRIDES = {
86+
"rfc7644-3.5.2.1-patch_op-add_members.json": PatchOp[Group],
87+
"rfc7644-3.5.2.2-patch_op-remove_all_members.json": PatchOp[Group],
88+
"rfc7644-3.5.2.2-patch_op-remove_and_add_one_member.json": PatchOp[Group],
89+
"rfc7644-3.5.2.2-patch_op-remove_one_member.json": PatchOp[Group],
90+
"rfc7644-3.5.2.3-patch_op-replace_all_members.json": PatchOp[Group],
91+
}
92+
93+
8594
def sample_model(sample: str) -> type:
95+
if sample in SAMPLE_MODEL_OVERRIDES:
96+
return SAMPLE_MODEL_OVERRIDES[sample]
8697
return SAMPLE_MODELS[sample.removesuffix(".json").split("-")[2]]
8798

8899

‎tests/test_patch_op_extensions.py‎

Lines changed: 50 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
from scim2_models import InvalidPathException
99
from scim2_models import PatchOp
1010
from scim2_models import PatchOperation
11-
from scim2_models import PathNotFoundException
1211
from scim2_models import User
1312
from scim2_models.resources.enterprise_user import EnterpriseUser
1413
from scim2_models.resources.resource import Resource
@@ -26,9 +25,9 @@ def test_patch_operation_extension_simple_attribute():
2625
}
2726
)
2827

29-
patch1 = PatchOp[User](
28+
patch1 = PatchOp[User[EnterpriseUser]](
3029
operations=[
31-
PatchOperation[User](
30+
PatchOperation[User[EnterpriseUser]](
3231
op=PatchOperation.Op.replace_,
3332
path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber",
3433
value="54321",
@@ -39,9 +38,9 @@ def test_patch_operation_extension_simple_attribute():
3938
assert result is True
4039
assert user[EnterpriseUser].employee_number == "54321"
4140

42-
patch2 = PatchOp[User](
41+
patch2 = PatchOp[User[EnterpriseUser]](
4342
operations=[
44-
PatchOperation[User](
43+
PatchOperation[User[EnterpriseUser]](
4544
op=PatchOperation.Op.add,
4645
path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:organization",
4746
value="ACME Corp",
@@ -52,9 +51,9 @@ def test_patch_operation_extension_simple_attribute():
5251
assert result is True
5352
assert user[EnterpriseUser].organization == "ACME Corp"
5453

55-
patch3 = PatchOp[User](
54+
patch3 = PatchOp[User[EnterpriseUser]](
5655
operations=[
57-
PatchOperation[User](
56+
PatchOperation[User[EnterpriseUser]](
5857
op=PatchOperation.Op.remove,
5958
path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:costCenter",
6059
)
@@ -77,9 +76,9 @@ def test_patch_operation_extension_complex_attribute():
7776
}
7877
)
7978

80-
patch1 = PatchOp[User](
79+
patch1 = PatchOp[User[EnterpriseUser]](
8180
operations=[
82-
PatchOperation[User](
81+
PatchOperation[User[EnterpriseUser]](
8382
op=PatchOperation.Op.replace_,
8483
path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager.value",
8584
value="new-manager-456",
@@ -91,9 +90,9 @@ def test_patch_operation_extension_complex_attribute():
9190
assert user[EnterpriseUser].manager.value == "new-manager-456"
9291
assert user[EnterpriseUser].manager.display_name == "John Smith"
9392

94-
patch2 = PatchOp[User](
93+
patch2 = PatchOp[User[EnterpriseUser]](
9594
operations=[
96-
PatchOperation[User](
95+
PatchOperation[User[EnterpriseUser]](
9796
op=PatchOperation.Op.replace_,
9897
path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager",
9998
value={
@@ -109,9 +108,9 @@ def test_patch_operation_extension_complex_attribute():
109108
assert user[EnterpriseUser].manager.value == "super-manager-789"
110109
assert user[EnterpriseUser].manager.display_name == "Alice Johnson"
111110

112-
patch3 = PatchOp[User](
111+
patch3 = PatchOp[User[EnterpriseUser]](
113112
operations=[
114-
PatchOperation[User](
113+
PatchOperation[User[EnterpriseUser]](
115114
op=PatchOperation.Op.remove,
116115
path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager",
117116
)
@@ -139,9 +138,9 @@ def test_patch_operation_extension_mutability_handled_by_model():
139138

140139
# This operation would fail during model validation for mutability,
141140
# but patch method assumes operations are already validated
142-
patch = PatchOp[User](
141+
patch = PatchOp[User[EnterpriseUser]](
143142
operations=[
144-
PatchOperation[User](
143+
PatchOperation[User[EnterpriseUser]](
145144
op=PatchOperation.Op.replace_,
146145
path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber",
147146
value="12345",
@@ -154,38 +153,30 @@ def test_patch_operation_extension_mutability_handled_by_model():
154153

155154

156155
def test_patch_operation_extension_invalid_path_error():
157-
"""Test invalidPath error for non-existent extension attributes.
158-
159-
:rfc:`RFC7644 §3.5.2 <7644#section-3.5.2>`: invalidPath errors occur when
160-
the path references an attribute that doesn't exist in the schema.
161-
"""
162-
user = User[EnterpriseUser].model_validate({"userName": "test.user"})
163-
164-
patch1 = PatchOp[User](
165-
operations=[
166-
PatchOperation[User](
167-
op=PatchOperation.Op.add,
168-
path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:invalidAttribute",
169-
value="test",
170-
)
171-
]
172-
)
173-
with pytest.raises(InvalidPathException):
174-
patch1.patch(user)
175-
assert user[EnterpriseUser] is None
156+
"""An attribute the extension does not declare is refused, and so is its sub-attribute."""
157+
with pytest.raises(ValidationError) as raised:
158+
PatchOp[User[EnterpriseUser]](
159+
operations=[
160+
PatchOperation[User[EnterpriseUser]](
161+
op=PatchOperation.Op.add,
162+
path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:invalidAttribute",
163+
value="test",
164+
)
165+
]
166+
)
167+
assert raised.value.errors()[0]["type"] == "scim_invalidPath"
176168

177-
patch2 = PatchOp[User](
178-
operations=[
179-
PatchOperation[User](
180-
op=PatchOperation.Op.add,
181-
path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager.invalidField",
182-
value="test",
183-
)
184-
]
185-
)
186-
with pytest.raises(InvalidPathException):
187-
patch2.patch(user)
188-
assert user[EnterpriseUser] is None
169+
with pytest.raises(ValidationError) as raised:
170+
PatchOp[User[EnterpriseUser]](
171+
operations=[
172+
PatchOperation[User[EnterpriseUser]](
173+
op=PatchOperation.Op.add,
174+
path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager.invalidField",
175+
value="test",
176+
)
177+
]
178+
)
179+
assert raised.value.errors()[0]["type"] == "scim_invalidPath"
189180

190181

191182
def test_urn_parsing_errors():
@@ -230,17 +221,16 @@ class TestResourceTypeVar(Resource):
230221

231222
typevar_field: T = None
232223

233-
user = TestResourceTypeVar()
234-
patch = PatchOp[TestResourceTypeVar](
235-
operations=[
236-
PatchOperation[TestResourceTypeVar](
237-
op=PatchOperation.Op.add, path="typevarField.subfield", value="test"
238-
)
239-
]
240-
)
224+
with pytest.raises(ValidationError) as raised:
225+
PatchOp[TestResourceTypeVar](
226+
operations=[
227+
PatchOperation[TestResourceTypeVar](
228+
op=PatchOperation.Op.add, path="typevarField.subfield", value="test"
229+
)
230+
]
231+
)
241232

242-
with pytest.raises(PathNotFoundException):
243-
patch.patch(user)
233+
assert raised.value.errors()[0]["type"] == "scim_invalidPath"
244234

245235

246236
def test_add_creates_the_parent_of_a_complex_attribute():
@@ -270,9 +260,9 @@ def test_patch_extension_schema_path_without_attribute():
270260
)
271261
user[EnterpriseUser] = EnterpriseUser()
272262

273-
patch = PatchOp[User](
263+
patch = PatchOp[User[EnterpriseUser]](
274264
operations=[
275-
PatchOperation[User](
265+
PatchOperation[User[EnterpriseUser]](
276266
op=PatchOperation.Op.add,
277267
path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User",
278268
value={
@@ -350,9 +340,9 @@ def test_patch_delete_extension_root():
350340
assert user[EnterpriseUser].employee_number == "12345"
351341
assert user[EnterpriseUser].cost_center == "Engineering"
352342

353-
patch = PatchOp[User](
343+
patch = PatchOp[User[EnterpriseUser]](
354344
operations=[
355-
PatchOperation[User](
345+
PatchOperation[User[EnterpriseUser]](
356346
op=PatchOperation.Op.remove,
357347
path="urn:ietf:params:scim:schemas:extension:enterprise:2.0:User",
358348
)

0 commit comments

Comments
 (0)