launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/merge-db-permissions into lp:launchpad with lp:~abentley/launchpad/releasefinder-perms as a prerequisite.

Commit message:
Give all scripts SELECT on access* tables; person merge triggers can complete.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1073846 in Launchpad itself: "Person merge failed: permission denied for relation accessartifact"
  https://bugs.launchpad.net/launchpad/+bug/1073846

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/merge-db-permissions/+merge/132615

Person merge failed because DB triggers require SELECT on accessartifact.


--------------------------------------------------------------------

RULES

    Pre-implementation: Sort of wgrant and flacoste
    * Add SELECT on accessartifact to person-merge-job
    * Oh, I discussed adding safe permissions to groups instead of users
      with William and Francis. Giving SELECT to all the access* tables
      will let every script read the data needed for permission checking
      * give SELECT on the access* tables for the script user.
      * Remove the duplicate grants

    ADDENDUM
    * I see Aaron just added SELECT to an access* table to fix the same
      problem in the PRF. I will merge his branch into mine.


QA

    * Visit https://qastaging.launchpad.net/people/+adminpeoplemerge
      and ~ailo.at into ~zequence
    * https://qastaging.launchpad.net/~ailo.at and wait for 404
    * If the page loads and does not show the merge notice, then
      merge failed.


LINT

  database/schema/security.cfg
  lib/lp/registry/tests/test_personset.py


LoC

    This is a fix for feature work.

TEST

    ./bin/test -vvc -t "(script|sharing|bugnotification|queue|processmail).*(job|script)" lp
    ./bin/test --vvc -t TestPersonSetMerge lp.registry.tests.test_personset


IMPLEMENTATION

I Added a test that reproduces the SQL error. I updated the DB permissions
to allow the test to pass. I removed the duplicate permissions...note that
I also deleted a duplicate declaration for accessartifact for one user.
  database/schema/security.cfg
  lib/lp/registry/tests/test_personset.py
-- 
https://code.launchpad.net/~sinzui/launchpad/merge-db-permissions/+merge/132615
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/merge-db-permissions into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2012-11-01 20:04:20 +0000
+++ database/schema/security.cfg	2012-11-01 20:04:20 +0000
@@ -328,6 +328,11 @@
 public.sharingjob                       = SELECT, INSERT, UPDATE, DELETE
 
 [script]
+public.accessartifact                   = SELECT
+public.accessartifactgrant              = SELECT
+public.accesspolicy                     = SELECT
+public.accesspolicygrant                = SELECT
+public.accesspolicygrantflat            = SELECT
 public.scriptactivity                   = SELECT, INSERT
 type=group
 
@@ -440,11 +445,7 @@
 [productreleasefinder]
 groups=script
 public.accessartifact                   = SELECT, INSERT, DELETE
-public.accesspolicy                     = SELECT
 public.accesspolicyartifact             = SELECT, INSERT, DELETE
-public.accesspolicygrantflat            = SELECT
-public.accessartifactgrant              = SELECT
-public.accesspolicygrant                = SELECT
 public.bug                              = SELECT
 public.bugtask                          = SELECT, UPDATE
 public.bugtaskflat                      = SELECT
@@ -571,10 +572,7 @@
 groups=script
 public.account                          = SELECT, INSERT
 public.accessartifact                   = SELECT, INSERT, DELETE
-public.accesspolicy                     = SELECT
 public.accesspolicyartifact             = SELECT, INSERT, DELETE
-public.accessartifactgrant              = SELECT
-public.accesspolicygrant                = SELECT
 public.answercontact                    = SELECT
 public.archive                          = SELECT
 public.binarypackagebuild               = SELECT
@@ -860,9 +858,7 @@
 groups=write,script
 public.accessartifact                   = SELECT, INSERT, DELETE
 public.accessartifactgrant              = SELECT, INSERT
-public.accesspolicy                     = SELECT
 public.accesspolicyartifact             = SELECT, INSERT, DELETE
-public.accesspolicygrant                = SELECT
 public.answercontact                    = SELECT
 public.archive                          = SELECT, UPDATE
 public.archivearch                      = SELECT
@@ -1409,10 +1405,7 @@
 [queued]
 groups=script
 public.accessartifact                   = SELECT, INSERT, DELETE
-public.accesspolicy                     = SELECT
 public.accesspolicyartifact             = SELECT, INSERT, DELETE
-public.accessartifactgrant              = SELECT
-public.accesspolicygrant                = SELECT
 public.account                          = SELECT, INSERT
 public.answercontact                    = SELECT
 public.archive                          = SELECT, UPDATE
@@ -1530,10 +1523,7 @@
 [bugnotification]
 groups=script
 public.accessartifact                   = SELECT, INSERT, DELETE
-public.accessartifactgrant              = SELECT
-public.accesspolicy                     = SELECT
 public.accesspolicyartifact             = SELECT, INSERT, DELETE
-public.accesspolicygrant                = SELECT
 public.account                          = SELECT
 public.answercontact                    = SELECT
 public.archive                          = SELECT
@@ -1707,10 +1697,7 @@
 groups=script
 public.accessartifact                   = SELECT, INSERT, DELETE
 public.accessartifactgrant              = SELECT, INSERT, DELETE
-public.accesspolicy                     = SELECT
 public.accesspolicyartifact             = SELECT, INSERT, DELETE
-public.accesspolicygrant                = SELECT
-public.accessartifact                   = SELECT, INSERT, DELETE
 public.account                          = SELECT, INSERT
 public.answercontact                    = SELECT
 public.archive                          = SELECT
@@ -1943,11 +1930,8 @@
 
 [sharing-jobs]
 groups=script
-public.accessartifact                   = SELECT
 public.accessartifactgrant              = SELECT, UPDATE, DELETE
-public.accesspolicy                     = SELECT
 public.accesspolicygrant                = SELECT, UPDATE, DELETE
-public.accesspolicygrantflat            = SELECT
 public.account                          = SELECT
 public.branch                           = SELECT
 public.branchsubscription               = SELECT, UPDATE, DELETE

=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py	2012-07-30 18:50:41 +0000
+++ lib/lp/registry/tests/test_personset.py	2012-11-01 20:04:20 +0000
@@ -16,12 +16,16 @@
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.enums import InformationType
 from lp.code.tests.helpers import remove_all_sample_data_branches
 from lp.registry.errors import (
     InvalidName,
     NameAlreadyTaken,
     )
-from lp.registry.interfaces.accesspolicy import IAccessPolicyGrantSource
+from lp.registry.interfaces.accesspolicy import (
+    IAccessArtifactGrantSource,
+    IAccessPolicyGrantSource,
+    )
 from lp.registry.interfaces.karma import IKarmaCacheManager
 from lp.registry.interfaces.mailinglist import MailingListStatus
 from lp.registry.interfaces.mailinglistsubscription import (
@@ -618,6 +622,23 @@
         dsp = self.factory.makeDistributionSourcePackage()
         self.assertConflictingSubscriptionDeletes(dsp)
 
+    def test_merge_accessartifactgrant(self):
+        # AccessArtifactGrants are transferred; DB triggers complete.
+        dupe = self.factory.makePerson()
+        bug = self.factory.makeBug(
+            information_type=InformationType.USERDATA)
+        artifact = self.factory.makeAccessArtifact(concrete=bug)
+        self.factory.makeAccessArtifactGrant(artifact, dupe)
+        person = self.factory.makePerson()
+        self._do_premerge(dupe, person)
+        with person_logged_in(person):
+            self._do_merge(dupe, person)
+        source = getUtility(IAccessArtifactGrantSource)
+        grantees = [
+            grant.grantee for grant in source.findByArtifact([artifact])
+            if grant.grantee == person]
+        self.assertContentEqual([person], grantees)
+
     def test_merge_accesspolicygrants(self):
         # AccessPolicyGrants are transferred from the duplicate.
         person = self.factory.makePerson()