Received: from malur.postgresql.org ([217.196.149.56])
	by arkaria.postgresql.org with esmtp (Exim 4.84_2)
	(envelope-from
 <pgsql-hackers-owner+M311838=pgsql-hackers-arkari@postgresql.org>)
	id 1cDxon-0006xl-He
	for pgsql-hackers@arkaria.postgresql.org; Mon, 05 Dec 2016 18:16:49 +0000
Received: from localhost ([127.0.0.1] helo=postgresql.org)
	by malur.postgresql.org with smtp (Exim 4.84_2)
	(envelope-from
 <pgsql-hackers-owner+M311838=pgsql-hackers-arkari@postgresql.org>)
	id 1cDxom-0000mX-Vn
	for pgsql-hackers@arkaria.postgresql.org; Mon, 05 Dec 2016 18:16:49 +0000
Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29])
	by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256)
	(Exim 4.84_2)
	(envelope-from <sfrost@tamriel.snowman.net>)
	id 1cDxol-0000kz-7H
	for pgsql-hackers@postgresql.org; Mon, 05 Dec 2016 18:16:47 +0000
Received: from tamriel.snowman.net ([72.66.115.51])
	by magus.postgresql.org with esmtp (Exim 4.84_2)
	(envelope-from <sfrost@tamriel.snowman.net>)
	id 1cDxoi-0002VX-74
	for pgsql-hackers@postgresql.org; Mon, 05 Dec 2016 18:16:46 +0000
Received: by tamriel.snowman.net (Postfix, from userid 1000)
	id 8624D5F7A0; Mon,  5 Dec 2016 13:15:42 -0500 (EST)
Date: Mon, 5 Dec 2016 13:15:42 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
	Jeevan Chalke <jeevan.chalke@enterprisedb.com>,
	Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Thom Brown <thom@linux.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	Haribabu Kommi <kommi.haribabu@gmail.com>
Subject: Re: Add support for restrictive RLS policies
Message-ID: <20161205181542.GD23417@tamriel.snowman.net>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Qrgsu6vtpU/OV/zm"
Content-Disposition: inline
In-Reply-To: 
 <CAEZATCVp6P0qmOEtiDWmMOrfcOvnhR_razna5Sa6YQaD27pdZg@mail.gmail.com>
 <CAEZATCWMD2gaJCzLtxu0v79JCvJL4cUjbNMENjLTrBP07zz_fw@mail.gmail.com>
 <CAEZATCURYzb8M_DMsmKQ9LQ+rZUmM=3irATtLgkRxopw=j=yjg@mail.gmail.com>
X-Editor: Vim http://www.vim.org/
X-Info: http://www.snowman.net
X-Operating-System: Linux/3.13.0-91-generic (x86_64)
X-Uptime: 13:11:24 up 149 days, 19:35, 28 users,  load average: 0.19, 0.13,
 0.08
User-Agent: Mutt/1.5.21 (2010-09-15)
X-Pg-Spam-Score: -3.4 (---)
List-Archive: <http://archives.postgresql.org/pgsql-hackers>
List-Help: <mailto:majordomo@postgresql.org?body=help>
List-ID: <pgsql-hackers.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@postgresql.org>
List-Post: <mailto:pgsql-hackers@postgresql.org>
List-Subscribe: <mailto:majordomo@postgresql.org?body=sub%20pgsql-hackers>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
X-Mailing-List: pgsql-hackers
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--Qrgsu6vtpU/OV/zm
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> This note reads a little awkwardly to me. I think I would write it as:
>=20
>     Note that <command>ALTER POLICY</command> only allows the set of roles
>     to which the policy applies and the <literal>USING</literal> and
>     <literal>WITH CHECK</literal> expressions to be modified.  To change
>     other properties of a policy, such as the command to which it applies
>     or whether it is permissive or restrictive, the policy must be dropped
>     and recreated.

Done.

[...]
> really makes sense in this context). So perhaps an additional note
> along the lines of:
>=20
>     Note that there needs to be at least one permissive policy to grant
>     access to records before restrictive policies can be usefully used to
>     reduce that access. If only restrictive policies exist, then no recor=
ds
>     will be accessible. When a mix of permissive and restrictive policies
>     are present, a record is only accessible if at least one of the
>     permissive policies passes, in addition to all the restrictive
>     policies.

Done.

> Also, I don't think it's necessary to keep quoting "restrictive" and
> "permissive" here.

Quotes removed.

> In get_policies_for_relation(), after the loop that does this:
[...]
> I think it should sort the restrictive policies by name, for the same
> reason that hook-restrictive policies are sorted -- so that the WITH
> CHECK expressions are checked in a well-defined order (which was
> chosen to be consistent with the order of checking of other similar
> things, like CHECK constraints and triggers). I also think that this
> should be a separate sort step from the sort for hook policies,
> inserted just after the loop above, so that the order is all internal
> policies sorted by name, followed by all hook policies sorted by name,
> to be consistent with the existing principle that hook policies are
> applied after internal policies.

Done, adjusted the comments a bit also and added to the regression tests
to test that the sorting is happening as expected.

> I looked through this in a little more detail and I found one other
> issue: the documentation for the system catalog table pg_policy and
> the view pg_policies needs to be updated to include the new columns
> that have been added.

I had noticed this also while going through it again, but thanks again
for the thorough review!

> Other than that, it all looks good to me, subject to the previous comment=
s.

Updated patch attached.

I'll push this in a bit.

Thanks to all who helped!

Stephen

--Qrgsu6vtpU/OV/zm
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJYRa7OAAoJEO1sijiDR2RVZRAP/j/X742oZEtDaUT5A0i+fF2d
eU5aoes3JFvJv3fNfKOdCWtdBzhW0oyQJrbXtkAskyII2Fv+ndTYvLwRGQeLY5v8
jVbNMB22Erg5N8+WgjODq7c4b2YP0KGofwo1LrPYuWoH/LW2Iiqq+mxfJiuGcEZF
T651NuTMnxt0Ec/Kj2SWWhjMVSd9wQ37oG3hiOm1Xa648vdZZMVDBL83iUHfx59v
7bx2KAHfFpYHKBOALiJvhnDfD9d2FFA249/PfnOiBH2ss6UwRZuneLSolRtsUGhC
rtdGSx/9i1yUS+akuCCv2viPYyu3We/ktYYkpcDn0zJG87Ra7CnkdfvPXov0OUWg
1MvCxghLYiu0t4VMLLYFjeJBKkwHhnK4OEI7UEmZTbWmop2UAfR8zs/IzE8a1LoL
JHWw4S2Wah2Hon60OKLBH9XWnaY4dImLG/3RrtK20KwaIAWeQDi6PcXw8VbpSszK
4BYP5qtFqYT/Y/SuBOAePQi4nUEdiZB3A9N8zx9HCUcwh/MnMmAV+55d03nfSYo7
+G/xRpZOF/hx4+LnJqr5Td5EKA0BM/cWVC+oGF3tJqEaBIqZ9pQP2d9NmfQKeVLH
/APq0qELsw7q3F49pnghL1DGoAHRX+wS9FhPBISaCYm8hxKpQ517+5Jx5jwXzPyS
6bTv6BlEaUDdpq8JvXM2
=6xyZ
-----END PGP SIGNATURE-----

--Qrgsu6vtpU/OV/zm--