launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> diff --git a/lib/lp/oci/model/ocirecipebuild.py b/lib/lp/oci/model/ocirecipebuild.py
> index c38bfa5..2e41955 100644
> --- a/lib/lp/oci/model/ocirecipebuild.py
> +++ b/lib/lp/oci/model/ocirecipebuild.py
> @@ -253,6 +263,11 @@ class OCIRecipeBuild(PackageBuildMixin, Storm):
>              ]
>          return self.status in cancellable_statuses
>  
> +    @property
> +    def is_private(self):
> +        """See `IBuildFarmJob`."""
> +        return self.recipe.git_ref.private

This is going to be a bit of a problem for the same sorts of reasons seen in https://code.launchpad.net/~cjwatson/launchpad/snap-build-record-code/+merge/365356: the git repository will need to be detachable to make deleting repositories viable.  Even before that, OCIRecipe.git_repository may already be None.

We'll need to add various privacy bits to the DB model to make this work properly.  In the meantime, maybe something like `self.recipe.owner.private or (self.recipe.git_repository is None or self.recipe.git_repository.private)` with an XXX comment saying that we'll need an OCIRecipeBuild.explicitly_private flag to make detaching git repositories safe?

> +
>      def retry(self):
>          """See `IOCIRecipeBuild`."""
>          assert self.can_be_retried, "Build %s cannot be retried" % self.id
> @@ -578,3 +593,62 @@ class OCIRecipeBuildSet(SpecificBuildFarmJobSourceMixin):
>              OCIRecipeBuild, OCIRecipeBuild.build_farm_job_id.is_in(
>                  bfj.id for bfj in build_farm_jobs))
>          return DecoratedResultSet(rows, pre_iter_hook=self.preloadBuildsData)
> +
> +
> +@implementer(IMacaroonIssuer)
> +class OCIRecipeBuildMacaroonIssuer(MacaroonIssuerBase):
> +
> +    identifier = "ocirecipe-build"

I'd slightly prefer "oci-recipe-build".

> +    issuable_via_authserver = True
> +
> +    def checkIssuingContext(self, context, **kwargs):
> +        """See `MacaroonIssuerBase`.
> +
> +        For issuing, the context is an `IOCIRecipeBuild`.
> +        """
> +        if not IOCIRecipeBuild.providedBy(context):
> +            raise BadMacaroonContext(context)
> +        if not removeSecurityProxy(context).is_private:
> +            raise BadMacaroonContext(
> +                context, "Refusing to issue macaroon for public build.")
> +        return removeSecurityProxy(context).id
> +
> +    def checkVerificationContext(self, context, **kwargs):
> +        """See `MacaroonIssuerBase`."""
> +        if not IGitRepository.providedBy(context):
> +            raise BadMacaroonContext(context)
> +        return context
> +
> +    def verifyPrimaryCaveat(self, verified, caveat_value, context, user=None,
> +                            **kwargs):
> +        """See `MacaroonIssuerBase`.
> +
> +        For verification, the context is an `IGitRepository`.  We check that
> +        the repository is needed to build the `IOCIRecipeBuild` that is the
> +        context of the macaroon, and that the context build is currently
> +        building.
> +        """
> +        # Circular import.
> +        from lp.oci.model.ocirecipe import OCIRecipe
> +
> +        # OCIRecipeBuild builds only support free-floating macaroons for Git
> +        # authentication, not ones bound to a user.
> +        if user:
> +            return False
> +        verified.user = NO_USER
> +
> +        if context is None:
> +            # We're only verifying that the macaroon could be valid for some
> +            # context.
> +            return True
> +
> +        try:
> +            build_id = int(caveat_value)
> +        except ValueError:
> +            return False
> +        return not IStore(OCIRecipeBuild).find(
> +            OCIRecipeBuild,
> +            OCIRecipeBuild.id == build_id,
> +            OCIRecipeBuild.recipe_id == OCIRecipe.id,
> +            OCIRecipe.git_repository == context,
> +            OCIRecipeBuild.status == BuildStatus.BUILDING).is_empty()


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/396932
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:ocirecipebuild-macaroon-issuer.