Merge pull request #7716 from freedomofpress/7682-stricter-typing

feat(`api2`): validate and freeze dataclasses

Author: legoktm

securedrop/journalist_app/api2/types.py

@@ -2,17 +2,22 @@
 from enum import IntEnum, StrEnum, auto
 from typing import (
     Any,
+    Callable,
+    Iterable,
     List,
     Mapping,
     NewType,
     Optional,
     Set,
     Tuple,
 )
+from uuid import UUID
 
 Record = NewType("Record", dict[str, Any])
 Version = NewType("Version", str)
 
+VERSION_LEN = 64  # hex digits
+
 
 # NB.  Ideally we'd have a generic UUID[T], but the semantics don't change
 # before mypy 1.12, which is incompatible with our use elsewhere of sqlmypy.
@@ -63,7 +68,7 @@ class Index:
     journalists: dict[JournalistUUID, Version] = field(default_factory=dict)
 
 
-@dataclass
+@dataclass(frozen=True)
 class Target:
     """Base class for `<Resource>Target` dataclasses, to make their union usable
     at runtime.  Subclass at least with:
@@ -74,63 +79,133 @@ class Target:
 
     version: Version
 
+    def __post_init__(self) -> None:
+        version = str(self.version)
 
-@dataclass
+        if len(version) != VERSION_LEN:
+            raise ValueError(f"version must have {VERSION_LEN} hex digits")
+
+        try:
+            int(version, 16)
+        except ValueError:
+            raise ValueError("version must be hex-encoded")
+
+
+@dataclass(frozen=True)
 class SourceTarget(Target):
     source_uuid: SourceUUID
 
+    def __post_init__(self) -> None:
+        super().__post_init__()
+        if not self.source_uuid:
+            raise ValueError("source_uuid must be non-empty")
 
-@dataclass
+        try:
+            UUID(str(self.source_uuid))
+        except ValueError:
+            raise ValueError(f"invalid source UUID: {self.source_uuid}")
+
+
+@dataclass(frozen=True)
 class ItemTarget(Target):
     item_uuid: ItemUUID
 
+    def __post_init__(self) -> None:
+        super().__post_init__()
+        if not self.item_uuid:
+            raise ValueError("item_uuid must be non-empty")
 
-@dataclass
+        try:
+            UUID(str(self.item_uuid))
+        except ValueError:
+            raise ValueError(f"invalid item UUID: {self.item_uuid}")
+
+
+@dataclass(frozen=True)
 class EventData:
     """
     Base class for `<EventType>Data dataclasses, to make their union usable at runtime.
     For non-empty events, subclass and add to `EVENT_DATA_TYPES`.
     """
 
 
-@dataclass
+@dataclass(frozen=True)
 class ReplySentData(EventData):
     uuid: ReplyUUID
     reply: str
 
+    def __post_init__(self) -> None:
+        try:
+            UUID(str(self.uuid))
+        except ValueError:
+            raise ValueError(f"invalid reply UUID: {self.uuid}")
+
+        if not self.reply:
+            raise ValueError("reply must be a non-empty string")
+
 
 EVENT_DATA_TYPES = {EventType.REPLY_SENT: ReplySentData}
 
 
-@dataclass
+@dataclass(frozen=True)
 class Event:
     id: EventID
-    target: Target | Mapping
+    target: Target | Mapping[str, Any]
     type: EventType
-    data: Optional[EventData | Mapping] = None
+    data: Optional[EventData | Mapping[str, Any]] = None
 
     def __post_init__(self) -> None:
-        if not isinstance(self.type, EventType):
-            self.type = EventType(self.type)  # strict enum
+        # ID must be usable as an int (for snowflake ordering; see section
+        # "Snowflake IDs" in `API2.md`):
+        if not str(self.id).isdigit():
+            raise ValueError(f"event ID must be an integer string: {self.id}")
 
-        if not isinstance(self.target, Target):
-            if "source_uuid" in self.target:
-                self.target = SourceTarget(**self.target)
-            elif "item_uuid" in self.target:
-                self.target = ItemTarget(**self.target)
+        # Normalize type:
+        if not isinstance(self.type, EventType):
+            object.__setattr__(self, "type", EventType(self.type))
+
+        # Normalize target:
+        target = self.target
+        if not isinstance(target, Target):
+            if not isinstance(target, Mapping):
+                raise TypeError(f"invalid event target: {target!r}")
+
+            if "source_uuid" in target:
+                target = SourceTarget(**target)
+            elif "item_uuid" in target:
+                target = ItemTarget(**target)
             else:
-                raise TypeError(f"invalid event target: {self.target}")
+                raise TypeError(f"invalid event target: {target}")
+
+            object.__setattr__(self, "target", target)
 
-        if not isinstance(self.data, EventData) and self.data and self.type in EVENT_DATA_TYPES:
+        # Normalize data:
+        data = self.data
+        if data is None:
+            return
+
+        # If it's already a `EventData` dataclass, validate it:
+        if isinstance(data, EventData):
+            expected = EVENT_DATA_TYPES.get(self.type)
+            if expected is not None and not isinstance(data, expected):
+                raise TypeError(f"invalid event data for type {self.type}")
+            return
+
+        # If it's a mapping for an event type that expects data, instantiate an
+        # `EventType` dataclass:
+        if isinstance(data, Mapping) and self.type in EVENT_DATA_TYPES:
             try:
-                self.data = EVENT_DATA_TYPES[self.type](**self.data)
+                data_obj = EVENT_DATA_TYPES[self.type](**data)
             except TypeError:
                 raise TypeError(f"invalid event data for type {self.type}")
+            object.__setattr__(self, "data", data_obj)
+
+        # Otherwise, discard it.
         else:
-            self.data = None
+            object.__setattr__(self, "data", None)
 
 
-@dataclass
+@dataclass(frozen=True)
 class EventResult:
     event_id: EventID
     status: EventStatus
@@ -140,7 +215,7 @@ class EventResult:
     items: dict[ItemUUID, Optional[Record]] = field(default_factory=dict)
 
 
-@dataclass
+@dataclass(frozen=True)
 class BatchRequest:
     # Source metadata:
     sources: Set[SourceUUID] = field(default_factory=set)
@@ -150,7 +225,26 @@ class BatchRequest:
     journalists: Set[JournalistUUID] = field(default_factory=set)
 
     # Events submitted by the client:
-    events: List[Event] = field(default_factory=list)
+    events: List[Event | Mapping[str, Any]] = field(default_factory=list)
+
+    def __post_init__(self) -> None:
+        def _normalize_uuids(raw: Iterable[Any], wrap: Callable) -> set:
+            try:
+                return {wrap(x) for x in raw}
+            except TypeError:
+                raise TypeError("expected an iterable")
+
+        object.__setattr__(self, "sources", _normalize_uuids(self.sources, SourceUUID))
+        object.__setattr__(self, "items", _normalize_uuids(self.items, ItemUUID))
+        object.__setattr__(self, "journalists", _normalize_uuids(self.journalists, JournalistUUID))
+
+        normalized_events: list[Event | Mapping[str, Any]] = []
+        for e in self.events:
+            if isinstance(e, (Event, Mapping)):
+                normalized_events.append(e)
+            else:
+                raise TypeError("BatchRequest.events must contain Event or Mapping instances")
+        object.__setattr__(self, "events", normalized_events)
 
 
 @dataclass

securedrop/tests/test_journalist_api2.py

@@ -12,7 +12,7 @@
 from flask_sqlalchemy import get_debug_queries
 from journalist_app import api2, create_app
 from journalist_app.api2.shared import json_version
-from journalist_app.api2.types import Event, EventType, ItemTarget, SourceTarget
+from journalist_app.api2.types import VERSION_LEN, Event, EventType, ItemTarget, SourceTarget
 from models import Reply, Source, SourceStar, Submission, db
 from sqlalchemy.orm.exc import MultipleResultsFound
 from tests.factories import SecureDropConfigFactory
@@ -550,15 +550,18 @@ def test_api2_item_deleted(
         assert Reply.query.filter(Reply.uuid == event.target.item_uuid).one_or_none() is None
 
         # Try to delete something that doesn't exist:
-        event.id = "345678"
-        event.target.item_uuid = "does not exist"
+        nonexistent_event = Event(
+            id="345678",
+            target=ItemTarget(item_uuid=str(uuid.uuid4()), version=reply_version),
+            type=EventType.ITEM_DELETED,
+        )
         response = app.post(
             url_for("api2.data"),
-            json={"events": [asdict(event)]},
+            json={"events": [asdict(nonexistent_event)]},
             headers=get_api_headers(journalist_api_token),
         )
-        assert response.json["events"][event.id] == [410, None]
-        assert event.target.item_uuid not in response.json["items"]
+        assert response.json["events"][nonexistent_event.id] == [410, None]
+        assert nonexistent_event.target.item_uuid not in response.json["items"]
 
 
 def test_api2_source_deleted(
@@ -574,7 +577,7 @@ def test_api2_source_deleted(
         # Try deleting the source with the wrong version
         event = Event(
             id="394758",
-            target=SourceTarget(source_uuid=source_uuid, version="wrong-version"),
+            target=SourceTarget(source_uuid=source_uuid, version="a" * VERSION_LEN),
             type=EventType.SOURCE_DELETED,
         )
         response = app.post(
@@ -624,14 +627,17 @@ def test_api2_source_deleted(
         assert Source.query.filter(Source.uuid == source_uuid).one_or_none() is None
 
         # Try to delete a source that doesn't exist
-        event.id = "234567"
-        event.target.source_uuid = "does-not-exist"
+        nonexistent_event = Event(
+            id="234567",
+            target=SourceTarget(source_uuid=str(uuid.uuid4()), version=source_version),
+            type=EventType.SOURCE_DELETED,
+        )
         response = app.post(
             url_for("api2.data"),
-            json={"events": [asdict(event)]},
+            json={"events": [asdict(nonexistent_event)]},
             headers=get_api_headers(journalist_api_token),
         )
-        assert response.json["events"][event.id] == [410, None]
+        assert response.json["events"][nonexistent_event.id] == [410, None]
         assert "does-not-exist" not in response.json["sources"]
 
 
@@ -653,7 +659,7 @@ def test_api2_source_conversation_deleted(
         # (intentionally not fetching the correct version)
         event = Event(
             id="498567",
-            target=SourceTarget(source_uuid=source_uuid, version="wrong-version"),
+            target=SourceTarget(source_uuid=source_uuid, version="a" * VERSION_LEN),
             type=EventType.SOURCE_CONVERSATION_DELETED,
         )
         response = app.post(
@@ -842,16 +848,19 @@ def test_api2_item_seen(
         updated_submission = Submission.query.filter(Submission.uuid == submission_uuid).one()
         assert updated_submission.downloaded is True
 
-        # Try with invalid item UUID
-        event.id = "234567"
-        event.target.item_uuid = "invalid-uuid"
+        # Try to mark seen an item that doesn't exist
+        no_such_item_event = Event(
+            id="234567",
+            target=ItemTarget(item_uuid=str(uuid.uuid4()), version=item_version),
+            type=EventType.ITEM_SEEN,
+        )
         response = app.post(
             url_for("api2.data"),
-            json={"events": [asdict(event)]},
+            json={"events": [asdict(no_such_item_event)]},
             headers=get_api_headers(journalist_api_token),
         )
-        assert response.json["events"][event.id][0] == 404
-        assert "could not find item" in response.json["events"][event.id][1]
+        assert response.json["events"][no_such_item_event.id][0] == 404
+        assert "could not find item" in response.json["events"][no_such_item_event.id][1]
 
 
 def test_api2_idempotence_period(journalist_app):
@@ -924,7 +933,7 @@ def test_api2_source_conversation_deleted_resubmission(
         # 1. Submit with the wrong version --> Conflict (409).
         event = Event(
             id="600100",
-            target=SourceTarget(source_uuid=source_uuid, version="wrong-version"),
+            target=SourceTarget(source_uuid=source_uuid, version="a" * VERSION_LEN),
             type=EventType.SOURCE_CONVERSATION_DELETED,
         )
         res1 = app.post(
@@ -955,14 +964,18 @@ def test_api2_source_conversation_deleted_resubmission(
         assert index.status_code == 200
         correct_version = index.json["sources"][source_uuid]
 
-        event.target = SourceTarget(source_uuid=source_uuid, version=correct_version)
+        corrected_event = Event(
+            id=event.id,
+            target=SourceTarget(source_uuid=source_uuid, version=correct_version),
+            type=event.type,
+        )
         res2 = app.post(
             url_for("api2.data"),
-            json={"events": [asdict(event)]},
+            json={"events": [asdict(corrected_event)]},
             headers=get_api_headers(journalist_api_token),
         )
         assert res2.status_code == 200
-        assert res2.json["events"][event.id] == [200, None]
+        assert res2.json["events"][corrected_event.id] == [200, None]
 
         # Confirm that items are returned as deleted.
         assert res2.json["sources"][source_uuid] is not None
@@ -980,11 +993,11 @@ def test_api2_source_conversation_deleted_resubmission(
         # 3. Resubmit the same event again --> Already Reported (208).
         res3 = app.post(
             url_for("api2.data"),
-            json={"events": [asdict(event)]},
+            json={"events": [asdict(corrected_event)]},
             headers=get_api_headers(journalist_api_token),
         )
         assert res3.status_code == 200
-        assert res3.json["events"][event.id][0] == 208
+        assert res3.json["events"][corrected_event.id][0] == 208
 
 
 def test_api2_reply_sent_then_requested_item_is_deduped(