Re: ExecutorCheckPerms() hook - Mailing list pgsql-hackers

(2010/05/25 4:11), Stephen Frost wrote:
> * 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.

We have two options; If the checker function takes the list of RangeTblEntry,
it will be comfortable to ExecCheckRTPerms(), but not DoCopy(). Inversely,
if the checker function takes arguments in my patch, it will be comfortable
to DoCopy(), but ExecCheckRTPerms().

In my patch, it takes 6 arguments, but we can reference all of them from
the given RangeTblEntry. On the other hand, if DoCopy() has to set up
a pseudo RangeTblEntry to call checker function, it entirely needs to set
up similar or a bit large number of variables.

As I replied in the earlier message, it may be an idea to rename and change
the definition of ExecCheckRTEPerms() without moving it anywhere.

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

Sorry, I missed the fallback path also needs SELECT permissions because
validateForeignKeyConstraint() calls RI_FKey_check_ins() which entirely
tries to execute SELECT statement using SPI_*() interface.
But, it is a separate issue from the DML permission reworks.

It seems to me the permission checks in RI_Initial_Check() is a bit ad-hoc.
What we really want to do here is validation of the new FK constraints.
So, the validateForeignKeyConstraint() intends to provide a fall-back code
when table-level permission is denied, doesn't it?

In this case, we should execute the secondary query without permission checks,
because the permissions of ALTER TABLE is already checked, and we can trust
the given query is harmless.

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

Yes, it is harmless expect for performances in a corner-case.
If user have table-level permissions, it does not need to check column-
level permissions, even if it is implemented.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>