Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add Module API for reading and writing global account data. #12391

Merged
merged 12 commits into from
Apr 11, 2022
1 change: 1 addition & 0 deletions changelog.d/12391.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add a module API for reading and writing global account data.
72 changes: 72 additions & 0 deletions synapse/module_api/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import copy
import email.utils
import logging
from typing import (
Expand Down Expand Up @@ -211,6 +212,7 @@ def __init__(self, hs: "HomeServer", auth_handler: AuthHandler) -> None:
# We expose these as properties below in order to attach a helpful docstring.
self._http_client: SimpleHttpClient = hs.get_simple_http_client()
self._public_room_list_manager = PublicRoomListManager(hs)
self._account_data_manager = AccountDataManager(hs)

self._spam_checker = hs.get_spam_checker()
self._account_validity_handler = hs.get_account_validity_handler()
Expand Down Expand Up @@ -431,6 +433,14 @@ def public_room_list_manager(self) -> "PublicRoomListManager":
"""
return self._public_room_list_manager

@property
def account_data_manager(self) -> "AccountDataManager":
clokep marked this conversation as resolved.
Show resolved Hide resolved
"""Allows reading and modifying users' account data.

Added in Synapse v1.57.0.
"""
return self._account_data_manager

@property
def public_baseurl(self) -> str:
"""The configured public base URL for this homeserver.
Expand Down Expand Up @@ -1386,3 +1396,65 @@ async def remove_room_from_public_room_list(self, room_id: str) -> None:
room_id: The ID of the room.
"""
await self._store.set_room_is_public(room_id, False)


class AccountDataManager:
clokep marked this conversation as resolved.
Show resolved Hide resolved
"""
Allows modules to manage account data.
"""

def __init__(self, hs: "HomeServer") -> None:
self._hs = hs
self._store = hs.get_datastores().main
self._handler = hs.get_account_data_handler()

def _validate_user_id(self, user_id: str) -> None:
"""
Validates a user ID is valid and local.
Private method to be used in other account data methods.
"""
user = UserID.from_string(user_id)
if not self._hs.is_mine(user):
raise ValueError(
f"{user_id} is not local to this homeserver; can't access account data for remote users."
)

async def get_global(self, user_id: str, data_type: str) -> Optional[JsonDict]:
"""
Gets some global account data, of a specified type, for the specified user.

The provided user ID must be a valid user ID of a local user.

Added in Synapse v1.57.0.
"""
self._validate_user_id(user_id)

data = await self._store.get_global_account_data_by_type_for_user(
user_id, data_type
)
# We clone to prevent the module accidentally mutating the dict that
# lives in the cache, as that could introduce nasty bugs.
return copy.deepcopy(data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we usually freeze things? I'm not sure practically how much of a difference it makes, but it would mean that people can't even try to modify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was half-and-half on this one. I obviously don't want modules to mutate internal data.
But if you're going to give the module a clone anyway, what does freezing it buy you? They already can't damage anything, since it's a clone.

Maybe it makes more sense anyway. I guess I'll do it for now, it'll be easier to revert later than the other way around if we're not keen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Freezing it makes it immutable, so will raise errors instead of allowing mutation but doing nothing. That seems clearer to me?


async def put_global(
self, user_id: str, data_type: str, new_data: JsonDict
) -> None:
"""
Puts some global account data, of a specified type, for the specified user.

The provided user ID must be a valid user ID of a local user.

Please note that this will overwrite existing the account data of that type
for that user!

Added in Synapse v1.57.0.
"""
self._validate_user_id(user_id)

# Ensure the user exists, so we don't just write to users that aren't there.
if await self._store.get_userinfo_by_id(user_id) is None:
raise ValueError(f"User {user_id} does not exist on this server.")

# Use the account data handler to write the account data.
# Crucially, this makes a replication request if necessary.
clokep marked this conversation as resolved.
Show resolved Hide resolved
await self._handler.add_account_data_for_user(user_id, data_type, new_data)
158 changes: 158 additions & 0 deletions tests/module_api/test_account_data_manager.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
# Copyright 2022 The Matrix.org Foundation C.I.C.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
from synapse.api.errors import SynapseError
from synapse.rest import admin

from tests.unittest import HomeserverTestCase


class ModuleApiTestCase(HomeserverTestCase):
servlets = [
admin.register_servlets,
]

def prepare(self, reactor, clock, homeserver) -> None:
self._store = homeserver.get_datastores().main
self._module_api = homeserver.get_module_api()
self._account_data_mgr = self._module_api.account_data_manager

self.user_id = self.register_user("kristina", "secret")

def test_get_global(self) -> None:
"""
Tests that getting global account data through the module API works as
expected, including getting `None` for unset account data.
"""
self.get_success(
self._store.add_account_data_for_user(
self.user_id, "test.data", {"wombat": True}
)
)

# Getting existent account data works as expected.
self.assertEqual(
self.get_success(
self._account_data_mgr.get_global(self.user_id, "test.data")
),
{"wombat": True},
)

# Getting non-existent account data returns None.
self.assertIsNone(
self.get_success(
self._account_data_mgr.get_global(self.user_id, "no.data.at.all")
)
)

def test_get_global_validation(self) -> None:
"""
Tests that invalid or remote user IDs are treated as errors and raised as exceptions,
whilst getting global account data for a user.

This is a design choice to try and communicate potential bugs to modules
earlier on.
"""
with self.assertRaises(SynapseError):
self.get_success_or_raise(
self._account_data_mgr.get_global("this isn't a user id", "test.data")
)

with self.assertRaises(ValueError):
self.get_success_or_raise(
self._account_data_mgr.get_global("@valid.but:remote", "test.data")
)

def test_get_global_no_mutability(self) -> None:
"""
Tests that modules can't introduce bugs into Synapse by mutating the result
of `get_global`.
"""
# First add some account data to set up the test.
self.get_success(
self._store.add_account_data_for_user(
self.user_id, "test.data", {"wombat": True}
)
)

# Request that account data from the normal store; check it's as we expect.
self.assertEqual(
self.get_success(
self._store.get_global_account_data_by_type_for_user(
self.user_id, "test.data"
)
),
{"wombat": True},
)

# Now, as a module, request that data and then mutate it (out of negligence or otherwise).
the_data = self.get_success(
self._account_data_mgr.get_global(self.user_id, "test.data")
)
the_data["wombat"] = False

# As Synapse, request that account data once more from the normal store; check it's as we expect.
self.assertEqual(
self.get_success(
self._store.get_global_account_data_by_type_for_user(
self.user_id, "test.data"
)
),
{"wombat": True},
)

def test_put_global(self) -> None:
"""
Tests that written account data using `put_global` can be read out again later.
"""

self.get_success(
self._module_api.account_data_manager.put_global(
self.user_id, "test.data", {"wombat": True}
)
)

# Request that account data from the normal store; check it's as we expect.
self.assertEqual(
self.get_success(
self._store.get_global_account_data_by_type_for_user(
self.user_id, "test.data"
)
),
{"wombat": True},
)

def test_put_global_validation(self) -> None:
clokep marked this conversation as resolved.
Show resolved Hide resolved
"""
Tests that a module can't write account data to user IDs that don't have
actual users registered to them.
"""

with self.assertRaises(SynapseError):
self.get_success_or_raise(
self._account_data_mgr.put_global(
"this isn't a user id", "test.data", {}
)
)

with self.assertRaises(ValueError):
self.get_success_or_raise(
self._account_data_mgr.put_global("@valid.but:remote", "test.data", {})
)

with self.assertRaises(ValueError):
self.get_success_or_raise(
self._module_api.account_data_manager.put_global(
"@notregistered:test", "test.data", {}
)
)