Uniformize spam-checker API, part 2: check_event_for_spam (#12808)

Signed-off-by: David Teller <davidt@element.io>

2 files changed, 46 insertions, 10 deletions

diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md
index 27c5a0ed5c..71f6f9f0ab 100644
--- a/docs/modules/spam_checker_callbacks.md
+++ b/docs/modules/spam_checker_callbacks.md
@@ -11,22 +11,29 @@ The available spam checker callbacks are:
 ### `check_event_for_spam`
 
 _First introduced in Synapse v1.37.0_
+_Signature extended to support Allow and Code in Synapse v1.60.0_
+_Boolean and string return value types deprecated in Synapse v1.60.0_
 
 ```python
-async def check_event_for_spam(event: "synapse.events.EventBase") -> Union[bool, str]
+async def check_event_for_spam(event: "synapse.module_api.EventBase") -> Union["synapse.module_api.ALLOW", "synapse.module_api.error.Codes", str, bool]
 ```
 
-Called when receiving an event from a client or via federation. The callback must return
-either:
-- an error message string, to indicate the event must be rejected because of spam and 
-  give a rejection reason to forward to clients;
-- the boolean `True`, to indicate that the event is spammy, but not provide further details; or
-- the booelan `False`, to indicate that the event is not considered spammy.
+Called when receiving an event from a client or via federation. The callback must return either:
+  - `synapse.module_api.ALLOW`, to allow the operation. Other callbacks
+    may still decide to reject it.
+  - `synapse.api.Codes` to reject the operation with an error code. In case
+    of doubt, `synapse.api.error.Codes.FORBIDDEN` is a good error code.
+  - (deprecated) a `str` to reject the operation and specify an error message. Note that clients
+    typically will not localize the error message to the user's preferred locale.
+  - (deprecated) on `False`, behave as `ALLOW`. Deprecated as confusing, as some
+    callbacks in expect `True` to allow and others `True` to reject.
+  - (deprecated) on `True`, behave as `synapse.api.error.Codes.FORBIDDEN`. Deprecated as confusing, as
+    some callbacks in expect `True` to allow and others `True` to reject.
 
 If multiple modules implement this callback, they will be considered in order. If a
-callback returns `False`, Synapse falls through to the next one. The value of the first
-callback that does not return `False` will be used. If this happens, Synapse will not call
-any of the subsequent implementations of this callback.
+callback returns `synapse.module_api.ALLOW`, Synapse falls through to the next one. The value of the
+first callback that does not return `synapse.module_api.ALLOW` will be used. If this happens, Synapse
+will not call any of the subsequent implementations of this callback.
 
 ### `user_may_join_room`
 
diff --git a/docs/upgrade.md b/docs/upgrade.md
index 92ca31b2f8..e7eadadb64 100644
--- a/docs/upgrade.md
+++ b/docs/upgrade.md
@@ -177,7 +177,36 @@ has queries that can be used to check a database for this problem in advance.
 
 </details>
 
+## SpamChecker API's `check_event_for_spam` has a new signature.
 
+The previous signature has been deprecated.
+
+Whereas `check_event_for_spam` callbacks used to return `Union[str, bool]`, they should now return `Union["synapse.module_api.Allow", "synapse.module_api.errors.Codes"]`.
+
+This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful.
+
+If your module implements `check_event_for_spam` as follows:
+
+```python
+async def check_event_for_spam(event):
+    if ...:
+        # Event is spam
+        return True
+    # Event is not spam
+    return False
+```
+
+you should rewrite it as follows:
+
+```python
+async def check_event_for_spam(event):
+    if ...:
+        # Event is spam, mark it as forbidden (you may use some more precise error
+        # code if it is useful).
+        return synapse.module_api.errors.Codes.FORBIDDEN
+    # Event is not spam, mark it as `ALLOW`.
+    return synapse.module_api.ALLOW
+```
 
 # Upgrading to v1.59.0