Merge ~cjwatson/launchpad:modern-ztk-prepare into launchpad:master

[Colin Watson]

Fix various problems upgrading to modern ZTK

I've been working on upgrading Launchpad to modern versions of its Zope Toolkit dependencies. This bundles a collection of small fixes discovered in the process, all of which are needed to make tests pass after the upgrade. This doesn't actually apply any package upgrades; that will come later.

I'd particularly like to get the change to lp.services.webapp.tests.test_session landed before upgrading, since that will allow us to verify that session IDs match before and after upgrading zope.session.

[Ioana Lasc (ilasc)]

looks good.

review: Approve

diff --git a/lib/lp/code/interfaces/gitref.py b/lib/lp/code/interfaces/gitref.py
index 16198eb..3c5f693 100644
--- a/lib/lp/code/interfaces/gitref.py
+++ b/lib/lp/code/interfaces/gitref.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Git reference ("ref") interfaces."""
@@ -419,7 +419,9 @@ class IGitRefEdit(Interface):
             # _schema_circular_imports.py.
             value_type=InlineObject(schema=Interface),
             description=_(dedent("""\
-                The new list of grants for this reference.  For example::
+                The new list of grants for this reference.
+
+                For example::
 
                     [
                         {
diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index 2fca251..386ad62 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -812,7 +812,9 @@ class IGitRepositoryEdit(IWebhookTarget):
             # _schema_circular_imports.py.
             value_type=InlineObject(schema=Interface),
             description=_(dedent("""\
-                The new list of rules for this repository.  For example::
+                The new list of rules for this repository.
+
+                For example::
 
                     [
                         {
diff --git a/lib/lp/registry/interfaces/poll.py b/lib/lp/registry/interfaces/poll.py
index 75a6af2..8f630d6 100644
--- a/lib/lp/registry/interfaces/poll.py
+++ b/lib/lp/registry/interfaces/poll.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = [
@@ -171,7 +171,9 @@ class IPoll(Interface):
             raise Invalid(
                 "A poll cannot close at the time (or before) it opens.")
         now = datetime.now(pytz.UTC)
-        twelve_hours_ahead = now + timedelta(hours=12)
+        # Allow a bit of slack to account for time between form creation and
+        # validation.
+        twelve_hours_ahead = now + timedelta(hours=12) - timedelta(seconds=60)
         start_date = poll.dateopens.astimezone(pytz.UTC)
         if start_date < twelve_hours_ahead:
             raise Invalid(
diff --git a/lib/lp/services/fields/__init__.py b/lib/lp/services/fields/__init__.py
index ee9e16f..6bbc0b3 100644
--- a/lib/lp/services/fields/__init__.py
+++ b/lib/lp/services/fields/__init__.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -65,7 +65,10 @@ from lazr.uri import (
     URI,
     )
 from zope.component import getUtility
-from zope.interface import implementer
+from zope.interface import (
+    implementer,
+    Interface,
+    )
 from zope.schema import (
     Bool,
     Bytes,
@@ -83,7 +86,6 @@ from zope.schema.interfaces import (
     IBytes,
     IDate,
     IDatetime,
-    Interface,
     IObject,
     IText,
     ITextLine,
diff --git a/lib/lp/services/job/runner.py b/lib/lp/services/job/runner.py
index 635e075..800d303 100644
--- a/lib/lp/services/job/runner.py
+++ b/lib/lp/services/job/runner.py
@@ -267,6 +267,17 @@ class BaseRunnableJob(BaseRunnableJobSource):
             response = self.runViaCelery(ignore_result)
             if not ignore_result:
                 BaseRunnableJob.celery_responses.append(response)
+            # transaction >= 1.6.0 removes data managers from the
+            # transaction before calling after-commit hooks; this means that
+            # the transaction begun in runViaCelery to look up details of
+            # the job doesn't get committed or rolled back, and so
+            # subsequent SQL statements executed by the caller confusingly
+            # see a database snapshot from before the Celery job was run,
+            # even if they use the block_on_job test helper.  Since
+            # runViaCelery never issues any data-modifying statements
+            # itself, the least confusing thing to do here is to just roll
+            # back its transaction.
+            transaction.abort()
 
     def celeryRunOnCommit(self):
         """Configure transaction so that commit runs this job via Celery."""
diff --git a/lib/lp/services/mail/mbox.py b/lib/lp/services/mail/mbox.py
index 0ae2444..1767523 100644
--- a/lib/lp/services/mail/mbox.py
+++ b/lib/lp/services/mail/mbox.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """An IMailer that stores messages in a specified mbox file."""
@@ -69,3 +69,10 @@ class MboxMailer:
             chained_mailer = getUtility(IMailer, self.mailer)
             chained_mailer.send(fromaddr, toaddrs, message)
         return message_id
+
+    def vote(self, fromaddr, toaddrs, message):
+        pass
+
+    def abort(self):
+        # We don't do any work until send() is called, so aborting is trivial.
+        pass
diff --git a/lib/lp/services/mail/stub.py b/lib/lp/services/mail/stub.py
index 98d40da..1492057 100644
--- a/lib/lp/services/mail/stub.py
+++ b/lib/lp/services/mail/stub.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """A stub IMailer for use in development and unittests."""
@@ -52,6 +52,13 @@ class StubMailer:
         sendmail = getUtility(IMailer, self.mailer)
         sendmail.send(self.from_addr, self.to_addrs, message)
 
+    def vote(self, fromaddr, toaddrs, message):
+        pass
+
+    def abort(self):
+        # We don't do any work until send() is called, so aborting is trivial.
+        pass
+
 
 test_emails = []
 
@@ -67,3 +74,10 @@ class TestMailer:
 
     def send(self, from_addr, to_addrs, message):
         test_emails.append((from_addr, to_addrs, message))
+
+    def vote(self, fromaddr, toaddrs, message):
+        pass
+
+    def abort(self):
+        # We don't do any work until send() is called, so aborting is trivial.
+        pass
diff --git a/lib/lp/services/scripts/__init__.py b/lib/lp/services/scripts/__init__.py
index 51456c2..d4a362d 100644
--- a/lib/lp/services/scripts/__init__.py
+++ b/lib/lp/services/scripts/__init__.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Library functions for use in all scripts.
@@ -20,10 +20,10 @@ import os
 import sys
 import threading
 
+import zope.component.hooks
 from zope.configuration.config import ConfigurationMachine
 from zope.security.management import setSecurityPolicy
 import zope.sendmail.delivery
-import zope.site.hooks
 
 from lp.services.config import config
 from lp.services.database.postgresql import ConnectionString
@@ -76,7 +76,7 @@ def execute_zcml_for_scripts(use_web_security=False):
     from zope.configuration import xmlconfig
 
     # Hook up custom component architecture calls
-    zope.site.hooks.setHooks()
+    zope.component.hooks.setHooks()
 
     # Load server-independent site config
     context = ConfigurationMachine()
diff --git a/lib/lp/services/webapp/tests/test_session.py b/lib/lp/services/webapp/tests/test_session.py
index 91c2bfa..b99ff4f 100644
--- a/lib/lp/services/webapp/tests/test_session.py
+++ b/lib/lp/services/webapp/tests/test_session.py
@@ -1,10 +1,13 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import datetime
+import hashlib
+import hmac
 
 from testtools import TestCase
 from testtools.matchers import Contains
+from zope.session.session import digestEncode
 
 from lp.services.webapp.login import (
     isFreshLogin,
@@ -67,6 +70,23 @@ class TestLaunchpadCookieClientIdManager(TestCase):
             dict(request.response.getHeaders())['Set-Cookie'],
             Contains('; httponly;'))
 
+    def test_stable_client_id(self):
+        # Changing the HMAC algorithm used for client IDs would invalidate
+        # existing sessions, so be careful that that doesn't happen
+        # accidentally across upgrades.
+        request = LaunchpadTestRequest()
+        idmanager = LaunchpadCookieClientIdManager()
+        idmanager._secret = 'secret'
+        data = b'random'
+        s = digestEncode(hashlib.sha1(data).digest())
+        mac = hmac.new(
+            s, idmanager.secret.encode(), digestmod=hashlib.sha1).digest()
+        sid = (s + digestEncode(mac)).decode()
+        idmanager.setRequestId(request, sid)
+        # getRequestId will only return the previously-set ID if it was
+        # generated using the correct secret with the correct algorithm.
+        self.assertEqual(sid, idmanager.getRequestId(request))
+
 
 class TestSessionRelatedFunctions(TestCaseWithFactory):
 
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 39d2725..f0310f3 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -64,7 +64,6 @@ from zope.component import (
     getUtility,
     )
 from zope.security.proxy import (
-    builtin_isinstance,
     Proxy,
     ProxyFactory,
     removeSecurityProxy,
@@ -4955,7 +4954,7 @@ def is_security_proxied_or_harmless(obj):
     """Check that the object is security wrapped or a harmless object."""
     if obj is None:
         return True
-    if builtin_isinstance(obj, Proxy):
+    if isinstance(obj, Proxy):
         return True
     if type(obj) in unwrapped_types:
         return True
diff --git a/lib/lp/testing/matchers.py b/lib/lp/testing/matchers.py
index ec9d22b..86266e6 100644
--- a/lib/lp/testing/matchers.py
+++ b/lib/lp/testing/matchers.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -38,10 +38,7 @@ from zope.interface.exceptions import (
     DoesNotImplement,
     )
 from zope.interface.verify import verifyObject
-from zope.security.proxy import (
-    builtin_isinstance,
-    Proxy,
-    )
+from zope.security.proxy import Proxy
 
 from lp.services.database.sqlbase import flush_database_caches
 from lp.services.webapp import canonical_url
@@ -249,7 +246,7 @@ class IsProxied(Matcher):
         return "Is proxied."
 
     def match(self, matchee):
-        if not builtin_isinstance(matchee, Proxy):
+        if not isinstance(matchee, Proxy):
             return IsNotProxied(matchee)
         return None