Re: ExecutorCheckPerms() hook

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)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