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.
As the actual implementation works, if this is None it tells you that the
parameter is required.
In the `ImportVulnerabilityJob` we are checking that the
`files_to_import.startswith()` with one of the `paths` listed in `git_paths`.
So if we set git_paths=[""], the empty string will make a import of all the
files.
We can set up the default to the empty script so this will work as you said, do
we want it? Maybe the actual implementation makes the user be conscious about
what he is making and he does not forgot that the `git_paths` exists.
What do you think? For sure we need to document it.
> + :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,
> + )
Good catch, tested it on the tests from `ImportVulnerabilityJob` but we should
test it also here :) Done.
> +
> +
> +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
Yes, forgot to add a comment. Indeed the circular one is the SOSSImporter. At
first it seems that the circular import is only caused by importing classes to
use python typing. As you dig into it you finally need to do the import here.
Changed it.
> + 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
Any clue on where should I look for this? Didn't find it.
> + 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(
What should that Exception look like if it is not the
`VulnerabilityJobInProgress` that we are currently raising.
Agree on returning the job. Do we need to create a ticket for this?
> + 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):
Done. Forgot to add them before, thank you! :D
> + 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
