launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13444
[Merge] lp:~adeuring/launchpad/milestone-sec-adapter into lp:launchpad
Abel Deuring has proposed merging lp:~adeuring/launchpad/milestone-sec-adapter into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~adeuring/launchpad/milestone-sec-adapter/+merge/129917
This branch adds a "sharing wawre" security adapter for IMilestone.
Milestones related to private products are now only shown to persons
with policy grants for the product; the only exception are some
attributes that leak no information: the database ID, and the methods
userCanView(), checkAuthenticated(), checkUnauthenticated().
The two latter methods should not appear at all -- we use adapters
for the security checks, so there is no need to define them in the
main class. And in fact they are not implemented at all.
Similary, IMilestone and some other interface classes included in
IMilestone define a few attributes that not implemented by class
Milestone.
We do not have very much time to finish the "Private products" story,
so I simply tried to ignore these inconsistencies.
Details of the change:
lp/registry/configure.zcml:
Access to most attributes and to "partial" interfaces that were public
now requires the permission launchpad.View; the permission
launchpad.AnyPerson is replaced with launchapd.AnyAllowedPerson.
(lp.services.webapp.authorization.LaunchpadSecurityPolicy.checkPermission()
has a "shortcut" for the permission launchpad.AnyPerson: no
dedicated security adapters are looked up for this permission,
so the new rule "data for milestones of private product should only
be visible for person having a policy grant" cannot be implemented
with this permission.)
I also sorted the attributes requiring lp.View alphabetically.
lp/security.py:
two new adapters for IMilestone and the permissions lp.View and
lp.AnyAllowedPerson, respectively.
Both methods delegate the security check to the new method
Milestone.userCanView()
lp/registry/model/milestone.py:
The new method userCanView(). The acutal permission check is done
by IProduct.userCanView()
lp/registry/tests/test_milestone.py:
Tests for the permissions.
The test class properties expected_get_permissions and
expected_set_permissions are also intended to document which permissions
are acutally used for IMilestone.
The tests are similar to the test pattern I also used for the
security adapters of IProduct and ISpecification, so I moved the
common method checkPermissions() to lp.testing.TestCase.
test:
./bin/test registry -vvt lp.registry.tests.test_milestone.MilestoneSecurityAdaperTestCase
no lint
--
https://code.launchpad.net/~adeuring/launchpad/milestone-sec-adapter/+merge/129917
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/milestone-sec-adapter into lp:launchpad.
=== modified file 'lib/lp/blueprints/tests/test_specification.py'
--- lib/lp/blueprints/tests/test_specification.py 2012-09-24 13:43:00 +0000
+++ lib/lp/blueprints/tests/test_specification.py 2012-10-16 14:50:28 +0000
@@ -139,26 +139,6 @@
self.assertRaises(
Unauthorized, getattr, specification, 'setTarget')
- def check_permissions(self, expected_permissions, used_permissions,
- type_):
- expected = set(expected_permissions.keys())
- self.assertEqual(
- expected, set(used_permissions.values()),
- 'Unexpected %s permissions' % type_)
- for permission in expected_permissions:
- attribute_names = set(
- name for name, value in used_permissions.items()
- if value == permission)
- self.assertEqual(
- expected_permissions[permission], attribute_names,
- 'Unexpected set of attributes with %s permission %s:\n'
- 'Defined but not expected: %s\n'
- 'Expected but not defined: %s'
- % (
- type_, permission,
- attribute_names - expected_permissions[permission],
- expected_permissions[permission] - attribute_names))
-
def test_get_permissions(self):
expected_get_permissions = {
CheckerPublic: set((
@@ -196,7 +176,7 @@
}
specification = self.factory.makeSpecification()
checker = getChecker(specification)
- self.check_permissions(
+ self.checkPermissions(
expected_get_permissions, checker.get_permissions, 'get')
def test_set_permissions(self):
@@ -211,7 +191,7 @@
}
specification = self.factory.makeSpecification()
checker = getChecker(specification)
- self.check_permissions(
+ self.checkPermissions(
expected_get_permissions, checker.set_permissions, 'set')
def test_security_adapters(self):
=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml 2012-10-09 10:28:02 +0000
+++ lib/lp/registry/configure.zcml 2012-10-16 14:50:28 +0000
@@ -1026,22 +1026,30 @@
<allow
attributes="
id
- name
+ userCanView
+ " />
+ <require
+ permission="launchpad.View"
+ attributes="
+ active
+ bugtasks
code_name
+ dateexpected
+ displayname
+ distribution
+ distroseries
+ getTags
+ getTagsData
+ name
product
- distribution
+ product_release
productseries
- distroseries
- dateexpected
- active
- summary
- target
series_target
- displayname
- title
- bugtasks
specifications
- product_release"/>
+ summary
+ target
+ title
+ "/>
<require
permission="launchpad.Edit"
attributes="
@@ -1051,15 +1059,13 @@
setTags
"/>
<require
- permission="zope.Public"
- attributes="
- getTags
- getTagsData
- "/>
- <allow interface="lp.bugs.interfaces.bugsummary.IBugSummaryDimension"/>
- <allow
+ permission="launchpad.View"
+ interface="lp.bugs.interfaces.bugsummary.IBugSummaryDimension"/>
+ <require
+ permission="launchpad.View"
interface="lp.bugs.interfaces.bugtarget.IHasBugs"/>
- <allow
+ <require
+ permission="launchpad.View"
interface="lp.bugs.interfaces.bugtarget.IHasOfficialBugTags"/>
<allow
interface="lp.app.interfaces.security.IAuthorization"/>
@@ -1070,10 +1076,11 @@
<!-- IStructuralSubscriptionTarget -->
- <allow
+ <require
+ permission="launchpad.View"
interface="lp.bugs.interfaces.structuralsubscription.IStructuralSubscriptionTargetRead" />
<require
- permission="launchpad.AnyPerson"
+ permission="launchpad.AnyAllowedPerson"
interface="lp.bugs.interfaces.structuralsubscription.IStructuralSubscriptionTargetWrite" />
</class>
=== modified file 'lib/lp/registry/interfaces/milestone.py'
--- lib/lp/registry/interfaces/milestone.py 2012-10-15 02:32:30 +0000
+++ lib/lp/registry/interfaces/milestone.py 2012-10-16 14:50:28 +0000
@@ -267,6 +267,9 @@
why this is not a property.
"""
+ def userCanView(user):
+ """True if the given user has access to this product."""
+
# Avoid circular imports
IBugTask['milestone'].schema = IMilestone
=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py 2012-10-06 23:14:35 +0000
+++ lib/lp/registry/model/milestone.py 2012-10-16 14:50:28 +0000
@@ -351,6 +351,17 @@
from lp.registry.model.milestonetag import MilestoneTag
return list(self.getTagsData().values(MilestoneTag.tag))
+ def userCanView(self, user):
+ """See `IMilestone`."""
+ # A database constraint ensures that either self.product
+ # or self.distribution is not None.
+ if self.product is None:
+ # Distributions are always public, and so are their
+ # milestones.
+ return True
+ # Delegate the permission check
+ return self.product.userCanView(user)
+
class MilestoneSet:
implements(IMilestoneSet)
=== modified file 'lib/lp/registry/tests/test_milestone.py'
--- lib/lp/registry/tests/test_milestone.py 2012-10-02 02:56:32 +0000
+++ lib/lp/registry/tests/test_milestone.py 2012-10-16 14:50:28 +0000
@@ -6,11 +6,20 @@
__metaclass__ = type
from operator import attrgetter
+from storm.exceptions import NoneError
import unittest
from zope.component import getUtility
+from zope.security.checker import (
+ CheckerPublic,
+ getChecker,
+ )
+from zope.security.interfaces import Unauthorized
+from lp.app.enums import InformationType
+from lp.app.interfaces.services import IService
from lp.app.errors import NotFoundError
+from lp.registry.enums import SharingPermission
from lp.registry.interfaces.distribution import IDistributionSet
from lp.registry.interfaces.milestone import (
IHasMilestones,
@@ -101,6 +110,244 @@
[1, 2, 3])
+class MilestoneSecurityAdaperTestCase(TestCaseWithFactory):
+ """A TestCase for the security adapter of milestones."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(MilestoneSecurityAdaperTestCase, self).setUp()
+ self.public_product = self.factory.makeProduct()
+ self.public_milestone = self.factory.makeMilestone(
+ product=self.public_product)
+ self.proprietary_product_owner = self.factory.makePerson()
+ self.proprietary_product = self.factory.makeProduct(
+ owner=self.proprietary_product_owner,
+ information_type=InformationType.PROPRIETARY)
+ self.proprietary_milestone = self.factory.makeMilestone(
+ product=self.proprietary_product)
+
+ expected_get_permissions = {
+ CheckerPublic: set((
+ 'id', 'checkAuthenticated', 'checkUnauthenticated',
+ 'userCanView',
+ )),
+ 'launchpad.View': set((
+ 'active', 'bug_subscriptions', 'bugtasks', 'code_name',
+ 'dateexpected', 'displayname', 'distribution', 'distroseries',
+ '_getOfficialTagClause', 'getBugSummaryContextWhereClause',
+ 'getBugTaskWeightFunction', 'getSubscription',
+ 'getSubscriptions', 'getTags', 'getTagsData',
+ 'getUsedBugTagsWithOpenCounts', 'name', 'official_bug_tags',
+ 'parent_subscription_target', 'product', 'product_release',
+ 'productseries', 'searchTasks', 'series_target',
+ 'specifications', 'summary', 'target', 'target_type_display',
+ 'title', 'userCanAlterBugSubscription',
+ 'userCanAlterSubscription', 'userHasBugSubscriptions',
+ )),
+ 'launchpad.AnyAllowedPerson': set((
+ 'addBugSubscription', 'addBugSubscriptionFilter',
+ 'addSubscription', 'removeBugSubscription',
+ )),
+ 'launchpad.Edit': set((
+ 'closeBugsAndBlueprints', 'createProductRelease',
+ 'destroySelf', 'setTags',
+ )),
+ }
+
+ def test_get_permissions(self):
+ milestone = self.factory.makeMilestone()
+ checker = getChecker(milestone)
+ self.checkPermissions(
+ self.expected_get_permissions, checker.get_permissions, 'get')
+
+ expected_set_permissions = {
+ 'launchpad.Edit': set((
+ 'active', 'code_name', 'dateexpected', 'distroseries', 'name',
+ 'product_release', 'productseries', 'summary',
+ )),
+ }
+
+ def test_set_permissions(self):
+ milestone = self.factory.makeMilestone()
+ checker = getChecker(milestone)
+ self.checkPermissions(
+ self.expected_set_permissions, checker.set_permissions, 'set')
+
+ def assertAccessAuthorzized(self, attribute_names, obj):
+ # Try to access the given attributes of obj. No exception
+ # should be raised.
+ for name in attribute_names:
+ # class Milestone does not implenet all attributes defined by
+ # class IMilestone. AttributeErrors caused by attempts to
+ # access these attribues are not relevant here: We simply
+ # want to be sure that no Unauthorized error is raised.
+ try:
+ getattr(obj, name)
+ except AttributeError:
+ pass
+
+ def assertAccessUnauthorzized(self, attribute_names, obj):
+ # Try to access the given attributes of obj. Unauthorized
+ # should be raised.
+ for name in attribute_names:
+ self.assertRaises(Unauthorized, getattr, obj, name)
+
+ def assertChangeAuthorzized(self, attribute_names, obj):
+ # Try to changes the given attributes of obj. Unauthorized
+ # should be raised.
+ for name in attribute_names:
+ # Not all attributes declared in configure.zcml to be
+ # settable actually exist. Attempts to set them raises
+ # an AttributeError. Setting an Attribute to None may no
+ # be allowed.
+ #
+ # Both errors can be ignored here: This method intends only
+ # to prove that Unauthorized is not raised.
+ try:
+ setattr(obj, name, None)
+ except (AttributeError, NoneError):
+ pass
+
+ def assertChangeUnauthorzized(self, attribute_names, obj):
+ # Try to changes the given attributes of obj. Unauthorized
+ # should be raised.
+ for name in attribute_names:
+ self.assertRaises(Unauthorized, setattr, obj, name, None)
+
+ def test_access_for_anonymous(self):
+ # Anonymous users have to public attributes of milestones for
+ # private and public products.
+ with person_logged_in(ANONYMOUS):
+ self.assertAccessAuthorzized(
+ self.expected_get_permissions[CheckerPublic],
+ self.public_milestone)
+ self.assertAccessAuthorzized(
+ self.expected_get_permissions[CheckerPublic],
+ self.proprietary_milestone)
+
+ # They have access to attributes requiring the permission
+ # launchpad.View of milestones for public products...
+ self.assertAccessAuthorzized(
+ self.expected_get_permissions['launchpad.View'],
+ self.public_milestone)
+
+ # ...but not to the same attributes of milestones for private
+ # products.
+ self.assertAccessUnauthorzized(
+ self.expected_get_permissions['launchpad.View'],
+ self.proprietary_milestone)
+
+ # They cannot access other attributes.
+ for permission, names in self.expected_get_permissions.items():
+ if permission in (CheckerPublic, 'launchpad.View'):
+ continue
+ self.assertAccessUnauthorzized(names, self.public_milestone)
+ self.assertAccessUnauthorzized(
+ names, self.proprietary_milestone)
+
+ # They cannot change any attributes.
+ for permission, names in self.expected_set_permissions.items():
+ self.assertChangeUnauthorzized(names, self.public_milestone)
+ self.assertChangeUnauthorzized(
+ names, self.proprietary_milestone)
+
+ def test_access_for_ordinary_user(self):
+ # Regular users have to public attributes of milestones for
+ # private and public products.
+ user = self.factory.makePerson()
+ with person_logged_in(user):
+ self.assertAccessAuthorzized(
+ self.expected_get_permissions[CheckerPublic],
+ self.public_milestone)
+ self.assertAccessAuthorzized(
+ self.expected_get_permissions[CheckerPublic],
+ self.proprietary_milestone)
+
+ # They have access to attributes requiring the permission
+ # launchpad.View or launchpad.AnyAllowedPerson of milestones
+ # for public products...
+ self.assertAccessAuthorzized(
+ self.expected_get_permissions['launchpad.View'],
+ self.public_milestone)
+ self.assertAccessAuthorzized(
+ self.expected_get_permissions['launchpad.AnyAllowedPerson'],
+ self.public_milestone)
+
+ # ...but not to the same attributes of milestones for private
+ # products.
+ self.assertAccessUnauthorzized(
+ self.expected_get_permissions['launchpad.View'],
+ self.proprietary_milestone)
+ self.assertAccessUnauthorzized(
+ self.expected_get_permissions['launchpad.AnyAllowedPerson'],
+ self.proprietary_milestone)
+
+ # They cannot access other attributes.
+ for permission, names in self.expected_get_permissions.items():
+ if permission in (
+ CheckerPublic, 'launchpad.View',
+ 'launchpad.AnyAllowedPerson'):
+ continue
+ self.assertAccessUnauthorzized(names, self.public_milestone)
+ self.assertAccessUnauthorzized(
+ names, self.proprietary_milestone)
+
+ # They cannot change attributes.
+ for permission, names in self.expected_set_permissions.items():
+ self.assertChangeUnauthorzized(names, self.public_milestone)
+ self.assertChangeUnauthorzized(
+ names, self.proprietary_milestone)
+
+ def test_access_for_user_with_grant_for_private_product(self):
+ # Users with a policy grant for a private product have access
+ # to most attributes of the private product.
+ user = self.factory.makePerson()
+ with person_logged_in(self.proprietary_product_owner):
+ getUtility(IService, 'sharing').sharePillarInformation(
+ self.proprietary_product, user, self.proprietary_product_owner,
+ {InformationType.PROPRIETARY: SharingPermission.ALL})
+
+ with person_logged_in(user):
+ self.assertAccessAuthorzized(
+ self.expected_get_permissions[CheckerPublic],
+ self.proprietary_milestone)
+
+ # They have access to attributes requiring the permission
+ # launchpad.View or launchpad.AnyAllowedPerson of milestones
+ # for the private product.
+ self.assertAccessAuthorzized(
+ self.expected_get_permissions['launchpad.View'],
+ self.proprietary_milestone)
+ self.assertAccessAuthorzized(
+ self.expected_get_permissions['launchpad.AnyAllowedPerson'],
+ self.proprietary_milestone)
+
+ # They cannot access other attributes.
+ for permission, names in self.expected_get_permissions.items():
+ if permission in (
+ CheckerPublic, 'launchpad.View',
+ 'launchpad.AnyAllowedPerson'):
+ continue
+ self.assertAccessUnauthorzized(
+ names, self.proprietary_milestone)
+
+ # They cannot change attributes.
+ for names in self.expected_set_permissions.values():
+ self.assertChangeUnauthorzized(
+ names, self.proprietary_milestone)
+
+ def test_access_for_product_owner(self):
+ # The owner of a private product can access all attributes.
+ with person_logged_in(self.proprietary_product_owner):
+ for names in self.expected_get_permissions.values():
+ self.assertAccessAuthorzized(names, self.proprietary_milestone)
+
+ # They can change attributes.
+ for permission, names in self.expected_set_permissions.items():
+ self.assertChangeAuthorzized(names, self.proprietary_milestone)
+
+
class HasMilestonesSnapshotTestCase(TestCaseWithFactory):
"""A TestCase for snapshots of pillars with milestones."""
=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py 2012-10-15 09:15:29 +0000
+++ lib/lp/registry/tests/test_product.py 2012-10-16 14:50:28 +0000
@@ -535,28 +535,6 @@
CannotChangeInformationType, 'Some series are packaged.'):
product.information_type = InformationType.PROPRIETARY
- def check_permissions(self, expected_permissions, used_permissions,
- type_):
- expected = set(expected_permissions.keys())
- self.assertEqual(
- expected, set(used_permissions.values()),
- 'Unexpected %s permissions' % type_)
- for permission in expected_permissions:
- attribute_names = set(
- name for name, value in used_permissions.items()
- if value == permission)
- self.assertEqual(
- expected_permissions[permission], attribute_names,
- 'Unexpected set of attributes with %s permission %s:\n'
- 'Defined but not expected: %s\n'
- 'Expected but not defined: %s'
- % (
- type_, permission,
- sorted(
- attribute_names - expected_permissions[permission]),
- sorted(
- expected_permissions[permission] - attribute_names)))
-
expected_get_permissions = {
CheckerPublic: set((
'active', 'id', 'information_type', 'pillar_category', 'private',
@@ -645,7 +623,7 @@
def test_get_permissions(self):
product = self.factory.makeProduct()
checker = getChecker(product)
- self.check_permissions(
+ self.checkPermissions(
self.expected_get_permissions, checker.get_permissions, 'get')
def test_set_permissions(self):
@@ -678,7 +656,7 @@
}
product = self.factory.makeProduct()
checker = getChecker(product)
- self.check_permissions(
+ self.checkPermissions(
expected_set_permissions, checker.set_permissions, 'set')
def test_access_launchpad_View_public_product(self):
=== modified file 'lib/lp/security.py'
--- lib/lp/security.py 2012-10-12 14:53:10 +0000
+++ lib/lp/security.py 2012-10-16 14:50:28 +0000
@@ -727,6 +727,25 @@
return False
+class ViewMilestone(AuthorizationBase):
+ permission = 'launchpad.View'
+ usedfor = IMilestone
+
+ def checkAuthenticated(self, user):
+ return self.obj.userCanView(user)
+
+ def checkUnauthenticated(self):
+ return self.obj.userCanView(user=None)
+
+
+class EditMilestone(ViewMilestone):
+ permission = 'launchpad.AnyAllowedPerson'
+ usedfor = IMilestone
+
+ def checkUnauthenticated(self):
+ return False
+
+
class EditMilestoneByTargetOwnerOrAdmins(AuthorizationBase):
permission = 'launchpad.Edit'
usedfor = IMilestone
=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py 2012-09-05 14:44:17 +0000
+++ lib/lp/testing/__init__.py 2012-10-16 14:50:28 +0000
@@ -704,6 +704,36 @@
raise AssertionError(
'string %r does not end with %r' % (s, suffix))
+ def checkPermissions(self, expected_permissions, used_permissions,
+ type_):
+ """Check if the used_permissions match expected_permissions.
+
+ :param expected_permissions: A dictionary mapping a permission
+ to a set of attribute names.
+ :param used_permissions: The property get_permissions or
+ set_permissions of getChecker(security_proxied_object).
+ :param type_: The string "set" or "get".
+ """
+ expected = set(expected_permissions.keys())
+ self.assertEqual(
+ expected, set(used_permissions.values()),
+ 'Unexpected %s permissions' % type_)
+ for permission in expected_permissions:
+ attribute_names = set(
+ name for name, value in used_permissions.items()
+ if value == permission)
+ self.assertEqual(
+ expected_permissions[permission], attribute_names,
+ 'Unexpected set of attributes with %s permission %s:\n'
+ 'Defined but not expected: %s\n'
+ 'Expected but not defined: %s'
+ % (
+ type_, permission,
+ sorted(
+ attribute_names - expected_permissions[permission]),
+ sorted(
+ expected_permissions[permission] - attribute_names)))
+
class TestCaseWithFactory(TestCase):
Follow ups
