launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #11539
lp:~wallyworld/launchpad/getInformationTypes-respect-policies-v2-1043368 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/getInformationTypes-respect-policies-v2-1043368 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1043368 in Launchpad itself: "SharingService.getInformationTypes must respect sharing policies"
https://bugs.launchpad.net/launchpad/+bug/1043368
Bug #1044546 in Launchpad itself: "+sharing shows '{policy_name}' for information types granted but not permitted by current combination of branch and bug sharing policies"
https://bugs.launchpad.net/launchpad/+bug/1044546
Bug #1045135 in Launchpad itself: "Changing a sharing policy can leave unused access policies lying around"
https://bugs.launchpad.net/launchpad/+bug/1045135
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/getInformationTypes-respect-policies-v2-1043368/+merge/122434
== Implementation ==
This branch deals with a few related bugs. We need the +sharing page to only offer the user valid information types to share, according to the bug and branch sharing policies which have been specified. But, there may be existing artifacts which use now illegal types which we must continue to support. So:
1. Sharing service getInformationTypes() returns the types for all current AP's
2. set[Bug|Branch]SharingPolicy prunes any unused AP's immediately rather than waiting for the garbo job
3. The garbo job now calls a _pruneUnusedPolicies() method on product.
4. Add a delete() method to IAccessPolicySource to keep everything properly architected.
Perhaps we don't need the garbo job anymore.
== Tests ==
Add tests for IAccessPolicySource.delete()
Add test for _pruneUnusedPolicies()
Refactor unused policy garbo job test.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/interfaces/accesspolicy.py
lib/lp/registry/interfaces/sharingservice.py
lib/lp/registry/model/accesspolicy.py
lib/lp/registry/model/distribution.py
lib/lp/registry/model/product.py
lib/lp/registry/services/sharingservice.py
lib/lp/registry/services/tests/test_sharingservice.py
lib/lp/registry/tests/test_accesspolicy.py
lib/lp/registry/tests/test_product.py
lib/lp/scripts/garbo.py
lib/lp/scripts/tests/test_garbo.py
--
https://code.launchpad.net/~wallyworld/launchpad/getInformationTypes-respect-policies-v2-1043368/+merge/122434
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/getInformationTypes-respect-policies-v2-1043368 into lp:launchpad.
=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py 2012-08-30 11:48:26 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py 2012-09-03 02:33:20 +0000
@@ -213,6 +213,9 @@
def findByTeam(teams):
"""Return a `ResultSet` of all `IAccessPolicy`s for the teams."""
+ def delete(policies):
+ """Delete the `IAccessPolicy objects."""
+
class IAccessPolicyGrantSource(Interface):
=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py 2012-09-02 23:01:19 +0000
+++ lib/lp/registry/interfaces/sharingservice.py 2012-09-03 02:33:20 +0000
@@ -112,7 +112,13 @@
"""
def getInformationTypes(pillar):
- """Return the allowed information types for the given pillar."""
+ """Return the allowed information types for the given pillar.
+
+ The allowed information types are those for which bugs and branches
+ may be created. This does not mean that there will necessarily be bugs
+ and branches of these types; the result is used to populate the allowed
+ choices in the grantee sharing pillar and other similar things.
+ """
def getBugSharingPolicies(pillar):
"""Return the allowed bug sharing policies for the given pillar."""
=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py 2012-08-30 11:48:26 +0000
+++ lib/lp/registry/model/accesspolicy.py 2012-09-03 02:33:20 +0000
@@ -259,6 +259,12 @@
cls,
Or(*(cls._constraintForPillar(pillar) for pillar in pillars)))
+ @classmethod
+ def delete(cls, policies):
+ """See `IAccessPolicySource`."""
+ ids = [policy.id for policy in policies]
+ IStore(cls).find(cls, cls.id.is_in(ids)).remove()
+
class AccessPolicyArtifact(StormBase):
implements(IAccessPolicyArtifact)
=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py 2012-09-02 23:01:19 +0000
+++ lib/lp/registry/model/distribution.py 2012-09-03 02:33:20 +0000
@@ -91,6 +91,7 @@
IFindOfficialBranchLinks,
)
from lp.registry.enums import (
+ FREE_INFORMATION_TYPES,
InformationType,
PRIVATE_INFORMATION_TYPES,
PUBLIC_INFORMATION_TYPES,
@@ -1574,10 +1575,7 @@
def getAllowedBugInformationTypes(self):
"""See `IDistribution.`"""
- types = set(InformationType.items)
- types.discard(InformationType.PROPRIETARY)
- types.discard(InformationType.EMBARGOED)
- return types
+ return FREE_INFORMATION_TYPES
def getDefaultBugInformationType(self):
"""See `IDistribution.`"""
=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py 2012-09-02 23:01:19 +0000
+++ lib/lp/registry/model/product.py 2012-09-03 02:33:20 +0000
@@ -119,12 +119,14 @@
from lp.registry.enums import (
BranchSharingPolicy,
BugSharingPolicy,
+ FREE_INFORMATION_TYPES,
InformationType,
PRIVATE_INFORMATION_TYPES,
)
from lp.registry.errors import CommercialSubscribersOnly
from lp.registry.interfaces.accesspolicy import (
IAccessPolicyGrantSource,
+ IAccessPolicyArtifactSource,
IAccessPolicySource,
)
from lp.registry.interfaces.oopsreferences import IHasOOPSReferences
@@ -577,6 +579,7 @@
branch_sharing_policy, BranchSharingPolicy, 'branches',
BRANCH_POLICY_ALLOWED_TYPES)
self.branch_sharing_policy = branch_sharing_policy
+ self._pruneUnusedPolicies()
def setBugSharingPolicy(self, bug_sharing_policy):
"""See `IProductEditRestricted`."""
@@ -584,16 +587,13 @@
bug_sharing_policy, BugSharingPolicy, 'bugs',
BUG_POLICY_ALLOWED_TYPES)
self.bug_sharing_policy = bug_sharing_policy
+ self._pruneUnusedPolicies()
def getAllowedBugInformationTypes(self):
"""See `IProduct.`"""
if self.bug_sharing_policy is not None:
return BUG_POLICY_ALLOWED_TYPES[self.bug_sharing_policy]
-
- types = set(InformationType.items)
- types.discard(InformationType.PROPRIETARY)
- types.discard(InformationType.EMBARGOED)
- return types
+ return FREE_INFORMATION_TYPES
def getDefaultBugInformationType(self):
"""See `IDistribution.`"""
@@ -622,6 +622,27 @@
grants.append((p, self.owner, self.owner))
getUtility(IAccessPolicyGrantSource).grant(grants)
+ def _pruneUnusedPolicies(self):
+ allowed_bug_types = set(
+ BUG_POLICY_ALLOWED_TYPES.get(
+ self.bug_sharing_policy, FREE_INFORMATION_TYPES))
+ allowed_branch_types = set(
+ BRANCH_POLICY_ALLOWED_TYPES.get(
+ self.branch_sharing_policy, FREE_INFORMATION_TYPES))
+ allowed_types = allowed_bug_types.union(allowed_branch_types)
+ # Fetch all APs, and after filtering out ones that are forbidden
+ # by the bug and branch policies, the APs that have no APAs are
+ # unused and can be deleted.
+ ap_source = getUtility(IAccessPolicySource)
+ access_policies = set(ap_source.findByPillar([self]))
+ apa_source = getUtility(IAccessPolicyArtifactSource)
+ unused_aps = [
+ ap for ap in access_policies
+ if ap.type not in allowed_types
+ and apa_source.findByPolicy([ap]).is_empty()]
+ getUtility(IAccessPolicyGrantSource).revokeByPolicy(unused_aps)
+ ap_source.delete(unused_aps)
+
@cachedproperty
def commercial_subscription(self):
return CommercialSubscription.selectOneBy(product=self)
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-09-02 23:01:19 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-09-03 02:33:20 +0000
@@ -32,7 +32,7 @@
from lp.registry.enums import (
BranchSharingPolicy,
BugSharingPolicy,
- InformationType,
+ PRIVATE_INFORMATION_TYPES,
SharingPermission,
)
from lp.registry.interfaces.accesspolicy import (
@@ -255,16 +255,14 @@
def getInformationTypes(self, pillar):
"""See `ISharingService`."""
- allowed_types = [
- InformationType.PRIVATESECURITY,
- InformationType.USERDATA]
- # Products with current commercial subscriptions are also allowed to
- # have a PROPRIETARY information type.
- if (IProduct.providedBy(pillar) and
- pillar.has_current_commercial_subscription):
- allowed_types.append(InformationType.PROPRIETARY)
-
- return self._makeEnumData(allowed_types)
+ allowed_private_types = [
+ policy.type
+ for policy in getUtility(IAccessPolicySource).findByPillar(
+ [pillar])]
+ # We want the types in a specific order.
+ return self._makeEnumData([
+ type for type in PRIVATE_INFORMATION_TYPES
+ if type in allowed_private_types])
def getBranchSharingPolicies(self, pillar):
"""See `ISharingService`."""
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py 2012-09-02 23:01:19 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-09-03 02:33:20 +0000
@@ -139,13 +139,20 @@
[InformationType.PRIVATESECURITY, InformationType.USERDATA])
def test_getInformationTypes_commercial_product(self):
- product = self.factory.makeProduct()
- self.factory.makeCommercialSubscription(product)
- self._assert_getInformationTypes(
- product,
- [InformationType.PRIVATESECURITY,
- InformationType.USERDATA,
- InformationType.PROPRIETARY])
+ product = self.factory.makeProduct(
+ branch_sharing_policy=BranchSharingPolicy.PROPRIETARY,
+ bug_sharing_policy=BugSharingPolicy.PROPRIETARY)
+ self._assert_getInformationTypes(
+ product,
+ [InformationType.PROPRIETARY])
+
+ def test_getInformationTypes_product_with_embargoed(self):
+ product = self.factory.makeProduct(
+ branch_sharing_policy=BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY,
+ bug_sharing_policy=BugSharingPolicy.PROPRIETARY)
+ self._assert_getInformationTypes(
+ product,
+ [InformationType.PROPRIETARY, InformationType.EMBARGOED])
def test_getInformationTypes_distro(self):
distro = self.factory.makeDistribution()
=== modified file 'lib/lp/registry/tests/test_accesspolicy.py'
--- lib/lp/registry/tests/test_accesspolicy.py 2012-08-30 11:48:26 +0000
+++ lib/lp/registry/tests/test_accesspolicy.py 2012-09-03 02:33:20 +0000
@@ -160,6 +160,18 @@
[ap.person
for ap in getUtility(IAccessPolicySource).findByTeam([team])])
+ def test_delete(self):
+ # delete functions as expected.
+ ap_source = getUtility(IAccessPolicySource)
+ pillars = [self.factory.makeProduct() for x in range(5)]
+ policies = list(ap_source.findByPillar(pillars))
+ getUtility(IAccessPolicyGrantSource).revokeByPolicy(policies[2:])
+ ap_source.delete(policies[2:])
+ IStore(policies[0]).invalidate()
+ self.assertRaises(LostObjectError, getattr, policies[3], 'pillar')
+ self.assertContentEqual(
+ policies[:2], ap_source.findByPillar(pillars))
+
class TestAccessArtifact(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py 2012-09-02 23:01:19 +0000
+++ lib/lp/registry/tests/test_product.py 2012-09-03 02:33:20 +0000
@@ -832,6 +832,42 @@
getUtility(IService, 'sharing').checkPillarAccess(
self.product, InformationType.PROPRIETARY, self.product.owner))
+ def test_unused_policies_are_pruned(self):
+ # When a sharing policy is changed, the allowed information types may
+ # become more restricted. If this case, any existing access polices
+ # for the now defunct information type(s) should be removed so long as
+ # there are no corresponding policy artifacts.
+
+ # We create a product with and ensure there's an APA.
+ ap_source = getUtility(IAccessPolicySource)
+ product = self.factory.makeProduct()
+ [ap] = ap_source.find([(product, InformationType.PRIVATESECURITY)])
+ self.factory.makeAccessPolicyArtifact(policy=ap)
+
+ def getAccessPolicyTypes(pillar):
+ return [
+ ap.type
+ for ap in ap_source.findByPillar([pillar])]
+
+ # Now change the sharing policies to PROPRIETARY
+ self.factory.makeCommercialSubscription(product=product)
+ with person_logged_in(product.owner):
+ product.setBugSharingPolicy(BugSharingPolicy.PROPRIETARY)
+ # Just bug sharing policy has been changed so all previous policy
+ # types are still valid.
+ self.assertContentEqual(
+ [InformationType.PRIVATESECURITY, InformationType.USERDATA,
+ InformationType.PROPRIETARY],
+ getAccessPolicyTypes(product))
+
+ product.setBranchSharingPolicy(BranchSharingPolicy.PROPRIETARY)
+ # Proprietary is permitted by the sharing policy, and there's a
+ # Private Security artifact. But Private isn't in use or allowed
+ # by a sharing policy, so it's now gone.
+ self.assertContentEqual(
+ [InformationType.PRIVATESECURITY, InformationType.PROPRIETARY],
+ getAccessPolicyTypes(product))
+
class ProductBugSharingPolicyTestCase(BaseSharingPolicyTests,
TestCaseWithFactory):
=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py 2012-08-31 01:27:57 +0000
+++ lib/lp/scripts/garbo.py 2012-09-03 02:33:20 +0000
@@ -47,7 +47,6 @@
from lp.answers.model.answercontact import AnswerContact
from lp.bugs.interfaces.bug import IBugSet
-from lp.bugs.interfaces.bugtarget import BUG_POLICY_ALLOWED_TYPES
from lp.bugs.model.bug import Bug
from lp.bugs.model.bugattachment import BugAttachment
from lp.bugs.model.bugnotification import BugNotification
@@ -58,7 +57,6 @@
)
from lp.code.enums import BranchVisibilityRule
from lp.code.interfaces.revision import IRevisionSet
-from lp.code.model.branchnamespace import BRANCH_POLICY_ALLOWED_TYPES
from lp.code.model.branchvisibilitypolicy import BranchVisibilityTeamPolicy
from lp.code.model.codeimportevent import CodeImportEvent
from lp.code.model.codeimportresult import CodeImportResult
@@ -67,12 +65,6 @@
RevisionCache,
)
from lp.hardwaredb.model.hwdb import HWSubmission
-from lp.registry.enums import FREE_INFORMATION_TYPES
-from lp.registry.interfaces.accesspolicy import (
- IAccessPolicyArtifactSource,
- IAccessPolicyGrantSource,
- IAccessPolicySource,
- )
from lp.registry.model.commercialsubscription import CommercialSubscription
from lp.registry.model.person import Person
from lp.registry.model.product import Product
@@ -1083,26 +1075,7 @@
def __call__(self, chunk_size):
products = list(self.findProducts()[:chunk_size])
for product in products:
- allowed_bug_types = set(
- BUG_POLICY_ALLOWED_TYPES.get(
- product.bug_sharing_policy, FREE_INFORMATION_TYPES))
- allowed_branch_types = set(
- BRANCH_POLICY_ALLOWED_TYPES.get(
- product.branch_sharing_policy, FREE_INFORMATION_TYPES))
- allowed_types = allowed_bug_types.union(allowed_branch_types)
- # Fetch all APs, and after filtering out ones that are forbidden
- # by the bug and branch policies, the APs that have no APAs are
- # unused and can be deleted.
- access_policies = set(
- getUtility(IAccessPolicySource).findByPillar([product]))
- apa_source = getUtility(IAccessPolicyArtifactSource)
- unused_aps = [
- ap for ap in access_policies
- if ap.type not in allowed_types
- and apa_source.findByPolicy([ap]).is_empty()]
- getUtility(IAccessPolicyGrantSource).revokeByPolicy(unused_aps)
- for ap in unused_aps:
- self.store.remove(ap)
+ product._pruneUnusedPolicies()
self.start_at = products[-1].id + 1
transaction.commit()
=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py 2012-08-29 21:45:07 +0000
+++ lib/lp/scripts/tests/test_garbo.py 2012-09-03 02:33:20 +0000
@@ -1049,7 +1049,6 @@
project_with_bvp.setBranchVisibilityTeamPolicy(
None, BranchVisibilityRule.FORBIDDEN)
-
def get_non_migrated_products():
return IMasterStore(Product).find(
Product,
@@ -1098,9 +1097,10 @@
switch_dbuser('testadmin')
product = self.factory.makeProduct()
self.factory.makeCommercialSubscription(product=product)
- with person_logged_in(product.owner):
- product.setBugSharingPolicy(BugSharingPolicy.PROPRIETARY)
- product.setBranchSharingPolicy(BranchSharingPolicy.PROPRIETARY)
+ self.factory.makeAccessPolicy(product, InformationType.PROPRIETARY)
+ naked_product = removeSecurityProxy(product)
+ naked_product.bug_sharing_policy = BugSharingPolicy.PROPRIETARY
+ naked_product.branch_sharing_policy = BranchSharingPolicy.PROPRIETARY
[ap] = getUtility(IAccessPolicySource).find(
[(product, InformationType.PRIVATESECURITY)])
self.factory.makeAccessPolicyArtifact(policy=ap)
Follow ups
