Uniformize spam-checker API, part 5: expand other spam-checker callbacks to return Tuple[Codes, dict]

[Yoric]

The objective of this push is to progressively expand the spam-checker API into letting spam-checkers provide reasons for which an event/operation has been rejected. In particular, we wish to prototype account suspension (by opposition to account removal) through spam-checkers and let users know what they need to do to have the account unsuspended.

Specifically, letting a spam-checker return an additional dict gives us a way to point users towards an individual url on which they can read up all the details about their suspension.

This is a followup to:

• Uniformize spam-checker API, part 1: the Code enum. #12703, Uniformize spam-checker API, part 1: turning Codes into an Enum.
• Uniformize spam-checker API, part 2: check_event_for_spam #12808, Uniformize spam-checker API, part 2: converting check_event_for_spam to let callbacks return Codes.
• Uniformize spam-checker API, part 3: Expand check_event_for_spam with the ability to return additional fields #12846, Uniformize spam-checker API, part 3: extend check_event_for_spam to let callbacks return additional fields.
• Uniformize spam-checker API, part 4: port other spam-checker callbacks to return Union[Allow, Codes]. #12857, Uniformize spam-checker API, part 4: convert other spam-checker callbacks to return Codes.

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.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[Yoric]

As far as I can tell, the monolith test fails because of a network issue (cannot assign IP), unrelated to the PR.

[Yoric]

For some reason, poetry run ./scripts-dev/lint.sh disagree on CI and on my machine.

[babolivier]

Sorry about the round-trip time for this one!

[babolivier]

It looks to me like this test could be simplified a lot by using parameterized.expand, see

synapse/tests/http/test_proxyagent.py

Lines 56 to 208 in 339c391

	@parameterized.expand(
	[
	# host
	[b"localhost", b"http", b"localhost", 1080, None],
	[b"localhost:9988", b"http", b"localhost", 9988, None],
	# host+scheme
	[b"https://localhost", b"https", b"localhost", 1080, None],
	[b"https://localhost:1234", b"https", b"localhost", 1234, None],
	# ipv4
	[b"1.2.3.4", b"http", b"1.2.3.4", 1080, None],
	[b"1.2.3.4:9988", b"http", b"1.2.3.4", 9988, None],
	# ipv4+scheme
	[b"https://1.2.3.4", b"https", b"1.2.3.4", 1080, None],
	[b"https://1.2.3.4:9988", b"https", b"1.2.3.4", 9988, None],
	# ipv6 - without brackets is broken
	# [
	# b"2001:0db8:85a3:0000:0000:8a2e:0370:effe",
	# b"http",
	# b"2001:0db8:85a3:0000:0000:8a2e:0370:effe",
	# 1080,
	# None,
	# ],
	# [
	# b"2001:0db8:85a3:0000:0000:8a2e:0370:1234",
	# b"http",
	# b"2001:0db8:85a3:0000:0000:8a2e:0370:1234",
	# 1080,
	# None,
	# ],
	# [b"::1", b"http", b"::1", 1080, None],
	# [b"::ffff:0.0.0.0", b"http", b"::ffff:0.0.0.0", 1080, None],
	# ipv6 - with brackets
	[
	b"[2001:0db8:85a3:0000:0000:8a2e:0370:effe]",
	b"http",
	b"2001:0db8:85a3:0000:0000:8a2e:0370:effe",
	1080,
	None,
	],
	[
	b"[2001:0db8:85a3:0000:0000:8a2e:0370:1234]",
	b"http",
	b"2001:0db8:85a3:0000:0000:8a2e:0370:1234",
	1080,
	None,
	],
	[b"[::1]", b"http", b"::1", 1080, None],
	[b"[::ffff:0.0.0.0]", b"http", b"::ffff:0.0.0.0", 1080, None],
	# ipv6+port
	[
	b"[2001:0db8:85a3:0000:0000:8a2e:0370:effe]:9988",
	b"http",
	b"2001:0db8:85a3:0000:0000:8a2e:0370:effe",
	9988,
	None,
	],
	[
	b"[2001:0db8:85a3:0000:0000:8a2e:0370:1234]:9988",
	b"http",
	b"2001:0db8:85a3:0000:0000:8a2e:0370:1234",
	9988,
	None,
	],
	[b"[::1]:9988", b"http", b"::1", 9988, None],
	[b"[::ffff:0.0.0.0]:9988", b"http", b"::ffff:0.0.0.0", 9988, None],
	# ipv6+scheme
	[
	b"https://[2001:0db8:85a3:0000:0000:8a2e:0370:effe]",
	b"https",
	b"2001:0db8:85a3:0000:0000:8a2e:0370:effe",
	1080,
	None,
	],
	[
	b"https://[2001:0db8:85a3:0000:0000:8a2e:0370:1234]",
	b"https",
	b"2001:0db8:85a3:0000:0000:8a2e:0370:1234",
	1080,
	None,
	],
	[b"https://[::1]", b"https", b"::1", 1080, None],
	[b"https://[::ffff:0.0.0.0]", b"https", b"::ffff:0.0.0.0", 1080, None],
	# ipv6+scheme+port
	[
	b"https://[2001:0db8:85a3:0000:0000:8a2e:0370:effe]:9988",
	b"https",
	b"2001:0db8:85a3:0000:0000:8a2e:0370:effe",
	9988,
	None,
	],
	[
	b"https://[2001:0db8:85a3:0000:0000:8a2e:0370:1234]:9988",
	b"https",
	b"2001:0db8:85a3:0000:0000:8a2e:0370:1234",
	9988,
	None,
	],
	[b"https://[::1]:9988", b"https", b"::1", 9988, None],
	# with credentials
	[
	b"https://user:[email protected]:9988",
	b"https",
	b"1.2.3.4",
	9988,
	b"user:pass",
	],
	[b"user:[email protected]:9988", b"http", b"1.2.3.4", 9988, b"user:pass"],
	[
	b"https://user:[email protected]:9988",
	b"https",
	b"proxy.local",
	9988,
	b"user:pass",
	],
	[
	b"user:[email protected]:9988",
	b"http",
	b"proxy.local",
	9988,
	b"user:pass",
	],
	]
	)
	def test_parse_proxy(
	self,
	proxy_string: bytes,
	expected_scheme: bytes,
	expected_hostname: bytes,
	expected_port: int,
	expected_credentials: Optional[bytes],
	):
	"""
	Tests that a given proxy URL will be broken into the components.
	Args:
	proxy_string: The proxy connection string.
	expected_scheme: Expected value of proxy scheme.
	expected_hostname: Expected value of proxy hostname.
	expected_port: Expected value of proxy port.
	expected_credentials: Expected value of credentials.
	Must be in form '<username>:<password>' or None
	"""
	proxy_cred = None
	if expected_credentials:
	proxy_cred = ProxyCredentials(expected_credentials)
	self.assertEqual(
	(
	expected_scheme,
	expected_hostname,
	expected_port,
	proxy_cred,
	),
	parse_proxy(proxy_string),
	)

or

synapse/tests/app/test_openid_listener.py

Lines 42 to 79 in 5a15377

	@parameterized.expand(
	[
	(["federation"], "auth_fail"),
	([], "no_resource"),
	(["openid", "federation"], "auth_fail"),
	(["openid"], "auth_fail"),
	]
	)
	def test_openid_listener(self, names, expectation):
	"""
	Test different openid listener configurations.
	401 is success here since it means we hit the handler and auth failed.
	"""
	config = {
	"port": 8080,
	"type": "http",
	"bind_addresses": ["0.0.0.0"],
	"resources": [{"names": names}],
	}
	# Listen with the config
	self.hs._listen_http(parse_listener_def(config))
	# Grab the resource from the site that was told to listen
	site = self.reactor.tcpServers[0][1]
	try:
	site.resource.children[b"_matrix"].children[b"federation"]
	except KeyError:
	if expectation == "no_resource":
	return
	raise
	channel = make_request(
	self.reactor, site, "GET", "/_matrix/federation/v1/openid/userinfo"
	)
	self.assertEqual(channel.code, 401)

for examples.

[Yoric]

So, introduce a new framework to this test along with magic code generation to get rid of one loop?
I can do that if you insist, but my first impression is that this would actually increase cognitive load of reading that test.

[babolivier]

What do you mean by introducing a new framework? We already use it quite a bit in Synapse's tests suite. In my opinion it makes tests much easier to read as it reduces the complexity of the actual test code.

[Yoric]

I mean that reading this test doesn't require understanding that framework yet. If you wish me to introduce it, I can do that, of course.

[Yoric]

Well, your wish is my command :)

[babolivier]

This plus I really think we should be using parameterized in test_spam_checker_check_event_for_spam.