Skip macaroon check for access tokens in the db

2 files changed, 28 insertions, 186 deletions

diff --git a/synapse/api/auth.py b/synapse/api/auth.py
index 5bbbe8e2e7..7d76dbd661 100644
--- a/synapse/api/auth.py
+++ b/synapse/api/auth.py
@@ -288,20 +288,28 @@ class Auth(object):
         Raises:
             AuthError if no user by that token exists or the token is invalid.
         """
-        try:
-            user_id, guest = self._parse_and_validate_macaroon(token, rights)
-        except _InvalidMacaroonException:
-            # doesn't look like a macaroon: treat it as an opaque token which
-            # must be in the database.
-            # TODO: it would be nice to get rid of this, but apparently some
-            # people use access tokens which aren't macaroons
+
+        if rights == "access":
+            # first look in the database
             r = yield self._look_up_user_by_access_token(token)
-            defer.returnValue(r)
+            if r:
+                defer.returnValue(r)
 
+        # otherwise it needs to be a valid macaroon
         try:
+            user_id, guest = self._parse_and_validate_macaroon(token, rights)
             user = UserID.from_string(user_id)
 
-            if guest:
+            if rights == "access":
+                if not guest:
+                    # non-guest access tokens must be in the database
+                    logger.warning("Unrecognised access token - not in store.")
+                    raise AuthError(
+                        self.TOKEN_NOT_FOUND_HTTP_STATUS,
+                        "Unrecognised access token.",
+                        errcode=Codes.UNKNOWN_TOKEN,
+                    )
+
                 # Guest access tokens are not stored in the database (there can
                 # only be one access token per guest, anyway).
                 #
@@ -342,31 +350,15 @@ class Auth(object):
                     "device_id": None,
                 }
             else:
-                # This codepath exists for several reasons:
-                #   * so that we can actually return a token ID, which is used
-                #     in some parts of the schema (where we probably ought to
-                #     use device IDs instead)
-                #   * the only way we currently have to invalidate an
-                #     access_token is by removing it from the database, so we
-                #     have to check here that it is still in the db
-                #   * some attributes (notably device_id) aren't stored in the
-                #     macaroon. They probably should be.
-                # TODO: build the dictionary from the macaroon once the
-                # above are fixed
-                ret = yield self._look_up_user_by_access_token(token)
-                if ret["user"] != user:
-                    logger.error(
-                        "Macaroon user (%s) != DB user (%s)",
-                        user,
-                        ret["user"]
-                    )
-                    raise AuthError(
-                        self.TOKEN_NOT_FOUND_HTTP_STATUS,
-                        "User mismatch in macaroon",
-                        errcode=Codes.UNKNOWN_TOKEN
-                    )
+                raise RuntimeError("Unknown rights setting %s", rights)
             defer.returnValue(ret)
-        except (pymacaroons.exceptions.MacaroonException, TypeError, ValueError):
+        except (
+            _InvalidMacaroonException,
+            pymacaroons.exceptions.MacaroonException,
+            TypeError,
+            ValueError,
+        ) as e:
+            logger.warning("Invalid macaroon in auth: %s %s", type(e), e)
             raise AuthError(
                 self.TOKEN_NOT_FOUND_HTTP_STATUS, "Invalid macaroon passed.",
                 errcode=Codes.UNKNOWN_TOKEN
@@ -496,11 +488,8 @@ class Auth(object):
     def _look_up_user_by_access_token(self, token):
         ret = yield self.store.get_user_by_access_token(token)
         if not ret:
-            logger.warn("Unrecognised access token - not in store.")
-            raise AuthError(
-                self.TOKEN_NOT_FOUND_HTTP_STATUS, "Unrecognised access token.",
-                errcode=Codes.UNKNOWN_TOKEN
-            )
+            defer.returnValue(None)
+
         # we use ret.get() below because *lots* of unit tests stub out
         # get_user_by_access_token in a way where it only returns a couple of
         # the fields.
diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py
index a82d737e71..1faeb92f1e 100644
--- a/tests/api/test_auth.py
+++ b/tests/api/test_auth.py
@@ -198,8 +198,6 @@ class AuthTestCase(unittest.TestCase):
 
     @defer.inlineCallbacks
     def test_get_user_from_macaroon(self):
-        # TODO(danielwh): Remove this mock when we remove the
-        # get_user_by_access_token fallback.
         self.store.get_user_by_access_token = Mock(
             return_value={
                 "name": "@baldrick:matrix.org",
@@ -228,6 +226,7 @@ class AuthTestCase(unittest.TestCase):
         self.store.get_user_by_id = Mock(return_value={
             "is_guest": True,
         })
+        self.store.get_user_by_access_token = Mock(return_value=None)
 
         user_id = "@baldrick:matrix.org"
         macaroon = pymacaroons.Macaroon(
@@ -248,152 +247,6 @@ class AuthTestCase(unittest.TestCase):
         self.store.get_user_by_id.assert_called_with(user_id)
 
     @defer.inlineCallbacks
-    def test_get_user_from_macaroon_user_db_mismatch(self):
-        self.store.get_user_by_access_token = Mock(
-            return_value={"name": "@percy:matrix.org"}
-        )
-
-        user = "@baldrick:matrix.org"
-        macaroon = pymacaroons.Macaroon(
-            location=self.hs.config.server_name,
-            identifier="key",
-            key=self.hs.config.macaroon_secret_key)
-        macaroon.add_first_party_caveat("gen = 1")
-        macaroon.add_first_party_caveat("type = access")
-        macaroon.add_first_party_caveat("user_id = %s" % (user,))
-        with self.assertRaises(AuthError) as cm:
-            yield self.auth.get_user_by_access_token(macaroon.serialize())
-        self.assertEqual(401, cm.exception.code)
-        self.assertIn("User mismatch", cm.exception.msg)
-
-    @defer.inlineCallbacks
-    def test_get_user_from_macaroon_missing_caveat(self):
-        # TODO(danielwh): Remove this mock when we remove the
-        # get_user_by_access_token fallback.
-        self.store.get_user_by_access_token = Mock(
-            return_value={"name": "@baldrick:matrix.org"}
-        )
-
-        macaroon = pymacaroons.Macaroon(
-            location=self.hs.config.server_name,
-            identifier="key",
-            key=self.hs.config.macaroon_secret_key)
-        macaroon.add_first_party_caveat("gen = 1")
-        macaroon.add_first_party_caveat("type = access")
-
-        with self.assertRaises(AuthError) as cm:
-            yield self.auth.get_user_by_access_token(macaroon.serialize())
-        self.assertEqual(401, cm.exception.code)
-        self.assertIn("No user caveat", cm.exception.msg)
-
-    @defer.inlineCallbacks
-    def test_get_user_from_macaroon_wrong_key(self):
-        # TODO(danielwh): Remove this mock when we remove the
-        # get_user_by_access_token fallback.
-        self.store.get_user_by_access_token = Mock(
-            return_value={"name": "@baldrick:matrix.org"}
-        )
-
-        user = "@baldrick:matrix.org"
-        macaroon = pymacaroons.Macaroon(
-            location=self.hs.config.server_name,
-            identifier="key",
-            key=self.hs.config.macaroon_secret_key + "wrong")
-        macaroon.add_first_party_caveat("gen = 1")
-        macaroon.add_first_party_caveat("type = access")
-        macaroon.add_first_party_caveat("user_id = %s" % (user,))
-
-        with self.assertRaises(AuthError) as cm:
-            yield self.auth.get_user_by_access_token(macaroon.serialize())
-        self.assertEqual(401, cm.exception.code)
-        self.assertIn("Invalid macaroon", cm.exception.msg)
-
-    @defer.inlineCallbacks
-    def test_get_user_from_macaroon_unknown_caveat(self):
-        # TODO(danielwh): Remove this mock when we remove the
-        # get_user_by_access_token fallback.
-        self.store.get_user_by_access_token = Mock(
-            return_value={"name": "@baldrick:matrix.org"}
-        )
-
-        user = "@baldrick:matrix.org"
-        macaroon = pymacaroons.Macaroon(
-            location=self.hs.config.server_name,
-            identifier="key",
-            key=self.hs.config.macaroon_secret_key)
-        macaroon.add_first_party_caveat("gen = 1")
-        macaroon.add_first_party_caveat("type = access")
-        macaroon.add_first_party_caveat("user_id = %s" % (user,))
-        macaroon.add_first_party_caveat("cunning > fox")
-
-        with self.assertRaises(AuthError) as cm:
-            yield self.auth.get_user_by_access_token(macaroon.serialize())
-        self.assertEqual(401, cm.exception.code)
-        self.assertIn("Invalid macaroon", cm.exception.msg)
-
-    @defer.inlineCallbacks
-    def test_get_user_from_macaroon_expired(self):
-        # TODO(danielwh): Remove this mock when we remove the
-        # get_user_by_access_token fallback.
-        self.store.get_user_by_access_token = Mock(
-            return_value={"name": "@baldrick:matrix.org"}
-        )
-
-        self.store.get_user_by_access_token = Mock(
-            return_value={"name": "@baldrick:matrix.org"}
-        )
-
-        user = "@baldrick:matrix.org"
-        macaroon = pymacaroons.Macaroon(
-            location=self.hs.config.server_name,
-            identifier="key",
-            key=self.hs.config.macaroon_secret_key)
-        macaroon.add_first_party_caveat("gen = 1")
-        macaroon.add_first_party_caveat("type = access")
-        macaroon.add_first_party_caveat("user_id = %s" % (user,))
-        macaroon.add_first_party_caveat("time < -2000")  # ms
-
-        self.hs.clock.now = 5000  # seconds
-        self.hs.config.expire_access_token = True
-        # yield self.auth.get_user_by_access_token(macaroon.serialize())
-        # TODO(daniel): Turn on the check that we validate expiration, when we
-        # validate expiration (and remove the above line, which will start
-        # throwing).
-        with self.assertRaises(AuthError) as cm:
-            yield self.auth.get_user_by_access_token(macaroon.serialize())
-        self.assertEqual(401, cm.exception.code)
-        self.assertIn("Invalid macaroon", cm.exception.msg)
-
-    @defer.inlineCallbacks
-    def test_get_user_from_macaroon_with_valid_duration(self):
-        # TODO(danielwh): Remove this mock when we remove the
-        # get_user_by_access_token fallback.
-        self.store.get_user_by_access_token = Mock(
-            return_value={"name": "@baldrick:matrix.org"}
-        )
-
-        self.store.get_user_by_access_token = Mock(
-            return_value={"name": "@baldrick:matrix.org"}
-        )
-
-        user_id = "@baldrick:matrix.org"
-        macaroon = pymacaroons.Macaroon(
-            location=self.hs.config.server_name,
-            identifier="key",
-            key=self.hs.config.macaroon_secret_key)
-        macaroon.add_first_party_caveat("gen = 1")
-        macaroon.add_first_party_caveat("type = access")
-        macaroon.add_first_party_caveat("user_id = %s" % (user_id,))
-        macaroon.add_first_party_caveat("time < 900000000")  # ms
-
-        self.hs.clock.now = 5000  # seconds
-        self.hs.config.expire_access_token = True
-
-        user_info = yield self.auth.get_user_by_access_token(macaroon.serialize())
-        user = user_info["user"]
-        self.assertEqual(UserID.from_string(user_id), user)
-
-    @defer.inlineCallbacks
     def test_cannot_use_regular_token_as_guest(self):
         USER_ID = "@percy:matrix.org"
         self.store.add_access_token_to_user = Mock()