Code review comment for lp:~cjwatson/launchpad/librarian-accept-macaroon

Revision history for this message
Colin Watson (cjwatson) 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?

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.

The only thing here that feels at all like a layering violation to me is that BPBMacaroonIssuer.verifyMacaroon takes an LFA ID rather than a build. But, firstly, that's something that it's free to generalise later without reference to the librarian (although I suppose that would be easier if it took an actual LFA; that would require a little bit of extra work in the librarian to avoid an extra query), and, secondly, it's OK for BPBs to know about LFAs, just not vice versa.

« Back to merge proposal