Explain the meaning of spam checker callbacks' return values

[DMRobertson]

#10758 documented various module API callbacks which must return a bool. It did not specify the meaning of those bools, leaving module authors to either inspect Synapse's source, or else guess the meaning of the return value based on the function's name.

In this change I have tried to explicitly spell out what each return value indicates. Please double-check this carefully: I did this in a hurry, and am not familiar with the module API.

Motivation: Most callbacks return False to prevent some kind of action from taking place. Unfortunately check_username_for_spam is the opposite (return True to exclude from user directory searches; False for no effect). This isn't obvious. As a result, for about a week or two we ran a module on matrix.org which effectively considered all users spammy.

[clokep]

Thanks for improving those docs!

[clokep]

Note that this won't be forwarded to clients over federation, but I don't know if that nuance is worth explaining.

[DMRobertson]

Could say local clients here maybe?

[clokep]

Interestingly this is called for both client-server API and federation, while user_may_send_3pid_invite is not (I suspect it doesn't make sense to, not sure it is worth calling out).

[clokep]

I believe this one is incorrect, see

synapse/synapse/rest/media/v1/media_storage.py

Lines 148 to 156 in 10a88ba

	spam = await self.spam_checker.check_media_file_for_spam(
	ReadableFileWrapper(self.clock, fname), file_info
	)
	if spam:
	logger.info("Blocking media due to spam checker")
	# Note that we'll delete the stored media, due to the
	# try/except below. The media also won't be stored in
	# the DB.
	raise SpamMediaException()

Suggested change

	Called when storing a local or remote file. The module must return a `bool` indicating
	whether the given file can be stored in the homeserver's media store. Return `False` if
	to prevent this file from being stored; otherwise return `True`.
	Called when storing a local or remote file. The module must return a `bool` indicating
	whether the given file can be stored in the homeserver's media store. Return `True`
	to prevent this file from being stored; otherwise return `False`.

[DMRobertson]

Thanks for spotting this. I propose wording this slightly differently---commit to follow shortly.

[babolivier]

This isn't obvious. As a result, for about a week or two we ran a module on matrix.org which effectively considered all users spammy.

For clarification, this was caused by the documentation lying (and saying that True meant users can appear in the user directory, which isn't the case, which is my bad), not by it being unclear. Though 👍 for clarification, it's worth doing it regardless.

[DMRobertson]

This isn't obvious. As a result, for about a week or two we ran a module on matrix.org which effectively considered all users spammy.

For clarification, this was caused by the documentation lying (and saying that True meant users can appear in the user directory, which isn't the case, which is my bad), not by it being unclear. Though +1 for clarification, it's worth doing it regardless.

I'm not sure I'd agree with that---I genuinely wasn't sure what the documentation was describing until I consulted the source code.

From chat elsewhere, I think the causes were

• putting together the module under time pressure;
• (reasonably!) assuming that this callback behaved like the others (return True for "no effect"); and
• booleans being inherently ambiguous (as opposed to some kind of named enum).

I'm not sure there was time to actually consult the documentation. But as you say, we may as well keep our docs in order.

[clokep]

Looks great, thank you!