launchpad-reviewers team mailing list archive

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

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:registry-url-validation into launchpad:master.

Commit message:
Avoiding invalid registry URL for OCI push rules.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/384570
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:registry-url-validation into launchpad:master.

diff --git a/lib/lp/oci/interfaces/ociregistrycredentials.py b/lib/lp/oci/interfaces/ociregistrycredentials.py
index 0405905..5c5a71a 100644
--- a/lib/lp/oci/interfaces/ociregistrycredentials.py
+++ b/lib/lp/oci/interfaces/ociregistrycredentials.py
@@ -22,7 +22,10 @@ from zope.schema import (
 
 from lp import _
 from lp.registry.interfaces.role import IHasOwner
-from lp.services.fields import PersonChoice
+from lp.services.fields import (
+    PersonChoice,
+    URIField,
+    )
 
 
 @error_status(http_client.CONFLICT)
@@ -61,7 +64,8 @@ class IOCIRegistryCredentialsEditableAttributes(IHasOwner):
                       "push rules using them."),
         readonly=False)
 
-    url = TextLine(
+    url = URIField(
+        allowed_schemes=['http', 'https'],
         title=_("URL"),
         description=_("The registry URL."),
         required=True,
diff --git a/lib/lp/oci/model/ociregistrycredentials.py b/lib/lp/oci/model/ociregistrycredentials.py
index e94c597..49c740f 100644
--- a/lib/lp/oci/model/ociregistrycredentials.py
+++ b/lib/lp/oci/model/ociregistrycredentials.py
@@ -23,8 +23,10 @@ from storm.locals import (
     )
 from zope.component import getUtility
 from zope.interface import implementer
+from zope.schema import ValidationError
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.validators.url import validate_url
 from lp.oci.interfaces.ociregistrycredentials import (
     IOCIRegistryCredentials,
     IOCIRegistryCredentialsSet,
@@ -59,6 +61,15 @@ class OCIRegistrySecretsEncryptedContainer(NaClEncryptedContainerBase):
             return None
 
 
+def url_validator(allowed_schemes):
+    def wrapped(obj, attr, value):
+        if not validate_url(value, allowed_schemes):
+            raise ValidationError(
+                "%s is not a valid URL for '%s' attribute" % (value, attr))
+        return value
+    return wrapped
+
+
 @implementer(IOCIRegistryCredentials)
 class OCIRegistryCredentials(Storm):
 
@@ -69,7 +80,9 @@ class OCIRegistryCredentials(Storm):
     owner_id = Int(name='owner', allow_none=False)
     owner = Reference(owner_id, 'Person.id')
 
-    url = Unicode(name="url", allow_none=False)
+    url = Unicode(
+        name="url", allow_none=False, validator=url_validator(
+            IOCIRegistryCredentials['url'].allowed_schemes))
 
     _credentials = JSON(name="credentials", allow_none=True)
 
diff --git a/lib/lp/oci/tests/test_ocipushrule.py b/lib/lp/oci/tests/test_ocipushrule.py
index ed3c0b6..4c6db56 100644
--- a/lib/lp/oci/tests/test_ocipushrule.py
+++ b/lib/lp/oci/tests/test_ocipushrule.py
@@ -5,14 +5,17 @@
 
 from __future__ import absolute_import, print_function, unicode_literals
 
+from storm.store import Store
 from testtools.matchers import MatchesStructure
 from zope.component import getUtility
+from zope.schema import ValidationError
 
 from lp.oci.interfaces.ocipushrule import (
     IOCIPushRule,
     IOCIPushRuleSet,
     OCIPushRuleAlreadyExists,
     )
+from lp.oci.model.ociregistrycredentials import OCIRegistryCredentials
 from lp.oci.tests.helpers import OCIConfigHelperMixin
 from lp.testing import (
     person_logged_in,
@@ -57,6 +60,17 @@ class TestOCIPushRule(OCIConfigHelperMixin, TestCaseWithFactory):
             registry_credentials=credentials)
         self.assertEqual(credentials.username, push_rule.username)
 
+    def test_valid_registry_url(self):
+        owner = self.factory.makePerson()
+        url = "asdf://foo.com"
+        credentials = {"username": "foo"}
+        self.assertRaisesRegex(
+            ValidationError,
+            "asdf://foo.com is not a valid URL for 'url' attribute",
+            OCIRegistryCredentials, owner, url, credentials)
+        # Avoid trying to flush the incomplete object on cleanUp.
+        Store.of(owner).rollback()
+
 
 class TestOCIPushRuleSet(OCIConfigHelperMixin, TestCaseWithFactory):
 
diff --git a/lib/lp/oci/tests/test_ocirecipe.py b/lib/lp/oci/tests/test_ocirecipe.py
index 74d15e0..35bdab9 100644
--- a/lib/lp/oci/tests/test_ocirecipe.py
+++ b/lib/lp/oci/tests/test_ocirecipe.py
@@ -13,6 +13,7 @@ from six import (
     string_types,
     )
 from storm.exceptions import LostObjectError
+from storm.store import Store
 from testtools.matchers import (
     ContainsDict,
     Equals,
@@ -24,6 +25,7 @@ from testtools.matchers import (
     )
 import transaction
 from zope.component import getUtility
+from zope.schema import ValidationError
 from zope.security.interfaces import (
     ForbiddenAttribute,
     Unauthorized,
@@ -432,6 +434,21 @@ class TestOCIRecipe(OCIConfigHelperMixin, TestCaseWithFactory):
                 push_rule,
                 recipe.push_rules[0])
 
+    def test_newPushRule_invalid_url(self):
+        self.setConfig()
+        recipe = self.factory.makeOCIRecipe()
+        url = 'asdf://foo.com'
+        image_name = ensure_text(self.factory.getUniqueString())
+        credentials = {
+            "username": "test-username", "password": "test-password"}
+
+        with person_logged_in(recipe.owner):
+            self.assertRaises(
+                ValidationError, recipe.newPushRule,
+                recipe.owner, url, image_name, credentials)
+            # Avoid trying to flush the incomplete object on cleanUp.
+            Store.of(recipe).rollback()
+
     def test_newPushRule_same_details(self):
         self.setConfig()
         recipe = self.factory.makeOCIRecipe()