Re: Reworks of DML permission checks

The attached patch is the revised one.

* It was rebased to the latest git HEAD.
* Prototype of ExecCheckRTEPerms() was changed; it become to return
  a bool value to inform the caller its access control decision, and
  its 'ereport_on_violation' argument has gone.
* ExecCheckRTPerms() calls aclcheck_error() when ExecCheckRTEPerms()
  returned false, and 'ereport_on_violation' is true.
* Add '#include "executor/executor.h"' on the ri_triggers.c.

Thanks,

(2010/07/20 9:24), KaiGai Kohei wrote:
> (2010/07/20 3:13), Robert Haas wrote:
>> 2010/7/12 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>>> (2010/07/10 5:53), Robert Haas wrote:
>>>> 2010/6/14 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>>>>> The attached patch tries to rework DML permission checks.
>>>>>
>>>>> It was mainly checked at the ExecCheckRTEPerms(), but same logic was
>>>>> implemented in COPY TO/FROM statement and RI_Initial_Check().
>>>>>
>>>>> This patch tries to consolidate these permission checks into a common
>>>>> function to make access control decision on DML permissions. It enables
>>>>> to eliminate the code duplication, and improve consistency of access
>>>>> controls.
>>>>
>>>> This patch is listed on the CommitFest page, but I'm not sure if it
>>>> represents the latest work on this topic.  At a minimum, it needs to
>>>> be rebased.
>>>>
>>>> I am not excited about moving ExecCheckRT[E]Perms to some other place
>>>> in the code.  It seems to me that will complicate back-patching with
>>>> no corresponding advantage.  I'd suggest we not do that.    The COPY
>>>> and RI code can call ExecCheckRTPerms() where it is. Maybe at some
>>>> point we will have a grand master plan for how this should all be laid
>>>> out, but right now I'd prefer localized changes.
>>>>
>>>
>>> OK, I rebased and revised the patch not to move ExecCheckRTPerms()
>>> from executor/execMain.c.
>>> In the attached patch, DoCopy() and RI_Initial_Check() calls that
>>> function to consolidate dml access control logic.
>>
>> This patch contains a number of copies of the following code:
>>
>> +               {
>> +                       if (ereport_on_violation)
>> +                               aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS,
>> +
>> get_rel_name(relOid));
>> +                       return false;
>> +               }
>>
>> What if we don't pass ereport_on_violation down to
>> ExecCheckRTEPerms(), and just have it return a boolean?  Then
>> ExecCheckRTPerms() can throw the error if ereport_on_violation is
>> true, and return false otherwise.
>>
> All the error messages are indeed same, so it seems to me fair enough.
>
> As long as we don't need to report the error using aclcheck_error_col(),
> instead of aclcheck_error(), this change will keep the code simple.
> If it is preferable to show users the column-name in access violations,
> we need to raise an error from ExecCheckRTEPerms().
>
>> With this patch, ri_triggers.c emits a compiler warning, apparently
>> due to a missing include.
>>
> Oh, sorry, I'll fix it soon.
>
> Thanks,


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