← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] ~cjwatson/launchpad:oci-new-push-rule-credentials-owner into launchpad:master

 

Colin Watson has proposed merging ~cjwatson/launchpad:oci-new-push-rule-credentials-owner into launchpad:master.

Commit message:
Export OCIRecipe.newPushRule(credentials_owner) on the API

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/389760

It normally makes sense to allow controlling the owner of newly-created objects such as OCI registry credentials in webservice API methods.  It's also more likely for the recipe owner to be a reasonable default for the owner of new credentials than the logged-in user: team-owned recipes will normally want team-owned credentials too.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:oci-new-push-rule-credentials-owner into launchpad:master.
diff --git a/lib/lp/oci/interfaces/ocirecipe.py b/lib/lp/oci/interfaces/ocirecipe.py
index 37f74a2..c433825 100644
--- a/lib/lp/oci/interfaces/ocirecipe.py
+++ b/lib/lp/oci/interfaces/ocirecipe.py
@@ -300,7 +300,6 @@ class IOCIRecipeEdit(IWebhookTarget):
     def destroySelf():
         """Delete this OCI recipe, provided that it has no builds."""
 
-    @call_with(owner=REQUEST_USER)
     @operation_parameters(
         registry_url=TextLine(
             title=_("Registry URL"),
@@ -314,11 +313,16 @@ class IOCIRecipeEdit(IWebhookTarget):
             title=_("Registry credentials"),
             description=_(
                 "The credentials to use in pushing the image to the registry"),
-            required=True))
+            required=True),
+        credentials_owner=PersonChoice(
+            title=_("Registry credentials owner"),
+            required=False,
+            vocabulary="AllUserTeamsParticipationPlusSelf"))
     # Really IOCIPushRule, patched in lp.oci.interfaces.webservice.
     @export_factory_operation(Interface, [])
     @operation_for_version("devel")
-    def newPushRule(owner, registry_url, image_name, credentials):
+    def newPushRule(registry_url, image_name, credentials,
+                    credentials_owner=None):
         """Add a new rule for pushing builds of this recipe to a registry."""
 
 
diff --git a/lib/lp/oci/model/ocirecipe.py b/lib/lp/oci/model/ocirecipe.py
index 0125114..2a83ccc 100644
--- a/lib/lp/oci/model/ocirecipe.py
+++ b/lib/lp/oci/model/ocirecipe.py
@@ -498,11 +498,17 @@ class OCIRecipe(Storm, WebhookTargetMixin):
     def can_upload_to_registry(self):
         return not self.push_rules.is_empty()
 
-    def newPushRule(self, owner, registry_url, image_name, credentials):
+    def newPushRule(self, registry_url, image_name, credentials,
+                    credentials_owner=None):
         """See `IOCIRecipe`."""
-
+        if credentials_owner is None:
+            # Ideally this should probably be a required parameter, but
+            # earlier versions of this method didn't allow passing the
+            # credentials owner via the webservice API, so for compatibility
+            # we give it a default.
+            credentials_owner = self.owner
         oci_credentials = getUtility(IOCIRegistryCredentialsSet).getOrCreate(
-            owner, registry_url, credentials)
+            credentials_owner, registry_url, credentials)
         push_rule = getUtility(IOCIPushRuleSet).new(
             self, oci_credentials, image_name)
         Store.of(push_rule).flush()
diff --git a/lib/lp/oci/tests/helpers.py b/lib/lp/oci/tests/helpers.py
index e199241..52561a3 100644
--- a/lib/lp/oci/tests/helpers.py
+++ b/lib/lp/oci/tests/helpers.py
@@ -11,6 +11,11 @@ __all__ = []
 import base64
 
 from nacl.public import PrivateKey
+from testtools.matchers import (
+    AfterPreprocessing,
+    MatchesAll,
+    )
+from zope.security.proxy import removeSecurityProxy
 
 from lp.oci.interfaces.ocirecipe import OCI_RECIPE_ALLOW_CREATE
 from lp.services.features.testing import FeatureFixture
@@ -30,3 +35,18 @@ class OCIConfigHelperMixin:
                 bytes(self.private_key)).decode("UTF-8"))
         # Default feature flags for our tests
         self.useFixture(FeatureFixture({OCI_RECIPE_ALLOW_CREATE: 'on'}))
+
+
+class MatchesOCIRegistryCredentials(MatchesAll):
+    """Matches an `OCIRegistryCredentials` object.
+
+    `main_matcher` matches the `OCIRegistryCredentials` object itself, while
+    `credentials_matcher` matches the result of its `getCredentials` method.
+    """
+
+    def __init__(self, main_matcher, credentials_matcher):
+        super(MatchesOCIRegistryCredentials, self).__init__(
+            main_matcher,
+            AfterPreprocessing(
+                lambda matchee: removeSecurityProxy(matchee).getCredentials(),
+                credentials_matcher))
diff --git a/lib/lp/oci/tests/test_ocirecipe.py b/lib/lp/oci/tests/test_ocirecipe.py
index 880c351..ad67a7a 100644
--- a/lib/lp/oci/tests/test_ocirecipe.py
+++ b/lib/lp/oci/tests/test_ocirecipe.py
@@ -8,10 +8,7 @@ from __future__ import absolute_import, print_function, unicode_literals
 import json
 
 from fixtures import FakeLogger
-from six import (
-    ensure_text,
-    string_types,
-    )
+from six import string_types
 from storm.exceptions import LostObjectError
 from storm.store import Store
 from testtools.matchers import (
@@ -51,7 +48,10 @@ from lp.oci.interfaces.ocirecipe import (
     )
 from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuildSet
 from lp.oci.interfaces.ocirecipejob import IOCIRecipeRequestBuildsJobSource
-from lp.oci.tests.helpers import OCIConfigHelperMixin
+from lp.oci.tests.helpers import (
+    MatchesOCIRegistryCredentials,
+    OCIConfigHelperMixin,
+    )
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.config import config
 from lp.services.database.constants import (
@@ -453,39 +453,53 @@ class TestOCIRecipe(OCIConfigHelperMixin, TestCaseWithFactory):
         self.setConfig()
         recipe = self.factory.makeOCIRecipe()
         url = self.factory.getUniqueURL()
-        image_name = ensure_text(self.factory.getUniqueString())
+        image_name = self.factory.getUniqueUnicode()
         credentials = {
             "username": "test-username", "password": "test-password"}
 
-        with person_logged_in(recipe.owner):
+        with person_logged_in(recipe.registrant):
             push_rule = recipe.newPushRule(
-                recipe.owner, url, image_name, credentials)
-            self.assertEqual(
-                image_name,
-                push_rule.image_name)
-            self.assertEqual(
-                url,
-                push_rule.registry_credentials.url)
-            self.assertEqual(
-                credentials,
-                push_rule.registry_credentials.getCredentials())
-
-            self.assertEqual(
-                push_rule,
-                recipe.push_rules[0])
+                url, image_name, credentials,
+                credentials_owner=recipe.registrant)
+            self.assertThat(push_rule, MatchesStructure(
+                image_name=Equals(image_name),
+                registry_credentials=MatchesOCIRegistryCredentials(
+                    MatchesStructure.byEquality(
+                        owner=recipe.registrant, url=url),
+                    Equals(credentials))))
+            self.assertEqual(push_rule, recipe.push_rules[0])
+
+    def test_newPushRule_default_owner(self):
+        # The registry credentials for a new push rule default to being
+        # owned by the recipe owner.
+        self.setConfig()
+        recipe = self.factory.makeOCIRecipe()
+        url = self.factory.getUniqueURL()
+        image_name = self.factory.getUniqueUnicode()
+        credentials = {
+            "username": "test-username", "password": "test-password"}
+
+        with person_logged_in(recipe.registrant):
+            push_rule = recipe.newPushRule(url, image_name, credentials)
+            self.assertThat(push_rule, MatchesStructure(
+                image_name=Equals(image_name),
+                registry_credentials=MatchesOCIRegistryCredentials(
+                    MatchesStructure.byEquality(owner=recipe.owner, url=url),
+                    Equals(credentials))))
+            self.assertEqual(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())
+        image_name = self.factory.getUniqueUnicode()
         credentials = {
             "username": "test-username", "password": "test-password"}
 
         with person_logged_in(recipe.owner):
             self.assertRaises(
                 ValidationError, recipe.newPushRule,
-                recipe.owner, url, image_name, credentials)
+                url, image_name, credentials, credentials_owner=recipe.owner)
             # Avoid trying to flush the incomplete object on cleanUp.
             Store.of(recipe).rollback()
 
@@ -493,17 +507,17 @@ class TestOCIRecipe(OCIConfigHelperMixin, TestCaseWithFactory):
         self.setConfig()
         recipe = self.factory.makeOCIRecipe()
         url = self.factory.getUniqueURL()
-        image_name = ensure_text(self.factory.getUniqueString())
+        image_name = self.factory.getUniqueUnicode()
         credentials = {
             "username": "test-username", "password": "test-password"}
 
         with person_logged_in(recipe.owner):
             recipe.newPushRule(
-                recipe.owner, url, image_name, credentials)
+                url, image_name, credentials, credentials_owner=recipe.owner)
             self.assertRaises(
                 OCIPushRuleAlreadyExists,
                 recipe.newPushRule,
-                recipe.owner, url, image_name, credentials)
+                url, image_name, credentials, credentials_owner=recipe.owner)
 
     def test_set_official_directly_is_forbidden(self):
         recipe = self.factory.makeOCIRecipe()
@@ -1151,7 +1165,11 @@ class TestOCIRecipeWebservice(OCIConfigHelperMixin, TestCaseWithFactory):
 
         new_obj_url = resp.getHeader("Location")
         ws_push_rule = self.load_from_api(new_obj_url)
-        self.assertEqual(obj["image_name"], ws_push_rule["image_name"])
+        self.assertThat(ws_push_rule, ContainsDict({
+            "image_name": Equals(obj["image_name"]),
+            "registry_url": Equals(obj["registry_url"]),
+            "username": Equals("foo"),
+            }))
 
     def test_api_push_rules_exported(self):
         """Are push rules exported for a recipe?"""
diff --git a/lib/lp/registry/browser/tests/test_person.py b/lib/lp/registry/browser/tests/test_person.py
index 029f12c..e3f74db 100644
--- a/lib/lp/registry/browser/tests/test_person.py
+++ b/lib/lp/registry/browser/tests/test_person.py
@@ -22,11 +22,9 @@ from testscenarios import (
     WithScenarios,
     )
 from testtools.matchers import (
-    AfterPreprocessing,
     DocTestMatches,
     Equals,
     LessThan,
-    MatchesAll,
     MatchesDict,
     MatchesSetwise,
     MatchesStructure,
@@ -50,7 +48,10 @@ from lp.oci.interfaces.ocirecipe import OCI_RECIPE_ALLOW_CREATE
 from lp.oci.interfaces.ociregistrycredentials import (
     IOCIRegistryCredentialsSet,
     )
-from lp.oci.tests.helpers import OCIConfigHelperMixin
+from lp.oci.tests.helpers import (
+    MatchesOCIRegistryCredentials,
+    OCIConfigHelperMixin,
+    )
 from lp.registry.browser.person import PersonView
 from lp.registry.browser.team import TeamInvitationView
 from lp.registry.enums import PersonVisibility
@@ -1322,21 +1323,6 @@ class TestPersonRelatedProjectsView(TestCaseWithFactory):
         self.assertThat(view(), next_match)
 
 
-class MatchesOCIRegistryCredentials(MatchesAll):
-    """Matches an `OCIRegistryCredentials` object.
-
-    `main_matcher` matches the `OCIRegistryCredentials` object itself, while
-    `credentials_matcher` matches the result of its `getCredentials` method.
-    """
-
-    def __init__(self, main_matcher, credentials_matcher):
-        super(MatchesOCIRegistryCredentials, self).__init__(
-            main_matcher,
-            AfterPreprocessing(
-                lambda matchee: removeSecurityProxy(matchee).getCredentials(),
-                credentials_matcher))
-
-
 class TestPersonOCIRegistryCredentialsView(
         WithScenarios, BrowserTestCase, OCIConfigHelperMixin):