launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #12291
lp:~wallyworld/launchpad/branch-change-information-type-1052639 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/branch-change-information-type-1052639 into lp:launchpad.
Commit message:
When determining the allowed information types for a product branch, add an extra check to see if the caller has access to the product in addition to the check for the branch owner.
Requested reviews:
Curtis Hovey (sinzui)
Related bugs:
Bug #1052639 in Launchpad itself: "Cannot change the information type of a branch when I unshare with the owner"
https://bugs.launchpad.net/launchpad/+bug/1052639
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/branch-change-information-type-1052639/+merge/125625
== Implementation ==
When determining the allowed information types for a product branch, add an extra check to see if the caller has access to the product in addition to the check for the branch owner.
This change allows the product owner to change the information type of a branch after they have unshared with the branch owner. Before this change, the branch owner not having access to the product would have prevented an authorised person from changing the information type.
== Tests ==
Add some extra tests to test_branchnamespace.TestProductNamespacePrivacyWithInformationType
- test_proprietary_or_public_caller_grantee
- test_proprietary_caller_grantee
- test_embargoed_or_proprietary_caller_grantee
Rename some tests also since they were named inconsistently.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/browser/branchlisting.py
lib/lp/code/interfaces/branch.py
lib/lp/code/interfaces/branchnamespace.py
lib/lp/code/model/branch.py
lib/lp/code/model/branchnamespace.py
lib/lp/code/model/tests/test_branchnamespace.py
--
https://code.launchpad.net/~wallyworld/launchpad/branch-change-information-type-1052639/+merge/125625
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py 2012-09-18 18:36:09 +0000
+++ lib/lp/code/browser/branchlisting.py 2012-09-21 04:16:20 +0000
@@ -759,7 +759,8 @@
if target is None:
return False
namespace = target.getNamespace(self.user)
- return IBranchNamespacePolicy(namespace).getDefaultInformationType()
+ policy = IBranchNamespacePolicy(namespace)
+ return policy.getDefaultInformationType(self.user)
@property
def default_information_type_title(self):
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2012-09-18 18:36:09 +0000
+++ lib/lp/code/interfaces/branch.py 2012-09-21 04:16:20 +0000
@@ -980,7 +980,7 @@
def visibleByUser(user):
"""Can the specified user see this branch?"""
- def getAllowedInformationTypes(user):
+ def getAllowedInformationTypes(who):
"""Get a list of acceptable `InformationType`s for this branch.
If the user is a Launchpad admin, any type is acceptable. Otherwise
=== modified file 'lib/lp/code/interfaces/branchnamespace.py'
--- lib/lp/code/interfaces/branchnamespace.py 2012-07-12 04:36:41 +0000
+++ lib/lp/code/interfaces/branchnamespace.py 2012-09-21 04:16:20 +0000
@@ -110,15 +110,17 @@
:return: A Boolean value.
"""
- def getAllowedInformationTypes():
+ def getAllowedInformationTypes(who):
"""Get the information types that a branch in this namespace can have.
+ :param who: The user making the request.
:return: A sequence of `InformationType`s.
"""
- def getDefaultInformationType():
+ def getDefaultInformationType(who):
"""Get the default information type for branches in this namespace.
+ :param who: The user for whom to return the information type.
:return: An `InformationType`.
"""
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2012-09-19 13:22:42 +0000
+++ lib/lp/code/model/branch.py 2012-09-21 04:16:20 +0000
@@ -253,7 +253,7 @@
else:
# Otherwise the permitted types are defined by the namespace.
policy = IBranchNamespacePolicy(self.namespace)
- types = set(policy.getAllowedInformationTypes())
+ types = set(policy.getAllowedInformationTypes(who))
return types
def transitionToInformationType(self, information_type, who,
@@ -370,7 +370,8 @@
raise BranchTargetError(
'Only private teams may have personal private branches.')
namespace = target.getNamespace(self.owner)
- if self.information_type not in namespace.getAllowedInformationTypes():
+ if (self.information_type not in
+ namespace.getAllowedInformationTypes(user)):
raise BranchTargetError(
'%s branches are not allowed for target %s.' % (
self.information_type.title, target.displayname))
=== modified file 'lib/lp/code/model/branchnamespace.py'
--- lib/lp/code/model/branchnamespace.py 2012-09-21 00:28:49 +0000
+++ lib/lp/code/model/branchnamespace.py 2012-09-21 04:16:20 +0000
@@ -147,7 +147,7 @@
distroseries = sourcepackage.distroseries
sourcepackagename = sourcepackage.sourcepackagename
- information_type = self.getDefaultInformationType()
+ information_type = self.getDefaultInformationType(registrant)
if information_type is None:
raise BranchCreationForbidden()
@@ -205,7 +205,7 @@
"%s cannot create branches owned by %s"
% (registrant.displayname, owner.displayname))
- if not self.getAllowedInformationTypes():
+ if not self.getAllowedInformationTypes(registrant):
raise BranchCreationForbidden(
'You cannot create branches in "%s"' % self.name)
@@ -298,13 +298,13 @@
else:
return True
- def getAllowedInformationTypes(self):
+ def getAllowedInformationTypes(self, who=None):
"""See `IBranchNamespace`."""
raise NotImplementedError
- def getDefaultInformationType(self):
+ def getDefaultInformationType(self, who=None):
"""See `IBranchNamespace`."""
- if InformationType.USERDATA in self.getAllowedInformationTypes():
+ if InformationType.USERDATA in self.getAllowedInformationTypes(who):
return InformationType.USERDATA
return InformationType.PUBLIC
@@ -334,7 +334,7 @@
"""See `IBranchNamespace`."""
return '~%s/+junk' % self.owner.name
- def getAllowedInformationTypes(self):
+ def getAllowedInformationTypes(self, who=None):
"""See `IBranchNamespace`."""
# Private teams get private branches, everyone else gets public ones.
if (self.owner.is_team
@@ -418,20 +418,23 @@
else:
return None
- def getAllowedInformationTypes(self):
+ def getAllowedInformationTypes(self, who=None):
"""See `IBranchNamespace`."""
if not self._using_branchvisibilitypolicy:
# The project uses the new simplified branch_sharing_policy
# rules, so check them.
- # Some policies require that the owner have full access to
- # an information type. If it's required and the owner
+ # Some policies require that the branch owner or current user have
+ # full access to an information type. If it's required and the user
# doesn't hold it, no information types are legal.
required_grant = BRANCH_POLICY_REQUIRED_GRANTS[
self.product.branch_sharing_policy]
if (required_grant is not None
and not getUtility(IService, 'sharing').checkPillarAccess(
- self.product, required_grant, self.owner)):
+ self.product, required_grant, self.owner)
+ and (who is None
+ or not getUtility(IService, 'sharing').checkPillarAccess(
+ self.product, required_grant, who))):
return []
return BRANCH_POLICY_ALLOWED_TYPES[
@@ -470,16 +473,16 @@
types.extend(FREE_PRIVATE_INFORMATION_TYPES)
return types
- def getDefaultInformationType(self):
+ def getDefaultInformationType(self, who=None):
"""See `IBranchNamespace`."""
if not self._using_branchvisibilitypolicy:
default_type = BRANCH_POLICY_DEFAULT_TYPES[
self.product.branch_sharing_policy]
- if default_type not in self.getAllowedInformationTypes():
+ if default_type not in self.getAllowedInformationTypes(who):
return None
return default_type
- return super(ProductNamespace, self).getDefaultInformationType()
+ return super(ProductNamespace, self).getDefaultInformationType(who)
class PackageNamespace(_BaseNamespace):
@@ -511,7 +514,7 @@
"""See `IBranchNamespace`."""
return IBranchTarget(self.sourcepackage)
- def getAllowedInformationTypes(self):
+ def getAllowedInformationTypes(self, who=None):
"""See `IBranchNamespace`."""
return PUBLIC_INFORMATION_TYPES
=== modified file 'lib/lp/code/model/tests/test_branchnamespace.py'
--- lib/lp/code/model/tests/test_branchnamespace.py 2012-09-21 00:28:49 +0000
+++ lib/lp/code/model/tests/test_branchnamespace.py 2012-09-21 04:16:20 +0000
@@ -496,7 +496,7 @@
self.assertContentEqual([], namespace.getAllowedInformationTypes())
self.assertIs(None, namespace.getDefaultInformationType())
- def test_proprietary_or_public_grantor(self):
+ def test_proprietary_or_public_owner_grantee(self):
namespace = self.makeProductNamespace(
BranchSharingPolicy.PROPRIETARY_OR_PUBLIC)
with person_logged_in(namespace.product.owner):
@@ -510,13 +510,28 @@
InformationType.PROPRIETARY,
namespace.getDefaultInformationType())
+ def test_proprietary_or_public_caller_grantee(self):
+ namespace = self.makeProductNamespace(
+ BranchSharingPolicy.PROPRIETARY_OR_PUBLIC)
+ grantee = self.factory.makePerson()
+ with person_logged_in(namespace.product.owner):
+ getUtility(IService, 'sharing').sharePillarInformation(
+ namespace.product, grantee, namespace.product.owner,
+ {InformationType.PROPRIETARY: SharingPermission.ALL})
+ self.assertContentEqual(
+ NON_EMBARGOED_INFORMATION_TYPES,
+ namespace.getAllowedInformationTypes(grantee))
+ self.assertEqual(
+ InformationType.PROPRIETARY,
+ namespace.getDefaultInformationType(grantee))
+
def test_proprietary_anyone(self):
namespace = self.makeProductNamespace(
BranchSharingPolicy.PROPRIETARY)
self.assertContentEqual([], namespace.getAllowedInformationTypes())
self.assertIs(None, namespace.getDefaultInformationType())
- def test_proprietary_grantee(self):
+ def test_proprietary_branch_owner_grantee(self):
namespace = self.makeProductNamespace(
BranchSharingPolicy.PROPRIETARY)
with person_logged_in(namespace.product.owner):
@@ -530,13 +545,28 @@
InformationType.PROPRIETARY,
namespace.getDefaultInformationType())
+ def test_proprietary_caller_grantee(self):
+ namespace = self.makeProductNamespace(
+ BranchSharingPolicy.PROPRIETARY)
+ grantee = self.factory.makePerson()
+ with person_logged_in(namespace.product.owner):
+ getUtility(IService, 'sharing').sharePillarInformation(
+ namespace.product, grantee, namespace.product.owner,
+ {InformationType.PROPRIETARY: SharingPermission.ALL})
+ self.assertContentEqual(
+ [InformationType.PROPRIETARY],
+ namespace.getAllowedInformationTypes(grantee))
+ self.assertEqual(
+ InformationType.PROPRIETARY,
+ namespace.getDefaultInformationType(grantee))
+
def test_embargoed_or_proprietary_anyone(self):
namespace = self.makeProductNamespace(
BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY)
self.assertContentEqual([], namespace.getAllowedInformationTypes())
self.assertIs(None, namespace.getDefaultInformationType())
- def test_embargoed_or_proprietary_grantee(self):
+ def test_embargoed_or_proprietary_owner_grantee(self):
namespace = self.makeProductNamespace(
BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY)
with person_logged_in(namespace.product.owner):
@@ -550,6 +580,21 @@
InformationType.EMBARGOED,
namespace.getDefaultInformationType())
+ def test_embargoed_or_proprietary_caller_grantee(self):
+ namespace = self.makeProductNamespace(
+ BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY)
+ grantee = self.factory.makePerson()
+ with person_logged_in(namespace.product.owner):
+ getUtility(IService, 'sharing').sharePillarInformation(
+ namespace.product, grantee, namespace.product.owner,
+ {InformationType.PROPRIETARY: SharingPermission.ALL})
+ self.assertContentEqual(
+ [InformationType.PROPRIETARY, InformationType.EMBARGOED],
+ namespace.getAllowedInformationTypes(grantee))
+ self.assertEqual(
+ InformationType.EMBARGOED,
+ namespace.getDefaultInformationType(grantee))
+
class TestPackageNamespace(TestCaseWithFactory, NamespaceMixin):
"""Tests for `PackageNamespace`."""
Follow ups
