launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/no-edit-import-for-you into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1059324 in Launchpad itself: "Can't edit branch import"
  https://bugs.launchpad.net/launchpad/+bug/1059324

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/no-edit-import-for-you/+merge/127417

Raise Unauthorized if the user isn't an admin or in vcs imports on IBranch:+edit-import. Due to the ZCML, the branch owner is allowed to see the view, but will get an OOPS on submit. Cut this off at the pass by raising Unauthorized in the view's initialize method.

Clean up for net-negative LoC.
-- 
https://code.launchpad.net/~stevenk/launchpad/no-edit-import-for-you/+merge/127417
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/no-edit-import-for-you into lp:launchpad.

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2012-09-28 06:15:58 +0000
+++ lib/lp/code/browser/branch.py	2012-10-02 05:19:21 +0000
@@ -381,18 +381,14 @@
 
     def edit_import(self):
         text = 'Edit import source or review import'
-        enabled = True
         enabled = (
             self.context.branch_type == BranchType.IMPORTED and
             check_permission('launchpad.Edit', self.context.code_import))
-        return Link(
-            '+edit-import', text, icon='edit', enabled=enabled)
+        return Link('+edit-import', text, icon='edit', enabled=enabled)
 
     @enabled_with_permission('launchpad.Edit')
     def upgrade_branch(self):
-        enabled = False
-        if self.context.needs_upgrading:
-            enabled = True
+        enabled = self.context.needs_upgrading
         return Link(
             '+upgrade', 'Upgrade this branch', icon='edit', enabled=enabled)
 

=== modified file 'lib/lp/code/browser/codeimport.py'
--- lib/lp/code/browser/codeimport.py	2012-07-06 21:25:10 +0000
+++ lib/lp/code/browser/codeimport.py	2012-10-02 05:19:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser views for CodeImports."""
@@ -27,6 +27,7 @@
 from zope.formlib import form
 from zope.interface import Interface
 from zope.schema import Choice
+from zope.security.interfaces import Unauthorized
 
 from lp import _
 from lp.app.browser.launchpadform import (
@@ -35,7 +36,6 @@
     LaunchpadFormView,
     )
 from lp.app.errors import NotFoundError
-from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.widgets.itemswidgets import (
     LaunchpadDropdownWidget,
     LaunchpadRadioWidget,
@@ -67,6 +67,7 @@
     )
 from lp.code.interfaces.codeimportmachine import ICodeImportMachineSet
 from lp.registry.interfaces.product import IProduct
+from lp.registry.interfaces.role import IPersonRoles
 from lp.services.fields import URIField
 from lp.services.propertycache import cachedproperty
 from lp.services.webapp import (
@@ -155,9 +156,8 @@
     @cachedproperty
     def _super_user(self):
         """Is the user an admin or member of vcs-imports?"""
-        celebs = getUtility(ILaunchpadCelebrities)
-        return (self.user.inTeam(celebs.admin) or
-                self.user.inTeam(celebs.vcs_imports))
+        role = IPersonRoles(self.user)
+        return role.in_admin or role.in_vcs_imports
 
     def showOptionalMarker(self, field_name):
         """Don't show the optional marker for rcs locations."""
@@ -217,9 +217,7 @@
     """The fields presented on the form for editing a code import."""
 
     use_template(IBranch, ['owner'])
-    use_template(
-        ICodeImport,
-        ['rcs_type', 'cvs_root', 'cvs_module'])
+    use_template(ICodeImport, ['rcs_type', 'cvs_root', 'cvs_module'])
 
     svn_branch_url = URIField(
         title=_("Branch URL"), required=False,
@@ -306,10 +304,10 @@
 
     @property
     def label(self):
+        label = 'Request a code import'
         if self.context_is_product:
-            return 'Request a code import for %s' % self.context.displayname
-        else:
-            return 'Request a code import'
+            label += ' for %s' % self.context.displayname
+        return label
 
     @property
     def cancel_url(self):
@@ -338,7 +336,7 @@
             self.form_fields = any_owner_field + self.form_fields
 
     def setUpWidgets(self):
-        CodeImportBaseView.setUpWidgets(self)
+        super(CodeImportNewView, self).setUpWidgets()
 
         # Extract the radio buttons from the rcs_type widget, so we can
         # display them separately in the form.
@@ -542,9 +540,11 @@
         self.code_import = self.context.code_import
         if self.code_import is None:
             raise NotFoundError
+        if not self._super_user:
+            raise Unauthorized
         # The next and cancel location is the branch details page.
         self.cancel_url = self.next_url = canonical_url(self.context)
-        CodeImportBaseView.initialize(self)
+        super(CodeImportEditView, self).initialize()
 
     @property
     def adapters(self):

=== modified file 'lib/lp/code/browser/tests/test_codeimport.py'
--- lib/lp/code/browser/tests/test_codeimport.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/browser/tests/test_codeimport.py	2012-10-02 05:19:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 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."""
@@ -7,6 +7,8 @@
 
 import re
 
+from zope.security.interfaces import Unauthorized
+
 from lp.code.enums import RevisionControlSystems
 from lp.services.webapp import canonical_url
 from lp.testing import TestCaseWithFactory
@@ -53,3 +55,9 @@
         svn_details = find_tag_by_id(browser.contents, 'svn-import-details')
         self.assertSvnDetailsDisplayed(
             svn_details, RevisionControlSystems.SVN)
+
+    def test_branch_owner_of_import_forbidden(self):
+        cimport = self.factory.makeCodeImport()
+        url = canonical_url(cimport.branch, view_name='+edit-import')
+        self.assertRaises(
+            Unauthorized, self.getUserBrowser(url, user=cimport.branch.owner))

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2012-09-26 07:59:35 +0000
+++ lib/lp/code/model/branch.py	2012-10-02 05:19:21 +0000
@@ -1,8 +1,6 @@
 # Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=E0611,W0212,W0141,F0401
-
 __metaclass__ = type
 __all__ = [
     'Branch',

=== modified file 'lib/lp/code/model/codeimport.py'
--- lib/lp/code/model/codeimport.py	2011-12-30 06:14:56 +0000
+++ lib/lp/code/model/codeimport.py	2012-10-02 05:19:21 +0000
@@ -1,8 +1,6 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=E0611,W0212
-
 """Database classes including and related to CodeImport."""
 
 __metaclass__ = type
@@ -148,13 +146,6 @@
         if job is not None:
             if job.state == CodeImportJobState.PENDING:
                 CodeImportJobWorkflow().deletePendingJob(self)
-            else:
-                # When we have job killing, we might want to kill a running
-                # job.
-                pass
-        else:
-            # No job, so nothing to do.
-            pass
 
     results = SQLMultipleJoin(
         'CodeImportResult', joinColumn='code_import',
@@ -239,7 +230,6 @@
         else:
             getUtility(ICodeImportJobWorkflow).requestJob(
                 self.import_job, requester)
-        return None
 
 
 class CodeImportSet: