Prevent user tokens being used as guest tokens (#1675)

Make sure that a user cannot pretend to be a guest by adding 'guest = True'
caveats.

3 files changed, 115 insertions, 31 deletions

diff --git a/synapse/api/auth.py b/synapse/api/auth.py
index ddab210718..a99986714d 100644
--- a/synapse/api/auth.py
+++ b/synapse/api/auth.py
@@ -677,31 +677,28 @@ class Auth(object):
 
     @defer.inlineCallbacks
     def get_user_by_access_token(self, token, rights="access"):
-        """ Get a registered user's ID.
+        """ Validate access token and get user_id from it
 
         Args:
             token (str): The access token to get the user by.
+            rights (str): The operation being performed; the access token must
+                allow this.
         Returns:
             dict : dict that includes the user and the ID of their access token.
         Raises:
             AuthError if no user by that token exists or the token is invalid.
         """
         try:
-            ret = yield self.get_user_from_macaroon(token, rights)
-        except AuthError:
-            # TODO(daniel): Remove this fallback when all existing access tokens
-            # have been re-issued as macaroons.
-            if self.hs.config.expire_access_token:
-                raise
-            ret = yield self._look_up_user_by_access_token(token)
-
-        defer.returnValue(ret)
+            macaroon = pymacaroons.Macaroon.deserialize(token)
+        except Exception:  # deserialize can throw more-or-less anything
+            # 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
+            r = yield self._look_up_user_by_access_token(token)
+            defer.returnValue(r)
 
-    @defer.inlineCallbacks
-    def get_user_from_macaroon(self, macaroon_str, rights="access"):
         try:
-            macaroon = pymacaroons.Macaroon.deserialize(macaroon_str)
-
             user_id = self.get_user_id_from_macaroon(macaroon)
             user = UserID.from_string(user_id)
 
@@ -716,6 +713,30 @@ class Auth(object):
                     guest = True
 
             if guest:
+                # Guest access tokens are not stored in the database (there can
+                # only be one access token per guest, anyway).
+                #
+                # In order to prevent guest access tokens being used as regular
+                # user access tokens (and hence getting around the invalidation
+                # process), we look up the user id and check that it is indeed
+                # a guest user.
+                #
+                # It would of course be much easier to store guest access
+                # tokens in the database as well, but that would break existing
+                # guest tokens.
+                stored_user = yield self.store.get_user_by_id(user_id)
+                if not stored_user:
+                    raise AuthError(
+                        self.TOKEN_NOT_FOUND_HTTP_STATUS,
+                        "Unknown user_id %s" % user_id,
+                        errcode=Codes.UNKNOWN_TOKEN
+                    )
+                if not stored_user["is_guest"]:
+                    raise AuthError(
+                        self.TOKEN_NOT_FOUND_HTTP_STATUS,
+                        "Guest access token used for regular user",
+                        errcode=Codes.UNKNOWN_TOKEN
+                    )
                 ret = {
                     "user": user,
                     "is_guest": True,
@@ -743,7 +764,7 @@ class Auth(object):
                 #     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(macaroon_str)
+                ret = yield self._look_up_user_by_access_token(token)
                 if ret["user"] != user:
                     logger.error(
                         "Macaroon user (%s) != DB user (%s)",
diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py
index 886fec8701..286f0cef0a 100644
--- a/synapse/handlers/register.py
+++ b/synapse/handlers/register.py
@@ -81,7 +81,7 @@ class RegistrationHandler(BaseHandler):
                     "User ID already taken.",
                     errcode=Codes.USER_IN_USE,
                 )
-            user_data = yield self.auth.get_user_from_macaroon(guest_access_token)
+            user_data = yield self.auth.get_user_by_access_token(guest_access_token)
             if not user_data["is_guest"] or user_data["user"].localpart != localpart:
                 raise AuthError(
                     403,
diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py
index 2cf262bb46..4575dd9834 100644
--- a/tests/api/test_auth.py
+++ b/tests/api/test_auth.py
@@ -12,17 +12,22 @@
 # 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 tests import unittest
-from twisted.internet import defer
 
+import pymacaroons
 from mock import Mock
+from twisted.internet import defer
 
+import synapse.handlers.auth
 from synapse.api.auth import Auth
 from synapse.api.errors import AuthError
 from synapse.types import UserID
+from tests import unittest
 from tests.utils import setup_test_homeserver, mock_getRawHeaders
 
-import pymacaroons
+
+class TestHandlers(object):
+    def __init__(self, hs):
+        self.auth_handler = synapse.handlers.auth.AuthHandler(hs)
 
 
 class AuthTestCase(unittest.TestCase):
@@ -34,14 +39,17 @@ class AuthTestCase(unittest.TestCase):
 
         self.hs = yield setup_test_homeserver(handlers=None)
         self.hs.get_datastore = Mock(return_value=self.store)
+        self.hs.handlers = TestHandlers(self.hs)
         self.auth = Auth(self.hs)
 
         self.test_user = "@foo:bar"
         self.test_token = "_test_token_"
 
+        # this is overridden for the appservice tests
+        self.store.get_app_service_by_token = Mock(return_value=None)
+
     @defer.inlineCallbacks
     def test_get_user_by_req_user_valid_token(self):
-        self.store.get_app_service_by_token = Mock(return_value=None)
         user_info = {
             "name": self.test_user,
             "token_id": "ditto",
@@ -56,7 +64,6 @@ class AuthTestCase(unittest.TestCase):
         self.assertEquals(requester.user.to_string(), self.test_user)
 
     def test_get_user_by_req_user_bad_token(self):
-        self.store.get_app_service_by_token = Mock(return_value=None)
         self.store.get_user_by_access_token = Mock(return_value=None)
 
         request = Mock(args={})
@@ -66,7 +73,6 @@ class AuthTestCase(unittest.TestCase):
         self.failureResultOf(d, AuthError)
 
     def test_get_user_by_req_user_missing_token(self):
-        self.store.get_app_service_by_token = Mock(return_value=None)
         user_info = {
             "name": self.test_user,
             "token_id": "ditto",
@@ -158,7 +164,7 @@ class AuthTestCase(unittest.TestCase):
         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 = yield self.auth.get_user_from_macaroon(macaroon.serialize())
+        user_info = yield self.auth.get_user_by_access_token(macaroon.serialize())
         user = user_info["user"]
         self.assertEqual(UserID.from_string(user_id), user)
 
@@ -168,6 +174,10 @@ class AuthTestCase(unittest.TestCase):
 
     @defer.inlineCallbacks
     def test_get_guest_user_from_macaroon(self):
+        self.store.get_user_by_id = Mock(return_value={
+            "is_guest": True,
+        })
+
         user_id = "@baldrick:matrix.org"
         macaroon = pymacaroons.Macaroon(
             location=self.hs.config.server_name,
@@ -179,11 +189,12 @@ class AuthTestCase(unittest.TestCase):
         macaroon.add_first_party_caveat("guest = true")
         serialized = macaroon.serialize()
 
-        user_info = yield self.auth.get_user_from_macaroon(serialized)
+        user_info = yield self.auth.get_user_by_access_token(serialized)
         user = user_info["user"]
         is_guest = user_info["is_guest"]
         self.assertEqual(UserID.from_string(user_id), user)
         self.assertTrue(is_guest)
+        self.store.get_user_by_id.assert_called_with(user_id)
 
     @defer.inlineCallbacks
     def test_get_user_from_macaroon_user_db_mismatch(self):
@@ -200,7 +211,7 @@ class AuthTestCase(unittest.TestCase):
         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_from_macaroon(macaroon.serialize())
+            yield self.auth.get_user_by_access_token(macaroon.serialize())
         self.assertEqual(401, cm.exception.code)
         self.assertIn("User mismatch", cm.exception.msg)
 
@@ -220,7 +231,7 @@ class AuthTestCase(unittest.TestCase):
         macaroon.add_first_party_caveat("type = access")
 
         with self.assertRaises(AuthError) as cm:
-            yield self.auth.get_user_from_macaroon(macaroon.serialize())
+            yield self.auth.get_user_by_access_token(macaroon.serialize())
         self.assertEqual(401, cm.exception.code)
         self.assertIn("No user caveat", cm.exception.msg)
 
@@ -242,7 +253,7 @@ class AuthTestCase(unittest.TestCase):
         macaroon.add_first_party_caveat("user_id = %s" % (user,))
 
         with self.assertRaises(AuthError) as cm:
-            yield self.auth.get_user_from_macaroon(macaroon.serialize())
+            yield self.auth.get_user_by_access_token(macaroon.serialize())
         self.assertEqual(401, cm.exception.code)
         self.assertIn("Invalid macaroon", cm.exception.msg)
 
@@ -265,7 +276,7 @@ class AuthTestCase(unittest.TestCase):
         macaroon.add_first_party_caveat("cunning > fox")
 
         with self.assertRaises(AuthError) as cm:
-            yield self.auth.get_user_from_macaroon(macaroon.serialize())
+            yield self.auth.get_user_by_access_token(macaroon.serialize())
         self.assertEqual(401, cm.exception.code)
         self.assertIn("Invalid macaroon", cm.exception.msg)
 
@@ -293,12 +304,12 @@ class AuthTestCase(unittest.TestCase):
 
         self.hs.clock.now = 5000  # seconds
         self.hs.config.expire_access_token = True
-        # yield self.auth.get_user_from_macaroon(macaroon.serialize())
+        # 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_from_macaroon(macaroon.serialize())
+            yield self.auth.get_user_by_access_token(macaroon.serialize())
         self.assertEqual(401, cm.exception.code)
         self.assertIn("Invalid macaroon", cm.exception.msg)
 
@@ -327,6 +338,58 @@ class AuthTestCase(unittest.TestCase):
         self.hs.clock.now = 5000  # seconds
         self.hs.config.expire_access_token = True
 
-        user_info = yield self.auth.get_user_from_macaroon(macaroon.serialize())
+        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()
+
+        token = yield self.hs.handlers.auth_handler.issue_access_token(
+            USER_ID, "DEVICE"
+        )
+        self.store.add_access_token_to_user.assert_called_with(
+            USER_ID, token, "DEVICE"
+        )
+
+        def get_user(tok):
+            if token != tok:
+                return None
+            return {
+                "name": USER_ID,
+                "is_guest": False,
+                "token_id": 1234,
+                "device_id": "DEVICE",
+            }
+        self.store.get_user_by_access_token = get_user
+        self.store.get_user_by_id = Mock(return_value={
+            "is_guest": False,
+        })
+
+        # check the token works
+        request = Mock(args={})
+        request.args["access_token"] = [token]
+        request.requestHeaders.getRawHeaders = mock_getRawHeaders()
+        requester = yield self.auth.get_user_by_req(request, allow_guest=True)
+        self.assertEqual(UserID.from_string(USER_ID), requester.user)
+        self.assertFalse(requester.is_guest)
+
+        # add an is_guest caveat
+        mac = pymacaroons.Macaroon.deserialize(token)
+        mac.add_first_party_caveat("guest = true")
+        guest_tok = mac.serialize()
+
+        # the token should *not* work now
+        request = Mock(args={})
+        request.args["access_token"] = [guest_tok]
+        request.requestHeaders.getRawHeaders = mock_getRawHeaders()
+
+        with self.assertRaises(AuthError) as cm:
+            yield self.auth.get_user_by_req(request, allow_guest=True)
+
+        self.assertEqual(401, cm.exception.code)
+        self.assertEqual("Guest access token used for regular user", cm.exception.msg)
+
+        self.store.get_user_by_id.assert_called_with(USER_ID)