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.interfaces.pofile import IPOFile > from lp.translations.interfaces.potemplate import ( > IPOTemplate, IPOTemplateSubset) > @@ -195,6 +195,7 @@ > return (user.inTeam(celebrities.registry_experts) > or user.inTeam(celebrities.admin)) > > + > class ReviewProduct(ReviewByRegistryExpertsOrAdmins): > usedfor = IProduct > > @@ -211,8 +212,6 @@ > usedfor = IProjectSet > > > - > - > class ViewPillar(AuthorizationBase): > usedfor = IPillar > permission = 'launchpad.View' > @@ -385,8 +384,7 @@ > if user.inTeam(driver): > return True > admins = getUtility(ILaunchpadCelebrities).admin > - return (user.inTeam(self.obj.target.owner) or > - user.inTeam(admins)) > + return (user.inTeam(targetowner) or user.inTeam(admins)) > > Thanks for the cleanups! > 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. > class EditPackagesetSet(AuthorizationBase): > permission = 'launchpad.Edit' > usedfor = IPackagesetSet > > === 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.permission import CannotUploadToArchive, verify_upload > from lp.soyuz.interfaces.archive import ArchivePurpose, MAIN_ARCHIVE_PURPOSES > -from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet > from lp.soyuz.interfaces.publishing import PackagePublishingPocket > from canonical.launchpad.interfaces import ( > IBinaryPackageNameSet, IDistributionSet, ILibraryFileAliasSet, > @@ -477,16 +477,6 @@ > # Signature and ACL stuff > # > > - def _components_valid_for(self, person): > - """Return the set of components this person could upload to.""" > - permission_set = getUtility(IArchivePermissionSet) > - permissions = permission_set.componentsForUploader( > - self.policy.archive, person) > - possible_components = set( > - permission.component for permission in permissions) > - > - return possible_components > - > def verify_acl(self): > """Check the signer's upload rights. > > @@ -494,6 +484,14 @@ > or the explicit source package, or in the case of a PPA must own > it or be in the owning team. > """ > + # Binary uploads are never checked (they come in via the security > + # policy or from the buildds) so they don't need any ACL checks -- > + # this goes for PPAs as well as other archives. The only uploaded file > + # that matters is the DSC file for sources because it is the only > + # object that is overridden and created in the database. > + if self.binaryful: > + return > + > # Set up some convenient shortcut variables. > signer = self.changes.signer > archive = self.policy.archive > @@ -503,68 +501,15 @@ > self.logger.debug("No signer, therefore ACL not processed") > return > > - # Verify PPA uploads. > - if self.is_ppa: > - self.logger.debug("Don't verify signer ACL for PPA") > - if not archive.canUpload(signer): > - self.reject("Signer has no upload rights to this PPA.") > - return > - > - # Binary uploads are never checked (they come in via the security > - # policy or from the buildds) so they don't need any ACL checks. > - # The only uploaded file that matters is the DSC file for sources > - # because it is the only object that is overridden and created in > - # the database. > - if self.binaryful: > - return > - > - # Sometimes an uploader may upload a new package to a component > - # that he does not have rights to (but has rights to other components) > - # but we let this through because an archive admin may wish to > - # override it later. Consequently, if an uploader has no rights > - # at all to any component, we reject the upload right now even if it's > - # NEW. > - > - # Check if the user has package-specific rights. > source_name = getUtility( > ISourcePackageNameSet).queryByName(self.changes.dsc.package) > - if (source_name is not None and > - archive.canUpload(signer, source_name)): > - return > - > - # Now check whether this upload can be approved due to > - # package set based permissions. > - ap_set = getUtility(IArchivePermissionSet) > - if source_name is not None and signer is not None: > - if ap_set.isSourceUploadAllowed(archive, source_name, signer): > - return > - > - # If source_name is None then the package must be new, but we > - # kick it out anyway because it's impossible to look up > - # any permissions for it. > - possible_components = self._components_valid_for(signer) > - if not possible_components: > - # The user doesn't have package-specific rights or > - # component rights, so kick him out entirely. > - self.reject( > - "The signer of this package has no upload rights to this " > - "distribution's primary archive. Did you mean to upload " > - "to a PPA?") > - return > - > - # New packages go straight to the upload queue; we only check upload > - # rights for old packages. > - if self.is_new: > - return > - > - component = self.changes.dsc.component > - if component not in possible_components: > - # The uploader has no rights to the component. > - self.reject( > - "Signer is not permitted to upload to the component " > - "'%s' of file '%s'." % (component.name, > - self.changes.dsc.filename)) > - > + > + try: > + verify_upload( > + signer, source_name, archive, self.changes.dsc.component, > + not self.is_new) > + except CannotUploadToArchive, e: > + self.reject(str(e)) 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) > > # > # Handling checking of versions and overrides > > === added file 'lib/lp/archiveuploader/permission.py' > --- lib/lp/archiveuploader/permission.py 1970-01-01 00:00:00 +0000 > +++ lib/lp/archiveuploader/permission.py 2009-08-28 05:53:16 +0000 > @@ -0,0 +1,104 @@ > +# Copyright 2009 Canonical Ltd. This software is licensed under the > +# GNU Affero General Public License version 3 (see the file LICENSE). > + > +"""Permissions for uploading a package to an archive.""" > + > +__metaclass__ = type > +__all__ = [ > + 'CannotUploadToArchive', > + 'CannotUploadToPPA', > + 'components_valid_for', > + 'verify_upload', > + ] > + > +from zope.component import getUtility > + > +from lp.soyuz.interfaces.archive import ArchivePurpose > +from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet > + > + > +class CannotUploadToArchive(Exception): > + """Raised when a person cannot upload to archive.""" > + > + _fmt = '%(person)s has no upload rights to %(archive)s.' > + > + def __init__(self, **args): > + """Construct a `CannotUploadToArchive`.""" > + Exception.__init__(self, self._fmt % args) > + > + > +class CannotUploadToPPA(CannotUploadToArchive): > + """Raised when a person cannot upload to a PPA.""" > + > + _fmt = 'Signer has no upload rights to this PPA.' > + > + > +class NoRightsForArchive(CannotUploadToArchive): > + """Raised when a person has absolutely no upload rights to an archive.""" > + > + _fmt = ( > + "The signer of this package has no upload rights to this " > + "distribution's primary archive. Did you mean to upload to " > + "a PPA?") > + > + > +class NoRightsForComponent(CannotUploadToArchive): > + """Raised when a person tries to upload to a component without permission. > + """ > + > + _fmt = ( > + "Signer is not permitted to upload to the component '%(component)s'.") > + > + def __init__(self, component): > + CannotUploadToArchive.__init__(self, component=component.name) > + > + > +def components_valid_for(archive, person): > + """Return the components that 'person' can upload to 'archive'. > + > + :param archive: The `IArchive` than 'person' wishes to upload to. > + :param person: An `IPerson` wishing to upload to an archive. > + :return: A `set` of `IComponent`s that 'person' can upload to. > + """ > + permission_set = getUtility(IArchivePermissionSet) > + permissions = permission_set.componentsForUploader(archive, person) > + return set(permission.component for permission in permissions) > + > + > +def verify_upload(person, sourcepackagename, archive, component, > + strict_component=True): > + """Can 'person' upload 'sourcepackagename' to 'archive'? > + > + :param person: The `IPerson` trying to upload to the package. Referred to > + as 'the signer' in upload code. > + :param sourcepackagename: The source package being uploaded. None if the > + package is new. > + :param archive: The `IArchive` being uploaded to. > + :param component: The `IComponent` that the source package belongs to. > + :param strict_component: True if access to the specific component for the > + package is needed to upload to it. If False, then access to any > + package will do. > + :raise CannotUploadToArchive: If 'person' cannot upload to the archive. > + :return: Nothing of interest. > + """ > + # For PPAs... > + if archive.purpose == ArchivePurpose.PPA: This should be "if archive.isPPA" Saves an import :) > + if not archive.canUpload(person): > + raise CannotUploadToPPA() > + else: > + return True > + > + # For any other archive... > + ap_set = getUtility(IArchivePermissionSet) > + if sourcepackagename is not None and ( > + archive.canUpload(person, sourcepackagename) > + or ap_set.isSourceUploadAllowed(archive, sourcepackagename, person)): > + return Returning None, vs True above. You've invented a tri-state boolean. :) > + > + if not components_valid_for(archive, person): > + raise NoRightsForArchive() > + > + if (component is not None > + and strict_component > + and not archive.canUpload(person, component)): > + raise NoRightsForComponent(component) This logically drops off the end here and leaves me wondering what else can happen. I think it's another one of the consequences of using exceptions when you don't really need to. I'd much prefer to see a definitive return value. > > === added file 'lib/lp/archiveuploader/tests/test_permission.py' > --- lib/lp/archiveuploader/tests/test_permission.py 1970-01-01 00:00:00 +0000 > +++ lib/lp/archiveuploader/tests/test_permission.py 2009-08-28 05:48:38 +0000 > @@ -0,0 +1,191 @@ > +# Copyright 2009 Canonical Ltd. This software is licensed under the > +# GNU Affero General Public License version 3 (see the file LICENSE). > + > +"""Tests for the permissions for uploading to an archive.""" > + > +__metaclass__ = type > + > +import unittest > + > +from zope.component import getUtility > +from zope.security.proxy import removeSecurityProxy > + > +from canonical.testing import DatabaseFunctionalLayer > + > +from lp.archiveuploader.permission import ( > + CannotUploadToArchive, components_valid_for, verify_upload) > +from lp.registry.interfaces.gpg import GPGKeyAlgorithm, IGPGKeySet > +from lp.soyuz.interfaces.archive import ArchivePurpose > +from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet > +from lp.testing import TestCaseWithFactory > + > + > +class TestComponents(TestCaseWithFactory): > + > + layer = DatabaseFunctionalLayer > + > + def test_no_components_for_arbitrary_person(self): > + # By default, a person cannot upload to any component of an archive. > + archive = self.factory.makeArchive() > + person = self.factory.makePerson() > + self.assertEqual(set(), components_valid_for(archive, person)) > + > + def test_components_for_person_with_permissions(self): > + # If a person has been explicitly granted upload permissions to a > + # particular component, then those components are included in > + # components_valid_for. > + archive = self.factory.makeArchive() > + component = self.factory.makeComponent() > + person = self.factory.makePerson() > + ap_set = removeSecurityProxy(getUtility(IArchivePermissionSet)) Do you really need removeSecurityProxy? If so, please add a comment explaining why. > + ap_set.newComponentUploader(archive, person, component) > + self.assertEqual( > + set([component]), components_valid_for(archive, person)) > + > + > +class TestPermission(TestCaseWithFactory): > + > + layer = DatabaseFunctionalLayer > + > + def setUp(self): > + TestCaseWithFactory.setUp(self) I think our policy is to use super() now. Although that seems to change as much as fashion! > + permission_set = getUtility(IArchivePermissionSet) > + self.permission_set = removeSecurityProxy(permission_set) rSP again - needs a comment if you genuinely need it please. > + > + def assertCanUpload(self, person, spn, archive, component, > + strict_component=True): > + """Assert that 'person' can upload 'ssp' to 'archive'.""" ssp? Did you mean spn? > + # For now, just check that doesn't raise an exception. > + verify_upload(person, spn, archive, component, strict_component) > + > + def makeGPGKey(self, owner): > + """Give 'owner' a crappy GPG key for the purposes of testing.""" > + return getUtility(IGPGKeySet).new( > + owner.id, > + keyid='DEADBEEF', I'd never seen DEADBEEF until 2 days ago, and now I've seen it twice. Fantastic. > + fingerprint='A' * 40, > + keysize=self.factory.getUniqueInteger(), > + algorithm=GPGKeyAlgorithm.R, > + active=True, > + can_encrypt=False) Maybe it's better to put this func in the factory? > + > + def test_random_person_cannot_upload_to_ppa(self): > + # Arbitrary people cannot upload to a PPA. > + person = self.factory.makePerson() > + ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA) > + spn = self.factory.makeSourcePackageName() > + exception = self.assertRaises( > + CannotUploadToArchive, verify_upload, person, spn, ppa, None) > + self.assertEqual( > + 'Signer has no upload rights to this PPA.', str(exception)) > + > + def test_owner_can_upload_to_ppa(self): > + # If the archive is a PPA, and you own it, then you can upload pretty > + # much anything to it. > + team = self.factory.makeTeam() > + ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA, owner=team) > + person = self.factory.makePerson() > + removeSecurityProxy(team).addMember(person, team.teamowner) > + spn = self.factory.makeSourcePackageName() > + self.assertCanUpload(person, spn, ppa, None) > + > + def test_owner_can_upload_to_ppa_no_sourcepackage(self): > + # The owner can upload to PPAs even if the source package doesn't > + # exist yet. > + team = self.factory.makeTeam() > + ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA, owner=team) > + person = self.factory.makePerson() > + removeSecurityProxy(team).addMember(person, team.teamowner) > + self.assertCanUpload(person, None, ppa, None) > + > + def test_arbitrary_person_cannot_upload_to_primary_archive(self): > + # By default, you can't upload to the primary archive. > + person = self.factory.makePerson() > + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) > + spn = self.factory.makeSourcePackageName() > + exception = self.assertRaises( > + CannotUploadToArchive, verify_upload, person, spn, archive, None) > + self.assertEqual( > + ("The signer of this package has no upload rights to this " > + "distribution's primary archive. Did you mean to upload to " > + "a PPA?"), > + str(exception)) > + > + def test_package_specific_rights(self): > + # A person can be granted specific rights for uploading a package, > + # based only on the source package name. If they have these rights, > + # they can upload to the package. > + person = self.factory.makePerson() > + spn = self.factory.makeSourcePackageName() > + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) > + # We can't use a PPA, because they have a different logic for > + # permissions. We can't create an arbitrary archive, because there's > + # only one primary archive per distro. > + self.permission_set.newPackageUploader(archive, person, spn) > + self.assertCanUpload(person, spn, archive, None) > + > + def test_packageset_specific_rights(self): > + # A person with rights to upload to the package set can upload the > + # package set to the archive. > + person = self.factory.makePerson() > + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) > + spn = self.factory.makeSourcePackageName() > + package_set = self.factory.makePackageset(packages=[spn]) > + self.permission_set.newPackagesetUploader( > + archive, person, package_set) > + self.assertCanUpload(person, spn, archive, None) > + > + def test_component_rights(self): > + # A person allowed to upload to a particular component of an archive > + # can upload basically whatever they want to that component. > + person = self.factory.makePerson() > + spn = self.factory.makeSourcePackageName() > + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) > + component = self.factory.makeComponent() > + self.permission_set.newComponentUploader(archive, person, component) > + self.assertCanUpload(person, spn, archive, component) > + > + def test_incorrect_component_rights(self): > + # Even if a person has upload rights for a particular component in an > + # archive, it doesn't mean they have upload rights for everything in > + # that archive. > + person = self.factory.makePerson() > + spn = self.factory.makeSourcePackageName() > + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) > + permitted_component = self.factory.makeComponent() > + forbidden_component = self.factory.makeComponent() > + self.permission_set.newComponentUploader( > + archive, person, permitted_component) > + exception = self.assertRaises( > + CannotUploadToArchive, verify_upload, person, spn, archive, > + forbidden_component) > + self.assertEqual( > + u"Signer is not permitted to upload to the component '%s'." % ( > + forbidden_component.name), > + str(exception)) > + > + def test_component_rights_no_package(self): > + # A person allowed to upload to a particular component of an archive > + # can upload basically whatever they want to that component, even if > + # the package doesn't exist yet. > + person = self.factory.makePerson() > + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) > + component = self.factory.makeComponent() > + self.permission_set.newComponentUploader(archive, person, component) > + self.assertCanUpload(person, None, archive, component) > + > + def test_non_strict_component_rights(self): > + # If we aren't testing strict component access, then we only need to > + # have access to an arbitrary component. > + person = self.factory.makePerson() > + spn = self.factory.makeSourcePackageName() > + archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) > + component_a = self.factory.makeComponent() > + component_b = self.factory.makeComponent() > + self.permission_set.newComponentUploader(archive, person, component_b) > + self.assertCanUpload( > + person, spn, archive, component_a, strict_component=False) > + > + > +def test_suite(): > + return unittest.TestLoader().loadTestsFromName(__name__) > > === modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py' > --- lib/lp/archiveuploader/tests/test_uploadprocessor.py 2009-07-17 00:26:05 +0000 > +++ lib/lp/archiveuploader/tests/test_uploadprocessor.py 2009-08-28 05:25:25 +0000 > @@ -1268,8 +1268,7 @@ > # Make sure it failed. > self.assertEqual( > uploadprocessor.last_processed_upload.rejection_message, > - u"Signer is not permitted to upload to the component 'universe'" > - " of file 'bar_1.0-2.dsc'.") > + u"Signer is not permitted to upload to the component 'universe'.") > > # Now add permission to upload "bar" for name16. > bar_package = getUtility(ISourcePackageNameSet).queryByName("bar") > @@ -1322,8 +1321,7 @@ > # Make sure it failed. > self.assertEqual( > uploadprocessor.last_processed_upload.rejection_message, > - u"Signer is not permitted to upload to the component 'universe'" > - " of file 'bar_1.0-2.dsc'.") > + "Signer is not permitted to upload to the component 'universe'.") > > # Now put in place a package set, add 'bar' to it and define a > # permission for the former. 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. > > === modified file 'lib/lp/soyuz/interfaces/archivepermission.py' > --- lib/lp/soyuz/interfaces/archivepermission.py 2009-07-17 00:26:05 +0000 > +++ lib/lp/soyuz/interfaces/archivepermission.py 2009-08-03 17:10:12 +0000 > @@ -335,8 +335,8 @@ > :param archive: The archive the permission applies to. > :param person: An `IPerson` for whom you want to add permission. > :param packageset: An `IPackageset` or a string package set name. > - :param explicit: True if the package set in question requires > - specialist skills for proper handling. > + :param explicit: True if the permissions granted by this package set > + exclude permissions granted by non-explicit package sets. > :raises ValueError: if an `ArchivePermission` record for this > person and packageset already exists *but* with a different > 'explicit' flag value. > > === modified file 'lib/lp/soyuz/tests/test_publishing.py' > --- lib/lp/soyuz/tests/test_publishing.py 2009-07-19 04:41:14 +0000 > +++ lib/lp/soyuz/tests/test_publishing.py 2009-08-06 16:57:36 +0000 > @@ -152,7 +152,8 @@ > dsc_binaries='foo-bin', build_conflicts=None, > build_conflicts_indep=None, > dsc_maintainer_rfc822='Foo Bar