launchpad-reviewers team mailing list archive

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

j.c.sackett has proposed merging lp:~jcsackett/launchpad/userdata-check-is-borked into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/userdata-check-is-borked/+merge/116540

Summary
=======
The current checks for setting comment visibility use a helper method,
`userCanAccessUserData` which checks for a UserData access policy grant for
the user or an artifact grant.

Both are wrong.

Artifact grants shouldn't play into comments, b/c then anyone with access to
the bug has access to the comment, which defeats the point of hiding comments.
And the policy check isn't team aware, so it'll almost always deny access.

This removes that helper method and just uses the sharingservice to determine
if access is available.

Preimp
======
Spoke with William Grant.

Implementation
==============
The helper method is removed. In the one call site for the method, access for
the user is checked using the sharing service, which already has a method for
checking access on a person based on pillar and information type.

Some tests were updated, and the tests for artifacts were removed.

Tests
=====
bin/test -vvc -t comment -t bug_message

QA
==
Make sure access to comments and comment controls works as expected.

LoC
===
This is negative LoC.

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
  lib/lp/bugs/tests/test_bug_messages.py
  lib/lp/bugs/tests/test_bug_messages_webservice.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/userdata-check-is-borked/+merge/116540
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/userdata-check-is-borked into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/tests/test_bugcomment.py'
--- lib/lp/bugs/browser/tests/test_bugcomment.py	2012-07-17 15:32:15 +0000
+++ lib/lp/bugs/browser/tests/test_bugcomment.py	2012-07-24 20:26:23 +0000
@@ -269,16 +269,6 @@
             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)
-
 
 class TestBugCommentMicroformats(BrowserTestCase):
 

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-07-19 04:40:03 +0000
+++ lib/lp/bugs/model/bug.py	2012-07-24 20:26:23 +0000
@@ -2067,11 +2067,18 @@
         roles = IPersonRoles(user)
         if roles.in_admin or roles.in_registry_experts:
             return True
-        return self.userCanAccessUserData(user)
+        pillars = list(self.affected_pillars)
+        service = getUtility(IService, 'sharing')
+        for pillar in pillars:
+            if service.checkPillarAccess(
+                    pillar, InformationType.USERDATA, user):
+                return True
+        return False
 
     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(

=== modified file 'lib/lp/bugs/tests/test_bug_messages.py'
--- lib/lp/bugs/tests/test_bug_messages.py	2012-07-17 15:32:15 +0000
+++ lib/lp/bugs/tests/test_bug_messages.py	2012-07-24 20:26:23 +0000
@@ -77,12 +77,3 @@
         self.factory.makeAccessPolicyGrant(
             policy=policy, grantor=product.owner, grantee=person)
         self.assertTrue(bug.userCanSetCommentVisibility(person))
-
-    def test_artifact_grant_can_toggle_comment_visibility(self):
-        person = self.factory.makePerson()
-        product = self.factory.makeProduct()
-        bug = self.factory.makeBug(product=product)
-        artifact = self.factory.makeAccessArtifact(concrete=bug)
-        self.factory.makeAccessArtifactGrant(
-            artifact=artifact, grantor=product.owner, grantee=person)
-        self.assertTrue(bug.userCanSetCommentVisibility(person))

=== modified file 'lib/lp/bugs/tests/test_bug_messages_webservice.py'
--- lib/lp/bugs/tests/test_bug_messages_webservice.py	2012-07-24 06:39:54 +0000
+++ lib/lp/bugs/tests/test_bug_messages_webservice.py	2012-07-24 20:26:23 +0000
@@ -135,10 +135,3 @@
         self.factory.makeAccessPolicyGrant(
             policy=policy, grantor=pillar.owner, grantee=person)
         self._test_hide_comment(person)
-
-    def test_artifact_grantee_can_set_visible(self):
-        person = self.factory.makePerson()
-        artifact = self.factory.makeAccessArtifact(concrete=self.bug)
-        pillar = removeSecurityProxy(self.bug.default_bugtask).pillar
-        self.factory.makeAccessArtifactGrant(
-            artifact=artifact, grantor=pillar.owner, grantee=person)