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

[Timothy R. Chavez]

[r=jcsackett][bug=724740] Fixed LP #724740 (making a PPA private via the API is currently not possible) by automatically generating and setting the buildd secret when the archive is made private.

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.

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

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.

[lp:~timrchavez/launchpad/set_ppa_private_from_api_724740-2]

13152. By Timothy R. Chavez on 2011-06-10

Add a note indicating that the complaint lint will give regarding the private setter declaration may be ignord because it is valid as of Python 2.6

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

Unmerged revisions

13152. By Timothy R. Chavez on 2011-06-10

Add a note indicating that the complaint lint will give regarding the private setter declaration may be ignord because it is valid as of Python 2.6

13151. By Timothy R. Chavez on 2011-06-09

Merge trunk

13150. By root <email address hidden> on 2011-06-09

Merged trunk

13149. By Timothy R. Chavez on 2011-06-07

Merged with trunk

13148. By Timothy R. Chavez on 2011-06-03

Remove a now unnecessary import and word wrap a comment so it does not violate the 78 column constraint

13147. By Timothy R. Chavez on 2011-06-03

Merge trunk

13146. By Timothy R. Chavez on 2011-06-03

This commit changes three things:

 1. Make sure we discard the buildd_secret if we make the PPA public again

 2. Remove explicit buildd_secret assignments in testing, this should now
    be done implicitly if private is set True and buildd_secret is None

 3. Add test cases to validate buildd_secret is generated and set if
    private is True and buildd_secret is None and that buildd_secret
    is discarded if private is set back to False

13145. By Timothy R. Chavez on 2011-06-01

Generate and set the buildd_secret when a PPA is made private for the first time (LP #724740)

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2011-06-09 08:07:52 +0000
+++ lib/lp/soyuz/browser/archive.py	2011-06-10 00:34:30 +0000
@@ -60,7 +60,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,
@@ -2094,8 +2093,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-07 00:16:31 +0000
+++ lib/lp/soyuz/model/archive.py	2011-06-10 00:34:30 +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,
@@ -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,22 @@
             alsoProvides(self, IDistributionArchive)
 
     @property
+    def private(self):
+        return self._private
+
+    # 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
+    @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`."""
         return self.displayname

=== 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-10 00:34:30 +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)

=== modified file 'utilities/sourcedeps.cache'
--- utilities/sourcedeps.cache	2011-06-01 18:58:15 +0000
+++ utilities/sourcedeps.cache	2011-06-10 00:34:30 +0000
@@ -1,4 +1,8 @@
 {
+    "bzr-builder": [
+        68, 
+        "[e-mail]-20101123183213-777lz46xgagn1deg"
+    ], 
     "testresources": [
         16, 
         "[e-mail]-20050911111209-ee5da49011cf936a"
@@ -27,18 +31,14 @@
         24, 
         "[e-mail]-20100601182722-wo7h2fh0fvyw3aaq"
     ], 
+    "lpreview": [
+        23, 
+        "[e-mail]-20090720061538-euyh68ifavhy0pi8"
+    ], 
     "bzr-git": [
         259, 
         "[e-mail]-20110601140035-gl5merbechngjw5s"
     ], 
-    "loggerhead": [
-        445, 
-        "[e-mail]-20110325141442-536j4be3x0c464zy"
-    ], 
-    "bzr-builder": [
-        68, 
-        "[e-mail]-20101123183213-777lz46xgagn1deg"
-    ], 
     "bzr-loom": [
         49, 
         "[e-mail]-20110601122412-54vo3k8yae9i2zve"
@@ -47,9 +47,9 @@
         4, 
         "sinzui-20090526164636-1swugzupwvjgomo4"
     ], 
-    "lpreview": [
-        23, 
-        "[e-mail]-20090720061538-euyh68ifavhy0pi8"
+    "loggerhead": [
+        445, 
+        "[e-mail]-20110325141442-536j4be3x0c464zy"
     ], 
     "difftacular": [
         6,