From 1881b7f3d637bd2232ddbd1a390967273079c6c6 Mon Sep 17 00:00:00 2001
From: Pim Kunis <pim@kunis.nl>
Date: Thu, 19 Aug 2021 23:42:24 +0200
Subject: [PATCH] Add more type checking

---
 lib/matrix_server/check.ex                    | 10 +++
 lib/matrix_server/encodable_map.ex            |  6 ++
 lib/matrix_server/schema/event.ex             | 78 ++++++++++++++----
 lib/matrix_server/state_resolution.ex         |  4 +-
 .../state_resolution/authorization.ex         | 81 ++++++++++++-------
 lib/matrix_server/types/alias_id.ex           |  6 +-
 lib/matrix_server/types/event_id.ex           |  6 +-
 lib/matrix_server/types/group_id.ex           |  6 +-
 lib/matrix_server/types/room_id.ex            |  6 +-
 lib/matrix_server/types/user_id.ex            | 11 ++-
 10 files changed, 155 insertions(+), 59 deletions(-)
 create mode 100644 lib/matrix_server/check.ex

diff --git a/lib/matrix_server/check.ex b/lib/matrix_server/check.ex
new file mode 100644
index 0000000..b62d5a2
--- /dev/null
+++ b/lib/matrix_server/check.ex
@@ -0,0 +1,10 @@
+defmodule MatrixServer.Check do
+  import Ecto.Query
+  alias MatrixServer.{Repo, Account, Room}
+  alias MatrixServerWeb.Client.Request.CreateRoom
+
+  def create_room do
+    account = Repo.one!(from a in Account, limit: 1)
+    Room.create(account, %CreateRoom{})
+  end
+end
diff --git a/lib/matrix_server/encodable_map.ex b/lib/matrix_server/encodable_map.ex
index f3200ae..3d98b29 100644
--- a/lib/matrix_server/encodable_map.ex
+++ b/lib/matrix_server/encodable_map.ex
@@ -1,6 +1,7 @@
 # https://github.com/michalmuskala/jason/issues/69
 defmodule MatrixServer.EncodableMap do
   alias MatrixServer.EncodableMap
+  alias MatrixServer.Types.{UserId, RoomId, EventId, GroupId, AliasId}
 
   defstruct pairs: []
 
@@ -17,6 +18,11 @@ defmodule MatrixServer.EncodableMap do
         {k, v} when is_struct(v, DateTime) ->
           {k, DateTime.to_unix(v, :millisecond)}
 
+        {k, v}
+        when is_struct(v, UserId) or is_struct(v, RoomId) or is_struct(v, EventId) or
+               is_struct(v, GroupId) or is_struct(v, AliasId) ->
+          {k, to_string(v)}
+
         {k, v} when is_map(v) ->
           {k, from_map(v)}
 
diff --git a/lib/matrix_server/schema/event.ex b/lib/matrix_server/schema/event.ex
index c56a6a6..edd35e7 100644
--- a/lib/matrix_server/schema/event.ex
+++ b/lib/matrix_server/schema/event.ex
@@ -4,13 +4,28 @@ defmodule MatrixServer.Event do
   import Ecto.Query
 
   alias MatrixServer.{Repo, Room, Event, Account, EncodableMap, KeyServer}
+  alias MatrixServer.Types.UserId
+
+  # TODO: Could refactor to also always set prev_events, but not necessary.
+  @type t :: %__MODULE__{
+          type: String.t(),
+          origin_server_ts: DateTime.t(),
+          state_key: String.t(),
+          sender: UserId.t(),
+          content: map(),
+          prev_events: [String.t()] | nil,
+          auth_events: [String.t()],
+          unsigned: map() | nil,
+          signatures: map() | nil,
+          hashes: map() | nil
+        }
 
   @primary_key {:event_id, :string, []}
   schema "events" do
     field :type, :string
     field :origin_server_ts, :utc_datetime_usec
     field :state_key, :string
-    field :sender, :string
+    field :sender, UserId
     field :content, :map
     field :prev_events, {:array, :string}
     field :auth_events, {:array, :string}
@@ -21,16 +36,18 @@ defmodule MatrixServer.Event do
     belongs_to :room, Room, type: :string
   end
 
+  @spec new(Room.t(), Account.t()) :: %Event{}
   def new(%Room{id: room_id}, %Account{localpart: localpart}) do
     %Event{
       room_id: room_id,
-      sender: MatrixServer.get_mxid(localpart),
+      sender: %UserId{localpart: localpart, domain: MatrixServer.server_name()},
       origin_server_ts: DateTime.utc_now(),
       prev_events: [],
       auth_events: []
     }
   end
 
+  @spec create_room(Room.t(), Account.t(), String.t()) :: t()
   def create_room(
         room,
         %Account{localpart: localpart} = creator,
@@ -49,6 +66,7 @@ defmodule MatrixServer.Event do
     }
   end
 
+  @spec join(Room.t(), Account.t(), [t()]) :: t()
   def join(room, %Account{localpart: localpart} = sender, auth_events) do
     mxid = MatrixServer.get_mxid(localpart)
 
@@ -63,6 +81,7 @@ defmodule MatrixServer.Event do
     }
   end
 
+  @spec power_levels(Room.t(), Account.t(), [t()]) :: t()
   def power_levels(
         room,
         %Account{localpart: localpart} = sender,
@@ -94,6 +113,7 @@ defmodule MatrixServer.Event do
     }
   end
 
+  @spec name(Room.t(), Account.t(), String.t(), [t()]) :: %Event{}
   def name(room, sender, name, auth_events) do
     %Event{
       new(room, sender)
@@ -106,6 +126,7 @@ defmodule MatrixServer.Event do
     }
   end
 
+  @spec topic(Room.t(), Account.t(), String.t(), [t()]) :: t()
   def topic(room, sender, topic, auth_events) do
     %Event{
       new(room, sender)
@@ -118,6 +139,7 @@ defmodule MatrixServer.Event do
     }
   end
 
+  @spec join_rules(Room.t(), Account.t(), String.t(), [t()]) :: t()
   def join_rules(room, sender, join_rule, auth_events) do
     %Event{
       new(room, sender)
@@ -130,6 +152,7 @@ defmodule MatrixServer.Event do
     }
   end
 
+  @spec history_visibility(Room.t(), Account.t(), String.t(), [t()]) :: t()
   def history_visibility(room, sender, history_visibility, auth_events) do
     %Event{
       new(room, sender)
@@ -142,6 +165,7 @@ defmodule MatrixServer.Event do
     }
   end
 
+  @spec guest_access(Room.t(), Account.t(), String.t(), [t()]) :: t()
   def guest_access(room, sender, guest_access, auth_events) do
     %Event{
       new(room, sender)
@@ -154,6 +178,7 @@ defmodule MatrixServer.Event do
     }
   end
 
+  @spec is_control_event(t()) :: boolean()
   def is_control_event(%Event{type: "m.room.power_levels", state_key: ""}), do: true
   def is_control_event(%Event{type: "m.room.join_rules", state_key: ""}), do: true
 
@@ -162,12 +187,13 @@ defmodule MatrixServer.Event do
         state_key: state_key,
         sender: sender,
         content: %{membership: membership}
-      })
-      when sender != state_key and membership in ["leave", "ban"],
-      do: true
+      }) do
+    to_string(sender) != state_key and membership in ["leave", "ban"]
+  end
 
   def is_control_event(_), do: false
 
+  @spec is_state_event(t()) :: boolean()
   def is_state_event(%Event{state_key: state_key}), do: state_key != nil
 
   # Perform validations that can be done before state resolution.
@@ -175,12 +201,13 @@ defmodule MatrixServer.Event do
   # We assume that required keys, as well as in the content, is already validated.
 
   # Rule 1.4 is left to changeset validation.
+  @spec prevalidate(t()) :: boolean()
   def prevalidate(%Event{
         type: "m.room.create",
         prev_events: prev_events,
         auth_events: auth_events,
         room_id: room_id,
-        sender: sender
+        sender: %UserId{domain: domain}
       }) do
     # TODO: error check on domains?
     # TODO: rule 1.3
@@ -188,7 +215,7 @@ defmodule MatrixServer.Event do
     # Check rules: 1.1, 1.2
     prev_events == [] and
       auth_events == [] and
-      MatrixServer.get_domain(sender) == MatrixServer.get_domain(room_id)
+      domain == MatrixServer.get_domain(room_id)
   end
 
   def prevalidate(%Event{auth_events: auth_event_ids, prev_events: prev_event_ids} = event) do
@@ -214,18 +241,27 @@ defmodule MatrixServer.Event do
   end
 
   # Rule 4.1 is left to changeset validation.
-  defp do_prevalidate(%Event{type: "m.room.aliases", sender: sender, state_key: state_key}, _, _) do
+  @spec do_prevalidate(t(), [t()], [t()]) :: boolean()
+  defp do_prevalidate(
+         %Event{type: "m.room.aliases", sender: %UserId{domain: domain}, state_key: state_key},
+         _,
+         _
+       ) do
     # Check rule: 4.2
-    MatrixServer.get_domain(sender) == MatrixServer.get_domain(state_key)
+    domain == MatrixServer.get_domain(state_key)
   end
 
   # Rule 5.1 is left to changeset validation.
   # Rules 5.2.3, 5.2.4, 5.2.5 is left to state resolution.
   # Check rule: 5.2.1
   defp do_prevalidate(
-         %Event{type: "m.room.member", content: %{"membership" => "join"}, sender: sender},
+         %Event{
+           type: "m.room.member",
+           content: %{"membership" => "join"},
+           sender: %UserId{localpart: localpart, domain: domain}
+         },
          _,
-         [%Event{type: "m.room.create", state_key: sender}]
+         [%Event{type: "m.room.create", state_key: %UserId{localpart: localpart, domain: domain}}]
        ),
        do: true
 
@@ -239,17 +275,20 @@ defmodule MatrixServer.Event do
          },
          _,
          _
-       )
-       when sender != state_key,
-       do: false
+       ) do
+    to_string(sender) == state_key
+  end
 
   # All other rules will be checked during state resolution.
   defp do_prevalidate(_, _, _), do: true
 
+  @spec valid_auth_events?(t(), [t()]) :: boolean()
   defp valid_auth_events?(
          %Event{type: type, sender: sender, state_key: state_key, content: content},
          auth_events
        ) do
+    sender = to_string(sender)
+
     Enum.all?(auth_events, fn
       %Event{type: "m.room.create", state_key: ""} ->
         true
@@ -275,6 +314,7 @@ defmodule MatrixServer.Event do
     end)
   end
 
+  @spec calculate_content_hash(t()) :: {:ok, binary()} | {:error, Jason.EncodeError.t()}
   defp calculate_content_hash(event) do
     m =
       event
@@ -287,6 +327,7 @@ defmodule MatrixServer.Event do
     end
   end
 
+  @spec redact(t()) :: map()
   defp redact(%Event{type: type, content: content} = event) do
     redacted_event =
       event
@@ -312,6 +353,7 @@ defmodule MatrixServer.Event do
     %{redacted_event | content: redact_content(type, content)}
   end
 
+  @spec redact_content(String.t(), map()) :: map()
   defp redact_content("m.room.member", content), do: Map.take(content, ["membership"])
   defp redact_content("m.room.create", content), do: Map.take(content, ["creator"])
   defp redact_content("m.room.join_rules", content), do: Map.take(content, ["join_rule"])
@@ -336,6 +378,7 @@ defmodule MatrixServer.Event do
   defp redact_content(_, _), do: %{}
 
   # Adds content hash, adds signature and calculates event id.
+  @spec post_process(t()) :: {:ok, t()} | :error
   def post_process(event) do
     with {:ok, content_hash} <- calculate_content_hash(event) do
       encoded_hash = MatrixServer.encode_unpadded_base64(content_hash)
@@ -346,23 +389,30 @@ defmodule MatrixServer.Event do
 
         with {:ok, event} <- set_event_id(event) do
           {:ok, event}
+        else
+          _ -> :error
         end
       end
+    else
+      _ -> :error
     end
   end
 
+  @spec set_event_id(t()) :: {:ok, t()} | {:error, Jason.EncodeError.t()}
   def set_event_id(event) do
     with {:ok, event_id} <- generate_event_id(event) do
       {:ok, %Event{event | event_id: event_id}}
     end
   end
 
+  @spec generate_event_id(t()) :: {:ok, String.t()} | {:error, Jason.EncodeError.t()}
   defp generate_event_id(event) do
     with {:ok, hash} <- calculate_reference_hash(event) do
       {:ok, "$" <> MatrixServer.encode_url_safe_base64(hash)}
     end
   end
 
+  @spec calculate_reference_hash(t()) :: {:ok, binary()} | {:error, Jason.EncodeError.t()}
   defp calculate_reference_hash(event) do
     redacted_event =
       event
diff --git a/lib/matrix_server/state_resolution.ex b/lib/matrix_server/state_resolution.ex
index 9f6cb72..0eb38fc 100644
--- a/lib/matrix_server/state_resolution.ex
+++ b/lib/matrix_server/state_resolution.ex
@@ -4,6 +4,8 @@ defmodule MatrixServer.StateResolution do
   alias MatrixServer.{Repo, Event, Room}
   alias MatrixServer.StateResolution.Authorization
 
+  @type state_set :: map()
+
   def resolve(event), do: resolve(event, true)
 
   def resolve(%Event{room_id: room_id} = event, apply_state) do
@@ -191,7 +193,7 @@ defmodule MatrixServer.StateResolution do
 
     # TODO: refactor
     case room_events[pl_event_id] do
-      %Event{content: %{"users" => pl_users}} -> Map.get(pl_users, sender, 0)
+      %Event{content: %{"users" => pl_users}} -> Map.get(pl_users, to_string(sender), 0)
       nil -> 0
     end
   end
diff --git a/lib/matrix_server/state_resolution/authorization.ex b/lib/matrix_server/state_resolution/authorization.ex
index 61b7417..5563e40 100644
--- a/lib/matrix_server/state_resolution/authorization.ex
+++ b/lib/matrix_server/state_resolution/authorization.ex
@@ -3,7 +3,12 @@ defmodule MatrixServer.StateResolution.Authorization do
   import Ecto.Query
 
   alias MatrixServer.{Repo, Event}
+  alias MatrixServer.Types.UserId
+  alias MatrixServer.StateResolution, as: StateRes
 
+  @typep action :: :invite | :ban | :redact | :kick | {:event, Event.t()}
+
+  @spec authorized?(Event.t(), StateRes.state_set()) :: boolean()
   def authorized?(%Event{type: "m.room.create", prev_events: prev_events}, %{}),
     do: prev_events == []
 
@@ -18,7 +23,7 @@ defmodule MatrixServer.StateResolution.Authorization do
         state_set
       ) do
     join_rule = get_join_rule(state_set)
-    membership = get_membership(sender, state_set)
+    membership = get_membership(to_string(sender), state_set)
 
     # Check rules: 5.2.3, 5.2.4, 5.2.5
     cond do
@@ -48,7 +53,7 @@ defmodule MatrixServer.StateResolution.Authorization do
         },
         state_set
       ) do
-    sender_membership = get_membership(sender, state_set)
+    sender_membership = get_membership(to_string(sender), state_set)
     target_membership = get_membership(state_key, state_set)
     power_levels = get_power_levels(state_set)
 
@@ -56,7 +61,7 @@ defmodule MatrixServer.StateResolution.Authorization do
     cond do
       sender_membership != "join" -> false
       target_membership in ["join", "ban"] -> false
-      has_power_level(sender, power_levels, :invite) -> true
+      has_power_level?(to_string(sender), power_levels, :invite) -> true
       true -> false
     end
   end
@@ -71,7 +76,7 @@ defmodule MatrixServer.StateResolution.Authorization do
         state_set
       ) do
     # Check rule: 5.4.1
-    get_membership(sender, state_set) in ["invite", "join"]
+    get_membership(to_string(sender), state_set) in ["invite", "join"]
   end
 
   def authorized?(
@@ -83,18 +88,25 @@ defmodule MatrixServer.StateResolution.Authorization do
         },
         state_set
       ) do
-    sender_membership = get_membership(sender, state_set)
+    sender_membership = get_membership(to_string(sender), state_set)
     target_membership = get_membership(state_key, state_set)
     power_levels = get_power_levels(state_set)
-    sender_pl = get_user_power_level(sender, power_levels)
+    sender_pl = get_user_power_level(to_string(sender), power_levels)
     target_pl = get_user_power_level(state_key, power_levels)
 
     # Check rules: 5.4.2, 5.4.3, 5.4.4
     cond do
-      sender_membership != "join" -> false
-      target_membership == "ban" and not has_power_level(sender, power_levels, :ban) -> false
-      has_power_level(sender, power_levels, :kick) and target_pl < sender_pl -> true
-      true -> false
+      sender_membership != "join" ->
+        false
+
+      target_membership == "ban" and not has_power_level?(to_string(sender), power_levels, :ban) ->
+        false
+
+      has_power_level?(to_string(sender), power_levels, :kick) and target_pl < sender_pl ->
+        true
+
+      true ->
+        false
     end
   end
 
@@ -107,15 +119,15 @@ defmodule MatrixServer.StateResolution.Authorization do
         },
         state_set
       ) do
-    sender_membership = get_membership(sender, state_set)
+    sender_membership = get_membership(to_string(sender), state_set)
     power_levels = get_power_levels(state_set)
-    sender_pl = get_user_power_level(sender, power_levels)
+    sender_pl = get_user_power_level(to_string(sender), power_levels)
     target_pl = get_user_power_level(state_key, power_levels)
 
     # Check rules: 5.5.1, 5.5.2
     cond do
       sender_membership != "join" -> false
-      has_power_level(sender, power_levels, :ban) and target_pl < sender_pl -> true
+      has_power_level?(to_string(sender), power_levels, :ban) and target_pl < sender_pl -> true
       true -> false
     end
   end
@@ -125,14 +137,15 @@ defmodule MatrixServer.StateResolution.Authorization do
 
   def authorized?(%Event{sender: sender} = event, state_set) do
     # Check rule: 6
-    get_membership(sender, state_set) == "join" and _authorized?(event, state_set)
+    get_membership(to_string(sender), state_set) == "join" and _authorized?(event, state_set)
   end
 
+  @spec _authorized?(Event.t(), StateRes.state_set()) :: boolean()
   defp _authorized?(%Event{type: "m.room.third_party_invite", sender: sender}, state_set) do
     power_levels = get_power_levels(state_set)
     # Check rule: 7.1
 
-    has_power_level(sender, power_levels, :invite)
+    has_power_level?(to_string(sender), power_levels, :invite)
   end
 
   defp _authorized?(%Event{state_key: state_key, sender: sender} = event, state_set) do
@@ -140,19 +153,20 @@ defmodule MatrixServer.StateResolution.Authorization do
 
     # Check rules: 8, 9
     cond do
-      not has_power_level(sender, power_levels, {:event, event}) -> false
+      not has_power_level?(to_string(sender), power_levels, {:event, event}) -> false
       String.starts_with?(state_key, "@") and state_key != sender -> false
       true -> __authorized?(event, state_set)
     end
   end
 
+  @spec __authorized?(Event.t(), StateRes.state_set()) :: boolean()
   defp __authorized?(
          %Event{type: "m.room.power_levels", sender: sender, content: content},
          state_set
        ) do
     current_pls = get_power_levels(state_set)
     new_pls = content
-    sender_pl = get_user_power_level(sender, new_pls)
+    sender_pl = get_user_power_level(to_string(sender), new_pls)
 
     # Check rules: 10.2, 10.3, 10.4, 10.5
     cond do
@@ -166,40 +180,43 @@ defmodule MatrixServer.StateResolution.Authorization do
 
   defp __authorized?(_, _), do: true
 
+  @spec get_power_levels(StateRes.state_set()) :: map() | nil
   defp get_power_levels(state_set) do
-    case state_set[{"m.room.power_levels", ""}] do
-      %Event{content: content} -> content
-      nil -> nil
+    with %Event{content: content} <- state_set[{"m.room.power_levels", ""}] do
+      content
     end
   end
 
+  @spec get_join_rule(StateRes.state_set()) :: String.t() | nil
   defp get_join_rule(state_set) do
-    case state_set[{"m.room.join_rules", ""}] do
-      %Event{content: %{"join_rule" => join_rule}} -> join_rule
-      nil -> nil
+    with %Event{content: %{"join_rule" => join_rule}} <- state_set[{"m.room.join_rules", ""}] do
+      join_rule
     end
   end
 
+  @spec get_membership(String.t(), StateRes.state_set()) :: String.t() | nil
   defp get_membership(user, state_set) do
-    case state_set[{"m.room.member", user}] do
-      %Event{content: %{"membership" => membership}} -> membership
-      nil -> nil
+    with %Event{content: %{"membership" => membership}} <- state_set[{"m.room.member", user}] do
+      membership
     end
   end
 
-  defp has_power_level(user, power_levels, action) do
+  @spec has_power_level?(String.t(), map() | nil, action()) :: boolean()
+  defp has_power_level?(user, power_levels, action) do
     user_pl = get_user_power_level(user, power_levels)
     action_pl = get_action_power_level(action, power_levels)
 
     user_pl >= action_pl
   end
 
+  @spec get_user_power_level(String.t(), map() | nil) :: non_neg_integer()
   defp get_user_power_level(user, %{"users" => users}) when is_map_key(users, user),
     do: users[user]
 
   defp get_user_power_level(_, %{"users_default" => pl}), do: pl
   defp get_user_power_level(_, _), do: 0
 
+  @spec get_action_power_level(action(), map() | nil) :: non_neg_integer()
   defp get_action_power_level(:invite, %{"invite" => pl}), do: pl
   defp get_action_power_level(:invite, _), do: 50
   defp get_action_power_level(:ban, %{"ban" => pl}), do: pl
@@ -218,7 +235,7 @@ defmodule MatrixServer.StateResolution.Authorization do
       case power_levels do
         %{"state_default" => pl} -> pl
         %{} -> 50
-        _ -> 0
+        nil -> 0
       end
     else
       case power_levels do
@@ -228,6 +245,9 @@ defmodule MatrixServer.StateResolution.Authorization do
     end
   end
 
+  # TODO: Power_levels may not have all these keys defined.
+  @spec authorize_power_levels(UserId.t(), non_neg_integer(), map() | nil, map() | nil) ::
+          boolean()
   defp authorize_power_levels(
          user,
          user_pl,
@@ -239,9 +259,12 @@ defmodule MatrixServer.StateResolution.Authorization do
     valid_power_level_key_changes(Map.take(current_pls, keys), Map.take(new_pls, keys), user_pl) and
       valid_power_level_key_changes(current_events, new_events, user_pl) and
       valid_power_level_key_changes(current_users, new_users, user_pl) and
-      valid_power_level_users_changes(current_users, new_users, user, user_pl)
+      valid_power_level_users_changes(current_users, new_users, to_string(user), user_pl)
   end
 
+  defp authorize_power_levels(_, _, _, _), do: false
+
+  @spec valid_power_level_key_changes(map(), map(), non_neg_integer()) :: boolean()
   defp valid_power_level_key_changes(l1, l2, user_pl) do
     set1 = MapSet.new(l1)
     set2 = MapSet.new(l2)
diff --git a/lib/matrix_server/types/alias_id.ex b/lib/matrix_server/types/alias_id.ex
index f738486..74dd299 100644
--- a/lib/matrix_server/types/alias_id.ex
+++ b/lib/matrix_server/types/alias_id.ex
@@ -18,7 +18,7 @@ defmodule MatrixServer.Types.AliasId do
          [localpart, domain] <- String.split(rest, ":", parts: 2) do
       if String.length(localpart) + String.length(domain) + 2 <= 255 and
            MatrixServer.valid_domain?(domain) do
-        %AliasId{localpart: localpart, domain: domain}
+        {:ok, %AliasId{localpart: localpart, domain: domain}}
       else
         :error
       end
@@ -33,11 +33,11 @@ defmodule MatrixServer.Types.AliasId do
     "@" <> rest = s
     [localpart, domain] = String.split(rest, ":", parts: 2)
 
-    %AliasId{localpart: localpart, domain: domain}
+    {:ok, %AliasId{localpart: localpart, domain: domain}}
   end
 
   def load(_), do: :error
 
-  def dump(%AliasId{} = alias_id), do: to_string(alias_id)
+  def dump(%AliasId{} = alias_id), do: {:ok, to_string(alias_id)}
   def dump(_), do: :error
 end
diff --git a/lib/matrix_server/types/event_id.ex b/lib/matrix_server/types/event_id.ex
index 47fe242..7dc9d4d 100644
--- a/lib/matrix_server/types/event_id.ex
+++ b/lib/matrix_server/types/event_id.ex
@@ -18,7 +18,7 @@ defmodule MatrixServer.Types.EventId do
   def cast(s) when is_binary(s) do
     with "$" <> id <- s do
       if Regex.match?(@id_regex, id) do
-        %EventId{id: id}
+        {:ok, %EventId{id: id}}
       else
         :error
       end
@@ -32,11 +32,11 @@ defmodule MatrixServer.Types.EventId do
   def load(s) when is_binary(s) do
     "$" <> rest = s
 
-    %EventId{id: rest}
+    {:ok, %EventId{id: rest}}
   end
 
   def load(_), do: :error
 
-  def dump(%EventId{} = event_id), do: to_string(event_id)
+  def dump(%EventId{} = event_id), do: {:ok, to_string(event_id)}
   def dump(_), do: :error
 end
diff --git a/lib/matrix_server/types/group_id.ex b/lib/matrix_server/types/group_id.ex
index d73d8dc..542e03c 100644
--- a/lib/matrix_server/types/group_id.ex
+++ b/lib/matrix_server/types/group_id.ex
@@ -21,7 +21,7 @@ defmodule MatrixServer.Types.GroupId do
       if String.length(localpart) + String.length(domain) + 2 <= 255 and
            Regex.match?(@localpart_regex, localpart) and
            MatrixServer.valid_domain?(domain) do
-        %GroupId{localpart: localpart, domain: domain}
+        {:ok, %GroupId{localpart: localpart, domain: domain}}
       else
         :error
       end
@@ -36,11 +36,11 @@ defmodule MatrixServer.Types.GroupId do
     "@" <> rest = s
     [localpart, domain] = String.split(rest, ":", parts: 2)
 
-    %GroupId{localpart: localpart, domain: domain}
+    {:ok, %GroupId{localpart: localpart, domain: domain}}
   end
 
   def load(_), do: :error
 
-  def dump(%GroupId{} = group_id), do: to_string(group_id)
+  def dump(%GroupId{} = group_id), do: {:ok, to_string(group_id)}
   def dump(_), do: :error
 end
diff --git a/lib/matrix_server/types/room_id.ex b/lib/matrix_server/types/room_id.ex
index 0e7cf00..c4b266a 100644
--- a/lib/matrix_server/types/room_id.ex
+++ b/lib/matrix_server/types/room_id.ex
@@ -17,7 +17,7 @@ defmodule MatrixServer.Types.RoomId do
     with "!" <> rest <- s,
          [localpart, domain] <- String.split(rest, ":", parts: 2) do
       if MatrixServer.valid_domain?(domain) do
-        %RoomId{localpart: localpart, domain: domain}
+        {:ok, %RoomId{localpart: localpart, domain: domain}}
       else
         :error
       end
@@ -32,11 +32,11 @@ defmodule MatrixServer.Types.RoomId do
     "!" <> rest = s
     [localpart, domain] = String.split(rest, ":", parts: 2)
 
-    %RoomId{localpart: localpart, domain: domain}
+    {:ok, %RoomId{localpart: localpart, domain: domain}}
   end
 
   def load(_), do: :error
 
-  def dump(%RoomId{} = room_id), do: to_string(room_id)
+  def dump(%RoomId{} = room_id), do: {:ok, to_string(room_id)}
   def dump(_), do: :error
 end
diff --git a/lib/matrix_server/types/user_id.ex b/lib/matrix_server/types/user_id.ex
index 585f05e..2754fbc 100644
--- a/lib/matrix_server/types/user_id.ex
+++ b/lib/matrix_server/types/user_id.ex
@@ -3,6 +3,11 @@ defmodule MatrixServer.Types.UserId do
 
   alias MatrixServer.Types.UserId
 
+  @type t :: %__MODULE__{
+          localpart: String.t(),
+          domain: String.t()
+        }
+
   defstruct [:localpart, :domain]
 
   @localpart_regex ~r/^[a-z0-9._=\-\/]+$/
@@ -20,7 +25,7 @@ defmodule MatrixServer.Types.UserId do
          [localpart, domain] <- String.split(rest, ":", parts: 2) do
       if String.length(localpart) + String.length(domain) + 2 <= 255 and
            Regex.match?(@localpart_regex, localpart) and MatrixServer.valid_domain?(domain) do
-        %UserId{localpart: localpart, domain: domain}
+        {:ok, %UserId{localpart: localpart, domain: domain}}
       else
         :error
       end
@@ -35,11 +40,11 @@ defmodule MatrixServer.Types.UserId do
     "@" <> rest = s
     [localpart, domain] = String.split(rest, ":", parts: 2)
 
-    %UserId{localpart: localpart, domain: domain}
+    {:ok, %UserId{localpart: localpart, domain: domain}}
   end
 
   def load(_), do: :error
 
-  def dump(%UserId{} = user_id), do: to_string(user_id)
+  def dump(%UserId{} = user_id), do: {:ok, to_string(user_id)}
   def dump(_), do: :error
 end