launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/db-bug-878115 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #878115 in Launchpad itself: "DB privilege error: poimport on TranslationMessage"
  https://bugs.launchpad.net/launchpad/+bug/878115

For more details, see:
https://code.launchpad.net/~jtv/launchpad/db-bug-878115/+merge/79803

= Summary =

See bug 878115 for a description of the problem.  This seems to be a very, very rare thing.


== Proposed fix ==

Let's not complicate this.  Add the missing privilege.


== Pre-implementation notes ==

Discussed with Stuart and with Julian today: we really ought to get rid of these fine-grained database privileges for anything that's not in any way sensitive.  Far more trouble than it's worth.

For keeping track who accesses what, we should monitor accesses, not trigger more critical bugs.


== Implementation details ==

Did this on db-devel.  We used to be able to change security.cfg straight in devel, but I believe this is no longer the case.  Not that it matters much, now that we have fastdowntime.


== Tests ==

You got me there.  I didn't add any.  Why?  Because this is a policy change, not a mechanism change, covering an obscure code path.  I can change the unit-tests to run with "realistic" database privileges, at the cost of extra test time, but then the question becomes whether I missed any roles that might do the same thing.  (Actually I know I will: the appserver goes through the same code paths, but it already has the privileges it needs.)  Or I can scenario-test it, but that comes with considerable cost and negligible gain.

What if I got the diagnosis wrong?  Then the importer will have a privilege it doesn't need, and at some point the bug may recur.


== Demo and Q/A ==

The problem should not recur on the launchpad-errors-reports list.  It could be a long way either way though.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  database/schema/security.cfg
-- 
https://code.launchpad.net/~jtv/launchpad/db-bug-878115/+merge/79803
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/db-bug-878115 into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-10-03 18:34:08 +0000
+++ database/schema/security.cfg	2011-10-19 12:03:57 +0000
@@ -455,7 +455,7 @@
 public.customlanguagecode               = SELECT
 public.translationgroup                 = SELECT
 public.translationimportqueueentry      = SELECT
-public.translationmessage               = SELECT, INSERT, UPDATE
+public.translationmessage               = SELECT, DELETE, INSERT, UPDATE
 public.translationrelicensingagreement  = SELECT
 public.translator                       = SELECT
 public.validpersoncache                 = SELECT