Re: ExecutorCheckPerms() hook

(2010/05/24 22:18), Robert Haas wrote:
> 2010/5/24 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>> BTW, I guess the reason why permissions on attributes are not checked here is
>> that we missed it at v8.4 development.
> 
> That's a little worrying.  Can you construct and post a test case
> where this results in a user-visible failure in CVS HEAD?

Sorry, after more detailed consideration, it seems to me the permission
checks in RI_Initial_Check() and its fallback mechanism are nonsense.

See the following commands.
 postgres=# CREATE USER ymj; CREATE ROLE postgres=# CREATE TABLE pk_tbl (a int primary key, b text); NOTICE:  CREATE
TABLE/ PRIMARY KEY will create implicit index "pk_tbl_pkey" for table "pk_tbl" CREATE TABLE postgres=# CREATE TABLE
fk_tbl(x int, y text); CREATE TABLE postgres=# ALTER TABLE pk_tbl OWNER TO ymj; ALTER TABLE postgres=# ALTER TABLE
fk_tblOWNER TO ymj; ALTER TABLE postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; REVOKE postgres=# GRANT REFERENCES ON
pk_tbl,fk_tbl TO ymj; GRANT
 

At that time, the 'ymj' has ownership and REFERENCES permissions on
both of pk_tbl and fk_tbl. In this case, RI_Initial_Check() shall return
and the fallback-seqscan will run. But,
 postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); ERROR:  permission denied for relation pk_tbl
CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."pk_tbl" x WHERE "a" OPERATOR(pg_catalog.=) $1 FOR SHARE OF x"
 

From more careful observation of the code, the validateForeignKeyConstraint()
also calls RI_FKey_check_ins() for each scanned tuples, but it also execute
SELECT statement using SPI_*() interface internally.

In other words, both of execution paths entirely require SELECT permission
to validate new FK constraint.


I think we need a new SPI_*() interface which allows to run the given query
without any permission checks, because these queries are purely internal stuff,
so we can trust the query is harmless.
Is there any other idea?

>> The attached patch provides a common checker function of DML, and modifies
>> ExecCheckRTPerms(), CopyTo() and RI_Initial_Check() to call the checker
>> function instead of individual ACL checks.
> 
> This looks pretty sane to me, although I have not done a full review.
> I am disinclined to create a whole new directory for it.   I think the
> new function should go in src/backend/catalog/aclchk.c and be declared
> in src/include/utils/acl.h.  If that sounds reasonable to you, please
> revise and post an updated patch.
> 

I'm afraid of that the src/backend/catalog/aclchk.c will become overcrowding
in the future. If it is ugly to deploy checker functions in separated dir/files,
I think it is an idea to put it on the execMain.c, instead of ExecCheckRTEPerms().

It also suggest us where the checker functions should be deployed on the upcoming
DDL reworks. In similar way, we will deploy them on src/backend/command/pg_database
for example?

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