Fix issue2550954: History display breaks

.. on removed properties: Now changes to removed properties, and
link/unlink events from non-existing properties or classes no longer
trigger a traceback.  Concerning the visibility: We have a new
config-item obsolete_history_roles in the main section that defines
which roles may see removed properties. By default only role Admin is
allowed to see these.

Author: schlatterbeck

CHANGES.txt

@@ -480,6 +480,13 @@ Fixed:
   Bcc and cc users passed to nosymessage are not properly recorded.
   This results in duplicate emails. (patch by Trent Gamblin (trentgg)
   applied by John Rouillard).
+- issue2550954: History display breaks on removed properties
+  Now changes to removed properties, and link/unlink events from
+  non-existing properties or classes no longer trigger a traceback.
+  Concerning the visibility: We have a new config-item
+  obsolete_history_roles in the main section that defines which roles
+  may see removed properties. By default only role Admin is allowed to
+  see these.
 
 2016-01-11: 1.5.1
 

roundup/configuration.py

@@ -605,6 +605,14 @@ def str2value(self, value):
             " with Email Gateway.\n"
             "This is a comma-separated string of role names"
             " (e.g. 'Admin,User')."),
+        (Option, "obsolete_history_roles", "Admin",
+	    "On schema changes, properties or classes in the history may\n"
+	    "become obsolete.  Since normal access permissions do not apply\n"
+	    "(we don't know if a user should see such a property or class)\n"
+	    "a list of roles is specified here that are allowed to see\n"
+	    "these obsolete properties in the history. By default only the\n"
+	    "admin role may see these history entries, you can make them\n"
+	    "visible to all users by adding, e.g., the 'User' role here."),
         (Option, "error_messages_to", "user",
             # XXX This description needs better wording,
             #   with explicit allowed values list.

roundup/hyperdb.py

@@ -1015,6 +1015,10 @@ def history(self, nodeid, enforceperm=True, skipquiet=True):
         If the user requesting the history does not have View access
         to the property, the journal entry will not be shown. This can
         be disabled by setting enforceperm=False.
+
+        Note that there is a check for obsolete properties and classes
+        resulting from history changes. These are also only checked if
+        enforceperm is True.
         """
         if not self.do_journal:
             raise ValueError('Journalling is disabled for this class')
@@ -1024,17 +1028,27 @@ def history(self, nodeid, enforceperm=True, skipquiet=True):
 
         uid=self.db.getuid() # id of the person requesting the history
 
+        # Roles of the user and the configured obsolete_history_roles
+        hr = set(iter_roles(self.db.config.OBSOLETE_HISTORY_ROLES))
+        ur = set(self.db.user.get_roles(uid))
+        allow_obsolete = bool(hr & ur)
+
         for j in self.db.getjournal(self.classname, nodeid):
             # hide/remove journal entry if:
             #   property is quiet
             #   property is not (viewable or editable)
+            #   property is obsolete and not allow_obsolete
             id, evt_date, user, action, args = j
             if logger.isEnabledFor(logging.DEBUG):
                 j_repr = "%s"%(j,)
             else:
                 j_repr=''
             if args and type(args) == type({}):
                 for key in args.keys():
+                    if key not in self.properties :
+                        if enforceperm and not allow_obsolete:
+                            del j[4][key]
+                        continue
                     if skipquiet and self.properties[key].quiet:
                         logger.debug("skipping quiet property"
                                      " %s::%s in %s",
@@ -1048,7 +1062,8 @@ def history(self, nodeid, enforceperm=True, skipquiet=True):
                                 uid,
                                 self.classname,
                                 property=key )):
-                        logger.debug("skipping unaccessible property %s::%s seen by user%s in %s",
+                        logger.debug("skipping unaccessible property "
+                                     "%s::%s seen by user%s in %s",
                                 self.classname, key, uid, j_repr)
                         del j[4][key]
                         continue
@@ -1078,7 +1093,16 @@ def history(self, nodeid, enforceperm=True, skipquiet=True):
                     # j = id, evt_date, user, action, args
                     # 3|20170528045201.484|5|link|('issue', '5', 'blockedby')
                     linkcl, linkid, key = args
-                    cls = self.db.getclass(linkcl)
+                    cls = None
+                    try:
+                        cls = self.db.getclass(linkcl)
+                    except KeyError:
+                        pass
+                    # obsolete property or class
+                    if not cls or key not in cls.properties:
+                        if not enforceperm or allow_obsolete:
+                            journal.append(j)
+                        continue
                     # is the updated property quiet?
                     if skipquiet and cls.properties[key].quiet:
                         logger.debug("skipping quiet property: "

test/db_test_base.py

@@ -145,8 +145,8 @@ def tearDown(self):
         if os.path.exists(config.DATABASE):
             shutil.rmtree(config.DATABASE)
 
-    def open_database(self):
-        self.db = self.module.Database(config, 'admin')
+    def open_database(self, user='admin'):
+        self.db = self.module.Database(config, user)
 
 
 if os.environ.has_key('LOGGING_LEVEL'):
@@ -1275,6 +1275,42 @@ def testJournals(self):
         # see if the change was journalled
         self.assertNotEqual(jlen,  len(self.db.getjournal('issue', '1')))
 
+    def testJournalNonexistingProperty(self):
+        # Test for non-existing properties, link/unlink events to
+        # non-existing classes and link/unlink events to non-existing
+        # properties in a class: These all may be the result of a schema
+        # change and should not lead to a traceback.
+        self.db.user.create(username="mary")
+        id = self.db.issue.create(title="spam", status='1')
+        self.db.commit()
+        journal = self.db.getjournal('issue', id)
+        now     = date.Date('.')
+        sec     = date.Interval('0:00:01')
+        sec2    = date.Interval('0:00:02')
+        # Non-existing property changed
+        jp1 = dict(nonexisting = None)
+        journal.append ((id, now, '1', 'set', jp1))
+        # Link from user-class to non-existing property
+        jp2 = ('user', '1', 'xyzzy')
+        journal.append ((id, now+sec, '1', 'link', jp2))
+        # Link from non-existing class
+        jp3 = ('frobozz', '1', 'xyzzy')
+        journal.append ((id, now+sec2, '1', 'link', jp3))
+        self.db.setjournal('issue', id, journal)
+        self.db.commit()
+        result=self.db.issue.history(id)
+        result.sort()
+        self.assertEqual(len(result), 4)
+        self.assertEqual(result [1][4], jp1)
+        self.assertEqual(result [2][4], jp2)
+        self.assertEqual(result [3][4], jp3)
+        self.db.close()
+        # Verify that normal user doesn't see obsolete props/classes
+        self.open_database('mary')
+        setupSchema(self.db, 0, self.module)
+        result=self.db.issue.history(id)
+        self.assertEqual(len(result), 1)
+
     def testJournalPreCommit(self):
         id = self.db.user.create(username="mary")
         self.assertEqual(len(self.db.getjournal('user', id)), 1)