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

Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions changelog.d/17916.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Module developers will have access to user id of requester when adding `check_username_for_spam` callbacks to `spam_checker_module_callbacks`.
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
11 changes: 8 additions & 3 deletions synapse/module_api/callbacks/spamchecker_callbacks.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@
]
],
]
CHECK_USERNAME_FOR_SPAM_CALLBACK = Callable[[UserProfile], Awaitable[bool]]
CHECK_USERNAME_FOR_SPAM_CALLBACK = Callable[[UserProfile, str], Awaitable[bool]]
LEGACY_CHECK_REGISTRATION_FOR_SPAM_CALLBACK = Callable[
[
Optional[dict],
Expand Down Expand Up @@ -716,7 +716,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,6 +729,7 @@ 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.
Expand All @@ -735,7 +738,9 @@ async def check_username_for_spam(self, user_profile: UserProfile) -> bool:
with Measure(self.clock, f"{callback.__module__}.{callback.__qualname__}"):
# 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
Contributor 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

res = await delay_cancellation(
callback(user_profile.copy(), requester_id)
)
if res:
return True

Expand Down
4 changes: 2 additions & 2 deletions tests/handlers/test_user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -796,7 +796,7 @@ def test_spam_checker(self) -> None:
s = self.get_success(self.handler.search_users(u1, "user2", 10))
self.assertEqual(len(s["results"]), 1)

async def allow_all(user_profile: UserProfile) -> bool:
async def allow_all(user_profile: UserProfile, requester_id: str) -> bool:
# Allow all users.
return False

Expand All @@ -810,7 +810,7 @@ async def allow_all(user_profile: UserProfile) -> bool:
self.assertEqual(len(s["results"]), 1)

# Configure a spam checker that filters all users.
async def block_all(user_profile: UserProfile) -> bool:
async def block_all(user_profile: UserProfile, requester_id: str) -> bool:
# All users are spammy.
return True

Expand Down