From 99891d9b89a9d38ecab15c30d615e59282b0300e Mon Sep 17 00:00:00 2001 From: Artem Martynovich <artem.martynovich@gmail.com> Date: Sat, 18 Jan 2020 00:44:13 +0600 Subject: [PATCH 1/4] Don't fetch all devices for every package. --- backend/device_registry/templates/cve.html | 2 +- backend/device_registry/tests/test_all.py | 27 +++++---- backend/device_registry/views.py | 66 +++++++++++++++------- 3 files changed, 60 insertions(+), 35 deletions(-) diff --git a/backend/device_registry/templates/cve.html b/backend/device_registry/templates/cve.html index 9edda4ebb..d1ff3221e 100644 --- a/backend/device_registry/templates/cve.html +++ b/backend/device_registry/templates/cve.html @@ -40,7 +40,7 @@ <h1 style="margin-bottom: 0">CVE list{% if device_name %} for {{ device_name }}{ <td> {% for p in row.packages %} <a href="#" class="wott-popover"> - {{ p.device_urls|length }} + {{ p.devices_count }} <template> {% for du in p.device_urls %} <a href="{{ du.href }}">{{ du.text }}</a><br> diff --git a/backend/device_registry/tests/test_all.py b/backend/device_registry/tests/test_all.py index c708d3bfd..223919544 100644 --- a/backend/device_registry/tests/test_all.py +++ b/backend/device_registry/tests/test_all.py @@ -1423,11 +1423,11 @@ def test_sort_package_hosts_affected(self): self.assertListEqual(response.context_data['table_rows'], [ CVEView.TableRow(cve_name='CVE-2018-2', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ # These two AffectedPackage's should be sorted by hosts_affected - CVEView.AffectedPackage('one_second', [self.device0, self.device1]), - CVEView.AffectedPackage('one_first', [self.device0]) + CVEView.AffectedPackage('one_second', 2), + CVEView.AffectedPackage('one_first', 1) ], cve_date=self.today), CVEView.TableRow(cve_name='CVE-2018-1', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ - CVEView.AffectedPackage('one_first', [self.device0]) + CVEView.AffectedPackage('one_first', 1) ]) ]) @@ -1441,15 +1441,14 @@ def test_sort_urgency(self): self.assertListEqual(response.context_data['table_rows'], [ # These two TableRow's should be sorted by urgency CVEView.TableRow(cve_name='CVE-2018-2', cve_url='', urgency=Vulnerability.Urgency.HIGH, packages=[ - CVEView.AffectedPackage('one_first', [self.device0]), - CVEView.AffectedPackage('one_second', [self.device0]) + CVEView.AffectedPackage('one_first', 1), + CVEView.AffectedPackage('one_second', 1) ], cve_date=self.today), CVEView.TableRow(cve_name='CVE-2018-1', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ - CVEView.AffectedPackage('one_first', [self.device0]), - CVEView.AffectedPackage('one_second', [self.device0]) + CVEView.AffectedPackage('one_first', 1), + CVEView.AffectedPackage('one_second', 1) ]) ]) - self.assertEqual(len(response.context_data['table_rows'][0].packages[0].device_urls), 1) def test_sort_total_hosts_affected(self): self.packages[0].vulnerabilities.set(self.vulns) @@ -1460,11 +1459,11 @@ def test_sort_total_hosts_affected(self): self.assertListEqual(response.context_data['table_rows'], [ # These two TableRow's should be sorted by the sum of hosts_affected CVEView.TableRow(cve_name='CVE-2018-2', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ - CVEView.AffectedPackage('one_first', [self.device0]), - CVEView.AffectedPackage('one_second', [self.device0]) + CVEView.AffectedPackage('one_first', 1), + CVEView.AffectedPackage('one_second', 1) ], cve_date=self.today), CVEView.TableRow(cve_name='CVE-2018-1', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ - CVEView.AffectedPackage('one_first', [self.device0]) + CVEView.AffectedPackage('one_first', 1) ]) ]) @@ -1479,11 +1478,11 @@ def test_filter_device(self): self.assertEqual(response.status_code, 200) self.assertListEqual(response.context_data['table_rows'], [ CVEView.TableRow(cve_name='CVE-2018-2', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ - CVEView.AffectedPackage('one_first', [self.device0]), - CVEView.AffectedPackage('one_second', [self.device0]) + CVEView.AffectedPackage('one_first', 1), + CVEView.AffectedPackage('one_second', 1) ], cve_date=self.today), CVEView.TableRow(cve_name='CVE-2018-1', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ - CVEView.AffectedPackage('one_first', [self.device0]) + CVEView.AffectedPackage('one_first', 1) ]) ]) diff --git a/backend/device_registry/views.py b/backend/device_registry/views.py index ee09998e0..59ee6ca09 100644 --- a/backend/device_registry/views.py +++ b/backend/device_registry/views.py @@ -597,14 +597,7 @@ class Hyperlink(NamedTuple): class AffectedPackage(NamedTuple): name: str - devices: List[Device] - - @property - def device_urls(self): - return [CVEView.Hyperlink( - d.get_name(), - reverse('device_cve', kwargs={'device_pk': d.pk}) - ) for d in self.devices] + devices_count: int @property def upgrade_command(self): @@ -626,7 +619,7 @@ class TableRow(NamedTuple): @property def key(self): - return self.urgency, sum([len(p.devices) for p in self.packages]) + return self.urgency, sum([p.devices_count for p in self.packages]) @property def severity(self): @@ -651,26 +644,59 @@ def get_context_data(self, **kwargs): packages_query = Q(device__owner=user) vulns = Vulnerability.objects.filter(vuln_query, fix_available=True) \ - .values('name').distinct().annotate(max_urgency=Max('urgency'), - pubdate=Max('pub_date')) + .values('name').distinct().annotate(max_urgency=Max('urgency'), + pubdate=Max('pub_date')) vuln_info = {v['name']: (v['max_urgency'], v['pubdate']) for v in vulns} - packages = DebPackage.objects.filter(packages_query, - vulnerabilities__isnull=False, - vulnerabilities__fix_available=True)\ - .annotate(cve_name=F('vulnerabilities__name')) + + # TODO: use this instead of packages_with_vulns + # --- + # cve - package - device + ds = Device.objects.filter(owner=user, + deb_packages__vulnerabilities__isnull=False, + deb_packages__vulnerabilities__fix_available=True)\ + .annotate(package=F('deb_packages__name'), + cve=F('deb_packages__vulnerabilities__name'))\ + .order_by('cve', 'package') + + # cve - package - device_count + ds.values('cve', 'package').annotate(d_cnt=Count('pk')).order_by('cve', '-d_cnt') + + # cve - device_count + ds.values('cve').order_by().distinct().annotate(d_cnt=Count('pk')).order_by('-d_cnt') + + # table_rows = [] + # for d in ds: + # plist = sorted([self.AffectedPackage(package.name, (1 if device else devices_count_by_name[d.package])) + # for package in cve_packages], + # key=lambda package: package.devices_count, reverse=True) + # urgency, cve_date = vuln_info[cve_name] + # table_rows.append(self.TableRow(cve_name=d.cve, cve_url='', urgency=urgency, + # cve_date=cve_date, packages=plist)) + # --- + + + + packages_with_vulns = DebPackage.objects.filter(packages_query, + vulnerabilities__isnull=False, + vulnerabilities__fix_available=True) + packages = packages_with_vulns.only('pk').annotate(cve_name=F('vulnerabilities__name')) packages_by_cve = defaultdict(set) for p in packages: packages_by_cve[p.cve_name].add(p) + distinct_packages = packages_with_vulns.distinct().only('pk') + devices_count = DebPackage.objects.filter(pk__in=distinct_packages)\ + .annotate(devices_count=Count('device'), + filter=packages_query)\ + .values('name', 'devices_count') + devices_count_by_name = {p['name']: p['devices_count'] for p in devices_count} + table_rows = [] for cve_name, cve_packages in packages_by_cve.items(): - plist = sorted([self.AffectedPackage(package.name, - # FIXME: this line may need additional optimisation to avoid calling - # device_set.filter() every time. - [device] if device else list(package.device_set.filter(owner=user))) + plist = sorted([self.AffectedPackage(package.name, (1 if device else devices_count_by_name[package.name])) for package in cve_packages], - key=lambda package: len(package.devices), reverse=True) + key=lambda package: package.devices_count, reverse=True) urgency, cve_date = vuln_info[cve_name] table_rows.append(self.TableRow(cve_name=cve_name, cve_url='', urgency=urgency, cve_date=cve_date, packages=plist)) From c3e9fbea36e2b0b16d8177e72b169aac935e7acb Mon Sep 17 00:00:00 2001 From: Artem Martynovich <artem.martynovich@gmail.com> Date: Tue, 21 Jan 2020 16:45:36 +0600 Subject: [PATCH 2/4] Optimize CVEView by using window queries. [WIP] display affected devices. --- backend/device_registry/views.py | 95 ++++++++++++-------------------- 1 file changed, 36 insertions(+), 59 deletions(-) diff --git a/backend/device_registry/views.py b/backend/device_registry/views.py index 59ee6ca09..34f5ee000 100644 --- a/backend/device_registry/views.py +++ b/backend/device_registry/views.py @@ -6,7 +6,7 @@ from django.contrib.auth.decorators import login_required from django.contrib.auth.mixins import LoginRequiredMixin from django.db import transaction -from django.db.models import Case, When, Count, F +from django.db.models import Case, When, Count, Window from django.db.models import Q, Sum, Avg, IntegerField, Max from django.http import HttpResponseRedirect, HttpResponse, HttpResponseBadRequest, HttpResponseForbidden from django.shortcuts import get_object_or_404 @@ -598,6 +598,7 @@ class Hyperlink(NamedTuple): class AffectedPackage(NamedTuple): name: str devices_count: int + devices: List[NamedTuple] @property def upgrade_command(self): @@ -637,69 +638,45 @@ def get_context_data(self, **kwargs): if device_pk is not None: device = get_object_or_404(Device, pk=device_pk, owner=user) vuln_query = Q(debpackage__device__pk=device_pk) - packages_query = Q(device__pk=device_pk) else: device = None vuln_query = Q(debpackage__device__owner=user) - packages_query = Q(device__owner=user) - - vulns = Vulnerability.objects.filter(vuln_query, fix_available=True) \ - .values('name').distinct().annotate(max_urgency=Max('urgency'), - pubdate=Max('pub_date')) - vuln_info = {v['name']: (v['max_urgency'], v['pubdate']) for v in vulns} - - - # TODO: use this instead of packages_with_vulns - # --- - # cve - package - device - ds = Device.objects.filter(owner=user, - deb_packages__vulnerabilities__isnull=False, - deb_packages__vulnerabilities__fix_available=True)\ - .annotate(package=F('deb_packages__name'), - cve=F('deb_packages__vulnerabilities__name'))\ - .order_by('cve', 'package') - - # cve - package - device_count - ds.values('cve', 'package').annotate(d_cnt=Count('pk')).order_by('cve', '-d_cnt') - - # cve - device_count - ds.values('cve').order_by().distinct().annotate(d_cnt=Count('pk')).order_by('-d_cnt') - - # table_rows = [] - # for d in ds: - # plist = sorted([self.AffectedPackage(package.name, (1 if device else devices_count_by_name[d.package])) - # for package in cve_packages], - # key=lambda package: package.devices_count, reverse=True) - # urgency, cve_date = vuln_info[cve_name] - # table_rows.append(self.TableRow(cve_name=d.cve, cve_url='', urgency=urgency, - # cve_date=cve_date, packages=plist)) - # --- - - - - packages_with_vulns = DebPackage.objects.filter(packages_query, - vulnerabilities__isnull=False, - vulnerabilities__fix_available=True) - packages = packages_with_vulns.only('pk').annotate(cve_name=F('vulnerabilities__name')) - packages_by_cve = defaultdict(set) - for p in packages: - packages_by_cve[p.cve_name].add(p) - - distinct_packages = packages_with_vulns.distinct().only('pk') - devices_count = DebPackage.objects.filter(pk__in=distinct_packages)\ - .annotate(devices_count=Count('device'), - filter=packages_query)\ - .values('name', 'devices_count') - devices_count_by_name = {p['name']: p['devices_count'] for p in devices_count} + + vuln_names = Vulnerability.objects.only('name').filter(debpackage__device__owner=user, + fix_available=True).values('name') + vuln_pub_dates_qs = Vulnerability.objects.only('name').filter(name__in=vuln_names) \ + .values('name').annotate(pubdate=Max('pub_date')).distinct() + vuln_pub_dates = {v['name']: v['pubdate'] for v in vuln_pub_dates_qs} + + qs0 = Vulnerability.objects.filter(fix_available=True) \ + .values('name').annotate(max_urgency=Max('urgency')) \ + .filter(vuln_query) \ + .values('name', 'max_urgency', 'debpackage__pk', 'debpackage__device__pk', 'debpackage__name', 'debpackage__device__name', 'debpackage__device__deviceinfo__fqdn') + qs1 = qs0.annotate(devcnt=Window(expression=Count('debpackage__name'), + partition_by=['name', 'debpackage__name']), + cvecnt=Window(expression=Count('debpackage__name'), + partition_by=['name'])) \ + .order_by('-max_urgency', '-cvecnt', 'name', '-devcnt', 'debpackage__name') table_rows = [] - for cve_name, cve_packages in packages_by_cve.items(): - plist = sorted([self.AffectedPackage(package.name, (1 if device else devices_count_by_name[package.name])) - for package in cve_packages], - key=lambda package: package.devices_count, reverse=True) - urgency, cve_date = vuln_info[cve_name] - table_rows.append(self.TableRow(cve_name=cve_name, cve_url='', urgency=urgency, - cve_date=cve_date, packages=plist)) + current_row = None + current_package = None + for device_package_cve in qs1: + cve_name, package_name, urgency, devices_count,\ + device_pk, device_name, device_fqdn = (device_package_cve[k] for k in [ + 'name', 'debpackage__name', 'max_urgency', 'devcnt', + 'debpackage__device__pk', 'debpackage__device__name', + 'debpackage__device__deviceinfo__fqdn']) + if not current_row or current_row.cve_name != cve_name: + current_row = self.TableRow(cve_name, urgency, [], '', vuln_pub_dates[cve_name]) + table_rows.append(current_row) + current_package = None + if not current_package or current_package.name != package_name: + current_package = self.AffectedPackage(package_name, devices_count, []) + current_row.packages.append(current_package) + device_pretty_name = device_name or device_fqdn[:36] + current_package.devices.append(self.Hyperlink(href=reverse('device_cve', kwargs={'device_pk': device_pk}), + text=device_pretty_name)) context['table_rows'] = sorted(table_rows, key=lambda r: r.key, reverse=True) From eea540e4538fe2f02545f20825257acfdb2082ce Mon Sep 17 00:00:00 2001 From: Artem Martynovich <artem.martynovich@gmail.com> Date: Tue, 21 Jan 2020 17:55:01 +0600 Subject: [PATCH 3/4] Fix tests, bring back Affected Nodes popovers. --- backend/device_registry/templates/cve.html | 2 +- backend/device_registry/tests/test_all.py | 31 +++++++++++++--------- backend/device_registry/views.py | 5 +++- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/backend/device_registry/templates/cve.html b/backend/device_registry/templates/cve.html index d1ff3221e..76a40c3dc 100644 --- a/backend/device_registry/templates/cve.html +++ b/backend/device_registry/templates/cve.html @@ -42,7 +42,7 @@ <h1 style="margin-bottom: 0">CVE list{% if device_name %} for {{ device_name }}{ <a href="#" class="wott-popover"> {{ p.devices_count }} <template> - {% for du in p.device_urls %} + {% for du in p.devices %} <a href="{{ du.href }}">{{ du.text }}</a><br> {% endfor %} </template> diff --git a/backend/device_registry/tests/test_all.py b/backend/device_registry/tests/test_all.py index 223919544..12317bfe6 100644 --- a/backend/device_registry/tests/test_all.py +++ b/backend/device_registry/tests/test_all.py @@ -1413,6 +1413,10 @@ def setUp(self): self.device0.deb_packages.set(self.packages) self.device_unrelated.deb_packages.set(self.packages) + def _hyperlinks(self, devices): + return [CVEView.Hyperlink(text=device.get_name(), href=reverse('device_cve', kwargs={'device_pk': device.pk})) + for device in devices] + def test_sort_package_hosts_affected(self): self.packages[0].vulnerabilities.set(self.vulns) self.packages[1].vulnerabilities.set(self.vulns[1:]) @@ -1423,11 +1427,11 @@ def test_sort_package_hosts_affected(self): self.assertListEqual(response.context_data['table_rows'], [ CVEView.TableRow(cve_name='CVE-2018-2', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ # These two AffectedPackage's should be sorted by hosts_affected - CVEView.AffectedPackage('one_second', 2), - CVEView.AffectedPackage('one_first', 1) + CVEView.AffectedPackage('one_second', 2, self._hyperlinks([self.device0, self.device1])), + CVEView.AffectedPackage('one_first', 1, self._hyperlinks([self.device0])) ], cve_date=self.today), CVEView.TableRow(cve_name='CVE-2018-1', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ - CVEView.AffectedPackage('one_first', 1) + CVEView.AffectedPackage('one_first', 1, self._hyperlinks([self.device0])) ]) ]) @@ -1441,14 +1445,15 @@ def test_sort_urgency(self): self.assertListEqual(response.context_data['table_rows'], [ # These two TableRow's should be sorted by urgency CVEView.TableRow(cve_name='CVE-2018-2', cve_url='', urgency=Vulnerability.Urgency.HIGH, packages=[ - CVEView.AffectedPackage('one_first', 1), - CVEView.AffectedPackage('one_second', 1) + CVEView.AffectedPackage('one_first', 1, self._hyperlinks([self.device0])), + CVEView.AffectedPackage('one_second', 1, self._hyperlinks([self.device0])) ], cve_date=self.today), CVEView.TableRow(cve_name='CVE-2018-1', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ - CVEView.AffectedPackage('one_first', 1), - CVEView.AffectedPackage('one_second', 1) + CVEView.AffectedPackage('one_first', 1, self._hyperlinks([self.device0])), + CVEView.AffectedPackage('one_second', 1, self._hyperlinks([self.device0])) ]) ]) + # self.assertEqual(len(response.context_data['table_rows'][0].packages[0].device_urls), 1) def test_sort_total_hosts_affected(self): self.packages[0].vulnerabilities.set(self.vulns) @@ -1459,11 +1464,11 @@ def test_sort_total_hosts_affected(self): self.assertListEqual(response.context_data['table_rows'], [ # These two TableRow's should be sorted by the sum of hosts_affected CVEView.TableRow(cve_name='CVE-2018-2', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ - CVEView.AffectedPackage('one_first', 1), - CVEView.AffectedPackage('one_second', 1) + CVEView.AffectedPackage('one_first', 1, self._hyperlinks([self.device0])), + CVEView.AffectedPackage('one_second', 1, self._hyperlinks([self.device0])) ], cve_date=self.today), CVEView.TableRow(cve_name='CVE-2018-1', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ - CVEView.AffectedPackage('one_first', 1) + CVEView.AffectedPackage('one_first', 1, self._hyperlinks([self.device0])) ]) ]) @@ -1478,11 +1483,11 @@ def test_filter_device(self): self.assertEqual(response.status_code, 200) self.assertListEqual(response.context_data['table_rows'], [ CVEView.TableRow(cve_name='CVE-2018-2', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ - CVEView.AffectedPackage('one_first', 1), - CVEView.AffectedPackage('one_second', 1) + CVEView.AffectedPackage('one_first', 1, self._hyperlinks([self.device0])), + CVEView.AffectedPackage('one_second', 1, self._hyperlinks([self.device0])) ], cve_date=self.today), CVEView.TableRow(cve_name='CVE-2018-1', cve_url='', urgency=Vulnerability.Urgency.LOW, packages=[ - CVEView.AffectedPackage('one_first', 1) + CVEView.AffectedPackage('one_first', 1, self._hyperlinks([self.device0])) ]) ]) diff --git a/backend/device_registry/views.py b/backend/device_registry/views.py index 34f5ee000..77383d56d 100644 --- a/backend/device_registry/views.py +++ b/backend/device_registry/views.py @@ -674,7 +674,10 @@ def get_context_data(self, **kwargs): if not current_package or current_package.name != package_name: current_package = self.AffectedPackage(package_name, devices_count, []) current_row.packages.append(current_package) - device_pretty_name = device_name or device_fqdn[:36] + if device_name or device_fqdn: + device_pretty_name = device_name or device_fqdn[:36] + else: + device_pretty_name = f"device_{device_pk}" current_package.devices.append(self.Hyperlink(href=reverse('device_cve', kwargs={'device_pk': device_pk}), text=device_pretty_name)) From c8326774c435a0985b959789620a806fdbfc9771 Mon Sep 17 00:00:00 2001 From: Artem Martynovich <artem.martynovich@gmail.com> Date: Wed, 22 Jan 2020 14:37:35 +0600 Subject: [PATCH 4/4] Don't scroll to the top when clicking on popover links. Hide popovers on outside click. --- backend/device_registry/templates/cve.html | 26 +++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/backend/device_registry/templates/cve.html b/backend/device_registry/templates/cve.html index 76a40c3dc..274c9111d 100644 --- a/backend/device_registry/templates/cve.html +++ b/backend/device_registry/templates/cve.html @@ -76,16 +76,36 @@ <h1 style="margin-bottom: 0">CVE list{% if device_name %} for {{ device_name }}{ {{ block.super }} <script> + let popoverShown = null; + $(function () { $('[data-toggle="popover"]').popover() }); - $('.wott-popover').popover({ + $('.wott-popover').click((e) => { + e.preventDefault(); // prevent scrolling to the top (because <a href="#">) + if(popoverShown) // hide previous popover + $(popoverShown).popover('hide'); + $(e.target).popover('show'); + popoverShown = e.target; + }).popover({ html: true, - trigger: 'click', + trigger: 'manual', title: 'Details', content: function() { - return $(this).children('template').html(); + return $(this).children('template').html(); + } + }); + + $('body').click((e) => { + if(e.target == popoverShown || $(e.target).parents('.popover').length) { + // clicked on popover link or inside popover + return; + } + if(popoverShown) { + // clicked anywhere outside the popover while it's shown -> hide it + $(popoverShown).popover('hide'); + popoverShown = null; } }) </script>