Re: ExecutorCheckPerms() hook - Mailing list pgsql-hackers
From | Stephen Frost |
---|---|
Subject | Re: ExecutorCheckPerms() hook |
Date | |
Msg-id | 20100524191111.GG21875@tamriel.snowman.net Whole thread Raw |
In response to | Re: ExecutorCheckPerms() hook (KaiGai Kohei <kaigai@ak.jp.nec.com>) |
Responses |
Re: ExecutorCheckPerms() hook
|
List | pgsql-hackers |
* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote: > I'd like to point out two more points are necessary to be considered > for DML permission checks in addition to ExecCheckRTPerms(). > > * DoCopy() > > Although DoCopy() is called from standard_ProcessUtility(), it performs > as DML statement, rather than DDL. It check ACL_SELECT or ACL_INSERT on > the copied table or attributes, similar to what ExecCheckRTEPerms() doing. Rather than construct a complicated API for this DML activity, why don't we just make ExecCheckRTPerms available for DoCopy to use? It seems like we could move ExecCheckRTPerms() to acl.c without too much trouble. acl.h already includes parsenodes.h and has knowledge of RangeVar's. Once DoCopy is using that, this issue resolves itself with the hook that Robert already wrote up. > * RI_Initial_Check() > > RI_Initial_Check() is a function called on ALTER TABLE command to add FK > constraints between two relations. The permission to execute this ALTER TABLE > command itself is checked on ATPrepCmd() and ATAddForeignKeyConstraint(), > so it does not affect anything on the DML permission reworks. I'm not really thrilled with how RI_Initial_Check() does it's own permissions checking and then calls SPI expecting things to 'just work'. Not sure if there's some way we could handle failure from SPI, or, if it was changed to call ExecCheckRTPerms() instead, how it would handle failure cases from there. One possible solution would be to have an additional option to ExecCheckRTPerms() which asks it to just check and return false if there's a problem, rather than unconditionally calling aclcheck_error() whenever it finds a problem. Using the same function for both the initial check in RI_Initial_Check() and then from SPI would eliminate issues where those two checks disagree for some reason, which would be good in the general case. > BTW, I guess the reason why permissions on attributes are not checked here is > that we missed it at v8.4 development. Indeed, but at the same time, this looks to be a 'fail-safe' situation. Basically, this is checking table-level permissions, which, if you have, gives you sufficient rights to SELECT against the table (any column). What this isn't doing is allowing the option of column-level permissions to be sufficient for this requirement. That's certainly an oversight in the column-level permissions handling (sorry about that), but it's not horrible- there's a workaround if RI_Initial_Check returns false already anyway. Basically, if you are using column-level privs, and you have necessary rights to do this w/ those permissions (but don't have table-level rights), it's not going to be as fast as it could be. > The most part of the checker function is cut & paste from ExecCheckRTEPerms(), > but its arguments are modified for easy invocation from other functions. As mentioned above, it seems like this would be better the other way- have the callers build RangeTbl's and then call ExecCheckRTPerms(). It feels like that approach might be more 'future-proof' as well. Thanks, Stephen
pgsql-hackers by date: