Fix adding new rows instead of updating them if one of the key values is a NULL in upserts. (#4369)

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.

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())
 

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