more RLS oversights

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:

I happened to notice this morning while hacking that the
"hasRowSecurity" fields added to PlannerGlobal and PlannedStmt have
not been given proper nodefuncs.c support. Both need to be added to
outfuncs.c, and the latter to copyfuncs.c. The latter omission may
well be a security bug, although I haven't attempted to verify that,
but fortunately this isn't released yet.

I saw this and will address it. Would be great if you wouldn't mind
CC'ing me directly on anything RLS-related, same as you CC'd me on the
column-privilege backpatch. I expect I'll probably notice anyway, but
I'll see them faster when I'm CC'd.

I agree that it's great that we're catching issues prior to when the
feature is released and look forward to anything else you (or anyone
else!) finds.

Thanks!

Stephen

Robert, all,

* Stephen Frost (sfrost@snowman.net) wrote:

* Robert Haas (robertmhaas@gmail.com) wrote:

I happened to notice this morning while hacking that the
"hasRowSecurity" fields added to PlannerGlobal and PlannedStmt have
not been given proper nodefuncs.c support. Both need to be added to
outfuncs.c, and the latter to copyfuncs.c. The latter omission may
well be a security bug, although I haven't attempted to verify that,
but fortunately this isn't released yet.

I saw this and will address it. Would be great if you wouldn't mind
CC'ing me directly on anything RLS-related, same as you CC'd me on the
column-privilege backpatch. I expect I'll probably notice anyway, but
I'll see them faster when I'm CC'd.

I agree that it's great that we're catching issues prior to when the
feature is released and look forward to anything else you (or anyone
else!) finds.

I've pushed a fix for this. Please let me know if you see any other
issues or run into any problems.

Thanks!

Stephen

On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote:

* Stephen Frost (sfrost@snowman.net) wrote:

I agree that it's great that we're catching issues prior to when the
feature is released and look forward to anything else you (or anyone
else!) finds.

I've pushed a fix for this. Please let me know if you see any other
issues or run into any problems.

(1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() on
the node trees. Test case:

begin;
set row_security = force;
create table t (c) as values ('bar'::text);
create policy p on t using (c < ('foo'::text COLLATE "C"));
alter table t enable row level security;
table pg_policy; -- note ":inputcollid 0"
select * from t; -- ERROR: could not determine which collation ...
rollback;

(2) CreatePolicy() and AlterPolicy() omit to create a pg_shdepend entry for
each role in the TO clause. Test case:

begin;
create role alice;
create table t (c) as values ('bar'::text);
grant select on table t to alice;
create policy p on t to alice using (true);
select refclassid::regclass, refobjid, refobjsubid, deptype
from pg_depend where classid = 'pg_policy'::regclass;
select refclassid::regclass, refobjid, deptype
from pg_shdepend where classid = 'pg_policy'::regclass;
savepoint q; drop role alice; rollback to q;
revoke all on table t from alice;
\d t
drop role alice;
\d t
rollback;

+static void
+dumpPolicy(Archive *fout, PolicyInfo *polinfo)

...

+	if (polinfo->polqual != NULL)
+		appendPQExpBuffer(query, " USING %s", polinfo->polqual);

(3) The USING clause needs parentheses; a dump+reload failed like so:

pg_restore: [archiver (db)] could not execute query: ERROR: syntax error at or near "CASE"
LINE 2: CASE
^
Command was: CREATE POLICY "p2" ON "category" FOR ALL TO PUBLIC USING
CASE
WHEN ("current_user"() = 'rls_regress_user1'::"name") THE...

Add the same parentheses to psql \d output also, keeping that output similar
to the SQL syntax.

(3a) I found this by hacking the rowsecurity.sql regression test to not drop
its objects, then running the pg_upgrade test suite. New features that affect
pg_dump should leave objects in the regression database to test the pg_dump
support via that suite.

(4) When DefineQueryRewrite() is about to convert a table to a view, it checks
the table for features unavailable to views. For example, it rejects tables
having triggers. It omits to reject tables having relrowsecurity or a
pg_policy record. Test case:

begin;
set row_security = force;
create table t (c) as select * from generate_series(1,5);
create policy p on t using (c % 2 = 1);
alter table t enable row level security;
table t;
truncate t;
create rule "_RETURN" as on select to t do instead
select * from generate_series(1,5) t0(c);
table t;
select polrelid::regclass from pg_policy;
select relrowsecurity from pg_class where oid = 't'::regclass;
rollback;

+  <para>
+   Referential integrity checks, such as unique or primary key constraints
+   and foreign key references, will bypass row security to ensure that
+   data integrity is maintained.  Care must be taken when developing
+   schemas and row level policies to avoid a "covert channel" leak of
+   information through these referential integrity checks.

...

+	/*
+	 * Row-level security should be disabled in the case where a foreign-key
+	 * relation is queried to check existence of tuples that references the
+	 * primary-key being modified.
+	 */
+	temp_sec_context = save_sec_context | SECURITY_LOCAL_USERID_CHANGE;
+	if (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK
+		|| qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK_FROM_PK
+		|| qkey->constr_queryno == RI_PLAN_RESTRICT_DEL_CHECKREF
+		|| qkey->constr_queryno == RI_PLAN_RESTRICT_UPD_CHECKREF)
+		temp_sec_context |= SECURITY_ROW_LEVEL_DISABLED;

(5) This code does not use SECURITY_ROW_LEVEL_DISABLED for foreign keys with
CASCADE, SET NULL or SET DEFAULT actions. The associated documentation says
nothing about this presumably-important distinction.

Is SECURITY_ROW_LEVEL_DISABLED mode even needed? We switch users to the owner
of the FROM-clause table before running an RI query. That means use of this
mode can only matter under row_security=force, right? I would rest easier if
this mode went away, because it is a security landmine. If user code managed
to run in this mode, it would bypass every policy in the database. (I find no
such vulnerability today, because we use the mode only for parse analysis of
ri_triggers.c queries.)

(6) AlterPolicy() calls InvokeObjectPostAlterHook(PolicyRelationId, ...), but
CreatePolicy() and DropPolicy() lack their respective hook invocations.

(7) Using an aggregate function in a policy predicate elicits an inapposite
error message due to use of EXPR_KIND_WHERE for parse analysis. Need a new
ParseExprKind. Test case:

begin;
set row_security = force;
create table t (c) as values ('bar'::text);
-- ERROR: aggregate functions are not allowed in WHERE
create policy p on t using (max(c));
rollback;

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Noah,

* Noah Misch (noah@leadboat.com) wrote:

On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote:

* Stephen Frost (sfrost@snowman.net) wrote:

I agree that it's great that we're catching issues prior to when the
feature is released and look forward to anything else you (or anyone
else!) finds.

I've pushed a fix for this. Please let me know if you see any other
issues or run into any problems.

(1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() on
the node trees. Test case:

Will fix.

(2) CreatePolicy() and AlterPolicy() omit to create a pg_shdepend entry for
each role in the TO clause. Test case:

Will fix.

+static void
+dumpPolicy(Archive *fout, PolicyInfo *polinfo)

...

+	if (polinfo->polqual != NULL)
+		appendPQExpBuffer(query, " USING %s", polinfo->polqual);

(3) The USING clause needs parentheses; a dump+reload failed like so:

Will fix.

Add the same parentheses to psql \d output also, keeping that output similar
to the SQL syntax.

Yup.

(3a) I found this by hacking the rowsecurity.sql regression test to not drop
its objects, then running the pg_upgrade test suite. New features that affect
pg_dump should leave objects in the regression database to test the pg_dump
support via that suite.

Will fix.

(4) When DefineQueryRewrite() is about to convert a table to a view, it checks
the table for features unavailable to views. For example, it rejects tables
having triggers. It omits to reject tables having relrowsecurity or a
pg_policy record. Test case:

Will fix.

+  <para>
+   Referential integrity checks, such as unique or primary key constraints
+   and foreign key references, will bypass row security to ensure that
+   data integrity is maintained.  Care must be taken when developing
+   schemas and row level policies to avoid a "covert channel" leak of
+   information through these referential integrity checks.

...

+	/*
+	 * Row-level security should be disabled in the case where a foreign-key
+	 * relation is queried to check existence of tuples that references the
+	 * primary-key being modified.
+	 */
+	temp_sec_context = save_sec_context | SECURITY_LOCAL_USERID_CHANGE;
+	if (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK
+		|| qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK_FROM_PK
+		|| qkey->constr_queryno == RI_PLAN_RESTRICT_DEL_CHECKREF
+		|| qkey->constr_queryno == RI_PLAN_RESTRICT_UPD_CHECKREF)
+		temp_sec_context |= SECURITY_ROW_LEVEL_DISABLED;

(5) This code does not use SECURITY_ROW_LEVEL_DISABLED for foreign keys with
CASCADE, SET NULL or SET DEFAULT actions. The associated documentation says
nothing about this presumably-important distinction.

Agreed, will fix.

Is SECURITY_ROW_LEVEL_DISABLED mode even needed? We switch users to the owner
of the FROM-clause table before running an RI query. That means use of this
mode can only matter under row_security=force, right? I would rest easier if
this mode went away, because it is a security landmine. If user code managed
to run in this mode, it would bypass every policy in the database. (I find no
such vulnerability today, because we use the mode only for parse analysis of
ri_triggers.c queries.)

That's a very interesting point.. At first blush, I agree, it shouldn't
be necessary. I'll play with it and see if I can get everything to work
properly without it.

(6) AlterPolicy() calls InvokeObjectPostAlterHook(PolicyRelationId, ...), but
CreatePolicy() and DropPolicy() lack their respective hook invocations.

Will fix.

(7) Using an aggregate function in a policy predicate elicits an inapposite
error message due to use of EXPR_KIND_WHERE for parse analysis. Need a new
ParseExprKind. Test case:

Will fix.

Thanks a lot for your review! Much appreciated.

Stephen

On 07/03/2015 10:03 AM, Noah Misch wrote:

(1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() on
the node trees. Test case:

begin;
set row_security = force;
create table t (c) as values ('bar'::text);
create policy p on t using (c < ('foo'::text COLLATE "C"));
alter table t enable row level security;
table pg_policy; -- note ":inputcollid 0"
select * from t; -- ERROR: could not determine which collation ...
rollback;

The attached fixes this issue for me, but I am unsure whether we really
need/want the regression test. Given the recent push to increase test
coverage maybe so. Any objections?

Joe

On Fri, Jul 10, 2015 at 03:08:53PM -0700, Joe Conway wrote:

On 07/03/2015 10:03 AM, Noah Misch wrote:

(1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() on
the node trees.

The attached fixes this issue for me, but I am unsure whether we really
need/want the regression test. Given the recent push to increase test
coverage maybe so.

I wouldn't remove the test from your patch.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/10/2015 06:15 PM, Noah Misch wrote:

On Fri, Jul 10, 2015 at 03:08:53PM -0700, Joe Conway wrote:

On 07/03/2015 10:03 AM, Noah Misch wrote:

(1) CreatePolicy() and AlterPolicy() omit to call
assign_expr_collations() on the node trees.

The attached fixes this issue for me, but I am unsure whether we
really need/want the regression test. Given the recent push to
increase test coverage maybe so.

I wouldn't remove the test from your patch.

Ok -- pushed.

- --
Joe Conway
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVoYj2AAoJEDfy90M199hleBcP/3anI8IIEkFyPiUDX0QvzfEM
EOBm+AdolwgAvscU6RaDbrVXJlE32YbSWsXhtXOA5jJvhY80ln3YO+ko1ONWV3dW
iYbvSO+zQalHDqmID2bqbnY/k+7GTGWPCTsSLMsmUK7P0QCVP2f02lCukqr4yWpH
tIVbOfb0A1+Mrb9dxta43Bj32maBBiEpWIwaebotik6BmfwHNeeaZ082PUJQvaqS
wtshrlctAaCsCyjQnNiPvtD9mw0rlSWOhNDc7R8KGflWnwXmBlyu7jD4aFHKcPZO
v1ErqG2Z0allm3p6snpFbiunQssVpHgF7V8FcWxIReu73lV25ig3Ix56MoUXHIOq
a2Y5iAfRw106V1GA6ARW0kjCaE0DrRcfA6/Um8LeEhw44cvUBZkhXx/ozt0t62pz
6mvhKN4UjmO/XfbA9GEN7b9kDz+LZtMFQ1PqcH7mK3OYKgGfYTAdJOA7qwHuWMBC
MGVHP5WEUCJEToTNyzVe0matOH8+IHS4LQ9qAtUFVCmhh27FK0m8kjoZAmT/xDk5
uNcMG9mvBOTZe5EmVdC1gywDOsRntzAgWM1SFBK2v0YEgj3YarKll839Jm+dNHGZ
nxjniR/XJkNxISrTN6Qq797nhYsmpqRg+d7ZVk0GxmhfNNp/f2SFRVB9/9ovrk4x
//7pllazs48qu6e/3eYK
=EEZ7
-----END PGP SIGNATURE-----

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/03/2015 10:03 AM, Noah Misch wrote:

(2) CreatePolicy() and AlterPolicy() omit to create a pg_shdepend
entry for each role in the TO clause. Test case:

Please see the attached patch. Note that I used SHARED_DEPENDENCY_ACL
for this. It seems appropriate, but possibly we should invent a new
shared dependency type for this use case? Comments?

Thanks,

Joe

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVtSlAAAoJEDfy90M199hlrkIP/0BqMj30JpJVj3H5pIopU0RZ
pesBzFzzsWmt2QmasX9hajRfo6eXQAWPmKQmw/NDTJvHHNACxLdo0MHE7A9y7No7
aZIFTm0KhnG4jpzxfzpGqQ4ron5Vsc2TlNQgCBYCtbEEtuD0mV2qmcuTkqGte7iB
7iqneRBhmTYBy63X7S0ir4AhDi+JJg8P4F7YuU/XMcha5v5CpNJAToPpW7mCoZ8O
9w/RbXCHso7p1DIxfx4tfxVwLQ7j7G2j0rXbuA2e6OKfwZWWrk5E8Wnvc3xy3yCY
J37fcjOxFdhU/j1j+Tr90LYOSuRu5fQ4z6PmmsPkBM3T0Vllz/nNP64aVKbmHzga
szrIBERRs2icKa4OI08qZF42ObIEv0/t/NuyrXpegY6awRNNNsjyZvwM+51zdjw1
fuWh+2rObzh3RDH8lPB0N0rVfDMM+wU85Vaekckv/7UQ/pbWcwsYD/tykA1engQ8
Ww8kBAEct4dWdqppTqvLLxLgo4d+vuiS1mJ7SRY2aZFRX8QQ8TfB3eObETUsU4/X
9UWyMn+E0Au9ICX+wfD4whaBKst8EuO36qOZx0oZt+++EMOlzypgkCCxDtZO0KWd
KZHbtmJXHFVH+ihbEGXAKMIx+gLqSDG3IKy9MIJxpB3JY3XSdBNqobL2hiQy36/w
svK7i+mEYmUCQ6pB1j8c
=P1AS
-----END PGP SIGNATURE-----

Joe Conway wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/03/2015 10:03 AM, Noah Misch wrote:

(2) CreatePolicy() and AlterPolicy() omit to create a pg_shdepend
entry for each role in the TO clause. Test case:

Please see the attached patch. Note that I used SHARED_DEPENDENCY_ACL
for this. It seems appropriate, but possibly we should invent a new
shared dependency type for this use case? Comments?

Hmm, these are not ACL objects, so conceptually it seems cleaner to use
a different symbol for this. I think the catalog state and the error
messages would be a bit confusing otherwise.

if (spec->roletype == ROLESPEC_PUBLIC)
{
! Datum *tmp_role_oids;
!
! if (*num_roles != 1)
ereport(WARNING,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("ignoring roles specified other than public"),
errhint("All roles are members of the public role.")));
! *num_roles = 1;
! tmp_role_oids = (Datum *) palloc(*num_roles * sizeof(Datum));
! tmp_role_oids[0] = ObjectIdGetDatum(ACL_ID_PUBLIC);

Isn't this leaking the previously allocated array? Not sure it's all
that critical, but still. (I don't think you really need to call palloc
at all here.)

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/27/2015 01:13 PM, Alvaro Herrera wrote:

Hmm, these are not ACL objects, so conceptually it seems cleaner to
use a different symbol for this. I think the catalog state and the
error messages would be a bit confusing otherwise.

Ok -- done

Isn't this leaking the previously allocated array? Not sure it's
all that critical, but still. (I don't think you really need to
call palloc at all here.)

Agreed -- I think the attached is a bit cleaner.

Any other comments or concerns?

- --
Joe Conway
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVts3+AAoJEDfy90M199hlzTIQAJ6LEOrEEhHEjsoVvCT6TUAu
pMQl/LWmb0s3/vF9o5VFTpVd21k0LxcggD+DdbxSagiw1WpLK5x67C9Lj5uuFn/d
3a95nFnQje3ciQJaAKS4XcGyx8+6rHPZqfBRC1rARbLuDHrwpKqmbKwvpQCud4xN
kD7OolYk5Gd3cId0xH0XBHuwLVJg4Bt/MAexrcHn+kXuQoF8V6iOjnmBI/BHvTQy
w4j4ov7DpWSAR1ZiCTCkF2ZuNd9TJ8FmAEtSDVrlWMxB9J+9PU5oTfD50jp62BTz
wycANVYmbpCyZ7DAAiqopt3IQFIiF/1bYwzWH3/M8RRMKJF8HNyE8KBPDyC/doO5
0c0poCugJI2wOhumLGJShizgSAS85vqijh2Uxpp2yQxdStMnADykzT4Nb5EnEJVE
i7OKy6w+2xyilfFGEWhHfS7uo5Y0zBorhpjgT9fRaqPBGoK4jYwZoO8w97SUd9aS
kNXOCfmFxvcDFSZIYZP77pOeJZR2dLCbr+X9bF1Fz5S4FVkgCXVOp9rmsqzgxoDh
lcpkDh9zPPezdczRkfq/qCf0lmzGg8apdqdr7+g8DxU+01OvPspEdpovQQN0HXlM
m5Kbny4KCWhAgBTyAsOFTEy6lK7u4KsHV1cee3bG+ev65czbQ14gGRMJc/Lhm6Gg
Apcn/UcF14vLWxYVo6lS
=pS6L
-----END PGP SIGNATURE-----

On 07/03/2015 10:03 AM, Noah Misch wrote:

+static void
+dumpPolicy(Archive *fout, PolicyInfo *polinfo)

...

+	if (polinfo->polqual != NULL)
+		appendPQExpBuffer(query, " USING %s", polinfo->polqual);

(3) The USING clause needs parentheses; a dump+reload failed like so:

Also needed for WITH CHECK. Fix pushed to 9.5 and HEAD.

Joe

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 07/03/2015 10:03 AM, Noah Misch wrote:

(4) When DefineQueryRewrite() is about to convert a table to a view, it checks
the table for features unavailable to views. For example, it rejects tables
having triggers. It omits to reject tables having relrowsecurity or a
pg_policy record. Test case:

Please see the attached patch for this issue. Comments?

Thanks,

Joe

* Joe Conway (joe.conway@crunchydata.com) wrote:

On 07/03/2015 10:03 AM, Noah Misch wrote:

(4) When DefineQueryRewrite() is about to convert a table to a view, it checks
the table for features unavailable to views. For example, it rejects tables
having triggers. It omits to reject tables having relrowsecurity or a
pg_policy record. Test case:

Please see the attached patch for this issue. Comments?

Looks good to me.

Thanks!

Stephen

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/27/2015 05:34 PM, Joe Conway wrote:

On 07/27/2015 01:13 PM, Alvaro Herrera wrote:

Hmm, these are not ACL objects, so conceptually it seems cleaner
to use a different symbol for this. I think the catalog state
and the error messages would be a bit confusing otherwise.

Ok -- done

Isn't this leaking the previously allocated array? Not sure
it's all that critical, but still. (I don't think you really
need to call palloc at all here.)

Agreed -- I think the attached is a bit cleaner.

Any other comments or concerns?

Pushed to HEAD and 9.5

Joe

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVuAp9AAoJEDfy90M199hlokgP/2RVYjPiwM0EcE9pAdGP3Uqa
qkCqj5cPq/5SdLH553h/Xd6056qwchoMJYQo9RZzzVR4wS4HPyeZ00e33RrJZbtm
MGvjat1NJWEyl03y1AwAy9c9yHCRJa5vQlKtm0v2xWDG3xXqwejz/SK2ijw2IWVW
W1/7OnObbquHa7a+IsO1ndXoI0jECzOFDXY6YzFVJuPAf3asOHT44lJbHkfUq3Kv
k5eK14Xrb3kcYqhBQg+eG50lr1aRDj8yDlYQuoGmw/dg/X0TyAo2v+AOaFrN8rXD
igsYaMDafsXY55izkiRuNfcYZAnHC7hNVt+ffV+wLycCTiaEEflp+BCxLTYmh53T
xDB39Lr0Sz7AP77l0M9hbMr3Ao3GA1KFOc9OfRN11eSo5Y6uDtpMeOIFBAke6hxl
DanWPc/YmXzacga99xzOQglDZkDWTohWsEDwniRJmi7UC0Z/gwZ2P60OnwE1lqbd
eOmUu0JZCwklInWoDo9XmWdfp9+OrviGNm0vhQbplhEm/LC9PqBB/DOy44QjSv84
jfY/iMPn8uvGqWiQ/65za1O/1QsRukgp5PVnj7TyNojSskSuAYOF5BcFVIdB7krj
ZKHChreUMVw1nH8py4HkdPOXTHmAItV9/9T2c/UUuJWAECiLy+tIY/if+Tzi+Zn6
nRm99YM401PAsRKLyn0m
=gYmH
-----END PGP SIGNATURE-----

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/28/2015 11:50 AM, Stephen Frost wrote:

* Joe Conway (joe.conway@crunchydata.com) wrote:

On 07/03/2015 10:03 AM, Noah Misch wrote:

(4) When DefineQueryRewrite() is about to convert a table to a
view, it checks the table for features unavailable to views.
For example, it rejects tables having triggers. It omits to
reject tables having relrowsecurity or a pg_policy record.
Test case:

Please see the attached patch for this issue. Comments?

Looks good to me.

Thanks -- pushed to HEAD and 9.5

Joe

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVuA+UAAoJEDfy90M199hlrtYQAJGVucRtarWTQoP39of8IYXE
OoAi+q6CKEJWEJdVHmnnCwMc6y0lx0+Qugpb5SAVqMG/0oYvZ/bnvvN5OHMxujCI
bw7A/eJ62BILAt07Oczs1gtt+k4PT1001c4jFjgSC6AV51cZ9UM+d0b4C2YhKcm7
B4tycmC/yGxa5UFT6vdOhneILzBeOMLxgMXKouoD7Gnf4UKh1KUWxxWKU9q0Kl7d
mvRJke1wj/+WfquO09g33+1jHCsnbV4fcQ1q8RQbFPItpqcp4alsuuIYzLiPRbbs
VtZYurFLpjKdJg8OuHw9IYNbDjPZoEuv5eV16aSjOJxnngcXROxwXOwZMVaCuQMq
1D5FoUunJqsqlxVKggJJLfBt9uM/gafjmnRHGBZftfX/A3ZM6c6YewA68Nxo+rTE
xtgA+n1lzWsahje0n2KSFUNRSCqdWcnLQ/HtnVcNjGdFdCxeUDQ5kUAE1hFCMCXe
5eqAvohQQ25RiurZ1rI1IfUoeAPRDp3nvgcMMM7FQMmv6oKNBLr2RivVz0ZE17vd
Htz0y0cPx8mJgEHKMJrV/yF9odECTxevBzkO5rASLCLnGHEYp8WZfqWO/s/HoujS
KU99lfzOfnyBIyl2zIGSmkmCvUIqaP1cUP5xMHIedNhjDRy/Rt6IxwA9qEtgUokI
sC6BWHpxd19RAh5NLXCK
=NFOR
-----END PGP SIGNATURE-----

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 07/03/2015 10:03 AM, Noah Misch wrote:

(6) AlterPolicy() calls InvokeObjectPostAlterHook(PolicyRelationId, ...), but
CreatePolicy() and DropPolicy() lack their respective hook invocations.

Patch attached. Actually AlterPolicy() was also missing its hook -- the
existing InvokeObjectPostAlterHook() was only in rename_policy().

I'm not 100% sure about the hook placement -- would appreciate if
someone could confirm I got it correct.

Joe

On 07/03/2015 10:03 AM, Noah Misch wrote:

(7) Using an aggregate function in a policy predicate elicits an inapposite
error message due to use of EXPR_KIND_WHERE for parse analysis. Need a new
ParseExprKind. Test case:

Patch attached. Comments?

Joe

On 29 July 2015 at 02:36, Joe Conway <joe.conway@crunchydata.com> wrote:

On 07/03/2015 10:03 AM, Noah Misch wrote:

(6) AlterPolicy() calls InvokeObjectPostAlterHook(PolicyRelationId, ...), but
CreatePolicy() and DropPolicy() lack their respective hook invocations.

Patch attached. Actually AlterPolicy() was also missing its hook -- the
existing InvokeObjectPostAlterHook() was only in rename_policy().

I'm not 100% sure about the hook placement -- would appreciate if
someone could confirm I got it correct.

The CreatePolicy() and AlterPolicy() changes look OK to me, but the
RemovePolicyById() change looks to be unnecessary ---
RemovePolicyById() is called only from doDeletion(), which in turned
is called only from deleteOneObject(), which already invokes the drop
hooks. So ISTM that RemovePolicyById() doesn't need to do anything,
right?

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 29 July 2015 at 05:02, Joe Conway <joe.conway@crunchydata.com> wrote:

On 07/03/2015 10:03 AM, Noah Misch wrote:

(7) Using an aggregate function in a policy predicate elicits an inapposite
error message due to use of EXPR_KIND_WHERE for parse analysis. Need a new
ParseExprKind. Test case:

Patch attached. Comments?

I don't think there is any point in adding the new function
transformPolicyClause(), which is identical to transformWhereClause().
You can just use transformWhereClause() with EXPR_KIND_POLICY. It's
already used for lots of other expression kinds.

There are a couple of places in test_rls_hooks.c that also need updating.

I think that check_agglevels_and_constraints() and
transformWindowFuncCall() could be made to emit more targeted error
messages for EXPR_KIND_POLICY, for example "aggregate functions are
not allowed in policy USING and WITH CHECK expressions".

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers