Re: [HACKERS] Reworks of DML permission checks
2010/7/19 KaiGai Kohei kai...@ak.jp.nec.com: 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. Committed with some changes to the comments. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks of DML permission checks
2010/7/12 KaiGai Kohei kai...@ak.jp.nec.com:
(2010/07/10 5:53), Robert Haas wrote:
2010/6/14 KaiGai Koheikai...@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.
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks of DML permission checks
(2010/07/20 3:13), Robert Haas wrote:
2010/7/12 KaiGai Koheikai...@ak.jp.nec.com:
(2010/07/10 5:53), Robert Haas wrote:
2010/6/14 KaiGai Koheikai...@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 kai...@ak.jp.nec.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 Koheikai...@ak.jp.nec.com:
(2010/07/10 5:53), Robert Haas wrote:
2010/6/14 KaiGai Koheikai...@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 kai...@ak.jp.nec.com
pgsql-v9.1-reworks-dml-checks.3.patch
Description: application/octect-stream
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks of DML permission checks
(2010/07/10 5:53), Robert Haas wrote: 2010/6/14 KaiGai Koheikai...@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. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com pgsql-v9.1-reworks-dml-checks.2.patch Description: application/octect-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks of DML permission checks
2010/6/14 KaiGai Kohei kai...@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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Reworks of DML permission checks
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. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com pgsql-v9.1-reworks-dml-checks.1.patch Description: application/octect-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
