From e9843c2709d8c91a75df0ad9754e65968a54791d Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Mon, 12 Sep 2022 11:11:44 +0200 Subject: [PATCH] Support ranges and networks in IP admin search (#19674) * Support ranges and networks in IP admin search --- Makefile-docker | 1 + package-lock.json | 14 +++++ package.json | 3 +- src/olympia/users/admin.py | 70 +++++++++++++++------ src/olympia/users/tests/test_admin.py | 89 +++++++++++++++++++++++++++ static/css/admin/userprofile.css | 12 ++++ static/js/admin/userprofile.js | 27 ++++++-- static/js/exports.js | 2 + 8 files changed, 193 insertions(+), 25 deletions(-) create mode 100644 static/js/exports.js diff --git a/Makefile-docker b/Makefile-docker index 0cc96581c5..eb1df794ab 100644 --- a/Makefile-docker +++ b/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 \ diff --git a/package-lock.json b/package-lock.json index 74d87403c3..85ae0124fa 100644 --- a/package-lock.json +++ b/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", diff --git a/package.json b/package.json index c37e7ae517..9eb48a90be 100644 --- a/package.json +++ b/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" diff --git a/src/olympia/users/admin.py b/src/olympia/users/admin.py index 3fbebf0566..175af05a62 100644 --- a/src/olympia/users/admin.py +++ b/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 diff --git a/src/olympia/users/tests/test_admin.py b/src/olympia/users/tests/test_admin.py index 2bb266207b..dfe3eef44b 100644 --- a/src/olympia/users/tests/test_admin.py +++ b/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') diff --git a/static/css/admin/userprofile.css b/static/css/admin/userprofile.css index 88bafaebfd..fec0cae66b 100644 --- a/static/css/admin/userprofile.css +++ b/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; } diff --git a/static/js/admin/userprofile.js b/static/js/admin/userprofile.js index f3181391fa..b5a4da56ec 100644 --- a/static/js/admin/userprofile.js +++ b/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); } }); diff --git a/static/js/exports.js b/static/js/exports.js new file mode 100644 index 0000000000..2c79095fb4 --- /dev/null +++ b/static/js/exports.js @@ -0,0 +1,2 @@ +// Minimal hack to be able to use JavaScript modules client-side +var exports = {};