Support ranges and networks in IP admin search (#19674)

* Support ranges and networks in IP admin search

Makefile-docker

@@ -36,6 +36,7 @@ jquery.cookie/jquery.cookie.js \
 jszip/dist/jszip.js \
 timeago/jquery.timeago.js \
 underscore/underscore.js \
+netmask/lib/netmask.js \
 
 NODE_LIBS_JQUERY_UI := \
 jquery-ui/ui/data.js \

package-lock.json

@@ -20,6 +20,7 @@
         "jquery.cookie": "1.4.1",
         "jszip": "3.10.1",
         "less": "4.1.3",
+        "netmask": "^2.0.2",
         "source-map": "0.7.4",
         "timeago": "1.6.7",
         "underscore": "1.13.4"
@@ -4709,6 +4710,14 @@
         "ms": "^2.1.1"
       }
     },
+    "node_modules/netmask": {
+      "version": "2.0.2",
+      "resolved": "https://registry.npmjs.org/netmask/-/netmask-2.0.2.tgz",
+      "integrity": "sha512-dBpDMdxv9Irdq66304OLfEmQ9tbNRFnFTuZiLo+bD+r332bBmMJ8GBLXklIXXgxd3+v9+KUnZaUR5PJMa75Gsg==",
+      "engines": {
+        "node": ">= 0.4.0"
+      }
+    },
     "node_modules/node-int64": {
       "version": "0.4.0",
       "resolved": "https://registry.npmjs.org/node-int64/-/node-int64-0.4.0.tgz",
@@ -9990,6 +9999,11 @@
         }
       }
     },
+    "netmask": {
+      "version": "2.0.2",
+      "resolved": "https://registry.npmjs.org/netmask/-/netmask-2.0.2.tgz",
+      "integrity": "sha512-dBpDMdxv9Irdq66304OLfEmQ9tbNRFnFTuZiLo+bD+r332bBmMJ8GBLXklIXXgxd3+v9+KUnZaUR5PJMa75Gsg=="
+    },
     "node-int64": {
       "version": "0.4.0",
       "resolved": "https://registry.npmjs.org/node-int64/-/node-int64-0.4.0.tgz",

package.json

@@ -18,14 +18,15 @@
     "jquery.cookie": "1.4.1",
     "jszip": "3.10.1",
     "less": "4.1.3",
+    "netmask": "^2.0.2",
     "source-map": "0.7.4",
     "timeago": "1.6.7",
     "underscore": "1.13.4"
   },
   "devDependencies": {
     "jest": "^29.0.0",
-    "jest-environment-jsdom": "^29.0.0",
     "jest-date-mock": "^1.0.8",
+    "jest-environment-jsdom": "^29.0.0",
     "lodash": "^4.17.21",
     "prettier": "^2.0.5",
     "terser": "^5.14.2"

src/olympia/users/admin.py

@@ -159,7 +159,7 @@ class UserAdmin(CommaSearchInAdminMixin, admin.ModelAdmin):
     actions = ['ban_action', 'reset_api_key_action', 'reset_session_action']
 
     class Media:
-        js = ('js/admin/userprofile.js',)
+        js = ('js/admin/userprofile.js', 'js/exports.js', 'js/node_lib/netmask.js')
         css = {'all': ('css/admin/userprofile.css',)}
 
     def get_list_display(self, request):
@@ -171,37 +171,69 @@ class UserAdmin(CommaSearchInAdminMixin, admin.ModelAdmin):
         search_term = (
             search_form.cleaned_data.get(SEARCH_VAR) if search_form.is_valid() else None
         )
-        if search_term and self.ip_addresses_if_query_is_all_ip_addresses(search_term):
+        if search_term and self.ip_addresses_and_networks_from_query(search_term):
             return (*self.list_display, *self.extra_list_display_for_ip_searches)
         return self.list_display
 
-    def ip_addresses_if_query_is_all_ip_addresses(self, search_term):
+    def ip_addresses_and_networks_from_query(self, search_term):
         # Caller should already have cleaned up search_term at this point,
         # removing whitespace etc if there is a comma separating multiple
         # terms.
         search_terms = search_term.split(',')
         ips = []
+        networks = []
         for term in search_terms:
+            # If term is a number, skip trying to recognize an IP address
+            # entirely, because ip_address() is able to understand IP addresses
+            # as integers, and we don't want that, it's likely an user ID.
+            if term.isdigit():
+                return None
+            # Is the search term an IP ?
             try:
-                ip = ipaddress.ip_address(term)
-                ip_str = str(ip)
-                if ip_str != term:
-                    raise ValueError
-                ips.append(ip_str)
+                ips.append(ipaddress.ip_address(term))
+                continue
             except ValueError:
-                break
-        if search_terms == ips:
-            # If all we are searching for are IPs, we'll use our custom IP
-            # search.
-            # Note that this comparison relies on ips being stored as strings,
-            # if that were to change that it would break.
-            return ips
-        return None
+                pass
+            # Is the search term a network ?
+            try:
+                networks.append(ipaddress.ip_network(term))
+                continue
+            except ValueError:
+                pass
+            # Is the search term an IP range ?
+            if term.count('-') == 1:
+                try:
+                    networks.extend(
+                        ipaddress.summarize_address_range(
+                            *(ipaddress.ip_address(i.strip()) for i in term.split('-'))
+                        )
+                    )
+                    continue
+                except (ValueError, TypeError):
+                    pass
+            # That search term doesn't look like an IP, network or range, so
+            # we're not doing an IP search.
+            return None
+        return {'ips': ips, 'networks': networks}
 
     def get_search_results(self, request, queryset, search_term):
-        ips = self.ip_addresses_if_query_is_all_ip_addresses(search_term)
-        if ips:
-            condition = Q(activitylog__iplog__ip_address_binary__in=ips)
+        ips_and_networks = self.ip_addresses_and_networks_from_query(search_term)
+        if ips_and_networks:
+            condition = Q()
+            if ips_and_networks['ips']:
+                # IPs search can be implemented in a single __in=() query.
+                condition |= Q(
+                    activitylog__iplog__ip_address_binary__in=ips_and_networks['ips']
+                )
+            if ips_and_networks['networks']:
+                # Networks search need one __range conditions for each network.
+                for network in ips_and_networks['networks']:
+                    condition |= Q(
+                        activitylog__iplog__ip_address_binary__range=(
+                            network[0],
+                            network[-1],
+                        )
+                    )
             # We want to duplicate the joins against activitylog + iplog so
             # that one is used for the search, and the other for the group
             # concat showing all IPs for activities of that user. Django

src/olympia/users/tests/test_admin.py

@@ -212,6 +212,50 @@ class TestUserAdmin(TestCase):
         # Make sure last login is now displayed, and has the right value.
         assert doc('.field-known_ip_adresses').text() == '127.0.0.2'
 
+    def test_search_for_ip_range(self):
+        user = user_factory(email='someone@mozilla.com')
+        self.grant_permission(user, 'Users:Edit')
+        self.client.force_login(user)
+        with core.override_remote_addr('127.0.0.2'):
+            ActivityLog.create(amo.LOG.ADD_RATING, user=self.user)
+        self.user.update(email='foo@bar.com')
+        with self.assertNumQueries(6):
+            # - 2 savepoint/release
+            # - 2 logged in user & groups
+            # - 1 count for the main search query
+            # - 1 main search query
+            response = self.client.get(
+                self.list_url, {'q': '127.0.0.1-127.0.0.3,'}, follow=True
+            )
+        assert response.status_code == 200
+        doc = pq(response.content)
+        # Make sure it's the right user.
+        assert doc('.field-email').text() == self.user.email
+        # Make sure last login is now displayed, and has the right value.
+        assert doc('.field-known_ip_adresses').text() == '127.0.0.2'
+
+    def test_search_for_ip_network(self):
+        user = user_factory(email='someone@mozilla.com')
+        self.grant_permission(user, 'Users:Edit')
+        self.client.force_login(user)
+        with core.override_remote_addr('127.0.0.2'):
+            ActivityLog.create(amo.LOG.ADD_RATING, user=self.user)
+        self.user.update(email='foo@bar.com')
+        with self.assertNumQueries(6):
+            # - 2 savepoint/release
+            # - 2 logged in user & groups
+            # - 1 count for the main search query
+            # - 1 main search query
+            response = self.client.get(
+                self.list_url, {'q': '127.0.0.0/24'}, follow=True
+            )
+        assert response.status_code == 200
+        doc = pq(response.content)
+        # Make sure it's the right user.
+        assert doc('.field-email').text() == self.user.email
+        # Make sure last login is now displayed, and has the right value.
+        assert doc('.field-known_ip_adresses').text() == '127.0.0.2'
+
     def test_search_for_multiple_ips_with_garbage(self):
         user = user_factory(email='someone@mozilla.com')
         self.grant_permission(user, 'Users:Edit')
@@ -279,6 +323,51 @@ class TestUserAdmin(TestCase):
             '127.0.0.2\n127.0.0.3 127.0.0.2 127.0.0.3'
         )
 
+    def test_search_for_ip_network_with_deduplication(self):
+        user = user_factory(email='someone@mozilla.com')
+        self.grant_permission(user, 'Users:Edit')
+        self.client.force_login(user)
+        # Will match once with the last_login
+        with core.override_remote_addr('127.0.0.3'):
+            ActivityLog.create(amo.LOG.LOG_IN, user=self.user)
+        self.user.update(email='foo@bar.com')
+        # Will match twice, will only show up once since it's the same user.
+        extra_user = user_factory(email='extra@bar.com')
+        with core.override_remote_addr('127.0.0.2'):
+            ActivityLog.create(amo.LOG.LOG_IN, user=extra_user)
+            ActivityLog.create(amo.LOG.LOG_IN, user=extra_user)
+        # Will match 4 times, will only show up once since it's the same user.
+        extra_extra_user = user_factory(email='extra_extra@bar.com')
+        with core.override_remote_addr('127.0.0.3'):
+            ActivityLog.create(amo.LOG.LOG_IN, user=extra_extra_user)
+        with core.override_remote_addr('127.0.0.2'):
+            ActivityLog.create(amo.LOG.RESTRICTED, user=extra_extra_user)
+            ActivityLog.create(amo.LOG.ADD_RATING, user=extra_extra_user)
+            ActivityLog.create(amo.LOG.ADD_VERSION, user=extra_extra_user)
+        with self.assertNumQueries(6):
+            # - 2 savepoint/release
+            # - 2 logged in user & groups
+            # - 1 count for the main search query
+            # - 1 main search query
+            response = self.client.get(
+                self.list_url, {'q': '127.0.0.0/24'}, follow=True
+            )
+        assert response.status_code == 200
+        doc = pq(response.content)
+        assert len(doc('#result_list tbody tr')) == 3
+        # Make sure it's the right users.
+        assert doc('.field-email').text() == ' '.join(
+            [
+                extra_extra_user.email,
+                extra_user.email,
+                self.user.email,
+            ]
+        )
+        # Make sure each IP only appears once for each row.
+        assert doc('.field-known_ip_adresses').text() == (
+            '127.0.0.2\n127.0.0.3 127.0.0.2 127.0.0.3'
+        )
+
     def test_search_for_mixed_strings(self):
         # IP search is deactivated if the search term don't all look like IPs
         user = user_factory(email='someone@mozilla.com')

static/css/admin/userprofile.css

@@ -28,6 +28,18 @@
     font-weight: bold;
 }
 
+#changelist-search > div {
+    display: flex;
+    align-items: center;
+    gap: 2px;
+}
+
+#toolbar #searchbar {
+    font-size: 18px;
+    height: auto;
+    width: 42em;
+}
+
 #toolbar #searchbar.dirty {
     border-color: orange;
 }

static/js/admin/userprofile.js

@@ -7,6 +7,8 @@ document.addEventListener('DOMContentLoaded', () => {
   var original_search_terms;
   var ip_fields;
 
+  const Netmask = exports.Netmask;
+
   function are_set_equal(a, b) {
     /** Return whether or not two sets contains the same values. */
     if (a.size !== b.size) {
@@ -43,11 +45,24 @@ document.addEventListener('DOMContentLoaded', () => {
      * loaded.
      */
     for (const field of ip_fields) {
-      const value = field.textContent;
-      if (value.length <= 1) {
+      const ip = field.textContent;
+      if (ip.length <= 1) {
         continue;
       }
-      if (!values.has(value)) {
+      // Our Set can contain IPs and networks, so we can't just do
+      // values.has(ip), we have to iterate on it and look if it's contained
+      // in the corresponding block (Netmask(ip) returns a block with only that
+      // IP in it).
+      let found = false;
+      values.forEach((item) => {
+        let block = new Netmask(item);
+        if (block.contains(ip)) {
+          found = true;
+          return;
+        }
+      });
+
+      if (!found) {
         field.classList.add('notinsearch');
       } else {
         field.classList.remove('notinsearch');
@@ -100,7 +115,9 @@ document.addEventListener('DOMContentLoaded', () => {
     '.change-list .field-known_ip_adresses li',
   );
   add_add_remove_links();
-  highlight_ips_not_in(get_search_bar_terms());
+  const values = get_search_bar_terms();
+  search_bar.value = [...values].join(', ');
+  highlight_ips_not_in(values);
 
   document.querySelector('#result_list').addEventListener('click', (e) => {
     const target = e.target;
@@ -114,7 +131,7 @@ document.addEventListener('DOMContentLoaded', () => {
       } else if (target.classList.contains('deletelink')) {
         values.delete(new_value);
       }
-      search_bar.value = [...values].join(',');
+      search_bar.value = [...values].join(', ');
       highlight_ips_not_in(values);
     }
   });

static/js/exports.js

@@ -0,0 +1,2 @@
+// Minimal hack to be able to use JavaScript modules client-side
+var exports = {};