Date: Mon, 26 Sep 2016 16:55:54 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Stephen Frost <sfrost@snowman.net>
Cc: 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>
Subject: Re: Add support for restrictive RLS policies
Message-ID: <20160926195554.GA176476@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20160926193725.GJ5148@tamriel.snowman.net>
User-Agent: Mutt/1.5.23 (2014-03-12)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

Stephen Frost wrote:

Stephen, the typo "awseome" in the tests is a bit distracting.  Can you
please fix it?

> Updated patch changes to using IDENT and an strcmp() (similar to
> AlterOptRoleElem and vacuum_option_elem) to check the results at parse-time,
> and then throwing a more specific error if an unexpected value is found
> (instead of just 'syntax error').  This avoids adding any keywords.

Thanks.

> diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml
> index 89d2787..d930052 100644
> --- a/doc/src/sgml/ref/create_policy.sgml
> +++ b/doc/src/sgml/ref/create_policy.sgml
> @@ -22,6 +22,7 @@ PostgreSQL documentation
>   <refsynopsisdiv>
>  <synopsis>
>  CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable class="parameter">table_name</replaceable>
> +    [ AS ( PERMISSIVE | RESTRICTIVE ) ]

I think you should use braces here, not parens:

    [ AS { PERMISSIVE | RESTRICTIVE } ]

>     <varlistentry>
> +    <term><replaceable class="parameter">permissive</replaceable></term>
> +    <listitem>
> +     <para>
> +      If the policy is a "permissive" or "restrictive" policy.  Permissive
> +      policies are the default and always add visibillity- they ar "OR"d
> +      together to allow the user access to all rows through any of the
> +      permissive policies they have access to.  Alternativly, a policy can
> +      instead by "restrictive", meaning that the policy will be "AND"d
> +      with other restrictive policies and with the result of all of the
> +      permissive policies on the table.
> +     </para>
> +    </listitem>
> +   </varlistentry>

I don't think this paragraph is right -- you should call out each of the
values PERMISSIVE and RESTRICTIVE (in upper case) instead.  Also note
typos "Alternativly" and "visibillity".

I dislike the "AND"d and "OR"d spelling of those terms.  Currently they
only appear in comments within rowsecurity.c (of your authorship too, I
imagine).  I think it'd be better to find actual words for those
actions.

> diff --git a/src/include/catalog/pg_policy.h b/src/include/catalog/pg_policy.h
> index d73e9c2..30dc367 100644
> --- a/src/include/catalog/pg_policy.h
> +++ b/src/include/catalog/pg_policy.h
> @@ -23,6 +23,7 @@ CATALOG(pg_policy,3256)
>  	NameData	polname;		/* Policy name. */
>  	Oid			polrelid;		/* Oid of the relation with policy. */
>  	char		polcmd;			/* One of ACL_*_CHR, or '*' for all */
> +	bool		polpermissive;	/* restrictive or permissive policy */
>  
>  #ifdef CATALOG_VARLEN
>  	Oid			polroles[1];	/* Roles associated with policy, not-NULL */
> @@ -42,12 +43,13 @@ typedef FormData_pg_policy *Form_pg_policy;
>   *		compiler constants for pg_policy
>   * ----------------
>   */
> -#define Natts_pg_policy				6
> -#define Anum_pg_policy_polname		1
> -#define Anum_pg_policy_polrelid		2
> -#define Anum_pg_policy_polcmd		3
> -#define Anum_pg_policy_polroles		4
> -#define Anum_pg_policy_polqual		5
> -#define Anum_pg_policy_polwithcheck 6
> +#define Natts_pg_policy					6
> +#define Anum_pg_policy_polname			1
> +#define Anum_pg_policy_polrelid			2
> +#define Anum_pg_policy_polcmd			3
> +#define Anum_pg_policy_polpermissive	4
> +#define Anum_pg_policy_polroles			5
> +#define Anum_pg_policy_polqual			6
> +#define Anum_pg_policy_polwithcheck 	7

I don't understand this part.  Didn't you say previously that we already
had this capability in 9.5 and you were only exposing it over SQL?  If
that is true, how come you need to add a new column to this catalog?

-- 
Álvaro Herrera                https://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