launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07731
lp:~wallyworld/launchpad/legacy-unsubscribe-revokes-access-997425 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/legacy-unsubscribe-revokes-access-997425 into lp:launchpad with lp:~wallyworld/launchpad/revoke-membership-delete-subscriptions-996882 as a prerequisite.
Requested reviews:
Curtis Hovey (sinzui)
Related bugs:
Bug #997425 in Launchpad itself: "Remove access when someone is unsubscribed from a bug or branch"
https://bugs.launchpad.net/launchpad/+bug/997425
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/legacy-unsubscribe-revokes-access-997425/+merge/105312
== Implementation ==
This branch starts the process of moving code to mirror bug subscriptions and bug visibility. Eventually subscriptions and visibility/access will be divorced, but we want to ease into that situation. We someone is unsubscribed from a bug, eventually we would not normally want to also revoke access. But for now we will. This is currently done by triggers but we want to delete those. So this branch adds the unsubscribe->revoke code to the model and puts it behind a feature flag so we can coordinate when it is turned on with the trigger removal.
To test, I needed a way to disable the triggers. So I added a new fixture, which allows selected triggers to be disabled as required. The fixture is DisableTriggerFixture.
== Tests ==
Added test for the new fixture:
bin/test -vvct TestDisableTriggerFixture
Added new tests for the bug unsubscribe->revoke code:
- test_privateBugUnsubscribeRevokesVisibility
- test_privateBugUnsubscribeRetainsVisibility
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/model/bug.py
lib/lp/bugs/tests/test_bugvisibility.py
lib/lp/services/features/flags.py
lib/lp/testing/fixture.py
lib/lp/testing/tests/test_fixture.py
--
https://code.launchpad.net/~wallyworld/launchpad/legacy-unsubscribe-revokes-access-997425/+merge/105312
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2012-05-09 06:12:52 +0000
+++ lib/lp/bugs/model/bug.py 2012-05-10 11:42:21 +0000
@@ -162,6 +162,10 @@
PRIVATE_INFORMATION_TYPES,
SECURITY_INFORMATION_TYPES,
)
+from lp.registry.interfaces.accesspolicy import (
+ IAccessArtifactGrantSource,
+ IAccessArtifactSource,
+ )
from lp.registry.interfaces.distribution import IDistribution
from lp.registry.interfaces.distroseries import IDistroSeries
from lp.registry.interfaces.person import (
@@ -880,6 +884,14 @@
store.flush()
self.updateHeat()
del get_property_cache(self)._known_viewers
+
+ # Revoke access to bug if feature flag is on.
+ flag = 'disclosure.legacy_subscription_visibility.enabled'
+ if bool(getFeatureFlag(flag)):
+ artifacts_to_delete = getUtility(
+ IAccessArtifactSource).find([self])
+ getUtility(IAccessArtifactGrantSource).revokeByArtifact(
+ artifacts_to_delete, [person])
return
def unsubscribeFromDupes(self, person, unsubscribed_by):
=== modified file 'lib/lp/bugs/tests/test_bugvisibility.py'
--- lib/lp/bugs/tests/test_bugvisibility.py 2012-05-09 06:12:52 +0000
+++ lib/lp/bugs/tests/test_bugvisibility.py 2012-05-10 11:42:21 +0000
@@ -4,21 +4,26 @@
"""Tests for visibility of a bug."""
from lp.registry.enums import InformationType
+from lp.services.features.testing import FeatureFixture
from lp.testing import (
celebrity_logged_in,
TestCaseWithFactory,
)
+from lp.testing.fixture import DisableTriggerFixture
from lp.testing.layers import (
DatabaseFunctionalLayer,
- LaunchpadFunctionalLayer,
+ ZopelessDatabaseLayer,
)
+LEGACY_VISIBILITY_FLAG = {
+ u"disclosure.legacy_subscription_visibility.enabled": u"true"}
+
+
class TestPublicBugVisibility(TestCaseWithFactory):
"""Test visibility for a public bug."""
layer = DatabaseFunctionalLayer
- #layer = LaunchpadFunctionalLayer
def setUp(self):
super(TestPublicBugVisibility, self).setUp()
@@ -38,7 +43,7 @@
class TestPrivateBugVisibility(TestCaseWithFactory):
"""Test visibility for a private bug."""
- layer = LaunchpadFunctionalLayer
+ layer = ZopelessDatabaseLayer
def setUp(self):
super(TestPrivateBugVisibility, self).setUp()
@@ -78,6 +83,43 @@
self.bug.subscribe(user, self.owner)
self.assertTrue(self.bug.userCanView(user))
- def test_publicBugAnonUser(self):
+ def test_privateBugAnonUser(self):
# Since the bug is private, the anonymous user cannot see it.
self.assertFalse(self.bug.userCanView(None))
+
+ @property
+ def disable_trigger_fixture(self):
+ return DisableTriggerFixture(
+ {'bugsubscription':
+ 'bugsubscription_mirror_legacy_access_t',
+ 'bug': 'bug_mirror_legacy_access_t',
+ 'bugtask': 'bugtask_mirror_legacy_access_t',
+ })
+
+ def test_privateBugUnsubscribeRevokesVisibility(self):
+ # A person unsubscribed from a private bug can no longer see it.
+ # Requires feature flag since the default model behaviour is to leave
+ # any access grants untouched.
+ # This test disables the current temporary database triggers which
+ # mirror subscription status into visibility.
+ with FeatureFixture(LEGACY_VISIBILITY_FLAG):
+ user = self.factory.makePerson()
+ with celebrity_logged_in('admin'):
+ self.bug.subscribe(user, self.owner)
+ self.assertTrue(self.bug.userCanView(user))
+ with self.disable_trigger_fixture:
+ self.bug.unsubscribe(user, self.owner)
+ self.assertFalse(self.bug.userCanView(user))
+
+ def test_privateBugUnsubscribeRetainsVisibility(self):
+ # A person unsubscribed from a private bug can still see it if the
+ # feature flag to enable legacy subscription visibility is not set.
+ # This test disables the current temporary database triggers which
+ # mirror subscription status into visibility.
+ user = self.factory.makePerson()
+ with celebrity_logged_in('admin'):
+ self.bug.subscribe(user, self.owner)
+ self.assertTrue(self.bug.userCanView(user))
+ with self.disable_trigger_fixture:
+ self.bug.unsubscribe(user, self.owner)
+ self.assertTrue(self.bug.userCanView(user))
=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py 2012-05-08 01:31:10 +0000
+++ lib/lp/services/features/flags.py 2012-05-10 11:42:21 +0000
@@ -281,6 +281,13 @@
'',
'',
''),
+ ('disclosure.legacy_subscription_visibility.enabled',
+ 'boolean',
+ ('If true, the legacy behaviour of unsubscribing from a bug or branch'
+ 'revokes access.'),
+ '',
+ '',
+ ''),
('disclosure.enhanced_sharing.writable',
'boolean',
('If true, will allow the use of the new sharing view and apis used '
=== modified file 'lib/lp/testing/fixture.py'
--- lib/lp/testing/fixture.py 2012-05-08 21:04:08 +0000
+++ lib/lp/testing/fixture.py 2012-05-10 11:42:21 +0000
@@ -7,6 +7,7 @@
__all__ = [
'CaptureOops',
'DemoMode',
+ 'DisableTriggerFixture',
'PGBouncerFixture',
'PGNotReadyError',
'Urllib2Fixture',
@@ -25,7 +26,6 @@
EnvironmentVariableFixture,
Fixture,
)
-import itertools
from lazr.restful.utils import get_current_browser_request
import oops
import oops_amqp
@@ -41,6 +41,7 @@
from zope.component import (
adapter,
getGlobalSiteManager,
+ getUtility,
provideHandler,
)
from zope.interface import Interface
@@ -57,6 +58,12 @@
from lp.services.messaging.rabbit import connect
from lp.services.timeline.requesttimeline import get_request_timeline
from lp.services.webapp.errorlog import ErrorReportEvent
+from lp.services.webapp.interfaces import (
+ DEFAULT_FLAVOR,
+ MAIN_STORE,
+ IStoreSelector,
+ )
+from lp.testing.dbuser import dbuser
class PGNotReadyError(Exception):
@@ -132,7 +139,7 @@
"""Start PGBouncer, waiting for it to accept connections if neccesary.
"""
super(PGBouncerFixture, self).start()
- s = socket.socket()
+ socket.socket()
for i in xrange(retries):
try:
socket.create_connection((self.host, self.port))
@@ -146,7 +153,6 @@
raise PGNotReadyError("Not ready after %d attempts." % retries)
-
class ZopeAdapterFixture(Fixture):
"""A fixture to register and unregister an adapter."""
@@ -388,3 +394,33 @@
<a href="http://example.com";>File a bug</a>.
''')
self.addCleanup(lambda: config.pop('demo-fixture'))
+
+
+class DisableTriggerFixture(Fixture):
+ """Let tests disable database triggers."""
+
+ def __init__(self, table_triggers=None):
+ self.table_triggers = table_triggers or {}
+
+ def setUp(self):
+ super(DisableTriggerFixture, self).setUp()
+ self._disable_triggers()
+ self.addCleanup(self._enable_triggers)
+
+ def _process_triggers(self, mode):
+ store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+ with dbuser('postgres'):
+ for table, trigger in self.table_triggers.items():
+ sql = ("ALTER TABLE %(table)s %(mode)s trigger "
+ "%(trigger)s") % {
+ 'table': table,
+ 'mode': mode,
+ 'trigger': trigger,
+ }
+ store.execute(sql)
+
+ def _disable_triggers(self):
+ self._process_triggers(mode='DISABLE')
+
+ def _enable_triggers(self):
+ self._process_triggers(mode='ENABLE')
=== modified file 'lib/lp/testing/tests/test_fixture.py'
--- lib/lp/testing/tests/test_fixture.py 2012-01-18 07:35:31 +0000
+++ lib/lp/testing/tests/test_fixture.py 2012-05-10 11:42:21 +0000
@@ -36,6 +36,7 @@
from lp.testing import TestCase
from lp.testing.fixture import (
CaptureOops,
+ DisableTriggerFixture,
PGBouncerFixture,
ZopeAdapterFixture,
ZopeUtilityFixture,
@@ -45,6 +46,7 @@
DatabaseLayer,
LaunchpadLayer,
LaunchpadZopelessLayer,
+ ZopelessDatabaseLayer,
)
@@ -263,3 +265,62 @@
capture = self.useFixture(CaptureOops())
capture.sync()
capture.sync()
+
+
+class TestDisableTriggerFixture(TestCase):
+ """Test the DisableTriggerFixture class."""
+
+ layer = ZopelessDatabaseLayer
+
+ def setUp(self):
+ super(TestDisableTriggerFixture, self).setUp()
+ con_str = dbconfig.rw_main_master + ' user=launchpad_main'
+ con = psycopg2.connect(con_str)
+ con.set_isolation_level(0)
+ self.cursor = con.cursor()
+ # Create a test table and trigger.
+ setup_sql = """
+ CREATE OR REPLACE FUNCTION trig() RETURNS trigger
+ LANGUAGE plpgsql
+ AS
+ 'BEGIN
+ update test_trigger set col_b = NEW.col_a
+ where col_a = NEW.col_a;
+ RETURN NULL;
+ END;';
+ DROP TABLE IF EXISTS test_trigger CASCADE;
+ CREATE TABLE test_trigger(col_a integer, col_b integer);
+ CREATE TRIGGER test_trigger_t
+ AFTER INSERT on test_trigger
+ FOR EACH ROW EXECUTE PROCEDURE trig();
+ """
+ self.cursor.execute(setup_sql)
+ self.addCleanup(self._cleanup)
+
+ def _cleanup(self):
+ self.cursor.execute('DROP TABLE test_trigger CASCADE;')
+ self.cursor.close()
+ self.cursor.connection.close()
+
+ def test_triggers_are_disabled(self):
+ # Test that the fixture correctly disables specified triggers.
+ with DisableTriggerFixture({'test_trigger': 'test_trigger_t'}):
+ self.cursor.execute(
+ 'INSERT INTO test_trigger(col_a) values (1)')
+ self.cursor.execute(
+ 'SELECT col_b FROM test_trigger WHERE col_a = 1')
+ [col_b] = self.cursor.fetchone()
+ self.assertEqual(None, col_b)
+
+ def test_triggers_are_enabled_after(self):
+ # Test that the fixture correctly enables the triggers again when it
+ # is done.
+ with DisableTriggerFixture({'test_trigger': 'test_trigger_t'}):
+ self.cursor.execute(
+ 'INSERT INTO test_trigger(col_a) values (1)')
+ self.cursor.execute(
+ 'INSERT INTO test_trigger(col_a) values (2)')
+ self.cursor.execute(
+ 'SELECT col_b FROM test_trigger WHERE col_a = 2')
+ [col_b] = self.cursor.fetchone()
+ self.assertEqual(2, col_b)
Follow ups
