From 7960c26fda8dd8d2d1333b45ef4f5c644930a619 Mon Sep 17 00:00:00 2001
From: Amber Brown <hawkowl@atleastfornow.net>
Date: Wed, 9 Jan 2019 22:26:25 +1100
Subject: [PATCH] Fix adding new rows instead of updating them if one of the
 key values is a NULL in upserts. (#4369)

---
 changelog.d/4369.bugfix          |  1 +
 synapse/storage/_base.py         | 10 ++++-
 tests/storage/test_client_ips.py | 71 ++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 1 deletion(-)
 create mode 100644 changelog.d/4369.bugfix

diff --git a/changelog.d/4369.bugfix b/changelog.d/4369.bugfix
new file mode 100644
index 000000000..a78d55793
--- /dev/null
+++ b/changelog.d/4369.bugfix
@@ -0,0 +1 @@
+Prevent users with access tokens predating the introduction of device IDs from creating spurious entries in the user_ips table.
diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py
index 1d3069b14..865b5e915 100644
--- a/synapse/storage/_base.py
+++ b/synapse/storage/_base.py
@@ -547,11 +547,19 @@ class SQLBaseStore(object):
         if lock:
             self.database_engine.lock_table(txn, table)
 
+        def _getwhere(key):
+            # If the value we're passing in is None (aka NULL), we need to use
+            # IS, not =, as NULL = NULL equals NULL (False).
+            if keyvalues[key] is None:
+                return "%s IS ?" % (key,)
+            else:
+                return "%s = ?" % (key,)
+
         # First try to update.
         sql = "UPDATE %s SET %s WHERE %s" % (
             table,
             ", ".join("%s = ?" % (k,) for k in values),
-            " AND ".join("%s = ?" % (k,) for k in keyvalues)
+            " AND ".join(_getwhere(k) for k in keyvalues)
         )
         sqlargs = list(values.values()) + list(keyvalues.values())
 
diff --git a/tests/storage/test_client_ips.py b/tests/storage/test_client_ips.py
index 4577e9422..858efe499 100644
--- a/tests/storage/test_client_ips.py
+++ b/tests/storage/test_client_ips.py
@@ -62,6 +62,77 @@ class ClientIpStoreTestCase(unittest.HomeserverTestCase):
             r,
         )
 
+    def test_insert_new_client_ip_none_device_id(self):
+        """
+        An insert with a device ID of NULL will not create a new entry, but
+        update an existing entry in the user_ips table.
+        """
+        self.reactor.advance(12345678)
+
+        user_id = "@user:id"
+
+        # Add & trigger the storage loop
+        self.get_success(
+            self.store.insert_client_ip(
+                user_id, "access_token", "ip", "user_agent", None
+            )
+        )
+        self.reactor.advance(200)
+        self.pump(0)
+
+        result = self.get_success(
+            self.store._simple_select_list(
+                table="user_ips",
+                keyvalues={"user_id": user_id},
+                retcols=["access_token", "ip", "user_agent", "device_id", "last_seen"],
+                desc="get_user_ip_and_agents",
+            )
+        )
+
+        self.assertEqual(
+            result,
+            [
+                {
+                    'access_token': 'access_token',
+                    'ip': 'ip',
+                    'user_agent': 'user_agent',
+                    'device_id': None,
+                    'last_seen': 12345678000,
+                }
+            ],
+        )
+
+        # Add another & trigger the storage loop
+        self.get_success(
+            self.store.insert_client_ip(
+                user_id, "access_token", "ip", "user_agent", None
+            )
+        )
+        self.reactor.advance(10)
+        self.pump(0)
+
+        result = self.get_success(
+            self.store._simple_select_list(
+                table="user_ips",
+                keyvalues={"user_id": user_id},
+                retcols=["access_token", "ip", "user_agent", "device_id", "last_seen"],
+                desc="get_user_ip_and_agents",
+            )
+        )
+        # Only one result, has been upserted.
+        self.assertEqual(
+            result,
+            [
+                {
+                    'access_token': 'access_token',
+                    'ip': 'ip',
+                    'user_agent': 'user_agent',
+                    'device_id': None,
+                    'last_seen': 12345878000,
+                }
+            ],
+        )
+
     def test_disabled_monthly_active_user(self):
         self.hs.config.limit_usage_by_mau = False
         self.hs.config.max_mau_value = 50