launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



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)

We should probably also check that the location matches.

It's also slightly confusing that verifyMacaroon doesn't call checkMacaroonIssuer -- seems like a recipe for mistakes later.

> +        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

This case is only used in tests. Would it be better to refactor this slightly so we didn't have the weird unused alternate case and tests that don't test the normal path?

> +        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

Cunning. Undebuggable and fragile, but it'll work for now and hopefully nobody will touch it in inappropriate ways before we have a more generic mechanism.

> 
> === modified file 'lib/lp/code/xmlrpc/git.py'
> --- lib/lp/code/xmlrpc/git.py	2016-10-06 20:37:39 +0000
> +++ lib/lp/code/xmlrpc/git.py	2016-10-07 15:56:06 +0000
> @@ -68,22 +73,47 @@
>          super(GitAPI, self).__init__(*args, **kwargs)
>          self.repository_set = getUtility(IGitRepositorySet)
>  
> -    def _performLookup(self, path):
> +    def _verifyMacaroon(self, macaroon_raw, repository=None):
> +        try:
> +            macaroon = Macaroon.deserialize(macaroon_raw)
> +        except Exception:
> +            return False
> +        try:
> +            issuer = getUtility(IMacaroonIssuer, macaroon.identifier)
> +        except ComponentLookupError:
> +            return False
> +        if repository is not None:
> +            return issuer.verifyMacaroon(macaroon, repository)
> +        else:
> +            return issuer.checkMacaroonIssuer(macaroon)
> +
> +    def _performLookup(self, requester, path, auth_params):

The new "requester" parameter seems unused.

>          repository, extra_path = getUtility(IGitLookup).getByPath(path)
>          if repository is None:
>              return None
> -        try:
> -            hosting_path = repository.getInternalPath()
> -        except Unauthorized:
> -            return None
> -        writable = (
> -            repository.repository_type == GitRepositoryType.HOSTED and
> -            check_permission("launchpad.Edit", repository))
> +        macaroon_raw = auth_params.get("macaroon")
> +        naked_repository = removeSecurityProxy(repository)
> +        if (macaroon_raw is not None and
> +                self._verifyMacaroon(macaroon_raw, naked_repository)):
> +            # The authentication parameters specifically grant access to
> +            # this repository, so we can bypass other checks.
> +            hosting_path = naked_repository.getInternalPath()
> +            writable = True
> +            private = naked_repository.private

I wouldn't half mind a quick assertion here that repository_type == IMPORTED. Makes the intent of the code clearer, and also hints that the infrastructure is not quite generic enough yet to safely reuse without substantial thought.

> +        else:
> +            try:
> +                hosting_path = repository.getInternalPath()
> +            except Unauthorized:
> +                return None
> +            writable = (
> +                repository.repository_type == GitRepositoryType.HOSTED and
> +                check_permission("launchpad.Edit", repository))
> +            private = repository.private
>          return {
>              "path": hosting_path,
>              "writable": writable,
>              "trailing": extra_path,
> -            "private": repository.private,
> +            "private": private,
>              }
>  
>      def _getGitNamespaceExtras(self, path, requester):


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