Re: Add support for restrictive RLS policies - Mailing list pgsql-hackers

Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Stephen Frost wrote:
>
> Stephen, the typo "awseome" in the tests is a bit distracting.  Can you
> please fix it?

Will fix.

> > 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.

No problem.

> > 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 } ]

Will fix.

> >     <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".

Will fix, along with the 'ar' typo.

> 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.

I'm certainly open to suggestions, should you, or anyone else, have
them.  I'll try and come up with something else, but that really is what
we're doing- literally using either AND or OR to join the expressions
together.

> > 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?

The capability exists in 9.5 through hooks which are available to
extensions, see the test_rls_hooks extension.  Those hooks are called
every time and therefore don't require the information about restrictive
policies to be tracked in pg_policy, and so they weren't.  Since this is
adding support for users to define restrictive policies, we need to save
those policies and therefore we need to distinguish which policies are
restrictive and which are permissive, hence the need to modify pg_policy
to track that information.

Thanks!

Stephen