launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/hidden-comments-are-userdata into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/hidden-comments-are-userdata/+merge/114531

Summary
=======
We recently determined that hidden comments on bugs can be considered
USERDATA. This vastly simplifies the rules for who can see/modify the
comment--it should all be controlled via access to the USERDATA policy, or
artifact access to the bug.

Preimp
======
Spoke with Curtis Hovey.

Implementation
==============
There is a check now that uses project roles to expand access. We replace that
with a check that ensures the user has USERDATA access to a bug pillar, or has
artifact access to the pillar.

Tests
=====
bin/test -vvct bugcomment

QA
==
Check bug comments as someone without access at all, as the comment owner, as
someone with USERDATA access, and someone with artifact access. You should be
able to see things as anyone but the no access at all person.

LoC
===
This is part of disclosure.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/tests/test_bugcomment.py
  lib/lp/bugs/model/bug.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/hidden-comments-are-userdata/+merge/114531
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/hidden-comments-are-userdata into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/tests/test_bugcomment.py'
--- lib/lp/bugs/browser/tests/test_bugcomment.py	2012-07-07 15:26:29 +0000
+++ lib/lp/bugs/browser/tests/test_bugcomment.py	2012-07-11 22:19:23 +0000
@@ -16,6 +16,7 @@
     HTMLContains,
     Tag,
     )
+from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.bugs.browser.bugcomment import (
@@ -27,6 +28,10 @@
     TestHideMessageControlMixin,
     TestMessageVisibilityMixin,
     )
+from lp.registry.enums import InformationType
+from lp.registry.interfaces.accesspolicy import (
+    IAccessPolicySource,
+    )
 from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.publisher import canonical_url
 from lp.services.webapp.testing import verifyObject
@@ -262,36 +267,25 @@
         context = self.getContext(comment_owner=owner)
         self._test_hide_link_visible(context, owner)
 
-    def test_pillar_owner_sees_hide_control(self):
-        # The pillar owner sees the hide control.
-        person = self.factory.makePerson()
-        context = self.getContext()
-        naked_bugtask = removeSecurityProxy(context.default_bugtask)
-        removeSecurityProxy(naked_bugtask.pillar).owner = person
-        self._test_hide_link_visible(context, person)
-
-    def test_pillar_driver_sees_hide_control(self):
-        # The pillar driver sees the hide control.
-        person = self.factory.makePerson()
-        context = self.getContext()
-        naked_bugtask = removeSecurityProxy(context.default_bugtask)
-        removeSecurityProxy(naked_bugtask.pillar).driver = person
-        self._test_hide_link_visible(context, person)
-
-    def test_pillar_bug_supervisor_sees_hide_control(self):
-        # The pillar bug supervisor sees the hide control.
-        person = self.factory.makePerson()
-        context = self.getContext()
-        naked_bugtask = removeSecurityProxy(context.default_bugtask)
-        removeSecurityProxy(naked_bugtask.pillar).bug_supervisor = person
-        self._test_hide_link_visible(context, person)
-
-    def test_pillar_security_contact_sees_hide_control(self):
-        # The pillar security contact sees the hide control.
-        person = self.factory.makePerson()
-        context = self.getContext()
-        naked_bugtask = removeSecurityProxy(context.default_bugtask)
-        removeSecurityProxy(naked_bugtask.pillar).security_contact = person
+    def test_userdata_grant_sees_hide_control(self):
+        # The pillar owner sees the hide control.
+        person = self.factory.makePerson()
+        context = self.getContext()
+        pillar = context.default_bugtask.product
+        policy = getUtility(IAccessPolicySource).find(
+            [(pillar, InformationType.USERDATA)])
+        self.factory.makeAccessPolicyGrant(
+            policy=policy, grantor=pillar.owner, grantee=person)
+        self._test_hide_link_visible(context, person)
+
+    def test_artifact_grant_sees_hide_control(self):
+        # The pillar owner sees the hide control.
+        person = self.factory.makePerson()
+        context = self.getContext()
+        pillar = context.default_bugtask.product
+        artifact = self.factory.makeAccessArtifact(concrete=context)
+        self.factory.makeAccessArtifactGrant(
+            artifact=artifact, grantor=pillar.owner, grantee=person)
         self._test_hide_link_visible(context, person)
 
 

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-07-10 03:42:05 +0000
+++ lib/lp/bugs/model/bug.py	2012-07-11 22:19:23 +0000
@@ -166,6 +166,8 @@
 from lp.registry.interfaces.accesspolicy import (
     IAccessArtifactGrantSource,
     IAccessArtifactSource,
+    IAccessPolicyGrantSource,
+    IAccessPolicySource,
     )
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distroseries import IDistroSeries
@@ -2061,26 +2063,36 @@
 
     def userCanSetCommentVisibility(self, user):
         """See `IBug`"""
-
         if user is None:
             return False
+        # Admins and registry experts always have permission.
         roles = IPersonRoles(user)
         if roles.in_admin or roles.in_registry_experts:
             return True
+        # Those who can access user data for the bug have permission too.
         flag = 'disclosure.users_hide_own_bug_comments.enabled'
-        return bool(getFeatureFlag(flag)) and self.userInProjectRole(roles)
+        return bool(getFeatureFlag(flag)) and self.userCanAccessUserData(user)
 
-    def userInProjectRole(self, user):
-        """ Return True if user has a project role for any affected pillar."""
-        roles = IPersonRoles(user)
-        if roles is None:
+    def userCanAccessUserData(self, user):
+        """ Return True if the user has access to USER_DATA data."""
+        # Check if the user has access via the pillar.
+        pillars = list(self.affected_pillars)
+        pillars_and_types = [(p, InformationType.USERDATA) for p in pillars]
+        access_policies = getUtility(IAccessPolicySource).find(
+            pillars_and_types)
+        access_grants = [(a, user) for a in access_policies]
+        access_grants = getUtility(IAccessPolicyGrantSource).find(
+            access_grants)
+        if not access_grants.is_empty():
+            return True
+        # User has no access via the pillars, check the bug itself.
+        artifact = getUtility(IAccessArtifactSource).find([self]).one()
+        if  artifact is None:
             return False
-        for pillar in self.affected_pillars:
-            if (roles.isOwner(pillar)
-                or roles.isOneOfDrivers(pillar)
-                or roles.isBugSupervisor(pillar)
-                or roles.isSecurityContact(pillar)):
-                return True
+        artifact_access_grant = getUtility(IAccessArtifactGrantSource).find(
+            [(artifact, user)]).one()
+        if artifact_access_grant is not None:
+            return True
         return False
 
     def linkHWSubmission(self, submission):