launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/use-access-grants-for-specifications-auth-check into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/use-access-grants-for-specifications-auth-check/+merge/123774

This branch changes the metod SPeicifcation.userCanView() so that it
uses access grants to check if a user may view a given specification.

For non-public specifications, this requires an access grant on either
the specification's target or on the specification itself.

The related tests required some changes. Most importantly, calling
specification.transitionToInformationType() raises currently
Unauthorized because the owner of a specification does not get at present
automatically a view grant if the specification is made non-public.

This should be fixed of course, but can be done in another branch --
it does not matter here, if transitionToInformationType() is called by
a product owner or a specification owner.

The test test_special_user_read_access() is now obsolete because
userCanView() does no longer check a user's role in the project.

test:
./bin/test -vvt    lp.blueprints.tests.test_specification

no lint

-- 
https://code.launchpad.net/~adeuring/launchpad/use-access-grants-for-specifications-auth-check/+merge/123774
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/use-access-grants-for-specifications-auth-check into lp:launchpad.

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2012-09-10 08:30:42 +0000
+++ lib/lp/blueprints/model/specification.py	2012-09-11 15:36:19 +0000
@@ -25,6 +25,13 @@
     SQLRelatedJoin,
     StringCol,
     )
+from storm.expr import (
+    And,
+    In,
+    Join,
+    Or,
+    Select,
+    )
 from storm.locals import (
     Desc,
     SQL,
@@ -78,6 +85,7 @@
 from lp.registry.interfaces.person import validate_public_person
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.productseries import IProductSeries
+from lp.registry.model.teammembership import TeamParticipation
 from lp.services.database.constants import (
     DEFAULT,
     UTC_NOW,
@@ -838,21 +846,62 @@
     def private(self):
         return self.information_type in PRIVATE_INFORMATION_TYPES
 
+    @cachedproperty
+    def _known_viewers(self):
+        """A set of known persons able to view the specifcation."""
+        return set()
+
     def userCanView(self, user):
         """See `ISpecification`."""
+        # Avoid circular imports.
+        from lp.registry.model.accesspolicy import (
+            AccessArtifact,
+            AccessPolicy,
+            AccessPolicyGrantFlat,
+            )
         if self.information_type in PUBLIC_INFORMATION_TYPES:
             return True
         if user is None:
             return False
-        # Temporary: we should access the grant tables instead of
-        # checking if a given user has special roles.
-        # The following is basically copied from
-        # EditSpecificationByRelatedPeople.checkAuthenticated()
-        return (user.in_admin or
-                user.isOwner(self.target) or
-                user.isOneOfDrivers(self.target) or
-                user.isOneOf(
-                    self, ['owner', 'drafter', 'assignee', 'approver']))
+        if user.id in self._known_viewers:
+            return True
+
+        # Check if access has been granted to the user for either
+        # the pillar of this specification or the specification
+        # itself.
+        #
+        # A DB constraint ensures that either Specification.product or
+        # Specification.distribution is not null.
+        if self.product is not None:
+            pillar_clause = AccessPolicy.product == self.productID
+        else:
+            pillar_clause = AccessPolicy.distribution == self.distributionID
+        tables = (
+            AccessPolicyGrantFlat,
+            Join(
+                AccessPolicy,
+                AccessPolicyGrantFlat.policy_id == AccessPolicy.id),
+            Join(
+                TeamParticipation,
+                AccessPolicyGrantFlat.grantee == TeamParticipation.teamID)
+            )
+        grants = Store.of(self).using(*tables).find(
+            AccessPolicyGrantFlat,
+            pillar_clause,
+            Or(
+                And(
+                    AccessPolicyGrantFlat.abstract_artifact == None,
+                    AccessPolicy.type == self.information_type),
+                In(
+                    AccessPolicyGrantFlat.abstract_artifact_id,
+                    Select(
+                        AccessArtifact.id,
+                        AccessArtifact.specification_id == self.id))),
+            TeamParticipation.personID == user.id)
+        if grants.is_empty():
+            return False
+        self._known_viewers.add(user.id)
+        return True
 
 
 class HasSpecificationsMixin:

=== modified file 'lib/lp/blueprints/tests/test_specification.py'
--- lib/lp/blueprints/tests/test_specification.py	2012-09-10 10:20:39 +0000
+++ lib/lp/blueprints/tests/test_specification.py	2012-09-11 15:36:19 +0000
@@ -18,6 +18,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.interfaces.security import IAuthorization
+from lp.app.interfaces.services import IService
 from lp.blueprints.enums import (
     NewSpecificationDefinitionStatus,
     SpecificationDefinitionStatus,
@@ -26,8 +27,10 @@
 from lp.blueprints.errors import TargetAlreadyHasSpecification
 from lp.blueprints.interfaces.specification import ISpecificationSet
 from lp.registry.enums import (
+    InformationType,
     PRIVATE_INFORMATION_TYPES,
     PUBLIC_INFORMATION_TYPES,
+    SharingPermission,
     )
 from lp.security import (
     AdminSpecification,
@@ -35,6 +38,7 @@
     EditWhiteboardSpecification,
     ViewSpecification,
     )
+from lp.services.propertycache import get_property_cache
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.interaction import ANONYMOUS
 from lp.testing import (
@@ -256,7 +260,7 @@
             PRIVATE_INFORMATION_TYPES)
         all_types = specification.getAllowedInformationTypes(ANONYMOUS)
         for information_type in all_types:
-            with person_logged_in(specification.owner):
+            with person_logged_in(specification.target.owner):
                 specification.transitionToInformationType(
                     information_type, specification.owner)
             error_expected = information_type not in PUBLIC_INFORMATION_TYPES
@@ -270,7 +274,7 @@
             PRIVATE_INFORMATION_TYPES)
         all_types = specification.getAllowedInformationTypes(ANONYMOUS)
         for information_type in all_types:
-            with person_logged_in(specification.owner):
+            with person_logged_in(specification.target.owner):
                 specification.transitionToInformationType(
                     information_type, specification.owner)
             self.write_access_to_ISpecificationView(
@@ -289,7 +293,7 @@
         user = self.factory.makePerson()
         all_types = specification.getAllowedInformationTypes(user)
         for information_type in all_types:
-            with person_logged_in(specification.owner):
+            with person_logged_in(specification.target.owner):
                 specification.transitionToInformationType(
                     information_type, specification.owner)
             error_expected = information_type not in PUBLIC_INFORMATION_TYPES
@@ -306,7 +310,7 @@
         user = self.factory.makePerson()
         all_types = specification.getAllowedInformationTypes(user)
         for information_type in all_types:
-            with person_logged_in(specification.owner):
+            with person_logged_in(specification.target.owner):
                 specification.transitionToInformationType(
                     information_type, specification.owner)
             error_expected = information_type not in PUBLIC_INFORMATION_TYPES
@@ -317,20 +321,74 @@
                 user, specification, error_expected=True,
                 attribute='name', value='foo')
 
-    def test_special_user_read_access(self):
-        # Users with special privileges can aceess the attributes
-        # of public and private specifcations.
-        specification = self.factory.makeSpecification()
-        removeSecurityProxy(specification.target)._ensurePolicies(
-            PRIVATE_INFORMATION_TYPES)
-        all_types = specification.getAllowedInformationTypes(
-            specification.owner)
-        for information_type in all_types:
-            with person_logged_in(specification.owner):
-                specification.transitionToInformationType(
-                    information_type, specification.owner)
-            self.read_access_to_ISpecificationView(
-                specification.owner, specification, error_expected=False)
+    def test_user_with_grant_for_target_read_access(self):
+        # Users with a grant for the specification's target
+        # have access to a specification if the information_type
+        # of the specification matches the type if the grant.
+        specification = self.factory.makeSpecification()
+        removeSecurityProxy(specification.target)._ensurePolicies(
+            PRIVATE_INFORMATION_TYPES)
+        user = self.factory.makePerson()
+        permissions = {
+            InformationType.PROPRIETARY: SharingPermission.ALL,
+            }
+        with person_logged_in(specification.target.owner):
+            getUtility(IService, 'sharing').sharePillarInformation(
+                specification.target, user, specification.target.owner,
+                permissions)
+        all_types = specification.getAllowedInformationTypes(user)
+        for information_type in all_types:
+            with person_logged_in(specification.target.owner):
+                specification.transitionToInformationType(
+                    information_type, specification.owner)
+            error_expected = (
+                information_type not in PUBLIC_INFORMATION_TYPES and
+                information_type not in permissions)
+            self.read_access_to_ISpecificationView(
+                user, specification, error_expected)
+            del get_property_cache(specification)._known_viewers
+
+    def test_user_with_grant_for_specification_read_access(self):
+        # Users with a grant for the specification have access to this
+        # specification.
+        specification = self.factory.makeSpecification()
+        removeSecurityProxy(specification.target)._ensurePolicies(
+            PRIVATE_INFORMATION_TYPES)
+        user = self.factory.makePerson()
+        with person_logged_in(specification.target.owner):
+            getUtility(IService, 'sharing').ensureAccessGrants(
+                [user], specification.target.owner,
+                specifications=[specification], ignore_permissions=True)
+        all_types = specification.getAllowedInformationTypes(user)
+        for information_type in all_types:
+            with person_logged_in(specification.target.owner):
+                specification.transitionToInformationType(
+                    information_type, specification.owner)
+            self.read_access_to_ISpecificationView(
+                user, specification, error_expected=False)
+
+    def test_user_with_grant_for_specification_write_access(self):
+        # Users with a grant for the specification can change the whiteboard
+        # but no other attributes.
+        specification = self.factory.makeSpecification()
+        removeSecurityProxy(specification.target)._ensurePolicies(
+            PRIVATE_INFORMATION_TYPES)
+        user = self.factory.makePerson()
+        with person_logged_in(specification.target.owner):
+            getUtility(IService, 'sharing').ensureAccessGrants(
+                [user], specification.target.owner,
+                specifications=[specification], ignore_permissions=True)
+        all_types = specification.getAllowedInformationTypes(user)
+        for information_type in all_types:
+            with person_logged_in(specification.target.owner):
+                specification.transitionToInformationType(
+                    information_type, specification.owner)
+            self.write_access_to_ISpecificationView(
+                user, specification, error_expected=False,
+                attribute='whiteboard', value='foo')
+            self.write_access_to_ISpecificationView(
+                user, specification, error_expected=True,
+                attribute='name', value='foo')
 
     def test_special_user_write_access(self):
         # Users with special privileges can change the attributes
@@ -341,15 +399,15 @@
         all_types = specification.getAllowedInformationTypes(
             specification.owner)
         for information_type in all_types:
-            with person_logged_in(specification.owner):
+            with person_logged_in(specification.target.owner):
                 specification.transitionToInformationType(
                     information_type, specification.owner)
             self.write_access_to_ISpecificationView(
-                specification.owner, specification, error_expected=False,
-                attribute='whiteboard', value='foo')
+                specification.target.owner, specification,
+                error_expected=False, attribute='whiteboard', value='foo')
             self.write_access_to_ISpecificationView(
-                specification.owner, specification, error_expected=False,
-                attribute='name', value='foo')
+                specification.target.owner, specification,
+                error_expected=False, attribute='name', value='foo')
 
 
 class TestSpecificationSet(TestCaseWithFactory):

=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py	2012-09-04 10:35:22 +0000
+++ lib/lp/registry/model/accesspolicy.py	2012-09-11 15:36:19 +0000
@@ -10,6 +10,7 @@
     'AccessPolicy',
     'AccessPolicyArtifact',
     'AccessPolicyGrant',
+    'AccessPolicyGrantFlat',
     'reconcile_access_for_artifact',
     ]