Re: [PATCH] remove redundant ownership checks

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> [ patch to remove EnableDisableRule's permissions check ]

I don't particularly like this patch, mainly because I disagree with
randomly removing permissions checks without any sort of plan about
where they ought to go. There are two principal entry points in
rewriteDefine.c (the other one being DefineQueryRewrite), and currently
they both do permissions checks. There is also a third major function
RenameRewriteRule, currently ifdef'd out because it's not used, which
is commented as "Note that it lacks a permissions check", indicating
that somebody (possibly me, I don't recall for sure) thought it was
surprising that there wasn't such a check there. This is sensible if
you suppose that this file implements rule utility commands that are
more or less directly called by the user, which is in fact the case for
DefineQueryRewrite, and it's not obvious why it wouldn't be the case for
EnableDisableRule. Since that's a globally exposed function, it's quite
possible that there's third-party code calling it and expecting it to do
the appropriate permissions check. (A quick look at Slony, in
particular, would be a good idea here.)

If we're going to start moving these checks around we need a very
well-defined notion of where permissions checks should be made, so that
everyone knows what to expect. I have not seen any plan for that.
Removing one check at a time because it appears to not be necessary
in the code paths you've looked at is not a plan.

regards, tom lane