launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



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

Nothing actually enforces that the user was checked as part of the verification. The CodeImportJob verifyPrimaryCaveat checks that it isn't there, but SnapBuildJob's doesn't. I think we need to ensure the user was verified and matches the input somewhere around this level to avoid mistakes.

I also wonder if we want to set an extra auth parameter when the user is authenticated by something other than a macaroon, to avoid macaroon being subtractive from user/uid.

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