Merge ~pappacena/launchpad:allow-edit-codeimport into launchpad:master

[Thiago F. Pappacena]

Allow users to edit their own code imports.

As we can see by the question https://answers.launchpad.net/launchpad/+question/690961, users cannot edit their own code imports. The only way to change it today is deleting the old repository, and editing the project to create a new code import. This is a long and non-intuitive path for users.

[Colin Watson (cjwatson)]

This is basically fine by me, but I'd suggest first checking with William for more details on his IRC comment on Tuesday:

  <wgrant> With old-style imports it required extreme care. But only CVS still uses that system.

... and whether we need to do anything about that here.

review: Approve

33bd03e... by Thiago F. Pappacena on 2020-06-01

Inlining security check for launchpad.Edit on ICodeImport

9c46f4f... by Thiago F. Pappacena on 2020-06-03

Adding launchpad.Moderate permission for code imports

By adding this new permission, we move what is currently launchpad.Edit
to the new launchpad.Moderate permission, and allow for launchpad.Edit
only the permission to set the code import URL. This way, users can
change their code import url, but not manage the code import status.

[Thiago F. Pappacena (pappacena)]

cjwatson, after a quick discussion with wgrant, I have pushed some changes here.

I'll quote wgrant's message on IRC, which is a good explanation of what I've done in the latest commit:

"I think the URL thing is fine, but we should ensure they don't have access to the suspend bits, or the whiteboard if that still exists, etc. It might make sense to move everything that is lp.Edit to lp.Moderate, then move the URL and any other targeted bits to lp.Edit."

Since I have changed quite a bit, it would be interesting to have another review round.

[Tom Wardill (twom)]

review: Approve

diff --git a/lib/lp/code/browser/codeimport.py b/lib/lp/code/browser/codeimport.py
index 635347d..8516afe 100644
--- a/lib/lp/code/browser/codeimport.py
+++ b/lib/lp/code/browser/codeimport.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser views for CodeImports."""
@@ -88,7 +88,6 @@ from lp.code.interfaces.gitnamespace import (
     IGitNamespacePolicy,
     )
 from lp.registry.interfaces.product import IProduct
-from lp.registry.interfaces.role import IPersonRoles
 from lp.services.beautifulsoup import BeautifulSoup
 from lp.services.fields import URIField
 from lp.services.propertycache import cachedproperty
@@ -98,6 +97,7 @@ from lp.services.webapp import (
     Navigation,
     stepto,
     )
+from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.batching import BatchNavigator
 from lp.services.webapp.breadcrumb import Breadcrumb
 from lp.services.webapp.escaping import structured
@@ -189,10 +189,14 @@ class CodeImportBaseView(LaunchpadFormView):
     custom_widget_url = CustomWidgetFactory(URIWidget, displayWidth=50)
 
     @cachedproperty
-    def _super_user(self):
-        """Is the user an admin or member of vcs-imports?"""
-        role = IPersonRoles(self.user)
-        return role.in_admin or role.in_vcs_imports
+    def _is_edit_user(self):
+        """Can this user edit specific fields?"""
+        return check_permission("launchpad.Edit", self.code_import)
+
+    @cachedproperty
+    def _is_moderator_user(self):
+        """Is a moderator of code imports?"""
+        return check_permission("launchpad.Moderate", self.code_import)
 
     def showOptionalMarker(self, field_name):
         """Don't show the optional marker for rcs locations."""
@@ -555,12 +559,22 @@ def _makeEditAction(label, status, text):
 
     def success(self, action, data):
         """Make the requested status change."""
-        if status is not None:
-            data['review_status'] = status
-        event = self.code_import.updateFromData(data, self.user)
-        if event is not None:
-            self.request.response.addNotification(
-                'The code import has been ' + text + '.')
+        if self._is_moderator_user:
+            # Moderators can change everything in code import, including its
+            # status.
+            if status is not None:
+                data['review_status'] = status
+            event = self.code_import.updateFromData(data, self.user)
+            if event is not None:
+                self.request.response.addNotification(
+                    'The code import has been ' + text + '.')
+        elif self._is_edit_user and "url" in data:
+            # Edit users can only change URL
+            new_url = data["url"]
+            if self.code_import.url != new_url:
+                self.code_import.url = new_url
+                self.request.response.addNotification(
+                    'The code import URL has been updated.')
         else:
             self.request.response.addNotification('No changes made.')
     name = label.lower().replace(' ', '_')
@@ -600,7 +614,7 @@ class CodeImportEditView(CodeImportBaseView):
         self.code_import = self.context.code_import
         if self.code_import is None:
             raise NotFoundError
-        if not self._super_user:
+        if not self._is_edit_user and not self._is_moderator_user:
             raise Unauthorized
         # The next and cancel location is the target details page.
         self.cancel_url = self.next_url = canonical_url(self.context)
@@ -629,7 +643,8 @@ class CodeImportEditView(CodeImportBaseView):
 
     def _showButtonForStatus(self, status):
         """If the status is different, and the user is super, show button."""
-        return self._super_user and self.code_import.review_status != status
+        return (self._is_moderator_user and
+                self.code_import.review_status != status)
 
     actions = form.Actions(
         _makeEditAction(_('Update'), None, 'updated'),
diff --git a/lib/lp/code/browser/tests/test_codeimport.py b/lib/lp/code/browser/tests/test_codeimport.py
index 97f1799..68b8b2e 100644
--- a/lib/lp/code/browser/tests/test_codeimport.py
+++ b/lib/lp/code/browser/tests/test_codeimport.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the code import browser code."""
@@ -13,12 +13,14 @@ from testtools.matchers import StartsWith
 from zope.security.interfaces import Unauthorized
 
 from lp.code.enums import (
+    CodeImportReviewStatus,
     RevisionControlSystems,
     TargetRevisionControlSystems,
     )
 from lp.code.tests.helpers import GitHostingFixture
 from lp.services.webapp import canonical_url
 from lp.testing import (
+    admin_logged_in,
     person_logged_in,
     TestCaseWithFactory,
     )
@@ -72,10 +74,50 @@ class TestImportDetails(TestCaseWithFactory):
             code_import, 'git-import-details',
             'This repository is an import of the Git repository')
 
-    def test_branch_owner_of_import_forbidden(self):
+    def test_other_users_are_forbidden_to_change_codeimport(self):
         # Unauthorized users are forbidden to edit an import.
         cimport = self.factory.makeCodeImport()
-        with person_logged_in(cimport.branch.owner):
+        another_person = self.factory.makePerson()
+        with person_logged_in(another_person):
             self.assertRaises(
                 Unauthorized, create_initialized_view, cimport.branch,
                 '+edit-import')
+
+    def test_branch_owner_of_import_can_edit_it(self):
+        # Owners are allowed to edit code import.
+        cimport = self.factory.makeCodeImport()
+        with person_logged_in(cimport.branch.owner):
+            view = create_initialized_view(
+                cimport.branch, '+edit-import', form={
+                    "field.actions.update": "update",
+                    "field.url": "http://foo.test"
+                })
+            self.assertEqual([], view.errors)
+            self.assertEqual('http://foo.test', cimport.url)
+
+    def test_branch_owner_of_import_cannot_change_status(self):
+        # Owners are allowed to edit code import.
+        cimport = self.factory.makeCodeImport()
+        original_url = cimport.url
+        with person_logged_in(cimport.branch.owner):
+            view = create_initialized_view(
+                cimport.branch, '+edit-import', form={
+                    "field.actions.suspend": "Suspend",
+                    "field.url": "http://foo.test"
+                })
+            self.assertEqual([], view.errors)
+            self.assertEqual(original_url, cimport.url)
+
+    def test_admin_can_change_code_import_status(self):
+        # Owners are allowed to edit code import.
+        cimport = self.factory.makeCodeImport()
+        with admin_logged_in():
+            view = create_initialized_view(
+                cimport.branch, '+edit-import', form={
+                    "field.actions.suspend": "Suspend",
+                    "field.url": "http://foo.test"
+                })
+            self.assertEqual([], view.errors)
+            self.assertEqual("http://foo.test", cimport.url)
+            self.assertEqual(
+                CodeImportReviewStatus.SUSPENDED, cimport.review_status)
diff --git a/lib/lp/code/configure.zcml b/lib/lp/code/configure.zcml
index ce409aa..09d5011 100644
--- a/lib/lp/code/configure.zcml
+++ b/lib/lp/code/configure.zcml
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
+<!-- Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
      GNU Affero General Public License version 3 (see the file LICENSE).
 -->
 
@@ -657,11 +657,14 @@
                        consecutive_failure_count
                        getImportDetailsForDisplay"/>
     <require
+       permission="launchpad.Edit"
+       set_attributes="url"/>
+    <require
        permission="launchpad.AnyPerson"
        attributes="tryFailingImportAgain
                    requestImport"/>
     <require
-       permission="launchpad.Edit"
+       permission="launchpad.Moderate"
        attributes="updateFromData"/>
 
     <!-- ICodeImport has no set_schema, because all modifications should be
diff --git a/lib/lp/code/stories/codeimport/xx-create-codeimport.txt b/lib/lp/code/stories/codeimport/xx-create-codeimport.txt
index c8cb7a1..99dda4a 100644
--- a/lib/lp/code/stories/codeimport/xx-create-codeimport.txt
+++ b/lib/lp/code/stories/codeimport/xx-create-codeimport.txt
@@ -83,6 +83,7 @@ When the user clicks continue, the import branch is created
     at http://bzr.example.com/firefox/trunk.
     The next import is scheduled to run
     as soon as possible.
+    Edit import source or review import
     >>> browser.getLink("http://bzr.example.com/firefox/trunk")
     <Link text='http://bzr.example.com/firefox/trunk'
            url='http://bzr.example.com/firefox/trunk'>
@@ -102,6 +103,7 @@ URL.
     at http://user:[e-mail]/firefox/trunk.
     The next import is scheduled to run
     as soon as possible.
+    Edit import source or review import
 
 Specifying a Launchpad URL results in an error.
 
@@ -130,6 +132,7 @@ But a Launchpad Git URL is OK.
     This branch is an import of the HEAD branch of the Git repository at
     git://git.launchpad.net/firefox.git.
     The next import is scheduled to run as soon as possible.
+    Edit import source or review import
 
 Requesting a Subversion import
 ==============================
@@ -153,6 +156,7 @@ When the user clicks continue, the import branch is created
     from http://svn.example.com/firefox/trunk.
     The next import is scheduled to run
     as soon as possible.
+    Edit import source or review import
     >>> browser.getLink("http://svn.example.com/firefox/trunk")
     <Link text='http://svn.example.com/firefox/trunk'
            url='http://svn.example.com/firefox/trunk'>
@@ -182,6 +186,7 @@ URL.
     from http://user:[e-mail]/firefox/trunk.
     The next import is scheduled to run
     as soon as possible.
+    Edit import source or review import
 
 
 Requesting a Git-to-Bazaar import
@@ -205,6 +210,7 @@ When the user clicks continue, the approved import branch is created.
     This branch is an import of the HEAD branch of the Git repository at
     git://example.com/firefox.git.
     The next import is scheduled to run as soon as possible.
+    Edit import source or review import
 
 
 Requesting a Git-to-Git import
@@ -233,6 +239,7 @@ When the user clicks continue, the approved import repository is created.
     This repository is an import of the Git repository at
     git://example.com/firefox.git.
     The next import is scheduled to run as soon as possible.
+    Edit import source or review import
 
 
 Requesting a CVS import
@@ -256,6 +263,7 @@ to identify the CVS branch.  A project and branch name are also required.
     :pserver:[e-mail]:/mozilla/cvs.
     The next import is scheduled to run
     as soon as possible.
+    Edit import source or review import
 
 Requesting a CVS import with invalid information
 ================================================
diff --git a/lib/lp/code/stories/codeimport/xx-edit-codeimport.txt b/lib/lp/code/stories/codeimport/xx-edit-codeimport.txt
index 2f65697..c7aebb5 100644
--- a/lib/lp/code/stories/codeimport/xx-edit-codeimport.txt
+++ b/lib/lp/code/stories/codeimport/xx-edit-codeimport.txt
@@ -76,8 +76,7 @@ and a 404 if the branch doesn't have an import.
 
     >>> import_browser.open(svn_import_location)
     >>> import_browser.getLink('Edit import source or review import')
-    Traceback (most recent call last):
-    LinkNotFoundError
+    <Link text='Edit import source or review import' url='.../+edit-import'>
 
 
 == Import details for a import that has been imported successfully ==
diff --git a/lib/lp/security.py b/lib/lp/security.py
index f7d7bf8..005c52f 100644
--- a/lib/lp/security.py
+++ b/lib/lp/security.py
@@ -1574,15 +1574,30 @@ class OnlyVcsImportsAndAdmins(AuthorizationBase):
         return user.in_admin or user.in_vcs_imports
 
 
-class EditCodeImport(OnlyVcsImportsAndAdmins):
+class EditCodeImport(AuthorizationBase):
     """Control who can edit the object view of a CodeImport.
 
     Currently, we restrict the visibility of the new code import
-    system to members of ~vcs-imports and Launchpad admins.
+    system to owners, members of ~vcs-imports and Launchpad admins.
     """
     permission = 'launchpad.Edit'
     usedfor = ICodeImport
 
+    def checkAuthenticated(self, user):
+        return (user.inTeam(self.obj.owner) or
+                user.in_admin or
+                user.in_vcs_imports)
+
+
+class ModerateCodeImport(OnlyVcsImportsAndAdmins):
+    """Control who can moderate a CodeImport.
+
+    Currently, we restrict the visibility of code import moderation
+    system to members of ~vcs-imports and Launchpad admins.
+    """
+    permission = 'launchpad.Moderate'
+    usedfor = ICodeImport
+
 
 class SeeCodeImportJobSet(OnlyVcsImportsAndAdmins):
     """Control who can see the CodeImportJobSet utility.