Merge lp:~timrchavez/launchpad/set_ppa_private_from_api_724740-4 into lp:launchpad

[Timothy R. Chavez]

[r=jcsackett][bug=724740] Provides facility to set a PPA private via the API

SUMMARY

As described by LP #724740, making a PPA private via the API is currently not possible.

PROPOSED FIX

Making a PPA private or public is not simply a boolean assignment. When a PPA is made private, a buildd_secret must be set and if it is made public again, the buildd_secret must be discarded. To avoid leakage / exposure of the buildd_secret it must be auto-generated internally rather than provided by the user via the API.

PRE-IMPLEMENTATION NOTES

The original plan that I discussed with Julian was to make the private attribute on IArchivePublic read-only, add a mutator, push all the _validate_archive_privacy logic into it and add the ability to generate and set the buildd_secret.

After speaking with Robert, I went with his Python properties suggestion so that both internal and external had a common interface for setting PPA privacy.

IMPLEMENTATION DETAILS

Because making a PPA private is more than just a boolean assignment, we use a Python setter to make the PPA private or public and to generate a buildd_secret as necessary.

Some things I learned from this approach:

Thinking that the 'updateContextFromData' method Jelmer introduced[1] to fix this problem for the UI was no longer necessary, I removed it, only to find that the view test for private set / buildd_secret unset had started failing.

It turns out that the 'buildd_secret' attribute was being set to None (the value passed via the form), after the 'private' attribute was being set (and we generated and set a new buildd_secret). The solution to this problem was to just remove the token creation bit from 'updateContextFromData', rather than the entire method.

Using a Python getter and setter for the private attribute broke Storm expressions. The "workaround" for this problem was to provide the Storm expressions with the underlying _private attribute that the private getter and setter access / manipulate.

TESTS

testr run -- -t test_archive_admin_view -t test_archive_privacy

I wrote an additional two tests in test_archive_privacy.py[2] to verify the buildd_secret is getting generated and set when a PPA is made private and that it is discarded if it the PPA is made public.

DEMO AND Q/A

To demo this you could write a small app which sets the 'private' attribute to True for a public PPA. Without this change, after calling lp_save() the value will revert back to False. With this change it should remain True. For example,

from launchpadlib.launchpad import Launchpad

launchpad = Launchpad.login_with(
        'test', service_root='https://api.launchpad.dev', version='devel')
user = launchpad.people['name16']
ppa = user.getPPAByName(name="test2")
ppa.private = True
ppa.lp_save()
print ppa.private

You could also go into the Archive web view and verify that the "Private" field is checked.

To verify there is no UI regression, you can go into the Archive web view, check the "Private" field, click Save, and then go back into the Archive web view to verify a buildd_secret has been generated and set for you (this sort of begs the question why this field even exists, though :))

LINT

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/browser/archive.py
  lib/lp/soyuz/model/archive.py
  lib/lp/soyuz/tests/test_archive_privacy.py

./lib/lp/soyuz/model/archive.py
     344: redefinition of function 'private' from line 340
make: *** [lint] Error 1

Apparently lint doesn't like this way of defining Python getters and setters?

[1] https://code.launchpad.net/~jelmer/launchpad/394798-auto-buildd-secret
[2] lib/lp/soyuz/tests/test_archive_privacy.py

[j.c.sackett (jcsackett)]

Hi Tim--

This looks pretty good, I have one comment (regarding the lint issue you pointed out) and one question regarding sourcedeps.cache

> === modified file 'lib/lp/soyuz/model/archive.py'
> --- lib/lp/soyuz/model/archive.py 2011-06-07 05:02:53 +0000
> +++ lib/lp/soyuz/model/archive.py 2011-06-09 03:09:34 +0000
> @@ -259,7 +260,7 @@
>
> publish = BoolCol(dbName='publish', notNull=True, default=True)
>
> - private = BoolCol(dbName='private', notNull=True, default=False,
> + _private = BoolCol(dbName='private', notNull=True, default=False,
> storm_validator=_validate_archive_privacy)
>
> require_virtualized = BoolCol(
> @@ -324,6 +325,19 @@
> alsoProvides(self, IDistributionArchive)
>
> @property
> + def private(self):
> + return self._private
> +
> + @private.setter
> + def private(self, private):
> + self._private = private
> + if private:
> + if not self.buildd_secret:
> + self.buildd_secret = create_token(20)
> + else:
> + self.buildd_secret = Nonek
> +
> + @property
> def title(self):
> """See `IArchive`."""
> return self.displayname

I may be out of date, but the way I've usually seen this done is

+ def _get_private(self):
+ return self._private
+
+ def _set_private(self):
+ self._private = private
+ if private:
+ if not self.buildd_secret:
+ self.buildd_secret = create_token(20)
+ else:
+ self.buildd_secret = None
+ private = property(_get_private, _set_private)

I see that .setter attr was introduced on the @property decorator
in 2.6; it's possible lint wasn't updated to catch this.

As @property.setter works, there's no need to change this, just providing an explanation. It may be worth it to add a comment indicating lint will complain so other people changing this file don't have to figure it out again themselves.

> === modified file 'utilities/sourcedeps.cache'
> --- utilities/sourcedeps.cache 2011-06-01 18:58:15 +0000
> +++ utilities/sourcedeps.cache 2011-06-09 03:09:34 +0000
> @@ -1,4 +1,8 @@
> {
> + "bzr-builder": [
> + 68,
> + "<email address hidden>"
> + ],
> "testresources": [
> 16,
> "<email address hidden>"
> @@ -27,18 +31,14 @@
> 24,
> "<email address hidden>"
> ],
> + "lpreview": [
> + 23,
> + "<email address hidden>"
> + ],
> "bzr-git": [
> 259,
> "<email address hidden>"
> ],
> - "loggerhead": [
> - 445,
> - "<email address hidden>"
> - ],
> - "bzr-builder": [
> - 68,
> - "<email address hidden>"
> - ],
> "bzr-loom": [
> 49,
> "<email address hidden>"
> @@ -47,9 +47,9 @@
> 4,
> "sinzui-20...

review: Needs Information

[Timothy R. Chavez (timrchavez)]

The sourcedeps.cache looks like my bad. Not actually sure how this happens. Launchpad is still a somewhat mystical beast to me :) I think it needs to be reverted back to whatever is in trunk.

I can go ahead and add the comment to the setter this evening as well. Thanks for the review.

[j.c.sackett (jcsackett)]

Just add the comment for the setter; having investigated the sourcedeps.cache thing it looks like it's just an artifact of when update-sourcecode updates it, and nothing to do with your branch. I saw the same phenomenon in another branch I reviewed a bit after yours.

I'll mark this approved, with the understanding that you're going to add that comment. Thanks, Tim.

review: Approve

[j.c.sackett (jcsackett)]

As a note, Tim, you'll need to find someone else to land this for you; normally as the reviewer I would, but I'll be unavailable shortly, so I'll likely not be around once you've added that comment.

[Timothy R. Chavez (timrchavez)]

I've added the note. I'll try to get Cody to land this if you are unavailable. Thanks (and my apologies for the slow turnaround)

[Timothy R. Chavez (timrchavez)]

Using Python properties to get/set the private attribute broke Storm expressions that were using Archive.private. To fix this, I changed these Storm expressions to use the underlying _private attribute.

[j.c.sackett (jcsackett)]

Tim--

This looks good. Thanks.

review: Approve

[Timothy R. Chavez (timrchavez)]

I believe I've identified the problem that was causing those test case failures. When going through and updating Storm expressions, I accidentally updated an SQL statement as well. Once fix, all tests that were failing ran successfully :)

[j.c.sackett (jcsackett)]

Yup, that seems to be the problem.

=== modified file 'lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py	2010-12-02 16:13:51 +0000
+++ lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py	2011-06-21 17:46:08 +0000
@@ -271,7 +271,7 @@
                 Archive.date_created >= last_success.date_completed]
 
         return store.find(
-            Archive, Archive.private == True, *extra_expr)
+            Archive, Archive._private == True, *extra_expr)
 
     def main(self):
         """Script entry point."""

=== modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
--- lib/lp/buildmaster/model/buildfarmjob.py	2011-05-27 21:12:25 +0000
+++ lib/lp/buildmaster/model/buildfarmjob.py	2011-06-21 17:46:08 +0000
@@ -417,13 +417,13 @@
                 Select(
                     Archive.id,
                     tables=(Archive,),
-                    where=(Archive.private == False)
+                    where=(Archive._private == False)
                     ),
                 Select(
                     Archive.id,
                     tables=(Archive,),
                     where=And(
-                        Archive.private == True,
+                        Archive._private == True,
                         Archive.ownerID.is_in(
                             Select(
                                 TeamParticipation.teamID,
@@ -445,7 +445,7 @@
                         Select(
                             Archive.id,
                             tables=(Archive,),
-                            where=(Archive.private == False)
+                            where=(Archive._private == False)
                             )
                         )
                     )

=== modified file 'lib/lp/registry/model/distributionsourcepackage.py'
--- lib/lp/registry/model/distributionsourcepackage.py	2011-06-07 03:30:25 +0000
+++ lib/lp/registry/model/distributionsourcepackage.py	2011-06-21 17:46:08 +0000
@@ -349,7 +349,7 @@
             Archive,
             Archive.distribution == self.distribution,
             Archive._enabled == True,
-            Archive.private == False,
+            Archive._private == False,
             SourcePackagePublishingHistory.archive == Archive.id,
             (SourcePackagePublishingHistory.status ==
                 PackagePublishingStatus.PUBLISHED),

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2011-06-11 00:49:33 +0000
+++ lib/lp/soyuz/browser/archive.py	2011-06-21 17:46:08 +0000
@@ -59,7 +59,6 @@
 
 from canonical.launchpad import _
 from canonical.launchpad.browser.librarian import FileNavigationMixin
-from canonical.launchpad.components.tokens import create_token
 from canonical.launchpad.helpers import english_list
 from canonical.launchpad.webapp import (
     canonical_url,
@@ -2059,8 +2058,8 @@
         archive as private, generate a secret for them.
         """
         if data['private'] and data['buildd_secret'] is None:
-            # buildd secrets are only used by builders, autogenerate one.
-            self.context.buildd_secret = create_token(16)
+            # The buildd secret is auto-generated and set when 'private'
+            # is set to True
             del(data['buildd_secret'])
         super(ArchiveAdminView, self).updateContextFromData(data)
 

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2011-06-16 20:12:00 +0000
+++ lib/lp/soyuz/model/archive.py	2011-06-21 17:46:08 +0000
@@ -59,6 +59,7 @@
     )
 from canonical.launchpad.components.tokens import (
     create_unique_token_for_table,
+    create_token,
     )
 from canonical.launchpad.database.librarian import (
     LibraryFileAlias,
@@ -271,7 +272,7 @@
 
     publish = BoolCol(dbName='publish', notNull=True, default=True)
 
-    private = BoolCol(dbName='private', notNull=True, default=False,
+    _private = BoolCol(dbName='private', notNull=True, default=False,
                       storm_validator=_validate_archive_privacy)
 
     require_virtualized = BoolCol(
@@ -336,6 +337,22 @@
         else:
             alsoProvides(self, IDistributionArchive)
 
+    # Note: You may safely ignore lint when it complains about this
+    # declaration.  As of Python 2.6, this is a perfectly valid way
+    # of adding a setter
+    @property
+    def private(self):
+        return self._private
+
+    @private.setter
+    def private(self, private):
+        self._private = private
+        if private:
+            if not self.buildd_secret:
+                self.buildd_secret = create_token(20)
+        else:
+            self.buildd_secret = None
+
     @property
     def title(self):
         """See `IArchive`."""
@@ -2205,7 +2222,7 @@
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
         return store.find(
             Archive,
-            Archive.private == True,
+            Archive._private == True,
             Archive.purpose == ArchivePurpose.PPA)
 
     def getCommercialPPAs(self):
@@ -2233,7 +2250,7 @@
         if name is not None:
             extra_exprs.append(Archive.name == name)
 
-        public_archive = And(Archive.private == False,
+        public_archive = And(Archive._private == False,
                              Archive._enabled == True)
 
         if user is not None:

=== modified file 'lib/lp/soyuz/tests/test_archive_privacy.py'
--- lib/lp/soyuz/tests/test_archive_privacy.py	2010-10-04 19:50:45 +0000
+++ lib/lp/soyuz/tests/test_archive_privacy.py	2011-06-21 17:46:08 +0000
@@ -29,7 +29,6 @@
         super(TestArchivePrivacy, self).setUp()
         self.private_ppa = self.factory.makeArchive(description='Foo')
         login('[e-mail]')
-        self.private_ppa.buildd_secret = 'blah'
         self.private_ppa.private = True
         self.joe = self.factory.makePerson(name='joe')
         self.fred = self.factory.makePerson(name='fred')
@@ -59,18 +58,15 @@
         super(TestArchivePrivacySwitching, self).setUp()
         self.public_ppa = self.factory.makeArchive()
         self.private_ppa = self.factory.makeArchive()
-        self.private_ppa.buildd_secret = 'blah'
         self.private_ppa.private = True
 
     def make_ppa_private(self, ppa):
         """Helper method to privatise a ppa."""
         ppa.private = True
-        ppa.buildd_secret = "secret"
 
     def make_ppa_public(self, ppa):
         """Helper method to make a PPA public (and use for assertRaises)."""
         ppa.private = False
-        ppa.buildd_secret = ''
 
     def test_switch_privacy_no_pubs_succeeds(self):
         # Changing the privacy is fine if there are no publishing
@@ -94,3 +90,11 @@
 
         self.assertRaises(
             CannotSwitchPrivacy, self.make_ppa_public, self.private_ppa)
+
+    def test_buildd_secret_was_generated(self):
+        self.make_ppa_private(self.public_ppa)
+        self.assertNotEqual(self.public_ppa.buildd_secret, None)
+
+    def test_discard_buildd_was_discarded(self):
+        self.make_ppa_public(self.private_ppa)
+        self.assertEqual(self.private_ppa.buildd_secret, None)