launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #08890
lp:~wallyworld/launchpad/create-aag-privacy-transitions2-1009364 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/create-aag-privacy-transitions2-1009364 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1009364 in Launchpad itself: "AccessArtifactGrants not created on bug privacy transitions without triggers"
https://bugs.launchpad.net/launchpad/+bug/1009364
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/create-aag-privacy-transitions2-1009364/+merge/110706
== Implementation ==
This branch ensures that, with the legacy bug visibility triggers removed, when a bug transitions to private, any direct subscribers are granted access to the bug.
A new set of model methods was added - get/findPeopleWithoutAccess. For a given artifact and a list of people, it returns those who cannot see the artifact. This is used in the transitionToInformationType method on bug, to add AAGs from those subscribers who cannot see the bug.
A couple of issues were encountered along the way and fixed.
The IBugTaskSet.searchBugIds() method was returning duplicate ids in it's result. This was fixed by tweaking the bugtasksearch search_bugs() method.
There was duplicate code to disable triggers due to separate landings. I kept the one which was done as a test fixture since this is required for existing uses.
The code to check whether a bug is visible to a subscriber calls searchBugIds() which requires that the bug has a bugtask or else it returns false. The createBug() method added subscribers before creating the default bugtask and so public bugs were having AAG's created. I changed the order of the code to address this.
== Tests ==
Add tests to test_sharingservice and test_accesspolicy to test the new get/findPeopleWithoutAccess() methods.
Add a new test test_transition_to_private_grants_subscribers_access() to TestBugPrivateAndSecurityRelatedUpdatesMixin to check the core functionality of this mp.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/model/bug.py
lib/lp/bugs/model/bugtasksearch.py
lib/lp/bugs/model/tests/test_bug.py
lib/lp/bugs/model/tests/test_bugtask.py
lib/lp/code/model/branch.py
lib/lp/registry/interfaces/accesspolicy.py
lib/lp/registry/interfaces/sharingservice.py
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/create-aag-privacy-transitions2-1009364/+merge/110706
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/create-aag-privacy-transitions2-1009364 into lp:launchpad.
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2012-06-14 01:50:05 +0000
+++ lib/lp/bugs/model/bug.py 2012-06-18 01:27:23 +0000
@@ -842,13 +842,15 @@
# Ensure that the subscription has been flushed.
Store.of(sub).flush()
- # Grant the subscriber access if they can't see the bug.
- service = getUtility(IService, 'sharing')
- bugs, ignored = service.getVisibleArtifacts(person, bugs=[self])
- if not bugs:
- service.ensureAccessGrants(
- person, subscribed_by, bugs=[self],
- ignore_permissions=True)
+ # Grant the subscriber access if they can't see the bug but only if
+ # there is at least one bugtask for which access can be checked.
+ if self.default_bugtask:
+ service = getUtility(IService, 'sharing')
+ bugs, ignored = service.getVisibleArtifacts(person, bugs=[self])
+ if not bugs:
+ service.ensureAccessGrants(
+ [person], subscribed_by, bugs=[self],
+ ignore_permissions=True)
# In some cases, a subscription should be created without
# email notifications. suppress_notify determines if
@@ -1804,6 +1806,21 @@
self.information_type = information_type
self._reconcileAccess()
+
+ # If the transition makes the bug private, then we need to ensure all
+ # subscribers can see the bug. For any who can't, we create an access
+ # artifact grant.
+ if information_type in PRIVATE_INFORMATION_TYPES:
+ # Grant the subscriber access if they can't see the bug.
+ service = getUtility(IService, 'sharing')
+ subscribers = self.getDirectSubscribers()
+ blind_subscribers = service.getPeopleWithoutAccess(
+ self, subscribers)
+ if blind_subscribers.count():
+ service.ensureAccessGrants(
+ blind_subscribers, who, bugs=[self],
+ ignore_permissions=True)
+
self.updateHeat()
flag = 'disclosure.unsubscribe_jobs.enabled'
@@ -2673,6 +2690,19 @@
bug, event = self.createBugWithoutTarget(params)
+ # Create the task on a product if one was passed.
+ if params.product:
+ getUtility(IBugTaskSet).createTask(
+ bug, params.owner, params.product, status=params.status)
+
+ # Create the task on a source package name if one was passed.
+ if params.distribution:
+ target = params.distribution
+ if params.sourcepackagename:
+ target = target.getSourcePackage(params.sourcepackagename)
+ getUtility(IBugTaskSet).createTask(
+ bug, params.owner, target, status=params.status)
+
if params.information_type in SECURITY_INFORMATION_TYPES:
if params.product:
context = params.product
@@ -2695,19 +2725,6 @@
else:
bug.subscribe(params.product.owner, params.owner)
- # Create the task on a product if one was passed.
- if params.product:
- getUtility(IBugTaskSet).createTask(
- bug, params.owner, params.product, status=params.status)
-
- # Create the task on a source package name if one was passed.
- if params.distribution:
- target = params.distribution
- if params.sourcepackagename:
- target = target.getSourcePackage(params.sourcepackagename)
- getUtility(IBugTaskSet).createTask(
- bug, params.owner, target, status=params.status)
-
bug_task = bug.default_bugtask
if params.assignee:
bug_task.transitionToAssignee(params.assignee)
=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py 2012-06-15 05:21:29 +0000
+++ lib/lp/bugs/model/bugtasksearch.py 2012-06-18 01:27:23 +0000
@@ -224,6 +224,7 @@
start = BugTaskFlat
if just_bug_ids:
want = BugTaskFlat.bug_id
+ orderby_expression = want
else:
want = BugTaskFlat.bugtask_id
decorators.append(lambda id: IStore(BugTask).get(BugTask, id))
@@ -275,6 +276,8 @@
result = store.using(*origin).find(want)
result.order_by(orderby_expression)
+ if just_bug_ids:
+ result.config(distinct=True)
return DecoratedResultSet(
result,
lambda row: reduce(lambda task, dec: dec(task), decorators, row),
=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py 2012-06-14 05:18:22 +0000
+++ lib/lp/bugs/model/tests/test_bug.py 2012-06-18 01:27:23 +0000
@@ -16,6 +16,7 @@
from zope.security.proxy import removeSecurityProxy
from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.app.interfaces.services import IService
from lp.bugs.adapters.bugchange import BugTitleChange
from lp.bugs.enums import (
BugNotificationLevel,
@@ -48,7 +49,7 @@
StormStatementRecorder,
TestCaseWithFactory,
)
-from lp.testing.dbtriggers import triggers_disabled
+from lp.testing.fixture import DisableTriggerFixture
from lp.testing.layers import DatabaseFunctionalLayer
from lp.testing.matchers import (
Equals,
@@ -57,11 +58,15 @@
)
-LEGACY_ACCESS_TRIGGERS = [
- ('bug', 'bug_mirror_legacy_access_t'),
- ('bugtask', 'bugtask_mirror_legacy_access_t'),
- ('bugsubscription', 'bugsubscription_mirror_legacy_access_t'),
- ]
+def disable_trigger_fixture():
+ # XXX 2012-05-22 wallyworld bug=1002596
+ # No need to use this fixture when triggers are removed.
+ return DisableTriggerFixture(
+ {'bugsubscription':
+ 'bugsubscription_mirror_legacy_access_t',
+ 'bug': 'bug_mirror_legacy_access_t',
+ 'bugtask': 'bugtask_mirror_legacy_access_t',
+ })
class TestBug(TestCaseWithFactory):
@@ -746,6 +751,24 @@
subscribers_before_public,
subscribers_after_public)
+ def test_transition_to_private_grants_subscribers_access(self):
+ # When a bug is made private, any direct subscribers should be granted
+ # access.
+ (bug, bug_owner, bugtask_a, bugtask_b, default_bugtask) = (
+ self.createBugTasksAndSubscribers())
+ some_person = self.factory.makePerson()
+ with disable_trigger_fixture():
+ with person_logged_in(bug_owner):
+ bug.subscribe(some_person, bug_owner)
+ subscribers = bug.getDirectSubscribers()
+ who = self.factory.makePerson(name='who')
+ bug.transitionToInformationType(
+ InformationType.USERDATA, who)
+
+ service = getUtility(IService, 'sharing')
+ peopleWithoutAccess = service.getPeopleWithoutAccess(bug, subscribers)
+ self.assertContentEqual([], peopleWithoutAccess)
+
def test_setPillarOwnerSubscribedIfNoBugSupervisor(self):
# The pillar owner is subscribed if the bug supervisor is not set and
# the bug is marked as USERDATA.
@@ -835,7 +858,7 @@
# There are also transitional triggers that do this. Disable
# them temporarily so we can be sure the application side works.
- with triggers_disabled(LEGACY_ACCESS_TRIGGERS):
+ with disable_trigger_fixture():
with admin_logged_in():
product = bug.default_bugtask.product
bug.transitionToInformationType(
=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py 2012-06-15 00:42:38 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py 2012-06-18 01:27:23 +0000
@@ -59,7 +59,6 @@
_build_status_clause,
_build_tag_search_clause,
)
-from lp.bugs.model.tests.test_bug import LEGACY_ACCESS_TRIGGERS
from lp.bugs.scripts.bugtasktargetnamecaches import (
BugTaskTargetNameCacheUpdater,
)
@@ -123,7 +122,6 @@
TestCaseWithFactory,
ws_object,
)
-from lp.testing.dbtriggers import triggers_disabled
from lp.testing.dbuser import (
dbuser,
switch_dbuser,
@@ -242,7 +240,7 @@
# There are also transitional triggers that do this. Disable
# them temporarily so we can be sure the application side works.
- with triggers_disabled(LEGACY_ACCESS_TRIGGERS):
+ with disable_trigger_fixture():
with admin_logged_in():
old_product = bug.default_bugtask.product
getUtility(IBugTaskSet).createManyTasks(
@@ -2209,7 +2207,7 @@
# There are also transitional triggers that do this. Disable
# them temporarily so we can be sure the application side works.
- with triggers_disabled(LEGACY_ACCESS_TRIGGERS):
+ with disable_trigger_fixture():
with admin_logged_in():
task.delete()
@@ -3290,7 +3288,7 @@
# There are also transitional triggers that do this. Disable
# them temporarily so we can be sure the application side works.
- with triggers_disabled(LEGACY_ACCESS_TRIGGERS):
+ with disable_trigger_fixture():
with admin_logged_in():
bug.default_bugtask.transitionToTarget(
new_product, new_product.owner)
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2012-06-14 09:09:59 +0000
+++ lib/lp/code/model/branch.py 2012-06-18 01:27:23 +0000
@@ -17,7 +17,6 @@
import pytz
import simplejson
from sqlobject import (
- BoolCol,
ForeignKey,
IntCol,
SQLMultipleJoin,
@@ -157,7 +156,6 @@
SQLBase,
sqlvalues,
)
-from lp.services.features import getFeatureFlag
from lp.services.helpers import shortlist
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.model.job import Job
@@ -837,7 +835,7 @@
person, branches=[self])
if not branches:
service.ensureAccessGrants(
- subscribed_by, person, branches=[self],
+ [person], subscribed_by, branches=[self],
ignore_permissions=True)
return subscription
=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py 2012-06-15 07:53:21 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py 2012-06-18 01:27:23 +0000
@@ -267,3 +267,10 @@
:param grantee: the access artifact grantee.
:param policies: a collection of `IAccessPolicy`s.
"""
+
+ def findPeopleWithoutAccess(artifact, people):
+ """Find the people without access to the specified artifact.
+
+ :param artifact: the access artifact for which to check access.
+ :param people: the people for whom to check access.
+ """
=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py 2012-06-08 02:16:58 +0000
+++ lib/lp/registry/interfaces/sharingservice.py 2012-06-18 01:27:23 +0000
@@ -70,6 +70,17 @@
:return: a collection of artifacts the person can see.
"""
+ def getPeopleWithoutAccess(artifact, people):
+ """Return the people who cannot access an artifact.
+
+ Given a list of people, return those who do not have access to the
+ specified bug or branch.
+
+ :param artifact: the bug or branch whose access is being checked.
+ :param people: the people whose access is being checked.
+ :return: a collection of people without access to the artifact.
+ """
+
def getInformationTypes(pillar):
"""Return the allowed information types for the given pillar."""
@@ -165,16 +176,17 @@
@export_write_operation()
@call_with(user=REQUEST_USER)
@operation_parameters(
- sharee=Reference(IPerson, title=_('Sharee'), required=True),
+ sharees=List(
+ Reference(IPerson, title=_('Sharee'), required=True)),
bugs=List(
Reference(schema=IBug), title=_('Bugs'), required=False),
branches=List(
Reference(schema=IBranch), title=_('Branches'), required=False))
@operation_for_version('devel')
- def ensureAccessGrants(sharee, user, branches=None, bugs=None):
+ def ensureAccessGrants(sharees, user, branches=None, bugs=None):
"""Ensure a sharee has an access grant to the specified artifacts.
- :param sharee: the person or team for whom to grant access
+ :param sharees: the people or teams for whom to grant access
:param user: the user making the request
:param bugs: the bugs for which to grant access
:param branches: the branches for which to grant access
=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py 2012-06-15 07:53:21 +0000
+++ lib/lp/registry/model/accesspolicy.py 2012-06-18 01:27:23 +0000
@@ -15,11 +15,11 @@
from collections import defaultdict
import pytz
-from storm import Undef
from storm.expr import (
And,
In,
Join,
+ Not,
Or,
Select,
SQL,
@@ -559,3 +559,28 @@
AccessArtifact.id == cls.abstract_artifact_id,
cls.grantee_id == grantee.id,
cls.policy_id.is_in(ids))
+
+ @classmethod
+ def findPeopleWithoutAccess(cls, artifact, people):
+ """See `IAccessPolicyGrantFlatSource`."""
+ person_ids = [person.id for person in people]
+ person_filter = TeamParticipation.personID.is_in(person_ids)
+
+ store = IStore(cls)
+ with_expr = With("grantees", store.find(
+ cls.grantee_id,
+ cls.abstract_artifact_id == artifact.id)
+ .config(distinct=True)._get_select())
+ grantee_select = (
+ Select(
+ (TeamParticipation.personID,),
+ tables=(TeamParticipation, Join("grantees",
+ SQL("grantees.grantee = TeamParticipation.team"))),
+ where=person_filter,
+ distinct=True))
+ result_set = store.with_(with_expr).find(
+ Person,
+ Not(In(Person.id, grantee_select)),
+ In(Person.id, person_ids))
+
+ return result_set
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-06-15 01:47:33 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-06-18 01:27:23 +0000
@@ -127,6 +127,15 @@
return visible_bugs, visible_branches
+ def getPeopleWithoutAccess(self, artifact, people):
+ """See `ISharingService`."""
+ ap_grant_flat = getUtility(IAccessPolicyGrantFlatSource)
+ access_artifacts = list(
+ getUtility(IAccessArtifactSource).find([artifact]))
+ return (
+ access_artifacts and
+ ap_grant_flat.findPeopleWithoutAccess(access_artifacts[0], people))
+
def getInformationTypes(self, pillar):
"""See `ISharingService`."""
allowed_types = [
@@ -365,7 +374,7 @@
# XXX 2012-06-13 wallyworld bug=1012448
# Remove branch subscriptions when information type fully implemented.
- def ensureAccessGrants(self, sharee, user, branches=None, bugs=None,
+ def ensureAccessGrants(self, sharees, user, branches=None, bugs=None,
ignore_permissions=False):
"""See `ISharingService`."""
@@ -391,9 +400,9 @@
artifacts_with_grants = [
artifact_grant.abstract_artifact
for artifact_grant in
- aagsource.find([(artifact, sharee) for artifact in artifacts])]
+ aagsource.find(product(artifacts, sharees))]
# Create access to bugs/branches for the specified sharee for which a
# grant does not already exist.
missing_artifacts = set(artifacts) - set(artifacts_with_grants)
getUtility(IAccessArtifactGrantSource).grant(
- list(product(missing_artifacts, [sharee], [user])))
+ list(product(missing_artifacts, sharees, [user])))
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py 2012-06-15 01:47:33 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-06-18 01:27:23 +0000
@@ -895,7 +895,7 @@
grantee = self.factory.makePerson()
with FeatureFixture(WRITE_FLAG):
self.service.ensureAccessGrants(
- grantee, user, bugs=bugs, branches=branches)
+ [grantee], user, bugs=bugs, branches=branches)
# Check that grantee has expected access grants.
shared_bugs = []
@@ -966,7 +966,7 @@
with FeatureFixture(WRITE_FLAG):
self.assertRaises(
Unauthorized, self.service.ensureAccessGrants,
- user, sharee, bugs=[bug])
+ [sharee], user, bugs=[bug])
def test_ensureAccessGrantsAnonymous(self):
# Anonymous users are not allowed.
@@ -991,7 +991,7 @@
login_person(owner)
self.assertRaises(
Unauthorized, self.service.ensureAccessGrants,
- sharee, product.owner, bugs=[bug])
+ [sharee], product.owner, bugs=[bug])
def test_getSharedArtifacts(self):
# Test the getSharedArtifacts method.
@@ -1051,6 +1051,35 @@
self.assertContentEqual(bug_tasks[:9], shared_bugtasks)
self.assertContentEqual(branches[:9], shared_branches)
+ def test_getPeopleWithoutAccess(self):
+ # Test the getPeopleWithoutAccess method.
+ owner = self.factory.makePerson()
+ product = self.factory.makeProduct(owner=owner)
+ login_person(owner)
+
+ # Make some people to check.
+ people = []
+ for x in range(0, 10):
+ person = self.factory.makePerson()
+ people.append(person)
+ bug = self.factory.makeBug(
+ product=product, owner=owner,
+ information_type=InformationType.USERDATA)
+ access_artifact = self.factory.makeAccessArtifact(concrete=bug)
+
+ def grant_access(person):
+ self.factory.makeAccessArtifactGrant(
+ artifact=access_artifact, grantee=person, grantor=owner)
+ return access_artifact
+
+ # Grant access to some of the people.
+ for person in people[:5]:
+ grant_access(person)
+
+ # Check the results.
+ without_access = self.service.getPeopleWithoutAccess(bug, people)
+ self.assertContentEqual(people[5:], without_access)
+
def test_getVisibleArtifacts(self):
# Test the getVisibleArtifacts method.
owner = self.factory.makePerson()
=== modified file 'lib/lp/registry/tests/test_accesspolicy.py'
--- lib/lp/registry/tests/test_accesspolicy.py 2012-06-15 07:53:21 +0000
+++ lib/lp/registry/tests/test_accesspolicy.py 2012-06-18 01:27:23 +0000
@@ -666,6 +666,20 @@
self.assertContentEqual(
[artifact], self.apgfs.findArtifactsByGrantee(grantee, [policy]))
+ def test_findPeopleWithoutAccess(self):
+ # findPeopleWithoutAccess() returns the people who do not have access
+ # to an artifact.
+ policy = self.factory.makeAccessPolicy()
+ artifact = self.factory.makeAccessArtifact()
+ grantee_with_access = self.factory.makePerson()
+ grantee_without_access = self.factory.makePerson()
+ self.factory.makeAccessArtifactGrant(artifact, grantee_with_access)
+ self.factory.makeAccessPolicyArtifact(artifact=artifact, policy=policy)
+ self.assertContentEqual(
+ [grantee_without_access],
+ self.apgfs.findPeopleWithoutAccess(
+ artifact, [grantee_with_access, grantee_without_access]))
+
class TestReconcileAccessPolicyArtifacts(TestCaseWithFactory):
=== removed file 'lib/lp/testing/dbtriggers.py'
--- lib/lp/testing/dbtriggers.py 2012-05-30 08:29:21 +0000
+++ lib/lp/testing/dbtriggers.py 1970-01-01 00:00:00 +0000
@@ -1,55 +0,0 @@
-# Copyright 2011 Canonical Ltd. This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Provides helpers for managing database triggers in tests."""
-
-__metaclass__ = type
-
-__all__ = [
- 'disable_trigger',
- 'enable_trigger',
- 'triggers_disabled',
- ]
-
-from contextlib import contextmanager
-
-from zope.component import getUtility
-
-from lp.services.database.sqlbase import quote_identifier
-from lp.services.webapp.interfaces import (
- IStoreSelector,
- MAIN_STORE,
- MASTER_FLAVOR,
- )
-from lp.testing.dbuser import dbuser
-
-
-def disable_trigger(table, trigger):
- store = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
- store.execute(
- 'ALTER TABLE %s DISABLE TRIGGER %s'
- % (quote_identifier(table), quote_identifier(trigger)))
-
-
-def enable_trigger(table, trigger):
- store = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
- store.execute(
- 'ALTER TABLE %s ENABLE TRIGGER %s'
- % (quote_identifier(table), quote_identifier(trigger)))
-
-
-@contextmanager
-def triggers_disabled(triggers):
- """A context manager that temporarily disables selected triggers.
-
- This commits and starts a new transaction.
-
- :param triggers: sequence of (table, trigger) tuples to disable.
- """
- with dbuser('postgres'):
- for trigger in triggers:
- disable_trigger(*trigger)
- yield
- with dbuser('postgres'):
- for trigger in triggers:
- enable_trigger(*trigger)
Follow ups
