launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06664
[Merge] lp:~wallyworld/launchpad/some-sharing-950562 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/some-sharing-950562 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #950562 in Launchpad itself: "Sharing service needs to support 'Some'"
https://bugs.launchpad.net/launchpad/+bug/950562
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/some-sharing-950562/+merge/97130
== Implementation ==
Branch 1 of X to add support for sharing permission 'some' to the sharing service. This branch:
- updates the sharePillarInformation interface to accept a dict of (information_type->permission)
- updates callsites for the above (including pillarsharingview javascript widget)
- updates getPillarSharees() to correctly include information types with permission 'some' as well as 'all'
The sharePillarInformation() permissions parameter is now a Dict but this was not supported by lazr.resful. I have added dict support to lazr.resful and this branch cannot land until an updated lazr.restful is put in the download cache.
The getPillarSharees() method uses an updated findGranteesByPolicy() on IAccessPolicyGrantFlatSource. The method used to return just a list of people. Now it returns a list of tuples (person, policy, permission).
This branch is not yet fully cooked - sharePillarInformation() doesn't yet implement support for storing 'some' permission, even though it's interface support it. The work will be done in a subsequent branch.
== Tests ==
Update tests for AccessPolicyGrantFlatSource
Update tests for sharing service
Update tests for pillar sharing view
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
versions.cfg
lib/lp/registry/interfaces/accesspolicy.py
lib/lp/registry/interfaces/sharingservice.py
lib/lp/registry/javascript/sharing/pillarsharingview.js
lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js
lib/lp/registry/model/accesspolicy.py
lib/lp/registry/services/sharingservice.py
lib/lp/registry/services/tests/test_sharingservice.py
lib/lp/registry/tests/test_accesspolicy.py
--
https://code.launchpad.net/~wallyworld/launchpad/some-sharing-950562/+merge/97130
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/some-sharing-950562 into lp:launchpad.
=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py 2012-03-08 08:01:44 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py 2012-03-13 03:10:25 +0000
@@ -215,11 +215,16 @@
"""Experimental query utility to search through the flattened schema."""
def findGranteesByPolicy(policies):
- """Find the `IPerson`s with access grants for the policies.
+ """Find teams or people with access grants for the policies.
This includes grants for artifacts in the policies.
:param policies: a collection of `IAccesPolicy`s.
+ :return: a collection of (`IPerson`, `IAccessPolicy`, permission)
+ where permission is a SharingPermission value.
+ 'ALL' means the person has an access policy grant and can see all
+ artifacts for the associated pillar.
+ 'SOME' means the person only has specified access artifact grants.
"""
def findArtifactsByGrantee(grantee, policies):
=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py 2012-03-08 04:26:43 +0000
+++ lib/lp/registry/interfaces/sharingservice.py 2012-03-13 03:10:25 +0000
@@ -22,12 +22,16 @@
from lazr.restful.fields import Reference
from zope.schema import (
Choice,
+ Dict,
List,
)
from lp import _
from lp.app.interfaces.services import IService
-from lp.registry.enums import InformationType
+from lp.registry.enums import (
+ InformationType,
+ SharingPermission,
+ )
from lp.registry.interfaces.person import IPerson
from lp.registry.interfaces.pillar import IPillar
@@ -57,10 +61,22 @@
@operation_parameters(
pillar=Reference(IPillar, title=_('Pillar'), required=True),
sharee=Reference(IPerson, title=_('Sharee'), required=True),
- information_types=List(Choice(vocabulary=InformationType)))
+ permissions=Dict(
+ key_type=Choice(vocabulary=InformationType),
+ value_type=Choice(vocabulary=SharingPermission)))
@operation_for_version('devel')
- def sharePillarInformation(pillar, sharee, information_types, user):
- """Ensure sharee has the grants for information types on a pillar."""
+ def sharePillarInformation(pillar, sharee, permissions, user):
+ """Ensure sharee has the grants for information types on a pillar.
+
+ :param pillar: the pillar for which to grant access
+ :param sharee: the person or team to grant
+ :param permissions: a dict of {InformationType: SharingPermission}
+ if SharingPermission is ALL, then create an access policy grant
+ if SharingPermission is SOME, then remove any access policy grants
+ if SharingPermission is NONE, then remove all grants for the access
+ policy
+ :param user: the user making the request
+ """
@export_write_operation()
@operation_parameters(
=== modified file 'lib/lp/registry/javascript/sharing/pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-03-08 13:47:33 +0000
+++ lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-03-13 03:10:25 +0000
@@ -269,6 +269,11 @@
var pillar_uri = LP.cache.context.self_link;
var person_uri = Y.lp.client.normalize_uri(selection_result.api_uri);
person_uri = Y.lp.client.get_absolute_uri(person_uri);
+ var permissions = [];
+ Y.Array.each(
+ selection_result.information_types, function(info_type) {
+ permissions.push([info_type, 'All']);
+ });
var self = this;
var y_config = {
on: {
@@ -283,7 +288,7 @@
parameters: {
pillar: pillar_uri,
sharee: person_uri,
- information_types: selection_result.information_types
+ permissions: permissions
}
};
this.get('lp_client').named_post(
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2012-03-09 05:04:21 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2012-03-13 03:10:25 +0000
@@ -282,7 +282,7 @@
expected_url = Y.lp.client.append_qs(
expected_url, 'sharee', person_uri);
expected_url = Y.lp.client.append_qs(
- expected_url, 'information_types', ['Policy 2']);
+ expected_url, 'permissions', 'Policy 2,All');
Y.Assert.areEqual(expected_url, mockio.last_request.config.data);
mockio.last_request.successJSON({
'resource_type_link': 'entity',
=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py 2012-03-08 08:01:44 +0000
+++ lib/lp/registry/model/accesspolicy.py 2012-03-13 03:10:25 +0000
@@ -15,6 +15,7 @@
from storm.expr import (
And,
Or,
+ SQL,
)
from storm.properties import (
DateTime,
@@ -333,8 +334,18 @@
def findGranteesByPolicy(cls, policies):
"""See `IAccessPolicyGrantFlatSource`."""
ids = [policy.id for policy in policies]
+ sharing_permission_term = SQL("""
+ CASE(
+ MIN(COALESCE(artifact, 0)))
+ WHEN 0 THEN 'ALL'
+ ELSE 'SOME'
+ END
+ """)
return IStore(cls).find(
- Person, Person.id == cls.grantee_id, cls.policy_id.is_in(ids))
+ (Person, AccessPolicy, sharing_permission_term),
+ Person.id == cls.grantee_id,
+ AccessPolicy.id == cls.policy_id,
+ cls.policy_id.is_in(ids)).group_by(Person, AccessPolicy)
@classmethod
def findArtifactsByGrantee(cls, grantee, policies):
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-03-12 06:30:28 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-03-13 03:10:25 +0000
@@ -81,37 +81,32 @@
@available_with_permission('launchpad.Driver', 'pillar')
def getPillarSharees(self, pillar):
"""See `ISharingService`."""
-
- # Currently support querying for sharing_permission = ALL
- # TODO - support querying for sharing_permission = SOME
-
policies = getUtility(IAccessPolicySource).findByPillar([pillar])
- policy_grant_source = getUtility(IAccessPolicyGrantSource)
- policy_grants = policy_grant_source.findByPolicy(policies)
+ ap_grant_flat = getUtility(IAccessPolicyGrantFlatSource)
+ grant_permissions = ap_grant_flat.findGranteesByPolicy(policies)
result = []
person_by_id = {}
request = get_current_web_service_request()
- for policy_grant in policy_grants:
- if not policy_grant.grantee.id in person_by_id:
- resource = EntryResource(policy_grant.grantee, request)
+ for (grantee, policy, sharing_permission) in grant_permissions:
+ if not grantee.id in person_by_id:
+ resource = EntryResource(grantee, request)
person_data = resource.toDataForJSON()
person_data['permissions'] = {}
- person_by_id[policy_grant.grantee.id] = person_data
+ person_by_id[grantee.id] = person_data
result.append(person_data)
- person_data = person_by_id[policy_grant.grantee.id]
- person_data['permissions'][policy_grant.policy.type.name] = (
- SharingPermission.ALL.name)
+ person_data = person_by_id[grantee.id]
+ person_data['permissions'][policy.type.name] = sharing_permission
return result
@available_with_permission('launchpad.Edit', 'pillar')
- def sharePillarInformation(self, pillar, sharee, information_types,
- user):
+ def sharePillarInformation(self, pillar, sharee, permissions, user):
"""See `ISharingService`."""
# We do not support adding sharees to project groups.
assert not IProjectGroup.providedBy(pillar)
+ information_types = permissions.keys()
pillar_info_types = [
(pillar, information_type)
for information_type in information_types]
@@ -147,10 +142,10 @@
request = get_current_web_service_request()
resource = EntryResource(sharee, request)
person_data = resource.toDataForJSON()
- permissions = {}
- for information_type in information_types:
- permissions[information_type.name] = SharingPermission.ALL.name
- person_data['permissions'] = permissions
+ permission_data = {}
+ for (information_type, permission) in permissions.items():
+ permission_data[information_type.name] = permission.name
+ person_data['permissions'] = permission_data
return person_data
@available_with_permission('launchpad.Edit', 'pillar')
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py 2012-03-12 06:18:13 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-03-13 03:10:25 +0000
@@ -46,14 +46,14 @@
super(TestSharingService, self).setUp()
self.service = getUtility(IService, 'sharing')
- def _makeShareeData(self, sharee, policy_types):
+ def _makeShareeData(self, sharee, policy_permissions):
# Unpack a sharee into its attributes and add in permissions.
request = get_current_web_service_request()
resource = EntryResource(sharee, request)
sharee_data = resource.toDataForJSON()
permissions = {}
- for policy in policy_types:
- permissions[policy.name] = SharingPermission.ALL.name
+ for (policy, permission) in policy_permissions:
+ permissions[policy.name] = permission.name
sharee_data['permissions'] = permissions
return sharee_data
@@ -104,11 +104,22 @@
pillar=pillar,
type=InformationType.PROPRIETARY)
grantee = self.factory.makePerson()
+ # Make access policy grant so that 'All' is returned.
self.factory.makeAccessPolicyGrant(access_policy, grantee)
- [sharee] = self.service.getPillarSharees(pillar)
- person_data = self._makeShareeData(
- grantee, [InformationType.PROPRIETARY])
- self.assertEqual(person_data, sharee)
+ # Make access artifact grants so that 'Some' is returned.
+ artifact_grant = self.factory.makeAccessArtifactGrant()
+ self.factory.makeAccessPolicyArtifact(
+ artifact=artifact_grant.abstract_artifact, policy=access_policy)
+
+ sharees = self.service.getPillarSharees(pillar)
+ expected_sharees = [
+ self._makeShareeData(
+ grantee,
+ [(InformationType.PROPRIETARY, SharingPermission.ALL)]),
+ self._makeShareeData(
+ artifact_grant.grantee,
+ [(InformationType.PROPRIETARY, SharingPermission.SOME)])]
+ self.assertContentEqual(expected_sharees, sharees)
def test_getProductSharees(self):
# Users with launchpad.Driver can view sharees.
@@ -164,21 +175,23 @@
policy, grantee=sharee, grantor=grantor)
# Now call sharePillarInformation will the grants we want.
- information_types = [
- InformationType.EMBARGOEDSECURITY,
- InformationType.USERDATA]
+ permissions = {
+ InformationType.EMBARGOEDSECURITY: SharingPermission.ALL,
+ InformationType.USERDATA: SharingPermission.SOME}
sharee_data = self.service.sharePillarInformation(
- pillar, sharee, information_types, grantor)
+ pillar, sharee, permissions, grantor)
policies = getUtility(IAccessPolicySource).findByPillar([pillar])
policy_grant_source = getUtility(IAccessPolicyGrantSource)
grants = policy_grant_source.findByPolicy(policies)
- self.assertEqual(grants.count(), len(information_types))
+ self.assertEqual(grants.count(), len(permissions))
for grant in grants:
self.assertEqual(grantor, grant.grantor)
self.assertEqual(sharee, grant.grantee)
- self.assertIn(grant.policy.type, information_types)
+ expected_permissions = [
+ (InformationType.EMBARGOEDSECURITY, SharingPermission.ALL),
+ (InformationType.USERDATA, SharingPermission.SOME)]
expected_sharee_data = self._makeShareeData(
- sharee, information_types)
+ sharee, expected_permissions)
self.assertEqual(expected_sharee_data, sharee_data)
def test_updateProjectGroupSharee_not_allowed(self):
@@ -189,7 +202,8 @@
login_person(owner)
self.assertRaises(
AssertionError, self.service.sharePillarInformation,
- project_group, sharee, [InformationType.USERDATA], owner)
+ project_group, sharee,
+ {InformationType.USERDATA: SharingPermission.ALL}, owner)
def test_updateProductSharee(self):
# Users with launchpad.Edit can add sharees.
@@ -206,13 +220,14 @@
self._test_sharePillarInformation(distro)
def _test_sharePillarInformationUnauthorized(self, pillar):
- # sharePillarInformation raises an Unauthorized exception if the user is
- # not permitted to do so.
+ # sharePillarInformation raises an Unauthorized exception if the user
+ # is not permitted to do so.
sharee = self.factory.makePerson()
user = self.factory.makePerson()
self.assertRaises(
Unauthorized, self.service.sharePillarInformation,
- pillar, sharee, [InformationType.USERDATA], user)
+ pillar, sharee,
+ {InformationType.USERDATA: SharingPermission.ALL}, user)
def test_sharePillarInformationAnonymous(self):
# Anonymous users are not allowed.
@@ -252,11 +267,14 @@
expected_information_types = (
set(information_types).difference(types_to_delete))
remaining_grantee_person_data = self._makeShareeData(
- grantee, expected_information_types)
+ grantee,
+ [(info_type, SharingPermission.ALL)
+ for info_type in expected_information_types])
+
expected_data.append(remaining_grantee_person_data)
# Add the data for the other sharee.
another_person_data = self._makeShareeData(
- another, information_types[:1])
+ another, [(information_types[0], SharingPermission.ALL)])
expected_data.append(another_person_data)
self.assertContentEqual(
expected_data, self.service.getPillarSharees(pillar))
@@ -297,19 +315,21 @@
def setUp(self):
super(ApiTestMixin, self).setUp()
- self.driver = self.factory.makePerson()
- self.pillar = self.factory.makeProduct(driver=self.driver)
- access_policy = self.factory.makeAccessPolicy(pillar=self.pillar)
+ self.owner = self.factory.makePerson()
+ self.pillar = self.factory.makeProduct(owner=self.owner)
self.grantee = self.factory.makePerson(name='grantee')
- self.factory.makeAccessPolicyGrant(
- policy=access_policy, grantee=self.grantee)
+ self.grantor = self.factory.makePerson()
+ self.grantee_uri = canonical_url(self.grantee, force_local_path=True)
+ self.grantor_uri = canonical_url(self.grantor, force_local_path=True)
transaction.commit()
def test_getPillarSharees(self):
# Test the getPillarSharees method.
[json_data] = self._getPillarSharees()
self.assertEqual('grantee', json_data['name'])
- self.assertIn('permissions', json_data)
+ self.assertEqual(
+ {InformationType.USERDATA.name: SharingPermission.ALL.name},
+ json_data['permissions'])
class TestWebService(ApiTestMixin, WebServiceTestCase):
@@ -319,6 +339,7 @@
super(TestWebService, self).setUp()
self.webservice = LaunchpadWebServiceCaller(
'launchpad-library', 'salgado-change-anything')
+ self._sharePillarInformation()
def test_url(self):
# Test that the url for the service is correct.
@@ -333,11 +354,25 @@
'/+services/sharing',
api_method, api_version='devel', **kwargs).jsonBody()
+ def _named_post(self, api_method, **kwargs):
+ return self.webservice.named_post(
+ '/+services/sharing',
+ api_method, api_version='devel', **kwargs).jsonBody()
+
def _getPillarSharees(self):
pillar_uri = canonical_url(self.pillar, force_local_path=True)
return self._named_get(
'getPillarSharees', pillar=pillar_uri)
+ def _sharePillarInformation(self):
+ pillar_uri = canonical_url(self.pillar, force_local_path=True)
+ return self._named_post(
+ 'sharePillarInformation', pillar=pillar_uri,
+ sharee=self.grantee_uri,
+ permissions={
+ InformationType.USERDATA.title: SharingPermission.ALL.title},
+ user=self.grantor_uri)
+
class TestLaunchpadlib(ApiTestMixin, TestCaseWithFactory):
"""Test launchpadlib access for the Sharing Service."""
@@ -346,13 +381,22 @@
def setUp(self):
super(TestLaunchpadlib, self).setUp()
- self.launchpad = self.factory.makeLaunchpadService(person=self.driver)
-
- def _getPillarSharees(self):
+ self.launchpad = self.factory.makeLaunchpadService(person=self.owner)
# XXX 2012-02-23 wallyworld bug 681767
# Launchpadlib can't do relative url's
- service = self.launchpad.load(
+ self.service = self.launchpad.load(
'%s/+services/sharing' % self.launchpad._root_uri)
- ws_pillar = ws_object(self.launchpad, self.pillar)
-# login_person(self.driver)
- return service.getPillarSharees(pillar=ws_pillar)
+ self._sharePillarInformation()
+
+ def _getPillarSharees(self):
+ ws_pillar = ws_object(self.launchpad, self.pillar)
+ return self.service.getPillarSharees(pillar=ws_pillar)
+
+ def _sharePillarInformation(self):
+ ws_pillar = ws_object(self.launchpad, self.pillar)
+ ws_grantee = ws_object(self.launchpad, self.grantee)
+ return self.service.sharePillarInformation(pillar=ws_pillar,
+ sharee=ws_grantee,
+ permissions={
+ InformationType.USERDATA.title: SharingPermission.ALL.title}
+ )
=== modified file 'lib/lp/registry/tests/test_accesspolicy.py'
--- lib/lp/registry/tests/test_accesspolicy.py 2012-03-09 04:10:32 +0000
+++ lib/lp/registry/tests/test_accesspolicy.py 2012-03-13 03:10:25 +0000
@@ -430,22 +430,28 @@
apgfs = getUtility(IAccessPolicyGrantFlatSource)
# People with grants on the policy show up.
+ policy_with_no_grantees = self.factory.makeAccessPolicy()
policy = self.factory.makeAccessPolicy()
policy_grant = self.factory.makeAccessPolicyGrant(policy=policy)
self.assertContentEqual(
- [policy_grant.grantee], apgfs.findGranteesByPolicy([policy]))
+ [(policy_grant.grantee, policy, 'ALL')],
+ apgfs.findGranteesByPolicy([policy, policy_with_no_grantees]))
# But not people with grants on artifacts.
artifact_grant = self.factory.makeAccessArtifactGrant()
self.assertContentEqual(
- [policy_grant.grantee], apgfs.findGranteesByPolicy([policy]))
+ [(policy_grant.grantee, policy, 'ALL')],
+ apgfs.findGranteesByPolicy([policy, policy_with_no_grantees]))
# Unless the artifacts are linked to the policy.
+ another_policy = self.factory.makeAccessPolicy()
self.factory.makeAccessPolicyArtifact(
- artifact=artifact_grant.abstract_artifact, policy=policy)
+ artifact=artifact_grant.abstract_artifact, policy=another_policy)
self.assertContentEqual(
- [policy_grant.grantee, artifact_grant.grantee],
- apgfs.findGranteesByPolicy([policy]))
+ [(policy_grant.grantee, policy, 'ALL'),
+ (artifact_grant.grantee, another_policy, 'SOME')],
+ apgfs.findGranteesByPolicy([
+ policy, another_policy, policy_with_no_grantees]))
def test_findArtifactsByGrantee(self):
# findArtifactsByGrantee() returns the artifacts for grantee for any of
=== modified file 'versions.cfg'
--- versions.cfg 2012-02-28 19:40:50 +0000
+++ versions.cfg 2012-03-13 03:10:25 +0000
@@ -39,7 +39,7 @@
lazr.delegates = 1.2.0
lazr.enum = 1.1.3
lazr.lifecycle = 1.1
-lazr.restful = 0.19.4
+lazr.restful = 0.19.5
lazr.restfulclient = 0.12.0
lazr.smtptest = 1.3
lazr.testing = 0.1.1
