launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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.")

Fair enough; fixed by giving this kwargs which can be issuer-dependent, as I also recently did with verifyMacaroon.  When I merge this up I'll change GitAPI to pass in the user that it already has in hand.

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

I think it's reasonable to make this more explicit, but lp.principal.person.openid-identifier seems a bit pedantic, and it isn't clear why e.g. we wouldn't have "account" in there too.  "lp.principal.openid-identifier" feels good enough to me, so I've gone with that.

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

The problem with isoformat is that it has no inverse in the standard library.  You have to either use more general parsers like dateutil, which I'd rather not do for something on a security boundary, or use strptime with a matching format string, at which point you might as well just use your own format string anyway.  Given that, the extra resolution is more or less free.

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

Right, if we need that then we can change this issuer to accept an Account rather than (or alternatively to) a Person, but that will be an entirely internal change and for now the XML-RPC run_with_login apparatus insists on being able to find a Person anyway.

However, I've at least changed the comparison around to avoid the adapter.

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