forward requester id to check username for spam callbacks #17916

WilsonLe · 2024-11-11T08:45:31Z

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Code style is correct
(run the linters)

CLAassistant · 2024-11-11T08:45:38Z

All committers have signed the CLA.

MatMaul · 2024-11-12T11:56:56Z

synapse/module_api/callbacks/spamchecker_callbacks.py

@@ -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()))


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())

Added unit tests to ensure backwards compat

MatMaul · 2024-11-12T12:10:11Z

Thanks for that!

docs/spam_checker.md and docs/modules/spam_checker_callbacks.md would also need an update.

…spam_callback

… of https://github.com/WilsonLe/synapse into feature/add_reader_to_check_username_for_spam_callback

MatMaul · 2024-11-14T08:46:33Z

tests/handlers/test_user_directory.py

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


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

Added assert is instance of string checks

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

MatMaul · 2024-11-14T08:47:47Z

tests/handlers/test_user_directory.py

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


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

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

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

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.

… add comments on old tests on why keeping them is important

…spam_callback

MatMaul

LGTM, we now need someone from the core team to trigger the CI :)

changelog.d/17916.feature

Co-authored-by: Patrick Cloke <[email protected]>

…spam_callback

erikjohnston

Otherwise, LGTM thanks!

erikjohnston · 2024-11-18T14:00:28Z

docs/modules/spam_checker_callbacks.md

@@ -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


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.

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

erikjohnston · 2024-11-18T14:04:14Z

tests/handlers/test_user_directory.py

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


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

…ster id is

…spam_callback

forward requester id to check username for spam callbacks

78a9cc5

add changelog

38e6eaa

WilsonLe marked this pull request as ready for review November 11, 2024 08:51

WilsonLe requested a review from a team as a code owner November 11, 2024 08:51

MatMaul reviewed Nov 12, 2024

View reviewed changes

WilsonLe added 5 commits November 12, 2024 22:48

add backward compat with callbacks without requester_id defined

81a36d9

update docs

f9a1254

Merge branch 'develop' into feature/add_reader_to_check_username_for_…

2cae58f

…spam_callback

add contribution credits

c696620

Merge branch 'feature/add_reader_to_check_username_for_spam_callback'…

4df9a8d

… of https://github.com/WilsonLe/synapse into feature/add_reader_to_check_username_for_spam_callback

MatMaul reviewed Nov 14, 2024

View reviewed changes

WilsonLe added 2 commits November 14, 2024 16:00

add stricter tests to check for requester id is correctly passed, and…

b8d94e7

… add comments on old tests on why keeping them is important

Merge branch 'develop' into feature/add_reader_to_check_username_for_…

2372961

…spam_callback

MatMaul approved these changes Nov 14, 2024

View reviewed changes

github-actions bot deployed to PR Documentation Preview November 14, 2024 15:09 Active

clokep reviewed Nov 14, 2024

View reviewed changes

changelog.d/17916.feature Outdated Show resolved Hide resolved

WilsonLe and others added 2 commits November 15, 2024 02:45

Update changelog.d/17916.feature

97dcd01

Co-authored-by: Patrick Cloke <[email protected]>

Merge branch 'develop' into feature/add_reader_to_check_username_for_…

92c0292

…spam_callback

erikjohnston reviewed Nov 18, 2024

View reviewed changes

WilsonLe added 3 commits November 18, 2024 13:15

adjust unit tests for stricter tests, add documentation on what reque…

fe20d06

…ster id is

Merge branch 'develop' into feature/add_reader_to_check_username_for_…

aa23bf8

…spam_callback

Merge branch 'develop' into feature/add_reader_to_check_username_for_…

2f4ac97

…spam_callback

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

forward requester id to check username for spam callbacks #17916

forward requester id to check username for spam callbacks #17916

WilsonLe commented Nov 11, 2024

CLAassistant commented Nov 11, 2024 •

edited

Loading

MatMaul Nov 12, 2024

WilsonLe Nov 12, 2024

MatMaul commented Nov 12, 2024

MatMaul Nov 14, 2024

WilsonLe Nov 14, 2024

erikjohnston Nov 18, 2024

MatMaul Nov 14, 2024

WilsonLe Nov 14, 2024

WilsonLe Nov 14, 2024

WilsonLe Nov 14, 2024

MatMaul Nov 14, 2024

MatMaul left a comment

erikjohnston left a comment

erikjohnston Nov 18, 2024

WilsonLe Nov 18, 2024

WilsonLe Nov 25, 2024

erikjohnston Nov 18, 2024

forward requester id to check username for spam callbacks #17916

Are you sure you want to change the base?

forward requester id to check username for spam callbacks #17916

Conversation

WilsonLe commented Nov 11, 2024

Pull Request Checklist

CLAassistant commented Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatMaul commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatMaul left a comment

Choose a reason for hiding this comment

erikjohnston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Nov 11, 2024 •

edited

Loading