Merge ~cjwatson/launchpad:artifactory-quoting-on-deploy into launchpad:master

[Colin Watson]

URL-quote property values for ArtifactoryPath.deploy_file

`ArtifactoryPath.deploy_file` and `ArtifactoryPath.set_properties` have two entirely distinct quoting conventions. Astonishingly, the Python bindings for the former leave this entirely up to the caller without saying so. Fortunately, all that `deploy_file` needs is URL-quoting according to `urllib.parse.quote_plus`. Do this.

In the process of fixing this, I noticed that the Artifactory fixture's endpoints don't correctly unquote the URL path, so I fixed that.

Testing against a real instance:

  >>> (testpath / 'pool' / 'base-files_12ubuntu4.1_amd64.deb').deploy_file('/home/cjwatson/base-files_12ubuntu4.1_amd64.deb', parameters={'unquoted': ['foo+bar;baz'], 'backslashquoted': [r'foo\+bar\;baz'], 'urlquoted': [quote_plus('foo+bar;baz')]})
  >>> (testpath / 'pool' / 'base-files_12ubuntu4.1_amd64.deb').properties
  {'unquoted': ['foo bar'], 'backslashquoted': ['foo\\ bar\\'], 'urlquoted': ['foo+bar;baz'], 'baz': ['']}

[Jürgen Gmach (jugmac00)]

review: Approve

diff --git a/lib/lp/archivepublisher/artifactory.py b/lib/lp/archivepublisher/artifactory.py
index 611100c..2268d71 100644
--- a/lib/lp/archivepublisher/artifactory.py
+++ b/lib/lp/archivepublisher/artifactory.py
@@ -13,6 +13,7 @@ import tempfile
 from collections import defaultdict
 from pathlib import Path, PurePath
 from typing import List, Optional
+from urllib.parse import quote_plus
 
 import requests
 from artifactory import ArtifactoryPath
@@ -271,6 +272,12 @@ class ArtifactoryPoolEntry:
         properties = self.calculateProperties(
             self.makeReleaseID(self.pub_file), []
         )
+        # Property values must be URL-quoted for
+        # `ArtifactoryPath.deploy_file`; it does not handle that itself.
+        properties = {
+            key: [quote_plus(v) for v in value]
+            for key, value in properties.items()
+        }
         fd, name = tempfile.mkstemp(prefix="temp-download.")
         f = os.fdopen(fd, "wb")
         try:
diff --git a/lib/lp/archivepublisher/tests/artifactory_fixture.py b/lib/lp/archivepublisher/tests/artifactory_fixture.py
index 93bcf41..13a9ab8 100644
--- a/lib/lp/archivepublisher/tests/artifactory_fixture.py
+++ b/lib/lp/archivepublisher/tests/artifactory_fixture.py
@@ -11,9 +11,10 @@ import fnmatch
 import hashlib
 import json
 import re
+from collections import defaultdict
 from datetime import datetime, timezone
 from pathlib import Path
-from urllib.parse import parse_qs, unquote, urlparse
+from urllib.parse import parse_qs, unquote, unquote_plus, urlparse
 
 import responses
 from fixtures import Fixture
@@ -88,7 +89,7 @@ class FakeArtifactoryFixture(Fixture):
 
     def _handle_download(self, request):
         """Handle a request to download an existing file."""
-        path = urlparse(request.url[len(self.repo_url) :]).path
+        path = unquote(urlparse(request.url[len(self.repo_url) :]).path)
         if path in self._fs and "size" in self._fs[path]:
             return (
                 200,
@@ -101,7 +102,7 @@ class FakeArtifactoryFixture(Fixture):
     def _handle_stat(self, request):
         """Handle a request to stat an existing file."""
         parsed_url = urlparse(request.url[len(self.api_url) :])
-        path = parsed_url.path
+        path = unquote(parsed_url.path)
         if path in self._fs:
             stat = {"repo": self.repository_name, "path": path}
             stat.update(self._fs[path])
@@ -134,8 +135,22 @@ class FakeArtifactoryFixture(Fixture):
         """
         return re.sub(r"\\([,|=;])", r"\1", unquote(text))
 
+    def _decode_matrix_parameters(self, encoded):
+        """Decode matrix parameters that were encoded as part of a request.
+
+        `ArtifactoryPath.deploy` encodes properties like this.
+        """
+        properties = defaultdict(list)
+        for param in encoded.split(";"):
+            key, value = param.split("=", 1)
+            properties[unquote_plus(key)].append(unquote_plus(value))
+        return properties
+
     def _decode_properties(self, encoded):
-        """Decode properties that were encoded as part of a request."""
+        """Decode properties that were encoded as part of a request.
+
+        `ArtifactoryPath.set_properties` encodes properties like this.
+        """
         properties = {}
         for param in self._split(";", encoded):
             key, value = re.match(r"((?:\\[,|=;]|[^=])+)=(.*)", param).groups()
@@ -155,11 +170,11 @@ class FakeArtifactoryFixture(Fixture):
         else:
             params = ""
         parsed_url = urlparse(url)
-        path = parsed_url.path
+        path = unquote(parsed_url.path)
         if path.endswith("/"):
             self.add_dir(path.rstrip("/"))
         elif path.rsplit("/", 1)[0] in self._fs:
-            properties = self._decode_properties(params)
+            properties = self._decode_matrix_parameters(params)
             self.add_file(
                 path,
                 request.body,
@@ -171,7 +186,7 @@ class FakeArtifactoryFixture(Fixture):
     def _handle_set_properties(self, request):
         """Handle a request to set properties on an existing file."""
         parsed_url = urlparse(request.url[len(self.api_url) :])
-        path = parsed_url.path
+        path = unquote(parsed_url.path)
         if path in self._fs:
             query = parse_qs(parsed_url.query)
             properties = self._decode_properties(query["properties"][0])
@@ -183,7 +198,7 @@ class FakeArtifactoryFixture(Fixture):
     def _handle_delete_properties(self, request):
         """Handle a request to delete properties from an existing file."""
         parsed_url = urlparse(request.url[len(self.api_url) :])
-        path = parsed_url.path
+        path = unquote(parsed_url.path)
         if path in self._fs:
             query = parse_qs(parsed_url.query)
             for key in query["properties"][0].split(","):
@@ -265,7 +280,7 @@ class FakeArtifactoryFixture(Fixture):
 
     def _handle_delete(self, request):
         """Handle a request to delete an existing file."""
-        path = urlparse(request.url[len(self.repo_url) :]).path
+        path = unquote(urlparse(request.url[len(self.repo_url) :]).path)
         if not path.endswith("/") and path in self._fs:
             self.remove_file(path)
         return 200, {}, ""
diff --git a/lib/lp/archivepublisher/tests/test_artifactory.py b/lib/lp/archivepublisher/tests/test_artifactory.py
index 0e013e5..196571d 100644
--- a/lib/lp/archivepublisher/tests/test_artifactory.py
+++ b/lib/lp/archivepublisher/tests/test_artifactory.py
@@ -131,8 +131,8 @@ class TestArtifactoryPool(TestCase):
         foo = ArtifactoryPoolTestingFile(
             pool=pool,
             source_name="foo",
-            source_version="1.0",
-            filename="foo-1.0.deb",
+            source_version="1.0+1",
+            filename="foo-1.0+1.deb",
             release_type=FakeReleaseType.BINARY,
             release_id=1,
         )
@@ -144,7 +144,7 @@ class TestArtifactoryPool(TestCase):
             {
                 "launchpad.release-id": ["binary:1"],
                 "launchpad.source-name": ["foo"],
-                "launchpad.source-version": ["1.0"],
+                "launchpad.source-version": ["1.0+1"],
             },
             foo.getProperties(),
         )