launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/refactor-code-import-macaroons into lp:launchpad with lp:~cjwatson/launchpad/refactor-macaroon-testing as a prerequisite.

Commit message:
Push repository -> job inference down from GitAPI to the macaroon issuer.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/refactor-code-import-macaroons/+merge/365871

I think this makes slightly more logical sense, and it will shortly make it easier to cope with the case where a different macaroon issuer might be involved.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/refactor-code-import-macaroons into lp:launchpad.

=== modified file 'lib/lp/code/model/codeimportjob.py'
--- lib/lp/code/model/codeimportjob.py	2019-04-11 17:02:00 +0000
+++ lib/lp/code/model/codeimportjob.py	2019-04-11 17:02:01 +0000
@@ -27,6 +27,7 @@
     CodeImportMachineState,
     CodeImportResultStatus,
     CodeImportReviewStatus,
+    GitRepositoryType,
     RevisionControlSystems,
     )
 from lp.code.interfaces.branch import IBranch
@@ -34,6 +35,7 @@
     branch_id_alias,
     compose_public_url,
     )
+from lp.code.interfaces.codeimport import ICodeImportSet
 from lp.code.interfaces.codeimportevent import ICodeImportEventSet
 from lp.code.interfaces.codeimportjob import (
     ICodeImportJob,
@@ -43,6 +45,7 @@
     )
 from lp.code.interfaces.codeimportmachine import ICodeImportMachineSet
 from lp.code.interfaces.codeimportresult import ICodeImportResultSet
+from lp.code.interfaces.gitrepository import IGitRepository
 from lp.code.model.codeimportresult import CodeImportResult
 from lp.registry.interfaces.person import validate_public_person
 from lp.services.config import config
@@ -429,7 +432,22 @@
         return context.id
 
     def checkVerificationContext(self, context):
-        """See `MacaroonIssuerBase`."""
+        """See `MacaroonIssuerBase`.
+
+        For verification, the context may be an `ICodeImportJob`, in which
+        case we check that the context job is currently running; or it may
+        be an `IGitRepository`, in which case we check that the repository
+        is an imported repository with an associated code import, and then
+        perform the previously-stated check on its code import job.
+        """
+        if IGitRepository.providedBy(context):
+            if context.repository_type != GitRepositoryType.IMPORTED:
+                raise ValueError("%r is not an IMPORTED repository." % context)
+            code_import = getUtility(ICodeImportSet).getByGitRepository(
+                context)
+            if code_import is None:
+                raise ValueError("%r does not have a code import." % context)
+            context = code_import.import_job
         if not ICodeImportJob.providedBy(context):
             raise ValueError("Cannot handle context %r." % context)
         if context.state != CodeImportJobState.RUNNING:

=== modified file 'lib/lp/code/model/tests/test_codeimportjob.py'
--- lib/lp/code/model/tests/test_codeimportjob.py	2019-04-11 17:02:00 +0000
+++ lib/lp/code/model/tests/test_codeimportjob.py	2019-04-11 17:02:01 +0000
@@ -34,6 +34,7 @@
     CodeImportJobState,
     CodeImportResultStatus,
     CodeImportReviewStatus,
+    GitRepositoryType,
     TargetRevisionControlSystems,
     )
 from lp.code.interfaces.codehosting import (
@@ -1323,6 +1324,7 @@
         getUtility(ICodeImportJobWorkflow).startJob(job, machine)
         macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
         self.assertMacaroonVerifies(issuer, macaroon, job)
+        self.assertMacaroonVerifies(issuer, macaroon, job.code_import.target)
 
     def test_verifyMacaroon_good_no_context(self):
         machine = self.factory.makeCodeImportMachine(set_online=True)
@@ -1332,6 +1334,8 @@
         macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
         self.assertMacaroonVerifies(
             issuer, macaroon, job, require_context=False)
+        self.assertMacaroonVerifies(
+            issuer, macaroon, job.code_import.target, require_context=False)
 
     def test_verifyMacaroon_no_context_but_require_context(self):
         machine = self.factory.makeCodeImportMachine(set_online=True)
@@ -1371,6 +1375,25 @@
             ["Signatures do not match."],
             issuer, macaroon, job, require_context=False)
 
+    def test_verifyMacaroon_hosted_repository(self):
+        job = self.makeJob()
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
+        repository = self.factory.makeGitRepository()
+        self.assertMacaroonDoesNotVerify(
+            ["%r is not an IMPORTED repository." % repository],
+            issuer, macaroon, repository)
+
+    def test_verifyMacaroon_repository_with_no_code_import(self):
+        job = self.makeJob()
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
+        repository = self.factory.makeGitRepository(
+            repository_type=GitRepositoryType.IMPORTED)
+        self.assertMacaroonDoesNotVerify(
+            ["%r does not have a code import." % repository],
+            issuer, macaroon, repository)
+
     def test_verifyMacaroon_not_running(self):
         job = self.makeJob()
         issuer = getUtility(IMacaroonIssuer, "code-import-job")
@@ -1378,6 +1401,9 @@
         self.assertMacaroonDoesNotVerify(
             ["%r is not in the RUNNING state." % job],
             issuer, macaroon, job)
+        self.assertMacaroonDoesNotVerify(
+            ["%r is not in the RUNNING state." % job],
+            issuer, macaroon, job.code_import.target)
 
     def test_verifyMacaroon_wrong_job(self):
         machine = self.factory.makeCodeImportMachine(set_online=True)
@@ -1390,3 +1416,7 @@
             ["Caveat check for 'lp.code-import-job %s' failed." %
              other_job.id],
             issuer, macaroon, job)
+        self.assertMacaroonDoesNotVerify(
+            ["Caveat check for 'lp.code-import-job %s' failed." %
+             other_job.id],
+            issuer, macaroon, job.code_import.target)

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2019-04-11 17:02:00 +0000
+++ lib/lp/code/xmlrpc/git.py	2019-04-11 17:02:01 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementations of the XML-RPC APIs for Git."""
@@ -40,7 +40,6 @@
     InvalidNamespace,
     )
 from lp.code.interfaces.codehosting import LAUNCHPAD_ANONYMOUS
-from lp.code.interfaces.codeimport import ICodeImportSet
 from lp.code.interfaces.gitapi import IGitAPI
 from lp.code.interfaces.githosting import IGitHostingClient
 from lp.code.interfaces.gitjob import IGitRefScanJobSource
@@ -89,19 +88,8 @@
             issuer = getUtility(IMacaroonIssuer, macaroon.identifier)
         except ComponentLookupError:
             return False
-        if repository is not None:
-            if repository.repository_type != GitRepositoryType.IMPORTED:
-                return False
-            code_import = getUtility(ICodeImportSet).getByGitRepository(
-                repository)
-            if code_import is None:
-                return False
-            job = code_import.import_job
-            if job is None:
-                return False
-        else:
-            job = None
-        return issuer.verifyMacaroon(macaroon, job, require_context=False)
+        return issuer.verifyMacaroon(
+            macaroon, repository, require_context=False)
 
     def _performLookup(self, requester, path, auth_params):
         repository, extra_path = getUtility(IGitLookup).getByPath(path)