Merge lp:~jml/launchpad/package-permission-love into lp:launchpad

Hello reviewer,

This branch is a very important branch, and a very risky one too. Please take
extra care when reviewing it. It's important because much of the Ubuntu
Distributed Development work depends on it. It's risky because it changes
poorly-understood security code.

This extracts out a function called 'verify_upload' from the nascentupload
code in Soyuz. The function is intended to provide a canonical answer to the
question, "can this person upload to this package in this archive?".

Sadly, it doesn't actually provide a canonical answer just yet. There's logic
in the upload policy code that needs to be moved into it. Specifically, it
needs to grow pocket-based permission checks. However, since the branch is
quite large already, I think I'll save that for another change.

The function has been placed in 'lp.archiveuploader.permission'. I don't think
this is a good long-term location, and I'm happy to move it if you think of a
better one. However, I think I'd prefer to wait until 'verify_upload' checks
the pocket as well before I encourage its re-use by putting it in a more
public location.

The implementation of the function is quite straight-forward, if you know what
it's supposed to do. I hope that the unit tests and the docstring explain this
clearly.

I probably got a bit ahead of myself with the exceptions. I'm happy to change
them on request.

Since this is a refactoring patch, and since I don't _really_ understand the
code, I wanted to make as few behavioural changes as possible. However, I was
forced to make some:

  * The error message for when you cannot upload to a particular component no
    longer includes the name of the DSC file. wgrant informed me that the file
    name was never very useful anyway.

  * The binary upload check happens much sooner. It has nothing to do with
    ACLs, so we might as well check it first.

I had to do a fair bit of exploratory work to get this patch up-and-running,
particularly to get the unit tests looking like actual unit tests. Not all of
it ended up being used, but it's all actually useful. Here's a run-down:

 * 'makeComponent' added to the object factory.

 * 'makeArchive' can now create non-PPA archives.

 * Added 'makePackageset'.

 * Tweaked a decorator to use 'mergeFunctionMetadata'. This means that it'll
   have a useful name when it appears in exception stack traces.

 * Changed one of the soyuz helpers, 'getPubSource' so that you can use it to
   get a SourcePackagePublishingHistory record without actually doing a
   simulated upload.

 * Added a launchpad.Edit permission policy for packagesets. This allows them
   to be used in a non-Zopeless environment.

I also did a few incidental cleanups:

 * Changed a docstring in archivepermisson.py to actually match reality. I
   checked this one with Muharem, who was the original author.

 * Re-used an existing local variable in a permission check, rather than
   violating the Law of Demeter.

 * Various whitespace and lint.

Thanks for getting to the end of the cover letter! I'd like to get this patch
right, so please ask as many questions as possible.

Hi Jono, thanks for taking this change on, it's much appreciated!

I am happy to bless this branch, if you make the changes I recommend below
and when Celso has also looked at it.
r=bigjools

>The function has been placed in 'lp.archiveuploader.permission'. I don't think
>this is a good long-term location, and I'm happy to move it if you think of a
>better one. However, I think I'd prefer to wait until 'verify_upload' checks
>the pocket as well before I encourage its re-use by putting it in a more
>public location.

It should go in lp.soyuz somewhere, but I'm not sure where yet either.

>I probably got a bit ahead of myself with the exceptions. I'm happy to change
>them on request.

Yeah, I don't like them so much. See my inline comments.

> * The error message for when you cannot upload to a particular component no
> longer includes the name of the DSC file. wgrant informed me that the file
> name was never very useful anyway.

Should be ok as he says, it doesn't add any value and in fact could be confusing to newbies.

> * The binary upload check happens much sooner. It has nothing to do with
> ACLs, so we might as well check it first.

Should be ok as long as the upload processor tests still pass, I think.

>Thanks for getting to the end of the cover letter! I'd like to get this patch
>right, so please ask as many questions as possible.

Thanks for the other clean ups you made, this is a great branch!

> === modified file '.bzrignore'
> --- .bzrignore 2009-08-05 23:08:37 +0000
> +++ .bzrignore 2009-08-27 07:16:25 +0000
> @@ -49,4 +49,5 @@
> ./download-cache
> ./_pythonpath.py
> ./production-configs
> +lib/lp/archiveuploader/tests/data/suite/bar_1.0-2/bar_1.0.orig.tar.gz
> bzr.dev
>

You don't need to do this. The file is left behind by a test that
uses the bar_1.0-2 package when it downloads the .orig file from the
librarian. The right thing to do is to fix the test to clean up after
itself. We just need to figure out which test is doing it! I'll look
for it after I do this review.

> === modified file 'lib/canonical/launchpad/security.py'
> --- lib/canonical/launchpad/security.py 2009-08-20 12:36:07 +0000
> +++ lib/canonical/launchpad/security.py 2009-08-27 07:16:25 +0000
> @@ -63,7 +63,7 @@
> IMilestone, IProjectMilestone)
> from canonical.launchpad.interfaces.oauth import (
> IOAuthAccessToken, IOAuthRequestToken)
> -from lp.soyuz.interfaces.packageset import IPackagesetSet
> +from lp.soyuz.interfaces.packageset import IPackageset, IPackagesetSet
> from lp.translations.int...

On Sat, Aug 29, 2009 at 3:38 AM, Julian
Edwards<email address hidden> wrote:
> Review: Approve code
> Hi Jono, thanks for taking this change on, it's much appreciated!
>
> I am happy to bless this branch, if you make the changes I recommend below
> and when Celso has also looked at it.
> r=bigjools
>
>>The function has been placed in 'lp.archiveuploader.permission'. I don't think
>>this is a good long-term location, and I'm happy to move it if you think of a
>>better one. However, I think I'd prefer to wait until 'verify_upload' checks
>>the pocket as well before I encourage its re-use by putting it in a more
>>public location.
>
> It should go in lp.soyuz somewhere, but I'm not sure where yet either.
>

OK. I'll leave it where it is for now.

>>I probably got a bit ahead of myself with the exceptions. I'm happy to change
>>them on request.
>
> Yeah, I don't like them so much.  See my inline comments.
>

We dislike different things about them, I expect. Discussed further below.

>>Thanks for getting to the end of the cover letter! I'd like to get this patch
>>right, so please ask as many questions as possible.
>
> Thanks for the other clean ups you made, this is a great branch!
>

My pleasure. It's personally rewarding to watch this code become more usable.

>> === modified file '.bzrignore'
>> --- .bzrignore        2009-08-05 23:08:37 +0000
>> +++ .bzrignore        2009-08-27 07:16:25 +0000
>> @@ -49,4 +49,5 @@
>>  ./download-cache
>>  ./_pythonpath.py
>>  ./production-configs
>> +lib/lp/archiveuploader/tests/data/suite/bar_1.0-2/bar_1.0.orig.tar.gz
>>  bzr.dev
>>
>
> You don't need to do this.  The file is left behind by a test that
> uses the bar_1.0-2 package when it downloads the .orig file from the
> librarian.  The right thing to do is to fix the test to clean up after
> itself.  We just need to figure out which test is doing it! I'll look
> for it after I do this review.
>

Fair enough. I've removed the ignore line, filed bug 421705 and
assigned it to you.

>>  class DriverSpecification(AuthorizationBase):
>> @@ -514,6 +512,7 @@
>>          """IProjectMilestone is a fake content object."""
>>          return False
>>
>> +
>>  class EditMilestoneByTargetOwnerOrAdmins(AuthorizationBase):
>>      permission = 'launchpad.Edit'
>>      usedfor = IMilestone
>> @@ -2268,6 +2267,15 @@
>>              or user.inTeam(celebrities.admin))
>>
>>
>> +class EditPackageset(AuthorizationBase):
>> +    permission = 'launchpad.Edit'
>> +    usedfor = IPackageset
>> +
>> +    def checkAuthenticated(self, user):
>> +        """The owner of a package set can edit the object."""
>> +        return user.inTeam(self.obj.owner)
>> +
>> +
>
> Nice change.
>

Yeah, but it's made some tests fail. I've fixed them up as part of this reply.

>> === modified file 'lib/lp/archiveuploader/nascentupload.py'
>> --- lib/lp/archiveuploader/nascentupload.py   2009-07-17 00:26:05 +0000
>> +++ lib/lp/archiveuploader/nascentupload.py   2009-08-28 05:25:25 +0000
>> @@ -28,8 +28,8 @@
>>  from lp.archiveuploader.nascentuploadfile import (
>>      UploadError, UploadWarning, CustomUploadFile, SourceUploadFile,
>>      BaseBinaryUploadFile)
>> +from lp.archiveuploader.perm...

On Monday 31 August 2009 03:51:07 Jonathan Lange wrote:
> > Whenever I see exceptions used, I ask myself "is this caused by an
> > exceptional circumstance?" In this case, I'm not sure it is, but I can
> > see why you've used one - so that you can return an error string as well.
> > Is that a good enough reason to use an exception here? I guess the
> > code's neater but I still have a nagging feeling that it's bad form.
> >
> > How does this look in comparison:
> >
> > error = verify_upload(
> > signer, source_name, archive, self.changes.dsc.component,
> > not self.is_new)
> > if error is not None:
> > self.reject(error)
>
> I've never really understood the reasoning behind the "exceptional
> circumstance" argument.

Exceptions should be thrown only when there is an abnormal condition that the
code cannot handle. Exceptions should convey: "Woa! Something went wrong
here and I can't deal with it. BOOM." In this case, you're using them as a
routine return value, which doesn't fit that requirement. Also bearing in
mind that they're pretty expensive, if a piece of code can return a value to
indicate its results, then it should do so.

Obviously, "abnormal condition" is a little subjective, but I think in this
case it's clear cut, at least to me, since you're asking for a yes or no
answer.

> I've changed the code to return reason objects rather than raising
> exceptions -- returning a string is too ugly for me right now. This
> change doesn't effect the structure of the function at all.

Okay, it looks better, thanks.

> I've also added an assertCannotUpload to the test case and made the
> tests that check for negative conditions use that.

Excellent, thanks.

> > I suspect a lot of our existing tests for ACLs are now duplicated by your
> > new ones. I don't think we need to fix that in this branch, but it would
> > be great if you could file a bug about that so we remember to check/fix
> > in the future! I love deleting un-needed tests.
>
> Filed bug 421702 to track this.

Thanks.

> I also love deleting unneeded tests.

And correcting my grammar ;)

> mergeFunctionMetadata(f, g) takes the __name__, __doc__ and other
> properties from 'f' and mutates 'g' so that it has the same name,
> docstring and other properties. This is particularly useful for
> decorators, since the returned function (the modified 'g') has a
> useful name when it appears in a stack trace, rather than 'decorated'
> or 'function' or something equally miserable.

Ah! Nice.

> I did a merge from stable while replying to reviews, so I can't
> generate an interdiff easily.

:( I hope the Code diff updating stuff lands soon. Although I don't think it
generates partial diffs. Ho hum.

> Do I still need to wait for cprov's review?

Well he told me he'd done a review, but had not replied yet. I'll ask him
when he's online and reply to confirm.

Cheers
J

On Tue, Sep 1, 2009 at 8:24 AM, Julian
Edwards<email address hidden> wrote:
> On Monday 31 August 2009 03:51:07 Jonathan Lange wrote:
>> > Whenever I see exceptions used, I ask myself "is this caused by an
>> > exceptional circumstance?"  In this case, I'm not sure it is, but I can
>> > see why you've used one - so that you can return an error string as well.
>> >  Is that a good enough reason to use an exception here?  I guess the
>> > code's neater but I still have a nagging feeling that it's bad form.
>> >
>> > How does this look in comparison:
>> >
>> >    error = verify_upload(
>> >        signer, source_name, archive, self.changes.dsc.component,
>> >        not self.is_new)
>> >    if error is not None:
>> >        self.reject(error)
>>
>> I've never really understood the reasoning behind the "exceptional
>> circumstance" argument.
>
> Exceptions should be thrown only when there is an abnormal condition that the
> code cannot handle.  Exceptions should convey: "Woa!  Something went wrong
> here and I can't deal with it.  BOOM."  In this case, you're using them as a
> routine return value, which doesn't fit that requirement.  Also bearing in
> mind that they're pretty expensive, if a piece of code can return a value to
> indicate its results, then it should do so.
>
> Obviously, "abnormal condition" is a little subjective, but I think in this
> case it's clear cut, at least to me, since you're asking for a yes or no
> answer.

I don't feel totally against the previous approach of raising an
exception representing the reason why the upload was rejected,
although I'd expect it to return a boolean by its name.

If one day we decide to store rejected uploads (for tracing/stats
purpose) we will probably have to map the rejection reasons to a
DBEnum, which would still make sense since there is indeed only a
limited number of situations covered by this code.

The truth is, the proposed change already results in much clearer
callsites, the differences between raising or returning the
rejection_reason doesn't affect it. So, thanks for it :)

[snip]

>> Do I still need to wait for cprov's review?
>
> Well he told me he'd done a review, but had not replied yet.  I'll ask him
> when he's online and reply to confirm.

I'm sorry for blocking this change.

The only comment I have is about the new control variable added in
STP.getPubSource(). Why would you ever need to create an source
publication that was *never* uploaded and doesn't contain any file ?
It will look like a source imported by 'gina' but with no files.

I suppose you are trying to avoid hitting the librarian and it will
certainly speed you test up as long as the logic doesn't need that
logic, but I don't think those implication are clear in the method
docstring as they should.

Since there isn't any callsite using it, I guess that change doesn't
need be landed right now.

Source creation is really simple and doesn't really depend on any
distroseries (chroot) setup, it won't be that difficult to move it to
the Factory and once chroot-procedures get integrated in the DAS api
it won't be any harder to move binary creation methods either.

Let me know what you think.

--
Celso Provid...

On Tuesday 01 September 2009 14:15:16 Celso Providelo wrote:
> The only comment I have is about the new control variable added in
> STP.getPubSource(). Why would you ever need to create an source
> publication that was *never* uploaded and doesn't contain any file ?
> It will look like a source imported by 'gina' but with no files.
>
> I suppose you are trying to avoid hitting the librarian and it will
> certainly speed you test up as long as the logic doesn't need that
> logic, but I don't think those implication are clear in the method
> docstring as they should.

Right, it should be clearer about the implications.

But I feel it's the right thing to have in the majority of tests. We want
them to be as skinny and focused as possible. Introducing packageupload
creation in some cases causes the need for dbuser switching, which is really
horrible to have in the middle of a doctest.

On Tue, Sep 1, 2009 at 11:27 AM, Julian
Edwards<email address hidden> wrote:
> On Tuesday 01 September 2009 14:15:16 Celso Providelo wrote:
>> The only comment I have is about the new control variable added in
>> STP.getPubSource(). Why would you ever need to create an source
>> publication that was *never* uploaded and doesn't contain any file ?
>> It will look like a source imported by 'gina' but with no files.
>>
>> I suppose you are trying to avoid hitting the librarian and it will
>> certainly speed you test up as long as the logic doesn't need that
>> logic, but I don't think those implication are clear in the method
>> docstring as they should.
>
> Right, it should be clearer about the implications.
>
> But I feel it's the right thing to have in the majority of tests.  We want
> them to be as skinny and focused as possible.  Introducing packageupload
> creation in some cases causes the need for dbuser switching, which is really
> horrible to have in the middle of a doctest.

Right, I agree, but it only implies that the DB isolation aspect have
to be better encapsulated in the test helper, not necessarily that the
created objects should be incomplete.

After the patch, passing 'create_packageupload=False' means that the
returned object is not suitable for UI or disk publication test.

It is not only confusing for people trying to use the method (remember
we are FOSS) but also is controversial with the reasons why it was
introduced. If we want developers to use more 'naive' publications in
their tests because they are faster, why don't we make it easier to
use ?

One way to make it super-easy and clear to use is to port it directly
to the Factory:

* makeSourcePackageRelease(...)
* makeSourcePackagePublication(...)

I'm sure people will find it quicker than in STP and the confusion
will be gone. Use the factory normally and if you really need it
switch to STP (and think about improving the factory method)

(Sorry, it is getting way OT, we should start a separate thread for
this discussion)

--
Celso Providelo <email address hidden>
IRC: cprov, Jabber: <email address hidden>, Skype: cprovidelo
1024D/681B6469 C858 2652 1A6E F6A6 037B B3F7 9FF2 583E 681B 6469

Jono,

You changes are great, thanks for working on it. Two small nitpicks, though.

1) If remove the change on STP I'd be happier, if you find time to move the features I pointed before (makeSPR, makeSPPH) to the factory itself you will my hero for the whole month ;)

2) Factory.makeGPGKey() cannot be used more than once, since GPGKey.fingerprint has to be globally unique (which makes a lot of sense in the real world, right ?) Can you fix this with the existing unique-string support we already have ?

Other than that, great! r=me

On Tue, Sep 1, 2009 at 9:24 PM, Julian
Edwards<email address hidden> wrote:
> On Monday 31 August 2009 03:51:07 Jonathan Lange wrote:
>> > Whenever I see exceptions used, I ask myself "is this caused by an
>> > exceptional circumstance?"  In this case, I'm not sure it is, but I can
>> > see why you've used one - so that you can return an error string as well.
>> >  Is that a good enough reason to use an exception here?  I guess the
>> > code's neater but I still have a nagging feeling that it's bad form.
>> >
>> > How does this look in comparison:
>> >
>> >    error = verify_upload(
>> >        signer, source_name, archive, self.changes.dsc.component,
>> >        not self.is_new)
>> >    if error is not None:
>> >        self.reject(error)
>>
>> I've never really understood the reasoning behind the "exceptional
>> circumstance" argument.
>
> Exceptions should be thrown only when there is an abnormal condition that the
> code cannot handle.  Exceptions should convey: "Woa!  Something went wrong
> here and I can't deal with it.  BOOM."  In this case, you're using them as a
> routine return value, which doesn't fit that requirement.

That's just restating your point in different words. I really want to
understand the _reason_ for saying so.

> Also bearing in
> mind that they're pretty expensive, if a piece of code can return a value to
> indicate its results, then it should do so.
>

Are they really that expensive in Python?
http://www.python.org/doc/faq/general/#how-fast-are-exceptions is
unclear

>> I also love deleting unneeded tests.
>
> And correcting my grammar ;)
>

Oh, I didn't notice. Sorry :)

jml

On Wednesday 02 September 2009 03:21:08 Jonathan Lange wrote:
> That's just restating your point in different words. I really want to
> understand the _reason_ for saying so.

Maybe my experience is different to yours, but this is a well-established
programming convention in every place I've done programming work for the last
16 years. I can't find any exceptions (haha) to this convention when using
Google, either. Everything I've found talks about using them only for error
handling. Break strong conventions at your peril.

> > Also bearing in
> > mind that they're pretty expensive, if a piece of code can return a value
> > to indicate its results, then it should do so.
>
> Are they really that expensive in Python?
> http://www.python.org/doc/faq/general/#how-fast-are-exceptions is
> unclear

"Actually executing an exception is expensive" seems clear to me. :)

When you think about what happens, it makes sense.

On Wed, Sep 2, 2009 at 6:06 PM, Julian
Edwards<email address hidden> wrote:
> On Wednesday 02 September 2009 03:21:08 Jonathan Lange wrote:
>> That's just restating your point in different words. I really want to
>> understand the _reason_ for saying so.
>
> Maybe my experience is different to yours, but this is a well-established
> programming convention in every place I've done programming work for the last
> 16 years.  I can't find any exceptions (haha) to this convention when using
> Google, either.  Everything I've found talks about using them only for error
> handling.  Break strong conventions at your peril.
>

I wasn't trying to disagree with the convention at all, just to
understand it a bit better. In general, it sounds very sensible to me.

I've heard it cited as a reason to always return None rather than
raise "not found" exceptions, for example, and that's never really sat
well with me.

>> > Also bearing in
>> > mind that they're pretty expensive, if a piece of code can return a value
>> > to indicate its results, then it should do so.
>>
>> Are they really that expensive in Python?
>> http://www.python.org/doc/faq/general/#how-fast-are-exceptions is
>> unclear
>
> "Actually executing an exception is expensive" seems clear to me. :)
>
> When you think about what happens, it makes sense.

Well, it means that it's only expensive if it gets raised. What I mean
is that it's not clear to me that that matters for this function. I'm
guessing it does.

jml

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

As a data point, and not talking about the wisdom or not of using
exceptions for flow control.

In bzr we found the overhead of a
try:
except:

Made a substantial performance difference, inside innermost loops.

- -Rob
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkqfj28ACgkQ42zgmrPGrq4UZgCgyw36QL+CTnmu7vK0LCyIHFsU
6LoAnRmSFaxN6ZzHtTsAPLEr17cqDamz
=82Fk
-----END PGP SIGNATURE-----