launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/webservice-commit-feature-flag into lp:launchpad.

Commit message:
Disable the post-webservice-GET commit if the webservice.read_only_commit.disabled feature flag is set.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/webservice-commit-feature-flag/+merge/335287

I've had a quick look through the current set of @export_read_operation methods and haven't found any obvious cases that would cause trouble, but I suppose we'll see.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/webservice-commit-feature-flag into lp:launchpad.

=== modified file 'lib/lp/services/webapp/servers.py'
--- lib/lp/services/webapp/servers.py	2017-09-16 13:31:57 +0000
+++ lib/lp/services/webapp/servers.py	2017-12-17 01:39:00 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Definition of the internet servers that Launchpad uses."""
@@ -64,7 +64,10 @@
 from lp.app.errors import UnexpectedFormData
 import lp.layers
 from lp.services.config import config
-from lp.services.features import get_relevant_feature_controller
+from lp.services.features import (
+    get_relevant_feature_controller,
+    getFeatureFlag,
+    )
 from lp.services.features.flags import NullFeatureController
 from lp.services.feeds.interfaces.application import IFeedsApplication
 from lp.services.feeds.interfaces.feed import IFeed
@@ -1240,17 +1243,21 @@
 
     def finishReadOnlyRequest(self, request, ob, txn):
         """Commit the transaction even though there should be no writes."""
-        # WebServicePublication used to commit on every request to store
-        # OAuthNonces to prevent replay attacks. But TLS prevents replay
-        # attacks too, so we don't bother with nonces any more. However,
-        # this commit will stay here until we can switch it off in a
-        # controlled test to ensure that nothing depends on it on prod.
-        notify(FinishReadOnlyRequestEvent(ob, request))
-        # Transaction commits usually need to be aware of the possibility of
-        # a doomed transaction.  We do not expect that this code will
-        # encounter doomed transactions.  If it does, this will need to be
-        # revisited.
-        txn.commit()
+        if getFeatureFlag('webservice.read_only_commit.disabled'):
+            super(WebServicePublication, self).finishReadOnlyRequest(
+                request, ob, txn)
+        else:
+            # WebServicePublication used to commit on every request to store
+            # OAuthNonces to prevent replay attacks. But TLS prevents replay
+            # attacks too, so we don't bother with nonces any more. However,
+            # this commit will stay here until we can switch it off in a
+            # controlled test to ensure that nothing depends on it on prod.
+            notify(FinishReadOnlyRequestEvent(ob, request))
+            # Transaction commits usually need to be aware of the
+            # possibility of a doomed transaction.  We do not expect that
+            # this code will encounter doomed transactions.  If it does,
+            # this will need to be revisited.
+            txn.commit()
 
     def getPrincipal(self, request):
         """See `LaunchpadBrowserPublication`.

=== modified file 'lib/lp/services/webapp/tests/test_servers.py'
--- lib/lp/services/webapp/tests/test_servers.py	2016-09-14 11:13:06 +0000
+++ lib/lp/services/webapp/tests/test_servers.py	2017-12-17 01:39:00 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -31,6 +31,7 @@
     )
 
 from lp.app import versioninfo
+from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.interfaces import IFinishReadOnlyRequestEvent
 from lp.services.webapp.publication import LaunchpadBrowserPublication
 from lp.services.webapp.servers import (
@@ -52,7 +53,10 @@
     EventRecorder,
     TestCase,
     )
-from lp.testing.layers import FunctionalLayer
+from lp.testing.layers import (
+    FunctionalLayer,
+    ZopelessDatabaseLayer,
+    )
 
 
 class SetInWSGIEnvironmentTestCase(TestCase):
@@ -694,7 +698,7 @@
         self.log.append("ABORT")
 
 
-class TestFinishReadOnlyRequest(TestCase):
+class TestFinishReadOnlyRequestMixin:
     # Publications that have a finishReadOnlyRequest() method are obliged to
     # fire an IFinishReadOnlyRequestEvent.
 
@@ -725,6 +729,8 @@
         self.assertIs(fake_request, finish_event.request)
         self.assertIs(fake_object, finish_event.object)
 
+class TestFinishReadOnlyRequest(TestFinishReadOnlyRequestMixin, TestCase):
+
     def test_WebServicePub_fires_FinishReadOnlyRequestEvent(self):
         # WebServicePublication.finishReadOnlyRequest() issues an
         # IFinishReadOnlyRequestEvent and commits the transaction.
@@ -738,6 +744,23 @@
         self._test_publication(publication, ["ABORT"])
 
 
+class TestFinishReadOnlyRequestFeatureFlag(
+        TestFinishReadOnlyRequestMixin, TestCase):
+
+    layer = ZopelessDatabaseLayer
+
+    def test_WebServicePub_aborts_if_read_only_commit_disabled(self):
+        # If the webservice.read_only_commit.disabled feature flag is set,
+        # WebServicePublication.finishReadOnlyRequest() behaves like
+        # LaunchpadBrowserPublication.finishReadOnlyRequest() and aborts the
+        # transaction.  (This is intended to be the default behaviour in
+        # future.)
+        self.useFixture(FeatureFixture(
+            {'webservice.read_only_commit.disabled': 'on'}))
+        publication = WebServicePublication(None)
+        self._test_publication(publication, ["ABORT"])
+
+
 def test_suite():
     suite = unittest.TestSuite()
     suite.addTest(DocTestSuite(