launchpad-reviewers team mailing list archive

[Andrey Fedoseev <mp+427752@xxxxxxxxxxxxxxxxxx>]

Andrey Fedoseev has proposed merging ~andrey-fedoseev/launchpad:uct-import-fixes into launchpad:master.

Commit message:
Fix the issues in the UCT import script


Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~andrey-fedoseev/launchpad/+git/launchpad/+merge/427752

- Vulnerability.date_made_public requires a timezone-aware datetime instance but the script passes a naive datetime object.

- The log line after creating a vulnerability tries to log the vulnerability ID but the passed parameter is a Vulnerability instance.

- The transaction started by the script run is not committed. Colin suggested adding a --dry-run flag and either aborting or committing the transaction based on whether the flag is passed or not.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~andrey-fedoseev/launchpad:uct-import-fixes into launchpad:master.

diff --git a/lib/lp/bugs/scripts/tests/test_uctimport.py b/lib/lp/bugs/scripts/tests/test_uctimport.py
index 77a37d6..19f2904 100644
--- a/lib/lp/bugs/scripts/tests/test_uctimport.py
+++ b/lib/lp/bugs/scripts/tests/test_uctimport.py
@@ -4,6 +4,7 @@ import datetime
 from pathlib import Path
 from typing import List
 
+from pytz import UTC
 from zope.component import getUtility
 
 from lp.app.enums import InformationType
@@ -22,7 +23,7 @@ from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.model.sourcepackage import SourcePackage
 from lp.services.propertycache import clear_property_cache
 from lp.testing import TestCase, TestCaseWithFactory
-from lp.testing.layers import ZopelessDatabaseLayer
+from lp.testing.layers import LaunchpadZopelessLayer, ZopelessDatabaseLayer
 
 
 class TestUCTRecord(TestCase):
@@ -352,7 +353,7 @@ class TextCVE(TestCaseWithFactory):
 
 class TestUCTImporter(TestCaseWithFactory):
 
-    layer = ZopelessDatabaseLayer
+    layer = LaunchpadZopelessLayer
 
     def setUp(self, *args, **kwargs):
         super().setUp(*args, **kwargs)
@@ -474,7 +475,8 @@ class TestUCTImporter(TestCaseWithFactory):
             notes="author> text",
             mitigation="mitigation",
         )
-        self.importer = UCTImporter()
+        self.layer.txn.commit()
+        self.importer = UCTImporter(self.layer.txn)
 
     def checkBug(self, bug: Bug, cve: CVE):
         self.assertEqual(cve.sequence, bug.title)
@@ -797,3 +799,23 @@ class TestUCTImporter(TestCaseWithFactory):
         cve.references.pop(0)
         self.importer.update_bug(bug, cve, self.lp_cve)
         self.checkBug(bug, cve)
+
+    def test_import_cve(self):
+        self.importer.import_cve(self.cve)
+        self.assertIsNotNone(
+            self.importer.find_existing_bug(self.cve, self.lp_cve)
+        )
+
+    def test_import_cve_dry_run(self):
+        importer = UCTImporter(self.layer.txn, dry_run=True)
+        importer.import_cve(self.cve)
+        self.assertIsNone(importer.find_existing_bug(self.cve, self.lp_cve))
+
+    def test_naive_date_made_public(self):
+        cve = self.cve
+        cve.date_made_public = cve.date_made_public.replace(tzinfo=None)
+        bug = self.importer.create_bug(cve, self.lp_cve)
+        self.assertEqual(
+            UTC,
+            bug.vulnerabilities[0].date_made_public.tzinfo,
+        )
diff --git a/lib/lp/bugs/scripts/uctimport.py b/lib/lp/bugs/scripts/uctimport.py
index 6d1c216..a1a8f23 100644
--- a/lib/lp/bugs/scripts/uctimport.py
+++ b/lib/lp/bugs/scripts/uctimport.py
@@ -16,7 +16,7 @@ For each entry in UCT we:
 4. Update the statuses of Bug Tasks based on the information in the CVE entry
 """
 import logging
-from datetime import datetime
+from datetime import datetime, timezone
 from enum import Enum
 from itertools import chain
 from pathlib import Path
@@ -543,7 +543,9 @@ class UCTImportError(Exception):
 
 
 class UCTImporter:
-    def __init__(self):
+    def __init__(self, transaction, dry_run=False):
+        self.transaction = transaction
+        self.dry_run = dry_run
         self.bug_importer = getUtility(ILaunchpadCelebrities).bug_importer
 
     def import_cve_from_file(self, cve_path: Path) -> None:
@@ -562,10 +564,21 @@ class UCTImporter:
             logger.warning("Could not find the CVE in LP: %s", cve.sequence)
             return
         bug = self.find_existing_bug(cve, lp_cve)
-        if bug is None:
-            self.create_bug(cve, lp_cve)
+        self.transaction.begin()
+        try:
+            if bug is None:
+                self.create_bug(cve, lp_cve)
+            else:
+                self.update_bug(bug, cve, lp_cve)
+        except Exception:
+            self.transaction.abort()
+            raise
+
+        if self.dry_run:
+            logger.info("Dry-run mode enabled, all changes are reverted.")
+            self.transaction.abort()
         else:
-            self.update_bug(bug, cve, lp_cve)
+            self.transaction.commit()
 
     def find_existing_bug(
         self, cve: CVE, lp_cve: CveModel
@@ -677,6 +690,9 @@ class UCTImporter:
         lp_cve: CveModel,
         distribution: Distribution,
     ) -> Vulnerability:
+        date_made_public = cve.date_made_public
+        if date_made_public.tzinfo is None:
+            date_made_public = date_made_public.replace(tzinfo=timezone.utc)
         vulnerability = getUtility(IVulnerabilitySet).new(
             distribution=distribution,
             creator=bug.owner,
@@ -687,12 +703,12 @@ class UCTImporter:
             mitigation=cve.mitigation,
             importance=cve.importance,
             information_type=InformationType.PUBLICSECURITY,
-            date_made_public=cve.date_made_public,
+            date_made_public=date_made_public,
         )  # type: Vulnerability
 
         vulnerability.linkBug(bug, bug.owner)
 
-        logger.info("Create vulnerability with ID: %s", vulnerability)
+        logger.info("Created vulnerability with ID: %s", vulnerability.id)
 
         return vulnerability
 
diff --git a/scripts/uct-import.py b/scripts/uct-import.py
index 1d162fa..8124566 100755
--- a/scripts/uct-import.py
+++ b/scripts/uct-import.py
@@ -18,11 +18,20 @@ class UCTImportScript(LaunchpadScript):
     )
     loglevel = logging.INFO
 
+    def add_my_options(self):
+        self.parser.add_option(
+            "--dry-run",
+            action="store_true",
+            dest="dry_run",
+            default=False,
+            help="Don't commit changes to the DB.",
+        )
+
     def main(self):
         if len(self.args) != 1:
             self.parser.error("Please specify a CVE file to import")
 
-        importer = UCTImporter()
+        importer = UCTImporter(self.txn, dry_run=self.options.dry_run)
 
         cve_path = Path(self.args[0])
         importer.import_cve_from_file(cve_path)