On Fri, 2010-07-23 at 12:42 +0000, Gary Poster wrote: > Hey Salgado. Thank you. Below, I've deleted sections for which I thought no further discussion was necessary. > > On Jul 22, 2010, at 5:15 AM, Guilherme Salgado wrote: > > > On Wed, 2010-07-21 at 17:08 +0000, Gary Poster wrote: > >> Hi Salgado. Very cool. Thank you. Comments below. > >> > >> Requested: > >> > >> - Let's put the changes from > >> lib/canonical/launchpad/webapp/configure.zcml into > >> apidoc-configure-normal.zcml. > > > > Good catch. Done. > > I don't understand why you didn't also move this: > + + component="canonical.launchpad.systemhomes.apidocroot" > + provides="canonical.launchpad.webapp.interfaces.IAPIDocRoot" /> > Was it an oversight, or is there a reason to keep it where it is? Yep, oversight. Moved. > > > I'm now wondering if I should do the same for all the eggs that I've > > updated as part of this -- they could also register new things that > > we're not expecting. > > The reason why I singled in on this one is that it is registered in > zopeapp.zcml, so it will affect production. The rest will only affect > tests and development, so I'm less concerned in this regard. > > zope.publisher's update I guess is worth a look, actually. But that's > the only other one I see. > http://paste.ubuntu.com/467986/ is the diff between the previous configure.zcml and the current version, so nothing to worry about, I think? > > - FWIW, LaunchpadPrincipalAnnotations could subclass UserDict.UserDict > >> and be shortened to the __init__ plus a call to the shared constructor > >> and the interface declarations. Do it if you want. > > > > I'm all for it! > > > > Also, I thought I could make apidoc.lp.dev work for anonymous users by > > just registering an adapter like the above for > > IUnauthenticatedPrincipal, but then I got > > http://paste.ubuntu.com/467401/ > > Meh, digging into it just a bit, I see that preference group thing is > doing somewhat unusual security dances. I wouldn't be all that > surprised if that LocationError were actually hiding a > ForbiddenAttribute on a value that should be there, but maybe I'm > wrong. > > > I'm thinking that it may be better to actually require a logged-in user > > to use apidoc.lp.dev and ask them to login in case they're not, just > > like we do on other launchpad.AnyPerson pages. What do you think? > > Since this is only for devel instances, in the abstract there's no > reason not to expose this for anonymous users that I see. That will > also make it easier to make a static version of the apidoc, which I > think is part of the plan. Agreed. All I wanted was to make sure we require people to login as early as possible so that they don't see these obscure errors that one has no idea can be worked around just by logging in. > > If you want me to look into the traceback further just ask. I'd really appreciate as I wouldn't know where to start. Just some pointers so I can get started would be great. > > ... > > >> - I have said that I was fine with the change to authorization.py, but > >> actually I had another idea. I'm not 100% convinced the new idea is > >> better, but it is simpler, at least. The idea is to revert all > >> current changes from that file, and then add ``from zope.proxy import > >> removeAllProxies`` to the top. Then, change ``objecttoauthorize = > >> removeSecurityProxy(objecttoauthorize)`` to ``objecttoauthorize = > >> removeAllProxies(objecttoauthorize)``. This will remove the > >> LocationProxy, as well as all security proxies, as well as any other > >> zope.proxy-based proxies. What do you think? The upside is > >> simplicity. The downside is that this is a bigger hammer than we > >> precisely need. The counterargument against the downside is that the > >> big hammer is somewhat reasonable in context: we can't use these > >> proxies for weakrefs. The counterargument is convincing to me, and I > >> lean towards doing this, myself, with a comment explaining that we are > >> removing both security proxies and location proxies, which are only > >> used by the apidoc. Do what you will: I'm reasonably happy with what > >> you have now, if you don't like my new suggestion. > > > > I like your suggestion, so I did it. > > Cool. I do think that the comment there ("# Remove security proxies > from object to authorize.") should be changed to clarify what's going > on (that is, why we are not using removeSecurityProxy as we usually > do). I've noticed that earlier today and have that comment changed, although I haven't committed it yet. > > ... > > So to summarize: > > Requested: > > - Please also move the utility registration to the apidoc zcml, or tell me why it should stay where it is ("things break when I move it" is good enough at this stage). > > - Please update the comment in authorization.py to explain why we are using removeAllProxies. > > - Please look at the diff between zope.publisher 3.10.0 and 3.12.0. Done all of them. > > Nice to have: > > - See if you can come up with an easy way to keep the apidoc > registrations out of every test but the smoke test. This would be > worth talking with Benji about: I may be forgetting some easy > approach. > Ok, I'll check with him. > - Make devel instances available to anonymous users, in order to ease > creating the static copy. I'll try to help if you like. > I'd like to have this fixed before landing, so as said above I'd appreciate your help. > I'll approve merge-conditional on the requested stuff. > > Thanks > > Gary -- Guilherme Salgado