Merge lp:~jml/launchpad/drop-special-commercial-permissions into lp:launchpad

[Jonathan Lange]

No more special permissions for commercial archives.

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.

You can think of the deleted tests as summarizing what software-center-agent is about to lose.

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.

Another 25 deletions of credit, bring us up to 113 + 25 = 138.

Thanks,
jml

[Richard Harding (rharding)]

Thanks Jonathan, as a reviewer I'd tend to want to keep the check that the user doesn't have access/permissions to the private archive just so that I can be sure 100% that this doesn't enable something that was previously forbidden, but that's just because I'm not all that sure of the full set of possible ramifications.

I'm going to

review: Approve (code)

[Jonathan Lange (jml)]

I think that got truncated, "I'm going to".

We already have tests that arbitrary users cannot access private archives. I don't think we ought to have tests for non-access for every special user that we can think of – otherwise where does it end?

Thanks for the review!

jml

[James Westby (james-w)]

Hi,

This looks good to me from a code point of view.

Agree about the need for integration testing.

I think that we may also want to audit the current PPAs to make sure that
s-c-a won't lose permissions that it needs to complete sales of something
available in software-center. I'm not exactly sure how we would determine
that though.

Thanks,

James

review: Approve

[William Grant (wgrant)]

https://pastebin.canonical.com/65324/ is the number of commercial PPAs by owner.

It looks like you can probably now remove in_software_center_agent from lib/lp/registry/interfaces/role.py. The only remaining things that use the celebrity are Person.getArchiveSubscriptionURL(s), and they use it directly.

review: Approve (code)

[Jonathan Lange (jml)]

Thanks everyone. I've triggered an 'ec2 test' run for this and will work with achuni & the MyApps guys to make sure we can get proper QA done.

[Anthony Lenton (elachuni)]

Thanks jml,

We've verified sca is still able to go around its business with this change; the only process change is that we need to ensure the sca user is added to the team that owns the ppa now, so that it is able to create subscriptions. This is a reasonable requirement.

review: Approve

[Jonathan Lange (jml)]

OK, talking to Anthony, it would be better if we land this so that we can QA against staging.launchpad.net rather than qastaging.launchpad.net. I don't quite know how to make this happen, but I'd appreciate it if whoever lands this patch could do whatever's necessary.

I've added a note to the bug asking to please not mark it as qa-ok without first speaking with Anthony.

Thanks,
jml

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-04-27 00:48:11 +0000
+++ lib/lp/security.py	2012-05-04 10:47:25 +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/stories/webservice/xx-archive-commercial.txt'
--- lib/lp/soyuz/stories/webservice/xx-archive-commercial.txt	2012-05-01 12:09:24 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive-commercial.txt	2012-05-04 10:47:25 +0000
@@ -16,14 +16,16 @@
 
 == Software Center Agent ==
 
-Create the P3A marked commercial, and fetch the celebrity.
+Create the P3A where software_center_agent is an owner.
 
     >>> from zope.component import getUtility
     >>> from lp.app.interfaces.launchpad import ILaunchpadCelebrities
     >>> login('[e-mail]')
     >>> celebrity = getUtility(ILaunchpadCelebrities).software_center_agent
-    >>> archive = factory.makeArchive(
-    ...     name='commercial', private=True, commercial=True)
+    >>> owner = factory.makePerson()
+    >>> ppa_owner = factory.makeTeam(members=[celebrity, owner])
+    >>> archive = factory.makeArchive(name='commercial', private=True,
+    ...     owner=ppa_owner)
     >>> url = "/~%s/+archive/commercial" % archive.owner.name
     >>> person = factory.makePerson(name='joe')
     >>> logout()

=== modified file 'lib/lp/soyuz/tests/test_archive_agent.py'
--- lib/lp/soyuz/tests/test_archive_agent.py	2012-05-01 12:25:22 +0000
+++ lib/lp/soyuz/tests/test_archive_agent.py	2012-05-04 10:47:25 +0000
@@ -5,49 +5,30 @@
 
 from zope.component import getUtility
 
-from lp.services.webapp.authorization import check_permission
-from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriberSet
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 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)