launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:fix-oci-build-args-proxy into launchpad:master.

Commit message:
Removing security proxy on data being sent over xml-rpc call

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/391760
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:fix-oci-build-args-proxy into launchpad:master.

diff --git a/lib/lp/oci/model/ocirecipebuildbehaviour.py b/lib/lp/oci/model/ocirecipebuildbehaviour.py
index 742c8a2..1008884 100644
--- a/lib/lp/oci/model/ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/model/ocirecipebuildbehaviour.py
@@ -96,7 +96,10 @@ class OCIRecipeBuildBehaviour(SnapProxyMixin, BuildFarmJobBehaviourBase):
                 logger=logger))
 
         args['build_file'] = build.recipe.build_file
-        args['build_args'] = build.recipe.build_args
+        # We have to remove the security proxy that Zope applies to this
+        # dict, since otherwise we'll be unable to serialise it to
+        # XML-RPC.
+        args['build_args'] = removeSecurityProxy(build.recipe.build_args)
         args['build_path'] = build.recipe.build_path
 
         if build.recipe.git_ref is not None:
diff --git a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
index 6397405..9a15fb9 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
@@ -36,6 +36,7 @@ from testtools.twistedsupport import (
     )
 from twisted.internet import defer
 from zope.component import getUtility
+from zope.proxy import isProxy
 from zope.security.proxy import removeSecurityProxy
 
 from lp.buildmaster.enums import (
@@ -176,6 +177,24 @@ class TestAsyncOCIRecipeBuildBehaviour(MakeOCIBuildMixin, TestCaseWithFactory):
         self.useFixture(FeatureFixture({OCI_RECIPE_ALLOW_CREATE: 'on'}))
         self.addCleanup(shut_down_default_process_pool)
 
+    def assertHasNoZopeSecurityProxy(self, data):
+        """Makes sure that data doesn't contain a security proxy.
+
+        `data` can be a list, a tuple, a dict or an ordinary value. This
+        method checks `data` itself, and if it's a collection, it checks
+        each item in it.
+        """
+        self.assertFalse(
+            isProxy(data), "%s should not be a security proxy." % data)
+        # If it's a collection, keep searching for proxies.
+        if isinstance(data, (list, tuple)):
+            for i in data:
+                self.assertHasNoZopeSecurityProxy(i)
+        elif isinstance(data, dict):
+            for k, v in data.items():
+                self.assertHasNoZopeSecurityProxy(k)
+                self.assertHasNoZopeSecurityProxy(v)
+
     @defer.inlineCallbacks
     def test_composeBuildRequest(self):
         [ref] = self.factory.makeGitRefs()
@@ -238,6 +257,9 @@ class TestAsyncOCIRecipeBuildBehaviour(MakeOCIBuildMixin, TestCaseWithFactory):
             self.assertIn('universe', archive_line)
         with dbuser(config.builddmaster.dbuser):
             args = yield job.extraBuildArgs()
+        # Asserts that nothing here is a zope proxy, to avoid errors when
+        # serializing it for XML-RPC call.
+        self.assertHasNoZopeSecurityProxy(args)
         self.assertThat(args, MatchesDict({
             "archive_private": Is(False),
             "archives": Equals(expected_archives),
@@ -270,6 +292,7 @@ class TestAsyncOCIRecipeBuildBehaviour(MakeOCIBuildMixin, TestCaseWithFactory):
             self.assertIn('universe', archive_line)
         with dbuser(config.builddmaster.dbuser):
             args = yield job.extraBuildArgs()
+        self.assertHasNoZopeSecurityProxy(args)
         self.assertThat(args, MatchesDict({
             "archive_private": Is(False),
             "archives": Equals(expected_archives),