Merge lp:~leonardr/launchpad/grant-permissions-oauth into lp:launchpad/db-devel

[Leonard Richardson]

This branch introduces a brand new access level for OAuth tokens: GRANT_PERMISSIONS. GRANT_PERMISSIONS is different from other access levels (eg. READ_PUBLIC) in that it's designed to be used by one specific application: the forthcoming desktop credential manager for the Launchpad web service.

Currently GRANT_PERMISSIONS acts exactly like READ_PUBLIC, with the following exceptions:

1. GRANT_PERMISSIONS is not published in the list of access levels -- the client must know its name ahead of time.

2. GRANT_PERMISSIONS does not show up on the list of access levels in +authorize-token unless it is specifically requested and the _only_ access level the client requests. You can't let the end-user choose between WRITE_PRIVATE and GRANT_PERMISSIONS -- either your program needs GRANT_PERMISSIONS or it doesn't.

Eventually there will be a third exception:

3. GRANT_PERMISSIONS will be the only access level that can access the current user's list of OAuth access tokens, or invoke the named operation to create a new OAuth access token.

[Edwin Grubbs (edwin-grubbs)]

Hi Leonard,

This looks good. I just have a couple of wording suggestions below.

-Edwin

>=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
>--- lib/canonical/launchpad/webapp/interfaces.py 2010-05-02 23:43:35 + 0000
>+++ lib/canonical/launchpad/webapp/interfaces.py 2010-07-07 20:53:55 + 0000
>@@ -547,6 +547,16 @@
> for reading and changing anything, including private data.
> """)
>
>+ GRANT_PERMISSIONS = DBItem(60, """
>+ Grant Permissions
>+
>+ Not only will the application will be able to access Launchpad

s/will the application will be/will the application be/

>+ on your behalf, it will be able to grant access to your
>+ Launchpad account to any other application. This is a very
>+ powerful level of access. You should not grant this level of
>+ access to any application except the official desktop
>+ Launchpad credential manager.

"official Launchpad desktop..." makes more sense than "official desktop
Launchpad ...".

>+ """)
>
> class AccessLevel(DBEnumeratedType):
> """The level of access any given principal has."""
>

review: Approve

[lp:~leonardr/launchpad/grant-permissions-oauth]

11096. By Leonard Richardson on 2010-07-07

Rewording in response to feedback.

11097. By Leonard Richardson on 2010-08-23

Merge from trunk.

11098. By Leonard Richardson on 2010-08-24

Merge from trunk.

[Brad Crittenden (bac)]

Leonard can this branch be landed now? Is there anything blocking you?

=== modified file 'bootstrap.py'
--- bootstrap.py	2010-03-20 01:13:25 +0000
+++ bootstrap.py	2010-08-24 16:45:57 +0000
@@ -1,6 +1,6 @@
 ##############################################################################
 #
-# Copyright (c) 2006 Zope Corporation and Contributors.
+# Copyright (c) 2006 Zope Foundation and Contributors.
 # All Rights Reserved.
 #
 # This software is subject to the provisions of the Zope Public License,
@@ -16,11 +16,9 @@
 Simply run this script in a directory containing a buildout.cfg.
 The script accepts buildout command-line options, so you can
 use the -c option to specify an alternate configuration file.
-
-$Id$
 """
 
-import os, shutil, sys, tempfile, textwrap, urllib, urllib2
+import os, shutil, sys, tempfile, textwrap, urllib, urllib2, subprocess
 from optparse import OptionParser
 
 if sys.platform == 'win32':
@@ -32,11 +30,23 @@
 else:
     quote = str
 
+# See zc.buildout.easy_install._has_broken_dash_S for motivation and comments.
+stdout, stderr = subprocess.Popen(
+    [sys.executable, '-Sc',
+     'try:\n'
+     '    import ConfigParser\n'
+     'except ImportError:\n'
+     '    print 1\n'
+     'else:\n'
+     '    print 0\n'],
+    stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()
+has_broken_dash_S = bool(int(stdout.strip()))
+
 # In order to be more robust in the face of system Pythons, we want to
 # run without site-packages loaded.  This is somewhat tricky, in
 # particular because Python 2.6's distutils imports site, so starting
 # with the -S flag is not sufficient.  However, we'll start with that:
-if 'site' in sys.modules:
+if not has_broken_dash_S and 'site' in sys.modules:
     # We will restart with python -S.
     args = sys.argv[:]
     args[0:0] = [sys.executable, '-S']
@@ -109,13 +119,22 @@
                   help=("Specify a directory for storing eggs.  Defaults to "
                         "a temporary directory that is deleted when the "
                         "bootstrap script completes."))
+parser.add_option("-t", "--accept-buildout-test-releases",
+                  dest='accept_buildout_test_releases',
+                  action="store_true", default=False,
+                  help=("Normally, if you do not specify a --version, the "
+                        "bootstrap script and buildout gets the newest "
+                        "*final* versions of zc.buildout and its recipes and "
+                        "extensions for you.  If you use this flag, "
+                        "bootstrap and buildout will get the newest releases "
+                        "even if they are alphas or betas."))
 parser.add_option("-c", None, action="store", dest="config_file",
                    help=("Specify the path to the buildout configuration "
                          "file to be used."))
 
 options, args = parser.parse_args()
 
-# if -c was provided, we push it back into args for buildout' main function
+# if -c was provided, we push it back into args for buildout's main function
 if options.config_file is not None:
     args += ['-c', options.config_file]
 
@@ -130,16 +149,15 @@
     else:
         options.setup_source = setuptools_source
 
-args = args + ['bootstrap']
-
+if options.accept_buildout_test_releases:
+    args.append('buildout:accept-buildout-test-releases=true')
+args.append('bootstrap')
 
 try:
-    to_reload = False
     import pkg_resources
-    to_reload = True
+    import setuptools # A flag.  Sometimes pkg_resources is installed alone.
     if not hasattr(pkg_resources, '_distribute'):
         raise ImportError
-    import setuptools # A flag.  Sometimes pkg_resources is installed alone.
 except ImportError:
     ez_code = urllib2.urlopen(
         options.setup_source).read().replace('\r\n', '\n')
@@ -151,10 +169,8 @@
     if options.use_distribute:
         setup_args['no_fake'] = True
     ez['use_setuptools'](**setup_args)
-    if to_reload:
-        reload(pkg_resources)
-    else:
-        import pkg_resources
+    reload(sys.modules['pkg_resources'])
+    import pkg_resources
     # This does not (always?) update the default working set.  We will
     # do it.
     for path in sys.path:
@@ -167,23 +183,59 @@
        '-mqNxd',
        quote(eggs_dir)]
 
-if options.download_base:
-    cmd.extend(['-f', quote(options.download_base)])
+if not has_broken_dash_S:
+    cmd.insert(1, '-S')
 
-requirement = 'zc.buildout'
-if options.version:
-    requirement = '=='.join((requirement, options.version))
-cmd.append(requirement)
+find_links = options.download_base
+if not find_links:
+    find_links = os.environ.get('bootstrap-testing-find-links')
+if find_links:
+    cmd.extend(['-f', quote(find_links)])
 
 if options.use_distribute:
     setup_requirement = 'distribute'
 else:
     setup_requirement = 'setuptools'
 ws = pkg_resources.working_set
+setup_requirement_path = ws.find(
+    pkg_resources.Requirement.parse(setup_requirement)).location
 env = dict(
     os.environ,
-    PYTHONPATH=ws.find(
-        pkg_resources.Requirement.parse(setup_requirement)).location)
+    PYTHONPATH=setup_requirement_path)
+
+requirement = 'zc.buildout'
+version = options.version
+if version is None and not options.accept_buildout_test_releases:
+    # Figure out the most recent final version of zc.buildout.
+    import setuptools.package_index
+    _final_parts = '*final-', '*final'
+    def _final_version(parsed_version):
+        for part in parsed_version:
+            if (part[:1] == '*') and (part not in _final_parts):
+                return False
+        return True
+    index = setuptools.package_index.PackageIndex(
+        search_path=[setup_requirement_path])
+    if find_links:
+        index.add_find_links((find_links,))
+    req = pkg_resources.Requirement.parse(requirement)
+    if index.obtain(req) is not None:
+        best = []
+        bestv = None
+        for dist in index[req.project_name]:
+            distv = dist.parsed_version
+            if _final_version(distv):
+                if bestv is None or distv > bestv:
+                    best = [dist]
+                    bestv = distv
+                elif distv == bestv:
+                    best.append(dist)
+        if best:
+            best.sort()
+            version = best[-1].version
+if version:
+    requirement = '=='.join((requirement, version))
+cmd.append(requirement)
 
 if is_jython:
     import subprocess
@@ -193,7 +245,7 @@
 if exitcode != 0:
     sys.stdout.flush()
     sys.stderr.flush()
-    print ("An error occured when trying to install zc.buildout. "
+    print ("An error occurred when trying to install zc.buildout. "
            "Look above this message for any errors that "
            "were output by easy_install.")
     sys.exit(exitcode)

=== modified file 'lib/canonical/launchpad/browser/oauth.py'
--- lib/canonical/launchpad/browser/oauth.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/browser/oauth.py	2010-08-24 16:45:57 +0000
@@ -88,8 +88,13 @@
 
         token = consumer.newRequestToken()
         if self.request.headers.get('Accept') == HTTPResource.JSON_TYPE:
+            # Don't show the client the GRANT_PERMISSIONS access
+            # level. If they have a legitimate need to use it, they'll
+            # already know about it.
+            permissions = [permission for permission in OAuthPermission.items
+                           if permission != OAuthPermission.GRANT_PERMISSIONS]
             return self.getJSONRepresentation(
-                OAuthPermission.items, token, include_secret=True)
+                permissions, token, include_secret=True)
         return u'oauth_token=%s&oauth_token_secret=%s' % (
             token.key, token.secret)
 
@@ -100,6 +105,7 @@
 def create_oauth_permission_actions():
     """Return a list of `Action`s for each possible `OAuthPermission`."""
     actions = Actions()
+    actions_excluding_grant_permissions = Actions()
     def success(form, action, data):
         form.reviewToken(action.permission)
     for permission in OAuthPermission.items:
@@ -108,13 +114,15 @@
             condition=token_exists_and_is_not_reviewed)
         action.permission = permission
         actions.append(action)
-    return actions
-
+        if permission != OAuthPermission.GRANT_PERMISSIONS:
+            actions_excluding_grant_permissions.append(action)
+    return actions, actions_excluding_grant_permissions
 
 class OAuthAuthorizeTokenView(LaunchpadFormView, JSONTokenMixin):
     """Where users authorize consumers to access Launchpad on their behalf."""
 
-    actions = create_oauth_permission_actions()
+    actions, actions_excluding_grant_permissions = (
+        create_oauth_permission_actions())
     label = "Authorize application to access Launchpad on your behalf"
     schema = IOAuthRequestToken
     field_names = []
@@ -132,28 +140,47 @@
         acceptable subset of OAuthPermission.
 
         The user always has the option to deny the client access
-        altogether, so it makes sense for the client to specify the
-        least restrictions possible.
+        altogether, so it makes sense for the client to ask for the
+        least access possible.
 
         If the client sends nonsensical values for allow_permissions,
-        the end-user will be given an unrestricted choice.
+        the end-user will be given a choice among all the permissions
+        used by normal applications.
         """
+
         allowed_permissions = self.request.form_ng.getAll('allow_permission')
         if len(allowed_permissions) == 0:
-            return self.actions
+            return self.actions_excluding_grant_permissions
         actions = Actions()
+
+        # UNAUTHORIZED is always one of the options. If the client
+        # explicitly requested UNAUTHORIZED, remove it from the list
+        # to simplify the algorithm: we'll add it back later.
+        if OAuthPermission.UNAUTHORIZED.name in allowed_permissions:
+            allowed_permissions.remove(OAuthPermission.UNAUTHORIZED.name)
+
+        # GRANT_PERMISSIONS cannot be requested as one of several
+        # options--it must be the only option (other than
+        # UNAUTHORIZED). If GRANT_PERMISSIONS is one of several
+        # options, remove it from the list.
+        if (OAuthPermission.GRANT_PERMISSIONS.name in allowed_permissions
+            and len(allowed_permissions) > 1):
+            allowed_permissions.remove(OAuthPermission.GRANT_PERMISSIONS.name)
+
         for action in self.actions:
             if (action.permission.name in allowed_permissions
                 or action.permission is OAuthPermission.UNAUTHORIZED):
                 actions.append(action)
+
         if len(list(actions)) == 1:
             # The only visible action is UNAUTHORIZED. That means the
-            # client tried to restrict the actions but didn't name any
-            # actual actions (except possibly UNAUTHORIZED). Rather
-            # than present the end-user with an impossible situation
-            # where their only option is to deny access, we'll present
-            # the full range of actions.
-            return self.actions
+            # client tried to restrict the permissions but didn't name
+            # any actual permissions (except possibly
+            # UNAUTHORIZED). Rather than present the end-user with an
+            # impossible situation where their only option is to deny
+            # access, we'll present the full range of actions (except
+            # for GRANT_PERMISSIONS).
+            return self.actions_excluding_grant_permissions
         return actions
 
     def initialize(self):

=== modified file 'lib/canonical/launchpad/components/decoratedresultset.py'
--- lib/canonical/launchpad/components/decoratedresultset.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/components/decoratedresultset.py	2010-08-24 16:45:57 +0000
@@ -9,7 +9,6 @@
     ]
 
 from lazr.delegates import delegates
-from storm.expr import Column
 from storm.zope.interfaces import IResultSet
 from zope.security.proxy import removeSecurityProxy
 
@@ -31,9 +30,6 @@
 
     This behaviour is required for other classes as well (Distribution,
     DistroArchSeries), hence a generalised solution.
-
-    This class also fixes a bug currently in Storm's ResultSet.count
-    method (see below)
     """
     delegates(IResultSet, context='result_set')
 

=== modified file 'lib/canonical/launchpad/doc/webapp-authorization.txt'
--- lib/canonical/launchpad/doc/webapp-authorization.txt	2010-04-16 15:06:55 +0000
+++ lib/canonical/launchpad/doc/webapp-authorization.txt	2010-08-24 16:45:57 +0000
@@ -79,8 +79,27 @@
     >>> check_permission('launchpad.View', bug_1)
     False
 
-Users logged in through the web application, though, have full access,
-which means they can read/change any object they have access to.
+Now consider a principal authorized to create OAuth tokens. Whenever
+it's not creating OAuth tokens, it has a level of permission
+equivalent to READ_PUBLIC.
+
+    >>> principal.access_level = AccessLevel.GRANT_PERMISSIONS
+    >>> setupInteraction(principal)
+    >>> check_permission('launchpad.View', bug_1)
+    False
+
+    >>> check_permission('launchpad.Edit', sample_person)
+    False
+
+This may seem useless from a security standpoint, since once a
+malicious client is authorized to create OAuth tokens, it can escalate
+its privileges at any time by creating a new token for itself. The
+security benefit is more subtle: by discouraging feature creep in
+clients that have this super-access level, we reduce the risk that a
+bug in a _trusted_ client will enable privilege escalation attacks.
+
+Users logged in through the web application have full access, which
+means they can read/change any object they have access to.
 
     >>> mock_participation = Participation()
     >>> login('[e-mail]', mock_participation)

=== modified file 'lib/canonical/launchpad/pagetests/oauth/authorize-token.txt'
--- lib/canonical/launchpad/pagetests/oauth/authorize-token.txt	2010-02-05 13:25:46 +0000
+++ lib/canonical/launchpad/pagetests/oauth/authorize-token.txt	2010-08-24 16:45:57 +0000
@@ -44,7 +44,8 @@
     ...
     See all applications authorized to access Launchpad on your behalf.
 
-This page contains one submit button for each item of OAuthPermission.
+This page contains one submit button for each item of OAuthPermission,
+except for 'Grant Permissions', which must be specifically requested.
 
     >>> browser.getControl('No Access')
     <SubmitControl...
@@ -57,9 +58,14 @@
     >>> browser.getControl('Change Anything')
     <SubmitControl...
 
+    >>> browser.getControl('Grant Permissions')
+    Traceback (most recent call last):
+    ...
+    LookupError: label 'Grant Permissions'
+
     >>> actions = main_content.findAll('input', attrs={'type': 'submit'})
     >>> from canonical.launchpad.webapp.interfaces import OAuthPermission
-    >>> len(actions) == len(OAuthPermission.items)
+    >>> len(actions) == len(OAuthPermission.items) - 1
     True
 
 An application, when asking to access Launchpad on a user's behalf,
@@ -83,9 +89,28 @@
     Change Non-Private Data
     Change Anything
 
+The only time the 'Grant Permissions' permission shows up in this list
+is if the client specifically requests it, and no other
+permission. (Also requesting UNAUTHORIZED is okay--it will show up
+anyway.)
+
+    >>> print_access_levels('allow_permission=GRANT_PERMISSIONS')
+    No Access
+    Grant Permissions
+
+    >>> print_access_levels(
+    ...     'allow_permission=GRANT_PERMISSIONS&allow_permission=UNAUTHORIZED')
+    No Access
+    Grant Permissions
+
+    >>> print_access_levels(
+    ...     'allow_permission=WRITE_PUBLIC&allow_permission=GRANT_PERMISSIONS')
+    No Access
+    Change Non-Private Data
+
 If an application doesn't specify any valid access levels, or only
 specifies the UNAUTHORIZED access level, Launchpad will show all the
-access levels.
+access levels, except for GRANT_PERMISSIONS.
 
     >>> print_access_levels('')
     No Access

=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
--- lib/canonical/launchpad/webapp/interfaces.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/webapp/interfaces.py	2010-08-24 16:45:57 +0000
@@ -527,6 +527,15 @@
         for reading and changing anything, including private data.
         """)
 
+    GRANT_PERMISSIONS = DBItem(60, """
+        Grant Permissions
+
+        The application will be able to grant access to your Launchpad
+        account to any other application. This is a very powerful
+        level of access. You should not grant this level of access to
+        any application except the official Launchpad credential
+        manager.
+        """)
 
 class AccessLevel(DBEnumeratedType):
     """The level of access any given principal has."""

=== modified file 'lib/lp/archivepublisher/utils.py'
--- lib/lp/archivepublisher/utils.py	2010-08-20 20:31:18 +0000
+++ lib/lp/archivepublisher/utils.py	2010-08-24 16:45:57 +0000
@@ -109,7 +109,8 @@
         end = start + chunk_size
 
         # The reason why we listify the sliced ResultSet is because we
-        # cannot very it's size using 'count' (see bug #217644). However,
+        # cannot very it's size using 'count' (see bug #217644 and note
+        # that it was fixed in storm but not SQLObjectResultSet). However,
         # It's not exactly a problem considering non-empty set will be
         # iterated anyway.
         batch = list(self.input[start:end])

=== modified file 'lib/lp/archiveuploader/dscfile.py'
--- lib/lp/archiveuploader/dscfile.py	2010-08-21 13:54:20 +0000
+++ lib/lp/archiveuploader/dscfile.py	2010-08-24 16:45:57 +0000
@@ -13,10 +13,11 @@
     'SignableTagFile',
     'DSCFile',
     'DSCUploadedFile',
-    'findChangelog',
-    'findCopyright',
+    'find_changelog',
+    'find_copyright',
     ]
 
+from cStringIO import StringIO
 import errno
 import glob
 import os
@@ -73,6 +74,70 @@
 from lp.soyuz.interfaces.sourcepackageformat import SourcePackageFormat
 
 
+class DpkgSourceError(Exception):
+
+    _fmt = "Unable to unpack source package (%(result)s): %(output)s"
+
+    def __init__(self, output, result):
+        Exception.__init__(
+            self, self._fmt % {"output": output, "result": result})
+        self.output = output
+        self.result = result
+
+
+def unpack_source(dsc_filepath):
+    """Unpack a source package into a temporary directory
+
+    :param dsc_filepath: Path to the dsc file
+    :return: Path to the temporary directory with the unpacked sources
+    """
+    # Get a temporary dir together.
+    unpacked_dir = tempfile.mkdtemp()
+    try:
+        # chdir into it
+        cwd = os.getcwd()
+        os.chdir(unpacked_dir)
+        try:
+            args = ["dpkg-source", "-sn", "-x", dsc_filepath]
+            dpkg_source = subprocess.Popen(args, stdout=subprocess.PIPE,
+                                           stderr=subprocess.PIPE)
+            output, unused = dpkg_source.communicate()
+            result = dpkg_source.wait()
+        finally:
+            # When all is said and done, chdir out again so that we can
+            # clean up the tree with shutil.rmtree without leaving the
+            # process in a directory we're trying to remove.
+            os.chdir(cwd)
+
+        if result != 0:
+            dpkg_output = prefix_multi_line_string(output, "  ")
+            raise DpkgSourceError(result=result, output=dpkg_output)
+    except:
+        shutil.rmtree(unpacked_dir)
+        raise
+
+    return unpacked_dir
+
+
+def cleanup_unpacked_dir(unpacked_dir):
+    """Remove the directory with an unpacked source package.
+
+    :param unpacked_dir: Path to the directory.
+    """
+    try:
+        shutil.rmtree(unpacked_dir)
+    except OSError, error:
+        if errno.errorcode[error.errno] != 'EACCES':
+            raise UploadError(
+                "couldn't remove tmp dir %s: code %s" % (
+                unpacked_dir, error.errno))
+        else:
+            result = os.system("chmod -R u+rwx " + unpacked_dir)
+            if result != 0:
+                raise UploadError("chmod failed with %s" % result)
+            shutil.rmtree(unpacked_dir)
+
+
 class SignableTagFile:
     """Base class for signed file verification."""
 
@@ -160,7 +225,7 @@
             "rfc2047": rfc2047,
             "name": name,
             "email": email,
-            "person": person
+            "person": person,
             }
 
 
@@ -187,9 +252,9 @@
 
     # Note that files is actually only set inside verify().
     files = None
-    # Copyright and changelog_path are only set inside unpackAndCheckSource().
+    # Copyright and changelog are only set inside unpackAndCheckSource().
     copyright = None
-    changelog_path = None
+    changelog = None
 
     def __init__(self, filepath, digest, size, component_and_section,
                  priority, package, version, changes, policy, logger):
@@ -238,12 +303,9 @@
         else:
             self.processSignature()
 
-        self.unpacked_dir = None
-
     #
     # Useful properties.
     #
-
     @property
     def source(self):
         """Return the DSC source name."""
@@ -277,12 +339,11 @@
     #
     # DSC file checks.
     #
-
     def verify(self):
         """Verify the uploaded .dsc file.
 
-        This method is an error generator, i.e, it returns an iterator over all
-        exceptions that are generated while processing DSC file checks.
+        This method is an error generator, i.e, it returns an iterator over
+        all exceptions that are generated while processing DSC file checks.
         """
 
         for error in SourceUploadFile.verify(self):
@@ -518,82 +579,53 @@
         self.logger.debug(
             "Verifying uploaded source package by unpacking it.")
 
-        # Get a temporary dir together.
-        self.unpacked_dir = tempfile.mkdtemp()
-
-        # chdir into it
-        cwd = os.getcwd()
-        os.chdir(self.unpacked_dir)
-        dsc_in_tmpdir = os.path.join(self.unpacked_dir, self.filename)
-
-        package_files = self.files + [self]
         try:
-            for source_file in package_files:
-                os.symlink(
-                    source_file.filepath,
-                    os.path.join(self.unpacked_dir, source_file.filename))
-            args = ["dpkg-source", "-sn", "-x", dsc_in_tmpdir]
-            dpkg_source = subprocess.Popen(args, stdout=subprocess.PIPE,
-                                           stderr=subprocess.PIPE)
-            output, unused = dpkg_source.communicate()
-            result = dpkg_source.wait()
-        finally:
-            # When all is said and done, chdir out again so that we can
-            # clean up the tree with shutil.rmtree without leaving the
-            # process in a directory we're trying to remove.
-            os.chdir(cwd)
-
-        if result != 0:
-            dpkg_output = prefix_multi_line_string(output, "  ")
+            unpacked_dir = unpack_source(self.filepath)
+        except DpkgSourceError, e:
             yield UploadError(
                 "dpkg-source failed for %s [return: %s]\n"
                 "[dpkg-source output: %s]"
-                % (self.filename, result, dpkg_output))
-
-        # Copy debian/copyright file content. It will be stored in the
-        # SourcePackageRelease records.
-
-        # Check if 'dpkg-source' created only one directory.
-        temp_directories = [
-            dirname for dirname in os.listdir(self.unpacked_dir)
-            if os.path.isdir(dirname)]
-        if len(temp_directories) > 1:
-            yield UploadError(
-                'Unpacked source contains more than one directory: %r'
-                % temp_directories)
-
-        # XXX cprov 20070713: We should access only the expected directory
-        # name (<sourcename>-<no_epoch(no_revision(version))>).
-
-        # Locate both the copyright and changelog files for later processing.
-        for error in findCopyright(self, self.unpacked_dir, self.logger):
-            yield error
-
-        for error in findChangelog(self, self.unpacked_dir, self.logger):
-            yield error
-
-        self.logger.debug("Cleaning up source tree.")
+                % (self.filename, e.result, e.output))
+            return
+
+        try:
+            # Copy debian/copyright file content. It will be stored in the
+            # SourcePackageRelease records.
+
+            # Check if 'dpkg-source' created only one directory.
+            temp_directories = [
+                dirname for dirname in os.listdir(unpacked_dir)
+                if os.path.isdir(dirname)]
+            if len(temp_directories) > 1:
+                yield UploadError(
+                    'Unpacked source contains more than one directory: %r'
+                    % temp_directories)
+
+            # XXX cprov 20070713: We should access only the expected directory
+            # name (<sourcename>-<no_epoch(no_revision(version))>).
+
+            # Locate both the copyright and changelog files for later
+            # processing.
+            try:
+                self.copyright = find_copyright(unpacked_dir, self.logger)
+            except UploadError, error:
+                yield error
+                return
+            except UploadWarning, warning:
+                yield warning
+
+            try:
+                self.changelog = find_changelog(unpacked_dir, self.logger)
+            except UploadError, error:
+                yield error
+                return
+            except UploadWarning, warning:
+                yield warning
+        finally:
+            self.logger.debug("Cleaning up source tree.")
+            cleanup_unpacked_dir(unpacked_dir)
         self.logger.debug("Done")
 
-    def cleanUp(self):
-        if self.unpacked_dir is None:
-            return
-        try:
-            shutil.rmtree(self.unpacked_dir)
-        except OSError, error:
-            # XXX: dsilvers 2006-03-15: We currently lack a test for this.
-            if errno.errorcode[error.errno] != 'EACCES':
-                raise UploadError(
-                    "%s: couldn't remove tmp dir %s: code %s" % (
-                    self.filename, self.unpacked_dir, error.errno))
-            else:
-                result = os.system("chmod -R u+rwx " + self.unpacked_dir)
-                if result != 0:
-                    raise UploadError("chmod failed with %s" % result)
-                shutil.rmtree(self.unpacked_dir)
-        self.unpacked_dir = None
-
-
     def findBuild(self):
         """Find and return the SourcePackageRecipeBuild, if one is specified.
 
@@ -651,8 +683,8 @@
 
         changelog_lfa = self.librarian.create(
             "changelog",
-            os.stat(self.changelog_path).st_size,
-            open(self.changelog_path, "r"),
+            len(self.changelog),
+            StringIO(self.changelog),
             "text/x-debian-source-changelog",
             restricted=self.policy.archive.private)
 
@@ -716,6 +748,7 @@
           validation inside DSCFile.verify(); there is no
           store_in_database() method.
     """
+
     def __init__(self, filepath, digest, size, policy, logger):
         component_and_section = priority = "--no-value--"
         NascentUploadFile.__init__(
@@ -735,7 +768,7 @@
 
     :param source_file: The directory where the source was extracted
     :param source_dir: The directory where the source was extracted.
-    :return fullpath: The full path of the file, else return None if the 
+    :return fullpath: The full path of the file, else return None if the
                       file is not found.
     """
     # Instead of trying to predict the unpacked source directory name,
@@ -758,50 +791,42 @@
             return fullpath
     return None
 
-def findCopyright(dsc_file, source_dir, logger):
+
+def find_copyright(source_dir, logger):
     """Find and store any debian/copyright.
 
-    :param dsc_file: A DSCFile object where the copyright will be stored.
     :param source_dir: The directory where the source was extracted.
     :param logger: A logger object for debug output.
+    :return: Contents of copyright file
     """
-    try:
-        copyright_file = findFile(source_dir, 'debian/copyright')
-    except UploadError, error:
-        yield error
-        return
+    copyright_file = findFile(source_dir, 'debian/copyright')
     if copyright_file is None:
-        yield UploadWarning("No copyright file found.")
-        return
+        raise UploadWarning("No copyright file found.")
 
     logger.debug("Copying copyright contents.")
-    dsc_file.copyright = open(copyright_file).read().strip()
-
-
-def findChangelog(dsc_file, source_dir, logger):
+    return open(copyright_file).read().strip()
+
+
+def find_changelog(source_dir, logger):
     """Find and move any debian/changelog.
 
     This function finds the changelog file within the source package. The
     changelog file is later uploaded to the librarian by
     DSCFile.storeInDatabase().
 
-    :param dsc_file: A DSCFile object where the copyright will be stored.
     :param source_dir: The directory where the source was extracted.
     :param logger: A logger object for debug output.
+    :return: Changelog contents
     """
-    try:
-        changelog_file = findFile(source_dir, 'debian/changelog')
-    except UploadError, error:
-        yield error
-        return
+    changelog_file = findFile(source_dir, 'debian/changelog')
     if changelog_file is None:
         # Policy requires debian/changelog to always exist.
-        yield UploadError("No changelog file found.")
-        return
+        raise UploadError("No changelog file found.")
 
     # Move the changelog file out of the package direcotry
     logger.debug("Found changelog")
-    dsc_file.changelog_path = changelog_file
+    return open(changelog_file, 'r').read()
+
 
 
 def check_format_1_0_files(filename, file_type_counts, component_counts,

=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py	2010-08-20 20:31:18 +0000
+++ lib/lp/archiveuploader/nascentupload.py	2010-08-24 16:45:57 +0000
@@ -893,12 +893,6 @@
                 'Exception while accepting:\n %s' % e, exc_info=True)
             self.do_reject(notify)
             return False
-        else:
-            self.cleanUp()
-
-    def cleanUp(self):
-        if self.changes.dsc is not None:
-            self.changes.dsc.cleanUp()
 
     def do_reject(self, notify=True):
         """Reject the current upload given the reason provided."""
@@ -929,7 +923,6 @@
         self.queue_root.notify(summary_text=self.rejection_message,
             changes_file_object=changes_file_object, logger=self.logger)
         changes_file_object.close()
-        self.cleanUp()
 
     def _createQueueEntry(self):
         """Return a PackageUpload object."""

=== modified file 'lib/lp/archiveuploader/tests/test_dscfile.py'
--- lib/lp/archiveuploader/tests/test_dscfile.py	2010-08-20 20:31:18 +0000
+++ lib/lp/archiveuploader/tests/test_dscfile.py	2010-08-24 16:45:57 +0000
@@ -10,10 +10,12 @@
 from canonical.launchpad.scripts.logger import QuietFakeLogger
 from canonical.testing.layers import LaunchpadZopelessLayer
 from lp.archiveuploader.dscfile import (
+    cleanup_unpacked_dir,
     DSCFile,
-    findChangelog,
-    findCopyright,
+    find_changelog,
+    find_copyright,
     format_to_file_checker_map,
+    unpack_source,
     )
 from lp.archiveuploader.nascentuploadfile import UploadError
 from lp.archiveuploader.tests import (
@@ -37,9 +39,6 @@
 
 class TestDscFile(TestCase):
 
-    class MockDSCFile:
-        copyright = None
-
     def setUp(self):
         super(TestDscFile, self).setUp()
         self.tmpdir = self.makeTemporaryDirectory()
@@ -47,7 +46,6 @@
         os.makedirs(self.dir_path)
         self.copyright_path = os.path.join(self.dir_path, "copyright")
         self.changelog_path = os.path.join(self.dir_path, "changelog")
-        self.dsc_file = self.MockDSCFile()
 
     def testBadDebianCopyright(self):
         """Test that a symlink as debian/copyright will fail.
@@ -56,14 +54,10 @@
         dangling symlink in an attempt to try and access files on the system
         processing the source packages."""
         os.symlink("/etc/passwd", self.copyright_path)
-        errors = list(findCopyright(
-            self.dsc_file, self.tmpdir, mock_logger_quiet))
-
-        self.assertEqual(len(errors), 1)
-        self.assertIsInstance(errors[0], UploadError)
+        error = self.assertRaises(
+            UploadError, find_copyright, self.tmpdir, mock_logger_quiet)
         self.assertEqual(
-            errors[0].args[0],
-            "Symbolic link for debian/copyright not allowed")
+            error.args[0], "Symbolic link for debian/copyright not allowed")
 
     def testGoodDebianCopyright(self):
         """Test that a proper copyright file will be accepted"""
@@ -72,11 +66,8 @@
         file.write(copyright)
         file.close()
 
-        errors = list(findCopyright(
-            self.dsc_file, self.tmpdir, mock_logger_quiet))
-
-        self.assertEqual(len(errors), 0)
-        self.assertEqual(self.dsc_file.copyright, copyright)
+        self.assertEquals(
+            copyright, find_copyright(self.tmpdir, mock_logger_quiet))
 
     def testBadDebianChangelog(self):
         """Test that a symlink as debian/changelog will fail.
@@ -85,14 +76,10 @@
         dangling symlink in an attempt to try and access files on the system
         processing the source packages."""
         os.symlink("/etc/passwd", self.changelog_path)
-        errors = list(findChangelog(
-            self.dsc_file, self.tmpdir, mock_logger_quiet))
-
-        self.assertEqual(len(errors), 1)
-        self.assertIsInstance(errors[0], UploadError)
+        error = self.assertRaises(
+            UploadError, find_changelog, self.tmpdir, mock_logger_quiet)
         self.assertEqual(
-            errors[0].args[0],
-            "Symbolic link for debian/changelog not allowed")
+            error.args[0], "Symbolic link for debian/changelog not allowed")
 
     def testGoodDebianChangelog(self):
         """Test that a proper changelog file will be accepted"""
@@ -101,12 +88,8 @@
         file.write(changelog)
         file.close()
 
-        errors = list(findChangelog(
-            self.dsc_file, self.tmpdir, mock_logger_quiet))
-
-        self.assertEqual(len(errors), 0)
-        self.assertEqual(self.dsc_file.changelog_path,
-                         self.changelog_path)
+        self.assertEquals(
+            changelog, find_changelog(self.tmpdir, mock_logger_quiet))
 
     def testOversizedFile(self):
         """Test that a file larger than 10MiB will fail.
@@ -125,13 +108,10 @@
         file.write(empty_file)
         file.close()
 
-        errors = list(findChangelog(
-            self.dsc_file, self.tmpdir, mock_logger_quiet))
-
-        self.assertIsInstance(errors[0], UploadError)
+        error = self.assertRaises(
+            UploadError, find_changelog, self.tmpdir, mock_logger_quiet)
         self.assertEqual(
-            errors[0].args[0],
-            "debian/changelog file too large, 10MiB max")
+            error.args[0], "debian/changelog file too large, 10MiB max")
 
 
 class TestDscFileLibrarian(TestCaseWithFactory):
@@ -141,6 +121,7 @@
 
     def getDscFile(self, name):
         dsc_path = datadir(os.path.join('suite', name, name + '.dsc'))
+
         class Changes:
             architectures = ['source']
         logger = QuietFakeLogger()
@@ -157,10 +138,7 @@
         os.chmod(tempdir, 0555)
         try:
             dsc_file = self.getDscFile('bar_1.0-1')
-            try:
-                list(dsc_file.verify())
-            finally:
-                dsc_file.cleanUp()
+            list(dsc_file.verify())
         finally:
             os.chmod(tempdir, 0755)
 
@@ -292,3 +270,42 @@
         # A 3.0 (native) source with component tarballs is invalid.
         self.assertErrorsForFiles(
             [self.wrong_files_error], {NATIVE_TARBALL: 1}, {'foo': 1})
+
+
+class UnpackedDirTests(TestCase):
+    """Tests for unpack_source and cleanup_unpacked_dir."""
+
+    def test_unpack_source(self):
+        # unpack_source unpacks in a temporary directory and returns the
+        # path.
+        unpacked_dir = unpack_source(
+            datadir(os.path.join('suite', 'bar_1.0-1', 'bar_1.0-1.dsc')))
+        try:
+            self.assertEquals(["bar-1.0"], os.listdir(unpacked_dir))
+            self.assertContentEqual(
+                ["THIS_IS_BAR", "debian"],
+                os.listdir(os.path.join(unpacked_dir, "bar-1.0")))
+        finally:
+            cleanup_unpacked_dir(unpacked_dir)
+
+    def test_cleanup(self):
+        # cleanup_dir removes the temporary directory and all files under it.
+        temp_dir = self.makeTemporaryDirectory()
+        unpacked_dir = os.path.join(temp_dir, "unpacked")
+        os.mkdir(unpacked_dir)
+        os.mkdir(os.path.join(unpacked_dir, "bar_1.0"))
+        cleanup_unpacked_dir(unpacked_dir)
+        self.assertFalse(os.path.exists(unpacked_dir))
+
+    def test_cleanup_invalid_mode(self):
+        # cleanup_dir can remove a directory even if the mode does
+        # not allow it.
+        temp_dir = self.makeTemporaryDirectory()
+        unpacked_dir = os.path.join(temp_dir, "unpacked")
+        os.mkdir(unpacked_dir)
+        bar_path = os.path.join(unpacked_dir, "bar_1.0")
+        os.mkdir(bar_path)
+        os.chmod(bar_path, 0600)
+        os.chmod(unpacked_dir, 0600)
+        cleanup_unpacked_dir(unpacked_dir)
+        self.assertFalse(os.path.exists(unpacked_dir))

=== modified file 'lib/lp/archiveuploader/tests/test_nascentuploadfile.py'
--- lib/lp/archiveuploader/tests/test_nascentuploadfile.py	2010-08-21 13:54:20 +0000
+++ lib/lp/archiveuploader/tests/test_nascentuploadfile.py	2010-08-24 16:45:57 +0000
@@ -169,8 +169,7 @@
         uploadfile = self.createDSCFile(
             "foo.dsc", dsc, "main/net", "extra", "dulwich", "0.42",
             self.createChangesFile("foo.changes", changes))
-        (uploadfile.changelog_path, changelog_digest, changelog_size) = (
-            self.writeUploadFile("changelog", "DUMMY"))
+        uploadfile.changelog = "DUMMY"
         uploadfile.files = []
         release = uploadfile.storeInDatabase(None)
         self.assertEquals("0.42", release.version)

=== modified file 'lib/lp/hardwaredb/model/hwdb.py'
--- lib/lp/hardwaredb/model/hwdb.py	2010-08-20 20:31:18 +0000
+++ lib/lp/hardwaredb/model/hwdb.py	2010-08-24 16:45:57 +0000
@@ -64,9 +64,6 @@
     SQLBase,
     sqlvalues,
     )
-from canonical.launchpad.components.decoratedresultset import (
-    DecoratedResultSet,
-    )
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
 from canonical.launchpad.validators.name import valid_name
@@ -347,13 +344,7 @@
         # DISTINCT clause.
         result_set.config(distinct=True)
         result_set.order_by(HWSubmission.id)
-        # The Storm implementation of ResultSet.count() is incorrect if
-        # the select query uses the distinct directive (see bug #217644).
-        # DecoratedResultSet solves this problem by modifying the query
-        # to count only the records appearing in a subquery.
-        # We don't actually need to transform the results, which is why
-        # the second argument is a no-op.
-        return DecoratedResultSet(result_set, lambda result: result)
+        return result_set
 
     def _submissionsSubmitterSelects(
         self, target_column, bus, vendor_id, product_id, driver_name,

=== modified file 'lib/lp/registry/browser/distribution.py'
--- lib/lp/registry/browser/distribution.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/distribution.py	2010-08-24 16:45:57 +0000
@@ -474,18 +474,7 @@
         """See `AbstractPackageSearchView`."""
 
         if self.search_by_binary_name:
-            non_exact_matches = self.context.searchBinaryPackages(self.text)
-
-            # XXX Michael Nelson 20090605 bug=217644
-            # We are only using a decorated resultset here to conveniently
-            # get around the storm bug whereby count returns the count
-            # of non-distinct results, even though this result set
-            # is configured for distinct results.
-            def dummy_func(result):
-                return result
-            non_exact_matches = DecoratedResultSet(
-                non_exact_matches, dummy_func)
-
+            return self.context.searchBinaryPackages(self.text)
         else:
             non_exact_matches = self.context.searchSourcePackageCaches(
                 self.text)

=== modified file 'lib/lp/registry/browser/sourcepackage.py'
--- lib/lp/registry/browser/sourcepackage.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/sourcepackage.py	2010-08-24 16:45:57 +0000
@@ -542,7 +542,7 @@
         self.form_fields = Fields(
             Choice(__name__='upstream',
                    title=_('Registered upstream project'),
-                   default=None,
+                   default=self.other_upstream,
                    vocabulary=upstream_vocabulary,
                    required=True))
 

=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/team.py	2010-08-24 16:45:57 +0000
@@ -150,7 +150,7 @@
         "name", "visibility", "displayname", "contactemail",
         "teamdescription", "subscriptionpolicy",
         "defaultmembershipperiod", "renewal_policy",
-        "defaultrenewalperiod",  "teamowner",
+        "defaultrenewalperiod", "teamowner",
         ]
     private_prefix = PRIVATE_TEAM_PREFIX
 
@@ -767,7 +767,7 @@
 
     def renderTable(self):
         html = ['<table style="max-width: 80em">']
-        items = self.subscribers.currentBatch()
+        items = list(self.subscribers.currentBatch())
         assert len(items) > 0, (
             "Don't call this method if there are no subscribers to show.")
         # When there are more than 10 items, we use multiple columns, but

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2010-08-23 08:12:39 +0000
+++ lib/lp/registry/model/distroseries.py	2010-08-24 16:45:57 +0000
@@ -343,7 +343,7 @@
     @cachedproperty
     def _all_packagings(self):
         """Get an unordered list of all packagings.
-        
+
         :return: A ResultSet which can be decorated or tuned further. Use
             DistroSeries._packaging_row_to_packaging to extract the
             packaging objects out.
@@ -353,7 +353,7 @@
         # Packaging object.
         # NB: precaching objects like this method tries to do has a very poor
         # hit rate with storm - many queries will still be executed; consider
-        # ripping this out and instead allowing explicit inclusion of things 
+        # ripping this out and instead allowing explicit inclusion of things
         # like Person._all_members does - returning a cached object graph.
         # -- RBC 20100810
         # Avoid circular import failures.
@@ -1810,11 +1810,7 @@
             DistroSeries.hide_all_translations == False,
             DistroSeries.id == POTemplate.distroseriesID)
         result_set = result_set.config(distinct=True)
-        # XXX: henninge 2009-02-11 bug=217644: Convert to sequence right here
-        # because ResultSet reports a wrong count() when using DISTINCT. Also
-        # ResultSet does not implement __len__(), which would make it more
-        # like a sequence.
-        return list(result_set)
+        return result_set
 
     def findByName(self, name):
         """See `IDistroSeriesSet`."""

=== modified file 'lib/lp/registry/model/mailinglist.py'
--- lib/lp/registry/model/mailinglist.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/mailinglist.py	2010-08-24 16:45:57 +0000
@@ -389,7 +389,7 @@
                              TeamParticipation.team == self.team,
                              MailingListSubscription.person == Person.id,
                              MailingListSubscription.mailing_list == self)
-        return results.order_by(Person.displayname)
+        return results.order_by(Person.displayname, Person.name)
 
     def subscribe(self, person, address=None):
         """See `IMailingList`."""
@@ -451,8 +451,9 @@
                      MailingListSubscription.personID
                      == EmailAddress.personID),
             # pylint: disable-msg=C0301
-            LeftJoin(MailingList,
-                     MailingList.id == MailingListSubscription.mailing_listID),
+            LeftJoin(
+                MailingList,
+                MailingList.id == MailingListSubscription.mailing_listID),
             LeftJoin(TeamParticipation,
                      TeamParticipation.personID
                      == MailingListSubscription.personID),
@@ -472,8 +473,9 @@
                      MailingListSubscription.email_addressID
                      == EmailAddress.id),
             # pylint: disable-msg=C0301
-            LeftJoin(MailingList,
-                     MailingList.id == MailingListSubscription.mailing_listID),
+            LeftJoin(
+                MailingList,
+                MailingList.id == MailingListSubscription.mailing_listID),
             LeftJoin(TeamParticipation,
                      TeamParticipation.personID
                      == MailingListSubscription.personID),
@@ -664,8 +666,9 @@
                      MailingListSubscription.personID
                      == EmailAddress.personID),
             # pylint: disable-msg=C0301
-            LeftJoin(MailingList,
-                     MailingList.id == MailingListSubscription.mailing_listID),
+            LeftJoin(
+                MailingList,
+                MailingList.id == MailingListSubscription.mailing_listID),
             LeftJoin(TeamParticipation,
                      TeamParticipation.personID
                      == MailingListSubscription.personID),
@@ -678,8 +681,7 @@
             team.id for team in store.find(
                 Person,
                 And(Person.name.is_in(team_names),
-                    Person.teamowner != None))
-            )
+                    Person.teamowner != None)))
         list_ids = set(
             mailing_list.id for mailing_list in store.find(
                 MailingList,
@@ -709,8 +711,9 @@
                      MailingListSubscription.email_addressID
                      == EmailAddress.id),
             # pylint: disable-msg=C0301
-            LeftJoin(MailingList,
-                     MailingList.id == MailingListSubscription.mailing_listID),
+            LeftJoin(
+                MailingList,
+                MailingList.id == MailingListSubscription.mailing_listID),
             LeftJoin(TeamParticipation,
                      TeamParticipation.personID
                      == MailingListSubscription.personID),
@@ -756,8 +759,7 @@
             team.id for team in store.find(
                 Person,
                 And(Person.name.is_in(team_names),
-                    Person.teamowner != None))
-            )
+                    Person.teamowner != None)))
         team_members = store.using(*tables).find(
             (Team.name, Person.displayname, EmailAddress.email),
             And(TeamParticipation.teamID.is_in(team_ids),

=== modified file 'lib/lp/registry/tests/test_mailinglist.py'
--- lib/lp/registry/tests/test_mailinglist.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/tests/test_mailinglist.py	2010-08-24 16:45:57 +0000
@@ -1,64 +1,64 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from __future__ import with_statement
+
 __metaclass__ = type
 __all__ = []
 
-
-import unittest
-
-from canonical.launchpad.ftests import login
-from canonical.testing import LaunchpadFunctionalLayer
+from canonical.testing import DatabaseFunctionalLayer
 from lp.registry.interfaces.mailinglistsubscription import (
     MailingListAutoSubscribePolicy,
     )
 from lp.registry.interfaces.person import TeamSubscriptionPolicy
-from lp.testing import TestCaseWithFactory
+from lp.testing import login_celebrity, person_logged_in, TestCaseWithFactory
 
 
 class MailingList_getSubscribers_TestCase(TestCaseWithFactory):
     """Tests for `IMailingList`.getSubscribers()."""
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
-        # Create a team (tied to a mailing list) with one former member, one
-        # pending member and one active member.
         TestCaseWithFactory.setUp(self)
-        login('[e-mail]')
+        self.team, self.mailing_list = self.factory.makeTeamAndMailingList(
+            'test-mailinglist', 'team-owner')
+
+    def test_only_active_members_can_be_subscribers(self):
         former_member = self.factory.makePerson()
         pending_member = self.factory.makePerson()
         active_member = self.active_member = self.factory.makePerson()
-        self.team, self.mailing_list = self.factory.makeTeamAndMailingList(
-            'test-mailinglist', 'team-owner')
-        self.team.subscriptionpolicy = TeamSubscriptionPolicy.MODERATED
-
         # Each of our members want to be subscribed to a team's mailing list
         # whenever they join the team.
+        login_celebrity('admin')
         former_member.mailing_list_auto_subscribe_policy = (
             MailingListAutoSubscribePolicy.ALWAYS)
         active_member.mailing_list_auto_subscribe_policy = (
             MailingListAutoSubscribePolicy.ALWAYS)
         pending_member.mailing_list_auto_subscribe_policy = (
             MailingListAutoSubscribePolicy.ALWAYS)
-
+        self.team.subscriptionpolicy = TeamSubscriptionPolicy.MODERATED
         pending_member.join(self.team)
-        self.assertEqual(False, pending_member.inTeam(self.team))
-
         self.team.addMember(former_member, reviewer=self.team.teamowner)
         former_member.leave(self.team)
-        self.assertEqual(False, former_member.inTeam(self.team))
-
         self.team.addMember(active_member, reviewer=self.team.teamowner)
-        self.assertEqual(True, active_member.inTeam(self.team))
-
-    def test_only_active_members_can_be_subscribers(self):
         # Even though our 3 members want to subscribe to the team's mailing
         # list, only the active member is considered a subscriber.
-        subscribers = [self.active_member]
-        self.assertEqual(
-            subscribers, list(self.mailing_list.getSubscribers()))
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+        self.assertEqual(
+            [active_member], list(self.mailing_list.getSubscribers()))
+
+    def test_getSubscribers_order(self):
+        person_1 = self.factory.makePerson(name="pb1", displayname="Me")
+        with person_logged_in(person_1):
+            person_1.mailing_list_auto_subscribe_policy = (
+                MailingListAutoSubscribePolicy.ALWAYS)
+            person_1.join(self.team)
+        person_2 = self.factory.makePerson(name="pa2", displayname="Me")
+        with person_logged_in(person_2):
+            person_2.mailing_list_auto_subscribe_policy = (
+                MailingListAutoSubscribePolicy.ALWAYS)
+            person_2.join(self.team)
+        subscribers = self.mailing_list.getSubscribers()
+        self.assertEqual(2, subscribers.count())
+        self.assertEqual(
+            ['pa2', 'pb1'], [person.name for person in subscribers])

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2010-08-22 03:09:51 +0000
+++ lib/lp/registry/vocabularies.py	2010-08-24 16:45:57 +0000
@@ -95,9 +95,6 @@
     SQLBase,
     sqlvalues,
     )
-from canonical.launchpad.components.decoratedresultset import (
-    DecoratedResultSet,
-    )
 from canonical.launchpad.database.account import Account
 from canonical.launchpad.database.emailaddress import EmailAddress
 from canonical.launchpad.database.stormsugar import StartsWith
@@ -648,10 +645,7 @@
         else:
             result.order_by(Person.displayname, Person.name)
         result.config(limit=self.LIMIT)
-        # XXX: BradCrittenden 2009-04-24 bug=217644: Wrap the results to
-        # ensure the .count() method works until the Storm bug is fixed and
-        # integrated.
-        return DecoratedResultSet(result)
+        return result
 
     def search(self, text):
         """Return people/teams whose fti or email address match :text:."""
@@ -727,10 +721,7 @@
         result.config(distinct=True)
         result.order_by(Person.displayname, Person.name)
         result.config(limit=self.LIMIT)
-        # XXX: BradCrittenden 2009-04-24 bug=217644: Wrap the results to
-        # ensure the .count() method works until the Storm bug is fixed and
-        # integrated.
-        return DecoratedResultSet(result)
+        return result
 
 
 class ValidPersonVocabulary(ValidPersonOrTeamVocabulary):

=== modified file 'lib/lp/soyuz/doc/package-diff.txt'
--- lib/lp/soyuz/doc/package-diff.txt	2010-05-13 12:04:56 +0000
+++ lib/lp/soyuz/doc/package-diff.txt	2010-08-24 16:45:57 +0000
@@ -451,12 +451,7 @@
     >>> packagediff_set.getPendingDiffs().count()
     7
 
-XXX cprov 20070530: storm doesn't go well with  limited count()s
-See bug #217644. For now we have to listify the results and used
-the list length.
-
-    >>> r = packagediff_set.getPendingDiffs(limit=2)
-    >>> len(list(r))
+    >>> packagediff_set.getPendingDiffs(limit=2).count()
     2
 
 All package diffs targeting a set of source package releases can also

=== modified file 'lib/lp/soyuz/scripts/initialise_distroseries.py'
--- lib/lp/soyuz/scripts/initialise_distroseries.py	2010-08-20 20:31:18 +0000
+++ lib/lp/soyuz/scripts/initialise_distroseries.py	2010-08-24 16:45:57 +0000
@@ -25,8 +25,10 @@
     ArchivePurpose,
     IArchiveSet,
     )
+from lp.soyuz.interfaces.packageset import IPackagesetSet
 from lp.soyuz.interfaces.queue import PackageUploadStatus
 from lp.soyuz.model.packagecloner import clone_packages
+from lp.soyuz.model.packageset import Packageset
 
 
 class InitialisationError(Exception):
@@ -270,10 +272,28 @@
 
     def _copy_packagesets(self):
         """Copy packagesets from the parent distroseries."""
-        self._store.execute("""
-            INSERT INTO Packageset
-            (distroseries, owner, name, description, packagesetgroup)
-            SELECT %s, %s, name, description, packagesetgroup
-            FROM Packageset WHERE distroseries = %s
-            """ % sqlvalues(
-            self.distroseries, self.distroseries.owner, self.parent))
+        packagesets = self._store.find(Packageset, distroseries=self.parent)
+        parent_to_child = {}
+        # Create the packagesets, and any archivepermissions
+        for parent_ps in packagesets:
+            child_ps = getUtility(IPackagesetSet).new(
+                parent_ps.name, parent_ps.description,
+                self.distroseries.owner, distroseries=self.distroseries,
+                related_set=parent_ps)
+            self._store.execute("""
+                INSERT INTO Archivepermission
+                (person, permission, archive, packageset, explicit)
+                SELECT person, permission, %s, %s, explicit
+                FROM Archivepermission WHERE packageset = %s
+                """ % sqlvalues(
+                    self.distroseries.main_archive, child_ps.id,
+                    parent_ps.id))
+            parent_to_child[parent_ps] = child_ps
+        # Copy the relations between sets, and the contents
+        for old_series_ps, new_series_ps in parent_to_child.items():
+            old_series_sets = old_series_ps.setsIncluded(
+                direct_inclusion=True)
+            for old_series_child in old_series_sets:
+                new_series_ps.add(parent_to_child[old_series_child])
+            new_series_ps.add(old_series_ps.sourcesIncluded(
+                direct_inclusion=True))

=== modified file 'lib/lp/soyuz/scripts/tests/test_initialise_distroseries.py'
--- lib/lp/soyuz/scripts/tests/test_initialise_distroseries.py	2010-08-20 20:31:18 +0000
+++ lib/lp/soyuz/scripts/tests/test_initialise_distroseries.py	2010-08-24 16:45:57 +0000
@@ -23,6 +23,7 @@
 from canonical.testing.layers import LaunchpadZopelessLayer
 from lp.buildmaster.interfaces.buildbase import BuildStatus
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
 from lp.soyuz.interfaces.packageset import IPackagesetSet
 from lp.soyuz.interfaces.sourcepackageformat import SourcePackageFormat
 from lp.soyuz.model.distroarchseries import DistroArchSeries
@@ -87,7 +88,7 @@
             self.ubuntu['breezy-autotest'])
         ids = InitialiseDistroSeries(foobuntu)
         self.assertRaisesWithContent(
-            InitialisationError,"Parent series queues are not empty.",
+            InitialisationError, "Parent series queues are not empty.",
             ids.check)
 
     def assertDistroSeriesInitialisedCorrectly(self, foobuntu):
@@ -191,6 +192,7 @@
 
     def test_copying_packagesets(self):
         # If a parent series has packagesets, we should copy them
+        uploader = self.factory.makePerson()
         test1 = getUtility(IPackagesetSet).new(
             u'test1', u'test 1 packageset', self.hoary.owner,
             distroseries=self.hoary)
@@ -199,13 +201,11 @@
             distroseries=self.hoary)
         test3 = getUtility(IPackagesetSet).new(
             u'test3', u'test 3 packageset', self.hoary.owner,
-            distroseries=self.hoary)
-        foobuntu = self._create_distroseries(self.hoary)
-        self._set_pending_to_failed(self.hoary)
-        transaction.commit()
-        ids = InitialiseDistroSeries(foobuntu)
-        ids.check()
-        ids.initialise()
+            distroseries=self.hoary, related_set=test2)
+        test1.addSources('pmount')
+        getUtility(IArchivePermissionSet).newPackagesetUploader(
+            self.hoary.main_archive, uploader, test1)
+        foobuntu = self._full_initialise()
         # We can fetch the copied sets from foobuntu
         foobuntu_test1 = getUtility(IPackagesetSet).getByName(
             u'test1', distroseries=foobuntu)
@@ -219,8 +219,26 @@
         self.assertEqual(test2.description, foobuntu_test2.description)
         self.assertEqual(test3.description, foobuntu_test3.description)
         self.assertEqual(foobuntu_test1.relatedSets().one(), test1)
-        self.assertEqual(foobuntu_test2.relatedSets().one(), test2)
-        self.assertEqual(foobuntu_test3.relatedSets().one(), test3)
+        self.assertEqual(
+            list(foobuntu_test2.relatedSets()),
+            [test2, test3, foobuntu_test3])
+        self.assertEqual(
+            list(foobuntu_test3.relatedSets()),
+            [test2, foobuntu_test2, test3])
+        # The contents of the packagesets will have been copied.
+        foobuntu_srcs = foobuntu_test1.getSourcesIncluded(
+            direct_inclusion=True)
+        hoary_srcs = test1.getSourcesIncluded(direct_inclusion=True)
+        self.assertEqual(foobuntu_srcs, hoary_srcs)
+        # The uploader can also upload to the new distroseries.
+        self.assertTrue(
+            getUtility(IArchivePermissionSet).isSourceUploadAllowed(
+                self.hoary.main_archive, 'pmount', uploader,
+                distroseries=self.hoary))
+        self.assertTrue(
+            getUtility(IArchivePermissionSet).isSourceUploadAllowed(
+                foobuntu.main_archive, 'pmount', uploader,
+                distroseries=foobuntu))
 
     def test_script(self):
         # Do an end-to-end test using the command-line tool

=== modified file 'lib/lp/testing/fakelibrarian.py'
--- lib/lp/testing/fakelibrarian.py	2010-08-20 20:31:18 +0000
+++ lib/lp/testing/fakelibrarian.py	2010-08-24 16:45:57 +0000
@@ -151,6 +151,19 @@
         alias.checkCommitted()
         return StringIO(alias.content_string)
 
+    def pretendCommit(self):
+        """Pretend that there's been a commit.
+
+        When you add a file to the librarian (real or fake), it is not
+        fully available until the transaction that added the file has
+        been committed.  Call this method to make the FakeLibrarian act
+        as if there's been a commit, without actually committing a
+        database transaction.
+        """
+        # Note that all files have been committed to storage.
+        for alias in self.aliases.itervalues():
+            alias.file_committed = True
+
     def _makeAlias(self, file_id, name, content, content_type):
         """Create a `LibraryFileAlias`."""
         alias = InstrumentedLibraryFileAlias(
@@ -195,9 +208,7 @@
 
     def afterCompletion(self, txn):
         """See `ISynchronizer`."""
-        # Note that all files have been committed to storage.
-        for alias in self.aliases.itervalues():
-            alias.file_committed = True
+        self.pretendCommit()
 
     def newTransaction(self, txn):
         """See `ISynchronizer`."""

=== modified file 'lib/lp/testing/tests/test_fakelibrarian.py'
--- lib/lp/testing/tests/test_fakelibrarian.py	2010-08-20 20:31:18 +0000
+++ lib/lp/testing/tests/test_fakelibrarian.py	2010-08-24 16:45:57 +0000
@@ -109,6 +109,15 @@
         self.assertTrue(verifyObject(ISynchronizer, self.fake_librarian))
         self.assertIsInstance(self.fake_librarian, FakeLibrarian)
 
+    def test_pretend_commit(self):
+        name, text, alias_id = self._storeFile()
+
+        self.fake_librarian.pretendCommit()
+
+        retrieved_alias = getUtility(ILibraryFileAliasSet)[alias_id]
+        retrieved_alias.open()
+        self.assertEqual(text, retrieved_alias.read())
+
 
 class TestRealLibrarian(LibraryAccessScenarioMixin, TestCaseWithFactory):
     """Test the supported interface subset on the real librarian."""

=== modified file 'versions.cfg'
--- versions.cfg	2010-08-18 19:41:20 +0000
+++ versions.cfg	2010-08-24 16:45:57 +0000
@@ -101,7 +101,7 @@
 z3c.ptcompat = 0.5.3
 z3c.recipe.filetemplate = 2.1.0
 z3c.recipe.i18n = 0.5.3
-z3c.recipe.scripts = 1.0.0dev-gary-r110068
+z3c.recipe.scripts = 1.0.0
 z3c.recipe.tag = 0.2.0
 z3c.rml = 0.7.3
 z3c.skin.pagelet = 1.0.2
@@ -111,12 +111,12 @@
 z3c.viewlet = 1.0.0
 z3c.viewtemplate = 0.3.2
 z3c.zrtresource = 1.0.1
-zc.buildout = 1.5.0dev-gary-r111190
+zc.buildout = 1.5.0
 zc.catalog = 1.2.0
 zc.datetimewidget = 0.5.2
 zc.i18n = 0.5.2
 zc.lockfile = 1.0.0
-zc.recipe.egg = 1.2.3dev-gary-r110068
+zc.recipe.egg = 1.3.0
 zc.zservertracelog = 1.1.5
 ZConfig = 2.7.1
 zdaemon = 2.0.4