Merge lp:~stevenk/launchpad/bugs-use-information_type-redux into lp:launchpad

[Steve Kowalik]

Resurrect use of IBug.information_type, setting it properly at creation time.

Resurrect the rollback of using IBug.information_type that happened in r15006. The original MPs description is as follows:

Rename IBug.{private,security_related} to IBug._{private,security_related} as the next step in switching to information_type. This branch also adds IBug.{private,security_related} back as a property that depends on information_type if it is set.

Fixed some tests that assumed that IBug.{private,security_related} was something they could set directly.

I cleaned up a small amount of lint.

Further to those changes, I have created a new method, convert_to_information_type, which returns the relevant InformationType given a private and security_related parameter, which allows me to remove IBug._setInformationType() and to set the information_type of the bug right at the initial insert. I have added a test which checks that case.

[William Grant (wgrant)]

251 + def test_information_type_does_not_leak(self):
252 + product = self.factory.makeProduct()

Not entirely descriptive :)

Also:

10:52:57 < wgrant> StevenK: I think I mentioned this in the last review, but from the diff it looks like bug.private writes were previously permitted by the security policy, but I don't see any ZCML changes in this brnach to prevent that.
10:58:14 < StevenK> wgrant: wgrant: private/security_related aren't in the ZCML, so I'm not sure how to tell it "forbid anyone setting these"
11:00:04 < wgrant> StevenK: Indeed. I wonder if the removeSecurityProxy is really required, then

review: Approve (code)

=== modified file 'lib/lp/bugs/adapters/bug.py'
--- lib/lp/bugs/adapters/bug.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/adapters/bug.py	2012-03-26 00:18:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Resources having to do with Launchpad bugs."""
@@ -7,11 +7,14 @@
 __all__ = [
     'bugcomment_to_entry',
     'bugtask_to_privacy',
+    'convert_to_information_type',
     ]
 
 from lazr.restful.interfaces import IEntry
 from zope.component import getMultiAdapter
 
+from lp.registry.enums import InformationType
+
 
 def bugcomment_to_entry(comment, version):
     """Will adapt to the bugcomment to the real IMessage.
@@ -22,9 +25,21 @@
     return getMultiAdapter(
         (comment.bugtask.bug.messages[comment.index], version), IEntry)
 
+
 def bugtask_to_privacy(bugtask):
     """Adapt the bugtask to the underlying bug (which implements IPrivacy).
 
     Needed because IBugTask does not implement IPrivacy.
     """
     return bugtask.bug
+
+
+def convert_to_information_type(private, security_related):
+    if private and security_related:
+        return InformationType.EMBARGOEDSECURITY
+    elif security_related:
+        return InformationType.UNEMBARGOEDSECURITY
+    elif private:
+        return InformationType.USERDATA
+    else:
+        return InformationType.PUBLIC

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2012-03-24 04:42:32 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2012-03-26 00:18:19 +0000
@@ -40,10 +40,6 @@
 from lp.testing.views import create_initialized_view
 
 
-ON = 'on'
-OFF = None
-
-
 class BugsubscriptionPrivacyTests(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
@@ -52,7 +48,7 @@
         super(BugsubscriptionPrivacyTests, self).setUp()
         self.user = self.factory.makePerson()
         self.bug = self.factory.makeBug(owner=self.user)
-        removeSecurityProxy(self.bug).private = True
+        removeSecurityProxy(self.bug).setPrivate(True, self.user)
 
     def _assert_subscription_fails(self, team):
         with person_logged_in(self.user):

=== modified file 'lib/lp/bugs/mail/tests/test_handler.py'
--- lib/lp/bugs/mail/tests/test_handler.py	2012-03-24 04:42:32 +0000
+++ lib/lp/bugs/mail/tests/test_handler.py	2012-03-26 00:18:19 +0000
@@ -287,7 +287,7 @@
         notification = self.getLatestBugNotification()
         bug = notification.bug
         self.assertEqual('unsecure code', bug.title)
-        self.assertEqual(True, bug.security_related)
+        self.assertTrue(bug.security_related)
         self.assertEqual(['ajax'], bug.tags)
         self.assertEqual(1, len(bug.bugtasks))
         self.assertEqual(project, bug.bugtasks[0].target)
@@ -310,7 +310,7 @@
         notification = self.getLatestBugNotification()
         bug = notification.bug
         self.assertEqual('security issue', bug.title)
-        self.assertEqual(True, bug.security_related)
+        self.assertTrue(bug.security_related)
         self.assertEqual(1, len(bug.bugtasks))
         self.assertEqual(project, bug.bugtasks[0].target)
         recipients = set()

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-03-24 04:42:32 +0000
+++ lib/lp/bugs/model/bug.py	2012-03-26 00:18:19 +0000
@@ -89,6 +89,7 @@
     )
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.validators import LaunchpadValidationError
+from lp.bugs.adapters.bug import convert_to_information_type
 from lp.bugs.adapters.bugchange import (
     BranchLinkedToBug,
     BranchUnlinkedFromBug,
@@ -157,7 +158,11 @@
     )
 from lp.code.interfaces.branchcollection import IAllBranches
 from lp.hardwaredb.interfaces.hwdb import IHWSubmissionBugSet
-from lp.registry.enums import InformationType
+from lp.registry.enums import (
+    InformationType,
+    PRIVATE_INFORMATION_TYPES,
+    SECURITY_INFORMATION_TYPES,
+    )
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.registry.interfaces.person import (
@@ -347,12 +352,13 @@
         dbName='duplicateof', foreignKey='Bug', default=None)
     datecreated = UtcDateTimeCol(notNull=True, default=UTC_NOW)
     date_last_updated = UtcDateTimeCol(notNull=True, default=UTC_NOW)
-    private = BoolCol(notNull=True, default=False)
+    _private = BoolCol(dbName='private', notNull=True, default=False)
     date_made_private = UtcDateTimeCol(notNull=False, default=None)
     who_made_private = ForeignKey(
         dbName='who_made_private', foreignKey='Person',
         storm_validator=validate_public_person, default=None)
-    security_related = BoolCol(notNull=True, default=False)
+    _security_related = BoolCol(
+        dbName='security_related', notNull=True, default=False)
     information_type = EnumCol(
         enum=InformationType, default=InformationType.PUBLIC)
 
@@ -389,6 +395,20 @@
     heat_last_updated = UtcDateTimeCol(default=None)
     latest_patch_uploaded = UtcDateTimeCol(default=None)
 
+    @property
+    def private(self):
+        if self.information_type:
+            return self.information_type in PRIVATE_INFORMATION_TYPES
+        else:
+            return self._private
+
+    @property
+    def security_related(self):
+        if self.information_type:
+            return self.information_type in SECURITY_INFORMATION_TYPES
+        else:
+            return self._security_related
+
     @cachedproperty
     def _subscriber_cache(self):
         """Caches known subscribers."""
@@ -1697,16 +1717,6 @@
 
         return bugtask
 
-    def _setInformationType(self):
-        if self.private and self.security_related:
-            self.information_type = InformationType.EMBARGOEDSECURITY
-        elif self.private:
-            self.information_type = InformationType.USERDATA
-        elif self.security_related:
-            self.information_type = InformationType.UNEMBARGOEDSECURITY
-        else:
-            self.information_type = InformationType.PUBLIC
-
     def setPrivacyAndSecurityRelated(self, private, security_related, who):
         """ See `IBug`."""
         private_changed = False
@@ -1734,7 +1744,7 @@
                     raise BugCannotBePrivate(
                         "Multi-pillar bugs cannot be private.")
             private_changed = True
-            self.private = private
+            self._private = private
 
             if private:
                 self.who_made_private = who
@@ -1750,14 +1760,15 @@
 
         if self.security_related != security_related:
             security_related_changed = True
-            self.security_related = security_related
+            self._security_related = security_related
 
         if private_changed or security_related_changed:
             # Correct the heat for the bug immediately, so that we don't have
             # to wait for the next calculation job for the adjusted heat.
             self.updateHeat()
 
-        self._setInformationType()
+        self.information_type = convert_to_information_type(
+            self._private, self._security_related)
 
         if private_changed or security_related_changed:
             changed_fields = []
@@ -2829,9 +2840,6 @@
                 bug.subscribe(params.product.bug_supervisor, params.owner)
             else:
                 bug.subscribe(params.product.owner, params.owner)
-        else:
-            # nothing to do
-            pass
 
         # Create the task on a product if one was passed.
         if params.product:
@@ -2858,8 +2866,6 @@
         if notify_event:
             notify(event)
 
-        bug._setInformationType()
-
         # Calculate the bug's initial heat.
         bug.updateHeat()
 
@@ -2906,11 +2912,14 @@
                 date_made_private=params.datecreated,
                 who_made_private=params.owner)
 
+        information_type = convert_to_information_type(
+            params.private, params.security_related)
         bug = Bug(
             title=params.title, description=params.description,
-            private=params.private, owner=params.owner,
+            _private=params.private, owner=params.owner,
             datecreated=params.datecreated,
-            security_related=params.security_related,
+            _security_related=params.security_related,
+            information_type=information_type,
             **extra_params)
 
         if params.subscribe_owner:

=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py	2012-03-14 12:39:42 +0000
+++ lib/lp/bugs/model/tests/test_bug.py	2012-03-26 00:18:19 +0000
@@ -9,6 +9,7 @@
     )
 
 from pytz import UTC
+from storm.expr import Join
 from storm.store import Store
 from testtools.testcase import ExpectedException
 from zope.component import getUtility
@@ -970,6 +971,24 @@
             bug.setPrivate(True, bug.owner)
         self.assertEqual(InformationType.USERDATA, bug.information_type)
 
+    def test_information_type_does_not_leak(self):
+        # Make sure that bug notifications for private bugs do not leak to
+        # people with a subscription on the product.
+        product = self.factory.makeProduct()
+        with person_logged_in(product.owner):
+            product.addSubscription(product.owner, product.owner)
+        reporter = self.factory.makePerson()
+        bug = self.factory.makeBug(
+            private=True, product=product, owner=reporter)
+        recipients = Store.of(bug).using(
+            BugNotificationRecipient,
+            Join(BugNotification, BugNotification.bugID == bug.id)).find(
+            BugNotificationRecipient,
+            BugNotificationRecipient.bug_notificationID ==
+                BugNotification.id)
+        self.assertEqual(
+            [reporter], [recipient.person for recipient in recipients])
+
 
 class TestBugPrivateAndSecurityRelatedUpdatesPrivateProject(
         TestBugPrivateAndSecurityRelatedUpdatesMixin, TestCaseWithFactory):

=== modified file 'lib/lp/bugs/tests/test_bug_mirror_access_triggers.py'
--- lib/lp/bugs/tests/test_bug_mirror_access_triggers.py	2012-03-24 04:42:32 +0000
+++ lib/lp/bugs/tests/test_bug_mirror_access_triggers.py	2012-03-26 00:18:19 +0000
@@ -136,7 +136,7 @@
         bug = self.makeBugAndPolicies(private=True)
         self.assertIsNot(
             None, getUtility(IAccessArtifactSource).find([bug]).one())
-        bug.private = False
+        bug.setPrivate(False, bug.owner)
         self.assertIs(
             None, getUtility(IAccessArtifactSource).find([bug]).one())
 
@@ -144,7 +144,7 @@
         bug = self.makeBugAndPolicies(private=False)
         self.assertIs(
             None, getUtility(IAccessArtifactSource).find([bug]).one())
-        bug.private = True
+        bug.setPrivate(True, bug.owner)
         self.assertIsNot(
             None, getUtility(IAccessArtifactSource).find([bug]).one())
         self.assertEqual((1, 1), self.assertMirrored(bug))
@@ -158,7 +158,7 @@
         self.assertContentEqual(
             [InformationType.USERDATA],
             self.getPolicyTypesForArtifact(artifact))
-        bug.security_related = True
+        bug.setSecurityRelated(True, bug.owner)
         self.assertEqual((1, 1), self.assertMirrored(bug))
         self.assertContentEqual(
             [InformationType.EMBARGOEDSECURITY],

=== modified file 'lib/lp/registry/enums.py'
--- lib/lp/registry/enums.py	2012-03-24 04:42:32 +0000
+++ lib/lp/registry/enums.py	2012-03-26 00:18:19 +0000
@@ -9,7 +9,9 @@
     'DistroSeriesDifferenceType',
     'InformationType',
     'PersonTransferJobType',
+    'PRIVATE_INFORMATION_TYPES',
     'ProductJobType',
+    'SECURITY_INFORMATION_TYPES',
     'SharingPermission',
     ]
 
@@ -61,6 +63,15 @@
         """)
 
 
+PRIVATE_INFORMATION_TYPES = (
+    InformationType.EMBARGOEDSECURITY, InformationType.USERDATA,
+    InformationType.PROPRIETARY)
+
+
+SECURITY_INFORMATION_TYPES = (
+    InformationType.UNEMBARGOEDSECURITY, InformationType.EMBARGOEDSECURITY)
+
+
 class SharingPermission(DBEnumeratedType):
     """Sharing permission.
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-03-24 04:42:32 +0000
+++ lib/lp/registry/model/person.py	2012-03-26 00:18:19 +0000
@@ -1744,14 +1744,14 @@
                     Bug,
                     Join(BugSubscription, BugSubscription.bug_id == Bug.id)),
                 where=And(
-                    Bug.private == True,
+                    Bug._private == True,
                     BugSubscription.person_id == self.id)),
             Select(
                 Bug.id,
                 tables=(
                     Bug,
                     Join(BugTask, BugTask.bugID == Bug.id)),
-                where=And(Bug.private == True, BugTask.assignee == self.id)),
+                where=And(Bug._private == True, BugTask.assignee == self.id)),
             limit=1))
         if private_bugs_involved.rowcount:
             raise TeamSubscriptionPolicyError(

=== modified file 'lib/lp/services/feeds/stories/xx-security.txt'
--- lib/lp/services/feeds/stories/xx-security.txt	2012-03-24 04:42:32 +0000
+++ lib/lp/services/feeds/stories/xx-security.txt	2012-03-26 00:18:19 +0000
@@ -1,16 +1,16 @@
-== Feeds do not display private bugs ==
+Feeds do not display private bugs
+=================================
 
 Feeds never contain private bugs, as we are serving feeds over HTTP.
 First, set all the bugs to private.
 
     >>> from zope.security.interfaces import Unauthorized
     >>> from BeautifulSoup import BeautifulStoneSoup as BSS
+    >>> from lp.services.database.lpstorm import IStore
+    >>> import transaction
     >>> from lp.bugs.model.bug import Bug
-    >>> bugs = Bug.select()
-    >>> for bug in bugs:
-    ...     bug.private = True
-    >>> from lp.services.database.sqlbase import flush_database_updates
-    >>> flush_database_updates()
+    >>> IStore(Bug).find(Bug).set(_private=True)
+    >>> transaction.commit()
 
 There should be zero entries in these feeds, since all the bugs are private.
 
@@ -26,7 +26,8 @@
     >>> BSS(browser.contents)('entry')
     []
 
-    >>> browser.open('http://feeds.launchpad.dev/~simple-team/latest-bugs.atom')
+    >>> browser.open(
+    ...     'http://feeds.launchpad.dev/~simple-team/latest-bugs.atom')
     >>> BSS(browser.contents)('entry')
     []
 
@@ -66,7 +67,8 @@
     >>> print extract_text(BSS(browser.contents)('tr')[0])
     Bugs for Foo Bar
 
-    >>> browser.open('http://feeds.launchpad.dev/~simple-team/latest-bugs.html')
+    >>> browser.open(
+    ...     'http://feeds.launchpad.dev/~simple-team/latest-bugs.html')
     >>> len(BSS(browser.contents)('tr'))
     1