Merge ~cjwatson/launchpad:oci-recipe-push-rules-add into launchpad:master

[Colin Watson]

Allow adding push rules on OCIRecipe:+edit-push-rules

This is https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/388458, but with a few bugs fixed and with an improved visual layout.

Screenshot: https://people.canonical.com/~cjwatson/tmp/oci-recipe-edit-push-rules.png

[Thiago F. Pappacena (pappacena)]

LGTM (and nice visual!). I've added just a couple of comments.

review: Approve

diff --git a/lib/lp/oci/browser/ocirecipe.py b/lib/lp/oci/browser/ocirecipe.py
index 3dd764c..c2dceb2 100644
--- a/lib/lp/oci/browser/ocirecipe.py
+++ b/lib/lp/oci/browser/ocirecipe.py
@@ -25,12 +25,18 @@ from lazr.restful.interface import (
     )
 from zope.component import getUtility
 from zope.formlib.form import FormFields
+from zope.formlib.widget import (
+    DisplayWidget,
+    renderElement,
+    )
 from zope.interface import Interface
 from zope.schema import (
     Bool,
     Choice,
     List,
+    Password,
     TextLine,
+    ValidationError,
     )
 
 from lp.app.browser.launchpadform import (
@@ -44,7 +50,10 @@ from lp.app.errors import UnexpectedFormData
 from lp.app.widgets.itemswidgets import LabeledMultiCheckBoxWidget
 from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.code.browser.widgets.gitref import GitRefWidget
-from lp.oci.interfaces.ocipushrule import IOCIPushRuleSet
+from lp.oci.interfaces.ocipushrule import (
+    IOCIPushRuleSet,
+    OCIPushRuleAlreadyExists,
+    )
 from lp.oci.interfaces.ocirecipe import (
     IOCIRecipe,
     IOCIRecipeSet,
@@ -56,6 +65,8 @@ from lp.oci.interfaces.ocirecipe import (
     )
 from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuildSet
 from lp.oci.interfaces.ociregistrycredentials import (
+    IOCIRegistryCredentialsSet,
+    OCIRegistryCredentialsAlreadyExist,
     user_can_edit_credentials_for_owner,
     )
 from lp.services.features import getFeatureFlag
@@ -72,6 +83,7 @@ from lp.services.webapp import (
     stepthrough,
     structured,
     )
+from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.batching import BatchNavigator
 from lp.services.webapp.breadcrumb import NameBreadcrumb
 from lp.services.webhooks.browser import WebhookTargetNavigationMixin
@@ -264,7 +276,17 @@ def new_builds_notification_text(builds, already_pending=None):
         return builds_text
 
 
-class OCIRecipeEditPushRulesView(LaunchpadEditFormView):
+class InvisibleCredentialsWidget(DisplayWidget):
+    """A widget that just displays a private icon.
+
+    This indicates invisible credentials.
+    """
+
+    def __call__(self):
+        return renderElement("span", id=self.name, cssClass="sprite private")
+
+
+class OCIRecipeEditPushRulesView(LaunchpadFormView):
     """View for +ocirecipe-edit-push-rules.pt."""
 
     class schema(Interface):
@@ -311,41 +333,103 @@ class OCIRecipeEditPushRulesView(LaunchpadEditFormView):
         return field_type, rule_id
 
     def setUpFields(self):
-        """See `LaunchpadEditFormView`."""
-        LaunchpadEditFormView.setUpFields(self)
-
-        image_fields = []
+        super(OCIRecipeEditPushRulesView, self).setUpFields()
+        image_name_fields = []
+        url_fields = []
+        private_url_fields = []
+        username_fields = []
+        private_username_fields = []
+        password_fields = []
         delete_fields = []
-        creds = []
         for elem in list(self.context.push_rules):
-            image_fields.append(
+            image_name_fields.append(
                 TextLine(
-                    title=u'Image name',
                     __name__=self._getFieldName('image_name', elem.id),
                     default=elem.image_name,
                     required=True, readonly=False))
+            if check_permission('launchpad.View', elem.registry_credentials):
+                url_fields.append(
+                    TextLine(
+                        __name__=self._getFieldName('url', elem.id),
+                        default=elem.registry_credentials.url,
+                        required=True, readonly=True))
+                username_fields.append(
+                    TextLine(
+                        __name__=self._getFieldName('username', elem.id),
+                        default=elem.registry_credentials.username,
+                        required=True, readonly=True))
+            else:
+                # XXX cjwatson 2020-08-27: Ideally we'd be able to just show
+                # the URL, and maybe the username too, but the
+                # launchpad.View security adapter for OCIRegistryCredentials
+                # doesn't currently allow that in some cases (e.g. a
+                # team-owned recipe with a push rule using credentials owned
+                # by another team member).  In future it might make sense to
+                # add a launchpad.LimitedView adapter that grants access if
+                # the credentials are used by a push rule on one of the
+                # viewer's recipes.
+                private_url_fields.append(
+                    TextLine(
+                        __name__=self._getFieldName('url', elem.id),
+                        default='', required=True, readonly=True))
+                private_username_fields.append(
+                    TextLine(
+                        __name__=self._getFieldName('username', elem.id),
+                        default='', required=True, readonly=True))
             delete_fields.append(
                 Bool(
-                    title=u'Delete',
                     __name__=self._getFieldName('delete', elem.id),
                     default=False,
                     required=True, readonly=False))
-            creds.append(
-                TextLine(
-                    title=u'Username',
-                    __name__=self._getFieldName('username', elem.id),
-                    default=elem.registry_credentials.username,
-                    required=True, readonly=True))
-            creds.append(
-                TextLine(
-                    title=u'Registry URL',
-                    __name__=self._getFieldName('url', elem.id),
-                    default=elem.registry_credentials.url,
-                    required=True, readonly=True))
-
-        self.form_fields += FormFields(*image_fields)
-        self.form_fields += FormFields(*creds)
-        self.form_fields += FormFields(*delete_fields)
+        image_name_fields.append(
+            TextLine(
+                __name__=u'add_image_name',
+                required=False, readonly=False))
+        add_credentials = Choice(
+            __name__='add_credentials',
+            default='existing', values=('existing', 'new'),
+            required=False, readonly=False)
+        existing_credentials = Choice(
+            vocabulary='OCIRegistryCredentials',
+            required=False,
+            __name__=u'existing_credentials')
+        url_fields.append(
+            TextLine(
+                __name__=u'add_url',
+                required=False, readonly=False))
+        username_fields.append(
+            TextLine(
+                __name__=u'add_username',
+                required=False, readonly=False))
+        password_fields.append(
+            Password(
+                __name__=u'add_password',
+                required=False, readonly=False))
+        password_fields.append(
+            Password(
+                __name__=u'add_confirm_password',
+                required=False, readonly=False))
+
+        self.form_fields = (
+            FormFields(*image_name_fields) +
+            FormFields(*url_fields) +
+            FormFields(
+                *private_url_fields,
+                custom_widget=InvisibleCredentialsWidget) +
+            FormFields(*username_fields) +
+            FormFields(
+                *private_username_fields,
+                custom_widget=InvisibleCredentialsWidget) +
+            FormFields(*password_fields) +
+            FormFields(*delete_fields) +
+            FormFields(add_credentials, existing_credentials))
+
+    def setUpWidgets(self, context=None):
+        """See `LaunchpadFormView`."""
+        super(OCIRecipeEditPushRulesView, self).setUpWidgets(context=context)
+        for widget in self.widgets:
+            widget.display_label = False
+            widget.hint = None
 
     @property
     def label(self):
@@ -357,28 +441,60 @@ class OCIRecipeEditPushRulesView(LaunchpadEditFormView):
     def cancel_url(self):
         return canonical_url(self.context)
 
-    def getRulesWidgets(self, rule):
-
+    def getRuleWidgets(self, rule):
         widgets_by_name = {widget.name: widget for widget in self.widgets}
+        url_field_name = (
+                "field." + self._getFieldName("url", rule.id))
         image_field_name = (
                 "field." + self._getFieldName("image_name", rule.id))
         username_field_name = (
                 "field." + self._getFieldName("username", rule.id))
-        url_field_name = (
-                "field." + self._getFieldName("url", rule.id))
         delete_field_name = (
                 "field." + self._getFieldName("delete", rule.id))
         return {
+            "url": widgets_by_name[url_field_name],
             "image_name": widgets_by_name[image_field_name],
             "username": widgets_by_name[username_field_name],
-            "url": widgets_by_name[url_field_name],
             "delete": widgets_by_name[delete_field_name],
         }
 
+    def getNewRuleWidgets(self):
+        widgets_by_name = {widget.name: widget for widget in self.widgets}
+        return {
+            "image_name": widgets_by_name["field.add_image_name"],
+            "existing_credentials":
+                widgets_by_name["field.existing_credentials"],
+            "url": widgets_by_name["field.add_url"],
+            "username": widgets_by_name["field.add_username"],
+            "password": widgets_by_name["field.add_password"],
+            "confirm_password": widgets_by_name["field.add_confirm_password"],
+            }
+
     def parseData(self, data):
         """Rearrange form data to make it easier to process."""
         parsed_data = {}
-
+        add_image_name = data.get("add_image_name")
+        add_url = data.get("add_url")
+        add_username = data.get("add_username")
+        add_password = data.get("add_password")
+        add_confirm_password = data.get("add_confirm_password")
+        add_existing_credentials = data.get("existing_credentials")
+
+        # parse data from the Add new rule section of the form
+        if (add_url or add_username or add_password or
+                add_confirm_password or add_image_name or
+                add_existing_credentials):
+            parsed_data.setdefault(None, {
+                "image_name": add_image_name,
+                "url": add_url,
+                "username": add_username,
+                "password": add_password,
+                "confirm_password": add_confirm_password,
+                "existing_credentials": data["existing_credentials"],
+                "add_credentials": data["add_credentials"],
+                "action": "add",
+            })
+        # parse data from the Edit existing rule section of the form
         for field_name in sorted(
                 name for name in data if name.split(".")[0] == "image_name"):
             _, rule_id = self._parseFieldName(field_name)
@@ -395,6 +511,59 @@ class OCIRecipeEditPushRulesView(LaunchpadEditFormView):
 
         return parsed_data
 
+    def addNewRule(self, parsed_data):
+        add_data = parsed_data[None]
+        image_name = add_data.get("image_name")
+        url = add_data.get("url")
+        password = add_data.get("password")
+        confirm_password = add_data.get("confirm_password")
+        username = add_data.get("username")
+        existing_credentials = add_data.get("existing_credentials")
+        add_credentials = add_data.get("add_credentials")
+
+        if not image_name:
+            self.setFieldError("add_image_name", "Image name must be set.")
+            return
+
+        if add_credentials == "existing":
+            if not existing_credentials:
+                return
+
+            credentials = existing_credentials
+
+        elif add_credentials == "new":
+            if not url:
+                self.setFieldError("add_url", "Registry URL must be set.")
+                return
+            if password != confirm_password:
+                self.setFieldError(
+                    "add_confirm_password", "Passwords do not match.")
+                return
+
+            credentials_set = getUtility(IOCIRegistryCredentialsSet)
+            try:
+                credentials = credentials_set.getOrCreate(
+                    owner=self.context.owner, url=url,
+                    credentials={'username': username, 'password': password})
+            except OCIRegistryCredentialsAlreadyExist:
+                self.setFieldError(
+                    "add_url",
+                    "Credentials already exist with the same URL and "
+                    "username.")
+                return
+            except ValidationError:
+                self.setFieldError("add_url", "Not a valid URL.")
+                return
+
+        try:
+            getUtility(IOCIPushRuleSet).new(
+                self.context, credentials, image_name)
+        except OCIPushRuleAlreadyExists:
+            self.setFieldError(
+                "add_image_name",
+                "A push rule already exists with the same URL, image name, "
+                "and credentials.")
+
     def updatePushRulesFromData(self, parsed_data):
         rules_map = {
             rule.id: rule
@@ -415,6 +584,8 @@ class OCIRecipeEditPushRulesView(LaunchpadEditFormView):
                         rule.setNewImageName(image_name)
             elif action == "delete":
                 rule.destroySelf()
+            elif action == "add":
+                self.addNewRule(parsed_data)
             else:
                 raise AssertionError("unknown action: %s" % action)
 
diff --git a/lib/lp/oci/browser/tests/test_ocirecipe.py b/lib/lp/oci/browser/tests/test_ocirecipe.py
index e2cdd77..8b17168 100644
--- a/lib/lp/oci/browser/tests/test_ocirecipe.py
+++ b/lib/lp/oci/browser/tests/test_ocirecipe.py
@@ -17,9 +17,12 @@ import re
 
 from fixtures import FakeLogger
 import pytz
+from six.moves.urllib.parse import quote
 import soupmatchers
+from storm.locals import Store
 from testtools.matchers import (
     Equals,
+    Is,
     MatchesDict,
     MatchesSetwise,
     MatchesStructure,
@@ -887,12 +890,20 @@ class TestOCIRecipeEditPushRulesView(OCIConfigHelperMixin,
             "oci.build_series.%s" % self.distroseries.distribution.name:
                 self.distroseries.name,
         }))
-        oci_project = self.factory.makeOCIProject(
+        self.oci_project = self.factory.makeOCIProject(
             pillar=self.distroseries.distribution,
             ociprojectname="oci-project-name")
+
+        self.member = self.factory.makePerson()
+        self.team = self.factory.makeTeam(members=[self.person, self.member])
+
         self.recipe = self.factory.makeOCIRecipe(
             name="recipe-name", registrant=self.person, owner=self.person,
-            oci_project=oci_project)
+            oci_project=self.oci_project)
+
+        self.team_owned_recipe = self.factory.makeOCIRecipe(
+            name="recipe-name", registrant=self.person, owner=self.team,
+            oci_project=self.oci_project)
 
         self.setConfig()
 
@@ -1013,7 +1024,7 @@ class TestOCIRecipeEditPushRulesView(OCIConfigHelperMixin,
                         "Image name", "td", text=image_name))))
 
     def test_edit_oci_push_rules(self):
-        url = unicode(self.factory.getUniqueURL())
+        url = self.factory.getUniqueURL()
         credentials = {'username': 'foo', 'password': 'bar'}
         registry_credentials = getUtility(IOCIRegistryCredentialsSet).new(
             owner=self.person,
@@ -1077,8 +1088,52 @@ class TestOCIRecipeEditPushRulesView(OCIConfigHelperMixin,
         self.assertRaises(OCIPushRuleAlreadyExists,
                           browser.getControl("Save").click)
 
+    def test_edit_oci_push_rules_non_owner_of_credentials(self):
+        url = self.factory.getUniqueURL()
+        credentials = {'username': 'foo', 'password': 'bar'}
+        registry_credentials = getUtility(IOCIRegistryCredentialsSet).new(
+            owner=self.person,
+            url=url,
+            credentials=credentials)
+        image_names = [self.factory.getUniqueUnicode() for _ in range(2)]
+        push_rules = [
+            getUtility(IOCIPushRuleSet).new(
+                recipe=self.team_owned_recipe,
+                registry_credentials=registry_credentials,
+                image_name=image_name)
+            for image_name in image_names]
+        Store.of(push_rules[-1]).flush()
+        push_rule_ids = [push_rule.id for push_rule in push_rules]
+        browser = self.getViewBrowser(self.team_owned_recipe, user=self.member)
+        browser.getLink("Edit push rules").click()
+        row = soupmatchers.Tag(
+            "push rule row", "tr", attrs={"class": "push-rule"})
+        self.assertThat(browser.contents, soupmatchers.HTMLContains(
+            soupmatchers.Within(
+                row,
+                soupmatchers.Tag(
+                    "username widget", "span",
+                    attrs={
+                        "id": "field.username.%d" % push_rule_ids[0],
+                        "class": "sprite private",
+                        })),
+            soupmatchers.Within(
+                row,
+                soupmatchers.Tag(
+                    "url widget", "span",
+                    attrs={
+                        "id": "field.url.%d" % push_rule_ids[0],
+                        "class": "sprite private",
+                        }))))
+        browser.getControl(
+            name="field.image_name.%d" % push_rule_ids[0]).value = "image1"
+        browser.getControl("Save").click()
+        with person_logged_in(self.member):
+            self.assertEqual("image1", push_rules[0].image_name)
+            self.assertEqual(image_names[1], push_rules[1].image_name)
+
     def test_delete_oci_push_rules(self):
-        url = unicode(self.factory.getUniqueURL())
+        url = self.factory.getUniqueURL()
         credentials = {'username': 'foo', 'password': 'bar'}
         registry_credentials = getUtility(IOCIRegistryCredentialsSet).new(
             owner=self.person,
@@ -1100,8 +1155,215 @@ class TestOCIRecipeEditPushRulesView(OCIConfigHelperMixin,
             self.assertIsNone(
                 getUtility(IOCIPushRuleSet).getByID(push_rule.id))
 
+    def test_add_oci_push_rules_validations(self):
+        # Add new rule works when there are no rules in the DB.
+        browser = self.getViewBrowser(self.recipe, user=self.person)
+        browser.getLink("Edit push rules").click()
+
+        # Save does not error if there are no changes on the form.
+        browser.getControl("Save").click()
+        self.assertIn("Saved push rules", browser.contents)
+
+        # If only an image name is given but no registry URL, we fail with
+        # "Registry URL must be set".
+        browser.getLink("Edit push rules").click()
+        browser.getControl(name="field.add_image_name").value = "imagename1"
+        browser.getControl(name="field.add_credentials").value = "new"
+        browser.getControl("Save").click()
+        self.assertIn("Registry URL must be set", browser.contents)
+
+        # No image name entered on the form.  We assume user is only editing
+        # and we allow saving the form.
+        browser.getControl(name="field.add_image_name").value = ""
+        browser.getControl("Save").click()
+        self.assertIn("Saved push rules", browser.contents)
+
+    def test_add_oci_push_rules_new_empty_credentials(self):
+        # Supplying an image name and registry URL creates a credentials
+        # object without username or password, and a valid push rule based
+        # on that credentials object.
+        url = self.factory.getUniqueURL()
+        browser = self.getViewBrowser(self.recipe, user=self.person)
+        browser.getLink("Edit push rules").click()
+        browser.getControl(name="field.add_credentials").value = "new"
+        browser.getControl(name="field.add_image_name").value = "imagename1"
+        browser.getControl(name="field.add_url").value = url
+        browser.getControl("Save").click()
+        with person_logged_in(self.person):
+            rules = list(removeSecurityProxy(
+                getUtility(IOCIPushRuleSet).findByRecipe(self.recipe)))
+        self.assertEqual(len(rules), 1)
+        rule = rules[0]
+        self.assertThat(rule, MatchesStructure(
+            image_name=Equals("imagename1"),
+            registry_url=Equals(url),
+            registry_credentials=MatchesStructure(
+                url=Equals(url),
+                username=Is(None))))
+
+        with person_logged_in(self.person):
+            self.assertEqual(
+                {"password": None}, rule.registry_credentials.getCredentials())
+
+    def test_add_oci_push_rules_new_username_password(self):
+        # Supplying an image name, registry URL, username, and password
+        # creates a credentials object with the given username or password,
+        # and a valid push rule based on that credentials object.
+        url = self.factory.getUniqueURL()
+        browser = self.getViewBrowser(self.recipe, user=self.person)
+        browser.getLink("Edit push rules").click()
+        browser.getControl(name="field.add_credentials").value = "new"
+        browser.getControl(name="field.add_image_name").value = "imagename3"
+        browser.getControl(name="field.add_url").value = url
+        browser.getControl(name="field.add_username").value = "username"
+        browser.getControl(name="field.add_password").value = "password"
+        browser.getControl(
+            name="field.add_confirm_password").value = "password"
+        browser.getControl("Save").click()
+        with person_logged_in(self.person):
+            rules = list(removeSecurityProxy(
+                getUtility(IOCIPushRuleSet).findByRecipe(self.recipe)))
+        self.assertEqual(len(rules), 1)
+        rule = rules[0]
+        self.assertThat(rule, MatchesStructure(
+            image_name=Equals("imagename3"),
+            registry_url=Equals(url),
+            registry_credentials=MatchesStructure.byEquality(
+                url=url,
+                username="username")))
+        with person_logged_in(self.person):
+            self.assertEqual(
+                {"username": "username", "password": "password"},
+                rule.registry_credentials.getCredentials())
+
+    def test_add_oci_push_rules_existing_credentials_duplicate(self):
+        # Adding a new push rule using existing credentials fails if a rule
+        # with the same image name already exists.
+        existing_rule = self.factory.makeOCIPushRule(
+            recipe=self.recipe,
+            registry_credentials=self.factory.makeOCIRegistryCredentials(
+                owner=self.recipe.owner))
+        existing_image_name = existing_rule.image_name
+        existing_registry_url = existing_rule.registry_url
+        existing_username = existing_rule.username
+        browser = self.getViewBrowser(self.recipe, user=self.person)
+        browser.getLink("Edit push rules").click()
+        browser.getControl(name="field.add_credentials").value = "existing"
+        browser.getControl(name="field.add_image_name").value = (
+            existing_image_name)
+        browser.getControl(name="field.existing_credentials").value = (
+            "%s %s" % (quote(existing_registry_url), quote(existing_username)))
+        browser.getControl("Save").click()
+        self.assertIn(
+            "A push rule already exists with the same URL, "
+            "image name, and credentials.", browser.contents)
+
+    def test_add_oci_push_rules_existing_credentials(self):
+        # Previously added registry credentials can be chosen from the radio
+        # widget when adding a new rule.
+        # We correctly display the radio buttons widget when the
+        # username is empty in registry credentials and
+        # allow correctly adding new rule based on it
+        existing_rule = self.factory.makeOCIPushRule(
+            recipe=self.recipe,
+            registry_credentials=self.factory.makeOCIRegistryCredentials(
+                owner=self.recipe.owner, credentials={}))
+        existing_registry_url = existing_rule.registry_url
+        browser = self.getViewBrowser(self.recipe, user=self.person)
+        browser.getLink("Edit push rules").click()
+        browser.getControl(name="field.add_credentials").value = "existing"
+        browser.getControl(name="field.add_image_name").value = "imagename2"
+        browser.getControl(name="field.existing_credentials").value = (
+            quote(existing_registry_url))
+        browser.getControl("Save").click()
+        with person_logged_in(self.person):
+            rules = list(removeSecurityProxy(
+                getUtility(IOCIPushRuleSet).findByRecipe(self.recipe)))
+        self.assertEqual(len(rules), 2)
+        rule = rules[1]
+        self.assertThat(rule, MatchesStructure(
+            image_name=Equals("imagename2"),
+            registry_url=Equals(existing_registry_url),
+            registry_credentials=MatchesStructure(
+                url=Equals(existing_registry_url),
+                username=Is(None))))
+        with person_logged_in(self.person):
+            self.assertEqual({}, rule.registry_credentials.getCredentials())
+
+    def test_add_oci_push_rules_team_owned(self):
+        url = self.factory.getUniqueURL()
+        browser = self.getViewBrowser(self.team_owned_recipe, user=self.member)
+        browser.getLink("Edit push rules").click()
+        browser.getControl(
+            name="field.add_image_name").value = "imagename1"
+        browser.getControl(
+            name="field.add_url").value = url
+        browser.getControl(name="field.add_credentials").value = "new"
+        browser.getControl("Save").click()
+
+        with person_logged_in(self.member):
+            rules = list(removeSecurityProxy(
+                getUtility(IOCIPushRuleSet).findByRecipe(
+                    self.team_owned_recipe)))
+        self.assertEqual(len(rules), 1)
+        rule = rules[0]
+        self.assertThat(rule, MatchesStructure(
+            image_name=Equals(u'imagename1'),
+            registry_url=Equals(url),
+            registry_credentials=MatchesStructure(
+                url=Equals(url),
+                username=Is(None))))
+
+        with person_logged_in(self.member):
+            self.assertThat(
+                rule.registry_credentials.getCredentials(),
+                MatchesDict(
+                    {"password": Equals(None)}))
+
+    def test_edit_oci_push_rules_team_owned(self):
+        url = self.factory.getUniqueURL()
+        browser = self.getViewBrowser(self.team_owned_recipe, user=self.member)
+        browser.getLink("Edit push rules").click()
+        browser.getControl(
+            name="field.add_image_name").value = "imagename1"
+        browser.getControl(
+            name="field.add_url").value = url
+        browser.getControl(name="field.add_credentials").value = "new"
+        browser.getControl("Save").click()
+
+        # push rules created by another team member (self.member)
+        # can be edited by self.person
+        browser = self.getViewBrowser(self.team_owned_recipe, user=self.person)
+        browser.getLink("Edit push rules").click()
+        with person_logged_in(self.person):
+            rules = list(removeSecurityProxy(
+                getUtility(IOCIPushRuleSet).findByRecipe(
+                    self.team_owned_recipe)))
+        self.assertEqual(len(rules), 1)
+        rule = rules[0]
+        self.assertEqual("imagename1", browser.getControl(
+            name="field.image_name.%d" % rule.id).value)
+
+        # set image name to valid string
+        with person_logged_in(self.person):
+            browser.getControl(
+                name="field.image_name.%d" % rule.id).value = "image1"
+        browser.getControl("Save").click()
+
+        # and assert model changed
+        with person_logged_in(self.member):
+            self.assertEqual(
+                rule.image_name, "image1")
+
+        # self.member will see the new image name
+        browser = self.getViewBrowser(self.team_owned_recipe, user=self.member)
+        browser.getLink("Edit push rules").click()
+        with person_logged_in(self.member):
+            self.assertEqual("image1", browser.getControl(
+                name="field.image_name.%d" % rule.id).value)
+
     def test_edit_oci_registry_creds(self):
-        url = unicode(self.factory.getUniqueURL())
+        url = self.factory.getUniqueURL()
         credentials = {'username': 'foo', 'password': 'bar'}
         image_name = self.factory.getUniqueUnicode()
         registry_credentials = getUtility(IOCIRegistryCredentialsSet).new(
diff --git a/lib/lp/oci/javascript/ocirecipe.edit.js b/lib/lp/oci/javascript/ocirecipe.edit.js
 new file mode 100644
index 0000000..8b3d82d
--- /dev/null
+++ b/lib/lp/oci/javascript/ocirecipe.edit.js
@@ -0,0 +1,38 @@
+/* Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * @module Y.lp.oci.ocirecipe.edit
+ * @requires node, DOM
+ */
+YUI.add('lp.oci.ocirecipe.edit', function(Y) {
+    var module = Y.namespace('lp.oci.ocirecipe.edit');
+
+    module.set_enabled = function(field_id, is_enabled) {
+        var field = Y.DOM.byId(field_id);
+        if (field !== null) {
+            field.disabled = !is_enabled;
+        }
+    };
+
+    module.onclick_add_credentials = function(e) {
+        var value = '';
+        Y.all('input[name="field.add_credentials"]').each(function(node) {
+            if (node.get('checked')) {
+                value = node.get('value');
+            }
+        });
+        module.set_enabled('field.existing_credentials', value === 'existing');
+        module.set_enabled('field.add_url', value === 'new');
+        module.set_enabled('field.add_username', value === 'new');
+        module.set_enabled('field.add_password', value === 'new');
+        module.set_enabled('field.add_confirm_password', value === 'new');
+    };
+
+    module.setup = function() {
+        Y.all('input[name="field.add_credentials"]').on(
+            'click', module.onclick_add_credentials);
+
+        // Set the initial state.
+        module.onclick_add_credentials();
+    };
+}, '0.1', {'requires': ['node', 'DOM']});
diff --git a/lib/lp/oci/templates/ocirecipe-edit-push-rules.pt b/lib/lp/oci/templates/ocirecipe-edit-push-rules.pt
index ccfe716..eee7db7 100644
--- a/lib/lp/oci/templates/ocirecipe-edit-push-rules.pt
+++ b/lib/lp/oci/templates/ocirecipe-edit-push-rules.pt
@@ -7,35 +7,142 @@
   i18n:domain="launchpad">
 <body>
 
+<metal:block fill-slot="head_epilogue">
+  <style type="text/css">
+    table.push-rules-table {
+      max-width: 60%;
+      margin-bottom: 1em;
+    }
+    table.push-rules-table tr.even {
+      background-color: #eee;
+    }
+    /* These add up to 100%. */
+    tr .push-rule-url {
+      width: 35%;
+    }
+    tr .push-rule-image-name {
+      width: 20%;
+    }
+    tr .push-rule-username {
+      width: 10%;
+    }
+    tr .push-rule-password, tr .push-rule-confirm-password {
+      width: 15%;
+    }
+    tr .push-rule-delete {
+      width: 5%;
+    }
+  </style>
+</metal:block>
+
 <div metal:fill-slot="main">
     <div metal:use-macro="context/@@launchpad_form/form">
     <metal:formbody fill-slot="widgets">
       <p condition="view/can_edit_credentials">
         <a class="sprite edit" tal:attributes="href context/owner/fmt:url/+edit-oci-registry-credentials">Edit OCI registry credentials</a>
       </p>
-      <table class="form">
-        <tr tal:repeat="rule context/push_rules">
-          <tal:rules_widgets
-            define="rules_widgets python:view.getRulesWidgets(rule);
-                    parity python:'even' if repeat['rule'].even() else 'odd'">
-            <td tal:define="widget nocall:rules_widgets/image_name">
-              <metal:widget use-macro="context/@@launchpad_form/widget_div" />
-            </td>
-            <td tal:define="widget nocall:rules_widgets/username">
-              <metal:widget use-macro="context/@@launchpad_form/widget_div" />
-            </td>
-            <td tal:define="widget nocall:rules_widgets/url">
-              <metal:widget use-macro="context/@@launchpad_form/widget_div" />
-            </td>
-            <td tal:define="widget nocall:rules_widgets/delete">
-              <metal:widget use-macro="context/@@launchpad_form/widget_div" />
-            </td>
-          </tal:rules_widgets>
-        </tr>
+      <table class="listing push-rules-table">
+        <thead>
+          <tr>
+            <th class="push-rule-image-name">Image name</th>
+            <th class="push-rule-url">Registry URL</th>
+            <th class="push-rule-username">Username</th>
+            <th class="push-rule-password">Password</th>
+            <th class="push-rule-confirm-password">Confirm password</th>
+            <th class="push-rule-delete">Delete?</th>
+          </tr>
+        </thead>
+        <tbody>
+          <tal:rule repeat="rule view/push_rules">
+            <tal:rule_widgets
+                define="rule_widgets python:view.getRuleWidgets(rule);
+                        parity python:'even' if repeat['rule'].even() else 'odd'">
+              <tr tal:attributes="class string:push-rule ${parity}">
+                <td class="push-rule-image-name"
+                    tal:define="widget nocall:rule_widgets/image_name">
+                  <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+                </td>
+                <td class="push-rule-url"
+                    tal:define="widget nocall:rule_widgets/url">
+                  <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+                </td>
+                <td class="push-rule-username"
+                    tal:define="widget nocall:rule_widgets/username">
+                  <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+                </td>
+                <td colspan="2" />
+                <td class="push-rule-delete"
+                    tal:define="widget nocall:rule_widgets/delete">
+                  <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+                </td>
+              </tr>
+            </tal:rule_widgets>
+          </tal:rule>
+          <tal:new-rule
+              define="new_rule_widgets python:view.getNewRuleWidgets();
+                      parity python:'odd' if len(view.push_rules) % 2
+                                          else 'even'">
+            <tr tal:attributes="class parity">
+              <td class="push-rule-image-name"
+                  tal:define="widget nocall:new_rule_widgets/image_name">
+                <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+              </td>
+              <td colspan="5" />
+            </tr>
+            <tr tal:attributes="class parity">
+              <td>
+                <label>
+                  <input type="radio" name="field.add_credentials"
+                         value="existing" checked="checked" />
+                  Use existing credentials:
+                </label>
+              </td>
+              <td colspan="4"
+                  tal:define="widget nocall:new_rule_widgets/existing_credentials">
+                <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+              </td>
+              <td />
+            </tr>
+            <tr tal:attributes="class parity">
+              <td>
+                <label>
+                  <input type="radio" name="field.add_credentials"
+                         value="new" />
+                  Add new credentials:
+                </label>
+              </td>
+              <td class="push-rule-url"
+                  tal:define="widget nocall:new_rule_widgets/url">
+                <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+              </td>
+              <td class="push-rule-username"
+                  tal:define="widget nocall:new_rule_widgets/username">
+                <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+              </td>
+              <td class="push-rule-password"
+                  tal:define="widget nocall:new_rule_widgets/password">
+                <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+              </td>
+              <td class="push-rule-confirm-password"
+                  tal:define="widget nocall:new_rule_widgets/confirm_password">
+                <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+              </td>
+              <td />
+            </tr>
+          </tal:new-rule>
+        </tbody>
       </table>
     </metal:formbody>
 </div>
 
+  <script type="text/javascript">
+    LPJS.use('lp.oci.ocirecipe.edit', function(Y) {
+      Y.on('domready', function(e) {
+        Y.lp.oci.ocirecipe.edit.setup();
+      }, window);
+    });
+  </script>
+
 </div>
 </body>
 </html>
diff --git a/lib/lp/oci/vocabularies.py b/lib/lp/oci/vocabularies.py
index aa1fae3..e29fcd9 100644
--- a/lib/lp/oci/vocabularies.py
+++ b/lib/lp/oci/vocabularies.py
@@ -8,10 +8,19 @@ from __future__ import absolute_import, print_function, unicode_literals
 __metaclass__ = type
 __all__ = []
 
+from six.moves.urllib.parse import (
+    quote,
+    unquote,
+    )
+from zope.component import getUtility
 from zope.interface import implementer
 from zope.schema.vocabulary import SimpleTerm
 
+from lp.oci.interfaces.ociregistrycredentials import (
+    IOCIRegistryCredentialsSet,
+    )
 from lp.oci.model.ocirecipe import OCIRecipe
+from lp.oci.model.ociregistrycredentials import OCIRegistryCredentials
 from lp.services.webapp.vocabulary import (
     IHugeVocabulary,
     StormVocabularyBase,
@@ -35,6 +44,49 @@ class OCIRecipeDistroArchSeriesVocabulary(StormVocabularyBase):
         return len(self.context.getAllowedArchitectures())
 
 
+class OCIRegistryCredentialsVocabulary(StormVocabularyBase):
+
+    _table = OCIRegistryCredentials
+
+    def toTerm(self, obj):
+        if obj.username:
+            token = "%s %s" % (quote(obj.url), quote(obj.username))
+            title = "%s (%s)" % (obj.url, obj.username)
+        else:
+            token = quote(obj.url)
+            title = obj.url
+
+        return SimpleTerm(obj, token, title)
+
+    @property
+    def _entries(self):
+        return list(getUtility(
+            IOCIRegistryCredentialsSet).findByOwner(self.context.owner))
+
+    def __contains__(self, value):
+        """See `IVocabulary`."""
+        return value in self._entries
+
+    def __len__(self):
+        return len(self._entries)
+
+    def getTermByToken(self, token):
+        """See `IVocabularyTokenized`."""
+        try:
+            if ' ' in token:
+                url, username = token.split(' ', 1)
+                url = unquote(url)
+                username = unquote(username)
+            else:
+                username = None
+                url = unquote(token)
+            for obj in self._entries:
+                if obj.url == url and obj.username == username:
+                    return self.toTerm(obj)
+        except ValueError:
+            raise LookupError(token)
+
+
 @implementer(IHugeVocabulary)
 class OCIRecipeVocabulary(StormVocabularyBase):
     """All OCI Recipes of a given OCI project."""
diff --git a/lib/lp/oci/vocabularies.zcml b/lib/lp/oci/vocabularies.zcml
index 1a6b75c..24a57d8 100644
--- a/lib/lp/oci/vocabularies.zcml
+++ b/lib/lp/oci/vocabularies.zcml
@@ -16,6 +16,16 @@
     </class>
 
     <securedutility
+        name="OCIRegistryCredentials"
+        component="lp.oci.vocabularies.OCIRegistryCredentialsVocabulary"
+        provides="zope.schema.interfaces.IVocabularyFactory">
+        <allow interface="zope.schema.interfaces.IVocabularyFactory" />
+    </securedutility>
+
+    <class class="lp.oci.vocabularies.OCIRegistryCredentialsVocabulary">
+        <allow interface="lp.services.webapp.vocabulary.IHugeVocabulary" />
+    </class>
+    <securedutility
         name="OCIRecipe"
         component="lp.oci.vocabularies.OCIRecipeVocabulary"
         provides="zope.schema.interfaces.IVocabularyFactory">