launchpad-reviewers team mailing list archive

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

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:allow-edit-codeimport into launchpad:master.

Commit message:
Allow users to edit their own code imports.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

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.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:allow-edit-codeimport into launchpad:master.

diff --git a/lib/lp/code/browser/codeimport.py b/lib/lp/code/browser/codeimport.py
index 635347d..9afd60c 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,9 @@ class CodeImportBaseView(LaunchpadFormView):
     custom_widget_url = CustomWidgetFactory(URIWidget, displayWidth=50)
 
     @cachedproperty
-    def _super_user(self):
+    def _is_allowed_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
+        return check_permission("launchpad.Edit", self.code_import)
 
     def showOptionalMarker(self, field_name):
         """Don't show the optional marker for rcs locations."""
@@ -600,7 +599,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_allowed_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 +628,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_allowed_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..b7775fc 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."""
@@ -72,10 +72,23 @@ 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(cimport.url, 'http://foo.test')
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:password@xxxxxxxxxxxxxxx/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:password@xxxxxxxxxxxxxxx/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:anonymous@xxxxxxxxxxxxxxx:/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..e05c9b2 100644
--- a/lib/lp/security.py
+++ b/lib/lp/security.py
@@ -1574,11 +1574,21 @@ class OnlyVcsImportsAndAdmins(AuthorizationBase):
         return user.in_admin or user.in_vcs_imports
 
 
-class EditCodeImport(OnlyVcsImportsAndAdmins):
+class OwnerAndVcsImportsAndAdmins(AuthorizationBase):
+    """Base class that allows object owner, Launchpad admins and VCS Imports
+    experts."""
+
+    def checkAuthenticated(self, user):
+        return (user.inTeam(self.obj.owner) or
+                user.in_admin or
+                user.in_vcs_imports)
+
+
+class EditCodeImport(OwnerAndVcsImportsAndAdmins):
     """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