Re: ExecutorCheckPerms() hook

Stephen, thanks for comments.

The attached three patches are the revised and divided ones.

A: add makeRangeTblEntry()
B: major reworks of DML permission checks
C: add an ESP hook on the DML permission checks

(2010/05/27 0:09), Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> The attached patch is a revised one for DML permission checks.
>
> This is certainly alot better.
>
>> ToDo:
>> - makeRangeTblEntry() stuff to allocate a RTE node with given parameter
>> is not yet.
>
> I'd certainly like to see the above done, or to understand why it can't
> be if that turns out to be the case.

The patch-A tries to implement makeRangeTblEntry() which takes only rtekind
as argument right now.
Other fields are initialized to zero, using makeNode().

> A couple of other comments, all pretty minor things:
>
> - I'd still rather see the hook itself in another patch, but given that
> we've determined that none of this is going to go into 9.0, it's not
> as big a deal.

OK, I divided the ESP hook part into the patch-C.

> - The hook definition in aclchk.c should really be at the top of that
> file. We've been pretty consistant about putting hooks at the top of
> files instead of deep down in the file, this should also follow that
> scheme.

OK, I moved it.

> - Some of the comments at the top of chkpriv_rte_perms probably make
> sense to move up to where it's called from execMain.c. Specifically,
> the comments about the other RTE types (function, join, subquery).
> I'd probably change the comment in chkpriv_rte_perms to be simpler-
> "This is only used for checking plain relation permissions, nothing
> else is checked here", and also have that same comment around
> chkpriv_relation_perms, both in aclchk.c and in acl.h.

OK, I edited the comment as follows:

| /*
| * Do permissions checks. The check_relation_privileges() checks access
| * permissions for all relations listed in a range table, but does not
| * check anything for other RTE types (function, join, subquery, ...).
| * Function RTEs are checked by init_fcache when the function is prepared
| * for execution. Join, subquery, and special RTEs need no checks.
| */

> - I'd move chkpriv_relation_perms above chkpriv_rte_perms, it's what we
> expect people to use, after all.

OK, I reordered it.

> - Don't particularly like the function names. How about
> relation_privilege_check? Or rangetbl_privilege_check? We don't use
> 'perms' much (uh, at all?) in function names, and even if we did, it'd
> be redundant and not really help someone understand what the function
> is doing.

IIRC, Robert suggested that a verb should lead the function name.
So, I renamed it into check_relation_privileges() and check_rte_privileges().

> - I don't really like having 'abort' as the variable name for the 2nd
> argument. I'm not finding an obvious convention right now, but maybe
> something like "error_on_failure" instead?

The 'failure' may make an impression of generic errors not only permission denied.
How about 'error_on_violation'?

> - In DoCopy, some comments about what you're doing there to set up for
> calling chkpriv_relation_perms would be good (like the comment you
> removed- /* We don't have table permissions, check per-column
> permissions */, updated to for something like "build an RTE with the
> columns referenced marked to check for necessary privileges").
> Additionally, it might be worth considering if having an RTE built
> farther up in DoCopy would make sense and would then be usable for
> other bits in DoCopy.

I edited the comments as follows:

| /*
| * Check relation permissions.
| * We built an RTE with the relation and columns to be accessed
| * to check for necessary privileges in the common way.
| */

> - In RI_Initial_Check, why not build up an actual list of RTEs and just
> call chkpriv_relation_perms once? Also, you should add comments
> there, again, about what you're doing and why. If you can use another
> function to build the actual RTE, this will probably fall out more
> sensibly too.

Good catch! I fixed the invocation of checker function with list_make2().

And, I edited the comments as follows:

| /*
| * We built a pair of RTEs of FK/PK relations and columns referenced
| * in the test query to check necessary permission in the common way.
| */

> - Have you checked if there are any bad side-effects from calling
> ri_FetchConstraintInfo before doing the permissions checking?

The ri_FetchConstraintInfo() only references SysCaches to set up given
local variable without any other locks except for ones acquired by syscache.c.

> - The hook in acl.h should be separated out and brought to the top and
> documented independently as to exactly where the hook is and what it
> can be used for, along with what the arguments mean, etc. Similairly,
> chkpriv_relation_perms should really have a short comment for it about
> what it's for. Something more than 'security checker function'.

OK, at the patch-C, I moved the definition of the hook into the first half
of acl.h, but it needs to be declared after the AclResult definition.

BTW, I wonder whether acl.h is a correct place to explain about the hook,
although I added comments for the hook.
I think we should add a series of explanation about ESP hooks in the internal
section of the documentation, when the number of hooks reaches a dozen for
example.

Thanks,
--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>