-
Notifications
You must be signed in to change notification settings - Fork 204
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
base: develop
Are you sure you want to change the base?
Changes from all commits
78a9cc5
38e6eaa
81a36d9
f9a1254
2cae58f
c696620
4df9a8d
b8d94e7
2372961
97dcd01
92c0292
fe20d06
aa23bf8
2f4ac97
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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`. Contributed by [email protected]. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ | |
Optional, | ||
Tuple, | ||
Union, | ||
cast, | ||
) | ||
|
||
# `Literal` appears with Python 3.8. | ||
|
@@ -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], | ||
|
@@ -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 | ||
|
@@ -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())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Something like (untested and some stuffs are missing but it's so you get the idea) :
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -796,6 +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) | ||
|
||
# Kept old spam checker without `requester_id` tests for backwards compatibility. | ||
async def allow_all(user_profile: UserProfile) -> bool: | ||
# Allow all users. | ||
return False | ||
|
@@ -809,6 +810,7 @@ async def allow_all(user_profile: UserProfile) -> bool: | |
s = self.get_success(self.handler.search_users(u1, "user2", 10)) | ||
self.assertEqual(len(s["results"]), 1) | ||
|
||
# Kept old spam checker without `requester_id` tests for backwards compatibility. | ||
# Configure a spam checker that filters all users. | ||
async def block_all(user_profile: UserProfile) -> bool: | ||
# All users are spammy. | ||
|
@@ -820,6 +822,40 @@ 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: | ||
self.assertEqual(requester_id, u1) | ||
# Allow all users. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added assert is instance of string checks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we not do |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kept the old tests that does not have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add comments for backwards compatibility for these functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh good point I haven't noticed, thanks for the comments. |
||
) -> bool: | ||
self.assertEqual(requester_id, u1) | ||
# 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": { | ||
|
There was a problem hiding this comment.
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:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
There was a problem hiding this comment.
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.