Merge ~ilasc/launchpad:oci-recipe-edit-existing-rules into launchpad:master

[Ioana Lasc]

Edit push rules and credentials on Recipe

This branch adds the capability to edit and delete existing push rules on the Recipe page and from there also navigate to a screen where the user can edit & delete existing OCI registry credentials and add new ones.

The capability to add new push rules will follow in a separate MP.

[Thiago F. Pappacena (pappacena)]

Thanks for splitting the MP, ilasc. It helped a lot!

I have added a couple of comments, mostly about permission checking and a doubt the URL and implementation for the "Edit credentials" screen here and on /~/+edit-oci-registry-credentials.

review: Needs Information

df8fb5e... by Ioana Lasc on 2020-07-14

Redirect to Person page for registry credential CRUD

5bf7dbe... by Ioana Lasc on 2020-07-14

Add edit permissions to OCIPushRule

[Ioana Lasc (ilasc)]

Thiago thank you for the helpful comments!

Fully agreed on all: edit credentials here was a duplication of the Person page capabilities and we didn't really need to remove the security proxy to edit / destroy push rules in code.

MP is now ready for another look and much shorter!

[Thiago F. Pappacena (pappacena)]

Couple of small details about code style (see also twom comments) and avoiding duplicated URLs for the same set of objects. But otherwise, LGTM.

review: Approve

8d719cd... by Ioana Lasc on 2020-07-20

Address code review comments

49f4c67... by Ioana Lasc on 2020-07-20

Remove accidentally checked in banner.css

bcc0ec0... by Ioana Lasc on 2020-07-20

Fix issue with image name

[Ioana Lasc (ilasc)]

Thiago this is the branch I mentioned in Standup, addressed all comments and replied to Tom's.

The change I mentioned is in the last commit that adds the missing Unit Tests for "multiple push rules already in the DB when editing the image name on 1 rule".

It might be worth having another look at the MP to see if you spot anything else as there were code updates.

Thanks Thiago

review: Approve

[Thiago F. Pappacena (pappacena)]

The new test look really good. Good catch on spotting the bug!

I've added a minor comment, but the MP seems to be good to go.

diff --git a/lib/lp/oci/browser/configure.zcml b/lib/lp/oci/browser/configure.zcml
index b66e509..0aa6da8 100644
--- a/lib/lp/oci/browser/configure.zcml
+++ b/lib/lp/oci/browser/configure.zcml
@@ -61,6 +61,13 @@
             template="../../app/templates/generic-edit.pt" />
         <browser:page
             for="lp.oci.interfaces.ocirecipe.IOCIRecipe"
+            class="lp.oci.browser.ocirecipe.OCIRecipeEditPushRulesView"
+            permission="launchpad.Edit"
+            name="+edit-push-rules"
+            template="../templates/ocirecipe-edit-push-rules.pt" />
+
+        <browser:page
+            for="lp.oci.interfaces.ocirecipe.IOCIRecipe"
             class="lp.oci.browser.ocirecipe.OCIRecipeDeleteView"
             permission="launchpad.Edit"
             name="+delete"
diff --git a/lib/lp/oci/browser/ocirecipe.py b/lib/lp/oci/browser/ocirecipe.py
index 356ac1c..cac47ac 100644
--- a/lib/lp/oci/browser/ocirecipe.py
+++ b/lib/lp/oci/browser/ocirecipe.py
@@ -11,6 +11,7 @@ __all__ = [
     'OCIRecipeAdminView',
     'OCIRecipeContextMenu',
     'OCIRecipeDeleteView',
+    'OCIRecipeEditPushRulesView',
     'OCIRecipeEditView',
     'OCIRecipeNavigation',
     'OCIRecipeNavigationMenu',
@@ -23,10 +24,13 @@ from lazr.restful.interface import (
     use_template,
     )
 from zope.component import getUtility
+from zope.formlib.form import FormFields
 from zope.interface import Interface
 from zope.schema import (
+    Bool,
     Choice,
     List,
+    TextLine,
     )
 
 from lp.app.browser.launchpadform import (
@@ -36,6 +40,7 @@ from lp.app.browser.launchpadform import (
     )
 from lp.app.browser.lazrjs import InlinePersonEditPickerWidget
 from lp.app.browser.tales import format_link
+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
@@ -138,12 +143,17 @@ class OCIRecipeContextMenu(ContextMenu):
 
     facet = 'overview'
 
-    links = ('request_builds',)
+    links = ('request_builds', 'edit_push_rules')
 
     @enabled_with_permission('launchpad.Edit')
     def request_builds(self):
         return Link('+request-builds', 'Request builds', icon='add')
 
+    @enabled_with_permission('launchpad.Edit')
+    def edit_push_rules(self):
+        return Link(
+            '+edit-push-rules', 'Edit push rules', icon='edit')
+
 
 class OCIProjectRecipesView(LaunchpadView):
     """Default view for the list of OCI recipes of an OCI project."""
@@ -243,6 +253,165 @@ def new_builds_notification_text(builds, already_pending=None):
         return builds_text
 
 
+class OCIRecipeEditPushRulesView(LaunchpadEditFormView):
+    """View for +ocirecipe-edit-push-rules.pt."""
+
+    class schema(Interface):
+        """Schema for editing push rules."""
+
+    @cachedproperty
+    def push_rules(self):
+        return list(
+            getUtility(IOCIPushRuleSet).findByRecipe(self.context))
+
+    @property
+    def has_push_rules(self):
+        return len(self.push_rules) > 0
+
+    def _getFieldName(self, name, rule_id):
+        """Get the combined field name for an `OCIPushRule` ID.
+
+        In order to be able to render a table, we encode the rule ID
+        in the form field name.
+        """
+        return "%s.%d" % (name, rule_id)
+
+    def _parseFieldName(self, field_name):
+        """Parse a combined field name as described in `_getFieldName`.
+
+        :raises UnexpectedFormData: if the field name cannot be parsed or
+            the `OCIPushRule` cannot be found.
+        """
+        field_bits = field_name.split(".")
+        if len(field_bits) != 2:
+            raise UnexpectedFormData(
+                "Cannot parse field name: %s" % field_name)
+        field_type = field_bits[0]
+        try:
+            rule_id = int(field_bits[1])
+        except ValueError:
+            raise UnexpectedFormData(
+                "Cannot parse field name: %s" % field_name)
+        return field_type, rule_id
+
+    def setUpFields(self):
+        """See `LaunchpadEditFormView`."""
+        LaunchpadEditFormView.setUpFields(self)
+
+        image_fields = []
+        delete_fields = []
+        creds = []
+        for elem in list(self.context.push_rules):
+            image_fields.append(
+                TextLine(
+                    title=u'Image name',
+                    __name__=self._getFieldName('image_name', elem.id),
+                    default=elem.image_name,
+                    required=True, readonly=False))
+            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)
+
+    @property
+    def label(self):
+        return 'Edit OCI push rules for %s' % self.context.name
+
+    page_title = 'Edit OCI push rules'
+
+    @property
+    def cancel_url(self):
+        return canonical_url(self.context)
+
+    def getRulesWidgets(self, rule):
+
+        widgets_by_name = {widget.name: widget for widget in self.widgets}
+        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 {
+            "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 parseData(self, data):
+        """Rearrange form data to make it easier to process."""
+        parsed_data = {}
+
+        for field_name in sorted(
+                name for name in data if name.split(".")[0] == "image_name"):
+            _, rule_id = self._parseFieldName(field_name)
+            image_field_name = self._getFieldName("image_name", rule_id)
+            delete_field_name = self._getFieldName("delete", rule_id)
+            if data.get(delete_field_name):
+                action = "delete"
+            else:
+                action = "change"
+            parsed_data.setdefault(rule_id, {
+                "image_name": data.get(image_field_name),
+                "action": action,
+            })
+
+        return parsed_data
+
+    def updatePushRulesFromData(self, parsed_data):
+        rules_map = {
+            rule.id: rule
+            for rule in self.context.push_rules}
+        for rule_id, parsed_rules in parsed_data.items():
+            rule = rules_map.get(rule_id)
+            action = parsed_rules["action"]
+
+            if action == "change":
+                image_name = parsed_rules["image_name"]
+                if not image_name:
+                    self.setFieldError(
+                        self._getFieldName(
+                            "image_name", rule_id),
+                        "Image name must be set.")
+                else:
+                    if rule.image_name != image_name:
+                        rule.setNewImageName(image_name)
+            elif action == "delete":
+                rule.destroySelf()
+            else:
+                raise AssertionError("unknown action: %s" % action)
+
+    @action("Save")
+    def save(self, action, data):
+        parsed_data = self.parseData(data)
+        self.updatePushRulesFromData(parsed_data)
+
+        if not self.errors:
+            self.request.response.addNotification("Saved push rules")
+            self.next_url = canonical_url(self.context)
+
+
 class OCIRecipeRequestBuildsView(LaunchpadFormView):
     """A view for requesting builds of an OCI recipe."""
 
@@ -322,6 +491,7 @@ class IOCIRecipeEditSchema(Interface):
         "build_file",
         "build_daily",
         "require_virtualized",
+        "push_rules",
         ])
 
 
@@ -453,6 +623,7 @@ class OCIRecipeEditView(BaseOCIRecipeEditView, EnableProcessorsMixin):
         "git_ref",
         "build_file",
         "build_daily",
+        "push_rules",
         )
     custom_widget_git_ref = GitRefWidget
 
diff --git a/lib/lp/oci/browser/tests/test_ocirecipe.py b/lib/lp/oci/browser/tests/test_ocirecipe.py
index e25d463..24a9c6b 100644
--- a/lib/lp/oci/browser/tests/test_ocirecipe.py
+++ b/lib/lp/oci/browser/tests/test_ocirecipe.py
@@ -19,6 +19,8 @@ from fixtures import FakeLogger
 import pytz
 import soupmatchers
 from testtools.matchers import (
+    Equals,
+    MatchesDict,
     MatchesSetwise,
     MatchesStructure,
     )
@@ -35,11 +37,19 @@ from lp.oci.browser.ocirecipe import (
     OCIRecipeEditView,
     OCIRecipeView,
     )
+from lp.oci.interfaces.ocipushrule import (
+    IOCIPushRuleSet,
+    OCIPushRuleAlreadyExists,
+    )
 from lp.oci.interfaces.ocirecipe import (
     CannotModifyOCIRecipeProcessor,
     IOCIRecipeSet,
     OCI_RECIPE_ALLOW_CREATE,
     )
+from lp.oci.interfaces.ociregistrycredentials import (
+    IOCIRegistryCredentialsSet,
+    )
+from lp.oci.tests.helpers import OCIConfigHelperMixin
 from lp.services.database.constants import UTC_NOW
 from lp.services.features.testing import FeatureFixture
 from lp.services.propertycache import get_property_cache
@@ -831,6 +841,157 @@ class TestOCIRecipeRequestBuildsView(BaseTestOCIRecipeView):
             extract_text(find_main_content(browser.contents)))
 
 
+class TestOCIRecipeEditPushRulesView(OCIConfigHelperMixin,
+                                     BaseTestOCIRecipeView):
+    def setUp(self):
+        super(TestOCIRecipeEditPushRulesView, self).setUp()
+        self.ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+        self.distroseries = self.factory.makeDistroSeries(
+            distribution=self.ubuntu, name="shiny", displayname="Shiny")
+        self.architectures = []
+        for processor, architecture in ("386", "i386"), ("amd64", "amd64"):
+            das = self.factory.makeDistroArchSeries(
+                distroseries=self.distroseries, architecturetag=architecture,
+                processor=getUtility(IProcessorSet).getByName(processor))
+            das.addOrUpdateChroot(self.factory.makeLibraryFileAlias())
+            self.architectures.append(das)
+        self.useFixture(FeatureFixture({
+            OCI_RECIPE_ALLOW_CREATE: "on",
+            "oci.build_series.%s" % self.distroseries.distribution.name:
+                self.distroseries.name,
+        }))
+        oci_project = self.factory.makeOCIProject(
+            pillar=self.distroseries.distribution,
+            ociprojectname="oci-project-name")
+        self.recipe = self.factory.makeOCIRecipe(
+            name="recipe-name", registrant=self.person, owner=self.person,
+            oci_project=oci_project)
+
+        self.setConfig()
+
+    def test_edit_oci_push_rules(self):
+        url = unicode(self.factory.getUniqueURL())
+        credentials = {'username': 'foo', 'password': 'bar'}
+        registry_credentials = getUtility(IOCIRegistryCredentialsSet).new(
+            owner=self.person,
+            url=url,
+            credentials=credentials)
+        image_name = self.factory.getUniqueUnicode()
+        push_rule = getUtility(IOCIPushRuleSet).new(
+            recipe=self.recipe,
+            registry_credentials=registry_credentials,
+            image_name=image_name)
+        browser = self.getViewBrowser(self.recipe, user=self.person)
+        browser.getLink("Edit push rules").click()
+        # assert image name is displayed correctly
+        with person_logged_in(self.person):
+            self.assertEqual(image_name, browser.getControl(
+                name="field.image_name.%d" % push_rule.id).value)
+
+        # assert image name is required
+        with person_logged_in(self.person):
+            browser.getControl(
+                name="field.image_name.%d" % push_rule.id).value = ""
+        browser.getControl("Save").click()
+        self.assertIn("Required input is missing", browser.contents)
+
+        # set image name to valid string
+        with person_logged_in(self.person):
+            browser.getControl(
+                name="field.image_name.%d" % push_rule.id).value = "image1"
+        browser.getControl("Save").click()
+        # and assert model changed
+        with person_logged_in(self.person):
+            self.assertEqual(
+                push_rule.image_name, "image1")
+            # Create a second push rule and test we call setNewImageName only
+            # in cases where image name is different than the one on the model
+            # otherwise we get the exception on rows the user doesn't actually
+            # edit as the image name stays the same as it already is on the
+            # model and setNewImageName will obviously find that rule in the
+            # db with the same details
+            second_rule = getUtility(IOCIPushRuleSet).new(
+                recipe=self.recipe,
+                registry_credentials=registry_credentials,
+                image_name="second image")
+        browser = self.getViewBrowser(self.recipe, user=self.person)
+        browser.getLink("Edit push rules").click()
+        with person_logged_in(self.person):
+            browser.getControl(
+                name="field.image_name.%d" % push_rule.id).value = "image2"
+        browser.getControl("Save").click()
+        with person_logged_in(self.person):
+            self.assertEqual(
+                push_rule.image_name, "image2")
+
+        # Attempt to set the same name on the second rule
+        # will result in expected exception
+        browser = self.getViewBrowser(self.recipe, user=self.person)
+        browser.getLink("Edit push rules").click()
+        with person_logged_in(self.person):
+            browser.getControl(
+                name="field.image_name.%d" % second_rule.id).value = "image2"
+        self.assertRaises(OCIPushRuleAlreadyExists,
+                          browser.getControl("Save").click)
+
+    def test_delete_oci_push_rules(self):
+        url = unicode(self.factory.getUniqueURL())
+        credentials = {'username': 'foo', 'password': 'bar'}
+        registry_credentials = getUtility(IOCIRegistryCredentialsSet).new(
+            owner=self.person,
+            url=url,
+            credentials=credentials)
+        image_name = self.factory.getUniqueUnicode()
+        push_rule = getUtility(IOCIPushRuleSet).new(
+            recipe=self.recipe,
+            registry_credentials=registry_credentials,
+            image_name=image_name)
+        browser = self.getViewBrowser(self.recipe, user=self.person)
+        browser.getLink("Edit push rules").click()
+        with person_logged_in(self.person):
+            browser.getControl(
+                name="field.delete.%d" % push_rule.id).value = True
+        browser.getControl("Save").click()
+
+        with person_logged_in(self.person):
+            self.assertIsNone(
+                getUtility(IOCIPushRuleSet).getByID(push_rule.id))
+
+    def test_edit_oci_registry_creds(self):
+        url = unicode(self.factory.getUniqueURL())
+        credentials = {'username': 'foo', 'password': 'bar'}
+        image_name = self.factory.getUniqueUnicode()
+        registry_credentials = getUtility(IOCIRegistryCredentialsSet).new(
+            owner=self.person,
+            url=url,
+            credentials=credentials)
+        getUtility(IOCIPushRuleSet).new(
+            recipe=self.recipe,
+            registry_credentials=registry_credentials,
+            image_name=image_name)
+        browser = self.getViewBrowser(self.recipe, user=self.person)
+        browser.getLink("Edit push rules").click()
+        browser.getLink("Edit OCI registry credentials").click()
+
+        browser.getControl(name="field.add_url").value = url
+        browser.getControl(name="field.add_username").value = "new_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):
+            creds = list(getUtility(
+                IOCIRegistryCredentialsSet).findByOwner(
+                self.person))
+
+            self.assertEqual(url, creds[1].url)
+            self.assertThat(
+                (creds[1]).getCredentials(),
+                MatchesDict({"username": Equals("new_username"),
+                             "password": Equals("password")}))
+
+
 class TestOCIProjectRecipesView(BaseTestOCIRecipeView):
     def setUp(self):
         super(TestOCIProjectRecipesView, self).setUp()
diff --git a/lib/lp/oci/templates/ocirecipe-edit-push-rules.pt b/lib/lp/oci/templates/ocirecipe-edit-push-rules.pt
new file mode 100644
index 0000000..a1adb89
--- /dev/null
+++ b/lib/lp/oci/templates/ocirecipe-edit-push-rules.pt
@@ -0,0 +1,41 @@
+<html
+  xmlns="http://www.w3.org/1999/xhtml"
+  xmlns:tal="http://xml.zope.org/namespaces/tal"
+  xmlns:metal="http://xml.zope.org/namespaces/metal"
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
+  metal:use-macro="view/macro:page/main_only"
+  i18n:domain="launchpad">
+<body>
+
+<div metal:fill-slot="main">
+    <div metal:use-macro="context/@@launchpad_form/form">
+    <metal:formbody fill-slot="widgets">
+      <p condition="context/required:launchpad.Edit">
+        <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>
+    </metal:formbody>
+</div>
+
+</div>
+</body>
+</html>
diff --git a/lib/lp/oci/templates/ocirecipe-index.pt b/lib/lp/oci/templates/ocirecipe-index.pt
index cf7c549..88a95b3 100644
--- a/lib/lp/oci/templates/ocirecipe-index.pt
+++ b/lib/lp/oci/templates/ocirecipe-index.pt
@@ -143,6 +143,11 @@
       This OCI recipe has no push rules defined yet.
     </p>
 
+    <div tal:define="link context/menu:context/edit_push_rules"
+         tal:condition="link/enabled">
+      <tal:edit-push-rules replace="structure link/fmt:link"/>
+    </div>
+
   </div>
 
 </body>
diff --git a/lib/lp/registry/browser/person.py b/lib/lp/registry/browser/person.py
index 83e5757..a8ef6fc 100644
--- a/lib/lp/registry/browser/person.py
+++ b/lib/lp/registry/browser/person.py
@@ -102,7 +102,10 @@ from zope.schema.vocabulary import (
     SimpleVocabulary,
     )
 from zope.security.interfaces import Unauthorized
-from zope.security.proxy import removeSecurityProxy
+from zope.security.proxy import (
+    isinstance as zope_isinstance,
+    removeSecurityProxy,
+    )
 
 from lp import _
 from lp.app.browser.launchpadform import (
@@ -138,10 +141,12 @@ from lp.code.errors import InvalidNamespace
 from lp.code.interfaces.branchnamespace import IBranchNamespaceSet
 from lp.code.interfaces.gitlookup import IGitTraverser
 from lp.oci.interfaces.ocipushrule import IOCIPushRuleSet
+from lp.oci.interfaces.ocirecipe import IOCIRecipe
 from lp.oci.interfaces.ociregistrycredentials import (
     IOCIRegistryCredentialsSet,
     OCIRegistryCredentialsAlreadyExist,
     )
+from lp.oci.model.ocirecipe import OCIRecipe
 from lp.registry.browser import BaseRdfView
 from lp.registry.browser.branding import BrandingChangeView
 from lp.registry.browser.menu import (
@@ -199,7 +204,10 @@ from lp.registry.interfaces.teammembership import (
     )
 from lp.registry.interfaces.wikiname import IWikiNameSet
 from lp.registry.mail.notification import send_direct_contact_email
-from lp.registry.model.person import get_recipients
+from lp.registry.model.person import (
+    get_recipients,
+    Person,
+    )
 from lp.services.config import config
 from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.sqlbase import flush_database_updates
@@ -3669,8 +3677,14 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
 
     @cachedproperty
     def oci_registry_credentials(self):
-        return list(getUtility(
-            IOCIRegistryCredentialsSet).findByOwner(self.context))
+        if IPerson.providedBy(self.context):
+            owner = self.context
+        elif IOCIRecipe.providedBy(self.context):
+            owner = self.context.owner
+        else:
+            raise ValueError("Invalid context for this view")
+
+        return list(getUtility(IOCIRegistryCredentialsSet).findByOwner(owner))
 
     schema = Interface
 
@@ -3883,6 +3897,12 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
             credentials.destroySelf()
 
     def addCredentials(self, parsed_add_credentials):
+        if IPerson.providedBy(self.context):
+            owner = self.context
+        elif IOCIRecipe.providedBy(self.context):
+            owner = self.context.owner
+        else:
+            raise ValueError("Invalid context for this view")
         url = parsed_add_credentials["url"]
         password = parsed_add_credentials["password"]
         confirm_password = parsed_add_credentials["confirm_password"]
@@ -3902,7 +3922,7 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
                     'password': password}
                 try:
                     getUtility(IOCIRegistryCredentialsSet).new(
-                        owner=self.context,
+                        owner=owner,
                         url=url,
                         credentials=credentials)
                 except OCIRegistryCredentialsAlreadyExist:
@@ -3915,7 +3935,7 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
                 credentials = {'username': username}
                 try:
                     getUtility(IOCIRegistryCredentialsSet).new(
-                        owner=self.context,
+                        owner=owner,
                         url=url,
                         credentials=credentials)
                 except OCIRegistryCredentialsAlreadyExist:
diff --git a/lib/lp/security.py b/lib/lp/security.py
index bb87821..696c55d 100644
--- a/lib/lp/security.py
+++ b/lib/lp/security.py
@@ -3571,3 +3571,13 @@ class ViewOCIRegistryCredentials(AuthorizationBase):
 class ViewOCIPushRule(AnonymousAuthorization):
     """Anyone can view an `IOCIPushRule`."""
     usedfor = IOCIPushRule
+
+
+class OCIPushRuleEdit(AuthorizationBase):
+    permission = 'launchpad.Edit'
+    usedfor = IOCIPushRule
+
+    def checkAuthenticated(self, user):
+        return (
+            user.isOwner(self.obj.recipe) or
+            user.in_commercial_admin or user.in_admin)