launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/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/bug-878115/+merge/79919

= Summary =

See bug 878115 for a description of the problem.


== Proposed fix ==

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


== Pre-implementation notes ==

Discussed with Stuart and with Julian: 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 of who accesses what, we should monitor accesses, not create an endless stream of critical bugs.


== Implementation details ==

I thought this had to go to db-devel in our current process, but William says it should go to devel.


== Tests ==

You got me there. I didn't add any.

Why?  Well the _reason_ is that I'm at the same time lazy and eager to get this problem fixed.  My _justifications_ are:
 * This is a policy change, not a mechanism change.  No point in testing that each security.cfg entry does what it's supposed to.
 * The code path that fails is relatively obscure.  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.
 * The problem is unlikely to recur in its exact same form.  How do you test for not having missed anything in testing, such as a different database role that also executes the same code?
 * Actually, if I test this under the poimport DB role, I _know_ I will be ignoring a realistic case: the appserver goes through the same code paths, but it already has the privileges it needs.

What if I got the diagnosis wrong?  Then I fixed a different critical bug than what caused this one.  Still worthwhile.


== Demo and Q/A ==

The problem should not recur on the launchpad-errors-reports list.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  database/schema/security.cfg

-- 
https://code.launchpad.net/~jtv/launchpad/bug-878115/+merge/79919
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/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-20 07:10:28 +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