Launchpad itself

Code review comment for lp:~salgado/launchpad/apidoc

apidoc
Merge into devel

Revision history for this message

Guilherme Salgado (salgado) wrote on 2010-07-22:

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.

>
> Questions and comments:
>
> - Why is this necessary, do you know? I realize it is an existing
> code that you moved around, but neither the comment nor the "provides"
> made much sense to me, though Google did show me some hints that it
> might be standard practice for enabling the apidoc for some reason.
> Did you try removing it?
> + 
> + <meta:provides feature="devmode" />

For some reason it became necessary when I moved the zcml to
apidoc-configure-normal.zcml. I've moved the principal= part.

>
> - I'm concerned that the tests are going to be run with even more
> registrations that will not be part of production. I wish there were
> a light, easy way to start up another LP instance for the apidoc smoke
> test that then would not pollute the rest of the tests with the apidoc
> registrations in the zcml file. However, I don't know of one, and
> unless you do, I won't insist that we address this concern. It's
> certainly just slightly enlarging an existing problem, rather than
> making a new one.

I hadn't thought about that, but I'll see if I can find a way to not
make this situation any worse.

>
> - Did you audit zope.login to make sure it's registrations don't do
> something we need to be worried about from a security perspective? If
> not, one of us should. I'd feel somewhat happier if ``<include
> package="zope.login" />`` were part of the apidoc zcml, but then we're
> exacerbating the previous problem (that is, production registrations
> are more and more different from test registrations).

zope.login is now needed because the ILoginPassword adapter has been
moved there from zope.publisher, but you have a good point that it
should be audited.

I've just checked and it just registers the ILoginPassword adapters for
both IHTTPCredentials and IFTPCredentials as zope.publisher did until
version 3.10.

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.

>
> Suggested:
>
> - I suggest renaming LaunchpadPrincipalAnnotations to
> TemporaryPrincipalAnnotations: the behavior (and the fact that the
> implementation is effectively a stub) is something worth highlighting.
> Calling it "Launchpad" doesn't buy us anything.

I like your suggestion; renamed.

>
> - 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/

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?

>
> - For this comment:
> + 
> I suggest that this is not an XXX. To that point, this is not
> "Copied" but "Modified" (the "for" is different, and that's the key).

Indeed, I've changed that comment.

>
> - For this comment:
> + 
> You mentioned that these comments came from me. I think we can just
> remove the comment entirely. As I said on IRC, for the first half, I
> think I expected that this was going to have to go in overrides.
> Since it works without being in overrides, this is a "never mind...".
> For the second half ("Safer:..."), I think we're probably fine. It
> would be nice if we could easily register this only for the bots we
> care about, but, eh. It's all right, I think.

Cool, I've removed the comment.

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

>
> Things you can ignore:
>
> - For browser:menu tags in zcml, I was tempted to ask for a comment
> mentioning that these are necessary just to make apidoc's default
> registrations happy, but on reflection it's pretty clear that the
> whole zcml file is just there to make apidoc happy, so, whatever.

Indeed, there's a comment at the top of the file stating that.

>
> - I expected us to have a conditional in the zcml file for whether
> devmode is turned on, but I see that this is not necessary since it is
> only included in the development and test configuration.

Right.

--
Guilherme Salgado <https://launchpad.net/~salgado>

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.

> 
> Questions and comments:
> 
> - Why is this necessary, do you know?  I realize it is an existing
> code that you moved around, but neither the comment nor the "provides"
> made much sense to me, though Google did show me some hints that it
> might be standard practice for enabling the apidoc for some reason.
> Did you try removing it?
> +    
> +    <meta:provides feature="devmode" />

For some reason it became necessary when I moved the zcml to
apidoc-configure-normal.zcml. I've moved the principal= part.

> 
> - I'm concerned that the tests are going to be run with even more
> registrations that will not be part of production.  I wish there were
> a light, easy way to start up another LP instance for the apidoc smoke
> test that then would not pollute the rest of the tests with the apidoc
> registrations in the zcml file.  However, I don't know of one, and
> unless you do, I won't insist that we address this concern.  It's
> certainly just slightly enlarging an existing problem, rather than
> making a new one.

I hadn't thought about that, but I'll see if I can find a way to not
make this situation any worse.

> 
> - Did you audit zope.login to make sure it's registrations don't do
> something we need to be worried about from a security perspective?  If
> not, one of us should.  I'd feel somewhat happier if ``<include
> package="zope.login" />`` were part of the apidoc zcml, but then we're
> exacerbating the previous problem (that is, production registrations
> are more and more different from test registrations).

zope.login is now needed because the ILoginPassword adapter has been
moved there from zope.publisher, but you have a good point that it
should be audited.

I've just checked and it just registers the ILoginPassword adapters for
both IHTTPCredentials and IFTPCredentials as zope.publisher did until
version 3.10.

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.

> 
> Suggested:
> 
> - I suggest renaming LaunchpadPrincipalAnnotations to
> TemporaryPrincipalAnnotations: the behavior (and the fact that the
> implementation is effectively a stub) is something worth highlighting.
> Calling it "Launchpad" doesn't buy us anything.

I like your suggestion; renamed.

> 
> - 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/

> 
> - For this comment:
> +    
> I suggest that this is not an XXX.  To that point, this is not
> "Copied" but "Modified" (the "for" is different, and that's the key).

Indeed, I've changed that comment.

> 
> - For this comment:
> + 
> You mentioned that these comments came from me.  I think we can just
> remove the comment entirely.  As I said on IRC, for the first half, I
> think I expected that this was going to have to go in overrides.
> Since it works without being in overrides, this is a "never mind...".
> For the second half ("Safer:..."), I think we're probably fine.  It
> would be nice if we could easily register this only for the bots we
> care about, but, eh.  It's all right, I think.

Cool, I've removed the comment.

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

> 
> Things you can ignore:
> 
> - For browser:menu tags in zcml, I was tempted to ask for a comment
> mentioning that these are necessary just to make apidoc's default
> registrations happy, but on reflection it's pretty clear that the
> whole zcml file is just there to make apidoc happy, so, whatever.

Indeed, there's a comment at the top of the file stating that.

> 
> - I expected us to have a conditional in the zcml file for whether
> devmode is turned on, but I see that this is not necessary since it is
> only included in the development and test configuration.

Right.

-- 
Guilherme Salgado <https://launchpad.net/~salgado>

« Back to merge proposal