Move the "email unsubscribe" resource, refactor the macaroon generator & simplify the access token verification logic. (#12986)

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.

Macaroon generation is refactored:

- Centralised all macaroon generation and verification logic to the
  `MacaroonGenerator`
- Moved to `synapse.utils`
- Changed the constructor to require only a `Clock`, hostname, and a secret key
  (instead of a full `Homeserver`).
- Added tests for all methods.

5 files changed, 164 insertions, 17 deletions

diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py
index 54af9089e9..dfcfaf79b6 100644
--- a/tests/api/test_auth.py
+++ b/tests/api/test_auth.py
@@ -313,9 +313,7 @@ class AuthTestCase(unittest.HomeserverTestCase):
         self.assertEqual(self.store.insert_client_ip.call_count, 2)
 
     def test_get_user_from_macaroon(self):
-        self.store.get_user_by_access_token = simple_async_mock(
-            TokenLookupResult(user_id="@baldrick:matrix.org", device_id="device")
-        )
+        self.store.get_user_by_access_token = simple_async_mock(None)
 
         user_id = "@baldrick:matrix.org"
         macaroon = pymacaroons.Macaroon(
@@ -323,17 +321,14 @@ class AuthTestCase(unittest.HomeserverTestCase):
             identifier="key",
             key=self.hs.config.key.macaroon_secret_key,
         )
+        # "Legacy" macaroons should not work for regular users not in the database
         macaroon.add_first_party_caveat("gen = 1")
         macaroon.add_first_party_caveat("type = access")
         macaroon.add_first_party_caveat("user_id = %s" % (user_id,))
-        user_info = self.get_success(
-            self.auth.get_user_by_access_token(macaroon.serialize())
+        serialized = macaroon.serialize()
+        self.get_failure(
+            self.auth.get_user_by_access_token(serialized), InvalidClientTokenError
         )
-        self.assertEqual(user_id, user_info.user_id)
-
-        # TODO: device_id should come from the macaroon, but currently comes
-        # from the db.
-        self.assertEqual(user_info.device_id, "device")
 
     def test_get_guest_user_from_macaroon(self):
         self.store.get_user_by_id = simple_async_mock({"is_guest": True})
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 1231aed944..e6cd3af7b7 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -25,7 +25,7 @@ from synapse.handlers.sso import MappingException
 from synapse.server import HomeServer
 from synapse.types import JsonDict, UserID
 from synapse.util import Clock
-from synapse.util.macaroons import get_value_from_macaroon
+from synapse.util.macaroons import OidcSessionData, get_value_from_macaroon
 
 from tests.test_utils import FakeResponse, get_awaitable_result, simple_async_mock
 from tests.unittest import HomeserverTestCase, override_config
@@ -1227,7 +1227,7 @@ class OidcHandlerTestCase(HomeserverTestCase):
     ) -> str:
         from synapse.handlers.oidc import OidcSessionData
 
-        return self.handler._token_generator.generate_oidc_session_token(
+        return self.handler._macaroon_generator.generate_oidc_session_token(
             state=state,
             session_data=OidcSessionData(
                 idp_id="oidc",
@@ -1251,7 +1251,6 @@ async def _make_callback_with_userinfo(
         userinfo: the OIDC userinfo dict
         client_redirect_url: the URL to redirect to on success.
     """
-    from synapse.handlers.oidc import OidcSessionData
 
     handler = hs.get_oidc_handler()
     provider = handler._providers["oidc"]
@@ -1260,7 +1259,7 @@ async def _make_callback_with_userinfo(
     provider._fetch_userinfo = simple_async_mock(return_value=userinfo)  # type: ignore[assignment]
 
     state = "state"
-    session = handler._token_generator.generate_oidc_session_token(
+    session = handler._macaroon_generator.generate_oidc_session_token(
         state=state,
         session_data=OidcSessionData(
             idp_id="oidc",
diff --git a/tests/test_state.py b/tests/test_state.py
index 95f81bebae..b005dd8d0f 100644
--- a/tests/test_state.py
+++ b/tests/test_state.py
@@ -11,7 +11,7 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-from typing import Collection, Dict, List, Optional
+from typing import Collection, Dict, List, Optional, cast
 from unittest.mock import Mock
 
 from twisted.internet import defer
@@ -22,6 +22,8 @@ from synapse.api.room_versions import RoomVersions
 from synapse.events import make_event_from_dict
 from synapse.events.snapshot import EventContext
 from synapse.state import StateHandler, StateResolutionHandler
+from synapse.util import Clock
+from synapse.util.macaroons import MacaroonGenerator
 
 from tests import unittest
 
@@ -190,13 +192,18 @@ class StateTestCase(unittest.TestCase):
                 "get_clock",
                 "get_state_resolution_handler",
                 "get_account_validity_handler",
+                "get_macaroon_generator",
                 "hostname",
             ]
         )
+        clock = cast(Clock, MockClock())
         hs.config = default_config("tesths", True)
         hs.get_datastores.return_value = Mock(main=self.dummy_store)
         hs.get_state_handler.return_value = None
-        hs.get_clock.return_value = MockClock()
+        hs.get_clock.return_value = clock
+        hs.get_macaroon_generator.return_value = MacaroonGenerator(
+            clock, "tesths", b"verysecret"
+        )
         hs.get_auth.return_value = Auth(hs)
         hs.get_state_resolution_handler = lambda: StateResolutionHandler(hs)
         hs.get_storage_controllers.return_value = storage_controllers
diff --git a/tests/unittest.py b/tests/unittest.py
index e7f255b4fa..c645dd3563 100644
--- a/tests/unittest.py
+++ b/tests/unittest.py
@@ -315,7 +315,7 @@ class HomeserverTestCase(TestCase):
                         "is_guest": False,
                     }
 
-                async def get_user_by_req(request, allow_guest=False, rights="access"):
+                async def get_user_by_req(request, allow_guest=False):
                     assert self.helper.auth_user_id is not None
                     return create_requester(
                         UserID.from_string(self.helper.auth_user_id),
diff --git a/tests/util/test_macaroons.py b/tests/util/test_macaroons.py
new file mode 100644
index 0000000000..32125f7bb7
--- /dev/null
+++ b/tests/util/test_macaroons.py
@@ -0,0 +1,146 @@
+# Copyright 2022 The Matrix.org Foundation C.I.C.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from pymacaroons.exceptions import MacaroonVerificationFailedException
+
+from synapse.util.macaroons import MacaroonGenerator, OidcSessionData
+
+from tests.server import get_clock
+from tests.unittest import TestCase
+
+
+class MacaroonGeneratorTestCase(TestCase):
+    def setUp(self):
+        self.reactor, hs_clock = get_clock()
+        self.macaroon_generator = MacaroonGenerator(hs_clock, "tesths", b"verysecret")
+        self.other_macaroon_generator = MacaroonGenerator(
+            hs_clock, "tesths", b"anothersecretkey"
+        )
+
+    def test_guest_access_token(self):
+        """Test the generation and verification of guest access tokens"""
+        token = self.macaroon_generator.generate_guest_access_token("@user:tesths")
+        user_id = self.macaroon_generator.verify_guest_token(token)
+        self.assertEqual(user_id, "@user:tesths")
+
+        # Raises with another secret key
+        with self.assertRaises(MacaroonVerificationFailedException):
+            self.other_macaroon_generator.verify_guest_token(token)
+
+        # Check that an old access token without the guest caveat does not work
+        macaroon = self.macaroon_generator._generate_base_macaroon("access")
+        macaroon.add_first_party_caveat(f"user_id = {user_id}")
+        macaroon.add_first_party_caveat("nonce = 0123456789abcdef")
+        token = macaroon.serialize()
+
+        with self.assertRaises(MacaroonVerificationFailedException):
+            self.macaroon_generator.verify_guest_token(token)
+
+    def test_delete_pusher_token(self):
+        """Test the generation and verification of delete_pusher tokens"""
+        token = self.macaroon_generator.generate_delete_pusher_token(
+            "@user:tesths", "m.mail", "john@example.com"
+        )
+        user_id = self.macaroon_generator.verify_delete_pusher_token(
+            token, "m.mail", "john@example.com"
+        )
+        self.assertEqual(user_id, "@user:tesths")
+
+        # Raises with another secret key
+        with self.assertRaises(MacaroonVerificationFailedException):
+            self.other_macaroon_generator.verify_delete_pusher_token(
+                token, "m.mail", "john@example.com"
+            )
+
+        # Raises when verifying for another pushkey
+        with self.assertRaises(MacaroonVerificationFailedException):
+            self.macaroon_generator.verify_delete_pusher_token(
+                token, "m.mail", "other@example.com"
+            )
+
+        # Raises when verifying for another app_id
+        with self.assertRaises(MacaroonVerificationFailedException):
+            self.macaroon_generator.verify_delete_pusher_token(
+                token, "somethingelse", "john@example.com"
+            )
+
+        # Check that an old token without the app_id and pushkey still works
+        macaroon = self.macaroon_generator._generate_base_macaroon("delete_pusher")
+        macaroon.add_first_party_caveat("user_id = @user:tesths")
+        token = macaroon.serialize()
+        user_id = self.macaroon_generator.verify_delete_pusher_token(
+            token, "m.mail", "john@example.com"
+        )
+        self.assertEqual(user_id, "@user:tesths")
+
+    def test_short_term_login_token(self):
+        """Test the generation and verification of short-term login tokens"""
+        token = self.macaroon_generator.generate_short_term_login_token(
+            user_id="@user:tesths",
+            auth_provider_id="oidc",
+            auth_provider_session_id="sid",
+            duration_in_ms=2 * 60 * 1000,
+        )
+
+        info = self.macaroon_generator.verify_short_term_login_token(token)
+        self.assertEqual(info.user_id, "@user:tesths")
+        self.assertEqual(info.auth_provider_id, "oidc")
+        self.assertEqual(info.auth_provider_session_id, "sid")
+
+        # Raises with another secret key
+        with self.assertRaises(MacaroonVerificationFailedException):
+            self.other_macaroon_generator.verify_short_term_login_token(token)
+
+        # Wait a minute
+        self.reactor.pump([60])
+        # Shouldn't raise
+        self.macaroon_generator.verify_short_term_login_token(token)
+        # Wait another minute
+        self.reactor.pump([60])
+        # Should raise since it expired
+        with self.assertRaises(MacaroonVerificationFailedException):
+            self.macaroon_generator.verify_short_term_login_token(token)
+
+    def test_oidc_session_token(self):
+        """Test the generation and verification of OIDC session cookies"""
+        state = "arandomstate"
+        session_data = OidcSessionData(
+            idp_id="oidc",
+            nonce="nonce",
+            client_redirect_url="https://example.com/",
+            ui_auth_session_id="",
+        )
+        token = self.macaroon_generator.generate_oidc_session_token(
+            state, session_data, duration_in_ms=2 * 60 * 1000
+        ).encode("utf-8")
+        info = self.macaroon_generator.verify_oidc_session_token(token, state)
+        self.assertEqual(session_data, info)
+
+        # Raises with another secret key
+        with self.assertRaises(MacaroonVerificationFailedException):
+            self.other_macaroon_generator.verify_oidc_session_token(token, state)
+
+        # Should raise with another state
+        with self.assertRaises(MacaroonVerificationFailedException):
+            self.macaroon_generator.verify_oidc_session_token(token, "anotherstate")
+
+        # Wait a minute
+        self.reactor.pump([60])
+        # Shouldn't raise
+        self.macaroon_generator.verify_oidc_session_token(token, state)
+        # Wait another minute
+        self.reactor.pump([60])
+        # Should raise since it expired
+        with self.assertRaises(MacaroonVerificationFailedException):
+            self.macaroon_generator.verify_oidc_session_token(token, state)