ExecutorCheckPerms() hook
in [PgSql]
|
From: Stephen Frost on 14 Jun 2010 08:35 * Robert Haas (robertmhaas(a)gmail.com) wrote: > This is essentially the same patch that I wrote and posted several > weeks ago, with changes to the comments and renaming of the > identifiers. Are you trying to represent it as your own work? Ehh, I doubt it. He had included your patch in another patch that he was working, which I then reviewed and asked him to update/change, and I think part of that was me asking that he keep the hook patch split out. He then split it out of his patch rather than just going back to yours. > With all due respect, I intend to imply my own version. Please make > your other proposed patches apply on top of that. This strikes me as a case of "gee, won't git help here?". Perhaps we can use this as an opportunity to show how git can help. Then again, it's not exactly a huge patch. :) Thanks, Stephen (who won't mention the impetus for the hook being put here in the first place.. ;)
From: KaiGai Kohei on 14 Jun 2010 20:09 (2010/06/14 21:35), Stephen Frost wrote: > * Robert Haas (robertmhaas(a)gmail.com) wrote: >> This is essentially the same patch that I wrote and posted several >> weeks ago, with changes to the comments and renaming of the >> identifiers. Are you trying to represent it as your own work? > > Ehh, I doubt it. He had included your patch in another patch that he > was working, which I then reviewed and asked him to update/change, and > I think part of that was me asking that he keep the hook patch split > out. He then split it out of his patch rather than just going back to > yours. > >> With all due respect, I intend to imply my own version. Please make >> your other proposed patches apply on top of that. > > This strikes me as a case of "gee, won't git help here?". Perhaps we > can use this as an opportunity to show how git can help. Then again, > it's not exactly a huge patch. :) > The patch provides the same functionality with what you wrote and posted several weeks ago, but different from identifiers and comments. During the discussion, I was suggested that 'ExecutorCheckPerms_hook' is not an appropriate naming on the refactored DML permission check routine, because it is not still a part of the executor. So, I changed your original proposition. When ExecCheckRTPerms() was refactored to a common DML permission checker function, is the hook also renamed to more appropriately? If so, I don't have any opposition to go back to the original one. Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 9 Jul 2010 10:07 On Thu, May 20, 2010 at 1:33 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote: >> As for committing it, I would definitely like to commit the actual >> hook. �If we want the hook without the contrib module that's OK with >> me, although I generally feel it's useful to have examples of how >> hooks can be used, which is why I took the time to produce a working >> example. > > +1 on committing the hook. �As for the contrib module, it doesn't strike > me that there's much of a use-case for it as is. �I think it's enough > that it's available in the -hackers archives. OK, done that way. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 21 Jul 2010 19:45 2010/5/24 KaiGai Kohei <kaigai(a)ak.jp.nec.com>: > (2010/05/24 22:18), Robert Haas wrote: >> 2010/5/24 KaiGai Kohei<kaigai(a)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_tbl OWNER 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 the relevant case might be where ymj owns fk_tbl but not pk_tbl, and has REFERENCES but not SELECT on pk_tbl. Come to think of it, I wonder if REFERENCES on fk_tbl ought to be sufficient to create a foreign key. Currently, it requires ownership: rhaas=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); ERROR: must be owner of relation fk_tbl -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: KaiGai Kohei on 21 Jul 2010 20:13
(2010/07/22 8:45), Robert Haas wrote: > 2010/5/24 KaiGai Kohei<kaigai(a)ak.jp.nec.com>: >> (2010/05/24 22:18), Robert Haas wrote: >>> 2010/5/24 KaiGai Kohei<kaigai(a)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_tbl OWNER 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 the relevant case might be where ymj owns fk_tbl but not > pk_tbl, and has REFERENCES but not SELECT on pk_tbl. > > Come to think of it, I wonder if REFERENCES on fk_tbl ought to be > sufficient to create a foreign key. Currently, it requires ownership: > > rhaas=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); > ERROR: must be owner of relation fk_tbl > +1. We can find out a similar case in CreateTrigger(). If I was granted TRIGGER privilege on a certain table, I can create a new trigger on the table without its ownership. More commonly, it allows us to modify a certain property of the table without its ownership. Perhaps, if SQL permission would be more fine-grained, for example, "RENAME" permission might control RENAME TO statement, rather than its ownership. What is the reason why we check its ownership here, although we already have REFERENCES permission to control ADD FOREIGN KEY? Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |