From 6ac7e501ead40d7fe3d9c7624730d98da9e5db0e Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Wed, 10 Jul 2019 15:30:56 +0100
Subject: [PATCH 1/7] Add a mechanism for per-test configs

It's useful to be able to tweak the homeserver config to be used for each
test. This PR adds a mechanism to do so.
---
 changelog.d/5657.misc |  1 +
 tests/unittest.py     | 30 +++++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100644 changelog.d/5657.misc

diff --git a/changelog.d/5657.misc b/changelog.d/5657.misc
new file mode 100644
index 000000000000..bdec9ae4c0b6
--- /dev/null
+++ b/changelog.d/5657.misc
@@ -0,0 +1 @@
+Add a mechanism for per-test homeserver configuration in the unit tests.
diff --git a/tests/unittest.py b/tests/unittest.py
index a09e76c7c21d..67a1822d485b 100644
--- a/tests/unittest.py
+++ b/tests/unittest.py
@@ -157,6 +157,20 @@ class HomeserverTestCase(TestCase):
     """
     A base TestCase that reduces boilerplate for HomeServer-using test cases.
 
+    Defines a setUp method which creates a mock reactor, and instantiates a homeserver
+    running on that reactor.
+
+    There are various hooks for modifying the way that the homeserver is instantiated:
+
+    * override make_homeserver, for example by making it pass different parameters into
+      setup_test_homeserver.
+
+    * override default_config, to return a modified configuration dictionary for use
+      by setup_test_homeserver.
+
+    * On a per-test basis, you can attach an 'extra_config' attribute, with a dictionary
+      containing additional configuration settings to be added to the basic config dict.
+
     Attributes:
         servlets (list[function]): List of servlet registration function.
         user_id (str): The user ID to assume if auth is hijacked.
@@ -168,6 +182,13 @@ class HomeserverTestCase(TestCase):
     hijack_auth = True
     needs_threadpool = False
 
+    def __init__(self, methodName, *args, **kwargs):
+        super().__init__(methodName, *args, **kwargs)
+
+        # see if we have a custom config method for this test
+        method = getattr(self, methodName)
+        self._extra_config = getattr(method, "extra_config", None)
+
     def setUp(self):
         """
         Set up the TestCase by calling the homeserver constructor, optionally
@@ -276,7 +297,14 @@ def default_config(self, name="test"):
         Args:
             name (str): The homeserver name/domain.
         """
-        return default_config(name)
+        config = default_config(name)
+
+        # apply any additional config which was specified as an extra_config attribute
+        # on the test method..
+        if self._extra_config is not None:
+            config.update(self._extra_config)
+
+        return config
 
     def prepare(self, reactor, clock, homeserver):
         """

From c510a028f60a5039f40d5f164cdd70bf35e537af Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Wed, 10 Jul 2019 15:32:26 +0100
Subject: [PATCH 2/7] Implement access token expiry

Record how long an access token is valid for, and raise a soft-logout once it
expires.
---
 changelog.d/5660.feature                      |  1 +
 docs/sample_config.yaml                       | 11 +++++
 synapse/api/auth.py                           | 12 +++++
 synapse/api/errors.py                         |  8 +++-
 synapse/config/registration.py                | 16 +++++++
 synapse/handlers/auth.py                      | 17 +++++--
 synapse/handlers/register.py                  | 35 ++++++++++-----
 synapse/storage/registration.py               | 19 +++++---
 .../schema/delta/55/access_token_expiry.sql   | 18 ++++++++
 tests/api/test_auth.py                        |  6 ++-
 tests/handlers/test_auth.py                   | 20 ++++++---
 tests/handlers/test_register.py               |  5 ++-
 tests/rest/client/v1/test_login.py            | 45 +++++++++++++++++++
 tests/storage/test_registration.py            |  8 ++--
 14 files changed, 190 insertions(+), 31 deletions(-)
 create mode 100644 changelog.d/5660.feature
 create mode 100644 synapse/storage/schema/delta/55/access_token_expiry.sql

diff --git a/changelog.d/5660.feature b/changelog.d/5660.feature
new file mode 100644
index 000000000000..82889fdaf16b
--- /dev/null
+++ b/changelog.d/5660.feature
@@ -0,0 +1 @@
+Implement `session_lifetime` configuration option, after which access tokens will expire.
diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index 0462f0a17a2b..663ff31622f4 100644
--- a/docs/sample_config.yaml
+++ b/docs/sample_config.yaml
@@ -786,6 +786,17 @@ uploads_path: "DATADIR/uploads"
 #  renew_at: 1w
 #  renew_email_subject: "Renew your %(app)s account"
 
+# Time that a user's session remains valid for, after they log in.
+#
+# Note that this is not currently compatible with guest logins.
+#
+# Note also that this is calculated at login time: changes are not applied
+# retrospectively to users who have already logged in.
+#
+# By default, this is infinite.
+#
+#session_lifetime: 24h
+
 # The user must provide all of the below types of 3PID when registering.
 #
 #registrations_require_3pid:
diff --git a/synapse/api/auth.py b/synapse/api/auth.py
index afc6400948b7..d9e943c39c83 100644
--- a/synapse/api/auth.py
+++ b/synapse/api/auth.py
@@ -319,6 +319,17 @@ def get_user_by_access_token(self, token, rights="access"):
             # first look in the database
             r = yield self._look_up_user_by_access_token(token)
             if r:
+                valid_until_ms = r["valid_until_ms"]
+                if (
+                    valid_until_ms is not None
+                    and valid_until_ms < self.clock.time_msec()
+                ):
+                    # there was a valid access token, but it has expired.
+                    # soft-logout the user.
+                    raise InvalidClientTokenError(
+                        msg="Access token has expired", soft_logout=True
+                    )
+
                 defer.returnValue(r)
 
         # otherwise it needs to be a valid macaroon
@@ -505,6 +516,7 @@ def _look_up_user_by_access_token(self, token):
             "token_id": ret.get("token_id", None),
             "is_guest": False,
             "device_id": ret.get("device_id"),
+            "valid_until_ms": ret.get("valid_until_ms"),
         }
         defer.returnValue(user_info)
 
diff --git a/synapse/api/errors.py b/synapse/api/errors.py
index 41fd04cd548f..a6e753c30c26 100644
--- a/synapse/api/errors.py
+++ b/synapse/api/errors.py
@@ -245,8 +245,14 @@ def __init__(self, msg="Missing access token"):
 class InvalidClientTokenError(InvalidClientCredentialsError):
     """Raised when we didn't understand the access token in a request"""
 
-    def __init__(self, msg="Unrecognised access token"):
+    def __init__(self, msg="Unrecognised access token", soft_logout=False):
         super().__init__(msg=msg, errcode="M_UNKNOWN_TOKEN")
+        self._soft_logout = soft_logout
+
+    def error_dict(self):
+        d = super().error_dict()
+        d["soft_logout"] = self._soft_logout
+        return d
 
 
 class ResourceLimitError(SynapseError):
diff --git a/synapse/config/registration.py b/synapse/config/registration.py
index b895c4e9f4a9..34cb11468cdb 100644
--- a/synapse/config/registration.py
+++ b/synapse/config/registration.py
@@ -84,6 +84,11 @@ def read_config(self, config, **kwargs):
             "disable_msisdn_registration", False
         )
 
+        session_lifetime = config.get("session_lifetime")
+        if session_lifetime is not None:
+            session_lifetime = self.parse_duration(session_lifetime)
+        self.session_lifetime = session_lifetime
+
     def generate_config_section(self, generate_secrets=False, **kwargs):
         if generate_secrets:
             registration_shared_secret = 'registration_shared_secret: "%s"' % (
@@ -141,6 +146,17 @@ def generate_config_section(self, generate_secrets=False, **kwargs):
         #  renew_at: 1w
         #  renew_email_subject: "Renew your %%(app)s account"
 
+        # Time that a user's session remains valid for, after they log in.
+        #
+        # Note that this is not currently compatible with guest logins.
+        #
+        # Note also that this is calculated at login time: changes are not applied
+        # retrospectively to users who have already logged in.
+        #
+        # By default, this is infinite.
+        #
+        #session_lifetime: 24h
+
         # The user must provide all of the below types of 3PID when registering.
         #
         #registrations_require_3pid:
diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index da312b188e64..b74a6e9c6225 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -15,6 +15,7 @@
 # limitations under the License.
 
 import logging
+import time
 import unicodedata
 
 import attr
@@ -558,7 +559,7 @@ def _get_session_info(self, session_id):
         return self.sessions[session_id]
 
     @defer.inlineCallbacks
-    def get_access_token_for_user_id(self, user_id, device_id=None):
+    def get_access_token_for_user_id(self, user_id, device_id, valid_until_ms):
         """
         Creates a new access token for the user with the given user ID.
 
@@ -572,16 +573,26 @@ def get_access_token_for_user_id(self, user_id, device_id=None):
             device_id (str|None): the device ID to associate with the tokens.
                None to leave the tokens unassociated with a device (deprecated:
                we should always have a device ID)
+            valid_until_ms (int|None): when the token is valid until. None for
+                no expiry.
         Returns:
               The access token for the user's session.
         Raises:
             StoreError if there was a problem storing the token.
         """
-        logger.info("Logging in user %s on device %s", user_id, device_id)
+        fmt_expiry = ""
+        if valid_until_ms is not None:
+            fmt_expiry = time.strftime(
+                " until %Y-%m-%d %H:%M:%S", time.localtime(valid_until_ms / 1000.0)
+            )
+        logger.info("Logging in user %s on device %s%s", user_id, device_id, fmt_expiry)
+
         yield self.auth.check_auth_blocking(user_id)
 
         access_token = self.macaroon_gen.generate_access_token(user_id)
-        yield self.store.add_access_token_to_user(user_id, access_token, device_id)
+        yield self.store.add_access_token_to_user(
+            user_id, access_token, device_id, valid_until_ms
+        )
 
         # the device *should* have been registered before we got here; however,
         # it's possible we raced against a DELETE operation. The thing we
diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py
index 420c5cb5bce7..bb7cfd71b91d 100644
--- a/synapse/handlers/register.py
+++ b/synapse/handlers/register.py
@@ -84,6 +84,8 @@ def __init__(self, hs):
             self.device_handler = hs.get_device_handler()
             self.pusher_pool = hs.get_pusherpool()
 
+        self.session_lifetime = hs.config.session_lifetime
+
     @defer.inlineCallbacks
     def check_username(self, localpart, guest_access_token=None, assigned_user_id=None):
         if types.contains_invalid_mxid_characters(localpart):
@@ -599,6 +601,8 @@ def register_with_store(
     def register_device(self, user_id, device_id, initial_display_name, is_guest=False):
         """Register a device for a user and generate an access token.
 
+        The access token will be limited by the homeserver's session_lifetime config.
+
         Args:
             user_id (str): full canonical @user:id
             device_id (str|None): The device ID to check, or None to generate
@@ -619,20 +623,29 @@ def register_device(self, user_id, device_id, initial_display_name, is_guest=Fal
                 is_guest=is_guest,
             )
             defer.returnValue((r["device_id"], r["access_token"]))
-        else:
-            device_id = yield self.device_handler.check_device_registered(
-                user_id, device_id, initial_display_name
-            )
+
+        valid_until_ms = None
+        if self.session_lifetime is not None:
             if is_guest:
-                access_token = self.macaroon_gen.generate_access_token(
-                    user_id, ["guest = true"]
-                )
-            else:
-                access_token = yield self._auth_handler.get_access_token_for_user_id(
-                    user_id, device_id=device_id
+                raise Exception(
+                    "session_lifetime is not currently implemented for guest access"
                 )
+            valid_until_ms = self.clock.time_msec() + self.session_lifetime
+
+        device_id = yield self.device_handler.check_device_registered(
+            user_id, device_id, initial_display_name
+        )
+        if is_guest:
+            assert valid_until_ms is None
+            access_token = self.macaroon_gen.generate_access_token(
+                user_id, ["guest = true"]
+            )
+        else:
+            access_token = yield self._auth_handler.get_access_token_for_user_id(
+                user_id, device_id=device_id, valid_until_ms=valid_until_ms
+            )
 
-            defer.returnValue((device_id, access_token))
+        defer.returnValue((device_id, access_token))
 
     @defer.inlineCallbacks
     def post_registration_actions(
diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py
index 8e217c94089e..abbe1afce218 100644
--- a/synapse/storage/registration.py
+++ b/synapse/storage/registration.py
@@ -90,7 +90,8 @@ def get_user_by_access_token(self, token):
             token (str): The access token of a user.
         Returns:
             defer.Deferred: None, if the token did not match, otherwise dict
-                including the keys `name`, `is_guest`, `device_id`, `token_id`.
+                including the keys `name`, `is_guest`, `device_id`, `token_id`,
+                `valid_until_ms`.
         """
         return self.runInteraction(
             "get_user_by_access_token", self._query_for_auth, token
@@ -284,7 +285,7 @@ def is_server_admin(self, user):
     def _query_for_auth(self, txn, token):
         sql = (
             "SELECT users.name, users.is_guest, access_tokens.id as token_id,"
-            " access_tokens.device_id"
+            " access_tokens.device_id, access_tokens.valid_until_ms"
             " FROM users"
             " INNER JOIN access_tokens on users.name = access_tokens.user_id"
             " WHERE token = ?"
@@ -679,14 +680,16 @@ def _backgroud_update_set_deactivated_flag_txn(txn):
         defer.returnValue(batch_size)
 
     @defer.inlineCallbacks
-    def add_access_token_to_user(self, user_id, token, device_id=None):
+    def add_access_token_to_user(self, user_id, token, device_id, valid_until_ms):
         """Adds an access token for the given user.
 
         Args:
             user_id (str): The user ID.
             token (str): The new access token to add.
             device_id (str): ID of the device to associate with the access
-               token
+                token
+            valid_until_ms (int|None): when the token is valid until. None for
+                no expiry.
         Raises:
             StoreError if there was a problem adding this.
         """
@@ -694,7 +697,13 @@ def add_access_token_to_user(self, user_id, token, device_id=None):
 
         yield self._simple_insert(
             "access_tokens",
-            {"id": next_id, "user_id": user_id, "token": token, "device_id": device_id},
+            {
+                "id": next_id,
+                "user_id": user_id,
+                "token": token,
+                "device_id": device_id,
+                "valid_until_ms": valid_until_ms,
+            },
             desc="add_access_token_to_user",
         )
 
diff --git a/synapse/storage/schema/delta/55/access_token_expiry.sql b/synapse/storage/schema/delta/55/access_token_expiry.sql
new file mode 100644
index 000000000000..4590604bfd8a
--- /dev/null
+++ b/synapse/storage/schema/delta/55/access_token_expiry.sql
@@ -0,0 +1,18 @@
+/* Copyright 2019 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.
+ */
+
+-- when this access token can be used until, in ms since the epoch. NULL means the token
+-- never expires.
+ALTER TABLE access_tokens ADD COLUMN valid_until_ms BIGINT;
diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py
index ee92ceeb60ef..c0cb8ef296ff 100644
--- a/tests/api/test_auth.py
+++ b/tests/api/test_auth.py
@@ -262,9 +262,11 @@ def test_cannot_use_regular_token_as_guest(self):
         self.store.add_access_token_to_user = Mock()
 
         token = yield self.hs.handlers.auth_handler.get_access_token_for_user_id(
-            USER_ID, "DEVICE"
+            USER_ID, "DEVICE", valid_until_ms=None
+        )
+        self.store.add_access_token_to_user.assert_called_with(
+            USER_ID, token, "DEVICE", None
         )
-        self.store.add_access_token_to_user.assert_called_with(USER_ID, token, "DEVICE")
 
         def get_user(tok):
             if token != tok:
diff --git a/tests/handlers/test_auth.py b/tests/handlers/test_auth.py
index b204a0700d25..b03103d96ff5 100644
--- a/tests/handlers/test_auth.py
+++ b/tests/handlers/test_auth.py
@@ -117,7 +117,9 @@ def test_short_term_login_token_cannot_replace_user_id(self):
     def test_mau_limits_disabled(self):
         self.hs.config.limit_usage_by_mau = False
         # Ensure does not throw exception
-        yield self.auth_handler.get_access_token_for_user_id("user_a")
+        yield self.auth_handler.get_access_token_for_user_id(
+            "user_a", device_id=None, valid_until_ms=None
+        )
 
         yield self.auth_handler.validate_short_term_login_token_and_get_user_id(
             self._get_macaroon().serialize()
@@ -131,7 +133,9 @@ def test_mau_limits_exceeded_large(self):
         )
 
         with self.assertRaises(ResourceLimitError):
-            yield self.auth_handler.get_access_token_for_user_id("user_a")
+            yield self.auth_handler.get_access_token_for_user_id(
+                "user_a", device_id=None, valid_until_ms=None
+            )
 
         self.hs.get_datastore().get_monthly_active_count = Mock(
             return_value=defer.succeed(self.large_number_of_users)
@@ -150,7 +154,9 @@ def test_mau_limits_parity(self):
             return_value=defer.succeed(self.hs.config.max_mau_value)
         )
         with self.assertRaises(ResourceLimitError):
-            yield self.auth_handler.get_access_token_for_user_id("user_a")
+            yield self.auth_handler.get_access_token_for_user_id(
+                "user_a", device_id=None, valid_until_ms=None
+            )
 
         self.hs.get_datastore().get_monthly_active_count = Mock(
             return_value=defer.succeed(self.hs.config.max_mau_value)
@@ -166,7 +172,9 @@ def test_mau_limits_parity(self):
         self.hs.get_datastore().get_monthly_active_count = Mock(
             return_value=defer.succeed(self.hs.config.max_mau_value)
         )
-        yield self.auth_handler.get_access_token_for_user_id("user_a")
+        yield self.auth_handler.get_access_token_for_user_id(
+            "user_a", device_id=None, valid_until_ms=None
+        )
         self.hs.get_datastore().user_last_seen_monthly_active = Mock(
             return_value=defer.succeed(self.hs.get_clock().time_msec())
         )
@@ -185,7 +193,9 @@ def test_mau_limits_not_exceeded(self):
             return_value=defer.succeed(self.small_number_of_users)
         )
         # Ensure does not raise exception
-        yield self.auth_handler.get_access_token_for_user_id("user_a")
+        yield self.auth_handler.get_access_token_for_user_id(
+            "user_a", device_id=None, valid_until_ms=None
+        )
 
         self.hs.get_datastore().get_monthly_active_count = Mock(
             return_value=defer.succeed(self.small_number_of_users)
diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py
index 1b7e1dacee0e..90d012937404 100644
--- a/tests/handlers/test_register.py
+++ b/tests/handlers/test_register.py
@@ -272,7 +272,10 @@ def get_or_create_user(self, requester, localpart, displayname, password_hash=No
             )
         else:
             yield self.hs.get_auth_handler().delete_access_tokens_for_user(user_id)
-        yield self.store.add_access_token_to_user(user_id=user_id, token=token)
+
+        yield self.store.add_access_token_to_user(
+            user_id=user_id, token=token, device_id=None, valid_until_ms=None
+        )
 
         if displayname is not None:
             # logger.info("setting user display name: %s -> %s", user_id, displayname)
diff --git a/tests/rest/client/v1/test_login.py b/tests/rest/client/v1/test_login.py
index 0397f91a9e69..d3d2b2e521ee 100644
--- a/tests/rest/client/v1/test_login.py
+++ b/tests/rest/client/v1/test_login.py
@@ -2,10 +2,12 @@
 
 import synapse.rest.admin
 from synapse.rest.client.v1 import login
+from synapse.rest.client.v2_alpha.account import WhoamiRestServlet
 
 from tests import unittest
 
 LOGIN_URL = b"/_matrix/client/r0/login"
+TEST_URL = b"/_matrix/client/r0/account/whoami"
 
 
 class LoginRestServletTestCase(unittest.HomeserverTestCase):
@@ -13,6 +15,7 @@ class LoginRestServletTestCase(unittest.HomeserverTestCase):
     servlets = [
         synapse.rest.admin.register_servlets_for_client_rest_resource,
         login.register_servlets,
+        lambda hs, http_server: WhoamiRestServlet(hs).register(http_server),
     ]
 
     def make_homeserver(self, reactor, clock):
@@ -144,3 +147,45 @@ def test_POST_ratelimiting_per_account_failed_attempts(self):
         self.render(request)
 
         self.assertEquals(channel.result["code"], b"403", channel.result)
+
+    def test_soft_logout(self):
+        self.register_user("kermit", "monkey")
+
+        # we shouldn't be able to make requests without an access token
+        request, channel = self.make_request(b"GET", TEST_URL)
+        self.render(request)
+        self.assertEquals(channel.result["code"], b"401", channel.result)
+        self.assertEquals(channel.json_body["errcode"], "M_MISSING_TOKEN")
+
+        # log in as normal
+        params = {
+            "type": "m.login.password",
+            "identifier": {"type": "m.id.user", "user": "kermit"},
+            "password": "monkey",
+        }
+        request, channel = self.make_request(b"POST", LOGIN_URL, params)
+        self.render(request)
+
+        self.assertEquals(channel.result["code"], b"200", channel.result)
+
+        # we should now be able to make requests with the access token
+        access_token = channel.json_body["access_token"]
+        request, channel = self.make_request(
+            b"GET", TEST_URL, access_token=access_token
+        )
+        self.render(request)
+        self.assertEquals(channel.result["code"], b"200", channel.result)
+
+        # time passes
+        self.reactor.advance(24 * 3600 * 1000)
+
+        # ... and we should be soft-logouted
+        request, channel = self.make_request(
+            b"GET", TEST_URL, access_token=access_token
+        )
+        self.render(request)
+        self.assertEquals(channel.result["code"], b"401", channel.result)
+        self.assertEquals(channel.json_body["errcode"], "M_UNKNOWN_TOKEN")
+        self.assertEquals(channel.json_body["soft_logout"], True)
+
+    test_soft_logout.extra_config = {"session_lifetime": "24h"}
diff --git a/tests/storage/test_registration.py b/tests/storage/test_registration.py
index 9365c4622d5c..0253c4ac05b4 100644
--- a/tests/storage/test_registration.py
+++ b/tests/storage/test_registration.py
@@ -57,7 +57,7 @@ def test_register(self):
     def test_add_tokens(self):
         yield self.store.register_user(self.user_id, self.pwhash)
         yield self.store.add_access_token_to_user(
-            self.user_id, self.tokens[1], self.device_id
+            self.user_id, self.tokens[1], self.device_id, valid_until_ms=None
         )
 
         result = yield self.store.get_user_by_access_token(self.tokens[1])
@@ -72,9 +72,11 @@ def test_add_tokens(self):
     def test_user_delete_access_tokens(self):
         # add some tokens
         yield self.store.register_user(self.user_id, self.pwhash)
-        yield self.store.add_access_token_to_user(self.user_id, self.tokens[0])
         yield self.store.add_access_token_to_user(
-            self.user_id, self.tokens[1], self.device_id
+            self.user_id, self.tokens[0], device_id=None, valid_until_ms=None
+        )
+        yield self.store.add_access_token_to_user(
+            self.user_id, self.tokens[1], self.device_id, valid_until_ms=None
         )
 
         # now delete some

From 545a9e9ff831bab34ad3f9e8f56ddbd9f172405e Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Thu, 11 Jul 2019 12:27:28 +0100
Subject: [PATCH 3/7] More testing

---
 tests/rest/client/v1/test_login.py | 73 ++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 5 deletions(-)

diff --git a/tests/rest/client/v1/test_login.py b/tests/rest/client/v1/test_login.py
index d3d2b2e521ee..cb46ca4f88f7 100644
--- a/tests/rest/client/v1/test_login.py
+++ b/tests/rest/client/v1/test_login.py
@@ -2,6 +2,7 @@
 
 import synapse.rest.admin
 from synapse.rest.client.v1 import login
+from synapse.rest.client.v2_alpha import devices
 from synapse.rest.client.v2_alpha.account import WhoamiRestServlet
 
 from tests import unittest
@@ -15,6 +16,7 @@ class LoginRestServletTestCase(unittest.HomeserverTestCase):
     servlets = [
         synapse.rest.admin.register_servlets_for_client_rest_resource,
         login.register_servlets,
+        devices.register_servlets,
         lambda hs, http_server: WhoamiRestServlet(hs).register(http_server),
     ]
 
@@ -166,26 +168,87 @@ def test_soft_logout(self):
         request, channel = self.make_request(b"POST", LOGIN_URL, params)
         self.render(request)
 
-        self.assertEquals(channel.result["code"], b"200", channel.result)
+        self.assertEquals(channel.code, 200, channel.result)
+        access_token = channel.json_body["access_token"]
+        device_id = channel.json_body["device_id"]
 
         # we should now be able to make requests with the access token
-        access_token = channel.json_body["access_token"]
         request, channel = self.make_request(
             b"GET", TEST_URL, access_token=access_token
         )
         self.render(request)
-        self.assertEquals(channel.result["code"], b"200", channel.result)
+        self.assertEquals(channel.code, 200, channel.result)
 
         # time passes
-        self.reactor.advance(24 * 3600 * 1000)
+        self.reactor.advance(24 * 3600)
 
         # ... and we should be soft-logouted
         request, channel = self.make_request(
             b"GET", TEST_URL, access_token=access_token
         )
         self.render(request)
-        self.assertEquals(channel.result["code"], b"401", channel.result)
+        self.assertEquals(channel.code, 401, channel.result)
         self.assertEquals(channel.json_body["errcode"], "M_UNKNOWN_TOKEN")
         self.assertEquals(channel.json_body["soft_logout"], True)
 
+        #
+        # test behaviour after deleting the expired device
+        #
+
+        # we now log in as a different device
+        access_token_2 = self.login("kermit", "monkey")
+
+        # more requests with the expired token should still return a soft-logout
+        self.reactor.advance(3600)
+        request, channel = self.make_request(
+            b"GET", TEST_URL, access_token=access_token
+        )
+        self.render(request)
+        self.assertEquals(channel.code, 401, channel.result)
+        self.assertEquals(channel.json_body["errcode"], "M_UNKNOWN_TOKEN")
+        self.assertEquals(channel.json_body["soft_logout"], True)
+
+        # ... but if we delete that device, it will be a proper logout
+        self._delete_device(access_token_2, "kermit", "monkey", device_id)
+
+        request, channel = self.make_request(
+            b"GET", TEST_URL, access_token=access_token
+        )
+        self.render(request)
+        self.assertEquals(channel.code, 401, channel.result)
+        self.assertEquals(channel.json_body["errcode"], "M_UNKNOWN_TOKEN")
+        self.assertEquals(channel.json_body["soft_logout"], False)
+
     test_soft_logout.extra_config = {"session_lifetime": "24h"}
+
+    def _delete_device(self, access_token, user_id, password, device_id):
+        """Perform the UI-Auth to delete a device"""
+        request, channel = self.make_request(
+            b"DELETE", "devices/" + device_id, access_token=access_token
+        )
+        self.render(request)
+        self.assertEquals(channel.code, 401, channel.result)
+        # check it's a UI-Auth fail
+        self.assertEqual(
+            set(channel.json_body.keys()),
+            {"flows", "params", "session"},
+            channel.result,
+        )
+
+        auth = {
+            "type": "m.login.password",
+            # https://github.com/matrix-org/synapse/issues/5665
+            # "identifier": {"type": "m.id.user", "user": user_id},
+            "user": user_id,
+            "password": password,
+            "session": channel.json_body["session"],
+        }
+
+        request, channel = self.make_request(
+            b"DELETE",
+            "devices/" + device_id,
+            access_token=access_token,
+            content={"auth": auth},
+        )
+        self.render(request)
+        self.assertEquals(channel.code, 200, channel.result)

From 7880375f44cad96f76cabb96245f0b6ab4ffe32c Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Thu, 11 Jul 2019 13:56:57 +0100
Subject: [PATCH 4/7] use a decorator for custom config

---
 tests/unittest.py | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/tests/unittest.py b/tests/unittest.py
index 67a1822d485b..0f0c2ad69d03 100644
--- a/tests/unittest.py
+++ b/tests/unittest.py
@@ -168,8 +168,9 @@ class HomeserverTestCase(TestCase):
     * override default_config, to return a modified configuration dictionary for use
       by setup_test_homeserver.
 
-    * On a per-test basis, you can attach an 'extra_config' attribute, with a dictionary
-      containing additional configuration settings to be added to the basic config dict.
+    * On a per-test basis, you can use the @override_config decorator to give a
+      dictionary containing additional configuration settings to be added to the basic
+      config dict.
 
     Attributes:
         servlets (list[function]): List of servlet registration function.
@@ -185,9 +186,9 @@ class HomeserverTestCase(TestCase):
     def __init__(self, methodName, *args, **kwargs):
         super().__init__(methodName, *args, **kwargs)
 
-        # see if we have a custom config method for this test
+        # see if we have any additional config for this test
         method = getattr(self, methodName)
-        self._extra_config = getattr(method, "extra_config", None)
+        self._extra_config = getattr(method, "_extra_config", None)
 
     def setUp(self):
         """
@@ -299,8 +300,8 @@ def default_config(self, name="test"):
         """
         config = default_config(name)
 
-        # apply any additional config which was specified as an extra_config attribute
-        # on the test method..
+        # apply any additional config which was specified via the override_config
+        # decorator.
         if self._extra_config is not None:
             config.update(self._extra_config)
 
@@ -562,3 +563,27 @@ def attempt_wrong_password_login(self, username, password):
         )
         self.render(request)
         self.assertEqual(channel.code, 403, channel.result)
+
+
+def override_config(extra_config):
+    """A decorator which can be applied to test functions to give additional HS config
+
+    For use
+
+    For example:
+
+        class MyTestCase(HomeserverTestCase):
+            @override_config({"enable_registration": False, ...})
+            def test_foo(self):
+                ...
+
+    Args:
+        extra_config(dict): Additional config settings to be merged into the default
+            config dict before instantiating the test homeserver.
+    """
+
+    def decorator(func):
+        func._extra_config = extra_config
+        return func
+
+    return decorator

From e327d3253843b158af0b653a8abd5d7f684c23cf Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Thu, 11 Jul 2019 13:57:30 +0100
Subject: [PATCH 5/7] use @override_config

---
 tests/rest/client/v1/test_login.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/rest/client/v1/test_login.py b/tests/rest/client/v1/test_login.py
index cb46ca4f88f7..eae5411325fc 100644
--- a/tests/rest/client/v1/test_login.py
+++ b/tests/rest/client/v1/test_login.py
@@ -6,6 +6,7 @@
 from synapse.rest.client.v2_alpha.account import WhoamiRestServlet
 
 from tests import unittest
+from tests.unittest import override_config
 
 LOGIN_URL = b"/_matrix/client/r0/login"
 TEST_URL = b"/_matrix/client/r0/account/whoami"
@@ -150,6 +151,7 @@ def test_POST_ratelimiting_per_account_failed_attempts(self):
 
         self.assertEquals(channel.result["code"], b"403", channel.result)
 
+    @override_config({"session_lifetime": "24h"})
     def test_soft_logout(self):
         self.register_user("kermit", "monkey")
 
@@ -219,8 +221,6 @@ def test_soft_logout(self):
         self.assertEquals(channel.json_body["errcode"], "M_UNKNOWN_TOKEN")
         self.assertEquals(channel.json_body["soft_logout"], False)
 
-    test_soft_logout.extra_config = {"session_lifetime": "24h"}
-
     def _delete_device(self, access_token, user_id, password, device_id):
         """Perform the UI-Auth to delete a device"""
         request, channel = self.make_request(

From 427a9a72d71442f883c9c0fa18c1a112d94fbcb0 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Fri, 12 Jul 2019 12:40:10 +0100
Subject: [PATCH 6/7] Revert "use a decorator for custom config"

This reverts commit 7880375f44cad96f76cabb96245f0b6ab4ffe32c.
---
 tests/unittest.py | 37 ++++++-------------------------------
 1 file changed, 6 insertions(+), 31 deletions(-)

diff --git a/tests/unittest.py b/tests/unittest.py
index 0f0c2ad69d03..67a1822d485b 100644
--- a/tests/unittest.py
+++ b/tests/unittest.py
@@ -168,9 +168,8 @@ class HomeserverTestCase(TestCase):
     * override default_config, to return a modified configuration dictionary for use
       by setup_test_homeserver.
 
-    * On a per-test basis, you can use the @override_config decorator to give a
-      dictionary containing additional configuration settings to be added to the basic
-      config dict.
+    * On a per-test basis, you can attach an 'extra_config' attribute, with a dictionary
+      containing additional configuration settings to be added to the basic config dict.
 
     Attributes:
         servlets (list[function]): List of servlet registration function.
@@ -186,9 +185,9 @@ class HomeserverTestCase(TestCase):
     def __init__(self, methodName, *args, **kwargs):
         super().__init__(methodName, *args, **kwargs)
 
-        # see if we have any additional config for this test
+        # see if we have a custom config method for this test
         method = getattr(self, methodName)
-        self._extra_config = getattr(method, "_extra_config", None)
+        self._extra_config = getattr(method, "extra_config", None)
 
     def setUp(self):
         """
@@ -300,8 +299,8 @@ def default_config(self, name="test"):
         """
         config = default_config(name)
 
-        # apply any additional config which was specified via the override_config
-        # decorator.
+        # apply any additional config which was specified as an extra_config attribute
+        # on the test method..
         if self._extra_config is not None:
             config.update(self._extra_config)
 
@@ -563,27 +562,3 @@ def attempt_wrong_password_login(self, username, password):
         )
         self.render(request)
         self.assertEqual(channel.code, 403, channel.result)
-
-
-def override_config(extra_config):
-    """A decorator which can be applied to test functions to give additional HS config
-
-    For use
-
-    For example:
-
-        class MyTestCase(HomeserverTestCase):
-            @override_config({"enable_registration": False, ...})
-            def test_foo(self):
-                ...
-
-    Args:
-        extra_config(dict): Additional config settings to be merged into the default
-            config dict before instantiating the test homeserver.
-    """
-
-    def decorator(func):
-        func._extra_config = extra_config
-        return func
-
-    return decorator

From 5edca1535cf1cc2a30e7449bbece1964d807a412 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Fri, 12 Jul 2019 12:40:17 +0100
Subject: [PATCH 7/7] Revert "Add a mechanism for per-test configs"

This reverts commit 6ac7e501ead40d7fe3d9c7624730d98da9e5db0e.
---
 changelog.d/5657.misc |  1 -
 tests/unittest.py     | 30 +-----------------------------
 2 files changed, 1 insertion(+), 30 deletions(-)
 delete mode 100644 changelog.d/5657.misc

diff --git a/changelog.d/5657.misc b/changelog.d/5657.misc
deleted file mode 100644
index bdec9ae4c0b6..000000000000
--- a/changelog.d/5657.misc
+++ /dev/null
@@ -1 +0,0 @@
-Add a mechanism for per-test homeserver configuration in the unit tests.
diff --git a/tests/unittest.py b/tests/unittest.py
index 67a1822d485b..a09e76c7c21d 100644
--- a/tests/unittest.py
+++ b/tests/unittest.py
@@ -157,20 +157,6 @@ class HomeserverTestCase(TestCase):
     """
     A base TestCase that reduces boilerplate for HomeServer-using test cases.
 
-    Defines a setUp method which creates a mock reactor, and instantiates a homeserver
-    running on that reactor.
-
-    There are various hooks for modifying the way that the homeserver is instantiated:
-
-    * override make_homeserver, for example by making it pass different parameters into
-      setup_test_homeserver.
-
-    * override default_config, to return a modified configuration dictionary for use
-      by setup_test_homeserver.
-
-    * On a per-test basis, you can attach an 'extra_config' attribute, with a dictionary
-      containing additional configuration settings to be added to the basic config dict.
-
     Attributes:
         servlets (list[function]): List of servlet registration function.
         user_id (str): The user ID to assume if auth is hijacked.
@@ -182,13 +168,6 @@ class HomeserverTestCase(TestCase):
     hijack_auth = True
     needs_threadpool = False
 
-    def __init__(self, methodName, *args, **kwargs):
-        super().__init__(methodName, *args, **kwargs)
-
-        # see if we have a custom config method for this test
-        method = getattr(self, methodName)
-        self._extra_config = getattr(method, "extra_config", None)
-
     def setUp(self):
         """
         Set up the TestCase by calling the homeserver constructor, optionally
@@ -297,14 +276,7 @@ def default_config(self, name="test"):
         Args:
             name (str): The homeserver name/domain.
         """
-        config = default_config(name)
-
-        # apply any additional config which was specified as an extra_config attribute
-        # on the test method..
-        if self._extra_config is not None:
-            config.update(self._extra_config)
-
-        return config
+        return default_config(name)
 
     def prepare(self, reactor, clock, homeserver):
         """