From cbe314247b913ebaad154a106c92f0f27ea82472 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89loi=20Rivard?= Date: Fri, 18 Jul 2025 11:54:50 +0200 Subject: [PATCH] feat: add PatchOp validation checks --- scim2_models/rfc7644/message.py | 15 ++ scim2_models/rfc7644/patch_op.py | 109 ++++++-- scim2_models/utils.py | 27 ++ tests/test_patch_op.py | 439 ++++++++++++++++++++++++++++++- tests/test_path_validation.py | 22 ++ 5 files changed, 596 insertions(+), 16 deletions(-) diff --git a/scim2_models/rfc7644/message.py b/scim2_models/rfc7644/message.py index d32d854..febfba8 100644 --- a/scim2_models/rfc7644/message.py +++ b/scim2_models/rfc7644/message.py @@ -10,6 +10,8 @@ from pydantic import Tag from pydantic._internal._model_construction import ModelMetaclass +from scim2_models.rfc7643.resource import Resource + from ..base import BaseModel from ..scim_object import ScimObject from ..utils import UNION_TYPES @@ -108,3 +110,16 @@ def __new__( klass = super().__new__(cls, name, bases, attrs, **kwargs) return klass + + +def get_resource_class(obj) -> Optional[type[Resource]]: + """Extract the resource class from generic type parameter.""" + metadata = getattr(obj.__class__, "__pydantic_generic_metadata__", None) + if not metadata or not metadata.get("args"): + return None + + resource_class = metadata["args"][0] + if isinstance(resource_class, type) and issubclass(resource_class, Resource): + return resource_class + + return None diff --git a/scim2_models/rfc7644/patch_op.py b/scim2_models/rfc7644/patch_op.py index 1f3d581..a368c8c 100644 --- a/scim2_models/rfc7644/patch_op.py +++ b/scim2_models/rfc7644/patch_op.py @@ -1,6 +1,7 @@ from enum import Enum from typing import Annotated from typing import Any +from typing import Generic from typing import Optional from pydantic import Field @@ -8,9 +9,16 @@ from pydantic import model_validator from typing_extensions import Self +from ..annotations import Mutability from ..annotations import Required from ..attributes import ComplexAttribute +from ..base import BaseModel +from ..rfc7643.resource import AnyResource +from ..utils import extract_field_name +from ..utils import validate_scim_path_syntax +from .error import Error from .message import Message +from .message import get_resource_class class PatchOperation(ComplexAttribute): @@ -34,15 +42,67 @@ class Op(str, Enum): """The "path" attribute value is a String containing an attribute path describing the target of the operation.""" - @model_validator(mode="after") - def validate_path(self) -> Self: - # The "path" attribute value is a String containing an attribute path - # describing the target of the operation. The "path" attribute is - # OPTIONAL for "add" and "replace" and is REQUIRED for "remove" - # operations. See relevant operation sections below for details. + @field_validator("path") + @classmethod + def validate_path_syntax(cls, v: Optional[str]) -> Optional[str]: + """Validate path syntax according to RFC 7644 ABNF grammar (simplified).""" + if v is None: + return v + + # RFC 7644 Section 3.5.2: Path syntax validation according to ABNF grammar + if not validate_scim_path_syntax(v): + raise ValueError(Error.make_invalid_path_error().detail) + + return v + + def _validate_mutability( + self, resource_class: type[BaseModel], field_name: str + ) -> None: + """Validate mutability constraints.""" + # RFC 7644 Section 3.5.2: Servers should be tolerant of schema extensions + if field_name not in resource_class.model_fields: + return + + mutability = resource_class.get_field_annotation(field_name, Mutability) + # RFC 7643 Section 7: Attributes with mutability "readOnly" SHALL NOT be modified + if mutability == Mutability.read_only: + if self.op in (PatchOperation.Op.add, PatchOperation.Op.replace_): + raise ValueError(Error.make_mutability_error().detail) + + # RFC 7643 Section 7: Attributes with mutability "immutable" SHALL NOT be updated + elif mutability == Mutability.immutable: + if self.op == PatchOperation.Op.replace_: + raise ValueError(Error.make_mutability_error().detail) + + def _validate_required_attribute( + self, resource_class: type[BaseModel], field_name: str + ) -> None: + """Validate required attribute constraints for remove operations.""" + # RFC 7644 Section 3.5.2.3: Only validate for remove operations + if self.op != PatchOperation.Op.remove: + return + + # RFC 7644 Section 3.5.2: Servers should be tolerant of schema extensions + if field_name not in resource_class.model_fields: + return + + required = resource_class.get_field_annotation(field_name, Required) + + # RFC 7643 Section 7: Required attributes SHALL NOT be removed + if required == Required.true: + raise ValueError(Error.make_invalid_value_error().detail) + + @model_validator(mode="after") + def validate_operation_requirements(self) -> Self: + """Validate operation requirements according to RFC 7644.""" + # RFC 7644 Section 3.5.2.3: Path is required for remove operations if self.path is None and self.op == PatchOperation.Op.remove: - raise ValueError("Op.path is required for remove operations") + raise ValueError(Error.make_invalid_value_error().detail) + + # 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 ValueError(Error.make_invalid_value_error().detail) return self @@ -51,7 +111,7 @@ def validate_path(self) -> Self: @field_validator("op", mode="before") @classmethod def normalize_op(cls, v: Any) -> Any: - """Ignorecase for op. + """Ignore case for op. This brings `compatibility with Microsoft Entra `_: @@ -65,13 +125,8 @@ def normalize_op(cls, v: Any) -> Any: return v -class PatchOp(Message): - """Patch Operation as defined in :rfc:`RFC7644 §3.5.2 <7644#section-3.5.2>`. - - .. todo:: - - The models for Patch operations are defined, but their behavior is not implemented nor tested yet. - """ +class PatchOp(Message, Generic[AnyResource]): + """Patch Operation as defined in :rfc:`RFC7644 §3.5.2 <7644#section-3.5.2>`.""" schemas: Annotated[list[str], Required.true] = [ "urn:ietf:params:scim:api:messages:2.0:PatchOp" @@ -82,3 +137,27 @@ class PatchOp(Message): ) """The body of an HTTP PATCH request MUST contain the attribute "Operations", whose value is an array of one or more PATCH operations.""" + + @model_validator(mode="after") + def validate_operations(self) -> 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. + """ + 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: + continue + + field_name = extract_field_name(operation.path) + if field_name is None: + continue + + operation._validate_mutability(resource_class, field_name) + operation._validate_required_attribute(resource_class, field_name) + + return self diff --git a/scim2_models/utils.py b/scim2_models/utils.py index 567c6ce..982c8e0 100644 --- a/scim2_models/utils.py +++ b/scim2_models/utils.py @@ -156,3 +156,30 @@ def validate_scim_urn_syntax(path: str) -> bool: return False return True + + +def extract_field_name(path: str) -> Optional[str]: + """Extract the field name from a path. + + For now, only handle simple paths (no filters, no complex expressions). + Returns None for complex paths that require filter parsing. + + """ + # Handle URN paths + if path.startswith("urn:"): + # First validate it's a proper URN + if not validate_scim_urn_syntax(path): + return None + parts = path.rsplit(":", 1) + return parts[1] + + # Handle simple paths (no brackets, no filters) + if "[" in path or "]" in path: + return None # Complex filter path, not handled + + # Simple attribute path (may have dots for sub-attributes) + # For now, just take the first part before any dot + if "." in path: + return path.split(".")[0] + + return path diff --git a/tests/test_patch_op.py b/tests/test_patch_op.py index 6481777..43abf39 100644 --- a/tests/test_patch_op.py +++ b/tests/test_patch_op.py @@ -1,12 +1,34 @@ +"""Simplified tests for PatchOp and PatchOperation - keeping only essential tests.""" + +from typing import Annotated + import pytest from pydantic import ValidationError from scim2_models import PatchOp from scim2_models import PatchOperation +from scim2_models.annotations import Mutability +from scim2_models.annotations import Required +from scim2_models.rfc7643.resource import Resource +from scim2_models.rfc7644.message import get_resource_class + + +# Test resource class for basic syntax tests +class MockResource(Resource): + """Test resource class with various metadata annotations.""" + + mutable_field: str + read_only_field: Annotated[str, Mutability.read_only] + immutable_field: Annotated[str, Mutability.immutable] + required_field: Annotated[str, Required.true] + optional_field: Annotated[str, Required.false] = "default" def test_validate_patchop_case_insensitivith(): - """Validate that a patch operation's Op declaration is case-insensitive.""" + """Validate that a patch operation's Op declaration is case-insensitive. + + RFC 7644 Section 3.5.2: "The "op" parameter value is case insensitive." + """ assert PatchOp.model_validate( { "operations": [ @@ -38,6 +60,10 @@ def test_validate_patchop_case_insensitivith(): def test_path_required_for_remove_operations(): + """Test that path is required for remove operations. + + RFC 7644 Section 3.5.2.3: "If the "path" parameter is missing, the operation fails with HTTP status code 400." + """ PatchOp.model_validate( { "operations": [ @@ -61,3 +87,414 @@ def test_path_required_for_remove_operations(): ], } ) + + +def test_patch_operation_path_syntax_validation(): + """Test that invalid path syntax is rejected. + + RFC 7644 Section 3.5.2: Path syntax must follow ABNF rules defined in the specification. + """ + # Test invalid path starting with digit + with pytest.raises(ValidationError, match="path.*invalid"): + PatchOp.model_validate( + {"operations": [{"op": "replace", "path": "123invalid", "value": "test"}]} + ) + + # Test invalid path with double dots + with pytest.raises(ValidationError, match="path.*invalid"): + PatchOp.model_validate( + { + "operations": [ + {"op": "replace", "path": "invalid..path", "value": "test"} + ] + } + ) + + # Test invalid path with invalid characters + with pytest.raises(ValidationError, match="path.*invalid"): + PatchOp.model_validate( + {"operations": [{"op": "replace", "path": "invalid@path", "value": "test"}]} + ) + + # Test invalid URN path + with pytest.raises(ValidationError, match="path.*invalid"): + PatchOp.model_validate( + {"operations": [{"op": "replace", "path": "urn:invalid", "value": "test"}]} + ) + + # Test valid paths should work + valid_paths = [ + "userName", + "name.familyName", + "emails.value", + "urn:ietf:params:scim:schemas:core:2.0:User:userName", + "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber", + ] + + for path in valid_paths: + # Should not raise exception + patch_op = PatchOp.model_validate( + {"operations": [{"op": "replace", "path": path, "value": "test"}]} + ) + assert patch_op.operations[0].path == path + + +def test_patch_operation_value_required_for_add(): + """Test that value is required for add operations. + + RFC 7644 Section 3.5.2.1: "The "value" parameter contains a set of attributes to be added to the resource." + """ + # Test add without value should fail + with pytest.raises(ValidationError, match="required value.*missing"): + PatchOp.model_validate({"operations": [{"op": "add", "path": "userName"}]}) + + # Test add with null value should fail + with pytest.raises(ValidationError, match="required value.*missing"): + PatchOp.model_validate( + {"operations": [{"op": "add", "path": "userName", "value": None}]} + ) + + # Test add with value should work + patch_op = PatchOp.model_validate( + {"operations": [{"op": "add", "path": "userName", "value": "test"}]} + ) + assert patch_op.operations[0].value == "test" + + # Test replace without value should work (optional) + patch_op = PatchOp.model_validate( + {"operations": [{"op": "replace", "path": "userName"}]} + ) + assert patch_op.operations[0].value is None + + # Test remove without value should work (not applicable) + patch_op = PatchOp.model_validate( + {"operations": [{"op": "remove", "path": "userName"}]} + ) + assert patch_op.operations[0].value is None + + +def test_patch_operation_urn_path_validation(): + """Test URN path validation edge cases. + + RFC 7644 Section 3.5.2: URN paths must follow the format "urn:ietf:params:scim:schemas:..." for schema extensions. + """ + # Test valid URN paths + valid_urn_paths = [ + "urn:ietf:params:scim:schemas:core:2.0:User:userName", + "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber", + "urn:custom:namespace:schema:1.0:Resource:attribute", + ] + + for urn_path in valid_urn_paths: + patch_op = PatchOp.model_validate( + {"operations": [{"op": "replace", "path": urn_path, "value": "test"}]} + ) + assert patch_op.operations[0].path == urn_path + + # Test invalid URN paths + invalid_urn_paths = [ + "urn:too:short", # Not enough segments + "urn:ietf:params:scim:schemas:core:2.0:User:", # Empty attribute + "urn:ietf:params:scim:schemas:core:2.0:User:123invalid", # Attribute starts with digit + "noturn:ietf:params:scim:schemas:core:2.0:User:userName", # Doesn't start with urn: + ] + + for urn_path in invalid_urn_paths: + with pytest.raises(ValidationError, match="path.*invalid"): + PatchOp.model_validate( + {"operations": [{"op": "replace", "path": urn_path, "value": "test"}]} + ) + + +def test_patch_operation_path_edge_cases(): + """Test edge cases for path validation.""" + # Test None path (should be allowed for non-remove operations) + patch_op = PatchOp.model_validate( + {"operations": [{"op": "replace", "value": "test"}]} + ) + assert patch_op.operations[0].path is None + + # Test empty path (should be invalid) + with pytest.raises(ValidationError, match="path.*invalid"): + PatchOp.model_validate( + {"operations": [{"op": "replace", "path": "", "value": "test"}]} + ) + + # Test whitespace-only path (should be invalid) + with pytest.raises(ValidationError, match="path.*invalid"): + PatchOp.model_validate( + {"operations": [{"op": "replace", "path": " ", "value": "test"}]} + ) + + # Test URN with not enough parts (should be invalid) + with pytest.raises(ValidationError, match="path.*invalid"): + PatchOp.model_validate( + { + "operations": [ + {"op": "replace", "path": "urn:invalid:path", "value": "test"} + ] + } + ) + + # Test URN with no colon separation (should be invalid) + with pytest.raises(ValidationError, match="path.*invalid"): + PatchOp.model_validate( + {"operations": [{"op": "replace", "path": "urn:invalid", "value": "test"}]} + ) + + +def test_patch_operation_path_validator_directly(): + """Test path validator directly to ensure full coverage.""" + # Test that None path is handled correctly + result = PatchOperation.validate_path_syntax(None) + assert result is None + + # Test empty string handling + with pytest.raises(ValueError, match="path.*invalid"): + PatchOperation.validate_path_syntax("") + + # Test URN edge cases for full coverage + # This tests the case where a path starts with "urn" but has invalid format + with pytest.raises(ValueError, match="path.*invalid"): + PatchOperation.validate_path_syntax("urn:invalid") + + +# Test resource classes with different metadata +class MockUser(Resource): + """Test user resource with metadata annotations.""" + + user_name: str + password: Annotated[str, Mutability.write_only] + read_only_field: Annotated[str, Mutability.read_only] + immutable_field: Annotated[str, Mutability.immutable] + required_field: Annotated[str, Required.true] + optional_field: Annotated[str, Required.false] = "default" + + +class MockGroup(Resource): + """Test group resource with metadata annotations.""" + + display_name: Annotated[str, Required.true] + members: Annotated[list, Mutability.read_only] = [] + + +def test_patch_op_automatic_validation_with_user_type(): + """Test that PatchOp[User] automatically validates against User metadata. + + RFC 7643 Section 7: Attributes with mutability "readOnly" or "immutable" must be protected from modification. + """ + # Test read-only field violation + with pytest.raises(ValidationError, match="attempted modification.*mutability"): + PatchOp[MockUser].model_validate( + {"operations": [{"op": "add", "path": "read_only_field", "value": "test"}]} + ) + + # Test immutable field violation + with pytest.raises(ValidationError, match="attempted modification.*mutability"): + PatchOp[MockUser].model_validate( + { + "operations": [ + {"op": "replace", "path": "immutable_field", "value": "test"} + ] + } + ) + + # Test required field removal + with pytest.raises(ValidationError, match="required value.*missing"): + PatchOp[MockUser].model_validate( + {"operations": [{"op": "remove", "path": "required_field"}]} + ) + + +def test_patch_op_automatic_validation_allows_valid_operations(): + """Test that valid operations pass automatic validation.""" + # Valid operations should work + patch_op = PatchOp[MockUser].model_validate( + { + "operations": [ + {"op": "add", "path": "user_name", "value": "john.doe"}, + {"op": "replace", "path": "optional_field", "value": "new_value"}, + {"op": "remove", "path": "optional_field"}, + { + "op": "add", + "path": "immutable_field", + "value": "initial_value", + }, # ADD is allowed for immutable + ] + } + ) + + assert len(patch_op.operations) == 4 + + +def test_patch_op_automatic_validation_with_group_type(): + """Test that PatchOp[Group] validates against Group metadata. + + RFC 7643 Section 7: Attributes with mutability "readOnly" must be protected from modification. + """ + # Test read-only members field + with pytest.raises(ValidationError, match="attempted modification.*mutability"): + PatchOp[MockGroup].model_validate( + { + "operations": [ + {"op": "add", "path": "members", "value": [{"value": "user123"}]} + ] + } + ) + + # Valid operation should work + patch_op = PatchOp[MockGroup].model_validate( + { + "operations": [ + {"op": "replace", "path": "display_name", "value": "New Group Name"} + ] + } + ) + + assert len(patch_op.operations) == 1 + + +def test_patch_op_without_type_parameter_no_validation(): + """Test that PatchOp without type parameter doesn't do metadata validation.""" + # This should work even though it would violate User metadata + # because no type parameter means no automatic validation + patch_op = PatchOp.model_validate( + {"operations": [{"op": "add", "path": "read_only_field", "value": "test"}]} + ) + + assert len(patch_op.operations) == 1 + + +def test_patch_op_complex_paths_ignored(): + """Test that complex paths are ignored in automatic validation. + + RFC 7644 Section 3.5.2: Complex path expressions with filters are allowed but not validated at schema level. + """ + # Complex paths should be ignored (no validation error) + patch_op = PatchOp[MockUser].model_validate( + { + "operations": [ + { + "op": "replace", + "path": 'emails[type eq "work"].value', + "value": "test", + }, + {"op": "add", "path": 'groups[display eq "Admin"]', "value": "test"}, + ] + } + ) + + assert len(patch_op.operations) == 2 + + +def test_patch_op_nonexistent_fields_allowed(): + """Test that operations on non-existent fields are allowed. + + RFC 7644 Section 3.5.2: Servers should be tolerant of schema extensions and unrecognized attributes. + """ + # Operations on fields that don't exist should be allowed + patch_op = PatchOp[MockUser].model_validate( + { + "operations": [ + {"op": "add", "path": "nonexistent_field", "value": "test"}, + {"op": "remove", "path": "another_nonexistent_field"}, + ] + } + ) + + assert len(patch_op.operations) == 2 + + +def test_patch_op_field_without_annotations(): + """Test that operations on fields without mutability/required annotations are allowed.""" + + class MockUserWithoutAnnotations(Resource): + user_name: str # No mutability annotation + description: str = "default" # No required annotation + + # Operations on fields without annotations should be allowed + patch_op = PatchOp[MockUserWithoutAnnotations].model_validate( + { + "operations": [ + {"op": "add", "path": "user_name", "value": "test"}, + {"op": "replace", "path": "user_name", "value": "test2"}, + {"op": "remove", "path": "description"}, + ] + } + ) + + assert len(patch_op.operations) == 3 + + +def test_patch_op_urn_paths_validated(): + """Test that URN paths are validated with automatic validation.""" + # URN path for read-only field should fail + with pytest.raises(ValidationError, match="attempted modification.*mutability"): + PatchOp[MockUser].model_validate( + { + "operations": [ + { + "op": "replace", + "path": "urn:ietf:params:scim:schemas:core:2.0:User:read_only_field", + "value": "test", + } + ] + } + ) + + +def test_patch_op_read_only_field_remove_operation(): + """Test that remove operations on read-only fields are allowed. + + RFC 7643 Section 7: Read-only fields can be removed but not added/replaced. + """ + # Remove operation on read-only field should work + patch_op = PatchOp[MockUser].model_validate( + {"operations": [{"op": "remove", "path": "read_only_field"}]} + ) + assert len(patch_op.operations) == 1 + + +def test_patch_op_none_path_validation(): + """Test validation when path is None.""" + # Test with None path - should not validate field metadata + patch_op = PatchOp[MockUser].model_validate( + {"operations": [{"op": "replace", "value": {"read_only_field": "test"}}]} + ) + assert len(patch_op.operations) == 1 + assert patch_op.operations[0].path is None + + +def test_patch_op_get_resource_class_method(): + """Test the get_resource_class method directly.""" + # With type parameter + typed_patch = PatchOp[MockUser].model_validate( + {"operations": [{"op": "add", "path": "user_name", "value": "test"}]} + ) + resource_class = get_resource_class(typed_patch) + assert resource_class == MockUser + + # With a wrong type parameter + untyped_patch = PatchOp[int].model_validate( + {"operations": [{"op": "add", "path": "user_name", "value": "test"}]} + ) + resource_class = get_resource_class(untyped_patch) + assert resource_class is None + + # Without type parameter + untyped_patch = PatchOp.model_validate( + {"operations": [{"op": "add", "path": "user_name", "value": "test"}]} + ) + resource_class = get_resource_class(untyped_patch) + assert resource_class is None + + +def test_patch_op_not_type_parameter(): + """Test that existing code without type parameters still works.""" + # Old way of creating PatchOp should still work + patch_op = PatchOp( + operations=[{"op": "add", "path": "any_field", "value": "any_value"}] + ) + + assert len(patch_op.operations) == 1 + assert get_resource_class(patch_op) is None diff --git a/tests/test_path_validation.py b/tests/test_path_validation.py index 4f1f771..c40a70c 100644 --- a/tests/test_path_validation.py +++ b/tests/test_path_validation.py @@ -1,5 +1,6 @@ """Tests for SCIM path validation utilities.""" +from scim2_models.utils import extract_field_name from scim2_models.utils import validate_scim_path_syntax from scim2_models.utils import validate_scim_urn_syntax @@ -91,3 +92,24 @@ def test_validate_scim_urn_syntax_edge_cases(): assert not validate_scim_urn_syntax("urn:a:b:c:") # Empty attribute assert not validate_scim_urn_syntax("urn:a:b:") # Missing resource assert not validate_scim_urn_syntax("urn:") # Just urn: + + +def test_path_extraction(): + """Test path extraction logic.""" + # Test simple path + assert extract_field_name("simple_field") == "simple_field" + + # Test dotted path (should return first part) + assert extract_field_name("name.familyName") == "name" + + # Test URN path + assert ( + extract_field_name("urn:ietf:params:scim:schemas:core:2.0:User:userName") + == "userName" + ) + + # Test complex path with filter (should return None) + assert extract_field_name('emails[type eq "work"]') is None + + # Test invalid URN path + assert extract_field_name("urn:invalid") is None