Move the "email unsubscribe" resource, refactor the macaroon generator & simplify the access token verification logic

[sandhose]

This simplifies the access token verification logic by removing the rights parameter which was only ever used for the unsubscribe link in email notifications. The latter has been moved under the /_synapse namespace, since it is not a standard API.

This also makes the email verification link more secure, by embedding the app_id and pushkey in the macaroon and verifying it. This prevents the user from tampering the query parameters of that unsubscribe link

For macaroon generation, I:

• centralised all macaroon generation and verification logic to the MacaroonGenerator
• which I moved to synapse.utils
• changed its constructor to avoid it requiring a full Homeserver, but just a Clock, a hostname and a secret key
• which makes it quicker and easier to unit test, so I added tests for all of its methods

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)

[richvdh]

Haven't looked at the code here, but:

The latter has been moved under the /_synapse namespace

... we'll probably need to support the old path for a while, since otherwise any emails sent out just before upgrading will contain invalid links.

[sandhose]

... we'll probably need to support the old path for a while, since otherwise any emails sent out just before upgrading will contain invalid links.

I should have said that in the PR, but it's the case :)

synapse/synapse/rest/synapse/client/__init__.py

Lines 45 to 48 in 72e5833

	# Unsubscribe to notification emails link
	"/_synapse/client/unsubscribe": UnsubscribeResource(hs),
	# Legacy endpoint
	"/_matrix/client/unstable/pushers/remove": UnsubscribeResource(hs),

[sandhose]

I think I fixed the complement crash... It looks like complement runs with compression enabled on the client resources, which broke the building of the resource tree, because EncodingResourceWrapper has no listNames method, which is used in synapse.util.httpresourcetree.create_resource_tree

I don't know if it's the right fix, but if the resources from build_synapse_client_resource_tree are before the ClientRestResource in the resource dict, this crash does not happen. This reordering should work since dicts are ordered since python 3.6?

Here was the stack trace:

synapse_main | 2022-06-09 10:19:04,076 - synapse.app._base - 253 - CRITICAL - sentinel - Error during startup
synapse_main | Traceback (most recent call last):
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/app/_base.py", line 238, in wrapper
synapse_main |     await cb(*args, **kwargs)
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/app/homeserver.py", line 391, in start
synapse_main |     await _base.start(hs)
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/app/_base.py", line 465, in start
synapse_main |     hs.start_listening()
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/app/homeserver.py", line 282, in start_listening
synapse_main |     self._listener_http(self.config, listener)
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/app/homeserver.py", line 149, in _listener_http
synapse_main |     create_resource_tree(resources, root_resource),
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/util/httpresourcetree.py", line 50, in create_resource_tree
synapse_main |     if path_seg not in last_resource.listNames():
synapse_main | AttributeError: 'EncodingResourceWrapper' object has no attribute 'listNames'
synapse_main | Error during startup:
synapse_main | Traceback (most recent call last):
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/app/_base.py", line 238, in wrapper
synapse_main |     await cb(*args, **kwargs)
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/app/homeserver.py", line 391, in start
synapse_main |     await _base.start(hs)
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/app/_base.py", line 465, in start
synapse_main |     hs.start_listening()
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/app/homeserver.py", line 282, in start_listening
synapse_main |     self._listener_http(self.config, listener)
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/app/homeserver.py", line 149, in _listener_http
synapse_main |     create_resource_tree(resources, root_resource),
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/util/httpresourcetree.py", line 50, in create_resource_tree
synapse_main |     if path_seg not in last_resource.listNames():
synapse_main | AttributeError: 'EncodingResourceWrapper' object has no attribute 'listNames'

[clokep]

Not a huge issue, but I think this could have been split into smaller PRs to ease review. (One which moves and renames the resource; one which moves and consolidates that macaroon generation code; then another that actually changes behavior.)

[sandhose]

I think I replied to all comments, and fixed the 'legacy' route, ready for another round of review!

[clokep]

Looks good minus a couple quibbles.

[clokep]

Ah, I see UnsubscribeResource isn't a RestServlet...that's annoying. I suspect this is fine though! 👍

[sandhose]

It is, at least it worked when I tested locally :)