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.

_______________________________________________
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

Reply via email to