launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> 
> === modified file 'lib/lp/code/model/codeimportjob.py'
> --- lib/lp/code/model/codeimportjob.py	2016-10-03 17:00:56 +0000
> +++ lib/lp/code/model/codeimportjob.py	2016-10-07 15:56:06 +0000
> @@ -340,3 +348,57 @@
>          # 4)
>          getUtility(ICodeImportEventSet).newReclaim(
>              code_import, machine, job_id)
> +
> +
> +@implementer(IMacaroonIssuer)
> +class CodeImportJobMacaroonIssuer:
> +
> +    @property
> +    def _root_secret(self):
> +        secret = config.codeimport.macaroon_secret_key
> +        if not secret:
> +            raise RuntimeError(
> +                "codeimport.macaroon_secret_key not configured.")
> +        return secret
> +
> +    def issueMacaroon(self, context):
> +        """See `IMacaroonIssuer`."""
> +        assert context.code_import.git_repository is not None
> +        macaroon = Macaroon(
> +            location=config.vhost.mainsite.hostname,
> +            identifier="code-import-job", key=self._root_secret)
> +        macaroon.add_first_party_caveat("code-import-job %s" % context.id)
> +        return macaroon
> +
> +    def checkMacaroonIssuer(self, macaroon):
> +        """See `IMacaroonIssuer`."""
> +        try:
> +            verifier = Verifier()
> +            verifier.satisfy_general(
> +                lambda caveat: caveat.startswith("code-import-job "))
> +            return verifier.verify(macaroon, self._root_secret)
> +        except Exception:
> +            return False
> +
> +    def verifyMacaroon(self, macaroon, context):
> +        """See `IMacaroonIssuer`."""
> +        if IGitRepository.providedBy(context):
> +            if context.repository_type != GitRepositoryType.IMPORTED:
> +                return False
> +            code_import = getUtility(ICodeImportSet).getByGitRepository(
> +                context)
> +            if code_import is None:
> +                return False
> +            job = code_import.import_job
> +            if job is None:
> +                return False
> +        else:
> +            job = context

I agree it's odd, but on balance I prefer the opposite resolution from the one I think you're suggesting: I've moved the GitRepository -> CodeImportJob lookup out to the caller in GitAPI instead.  That way, CodeImportJobMacaroonIssuer operates on CodeImportJob objects and GitAPI handles the specifics of GitRepository objects, which makes a lot more sense to me.

> +        try:
> +            verifier = Verifier()
> +            verifier.satisfy_exact("code-import-job %s" % job.id)
> +            return (
> +                verifier.verify(macaroon, self._root_secret) and
> +                job.state == CodeImportJobState.RUNNING)
> +        except Exception:
> +            return False


-- 
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-auth/+merge/307968
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.