Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

forward requester id to check username for spam callbacks #17916

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
3 changes: 3 additions & 0 deletions changelog.d/17916.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Module developers will have access to user id of requester when adding `check_username_for_spam` callbacks to `spam_checker_module_callbacks`.

Contributed by [email protected]
WilsonLe marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion docs/modules/spam_checker_callbacks.md
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ this callback.
_First introduced in Synapse v1.37.0_

```python
async def check_username_for_spam(user_profile: synapse.module_api.UserProfile) -> bool
async def check_username_for_spam(user_profile: synapse.module_api.UserProfile, requester_id: str) -> bool
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a sentence or two for what requester_id is please, something like:

The requester_id parameter is the ID of the user that called the user directory API.

Copy link
Author

Choose a reason for hiding this comment

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

Added.

Copy link
Author

Choose a reason for hiding this comment

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

@erikjohnston Hi I couldn't reply to your comment on using self.assertEqual(requester_id, u1) directly but I've added the checks.

```

Called when computing search results in the user directory. The module must return a
Expand Down
4 changes: 2 additions & 2 deletions docs/spam_checker.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ class ExampleSpamChecker:
async def user_may_publish_room(self, userid, room_id):
return True # allow publishing of all rooms

async def check_username_for_spam(self, user_profile):
return False # allow all usernames
async def check_username_for_spam(self, user_profile, requester_id):
return False # allow all usernames regardless of requester

async def check_registration_for_spam(
self,
Expand Down
2 changes: 1 addition & 1 deletion synapse/handlers/user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ async def search_users(
non_spammy_users = []
for user in results["results"]:
if not await self._spam_checker_module_callbacks.check_username_for_spam(
user
user, user_id
):
non_spammy_users.append(user)
results["results"] = non_spammy_users
Expand Down
30 changes: 27 additions & 3 deletions synapse/module_api/callbacks/spamchecker_callbacks.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
Optional,
Tuple,
Union,
cast,
)

# `Literal` appears with Python 3.8.
Expand Down Expand Up @@ -168,7 +169,10 @@
]
],
]
CHECK_USERNAME_FOR_SPAM_CALLBACK = Callable[[UserProfile], Awaitable[bool]]
CHECK_USERNAME_FOR_SPAM_CALLBACK = Union[
Callable[[UserProfile], Awaitable[bool]],
Callable[[UserProfile, str], Awaitable[bool]],
]
LEGACY_CHECK_REGISTRATION_FOR_SPAM_CALLBACK = Callable[
[
Optional[dict],
Expand Down Expand Up @@ -716,7 +720,9 @@ async def user_may_publish_room(

return self.NOT_SPAM

async def check_username_for_spam(self, user_profile: UserProfile) -> bool:
async def check_username_for_spam(
self, user_profile: UserProfile, requester_id: str
) -> bool:
"""Checks if a user ID or display name are considered "spammy" by this server.

If the server considers a username spammy, then it will not be included in
Expand All @@ -727,15 +733,33 @@ async def check_username_for_spam(self, user_profile: UserProfile) -> bool:
* user_id
* display_name
* avatar_url
requester_id: The user ID of the user making the user directory search request.

Returns:
True if the user is spammy.
"""
for callback in self._check_username_for_spam_callbacks:
with Measure(self.clock, f"{callback.__module__}.{callback.__qualname__}"):
checker_args = inspect.signature(callback)
# Make a copy of the user profile object to ensure the spam checker cannot
# modify it.
res = await delay_cancellation(callback(user_profile.copy()))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to not break current checkers that do not yet have requester_id in their signature.

Something like (untested and some stuffs are missing but it's so you get the idea) :

checker_args = inspect.signature(callback)
if len(checker_args.parameters) == 2:
    callback(user_profile.copy(), requester_id)
else:
    callback(user_profile.copy())

Copy link
Author

Choose a reason for hiding this comment

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

Added unit tests to ensure backwards compat

# Also ensure backwards compatibility with spam checker callbacks
# that don't expect the requester_id argument.
if len(checker_args.parameters) == 2:
callback_with_requester_id = cast(
Callable[[UserProfile, str], Awaitable[bool]], callback
)
res = await delay_cancellation(
callback_with_requester_id(user_profile.copy(), requester_id)
)
else:
callback_without_requester_id = cast(
Callable[[UserProfile], Awaitable[bool]], callback
)
res = await delay_cancellation(
callback_without_requester_id(user_profile.copy())
)

if res:
return True

Expand Down
32 changes: 32 additions & 0 deletions tests/handlers/test_user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -820,6 +820,38 @@ async def block_all(user_profile: UserProfile) -> bool:
s = self.get_success(self.handler.search_users(u1, "user2", 10))
self.assertEqual(len(s["results"]), 0)

async def allow_all_expects_requester_id(
user_profile: UserProfile, requester_id: str
) -> bool:
# Allow all users.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add something like assert requester_id is not None to test that requester_id is correctly passed here.

Copy link
Author

Choose a reason for hiding this comment

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

Added assert is instance of string checks

Copy link
Member

Choose a reason for hiding this comment

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

Can we not do self.assertEqual(requester_id, u1)?

return False

# Configure a spam checker that does not filter any users.
spam_checker = self.hs.get_module_api_callbacks().spam_checker
spam_checker._check_username_for_spam_callbacks = [
allow_all_expects_requester_id
]

# The results do not change:
# We get one search result when searching for user2 by user1.
s = self.get_success(self.handler.search_users(u1, "user2", 10))
self.assertEqual(len(s["results"]), 1)

# Configure a spam checker that filters all users.
async def block_all_expects_requester_id(
user_profile: UserProfile, requester_id: str
Copy link
Contributor

Choose a reason for hiding this comment

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

let's remove requester_id: str param here so we do test backward compatibility, and add a comment about it.

Copy link
Author

Choose a reason for hiding this comment

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

I kept the old tests that does not have requester_id. I simply added more tests with requester_id.

Copy link
Author

Choose a reason for hiding this comment

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

I'll add comments for backwards compatibility for these functions.

Copy link
Author

Choose a reason for hiding this comment

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

Added

Copy link
Contributor

Choose a reason for hiding this comment

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

I kept the old tests that does not have requester_id. I simply added more tests with requester_id.

Oh good point I haven't noticed, thanks for the comments.

) -> bool:
# All users are spammy.
return True

spam_checker._check_username_for_spam_callbacks = [
block_all_expects_requester_id
]

# User1 now gets no search results for any of the other users.
s = self.get_success(self.handler.search_users(u1, "user2", 10))
self.assertEqual(len(s["results"]), 0)

@override_config(
{
"spam_checker": {
Expand Down