From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> |
Cc: | Stephen Frost <sfrost(at)snowman(dot)net>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Reworks of DML permission checks |
Date: | 2010-07-19 18:13:18 |
Message-ID: | AANLkTim4PkecormSbadh30XnWnkT08F+oqgqJV+uJGRQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
2010/7/12 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> (2010/07/10 5:53), Robert Haas wrote:
>> 2010/6/14 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)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.
With this patch, ri_triggers.c emits a compiler warning, apparently
due to a missing include.
Otherwise, the changes look pretty sensible, though I haven't tested them yet.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2010-07-19 19:06:00 | Re: TODO 9.0 done items removed |
Previous Message | David Fetter | 2010-07-19 18:09:37 | Re: SHOW TABLES |