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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
