launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14777
[Merge] lp:~sinzui/launchpad/allow-vcs-imports-to-rename-a-branch into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/allow-vcs-imports-to-rename-a-branch into lp:launchpad.
Commit message:
Allow ~vcs-imports members to rename imported branches.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #588943 in Launchpad itself: "~vcs-imports member renaming an import branch that they do not own oopses"
https://bugs.launchpad.net/launchpad/+bug/588943
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/allow-vcs-imports-to-rename-a-branch/+merge/139270
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
--
https://code.launchpad.net/~sinzui/launchpad/allow-vcs-imports-to-rename-a-branch/+merge/139270
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/allow-vcs-imports-to-rename-a-branch into lp:launchpad.
=== modified file 'lib/lp/code/browser/codeimport.py'
--- lib/lp/code/browser/codeimport.py 2012-11-26 08:40:20 +0000
+++ lib/lp/code/browser/codeimport.py 2012-12-11 17:42:43 +0000
@@ -302,7 +302,7 @@
# If the user can administer branches, then they should be able to
# assign the ownership of the branch to any valid person or team.
- if user_has_special_branch_access(self.user):
+ if user_has_special_branch_access(self.user, self.context):
owner_field = self.schema['owner']
any_owner_choice = Choice(
__name__='owner', title=owner_field.title,
=== 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-11 17:42:43 +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-11 17:42:43 +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-11 17:42:43 +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-11 17:42:43 +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-11 17:42:43 +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):
