Re: Review for "Add permission check on SELECT INTO"

Thanks for your reviewing.

The reason of this strange message was bug is the patch.

> CREATE TABLE public.copy1(a, b) AS SELECT * FROM public.test;
> ERROR:  whole-row update is not implemented

When it constructs a fake RangeTblEntry, it marked modifiedCols for
attribute 0 to (tupdesc->natts - 1), although it should be 1 to natts.
InvalidAttrNumber equals 0, so ExecCheckRTPerms got confused.

The attached patch is a revised version.
It fixed up this bug, and revised test cases to ensure permission
check error shall be raised due to the new table.

+SET SESSION AUTHORIZATION selinto_user;
+SELECT * INTO TABLE selinto_schema.tmp1
+ FROM pg_class WHERE relname like '%a%'; -- Error
+ERROR: permission denied for relation tmp1

Thanks,

2011/11/18 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> The patch is in context diff format and applies cleanly.
>
> The functionality is needed because it keeps users from circumventing
> privilege restrictions, as they can currently do in this case.
>
> There is no documentation, which I think is OK since it changes
> behaviour to work as documented.
>
> The patch compiles without warning.
>
> It contains a test case, but that test case does not test
> the feature at all!
>
> The expected (and encountered) result is:
>
> CREATE SCHEMA selinto_schema;
> CREATE USER selinto_user;
> ALTER DEFAULT PRIVILEGES IN SCHEMA selinto_schema
>         REVOKE INSERT ON TABLES FROM selinto_user;
> SET SESSION AUTHORIZATION selinto_user;
> SELECT * INTO TABLE selinto_schema.tmp1
>         FROM onek WHERE onek.unique1 < 2;     -- Error
> ERROR:  permission denied for relation onek
> RESET SESSION AUTHORIZATION;
> DROP SCHEMA selinto_schema CASCADE;
> DROP USER selinto_user;
>
> This does not fail because "selinto_user" lacks INSERT permission
> on "selinto_schema.tmp1" (he doesn't!), but because he lacks
> SELECT permission on "onek" (as the error message indicates).
> Setting default privileges on a schema can never revoke
> default privileges.
>
>
> The patch works as advertised in that it causes SELECT ... INTO
> and CREATE TABLE ... AS to fail, however the error message is
> misleading. Here a (correct) test case:
>
> CREATE ROLE laurenz LOGIN;
> CREATE TABLE public.test(id, val) AS VALUES (1, 'eins'), (2, 'zwei');
> GRANT SELECT ON public.test TO laurenz;
> ALTER DEFAULT PRIVILEGES FOR ROLE laurenz REVOKE INSERT ON TABLES FROM
> laurenz;
> SET ROLE laurenz;
> CREATE TABLE public.copy1(a, b) AS SELECT * FROM public.test;
> ERROR:  whole-row update is not implemented
> CREATE TABLE public.copy1(a) AS SELECT id FROM public.test;
> ERROR:  whole-row update is not implemented
> SELECT * INTO public.copy2 FROM public.test;
> ERROR:  whole-row update is not implemented
> RESET ROLE;
> ALTER DEFAULT PRIVILEGES FOR ROLE laurenz GRANT ALL ON TABLES TO
> laurenz;
> DROP TABLE test;
> DROP ROLE laurenz;
>
> The correct error message would be:
> ERROR:  permission denied for relation copy1
>
> It seems like a wrong code path is taken in ExecCheckRTEPerms,
> maybe there's something wrong with rte->modifiedCols.
>
>
> I'll mark the patch as "Waiting on Author" until these problems are
> addressed.
>
> Yours,
> Laurenz Albe
>
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>