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

I've never really understood the reasoning behind the "exceptional
circumstance" argument.

Still, I think it's much of a muchness, and I've changed it.

>> === 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 :)
>

Done.

>> +        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. :)
>

Changed to return True here.

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

I agree, except it's not actually a consequence of using exceptions,
it's a consequence of the old logic.

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.

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

Yes, I need it. Only admins and techboard members can edit archive
permission sets. This restriction has absolutely no bearing on these
tests or the correctness of the code under test, so it's simplest to
just remove the security proxy. Comment added.

>> +        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!
>

super() is broken when subclassing old-style classes. Our policy is to
use super() in those cases, but as long as we are subclassing
unittest.TestCase, we can't use super() reliably in tests.

See http://fuhm.net/super-harmful/.

>> +        permission_set = getUtility(IArchivePermissionSet)
>> +        self.permission_set = removeSecurityProxy(permission_set)
>
> rSP again - needs a comment if you genuinely need it please.
>

As above. Comment added.

>> +
>> +    def assertCanUpload(self, person, spn, archive, component,
>> +                        strict_component=True):
>> +        """Assert that 'person' can upload 'ssp' to 'archive'."""
>
> ssp?  Did you mean spn?
>

I did! Changed.

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

Happy to oblige. :)

>> +            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?
>

Yeah, I think so. Moved to the factory.

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


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

Filed bug 421702 to track this.

I also love deleting unneeded tests.

>> === 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 <email address hidden>',
>> -                     maintainer=None, creator=None, date_uploaded=UTC_NOW):
>> +                     maintainer=None, creator=None, date_uploaded=UTC_NOW,
>> +                     do_upload=True):
>>          """Return a mock source publishing record."""
>>          if sourcename is None:
>>              sourcename = self.default_package_name
>> @@ -194,17 +195,18 @@
>>              archive=archive, dateuploaded=date_uploaded)
>>
>>          changes_file_name = "%s_%s_source.changes" % (sourcename, version)
>> -        package_upload = self.addPackageUpload(
>> -            archive, distroseries, pocket,
>> -            changes_file_name=changes_file_name,
>> -            changes_file_content=changes_file_content)
>> -        package_upload.addSource(spr)
>> +        if do_upload:
>> +            package_upload = self.addPackageUpload(
>> +                archive, distroseries, pocket,
>> +                changes_file_name=changes_file_name,
>> +                changes_file_content=changes_file_content)
>> +            package_upload.addSource(spr)
>
> Nice change, it avoids some hokey with the db permissions if you don't
> really need a packageupload record.
>
> However, do_upload is a little misleading as it implies it's using the
> upload processor.  Can you change it to something like create_packageupload?
>

Renamed to create_packageupload.

>> === modified file 'lib/lp/testing/factory.py'
>> --- lib/lp/testing/factory.py 2009-08-21 18:06:18 +0000
>> +++ lib/lp/testing/factory.py 2009-08-28 05:48:38 +0000
>> @@ -26,6 +26,9 @@
>>  import pytz
>>  from storm.store import Store
>>  import transaction
>> +
>> +from twisted.python.util import mergeFunctionMetadata
>
> I know you mentioned this in the cover letter, but for my own curiosity what
> does it do?  I don't know twisted that well!
>

It's nothing to do with Twisted per se. Stuff in 'twisted.python' is
just generically useful code for Python apps, covering what Twisted
sees to be gaps in Python's own standard library.

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.

>> +
>>  from zope.component import ComponentLookupError, getUtility
>>  from zope.security.proxy import removeSecurityProxy
>>
>> @@ -100,7 +103,9 @@
>>  from lp.registry.interfaces.sourcepackagename import (
>>      ISourcePackageNameSet)
>>  from lp.registry.interfaces.ssh import ISSHKeySet, SSHKeyType
>> -from lp.testing import time_counter
>> +from lp.soyuz.interfaces.component import IComponentSet
>> +from lp.soyuz.interfaces.packageset import IPackagesetSet
>> +from lp.testing import run_with_login, time_counter
>>
>>  SPACE = ' '
>>
>> @@ -125,7 +130,7 @@
>>              return func(*args, **kw)
>>          finally:
>>              store_selector.pop()
>> -    return with_default_master_store
>> +    return mergeFunctionMetadata(func, with_default_master_store)
>>
>>
>>  # We use this for default paramters where None has a specific meaning.  For
>> @@ -1354,6 +1359,12 @@
>>              architecturetag, processorfamily, official, owner,
>>              supports_virtualized)
>>
>> +    def makeComponent(self, name=None):
>> +        """Make a new `IComponent`."""
>> +        if name is None:
>> +            name = self.getUniqueString()
>> +        return getUtility(IComponentSet).ensure(name)
>
> I remember talking about this on IRC.  I didn't initially like introducing
> bogus components, but I guess it does have the advantage of you knowing
> that it's only part of a test if something blows up.
>

Well, one big win with the test factory is helping tests to be clear
on what they are testing. When we use sample data or existing data
like 'universe', we can never be 100% sure if a test is relying on a
property peculiar to 'universe'. By making up a new, nonsense
component, we're making sure that our tests (and the
system-under-test) aren't making assumptions about components.

That said, if component were an enum, I'd definitely return one of the
existing components, since it implies that there actually is a
restricted range.

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

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

Thanks,
jml