launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

On 09/05/18 23:24, Colin Watson wrote:
> I don't understand the principle here.  Can you explain why it's any
> more of a layering violation for a BPB issuer to grant permission to
> access its associated LFAs than it is for a CodeImportJob issuer to
> grant permission to access its associated GitRepositories?

It was less obviously wrong there because the GitRepository
authorisation code already touched CodeImport and they were part of the
same Launchpad application. But in this case we have the librarian
touching complex bits of the Soyuz schema, which is ugly from a code
structure perspective and also means we have to be very careful around
synchronising schema changes and librarian deployments.

librarian today only examines non-librarian tables when GC introspects
the schema to work out which FKs there are to LFA.id, and that doesn't
seem like an architectural separation that we should abandon lightly.

> In general, my intent in designing the IMacaroonIssuer interface was
> that various subsystems could grant the specific permissions they
> needed, and the verifying code could then use the
> getUtility(IMacaroonIssuer, token.identifier) idiom to get the
> appropriate verifier without having to have specific knowledge of
> what the verifier in question does.  Apart from the security.cfg
> changes, nothing about the librarian changes here is at all specific
> to BPBs, and it would be perfectly possible to write a different
> verifier that grants access based on some other criteria;
> furthermore, if (hypothetically) a BPB needed access to some other
> private resource, the same macaroon could be used for both by
> generalising the verifier.

I agree that sharing macaroons would be ideal, but having the librarian
itself validate macaroons that may require arbitrarily broad and complex
database logic (particularly given the librarian is a Twisted app which
really shouldn't be doing synchronous DB calls at all) doesn't seem right.


-- 
https://code.launchpad.net/~cjwatson/launchpad/librarian-accept-macaroon/+merge/345079
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.