launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> 
> === modified file 'lib/lp/code/xmlrpc/git.py'
> --- lib/lp/code/xmlrpc/git.py	2019-05-09 15:44:59 +0000
> +++ lib/lp/code/xmlrpc/git.py	2019-05-10 16:51:16 +0000
> @@ -390,17 +397,18 @@
>          getUtility(IGitRefScanJobSource).create(
>              removeSecurityProxy(repository))
>  
> +    @return_fault
>      def authenticateWithPassword(self, username, password):
>          """See `IGitAPI`."""
> -        # XXX cjwatson 2016-10-06: We only support free-floating macaroons
> -        # at the moment, not ones bound to a user.
> -        if not username:
> -            verified = self._verifyMacaroon(password)
> -            if verified:
> -                auth_params = {"macaroon": password}
> -                if _is_issuer_internal(verified):
> -                    auth_params["user"] = LAUNCHPAD_SERVICES
> -                return auth_params
> +        user = getUtility(IPersonSet).getByName(username) if username else None
> +        verified = self._verifyMacaroon(password, user=user)
> +        if verified:
> +            auth_params = {"macaroon": password}
> +            if user is not None:
> +                auth_params["uid"] = user.id

We discussed the matter of auth parameters in this week's team meeting.

I agree that it was too easy to get things wrong as it stood, and I've spent some time thinking about how to improve things.  I don't see a way in which an extra auth parameter would particularly help matters.  To my mind the problem was more that the verification rules were too scattered and interwoven with the details of the two XML-RPC methods that need to verify macaroons (translatePath and checkRefPermissions).  I've therefore pulled them out to a _verifyAuthParams method which has a much clearer contract, so the only thing that the two call sites need to do is deal with their specific rules for granting access to verified internal services.

> +            elif _is_issuer_internal(verified):
> +                auth_params["user"] = LAUNCHPAD_SERVICES
> +            return auth_params
>          # Only macaroons are supported for password authentication.
>          return faults.Unauthorized()
>  


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