Looking good, and great test coverage :thumbs_up:
Added a few comments
Diff comments:
> diff --git a/lib/lp/bugs/interfaces/vulnerability.py
> b/lib/lp/bugs/interfaces/vulnerability.py
> index 90a9829..bda5c1a 100644
> --- a/lib/lp/bugs/interfaces/vulnerability.py
> +++ b/lib/lp/bugs/interfaces/vulnerability.py
> @@ -381,6 +394,94 @@ class IVulnerabilitySet(Interface):
> def findByIds(vulnerability_ids, visible_by_user=None):
> """Returns the vulnerabilities with the given IDs."""
>
> + @operation_parameters(
> + handler=Choice(
> + title=_("The handler to import or export vulnerability data"),
> + readonly=True,
> + required=True,
> + vocabulary=VulnerabilityHandlerEnum,
> + ),
> + git_repository=Reference(schema=IGitRepository),
> + git_ref=TextLine(
> + title=_("The git reference to import from."),
> + required=True,
> + ),
> + git_paths=List(
> + title=_("The list of paths to import from."),
> + value_type=TextLine(),
> + required=True,
> + ),
> + information_type=copy_field(
> + IVulnerabilityEditableAttributes["information_type"]
> + ),
> + import_since_commit_sha1=TextLine(
> + title=_(
> + "Commit sha1 from which to do the import. All files added or
> "
> + "modified since this commit will be imported."
> + ),
> + required=False,
> + ),
> + )
> + @call_with(requester=REQUEST_USER)
> + @export_write_operation()
> + @operation_for_version("devel")
> + def importData(
> + requester,
> + handler,
> + git_repository,
> + git_ref,
> + git_paths,
> + information_type,
> + import_since_commit_sha1,
> + ):
> + """Trigger a vulnerability data import
> +
> + :param handler: The handler that will perform the import.
> + :param git_repository: The git repository to import from.
> + :param git_ref: The git reference to import from.
> + :param git_paths: The paths in the git repo to import from.
Might not be the case now, but maybe we should make it so that if this is None
we just import from the whole repo? Maybe not even make it required... Not a
deal breaker here
> + :param information_type: The information type of the vulnerability.
> + If the git repo is private, the information type must be PRIVATE.
> + :param import_since_commit_sha1: The commit SHA1 to import from.
> + If null, import all data.
> + """
> +
> + @operation_parameters(
> + handler=Choice(
> + title=_("The handler to import or export vulnerability data"),
> + readonly=True,
> + required=True,
> + vocabulary=VulnerabilityHandlerEnum,
> + ),
> + sources=List(
> + title=_(
> + "List of objects to get data from. If null, import all data."
> + ),
> + required=False,
> + ),
> + )
> + @call_with(requester=REQUEST_USER)
> + @export_write_operation()
> + @operation_for_version("devel")
> + def exportData(
> + requester,
> + handler,
> + sources,
> + ):
> + """Trigger a vulnerability data export
> +
> + :param handler: The handler that will perform the export.
> + :param sources: List of objects to get data from. If null, import all
> + data.
> + """
> +
> + @collection_default_content()
> + def empty_list():
> + """Return an empty collection of vulnerabilities.
> +
> + This only exists to keep lazr.restful happy.
> + """
> +
>
> class IVulnerabilityActivity(Interface):
> """`IVulnerabilityActivity` attributes that require launchpad.View."""
> diff --git a/lib/lp/bugs/model/tests/test_vulnerability.py
> b/lib/lp/bugs/model/tests/test_vulnerability.py
> index 52d6a13..67b9597 100644
> --- a/lib/lp/bugs/model/tests/test_vulnerability.py
> +++ b/lib/lp/bugs/model/tests/test_vulnerability.py
> @@ -639,3 +650,385 @@ class TestVulnerabilitySet(TestCaseWithFactory):
> visible_by_user=person,
> ),
> )
> +
> +
> +class TestVulnerabilitySetImportData(TestCaseWithFactory):
> + layer = DatabaseFunctionalLayer
> +
> + def setUp(self):
> + super().setUp()
> + self.requester = self.factory.makePerson()
> + self.handler = VulnerabilityHandlerEnum.SOSS
> + self.git_ref = "ref/heads/main"
> + self.git_paths = ["cves"]
> + self.information_type = InformationType.PROPRIETARY
> + self.team = self.factory.makeTeam(members=[self.requester])
> +
> + def test_importData(self):
> + """Test that we can create a ImportVulnerabilityJob using importData
> + method.
> + """
> + self.useContext(feature_flags())
> + set_feature_flag(VULNERABILITY_IMPORT_ENABLED_FEATURE_FLAG, "true")
> +
> + self.factory.makeDistribution(name="soss", owner=self.requester)
> +
> + repo = self.factory.makeGitRepository(
> + owner=self.team, information_type=InformationType.USERDATA
> + )
> +
> + self.factory.makeGitRefs(
> + repository=repo,
> + paths=[self.git_ref],
> + )
> +
> + with person_logged_in(self.requester):
> + getUtility(IVulnerabilitySet).importData(
> + self.requester,
> + self.handler,
> + repo,
> + self.git_ref,
> + self.git_paths,
> + self.information_type,
> + import_since_commit_sha1=None,
> + )
> +
> + job = getUtility(IImportVulnerabilityJobSource).get(self.handler)
> + naked_job = removeSecurityProxy(job)
> + self.assertIsInstance(naked_job, ImportVulnerabilityJob)
> + self.assertEqual(naked_job.git_repository, repo.git_https_url)
> + self.assertEqual(naked_job.git_ref, self.git_ref)
> + self.assertEqual(naked_job.git_paths, self.git_paths)
> + self.assertEqual(
> + naked_job.information_type, self.information_type.value
> + )
> + self.assertEqual(naked_job.import_since_commit_sha1, None)
> +
> + def test_importData_feature_flag_disabled(self):
> + """Test that if the feature flag is disabled it raises the
> + NotImplementedError exception."""
> +
> + # All parameters None, feature flag is the first check
> + self.assertRaisesWithContent(
> + NotImplementedError,
> + "Vulnerability import API is currently disabled",
> + getUtility(IVulnerabilitySet).importData,
> + *(None,) * 7,
> + )
> +
> + def test_importData_git_repo_unauthorized(self):
> + """Test that a user cannot create a ImportVulnerabilityJob for a git
> + repository that is not visible for that user.
> + """
> + self.useContext(feature_flags())
> + set_feature_flag(VULNERABILITY_IMPORT_ENABLED_FEATURE_FLAG, "true")
> +
> + self.factory.makeDistribution(name="soss", owner=self.requester)
> +
> + repo = self.factory.makeGitRepository(
> + information_type=InformationType.PRIVATESECURITY
> + )
> +
> + self.factory.makeGitRefs(
> + repository=repo,
> + paths=[self.git_ref],
> + )
> +
> + self.assertRaises(
> + Unauthorized,
> + getUtility(IVulnerabilitySet).importData,
> + self.requester,
> + self.handler,
> + repo,
> + self.git_ref,
> + self.git_paths,
> + self.information_type,
> + import_since_commit_sha1=None,
> + )
> +
> + def test_importData_handler_unauthorized(self):
> + """Test that we cannot create a ImportVulnerabilityJob if the user is
> + not authorized to use the handler.
> + """
> + self.useContext(feature_flags())
> + set_feature_flag(VULNERABILITY_IMPORT_ENABLED_FEATURE_FLAG, "true")
> +
> + self.factory.makeDistribution(name="soss")
> +
> + repo = self.factory.makeGitRepository(
> + owner=self.team, information_type=InformationType.USERDATA
> + )
> +
> + self.factory.makeGitRefs(
> + repository=repo,
> + paths=[self.git_ref],
> + )
> +
> + self.assertRaisesWithContent(
> + UnauthorizedVulnerabilityHandler,
> + f"You don't have enough rights to use {self.handler}",
> + getUtility(IVulnerabilitySet).importData,
> + self.requester,
> + self.handler,
> + repo,
> + self.git_ref,
> + self.git_paths,
> + self.information_type,
> + import_since_commit_sha1=None,
> + )
> +
> + def test_importData_information_type_private(self):
> + """Test that we cannot create a ImportVulnerabilityJob when
> + information_type is public but the git repository is private.
> + """
> + self.useContext(feature_flags())
> + set_feature_flag(VULNERABILITY_IMPORT_ENABLED_FEATURE_FLAG, "true")
> +
> + self.factory.makeDistribution(name="soss", owner=self.requester)
> +
> + repo = self.factory.makeGitRepository(
> + owner=self.team, information_type=InformationType.USERDATA
> + )
> +
> + self.factory.makeGitRefs(
> + repository=repo,
> + paths=[self.git_ref],
> + )
> +
> + information_type = InformationType.PUBLIC
> + with person_logged_in(self.requester):
> + self.assertRaisesWithContent(
> + ValueError,
> + "information_type must be PRIVATE when importing from a "
> + "private git repository",
> + getUtility(IVulnerabilitySet).importData,
> + self.requester,
> + self.handler,
> + repo,
> + self.git_ref,
> + self.git_paths,
> + information_type,
> + import_since_commit_sha1=None,
> + )
> +
> + def test_importData_wrong_git_ref(self):
> + """Test that we cannot create a ImportVulnerabilityJob when git_ref
> is
> + not in the git repository.
> + """
> + self.useContext(feature_flags())
> + set_feature_flag(VULNERABILITY_IMPORT_ENABLED_FEATURE_FLAG, "true")
> +
> + self.factory.makeDistribution(name="soss", owner=self.requester)
> +
> + repo = self.factory.makeGitRepository(
> + owner=self.team, information_type=InformationType.USERDATA
> + )
> +
> + self.factory.makeGitRefs(
> + repository=repo,
> + paths=[self.git_ref],
> + )
> +
> + with person_logged_in(self.requester):
> + self.assertRaisesWithContent(
> + ValueError,
> + "wrong-git-ref does not exist in the specified git
> repository",
> + getUtility(IVulnerabilitySet).importData,
> + self.requester,
> + self.handler,
> + repo,
> + "wrong-git-ref",
> + self.git_paths,
> + self.information_type,
> + import_since_commit_sha1=None,
> + )
> +
> + def test_importData_wrong_import_since_commit_sha1(self):
> + """Test that we cannot create a ImportVulnerabilityJob when
> + import_since_commit_sha1 is not in git_ref.
> + """
> + self.useContext(feature_flags())
> + set_feature_flag(VULNERABILITY_IMPORT_ENABLED_FEATURE_FLAG, "true")
> +
> + self.factory.makeDistribution(name="soss", owner=self.requester)
> +
> + repo = self.factory.makeGitRepository(
> + owner=self.team, information_type=InformationType.USERDATA
> + )
> +
> + self.factory.makeGitRefs(
> + repository=repo,
> + paths=[self.git_ref],
> + )
> +
> + import_since_commit_sha1 = "1" * 40
> + with person_logged_in(self.requester):
> + self.assertRaisesWithContent(
> + ValueError,
> + f"{import_since_commit_sha1} does not exist in "
> + f"{self.git_ref}",
> + getUtility(IVulnerabilitySet).importData,
> + self.requester,
> + self.handler,
> + repo,
> + self.git_ref,
> + self.git_paths,
> + self.information_type,
> + import_since_commit_sha1,
> + )
Can we add one that checks that if a job already exists, we can't schedule a
new one?
> +
> +
> +class TestVulnerabilitySetWebService(TestCaseWithFactory):
> + layer = DatabaseFunctionalLayer
> +
> + def setUp(self):
> + super().setUp()
> + self.vulnerability_set = getUtility(IVulnerabilitySet)
> + self.vulnerability_set_url = api_url(self.vulnerability_set)
> + self.requester = self.factory.makePerson()
> + self.handler = VulnerabilityHandlerEnum.SOSS
> + self.factory.makeDistribution(name="soss", owner=self.requester)
> + self.team = self.factory.makeTeam(members=[self.requester])
> + self.git_paths = ["cves"]
> + self.information_type = InformationType.PROPRIETARY
> + self.git_ref = "ref/heads/main"
> +
> + def test_importData_webservice_required_arguments_missing(self):
> + """Test that importData webservice requires missing arguments."""
> + webservice = webservice_for_person(
> + self.requester,
> + default_api_version="devel",
> + )
> + response = webservice.named_post(
> + self.vulnerability_set_url,
> + "importData",
> + )
> + self.assertEqual(400, response.status)
> + self.assertEqual(
> + {
> + "git_paths: Required input is missing.",
> + "git_ref: Required input is missing.",
> + "git_repository: Required input is missing.",
> + "handler: Required input is missing.",
> + },
> + set(response.body.decode().split("\n")),
> + )
> +
> + def test_importData_webservice(self):
> + """Test that we can create a ImportVulnerabilityJob using the
> + webservice.
> + """
> + self.useContext(feature_flags())
> + set_feature_flag(VULNERABILITY_IMPORT_ENABLED_FEATURE_FLAG, "true")
> +
> + repo = self.factory.makeGitRepository(owner=self.team)
> + repo_url = api_url(repo)
> +
> + self.factory.makeGitRefs(
> + repository=repo,
> + paths=[self.git_ref],
> + )
> +
> + webservice = webservice_for_person(
> + self.requester,
> + default_api_version="devel",
> + )
> + response = webservice.named_post(
> + self.vulnerability_set_url,
> + "importData",
> + handler=self.handler.title,
> + git_repository=repo_url,
> + git_ref=self.git_ref,
> + git_paths=self.git_paths,
> + information_type=self.information_type.title,
> + import_since_commit_sha1=None,
> + )
> + self.assertEqual(200, response.status)
> +
> + def test_importData_webservice_feature_flag_disabled(self):
> + """Test that we cannot create a ImportVulnerabilityJob using the
> + webservice when the vulnerability import API is disabled.
> + """
> + repo = self.factory.makeGitRepository(owner=self.team)
> + repo_url = api_url(repo)
> +
> + self.factory.makeGitRefs(
> + repository=repo,
> + paths=[self.git_ref],
> + )
> +
> + webservice = webservice_for_person(
> + self.requester,
> + default_api_version="devel",
> + )
> + response = webservice.named_post(
> + self.vulnerability_set_url,
> + "importData",
> + handler=self.handler.title,
> + git_repository=repo_url,
> + git_ref=self.git_ref,
> + git_paths=self.git_paths,
> + information_type=self.information_type.title,
> + import_since_commit_sha1=None,
> + )
> + self.assertEqual(500, response.status)
> + self.assertEqual(
> + "Vulnerability import API is currently disabled",
> + response.body.decode().split("\n")[0],
> + )
> +
> + def test_importData_webservice_git_repo_unauthorized(self):
> + """Test that we cannot create a ImportVulnerabilityJob using the
> + webservice when the git repository is not visible by the user.
> + """
> + self.useContext(feature_flags())
> + set_feature_flag(VULNERABILITY_IMPORT_ENABLED_FEATURE_FLAG, "true")
> +
> + owner = self.factory.makePerson()
> + repo = self.factory.makeGitRepository(
> + owner=owner,
> + information_type=InformationType.PRIVATESECURITY,
> + )
> + with person_logged_in(owner):
> + repo_url = api_url(repo)
> +
> + self.factory.makeGitRefs(
> + repository=repo,
> + paths=[self.git_ref],
> + )
> +
> + webservice = webservice_for_person(
> + self.requester,
> + default_api_version="devel",
> + )
> + response = webservice.named_post(
> + self.vulnerability_set_url,
> + "importData",
> + handler=self.handler.title,
> + git_repository=repo_url,
> + git_ref=self.git_ref,
> + git_paths=self.git_paths,
> + information_type=self.information_type.title,
> + import_since_commit_sha1=None,
> + )
> +
> + # 401 Unauthorized
> + self.assertEqual(401, response.status)
> +
> + def test_exportData_webservice_required_arguments_missing(self):
> + """Test that exportData webservice requires missing arguments."""
> + webservice = webservice_for_person(
> + self.requester,
> + default_api_version="devel",
> + )
> + response = webservice.named_post(
> + self.vulnerability_set_url,
> + "exportData",
> + )
> + self.assertEqual(400, response.status)
> + self.assertEqual(
> + {
> + "handler: Required input is missing.",
> + },
> + set(response.body.decode().split("\n")),
> + )
> diff --git a/lib/lp/bugs/model/vulnerability.py
> b/lib/lp/bugs/model/vulnerability.py
> index c33cc0f..4d4ba07 100644
> --- a/lib/lp/bugs/model/vulnerability.py
> +++ b/lib/lp/bugs/model/vulnerability.py
> @@ -382,6 +392,96 @@ class VulnerabilitySet:
> clauses.append(get_vulnerability_privacy_filter(visible_by_user))
> return IStore(Vulnerability).find(Vulnerability, *clauses)
>
> + def importData(
> + self,
> + requester,
> + handler,
> + git_repository,
> + git_ref,
> + git_paths,
> + information_type,
> + import_since_commit_sha1,
> + ):
> + """See `IVulnerabilitySet`."""
> +
> + if not getFeatureFlag(VULNERABILITY_IMPORT_ENABLED_FEATURE_FLAG):
> + raise NotImplementedError(
> + "Vulnerability import API is currently disabled"
> + )
> +
> + # Launchpad's object permissions automatically check requester's
> + # permissions to git repo
> +
> + # Check requester's permissions to handler
> + from lp.bugs.enums import VulnerabilityHandlerEnum
Is this due to circular import?
> + from lp.bugs.scripts.soss.sossimport import SOSSImporter
> +
> + if handler == VulnerabilityHandlerEnum.SOSS:
> + importer = SOSSImporter()
> + else:
> + raise Exception("Handler not found")
> +
> + if not importer.checkUserPermissions(requester):
> + raise UnauthorizedVulnerabilityHandler(
> + f"You don't have enough rights to use {handler}"
> + )
> +
> + # If repository is private, information_type must be PRIVATE
> + if (
> + git_repository.private
> + and information_type in PUBLIC_INFORMATION_TYPES
> + ):
> + # TODO check what's usually the validation error codes we use
Let's get this TODO done before merging
> + raise ValueError(
> + "information_type must be PRIVATE when importing from a "
> + "private git repository"
> + )
> +
> + repo_ref = git_repository.getRefByPath(git_ref)
> + if repo_ref is None:
> + raise ValueError(
> + f"{git_ref} does not exist in the specified git repository"
> + )
> +
> + # Check import_since_commit_sha1 exists in git_ref
> + if import_since_commit_sha1 and not git_repository.checkCommitInRef(
> + import_since_commit_sha1, git_ref
> + ):
> + raise ValueError(
> + f"{import_since_commit_sha1} does not exist in {git_ref}"
> + )
> +
> + # Trigger the import job after validations pass
> + job_source = getUtility(IImportVulnerabilityJobSource)
> + job_source.create(
I think we should check if there is already a job created for that handler and
job type. If this is checked while we create it, maybe we want to catch that
error and return something more consistent with the other errors above?
We might also want to return the existing job, although I'd do that once we
have a defined way of displaying import/export data as part of the ticket Ílker
is currently working on
> + handler,
> + git_repository.git_https_url,
> + git_ref,
> + git_paths,
> + information_type.value,
> + import_since_commit_sha1,
> + )
> +
> + return None
> +
> + def exportData(
> + self,
> + requester,
> + handler,
> + sources,
> + ):
> + """See `IVulnerabilitySet`."""
> +
> + raise NotImplementedError(
> + "Vulnerability export API is currently disabled"
> + )
> +
> + return None
> +
> + def empty_list(self):
> + """See `IVulnerabilitySet`."""
> + return []
> +
>
> @implementer(IVulnerabilityActivity)
> class VulnerabilityActivity(StormBase):
> diff --git a/lib/lp/code/model/tests/test_gitrepository.py
> b/lib/lp/code/model/tests/test_gitrepository.py
> index 4e48291..62f50be 100644
> --- a/lib/lp/code/model/tests/test_gitrepository.py
> +++ b/lib/lp/code/model/tests/test_gitrepository.py
> @@ -2560,6 +2560,44 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
> hosting_fixture.getRefs.extract_kwargs(),
> )
>
> + master_sha1 = hashlib.sha1(b"refs/heads/master").hexdigest()
> + author = self.factory.makePerson()
> + with person_logged_in(author):
> + author_email = author.preferredemail.email
> + author_date = datetime(2015, 1, 1, tzinfo=timezone.utc)
> + committer_date = datetime(2015, 1, 2, tzinfo=timezone.utc)
> + self.useFixture(
> + GitHostingFixture(
> + commits=[
> + {
> + "sha1": master_sha1,
> + "message": "tip of master",
> + "author": {
> + "name": author.displayname,
> + "email": author_email,
> + "time": int(seconds_since_epoch(author_date)),
> + },
> + "committer": {
> + "name": "New Person",
> + "email": "new-per...@example.org",
> + "time": int(seconds_since_epoch(committer_date)),
> + },
> + "parents": [],
> + "tree": hashlib.sha1(b"").hexdigest(),
> + }
> + ]
> + )
> + )
> +
> + def test_checkCommitInRef(self):
I'd add a 2 similar tests that check:
1. the commit doesn't exist at all
2. the commit exists in the repo but not in the ref
> + repository = self.factory.makeGitRepository()
> + paths = ["refs/heads/master"]
> + ref = self.factory.makeGitRefs(repository=repository, paths=paths)
> + result = repository.checkCommitInRef(
> + ref[0].commit_sha1, "refs/heads/master"
> + )
> + self.assertEqual(result, True)
> +
> def test_fetchRefCommits(self):
> # fetchRefCommits fetches detailed tip commit metadata for the
> # requested refs.
--
https://code.launchpad.net/~enriqueesanchz/launchpad/+git/launchpad/+merge/491584
Your team Launchpad code reviewers is requested to review the proposed merge of
~enriqueesanchz/launchpad:add-import-export-endpoints into launchpad:master.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
