Re: [PATCH] remove redundant ownership checks

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> 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.

The goal of this was to increase consistancy with the rest of the code,
in particular, ATPrepCmd checks ownership rights on the table, and
anything which wants to check permissions beyond that has to do it
independently. Do I like that? No, not really.

> There are two principal entry points in
> rewriteDefine.c (the other one being DefineQueryRewrite), and currently
> they both do permissions checks.

DefineQueryRewrite gets called out of tcop/utility.c (either under a
CREATE VIEW or a CREATE RULE). tcop/utility.c expects the functions it
calls to to handle permissions checking (except in the one case it
doesn't call a function- T_IndexStmt). Interestingly, DefineIndex,
while it handles a number of other permissions checks, doesn't do checks
to ensure the caller is the table owner- it expects callers to have done
that (which happens both in tcop/utility.c for CREATE INDEX and in
ATPrepCmd for ALTER TABLE ...).

> 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.)

Personally I find it suprising that things called from ATExecCmd()
expect "some" permissions checks to have been done already.

> 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.

What I suggested previously, though it may be naive, is to do the
permissions checks when you actually have enough information to do them.
At the moment we do a 'simple' check in ATPrepCmd (essentially,
ownership of the relation) and then any more complicated checks have to
be done by the function which actually understands what's going on
enough to know what else needs to be checked (eg:
ATAddForeignKeyConstraint).

As I see it, you've mainly got three steps:

Parsing the command (syntax, basic understanding)
Validation (check for object existance, get object info, etc)
Implementation (do the requested action)

I would put the permissions checking between "Validation" and
"Implementation", ideally at the 'top' of "Implementation". This, imv,
is pretty similar to how we handle DML commands today-
parsing/validation are done first but then the permissions checks aren't
done until we actually go to run the command (of course, this also deals
with prepared queries and the like). Right now what we have are a bunch
of checks scattered around during Validation, as we come across things
we think should be checked (gee, we know the table the user referenced,
let's check if he owns it now).

Of course, this patch isn't doing that because it was intended to make
the existing code consistant, not institute a new policy for how
permissions checking should be done. The big downside about trying to
define a new policy is that it's alot more difficult to get agreement
on, and there are likely to be exceptions brought out that might make
the policy appear to be ill-formed.

Do people think this is a sensible approach? Are there known exceptions
where this won't work or would cause problems? Is it possible to make
that kind of deliniation in general? Should we work up an example patch
which moves some of these checks in that direction?

Thanks for your comments.

Stephen