Merge lp:~sinzui/launchpad/allow-vcs-imports-to-rename-a-branch into lp:launchpad

[Curtis Hovey]

Allow ~vcs-imports members to rename imported branches.

Members of ~vcs-branches are responsible for moderating imported branches.
An oops can be raised when the member renames an imported branch and the
member is not a member of the owning team. This error is raised even though
the member is not changing the branch owner.

RULES

    Pre-implementation: wgrant
    * The oops is caused by over zealous permission checking that does not
      distinguish between the branch name and the owner name in the branch's
      unique_name.
    * This issue also relates to asymmetric rules for responsibility and
      rules for permission. Permission to change the branch owner is given
      to ~admins and ~bazaar-experts, and most member of ~vcs-imports are
      also members of ~bazaar-experts. But ~bazaar-experts are not
      responsible for managing imports.
      * ~bazaar-experts were removed from user_has_special_branch_access().
      * Code that checks ownership or team membership must recognise that
        ~vcs-imports members has full control of all import branches.
      * The branch edit security checker has a complex check for the
        ~vcs-imports cases that needs to be available to model code
        to check a specific user.
      * user_has_special_branch_access() can do so again if the security
        checker code were moved into it.

QA

    * Visit https://code.qastaging.launchpad.net/~lfaraone/autokey/trunk/+edit
    * Verify the owner field uses the picker instead of a select list.
    * Change the name from "trunk" to "main" and submit.
    * Verify the change is accepted.

LINT

    lib/lp/security.py
    lib/lp/code/browser/codeimport.py
    lib/lp/code/interfaces/branch.py
    lib/lp/code/interfaces/branchnamespace.py
    lib/lp/code/model/branchnamespace.py
    lib/lp/code/model/tests/test_branchnamespace.py

LoC

    I have more than 10,000 lines of credit this week.

TEST

    ./bin/test -vc -t validateMove -t validateRegistrant lp.code.model.tests
    ./bin/test -vc -t 'test_branch\..*Edit' lp.code.browser.tests

IMPLEMENTATION

I moved the rules to check if the case is a ~vcs-imports member working
with a code import branch into user_has_special_branch_access().
    lib/lp/security.py
    lib/lp/code/interfaces/branchnamespace.py

I updated validateRegistrant() to accept an optional branch argument that
is in turn passed to user_has_special_branch_access. ~vcs members can
rename an imported branch and change the owner without doing arranging for
help.
    lib/lp/code/interfaces/branch.py
    lib/lp/code/model/branchnamespace.py
    lib/lp/code/model/tests/test_branchnamespace.py

I passed the branch to user_has_special_branch_access() so that vcs-import
members also permitted to reassign imported branch to anyone. This is a big
help to cases where we are asked to give the project and branch to someone
claiming a project.
    lib/lp/code/browser/codeimport.py

[Richard Harding (rharding)]

review: Approve

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2012-11-29 05:52:36 +0000
+++ lib/lp/code/interfaces/branch.py	2012-12-12 00:14:31 +0000
@@ -70,7 +70,6 @@
 
 from lp import _
 from lp.app.enums import InformationType
-from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.validators import LaunchpadValidationError
 from lp.code.bzr import (
     BranchFormat,
@@ -92,6 +91,7 @@
 from lp.code.interfaces.hasrecipes import IHasRecipes
 from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
 from lp.registry.interfaces.person import IPerson
+from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.role import IHasOwner
 from lp.services.config import config
@@ -1529,15 +1529,26 @@
         return sorted(links)
 
 
-def user_has_special_branch_access(user):
-    """Admins and bazaar experts have special access.
+def user_has_special_branch_access(user, branch=None):
+    """Admins and vcs-import members have have special access.
 
     :param user: A 'Person' or None.
+    :param branch: A branch or None when checking collection access.
     """
     if user is None:
         return False
-    celebs = getUtility(ILaunchpadCelebrities)
-    return user.inTeam(celebs.admin)
+    roles = IPersonRoles(user)
+    if roles.in_admin:
+        return True
+    if branch is None:
+        return False
+    code_import = branch.code_import
+    if code_import is None:
+        return False
+    return (
+        roles.in_vcs_imports
+        or (IPersonRoles(branch.owner).in_vcs_imports
+            and user.inTeam(code_import.registrant)))
 
 
 def get_db_branch_info(stacked_on_url, last_revision_id, control_string,

=== modified file 'lib/lp/code/interfaces/branchnamespace.py'
--- lib/lp/code/interfaces/branchnamespace.py	2012-09-21 04:04:51 +0000
+++ lib/lp/code/interfaces/branchnamespace.py	2012-12-12 00:14:31 +0000
@@ -124,10 +124,12 @@
         :return: An `InformationType`.
         """
 
-    def validateRegistrant(registrant):
+    def validateRegistrant(registrant, branch=None):
         """Check that the registrant can create a branch on this namespace.
 
         :param registrant: An `IPerson`.
+        :param branch: An optional `IBranch` to also check when working
+            with imported branches.
         :raises BranchCreatorNotMemberOfOwnerTeam: if the namespace owner is
             a team, and the registrant is not in that team.
         :raises BranchCreatorNotOwner: if the namespace owner is an individual

=== modified file 'lib/lp/code/model/branchnamespace.py'
--- lib/lp/code/model/branchnamespace.py	2012-10-11 08:09:12 +0000
+++ lib/lp/code/model/branchnamespace.py	2012-12-12 00:14:31 +0000
@@ -176,9 +176,9 @@
         notify(ObjectCreatedEvent(branch))
         return branch
 
-    def validateRegistrant(self, registrant):
+    def validateRegistrant(self, registrant, branch=None):
         """See `IBranchNamespace`."""
-        if user_has_special_branch_access(registrant):
+        if user_has_special_branch_access(registrant, branch):
             return
         owner = self.owner
         if not registrant.inTeam(owner):
@@ -213,7 +213,7 @@
         if name is None:
             name = branch.name
         self.validateBranchName(name)
-        self.validateRegistrant(mover)
+        self.validateRegistrant(mover, branch)
 
     def moveBranch(self, branch, mover, new_name=None,
                    rename_if_necessary=False):

=== modified file 'lib/lp/code/model/tests/test_branchnamespace.py'
--- lib/lp/code/model/tests/test_branchnamespace.py	2012-10-11 08:09:12 +0000
+++ lib/lp/code/model/tests/test_branchnamespace.py	2012-12-12 00:14:31 +0000
@@ -54,6 +54,7 @@
 from lp.registry.interfaces.product import NoSuchProduct
 from lp.registry.model.sourcepackage import SourcePackage
 from lp.testing import (
+    celebrity_logged_in,
     person_logged_in,
     TestCaseWithFactory,
     )
@@ -347,6 +348,32 @@
         namespace = ProductNamespace(person, product)
         self.assertEqual(IBranchTarget(product), namespace.target)
 
+    def test_validateMove_vcs_imports_rename_import_branch(self):
+        # Members of ~vcs-imports can rename any imported branch.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        name = self.factory.getUniqueString()
+        code_import = self.factory.makeCodeImport(
+            registrant=owner, target=IBranchTarget(product), branch_name=name)
+        branch = code_import.branch
+        new_name = self.factory.getUniqueString()
+        namespace = ProductNamespace(owner, product)
+        with celebrity_logged_in('vcs_imports') as mover:
+            self.assertIsNone(
+                namespace.validateMove(branch, mover, name=new_name))
+
+    def test_validateMove_vcs_imports_change_owner_import_branch(self):
+        # Members of ~vcs-imports can change the owner any imported branch.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        code_import = self.factory.makeCodeImport(
+            registrant=owner, target=IBranchTarget(product))
+        branch = code_import.branch
+        new_owner = self.factory.makePerson()
+        new_namespace = ProductNamespace(new_owner, product)
+        with celebrity_logged_in('vcs_imports') as mover:
+            self.assertIsNone(new_namespace.validateMove(branch, mover))
+
 
 class TestProductNamespacePrivacyWithInformationType(TestCaseWithFactory):
     """Tests for the privacy aspects of `ProductNamespace`.

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-12-07 14:30:26 +0000
+++ lib/lp/security.py	2012-12-12 00:14:31 +0000
@@ -30,7 +30,6 @@
 from lp.answers.interfaces.questionmessage import IQuestionMessage
 from lp.answers.interfaces.questionsperson import IQuestionsPerson
 from lp.answers.interfaces.questiontarget import IQuestionTarget
-from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.interfaces.security import IAuthorization
 from lp.app.interfaces.services import IService
 from lp.app.security import (
@@ -2211,25 +2210,10 @@
     usedfor = IBranch
 
     def checkAuthenticated(self, user):
-        can_edit = (
+        return (
             user.inTeam(self.obj.owner) or
-            user_has_special_branch_access(user.person) or
+            user_has_special_branch_access(user.person, self.obj) or
             can_upload_linked_package(user, self.obj))
-        if can_edit:
-            return True
-        # It used to be the case that all import branches were owned by the
-        # special, restricted team ~vcs-imports. For these legacy code import
-        # branches, we still want the code import registrant to be able to
-        # edit them. Similarly, we still want vcs-imports members to be able
-        # to edit those branches.
-        code_import = self.obj.code_import
-        if code_import is None:
-            return False
-        vcs_imports = getUtility(ILaunchpadCelebrities).vcs_imports
-        return (
-            user.in_vcs_imports
-            or (self.obj.owner == vcs_imports
-                and user.inTeam(code_import.registrant)))
 
 
 class ModerateBranch(EditBranch):