launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxxxxxx>]

Jonathan Lange has proposed merging lp:~jml/launchpad/drop-special-commercial-permissions into lp:launchpad with lp:~jml/launchpad/narrow-commercial-celebrity as a prerequisite.

Requested reviews:
  William Grant (wgrant): follow up from irc
  Anthony Lenton (elachuni)
  James Westby (james-w)
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #992691 in Launchpad itself: "Special permissions for 'Archive.commercial' are not needed"
  https://bugs.launchpad.net/launchpad/+bug/992691

For more details, see:
https://code.launchpad.net/~jml/launchpad/drop-special-commercial-permissions/+merge/104270

We've discovered that we don't actually need as much of the permission that being ILaunchpadCelebrity.software_center_agent gives us.

Specifically, we don't actually care if 'commercial' is set on PPAs or not, since software-center-agent is already an owner all of the PPAs that we create.  As far as I can tell, this just leaves software-center-agent with the mere power to get archive subscription URLs. As such, I've deleted some of the tests.

Code-wise this is pretty easy, but there's a relatively high integration risk. Thus, we're going to do integration testing with a demo copy of Launchpad run from EC2, so please don't land this until we give the all clear.

Also, I'd welcome reviews from a broader range of reviewers.

Thanks,
jml
-- 
https://code.launchpad.net/~jml/launchpad/drop-special-commercial-permissions/+merge/104270
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jml/launchpad/drop-special-commercial-permissions into lp:launchpad.

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-04-27 00:48:11 +0000
+++ lib/lp/security.py	2012-05-01 16:21:22 +0000
@@ -2379,10 +2379,6 @@
             if archive_subs:
                 return True
 
-        # The software center agent can view commercial archives
-        if self.obj.commercial:
-            return user.in_software_center_agent
-
         return False
 
     def checkUnauthenticated(self):
@@ -2437,10 +2433,6 @@
             user.in_ubuntu_security):
             return True
 
-        # The software center agent can change commercial archives
-        if self.obj.commercial:
-            return user.in_software_center_agent
-
         return False
 
 

=== modified file 'lib/lp/soyuz/tests/test_archive_agent.py'
--- lib/lp/soyuz/tests/test_archive_agent.py	2012-05-01 16:21:21 +0000
+++ lib/lp/soyuz/tests/test_archive_agent.py	2012-05-01 16:21:22 +0000
@@ -5,49 +5,32 @@
 
 from zope.component import getUtility
 
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.services.webapp.authorization import check_permission
 from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriberSet
 from lp.testing import (
     celebrity_logged_in,
+    person_logged_in,
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
 
 
-class TestArchivePrivacy(TestCaseWithFactory):
+class TestSoftwareCenterAgent(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
-    def test_check_permission(self):
-        """The software center agent has the relevant permissions for a
-        commercial archive, but not a private one.
-        """
-        ppa = self.factory.makeArchive(private=True, commercial=True)
-        with celebrity_logged_in('software_center_agent'):
-            self.assertEqual(check_permission('launchpad.View', ppa), True)
-            self.assertEqual(check_permission('launchpad.Append', ppa), True)
-
-    def test_check_permission_private(self):
-        ppa = self.factory.makeArchive(private=True, commercial=False)
-        with celebrity_logged_in('software_center_agent'):
-            self.assertEqual(check_permission('launchpad.View', ppa), False)
-            self.assertEqual(check_permission('launchpad.Append', ppa), False)
-
-    def test_add_subscription(self):
-        person = self.factory.makePerson()
-        ppa = self.factory.makeArchive(private=True, commercial=True)
-        with celebrity_logged_in('software_center_agent') as agent:
-            ppa.newSubscription(person, agent)
-            subscription = getUtility(IArchiveSubscriberSet).getBySubscriber(
-                person, archive=ppa).one()
-            self.assertEqual(subscription.registrant, agent)
-            self.assertEqual(subscription.subscriber, person)
-
     def test_getArchiveSubscriptionURL(self):
-        ppa = self.factory.makeArchive(private=True, commercial=True)
+        # The software center agent can get subscription URLs for any
+        # archive that it's an owner of.
+        owner = self.factory.makePerson()
+        agent = getUtility(ILaunchpadCelebrities).software_center_agent
+        ppa_owner = self.factory.makeTeam(members=[owner, agent])
+        ppa = self.factory.makeArchive(owner=ppa_owner, private=True)
         person = self.factory.makePerson()
         with celebrity_logged_in('software_center_agent') as agent:
             sources = person.getArchiveSubscriptionURL(agent, ppa)
+        with person_logged_in(ppa.owner):
             authtoken = ppa.getAuthToken(person).token
             url = ppa.archive_url.split('http://')[1]
         new_url = "http://%s:%s@%s"; % (person.name, authtoken, url)