Re: [PATCH] remove redundant ownership checks
От | Stephen Frost |
---|---|
Тема | Re: [PATCH] remove redundant ownership checks |
Дата | |
Msg-id | 20091218030931.GF17756@tamriel.snowman.net обсуждение исходный текст |
Ответ на | Re: [PATCH] remove redundant ownership checks (Tom Lane <tgl@sss.pgh.pa.us>) |
Список | pgsql-hackers |
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > KaiGai Kohei <kaigai@ak.jp.nec.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
В списке pgsql-hackers по дате отправления: