Re: Add support for restrictive RLS policies

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> 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.

No worries, took me a while to get back to adding the requested
documentation too.

> I do have a couple of
> comments relating to the documentation and one relating to the code:

Thanks!

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

I agree, that's better, will update.

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

Works for me, I'll add that, and remove the quotes around restrictive
and permissive.

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

Hmmm, is it really the case that the quals will always end up being
evaluated in that order though? Isn't order_qual_clauses() going to end
up changing the order based on the relative cost? If the cost is the
same it should maintain the order, but even that could change in the
future based on the comments, no? In short, I'm not entirely sure that
we actually want to be required to always evaluate the quals in order of
policy name and we might get complaints if we happen to make that work
today and it ends up being changed later.

Thanks!

Stephen