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

Add module API callbacks for adding and deleting local 3PID associations #15044

Merged
merged 10 commits into from
Feb 27, 2023
Merged

Add module API callbacks for adding and deleting local 3PID associations #15044

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
92f46e4
Add module API callbacks for adding and deleting local 3PID associations
anoadragon453 Feb 1, 2023
dc8bbe8
Poke Synapse modules when adding or removing a 3PID to a user's account
anoadragon453 Feb 1, 2023
3326a66
Remove bound 3pids separately from locally associated 3pids
anoadragon453 Feb 1, 2023
195f08c
Delete now unused user_delete_threepids storage method
anoadragon453 Feb 13, 2023
2688e5b
Deprecate `on_threepid_bind`
anoadragon453 Feb 3, 2023
ee8d1ca
Add documentation for new module api callback methods
anoadragon453 Feb 3, 2023
72a9c79
Add a couple unit tests
anoadragon453 Feb 2, 2023
312da2d
changelog
anoadragon453 Feb 9, 2023
a52fbcc
Move methods to after 3pid add/remove has successfully occurred
anoadragon453 Feb 22, 2023
fd26b16
Update references for v1.78 -> v1.79 for this feature. Fix upgrade notes
anoadragon453 Feb 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions synapse/events/third_party_rules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
ON_PROFILE_UPDATE_CALLBACK = Callable[[str, ProfileInfo, bool, bool], Awaitable]
ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK = Callable[[str, bool, bool], Awaitable]
ON_THREEPID_BIND_CALLBACK = Callable[[str, str, str], Awaitable]
ON_ADD_USER_THIRD_PARTY_IDENTIFIER_CALLBACK = Callable[[str, str, str], Awaitable]
ON_REMOVE_USER_THIRD_PARTY_IDENTIFIER_CALLBACK = Callable[[str, str, str], Awaitable]


def load_legacy_third_party_event_rules(hs: "HomeServer") -> None:
Expand Down Expand Up @@ -174,6 +176,12 @@ def __init__(self, hs: "HomeServer"):
ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK
] = []
self._on_threepid_bind_callbacks: List[ON_THREEPID_BIND_CALLBACK] = []
self._on_add_user_third_party_identifier_callbacks: List[
ON_ADD_USER_THIRD_PARTY_IDENTIFIER_CALLBACK
] = []
self._on_remove_user_third_party_identifier_callbacks: List[
ON_REMOVE_USER_THIRD_PARTY_IDENTIFIER_CALLBACK
] = []

def register_third_party_rules_callbacks(
self,
Expand All @@ -193,6 +201,12 @@ def register_third_party_rules_callbacks(
ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK
] = None,
on_threepid_bind: Optional[ON_THREEPID_BIND_CALLBACK] = None,
on_add_user_third_party_identifier: Optional[
ON_ADD_USER_THIRD_PARTY_IDENTIFIER_CALLBACK
] = None,
on_remove_user_third_party_identifier: Optional[
ON_REMOVE_USER_THIRD_PARTY_IDENTIFIER_CALLBACK
] = None,
) -> None:
"""Register callbacks from modules for each hook."""
if check_event_allowed is not None:
Expand Down Expand Up @@ -230,6 +244,11 @@ def register_third_party_rules_callbacks(
if on_threepid_bind is not None:
self._on_threepid_bind_callbacks.append(on_threepid_bind)

if on_add_user_third_party_identifier is not None:
self._on_add_user_third_party_identifier_callbacks.append(
on_add_user_third_party_identifier
)

async def check_event_allowed(
self,
event: EventBase,
Expand Down Expand Up @@ -525,3 +544,43 @@ async def on_threepid_bind(self, user_id: str, medium: str, address: str) -> Non
logger.exception(
"Failed to run module API callback %s: %s", callback, e
)

async def on_add_user_third_party_identifier(
self, user_id: str, medium: str, address: str
) -> None:
"""Called when an association between a user's Matrix ID and a third-party ID
(email, phone number) is about to be registered on the homeserver.

Args:
user_id: The User ID to include in the association.
medium: The medium of the third-party ID (email, msisdn).
address: The address of the third-party ID (i.e. an email address).
"""
for callback in self._on_add_user_third_party_identifier_callbacks:
try:
await callback(user_id, medium, address)
except Exception as e:
logger.exception(
"Failed to run module API callback %s: %s", callback, e
)

async def on_remove_user_third_party_identifier(
self, user_id: str, medium: str, address: str
) -> None:
"""Called when an association between a user's Matrix ID and a third-party ID
(email, phone number) is about to be removed on the homeserver. This is called
*after* any known bindings on identity servers for this association have been
removed.

Args:
user_id: The User ID including in the association to remove.
medium: The medium of the third-party ID (email, msisdn).
address: The address of the third-party ID (i.e. an email address).
"""
for callback in self._on_remove_user_third_party_identifier_callbacks:
try:
await callback(user_id, medium, address)
except Exception as e:
logger.exception(
"Failed to run module API callback %s: %s", callback, e
)
46 changes: 43 additions & 3 deletions synapse/handlers/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1543,6 +1543,17 @@ async def delete_access_tokens_for_user(
async def add_threepid(
self, user_id: str, medium: str, address: str, validated_at: int
) -> None:
"""
Adds an association between a user's Matrix ID and a third-party ID (email,
phone number).

Args:
user_id: The ID of the user to associate.
medium: The medium of the third-party ID (email, msisdn).
address: The address of the third-party ID (i.e. an email address).
validated_at: The timestamp in ms of when the validation that the user owns
this third-party ID occurred.
"""
# check if medium has a valid value
if medium not in ["email", "msisdn"]:
raise SynapseError(
Expand All @@ -1563,10 +1574,25 @@ async def add_threepid(
if medium == "email":
address = canonicalise_email(address)

await self.store.user_add_threepid(
user_id, medium, address, validated_at, self.hs.get_clock().time_msec()
# Inform Synapse modules that a 3PID association is about to be created.
await self._third_party_rules.on_add_user_third_party_identifier(
user_id, medium, address
)

try:
await self.store.user_add_threepid(
user_id, medium, address, validated_at, self.hs.get_clock().time_msec()
)
except Exception:
# We failed to store the association, but told Synapse modules otherwise.
# Tell them that the association was deleted.
await self._third_party_rules.on_remove_user_third_party_identifier(
user_id, medium, address
)
raise
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm curious on whether people think this level of caution is necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

I find it unintuitive that we trigger the event prior to performing the action.
The only reason I'd expect this would be if we were offering the modules the ability to cancel the event — however this does not seem to be the case.

As it stands, I'd likely prefer to trigger the events after the action has been performed instead, to avoid this 'takeback' situation.

If we are intending to let modules block the association, then we should probably support that here and now.
In my experience with other systems, though, there are usually multiple phases that an event listener can register themselves to an event for: at a minimum, before and after. (with the event only being cancellable before). Maybe we make that explicit with two different API callbacks (or two different phases) — it sounds awkward but then having no way to subscribe to events that are definitely going to happen is also awkward.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did intend to leave the door open to allowing modules to block the action, and I'm not opposed to adding it now. I only avoided doing so to save time, and don't consider it a breaking change to do so.

Having module callbacks that allow for both before and after an action sound reasonable. In the use case this is intended for (automatically binding 3pids to a company's Sydent instance), subscribing to a post-action callback does sound best (and delivers the guarantees we want without having to do an extra callback in case of a database exception. The module would just accept that if they were going for the pre-action callback.

I think I'm going to update this PR to only include the post-action callback (and leave the name as-is, as it sounds post-action-y already...). A future PR is welcome to add a can_add_user_third_party_identifier or somesuch that allows for blocking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in a52fbcc.


# Deprecated method for informing Synapse modules that a 3PID association
# has successfully been created.
await self._third_party_rules.on_threepid_bind(user_id, medium, address)

async def delete_threepid(
Expand Down Expand Up @@ -1597,7 +1623,21 @@ async def delete_threepid(
user_id, medium, address, id_server
)

await self.store.user_delete_threepid(user_id, medium, address)
# Inform Synapse modules that a 3PID association is about to be deleted.
await self._third_party_rules.on_remove_user_third_party_identifier(
user_id, medium, address
)

try:
await self.store.user_delete_threepid(user_id, medium, address)
except Exception:
# We failed to store the association, but told Synapse modules otherwise.
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
# Tell them that the association has come back.
await self._third_party_rules.on_add_user_third_party_identifier(
user_id, medium, address
)
raise

if medium == "email":
await self.store.delete_pusher_by_app_id_pushkey_user_id(
app_id="m.email", pushkey=address, user_id=user_id
Expand Down
10 changes: 10 additions & 0 deletions synapse/module_api/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,11 @@
CHECK_EVENT_ALLOWED_CALLBACK,
CHECK_THREEPID_CAN_BE_INVITED_CALLBACK,
CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK,
ON_ADD_USER_THIRD_PARTY_IDENTIFIER_CALLBACK,
ON_CREATE_ROOM_CALLBACK,
ON_NEW_EVENT_CALLBACK,
ON_PROFILE_UPDATE_CALLBACK,
ON_REMOVE_USER_THIRD_PARTY_IDENTIFIER_CALLBACK,
ON_THREEPID_BIND_CALLBACK,
ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK,
)
Expand Down Expand Up @@ -357,6 +359,12 @@ def register_third_party_rules_callbacks(
ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK
] = None,
on_threepid_bind: Optional[ON_THREEPID_BIND_CALLBACK] = None,
on_add_user_third_party_identifier: Optional[
ON_ADD_USER_THIRD_PARTY_IDENTIFIER_CALLBACK
] = None,
on_remove_user_third_party_identifier: Optional[
ON_REMOVE_USER_THIRD_PARTY_IDENTIFIER_CALLBACK
] = None,
) -> None:
"""Registers callbacks for third party event rules capabilities.

Expand All @@ -373,6 +381,8 @@ def register_third_party_rules_callbacks(
on_profile_update=on_profile_update,
on_user_deactivation_status_changed=on_user_deactivation_status_changed,
on_threepid_bind=on_threepid_bind,
on_add_user_third_party_identifier=on_add_user_third_party_identifier,
on_remove_user_third_party_identifier=on_remove_user_third_party_identifier,
)

def register_presence_router_callbacks(
Expand Down