launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/code/model/gitrepository.py'
> --- lib/lp/code/model/gitrepository.py	2019-04-02 08:12:27 +0000
> +++ lib/lp/code/model/gitrepository.py	2019-04-15 11:35:18 +0000
> @@ -1764,6 +1772,98 @@
>              repository.project_id: repository for repository in repositories}
>  
>  
> +@implementer(IMacaroonIssuer)
> +class GitRepositoryMacaroonIssuer(MacaroonIssuerBase):
> +
> +    identifier = "git-repository"
> +    allow_multiple = {"lp.expires"}
> +
> +    _timestamp_format = "%Y-%m-%dT%H:%M:%S.%f"
> +
> +    def __init__(self):
> +        super(GitRepositoryMacaroonIssuer, self).__init__()
> +        self.checkers = {
> +            "lp.openid-identifier": self.verifyOpenIDIdentifier,
> +            "lp.expires": self.verifyExpires,
> +            }
> +
> +    def checkIssuingContext(self, context):
> +        """See `MacaroonIssuerBase`.
> +
> +        For issuing, the context is an `IGitRepository`.
> +        """
> +        if not IGitRepository.providedBy(context):
> +            raise ValueError("Cannot handle context %r." % context)
> +        return context.id
> +
> +    def issueMacaroon(self, context):
> +        """See `IMacaroonIssuer`."""
> +        user = getUtility(ILaunchBag).user
> +        if user is None:
> +            raise Unauthorized(
> +                "git-repository macaroons may only be issued for a logged-in "
> +                "user.")

It's unusual for this to use LaunchBag. Probably acceptable for now, but does the issueMacaroon interface really need to be consistent across issuers?

> +        macaroon = super(GitRepositoryMacaroonIssuer, self).issueMacaroon(
> +            context)
> +        macaroon.add_first_party_caveat(
> +            "lp.openid-identifier " +
> +            user.account.openid_identifiers.any().identifier)

lp.principal.person.openid-identifier maybe, as we did for lp.principal.binary-package-build? Makes the purpose of it clearer vs. lp.git-repository.

> +        store = IStore(GitRepository)
> +        # XXX cjwatson 2019-04-09: Expire macaroons after the number of
> +        # seconds given in the code.git.access_token_expiry feature flag,
> +        # defaulting to a week.  This isn't very flexible, but for now it
> +        # saves on having to implement macaroon persistence in order that
> +        # users can revoke them.
> +        expiry_seconds_str = getFeatureFlag("code.git.access_token_expiry")
> +        if expiry_seconds_str is None:
> +            expiry_seconds = 60 * 60 * 24 * 7
> +        else:
> +            expiry_seconds = int(expiry_seconds_str)
> +        expiry = (
> +            get_transaction_timestamp(store) +
> +            timedelta(seconds=expiry_seconds))
> +        macaroon.add_first_party_caveat(
> +            "lp.expires " + expiry.strftime(self._timestamp_format))

Is the additional resolution worth not being able to use isoformat()?

> +        return macaroon
> +
> +    def checkVerificationContext(self, context, **kwargs):
> +        """See `MacaroonIssuerBase`.
> +
> +        For verification, the context is an `IGitRepository`.
> +        """
> +        if not IGitRepository.providedBy(context):
> +            raise ValueError("Cannot handle context %r." % context)
> +        return context
> +
> +    def verifyPrimaryCaveat(self, caveat_value, context, **kwargs):
> +        """See `MacaroonIssuerBase`."""
> +        if context is None:
> +            # We're only verifying that the macaroon could be valid for some
> +            # context.
> +            return True
> +        return caveat_value == str(context.id)
> +
> +    def verifyOpenIDIdentifier(self, caveat_value, context, **kwargs):
> +        """Verify an lp.openid-identifier caveat."""
> +        user = kwargs.get("user")
> +        try:
> +            account = getUtility(IAccountSet).getByOpenIDIdentifier(
> +                caveat_value)
> +        except LookupError:
> +            return False
> +        return user == IPerson(account)

I'm not sure it's worth requiring a username here; Canonical's other macaroons don't tend to require it. But I guess it doesn't hurt and we can always start ignoring it later.

> +
> +    def verifyExpires(self, caveat_value, context, **kwargs):
> +        """Verify an lp.expires caveat."""
> +        try:
> +            expires = datetime.strptime(
> +                caveat_value, self._timestamp_format).replace(tzinfo=pytz.UTC)
> +        except ValueError:
> +            return False
> +        store = IStore(GitRepository)
> +        return get_transaction_timestamp(store) < expires
> +
> +
>  def get_git_repository_privacy_filter(user, repository_class=GitRepository):
>      public_filter = repository_class.information_type.is_in(
>          PUBLIC_INFORMATION_TYPES)


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-issue-access-tokens/+merge/366052
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.