Merge lp:~jml/launchpad/package-permission-love into lp:launchpad
- package-permission-love
- Merge into devel
Status: | Merged |
---|---|
Merged at revision: | not available |
Proposed branch: | lp:~jml/launchpad/package-permission-love |
Merge into: | lp:launchpad |
Diff against target: | None lines |
To merge this branch: | bzr merge lp:~jml/launchpad/package-permission-love |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Celso Providelo (community) | approve | Approve | |
Julian Edwards (community) | code | Approve | |
Review via email: mp+10830@code.launchpad.net |
Commit message
[ui=rs] Extract a 'verify_upload' function from nascentupload that encapsulates almost everything need to check whether someone can upload a package. Removes a useless filename from the 'not allowed to upload to component' error message.
Description of the change
Jonathan Lange (jml) wrote : | # |
Julian Edwards (julian-edwards) wrote : | # |
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.archiveuplo
>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-
> +lib/lp/
> 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/
> --- lib/canonical/
> +++ lib/canonical/
> @@ -63,7 +63,7 @@
> IMilestone, IProjectMilestone)
> from canonical.
> IOAuthAccessToken, IOAuthRequestToken)
> -from lp.soyuz.
> +from lp.soyuz.
> from lp.translations
Jonathan Lange (jml) wrote : | # |
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.archiveuplo
>>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-
>> +lib/lp/
>> Â 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 DriverSpecifica
>> @@ -514,6 +512,7 @@
>> Â Â Â Â Â """IProjectMile
>> Â Â Â Â Â return False
>>
>> +
>> Â class EditMilestoneBy
>> Â Â Â permission = 'launchpad.Edit'
>> Â Â Â usedfor = IMilestone
>> @@ -2268,6 +2267,15 @@
>> Â Â Â Â Â Â Â or user.inTeam(
>>
>>
>> +class EditPackageset(
>> + Â Â permission = 'launchpad.Edit'
>> + Â Â usedfor = IPackageset
>> +
>> + Â Â def checkAuthentica
>> + Â Â Â Â """The owner of a package set can edit the object."""
>> + Â Â Â Â return user.inTeam(
>> +
>> +
>
> Nice change.
>
Yeah, but it's made some tests fail. I've fixed them up as part of this reply.
>> === modified file 'lib/lp/
>> --- lib/lp/
>> +++ lib/lp/
>> @@ -28,8 +28,8 @@
>> Â from lp.archiveuploa
>> Â Â Â UploadError, UploadWarning, CustomUploadFile, SourceUploadFile,
>> Â Â Â BaseBinaryUploa
>> +from lp.archiveuploa
Julian Edwards (julian-edwards) 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.
> > 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 ;)
> mergeFunctionMe
> 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
Celso Providelo (cprov) wrote : | # |
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.
>> > Â Â Â Â 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...
Julian Edwards (julian-edwards) 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.
Celso Providelo (cprov) wrote : | # |
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_
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:
* makeSourcePacka
* makeSourcePacka
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
Celso Providelo (cprov) wrote : | # |
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.
Other than that, great! r=me
Jonathan Lange (jml) wrote : | # |
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.
>> > Â Â Â Â 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://
unclear
>> I also love deleting unneeded tests.
>
> And correcting my grammar ;)
>
Oh, I didn't notice. Sorry :)
jml
Julian Edwards (julian-edwards) 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.
> > 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://
> unclear
"Actually executing an exception is expensive" seems clear to me. :)
When you think about what happens, it makes sense.
Jonathan Lange (jml) wrote : | # |
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://
>> 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
Robert Collins (lifeless) wrote : | # |
-----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://
iEYEARECAAYFAkq
6LoAnRmSFaxN6Zz
=82Fk
-----END PGP SIGNATURE-----
Preview Diff
1 | === modified file '.bzrignore' |
2 | --- .bzrignore 2009-08-05 23:08:37 +0000 |
3 | +++ .bzrignore 2009-08-27 07:16:25 +0000 |
4 | @@ -49,4 +49,5 @@ |
5 | ./download-cache |
6 | ./_pythonpath.py |
7 | ./production-configs |
8 | +lib/lp/archiveuploader/tests/data/suite/bar_1.0-2/bar_1.0.orig.tar.gz |
9 | bzr.dev |
10 | |
11 | === modified file 'lib/canonical/launchpad/security.py' |
12 | --- lib/canonical/launchpad/security.py 2009-08-20 12:36:07 +0000 |
13 | +++ lib/canonical/launchpad/security.py 2009-08-27 07:16:25 +0000 |
14 | @@ -63,7 +63,7 @@ |
15 | IMilestone, IProjectMilestone) |
16 | from canonical.launchpad.interfaces.oauth import ( |
17 | IOAuthAccessToken, IOAuthRequestToken) |
18 | -from lp.soyuz.interfaces.packageset import IPackagesetSet |
19 | +from lp.soyuz.interfaces.packageset import IPackageset, IPackagesetSet |
20 | from lp.translations.interfaces.pofile import IPOFile |
21 | from lp.translations.interfaces.potemplate import ( |
22 | IPOTemplate, IPOTemplateSubset) |
23 | @@ -195,6 +195,7 @@ |
24 | return (user.inTeam(celebrities.registry_experts) |
25 | or user.inTeam(celebrities.admin)) |
26 | |
27 | + |
28 | class ReviewProduct(ReviewByRegistryExpertsOrAdmins): |
29 | usedfor = IProduct |
30 | |
31 | @@ -211,8 +212,6 @@ |
32 | usedfor = IProjectSet |
33 | |
34 | |
35 | - |
36 | - |
37 | class ViewPillar(AuthorizationBase): |
38 | usedfor = IPillar |
39 | permission = 'launchpad.View' |
40 | @@ -385,8 +384,7 @@ |
41 | if user.inTeam(driver): |
42 | return True |
43 | admins = getUtility(ILaunchpadCelebrities).admin |
44 | - return (user.inTeam(self.obj.target.owner) or |
45 | - user.inTeam(admins)) |
46 | + return (user.inTeam(targetowner) or user.inTeam(admins)) |
47 | |
48 | |
49 | class DriverSpecification(AuthorizationBase): |
50 | @@ -514,6 +512,7 @@ |
51 | """IProjectMilestone is a fake content object.""" |
52 | return False |
53 | |
54 | + |
55 | class EditMilestoneByTargetOwnerOrAdmins(AuthorizationBase): |
56 | permission = 'launchpad.Edit' |
57 | usedfor = IMilestone |
58 | @@ -2268,6 +2267,15 @@ |
59 | or user.inTeam(celebrities.admin)) |
60 | |
61 | |
62 | +class EditPackageset(AuthorizationBase): |
63 | + permission = 'launchpad.Edit' |
64 | + usedfor = IPackageset |
65 | + |
66 | + def checkAuthenticated(self, user): |
67 | + """The owner of a package set can edit the object.""" |
68 | + return user.inTeam(self.obj.owner) |
69 | + |
70 | + |
71 | class EditPackagesetSet(AuthorizationBase): |
72 | permission = 'launchpad.Edit' |
73 | usedfor = IPackagesetSet |
74 | |
75 | === modified file 'lib/lp/archiveuploader/nascentupload.py' |
76 | --- lib/lp/archiveuploader/nascentupload.py 2009-07-17 00:26:05 +0000 |
77 | +++ lib/lp/archiveuploader/nascentupload.py 2009-08-28 05:25:25 +0000 |
78 | @@ -28,8 +28,8 @@ |
79 | from lp.archiveuploader.nascentuploadfile import ( |
80 | UploadError, UploadWarning, CustomUploadFile, SourceUploadFile, |
81 | BaseBinaryUploadFile) |
82 | +from lp.archiveuploader.permission import CannotUploadToArchive, verify_upload |
83 | from lp.soyuz.interfaces.archive import ArchivePurpose, MAIN_ARCHIVE_PURPOSES |
84 | -from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet |
85 | from lp.soyuz.interfaces.publishing import PackagePublishingPocket |
86 | from canonical.launchpad.interfaces import ( |
87 | IBinaryPackageNameSet, IDistributionSet, ILibraryFileAliasSet, |
88 | @@ -477,16 +477,6 @@ |
89 | # Signature and ACL stuff |
90 | # |
91 | |
92 | - def _components_valid_for(self, person): |
93 | - """Return the set of components this person could upload to.""" |
94 | - permission_set = getUtility(IArchivePermissionSet) |
95 | - permissions = permission_set.componentsForUploader( |
96 | - self.policy.archive, person) |
97 | - possible_components = set( |
98 | - permission.component for permission in permissions) |
99 | - |
100 | - return possible_components |
101 | - |
102 | def verify_acl(self): |
103 | """Check the signer's upload rights. |
104 | |
105 | @@ -494,6 +484,14 @@ |
106 | or the explicit source package, or in the case of a PPA must own |
107 | it or be in the owning team. |
108 | """ |
109 | + # Binary uploads are never checked (they come in via the security |
110 | + # policy or from the buildds) so they don't need any ACL checks -- |
111 | + # this goes for PPAs as well as other archives. The only uploaded file |
112 | + # that matters is the DSC file for sources because it is the only |
113 | + # object that is overridden and created in the database. |
114 | + if self.binaryful: |
115 | + return |
116 | + |
117 | # Set up some convenient shortcut variables. |
118 | signer = self.changes.signer |
119 | archive = self.policy.archive |
120 | @@ -503,68 +501,15 @@ |
121 | self.logger.debug("No signer, therefore ACL not processed") |
122 | return |
123 | |
124 | - # Verify PPA uploads. |
125 | - if self.is_ppa: |
126 | - self.logger.debug("Don't verify signer ACL for PPA") |
127 | - if not archive.canUpload(signer): |
128 | - self.reject("Signer has no upload rights to this PPA.") |
129 | - return |
130 | - |
131 | - # Binary uploads are never checked (they come in via the security |
132 | - # policy or from the buildds) so they don't need any ACL checks. |
133 | - # The only uploaded file that matters is the DSC file for sources |
134 | - # because it is the only object that is overridden and created in |
135 | - # the database. |
136 | - if self.binaryful: |
137 | - return |
138 | - |
139 | - # Sometimes an uploader may upload a new package to a component |
140 | - # that he does not have rights to (but has rights to other components) |
141 | - # but we let this through because an archive admin may wish to |
142 | - # override it later. Consequently, if an uploader has no rights |
143 | - # at all to any component, we reject the upload right now even if it's |
144 | - # NEW. |
145 | - |
146 | - # Check if the user has package-specific rights. |
147 | source_name = getUtility( |
148 | ISourcePackageNameSet).queryByName(self.changes.dsc.package) |
149 | - if (source_name is not None and |
150 | - archive.canUpload(signer, source_name)): |
151 | - return |
152 | - |
153 | - # Now check whether this upload can be approved due to |
154 | - # package set based permissions. |
155 | - ap_set = getUtility(IArchivePermissionSet) |
156 | - if source_name is not None and signer is not None: |
157 | - if ap_set.isSourceUploadAllowed(archive, source_name, signer): |
158 | - return |
159 | - |
160 | - # If source_name is None then the package must be new, but we |
161 | - # kick it out anyway because it's impossible to look up |
162 | - # any permissions for it. |
163 | - possible_components = self._components_valid_for(signer) |
164 | - if not possible_components: |
165 | - # The user doesn't have package-specific rights or |
166 | - # component rights, so kick him out entirely. |
167 | - self.reject( |
168 | - "The signer of this package has no upload rights to this " |
169 | - "distribution's primary archive. Did you mean to upload " |
170 | - "to a PPA?") |
171 | - return |
172 | - |
173 | - # New packages go straight to the upload queue; we only check upload |
174 | - # rights for old packages. |
175 | - if self.is_new: |
176 | - return |
177 | - |
178 | - component = self.changes.dsc.component |
179 | - if component not in possible_components: |
180 | - # The uploader has no rights to the component. |
181 | - self.reject( |
182 | - "Signer is not permitted to upload to the component " |
183 | - "'%s' of file '%s'." % (component.name, |
184 | - self.changes.dsc.filename)) |
185 | - |
186 | + |
187 | + try: |
188 | + verify_upload( |
189 | + signer, source_name, archive, self.changes.dsc.component, |
190 | + not self.is_new) |
191 | + except CannotUploadToArchive, e: |
192 | + self.reject(str(e)) |
193 | |
194 | # |
195 | # Handling checking of versions and overrides |
196 | |
197 | === added file 'lib/lp/archiveuploader/permission.py' |
198 | --- lib/lp/archiveuploader/permission.py 1970-01-01 00:00:00 +0000 |
199 | +++ lib/lp/archiveuploader/permission.py 2009-08-28 05:53:16 +0000 |
200 | @@ -0,0 +1,104 @@ |
201 | +# Copyright 2009 Canonical Ltd. This software is licensed under the |
202 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
203 | + |
204 | +"""Permissions for uploading a package to an archive.""" |
205 | + |
206 | +__metaclass__ = type |
207 | +__all__ = [ |
208 | + 'CannotUploadToArchive', |
209 | + 'CannotUploadToPPA', |
210 | + 'components_valid_for', |
211 | + 'verify_upload', |
212 | + ] |
213 | + |
214 | +from zope.component import getUtility |
215 | + |
216 | +from lp.soyuz.interfaces.archive import ArchivePurpose |
217 | +from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet |
218 | + |
219 | + |
220 | +class CannotUploadToArchive(Exception): |
221 | + """Raised when a person cannot upload to archive.""" |
222 | + |
223 | + _fmt = '%(person)s has no upload rights to %(archive)s.' |
224 | + |
225 | + def __init__(self, **args): |
226 | + """Construct a `CannotUploadToArchive`.""" |
227 | + Exception.__init__(self, self._fmt % args) |
228 | + |
229 | + |
230 | +class CannotUploadToPPA(CannotUploadToArchive): |
231 | + """Raised when a person cannot upload to a PPA.""" |
232 | + |
233 | + _fmt = 'Signer has no upload rights to this PPA.' |
234 | + |
235 | + |
236 | +class NoRightsForArchive(CannotUploadToArchive): |
237 | + """Raised when a person has absolutely no upload rights to an archive.""" |
238 | + |
239 | + _fmt = ( |
240 | + "The signer of this package has no upload rights to this " |
241 | + "distribution's primary archive. Did you mean to upload to " |
242 | + "a PPA?") |
243 | + |
244 | + |
245 | +class NoRightsForComponent(CannotUploadToArchive): |
246 | + """Raised when a person tries to upload to a component without permission. |
247 | + """ |
248 | + |
249 | + _fmt = ( |
250 | + "Signer is not permitted to upload to the component '%(component)s'.") |
251 | + |
252 | + def __init__(self, component): |
253 | + CannotUploadToArchive.__init__(self, component=component.name) |
254 | + |
255 | + |
256 | +def components_valid_for(archive, person): |
257 | + """Return the components that 'person' can upload to 'archive'. |
258 | + |
259 | + :param archive: The `IArchive` than 'person' wishes to upload to. |
260 | + :param person: An `IPerson` wishing to upload to an archive. |
261 | + :return: A `set` of `IComponent`s that 'person' can upload to. |
262 | + """ |
263 | + permission_set = getUtility(IArchivePermissionSet) |
264 | + permissions = permission_set.componentsForUploader(archive, person) |
265 | + return set(permission.component for permission in permissions) |
266 | + |
267 | + |
268 | +def verify_upload(person, sourcepackagename, archive, component, |
269 | + strict_component=True): |
270 | + """Can 'person' upload 'sourcepackagename' to 'archive'? |
271 | + |
272 | + :param person: The `IPerson` trying to upload to the package. Referred to |
273 | + as 'the signer' in upload code. |
274 | + :param sourcepackagename: The source package being uploaded. None if the |
275 | + package is new. |
276 | + :param archive: The `IArchive` being uploaded to. |
277 | + :param component: The `IComponent` that the source package belongs to. |
278 | + :param strict_component: True if access to the specific component for the |
279 | + package is needed to upload to it. If False, then access to any |
280 | + package will do. |
281 | + :raise CannotUploadToArchive: If 'person' cannot upload to the archive. |
282 | + :return: Nothing of interest. |
283 | + """ |
284 | + # For PPAs... |
285 | + if archive.purpose == ArchivePurpose.PPA: |
286 | + if not archive.canUpload(person): |
287 | + raise CannotUploadToPPA() |
288 | + else: |
289 | + return True |
290 | + |
291 | + # For any other archive... |
292 | + ap_set = getUtility(IArchivePermissionSet) |
293 | + if sourcepackagename is not None and ( |
294 | + archive.canUpload(person, sourcepackagename) |
295 | + or ap_set.isSourceUploadAllowed(archive, sourcepackagename, person)): |
296 | + return |
297 | + |
298 | + if not components_valid_for(archive, person): |
299 | + raise NoRightsForArchive() |
300 | + |
301 | + if (component is not None |
302 | + and strict_component |
303 | + and not archive.canUpload(person, component)): |
304 | + raise NoRightsForComponent(component) |
305 | |
306 | === added file 'lib/lp/archiveuploader/tests/test_permission.py' |
307 | --- lib/lp/archiveuploader/tests/test_permission.py 1970-01-01 00:00:00 +0000 |
308 | +++ lib/lp/archiveuploader/tests/test_permission.py 2009-08-28 05:48:38 +0000 |
309 | @@ -0,0 +1,191 @@ |
310 | +# Copyright 2009 Canonical Ltd. This software is licensed under the |
311 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
312 | + |
313 | +"""Tests for the permissions for uploading to an archive.""" |
314 | + |
315 | +__metaclass__ = type |
316 | + |
317 | +import unittest |
318 | + |
319 | +from zope.component import getUtility |
320 | +from zope.security.proxy import removeSecurityProxy |
321 | + |
322 | +from canonical.testing import DatabaseFunctionalLayer |
323 | + |
324 | +from lp.archiveuploader.permission import ( |
325 | + CannotUploadToArchive, components_valid_for, verify_upload) |
326 | +from lp.registry.interfaces.gpg import GPGKeyAlgorithm, IGPGKeySet |
327 | +from lp.soyuz.interfaces.archive import ArchivePurpose |
328 | +from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet |
329 | +from lp.testing import TestCaseWithFactory |
330 | + |
331 | + |
332 | +class TestComponents(TestCaseWithFactory): |
333 | + |
334 | + layer = DatabaseFunctionalLayer |
335 | + |
336 | + def test_no_components_for_arbitrary_person(self): |
337 | + # By default, a person cannot upload to any component of an archive. |
338 | + archive = self.factory.makeArchive() |
339 | + person = self.factory.makePerson() |
340 | + self.assertEqual(set(), components_valid_for(archive, person)) |
341 | + |
342 | + def test_components_for_person_with_permissions(self): |
343 | + # If a person has been explicitly granted upload permissions to a |
344 | + # particular component, then those components are included in |
345 | + # components_valid_for. |
346 | + archive = self.factory.makeArchive() |
347 | + component = self.factory.makeComponent() |
348 | + person = self.factory.makePerson() |
349 | + ap_set = removeSecurityProxy(getUtility(IArchivePermissionSet)) |
350 | + ap_set.newComponentUploader(archive, person, component) |
351 | + self.assertEqual( |
352 | + set([component]), components_valid_for(archive, person)) |
353 | + |
354 | + |
355 | +class TestPermission(TestCaseWithFactory): |
356 | + |
357 | + layer = DatabaseFunctionalLayer |
358 | + |
359 | + def setUp(self): |
360 | + TestCaseWithFactory.setUp(self) |
361 | + permission_set = getUtility(IArchivePermissionSet) |
362 | + self.permission_set = removeSecurityProxy(permission_set) |
363 | + |
364 | + def assertCanUpload(self, person, spn, archive, component, |
365 | + strict_component=True): |
366 | + """Assert that 'person' can upload 'ssp' to 'archive'.""" |
367 | + # For now, just check that doesn't raise an exception. |
368 | + verify_upload(person, spn, archive, component, strict_component) |
369 | + |
370 | + def makeGPGKey(self, owner): |
371 | + """Give 'owner' a crappy GPG key for the purposes of testing.""" |
372 | + return getUtility(IGPGKeySet).new( |
373 | + owner.id, |
374 | + keyid='DEADBEEF', |
375 | + fingerprint='A' * 40, |
376 | + keysize=self.factory.getUniqueInteger(), |
377 | + algorithm=GPGKeyAlgorithm.R, |
378 | + active=True, |
379 | + can_encrypt=False) |
380 | + |
381 | + def test_random_person_cannot_upload_to_ppa(self): |
382 | + # Arbitrary people cannot upload to a PPA. |
383 | + person = self.factory.makePerson() |
384 | + ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA) |
385 | + spn = self.factory.makeSourcePackageName() |
386 | + exception = self.assertRaises( |
387 | + CannotUploadToArchive, verify_upload, person, spn, ppa, None) |
388 | + self.assertEqual( |
389 | + 'Signer has no upload rights to this PPA.', str(exception)) |
390 | + |
391 | + def test_owner_can_upload_to_ppa(self): |
392 | + # If the archive is a PPA, and you own it, then you can upload pretty |
393 | + # much anything to it. |
394 | + team = self.factory.makeTeam() |
395 | + ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA, owner=team) |
396 | + person = self.factory.makePerson() |
397 | + removeSecurityProxy(team).addMember(person, team.teamowner) |
398 | + spn = self.factory.makeSourcePackageName() |
399 | + self.assertCanUpload(person, spn, ppa, None) |
400 | + |
401 | + def test_owner_can_upload_to_ppa_no_sourcepackage(self): |
402 | + # The owner can upload to PPAs even if the source package doesn't |
403 | + # exist yet. |
404 | + team = self.factory.makeTeam() |
405 | + ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA, owner=team) |
406 | + person = self.factory.makePerson() |
407 | + removeSecurityProxy(team).addMember(person, team.teamowner) |
408 | + self.assertCanUpload(person, None, ppa, None) |
409 | + |
410 | + def test_arbitrary_person_cannot_upload_to_primary_archive(self): |
411 | + # By default, you can't upload to the primary archive. |
412 | + person = self.factory.makePerson() |
413 | + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) |
414 | + spn = self.factory.makeSourcePackageName() |
415 | + exception = self.assertRaises( |
416 | + CannotUploadToArchive, verify_upload, person, spn, archive, None) |
417 | + self.assertEqual( |
418 | + ("The signer of this package has no upload rights to this " |
419 | + "distribution's primary archive. Did you mean to upload to " |
420 | + "a PPA?"), |
421 | + str(exception)) |
422 | + |
423 | + def test_package_specific_rights(self): |
424 | + # A person can be granted specific rights for uploading a package, |
425 | + # based only on the source package name. If they have these rights, |
426 | + # they can upload to the package. |
427 | + person = self.factory.makePerson() |
428 | + spn = self.factory.makeSourcePackageName() |
429 | + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) |
430 | + # We can't use a PPA, because they have a different logic for |
431 | + # permissions. We can't create an arbitrary archive, because there's |
432 | + # only one primary archive per distro. |
433 | + self.permission_set.newPackageUploader(archive, person, spn) |
434 | + self.assertCanUpload(person, spn, archive, None) |
435 | + |
436 | + def test_packageset_specific_rights(self): |
437 | + # A person with rights to upload to the package set can upload the |
438 | + # package set to the archive. |
439 | + person = self.factory.makePerson() |
440 | + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) |
441 | + spn = self.factory.makeSourcePackageName() |
442 | + package_set = self.factory.makePackageset(packages=[spn]) |
443 | + self.permission_set.newPackagesetUploader( |
444 | + archive, person, package_set) |
445 | + self.assertCanUpload(person, spn, archive, None) |
446 | + |
447 | + def test_component_rights(self): |
448 | + # A person allowed to upload to a particular component of an archive |
449 | + # can upload basically whatever they want to that component. |
450 | + person = self.factory.makePerson() |
451 | + spn = self.factory.makeSourcePackageName() |
452 | + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) |
453 | + component = self.factory.makeComponent() |
454 | + self.permission_set.newComponentUploader(archive, person, component) |
455 | + self.assertCanUpload(person, spn, archive, component) |
456 | + |
457 | + def test_incorrect_component_rights(self): |
458 | + # Even if a person has upload rights for a particular component in an |
459 | + # archive, it doesn't mean they have upload rights for everything in |
460 | + # that archive. |
461 | + person = self.factory.makePerson() |
462 | + spn = self.factory.makeSourcePackageName() |
463 | + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) |
464 | + permitted_component = self.factory.makeComponent() |
465 | + forbidden_component = self.factory.makeComponent() |
466 | + self.permission_set.newComponentUploader( |
467 | + archive, person, permitted_component) |
468 | + exception = self.assertRaises( |
469 | + CannotUploadToArchive, verify_upload, person, spn, archive, |
470 | + forbidden_component) |
471 | + self.assertEqual( |
472 | + u"Signer is not permitted to upload to the component '%s'." % ( |
473 | + forbidden_component.name), |
474 | + str(exception)) |
475 | + |
476 | + def test_component_rights_no_package(self): |
477 | + # A person allowed to upload to a particular component of an archive |
478 | + # can upload basically whatever they want to that component, even if |
479 | + # the package doesn't exist yet. |
480 | + person = self.factory.makePerson() |
481 | + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) |
482 | + component = self.factory.makeComponent() |
483 | + self.permission_set.newComponentUploader(archive, person, component) |
484 | + self.assertCanUpload(person, None, archive, component) |
485 | + |
486 | + def test_non_strict_component_rights(self): |
487 | + # If we aren't testing strict component access, then we only need to |
488 | + # have access to an arbitrary component. |
489 | + person = self.factory.makePerson() |
490 | + spn = self.factory.makeSourcePackageName() |
491 | + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) |
492 | + component_a = self.factory.makeComponent() |
493 | + component_b = self.factory.makeComponent() |
494 | + self.permission_set.newComponentUploader(archive, person, component_b) |
495 | + self.assertCanUpload( |
496 | + person, spn, archive, component_a, strict_component=False) |
497 | + |
498 | + |
499 | +def test_suite(): |
500 | + return unittest.TestLoader().loadTestsFromName(__name__) |
501 | |
502 | === modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py' |
503 | --- lib/lp/archiveuploader/tests/test_uploadprocessor.py 2009-07-17 00:26:05 +0000 |
504 | +++ lib/lp/archiveuploader/tests/test_uploadprocessor.py 2009-08-28 05:25:25 +0000 |
505 | @@ -1268,8 +1268,7 @@ |
506 | # Make sure it failed. |
507 | self.assertEqual( |
508 | uploadprocessor.last_processed_upload.rejection_message, |
509 | - u"Signer is not permitted to upload to the component 'universe'" |
510 | - " of file 'bar_1.0-2.dsc'.") |
511 | + u"Signer is not permitted to upload to the component 'universe'.") |
512 | |
513 | # Now add permission to upload "bar" for name16. |
514 | bar_package = getUtility(ISourcePackageNameSet).queryByName("bar") |
515 | @@ -1322,8 +1321,7 @@ |
516 | # Make sure it failed. |
517 | self.assertEqual( |
518 | uploadprocessor.last_processed_upload.rejection_message, |
519 | - u"Signer is not permitted to upload to the component 'universe'" |
520 | - " of file 'bar_1.0-2.dsc'.") |
521 | + "Signer is not permitted to upload to the component 'universe'.") |
522 | |
523 | # Now put in place a package set, add 'bar' to it and define a |
524 | # permission for the former. |
525 | |
526 | === modified file 'lib/lp/soyuz/interfaces/archivepermission.py' |
527 | --- lib/lp/soyuz/interfaces/archivepermission.py 2009-07-17 00:26:05 +0000 |
528 | +++ lib/lp/soyuz/interfaces/archivepermission.py 2009-08-03 17:10:12 +0000 |
529 | @@ -335,8 +335,8 @@ |
530 | :param archive: The archive the permission applies to. |
531 | :param person: An `IPerson` for whom you want to add permission. |
532 | :param packageset: An `IPackageset` or a string package set name. |
533 | - :param explicit: True if the package set in question requires |
534 | - specialist skills for proper handling. |
535 | + :param explicit: True if the permissions granted by this package set |
536 | + exclude permissions granted by non-explicit package sets. |
537 | :raises ValueError: if an `ArchivePermission` record for this |
538 | person and packageset already exists *but* with a different |
539 | 'explicit' flag value. |
540 | |
541 | === modified file 'lib/lp/soyuz/tests/test_publishing.py' |
542 | --- lib/lp/soyuz/tests/test_publishing.py 2009-07-19 04:41:14 +0000 |
543 | +++ lib/lp/soyuz/tests/test_publishing.py 2009-08-06 16:57:36 +0000 |
544 | @@ -152,7 +152,8 @@ |
545 | dsc_binaries='foo-bin', build_conflicts=None, |
546 | build_conflicts_indep=None, |
547 | dsc_maintainer_rfc822='Foo Bar <foo@bar.com>', |
548 | - maintainer=None, creator=None, date_uploaded=UTC_NOW): |
549 | + maintainer=None, creator=None, date_uploaded=UTC_NOW, |
550 | + do_upload=True): |
551 | """Return a mock source publishing record.""" |
552 | if sourcename is None: |
553 | sourcename = self.default_package_name |
554 | @@ -194,17 +195,18 @@ |
555 | archive=archive, dateuploaded=date_uploaded) |
556 | |
557 | changes_file_name = "%s_%s_source.changes" % (sourcename, version) |
558 | - package_upload = self.addPackageUpload( |
559 | - archive, distroseries, pocket, |
560 | - changes_file_name=changes_file_name, |
561 | - changes_file_content=changes_file_content) |
562 | - package_upload.addSource(spr) |
563 | + if do_upload: |
564 | + package_upload = self.addPackageUpload( |
565 | + archive, distroseries, pocket, |
566 | + changes_file_name=changes_file_name, |
567 | + changes_file_content=changes_file_content) |
568 | + package_upload.addSource(spr) |
569 | |
570 | - if filename is None: |
571 | - filename = "%s_%s.dsc" % (sourcename, version) |
572 | - alias = self.addMockFile( |
573 | - filename, filecontent, restricted=archive.private) |
574 | - spr.addFile(alias) |
575 | + if filename is None: |
576 | + filename = "%s_%s.dsc" % (sourcename, version) |
577 | + alias = self.addMockFile( |
578 | + filename, filecontent, restricted=archive.private) |
579 | + spr.addFile(alias) |
580 | |
581 | sspph = SecureSourcePackagePublishingHistory( |
582 | distroseries=distroseries, |
583 | |
584 | === modified file 'lib/lp/testing/factory.py' |
585 | --- lib/lp/testing/factory.py 2009-08-21 18:06:18 +0000 |
586 | +++ lib/lp/testing/factory.py 2009-08-28 05:48:38 +0000 |
587 | @@ -26,6 +26,9 @@ |
588 | import pytz |
589 | from storm.store import Store |
590 | import transaction |
591 | + |
592 | +from twisted.python.util import mergeFunctionMetadata |
593 | + |
594 | from zope.component import ComponentLookupError, getUtility |
595 | from zope.security.proxy import removeSecurityProxy |
596 | |
597 | @@ -100,7 +103,9 @@ |
598 | from lp.registry.interfaces.sourcepackagename import ( |
599 | ISourcePackageNameSet) |
600 | from lp.registry.interfaces.ssh import ISSHKeySet, SSHKeyType |
601 | -from lp.testing import time_counter |
602 | +from lp.soyuz.interfaces.component import IComponentSet |
603 | +from lp.soyuz.interfaces.packageset import IPackagesetSet |
604 | +from lp.testing import run_with_login, time_counter |
605 | |
606 | SPACE = ' ' |
607 | |
608 | @@ -125,7 +130,7 @@ |
609 | return func(*args, **kw) |
610 | finally: |
611 | store_selector.pop() |
612 | - return with_default_master_store |
613 | + return mergeFunctionMetadata(func, with_default_master_store) |
614 | |
615 | |
616 | # We use this for default paramters where None has a specific meaning. For |
617 | @@ -1354,6 +1359,12 @@ |
618 | architecturetag, processorfamily, official, owner, |
619 | supports_virtualized) |
620 | |
621 | + def makeComponent(self, name=None): |
622 | + """Make a new `IComponent`.""" |
623 | + if name is None: |
624 | + name = self.getUniqueString() |
625 | + return getUtility(IComponentSet).ensure(name) |
626 | + |
627 | def makeArchive(self, distribution=None, owner=None, name=None, |
628 | purpose = None): |
629 | """Create and return a new arbitrary archive. |
630 | @@ -1374,6 +1385,11 @@ |
631 | if purpose is None: |
632 | purpose = ArchivePurpose.PPA |
633 | |
634 | + # Making a distribution makes an archive, and there can be only one |
635 | + # per distribution. |
636 | + if purpose == ArchivePurpose.PRIMARY: |
637 | + return distribution.main_archive |
638 | + |
639 | return getUtility(IArchiveSet).new( |
640 | owner=owner, purpose=purpose, |
641 | distribution=distribution, name=name) |
642 | @@ -1581,6 +1597,24 @@ |
643 | distroseries = self.makeDistroRelease() |
644 | return distroseries.getSourcePackage(sourcepackagename) |
645 | |
646 | + def makePackageset(self, name=None, description=None, owner=None, |
647 | + packages=()): |
648 | + """Make an `IPackageset`.""" |
649 | + if name is None: |
650 | + name = self.getUniqueString(u'package-set-name') |
651 | + if description is None: |
652 | + description = self.getUniqueString(u'package-set-description') |
653 | + if owner is None: |
654 | + person = self.getUniqueString(u'package-set-owner') |
655 | + owner = self.makePerson(name=person) |
656 | + techboard = getUtility(ILaunchpadCelebrities).ubuntu_techboard |
657 | + ps_set = getUtility(IPackagesetSet) |
658 | + package_set = run_with_login( |
659 | + techboard.teamowner, |
660 | + lambda: ps_set.new(name, description, owner)) |
661 | + run_with_login(owner, lambda: package_set.add(packages)) |
662 | + return package_set |
663 | + |
664 | def getAnyPocket(self): |
665 | return PackagePublishingPocket.RELEASE |
666 |
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.archiveuplo ader.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 'mergeFunctionM etadata' . 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 blishingHistory record without actually doing a
get a SourcePackagePu
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.