Re: Add support for restrictive RLS policies

On 30 November 2016 at 21:54, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Unless there's further comments, I'll plan to push this sometime
> tomorrow.
>

Sorry I didn't have time to look at this properly. I was intending to,
but my day job just keeps getting in the way. I do have a couple of
comments relating to the documentation and one relating to the code:

- row-level security policy.
+ row-level security policy. Note that only the set of roles which the
+ policy applies to and the <literal>USING</literal> and
+ <literal>WITH CHECK</literal> expressions are able to be changed using
+ <command>ALTER POLICY</command>. To change other properties of a policy,
+ such as the command it is applied for or if it is a permissive or
+ restrictive policy, the policy must be dropped and recreated.

This note reads a little awkwardly to me. I think I would write it as:

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.

+ <term><replaceable class="parameter">PERMISSIVE</replaceable></term>
+ <listitem>
+ <para>
+ Specify that the policy is to be created as a "permissive" policy.
+ All "permissive" policies which are applicable to a given query will
+ be combined together using the boolean "OR" operator. By creating
+ "permissive" policies, administrators can add to the set of records
+ which can be accessed. Policies are PERMISSIVE by default.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><replaceable class="parameter">RESTRICTIVE</replaceable></term>
+ <listitem>
+ <para>
+ Specify that the policy is to be created as a "restrictive" policy.
+ All "restrictive" policies which are applicable to a given query will
+ be combined together using the boolean "AND" operator. By creating
+ "restrictive" policies, administrators can reduce the set of records
+ which can be accessed as all "restrictive" policies must be passed for
+ each record.
+ </para>
+ </listitem>
+ </varlistentry>

I don't think this or anywhere else makes it entirely clear that the
user needs to have at least one permissive policy in addition to any
restrictive policies for this to be useful. I think this section is
probably a good place to mention that, since it's probably the first
place people will read about restrictive policies. I think it also
needs to be spelled out exactly how a mix of permissive and
restrictive policies are combined, because there is more than one way
to combine a set of quals with ANDs and ORs (although only one way
really makes sense in this context). So perhaps an additional note
along the lines of:

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

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

In get_policies_for_relation(), after the loop that does this:

- *permissive_policies = lappend(*permissive_policies, policy);
+ {
+ if (policy->permissive)
+ *permissive_policies = lappend(*permissive_policies, policy);
+ else
+ *restrictive_policies = lappend(*restrictive_policies, policy);
+ }

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.

Regards,
Dean