Merge lp:~wgrant/launchpad/bug-mirror-access-policies into lp:launchpad/db-devel

[William Grant]

Triggers to mirror legacy bug disclosure into the new access policy schema.

This branch adds a PL/pgSQL function to mirror legacy bug disclosure (Bug, BugTask, BugSubscription) to the new schema (AccessArtifact, AccessArtifactGrant, AccessPolicyArtifact), and triggers to handle all updates. As discussed with stub last week, this is to let us safely gradually migrate to the new schema -- most of the triggers and function will go away once the migration is complete.

The mapping should be reasonably clear. If the bug is private, the set of AccessPolicyArtifacts is synced with the corresponding policies of the BugTasks' pillars, and the set of AccessArtifactGrants is synced with the BugSubscriptions. If the bug is public, anything that exists is removed.

This hasn't been tested on the stagings, only dogfood. On DF it performs at an average of ~0.8ms per bug when doing an initial population pass over every bug.

There are tests for most of the trigger and function behaviour.

[Stuart Bishop (stub)]

On Mon, Feb 27, 2012 at 1:36 PM, William Grant <email address hidden> wrote:

> +        INSERT INTO AccessArtifactGrant
> +            (artifact, grantee, grantor, date_created)
> +            SELECT DISTINCT ON (artifact_id, BugSubscription.person)
> +                   artifact_id, BugSubscription.person,
> +                   BugSubscription.subscribed_by, BugSubscription.date_created
> +                FROM
> +                    BugSubscription
> +                    LEFT JOIN AccessArtifactGrant
> +                        ON (AccessArtifactGrant.grantee =
> +                                BugSubscription.person
> +                            AND AccessArtifactGrant.artifact = artifact_id)
> +                WHERE
> +                    AccessArtifactGrant.grantee IS NULL
> +                    AND BugSubscription.bug = bug_id;

You need an ORDER BY clause here for the SELECT DISTINCT ON. I think
"ORDER BY artifact_id, BugSubscription.person,
BugSubscription.date_created".

Tests look good.

--
Stuart Bishop <email address hidden>

[Stuart Bishop (stub)]

review: Approve (db + tests)

=== added file 'database/schema/patch-2209-11-3.sql'
--- database/schema/patch-2209-11-3.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2209-11-3.sql	2012-02-27 09:40:26 +0000
@@ -0,0 +1,192 @@
+-- Copyright 2012 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+
+SET client_min_messages=ERROR;
+
+-- Magical superfunction to bring a bug's new access data into consistency
+-- with its legacy data.
+
+CREATE OR REPLACE FUNCTION bug_mirror_legacy_access(bug_id integer) RETURNS void
+    LANGUAGE plpgsql SECURITY DEFINER SET search_path = public
+    AS $$
+DECLARE
+    bug_row record;
+    artifact_id integer;
+    bugtask_row record;
+    policy_type integer;
+    pillars record;
+    access_policies integer[];
+BEGIN
+    SELECT * INTO bug_row FROM bug WHERE id = bug_id;
+    SELECT id INTO artifact_id FROM AccessArtifact WHERE bug = bug_id;
+    IF NOT bug_row.private THEN
+        IF artifact_id IS NOT NULL THEN
+            -- Bug is public, but there are access control rows. Destroy them.
+            DELETE FROM AccessArtifactGrant WHERE artifact = artifact_id;
+            DELETE FROM AccessPolicyArtifact WHERE artifact = artifact_id;
+            DELETE FROM AccessArtifact WHERE id = artifact_id;
+        END IF;
+    ELSE
+        -- Bug is private. Add missing rows, remove superfluous ones.
+
+        -- Ensure that there's a corresponding AccessArtifact.
+        IF artifact_id IS NULL THEN
+            INSERT INTO AccessArtifact (bug) VALUES (bug_row.id)
+                RETURNING id INTO artifact_id;
+        END IF;
+
+        -- Ensure that the AccessArtifactGrants match BugSubscriptions.
+        DELETE FROM AccessArtifactGrant
+            WHERE
+                artifact = artifact_id
+                AND grantee NOT IN (
+                    SELECT person FROM bugsubscription WHERE bug = bug_id);
+        INSERT INTO AccessArtifactGrant
+            (artifact, grantee, grantor, date_created)
+            SELECT DISTINCT ON (artifact_id, BugSubscription.person)
+                   artifact_id, BugSubscription.person,
+                   BugSubscription.subscribed_by, BugSubscription.date_created
+                FROM
+                    BugSubscription
+                    LEFT JOIN AccessArtifactGrant
+                        ON (AccessArtifactGrant.grantee =
+                                BugSubscription.person
+                            AND AccessArtifactGrant.artifact = artifact_id)
+                WHERE
+                    AccessArtifactGrant.grantee IS NULL
+                    AND BugSubscription.bug = bug_id
+                ORDER BY
+                    artifact_id,
+                    BugSubscription.person,
+                    BugSubscription.date_created;
+
+        -- Ensure that AccessPolicyArtifacts match the implied policy
+        -- type and the tasks' pillars.
+        SELECT (CASE
+                    WHEN NOT bug_row.security_related THEN 4
+                    WHEN bug_row.security_related THEN 3
+                END) INTO policy_type;
+        SELECT
+            array_agg(
+                DISTINCT COALESCE(bugtask.product, productseries.product))
+                AS products,
+            array_agg(
+                DISTINCT COALESCE(bugtask.distribution,
+                                  distroseries.distribution))
+                AS distributions
+            INTO pillars
+            FROM
+                bugtask
+                LEFT JOIN productseries
+                    ON productseries.id = bugtask.productseries
+                LEFT JOIN distroseries
+                    ON distroseries.id = bugtask.distroseries
+            WHERE bug = bug_id;
+        SELECT array_agg(id) FROM AccessPolicy
+            INTO access_policies
+            WHERE 
+                type = policy_type
+                AND (
+                    (product IS NOT NULL AND product = ANY(pillars.products))
+                    OR (distribution IS NOT NULL
+                        AND distribution = ANY(pillars.distributions)));
+        DELETE FROM AccessPolicyArtifact
+            WHERE
+                artifact = artifact_id
+            AND policy != ALL(access_policies);
+        INSERT INTO AccessPolicyArtifact
+            (artifact, policy)
+            SELECT DISTINCT artifact_id, policy
+                FROM unnest(access_policies) AS policy
+            EXCEPT
+            SELECT artifact_id, policy FROM AccessPolicyArtifact
+                WHERE artifact = artifact_id;
+    END IF;
+END;
+$$;
+
+-- Bug triggers
+
+CREATE OR REPLACE FUNCTION bug_mirror_legacy_access_trig() RETURNS trigger
+    LANGUAGE plpgsql
+    AS $$
+BEGIN
+    IF TG_OP = 'INSERT' THEN
+        PERFORM bug_mirror_legacy_access(NEW.id);
+    ELSIF TG_OP = 'UPDATE' THEN
+        IF NEW.private != OLD.private OR NEW.security_related != OLD.security_related THEN
+            PERFORM bug_mirror_legacy_access(OLD.id);
+        END IF;
+    END IF;
+    RETURN NULL;
+END;
+$$;
+
+
+DROP TRIGGER IF EXISTS bug_mirror_legacy_access_t ON bug;
+CREATE TRIGGER bug_mirror_legacy_access_t
+    AFTER INSERT OR UPDATE ON bug
+    FOR EACH ROW EXECUTE PROCEDURE bug_mirror_legacy_access_trig();
+
+
+-- BugTask triggers
+
+CREATE OR REPLACE FUNCTION bugtask_mirror_legacy_access_trig() RETURNS trigger
+    LANGUAGE plpgsql
+    AS $$
+BEGIN
+    IF TG_OP = 'INSERT' THEN
+        PERFORM bug_mirror_legacy_access(NEW.bug);
+    ELSIF TG_OP = 'UPDATE' THEN
+        IF NEW.bug != OLD.bug THEN
+            RAISE EXCEPTION 'cannot move bugtask to a different bug';
+        END IF;
+        IF (NEW.distribution IS DISTINCT FROM OLD.distribution
+            OR NEW.product IS DISTINCT FROM OLD.product
+            OR NEW.distroseries IS DISTINCT FROM OLD.distroseries
+            OR NEW.productseries IS DISTINCT FROM OLD.productseries) THEN
+            PERFORM bug_mirror_legacy_access(OLD.bug);
+        END IF;
+    ELSIF TG_OP = 'DELETE' THEN
+        PERFORM bug_mirror_legacy_access(OLD.bug);
+    END IF;
+    RETURN NULL;
+END;
+$$;
+
+
+DROP TRIGGER IF EXISTS bugtask_mirror_legacy_access_t ON bugtask;
+CREATE TRIGGER bugtask_mirror_legacy_access_t
+    AFTER INSERT OR UPDATE OR DELETE ON bugtask
+    FOR EACH ROW EXECUTE PROCEDURE bugtask_mirror_legacy_access_trig();
+
+
+-- BugSubscription triggers
+
+CREATE OR REPLACE FUNCTION bugsubscription_mirror_legacy_access_trig() RETURNS trigger
+    LANGUAGE plpgsql
+    AS $$
+BEGIN
+    IF TG_OP = 'INSERT' THEN
+        PERFORM bug_mirror_legacy_access(NEW.bug);
+    ELSIF TG_OP = 'UPDATE' THEN
+        IF NEW.bug != OLD.bug THEN
+            RAISE EXCEPTION 'cannot move bugsubscription to a different bug';
+        END IF;
+        IF NEW.person != OLD.person THEN
+            PERFORM bug_mirror_legacy_access(OLD.bug);
+        END IF;
+    ELSIF TG_OP = 'DELETE' THEN
+        PERFORM bug_mirror_legacy_access(OLD.bug);
+    END IF;
+    RETURN NULL;
+END;
+$$;
+
+DROP TRIGGER IF EXISTS
+    bugsubscription_mirror_legacy_access_t ON bugsubscription;
+CREATE TRIGGER bugsubscription_mirror_legacy_access_t
+    AFTER INSERT OR UPDATE OR DELETE ON bugsubscription
+    FOR EACH ROW EXECUTE PROCEDURE bugsubscription_mirror_legacy_access_trig();
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2209, 11, 3);

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2012-02-27 09:40:25 +0000
+++ database/schema/security.cfg	2012-02-27 09:40:26 +0000
@@ -15,6 +15,7 @@
 public.add_test_openid_identifier(integer) = EXECUTE
 public.alllocks                            =
 public.assert_patch_applied(integer, integer, integer) = EXECUTE
+public.bug_mirror_legacy_access(integer)   =
 public.bug_update_latest_patch_uploaded(integer) =
 public.bugnotificationarchive              =
 public.calculate_bug_heat(integer)         = EXECUTE

=== added file 'lib/lp/bugs/tests/test_bug_mirror_access_triggers.py'
--- lib/lp/bugs/tests/test_bug_mirror_access_triggers.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/tests/test_bug_mirror_access_triggers.py	2012-02-27 09:40:26 +0000
@@ -0,0 +1,213 @@
+# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from storm.store import Store
+from testtools.matchers import (
+    MatchesSetwise,
+    MatchesStructure,
+    )
+from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
+
+from lp.registry.enums import AccessPolicyType
+from lp.registry.interfaces.accesspolicy import (
+    IAccessArtifactGrantSource,
+    IAccessArtifactSource,
+    IAccessPolicySource,
+    IAccessPolicyArtifactSource,
+    )
+from lp.services.features.testing import FeatureFixture
+from lp.testing import TestCaseWithFactory
+from lp.testing.layers import DatabaseFunctionalLayer
+
+
+class TestBugMirrorAccessTriggers(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBugMirrorAccessTriggers, self).setUp()
+        self.useFixture(FeatureFixture(
+            {'disclosure.allow_multipillar_private_bugs.enabled': 'true'}))
+
+    def assertMirrored(self, bug):
+        """Check that a bug has been correctly mirrored to the new schema.
+
+        :return: A tuple of
+            (AccessArtifactGrant count, AccessPolicyArtifact count) for
+            additional checking.
+        """
+        # An AccessPolicyArtifact exists.
+        artifact = getUtility(IAccessArtifactSource).find([bug]).one()
+        self.assertIsNot(None, artifact.concrete_artifact)
+
+        # There is an AccessArtifactGrant for the subscriber.
+        subs = bug.getDirectSubscriptions()
+        grants = list(
+            getUtility(IAccessArtifactGrantSource).findByArtifact(
+                [artifact]))
+        self.assertThat(
+            grants,
+            MatchesSetwise(*[
+                MatchesStructure.byEquality(
+                    grantee=sub.person, grantor=sub.subscribed_by,
+                    date_created=sub.date_created) for sub in subs]))
+        grant_count = len(grants)
+
+        if removeSecurityProxy(bug).security_related:
+            policy_type = AccessPolicyType.EMBARGOEDSECURITY
+        else:
+            policy_type = AccessPolicyType.USERDATA
+
+        # Get the relevant access policies, confirming that there's one
+        # for every pillar.
+        pillars = set(
+            task.pillar for task in removeSecurityProxy(bug).bugtasks)
+        expected_policies = list(getUtility(IAccessPolicySource).find(
+            [(pillar, policy_type) for pillar in pillars]))
+        self.assertEqual(len(pillars), len(expected_policies))
+
+        # There are AccessPolicyArtifacts for each relevant policy.
+        policies = self.getPoliciesForArtifact(artifact)
+        self.assertContentEqual(expected_policies, policies)
+        policy_count = len(policies)
+
+        # And return some counts so the test can see we looked things up
+        # properly.
+        return grant_count, policy_count
+
+    def makeBugAndPolicies(self, private=False, policy_types=None):
+        if policy_types is None:
+            policy_types = [
+                AccessPolicyType.USERDATA, AccessPolicyType.EMBARGOEDSECURITY]
+        product = self.factory.makeProduct()
+        for policy_type in policy_types:
+            self.factory.makeAccessPolicy(pillar=product, type=policy_type)
+        bug = self.factory.makeBug(private=private, product=product)
+        return removeSecurityProxy(bug)
+
+    def getPoliciesForArtifact(self, artifact):
+        return set(
+            apa.policy for apa in
+            getUtility(IAccessPolicyArtifactSource).findByArtifact(
+                [artifact]))
+
+    def getPolicyTypesForArtifact(self, artifact):
+        return set(
+            policy.type for policy in self.getPoliciesForArtifact(artifact))
+
+    def test_public_has_nothing(self):
+        bug = self.factory.makeBug(private=False)
+        artifact = getUtility(IAccessArtifactSource).find([bug]).one()
+        self.assertIs(None, artifact)
+
+    def test_private(self):
+        bug = self.makeBugAndPolicies(private=True)
+        self.assertEqual((1, 1), self.assertMirrored(bug))
+
+    def test_add_subscriber(self):
+        bug = self.makeBugAndPolicies(private=True)
+        person = self.factory.makePerson()
+        bug.subscribe(person, person)
+        self.assertEqual((2, 1), self.assertMirrored(bug))
+
+    def test_remove_subscriber(self):
+        bug = self.makeBugAndPolicies(private=True)
+        person = self.factory.makePerson()
+        bug.subscribe(person, person)
+        Store.of(bug).flush()
+        bug.unsubscribe(person, person)
+        self.assertEqual((1, 1), self.assertMirrored(bug))
+
+    def test_add_task(self):
+        # Adding a task on a new product links its policy.
+        product = self.factory.makeProduct()
+        self.factory.makeAccessPolicy(
+            pillar=product, type=AccessPolicyType.USERDATA)
+
+        bug = self.makeBugAndPolicies(private=True)
+        bug.addTask(bug.owner, product)
+        self.assertEqual((1, 2), self.assertMirrored(bug))
+
+    def test_remove_task(self):
+        # Removing a task removes its policy.
+        product = self.factory.makeProduct()
+        self.factory.makeAccessPolicy(
+            pillar=product, type=AccessPolicyType.USERDATA)
+
+        bug = self.makeBugAndPolicies(private=True)
+        task = bug.addTask(bug.owner, product)
+        Store.of(bug).flush()
+        removeSecurityProxy(task).destroySelf()
+        self.assertEqual((1, 1), self.assertMirrored(bug))
+
+    def test_make_public(self):
+        bug = self.makeBugAndPolicies(private=True)
+        self.assertIsNot(
+            None, getUtility(IAccessArtifactSource).find([bug]).one())
+        bug.private = False
+        self.assertIs(
+            None, getUtility(IAccessArtifactSource).find([bug]).one())
+
+    def test_make_private(self):
+        bug = self.makeBugAndPolicies(private=False)
+        self.assertIs(
+            None, getUtility(IAccessArtifactSource).find([bug]).one())
+        bug.private = True
+        self.assertIsNot(
+            None, getUtility(IAccessArtifactSource).find([bug]).one())
+        self.assertEqual((1, 1), self.assertMirrored(bug))
+
+    def test_security_related(self):
+        # Setting the security_related flag uses EMBARGOEDSECURITY
+        # policies instead of USERDATA.
+        bug = self.makeBugAndPolicies(private=True)
+        [artifact] = getUtility(IAccessArtifactSource).find([bug])
+        self.assertEqual((1, 1), self.assertMirrored(bug))
+        self.assertContentEqual(
+            [AccessPolicyType.USERDATA],
+            self.getPolicyTypesForArtifact(artifact))
+        bug.security_related = True
+        self.assertEqual((1, 1), self.assertMirrored(bug))
+        self.assertContentEqual(
+            [AccessPolicyType.EMBARGOEDSECURITY],
+            self.getPolicyTypesForArtifact(artifact))
+
+    def test_productseries_task(self):
+        # A productseries task causes a link to its product's policy.
+        productseries = self.factory.makeProductSeries()
+        self.factory.makeAccessPolicy(
+            pillar=productseries.product, type=AccessPolicyType.USERDATA)
+
+        bug = self.makeBugAndPolicies(private=True)
+        bug.addTask(bug.owner, productseries)
+        self.assertEqual((1, 2), self.assertMirrored(bug))
+        # Adding the product doesn't increase the policy count.
+        bug.addTask(bug.owner, productseries.product)
+        self.assertEqual((1, 2), self.assertMirrored(bug))
+
+    def test_distribution_task(self):
+        # A distribution task causes a link to its policy.
+        distro = self.factory.makeDistribution()
+        self.factory.makeAccessPolicy(
+            pillar=distro, type=AccessPolicyType.USERDATA)
+
+        bug = self.makeBugAndPolicies(private=True)
+        bug.addTask(bug.owner, distro)
+        self.assertEqual((1, 2), self.assertMirrored(bug))
+
+    def test_distroseries_task(self):
+        # A distroseries task causes a link to its distribution's
+        # policy.
+        distroseries = self.factory.makeDistroSeries()
+        self.factory.makeAccessPolicy(
+            pillar=distroseries.distribution, type=AccessPolicyType.USERDATA)
+
+        bug = self.makeBugAndPolicies(private=True)
+        bug.addTask(bug.owner, distroseries)
+        self.assertEqual((1, 2), self.assertMirrored(bug))
+        # Adding the distro doesn't increase the policy count.
+        bug.addTask(bug.owner, distroseries.distribution)
+        self.assertEqual((1, 2), self.assertMirrored(bug))