From: Stephen Frost <sfrost@snowman.net>
To: Jeevan Chalke <jeevan.chalke@enterprisedb.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Thom Brown <thom@linux.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: Add support for restrictive RLS policies
Date: Tue, 27 Sep 2016 17:17:05 -0400
Message-ID: <20160927211705.GS5148@tamriel.snowman.net> (raw)
In-Reply-To: <CAM2+6=WaYrso+K4spreugPBxmBGNSwycXaS1B2a-dPieawJLwQ@mail.gmail.com>
References: <20160908192157.GA53172@alvherre.pgsql>
<20160908200912.GW4028@tamriel.snowman.net>
<5611.1473369685@sss.pgh.pa.us>
<CA+Tgmoaf9A5dS7rdoxsyQe6=rxH=nNSV1pfcz-gRd_p8cSiOZQ@mail.gmail.com>
<20160912015748.GF11453@tamriel.snowman.net>
<CAM2+6=X_Fq24ukRo_1hcxqy00MPP6b=HPPT3avSV8N=oz6CB8A@mail.gmail.com>
<CAM2+6=VZhP5hyWAwQWDLKhKcb7SQFRKDvnvckswH19Dte2ra9w@mail.gmail.com>
<20160926192733.GI5148@tamriel.snowman.net>
<CAM2+6=UHCuD_wT2kK_H2zL_9k8XabVLxOUaBgWcWqsSvBP1gXg@mail.gmail.com>
<CAM2+6=WaYrso+K4spreugPBxmBGNSwycXaS1B2a-dPieawJLwQ@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Jeevan,
* Jeevan Chalke (jeevan.chalke@enterprisedb.com) wrote:
> Here are the review comments:
[... many good comments ...]
Many thanks for the review, I believe I agree with pretty much all your
comments and will update the patch accordingly.
Responses for a couple of them are below.
> 6. I think following changes are irrelevant for this patch.
> Should be submitted as separate patch if required.
Those changes were adding support for tab completion of the new
restrictive / permissive options, which certainly seems appropriate for
the patch which is adding those options. I realize it looks like a lot
for just two new options, but that's because there's a number of ways to
get to them.
> 10. ALTER POLICY has no changes for this. Any reason why that is not
> considered here.
Generally speaking, I don't believe it makes sense to flip a policy from
permissive to restrictive or vice-versa, they're really quite different
things. We also don't support altering the "command" type for a policy.
> 12. Since PERMISSIVE is default, we should dump only "RESTRICTIVE" when
> appropriate, like other default cases.
> strcmp(polinfo->polpermissive,"t") == 0 ? "PERMISSIVE" : "RESTRICTIVE"
Sure, we could do that, though I suppose we'd want to do that for all of
the defaults for policies.
> 13. Since PERMISSIVE is default, do we need changes like below?
> - \QCREATE POLICY p1 ON test_table FOR ALL TO PUBLIC \E
> + \QCREATE POLICY p1 ON test_table AS PERMISSIVE FOR ALL TO
> PUBLIC \E
>
> I am not familiar or used TAP framework. So no idea about these changes.
If we change pg_dump to not output AS PERMISSIVE for permissive
policies, then the TAP test will need to be updated to not include AS
PERMISSIVE (or FOR ALL TO PUBLIC, if we're going down that route).
Thanks!
Stephen
Attachments:
signature.asc (application/pgp-signature, 818B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sfrost@snowman.net, jeevan.chalke@enterprisedb.com, robertmhaas@gmail.com, tgl@sss.pgh.pa.us, alvherre@2ndquadrant.com, thom@linux.com
Subject: Re: Add support for restrictive RLS policies
In-Reply-To: <20160927211705.GS5148@tamriel.snowman.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox