Re: RLS open items are vague and unactionable

On 13 September 2015 at 22:54, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Not in front of my laptop and will review it when I get back in more detail,
> but the original approach that I tried was changing
> get_policies_for_relation to try and build everything necessary, which
> didn't work as we need to OR the various ALL/SELECT policies together and
> then AND the result to apply the filtering.
>

Yes that wouldn't work because get_policies_for_relation() isn't the
place for that. It has a clear responsibility which is to build and
return 2 lists of policies (permissive and restrictive) for the
specified relation, command type and user role. It doesn't have any
say in what is done with those policies (how their quals get
combined), it just fetches them.

It shouldn't be necessary to change get_policies_for_relation() at all
to support the RETURNING check. You just need to call it with
CMD_SELECT. BTW, your change to change get_policies_for_relation() has
a bug -- if the policy is for ALL commands it gets added
unconditionally to the list of returning_policies, regardless of
whether it applies to the current user role. That's the risk of
complicating the function by trying to make it do more than it was
designed to do.

> Seems like that might not be an issue with your proposed approach, but
> wouldn't we need to either pass in fresh lists and then append them to the
> existing lists, or change the functions to work with non-empty lists? (I had
> thought about changing that anyway since there's often cases where it's
> useful to be able to call a function which adds to an existing list).
> Further, actually, we'd still have to figure out how to build up the OR'd
> qual from the ALL/SELECT policies and then add that to the restrictive set.
> That didn't appear easy to do from get_policies_for_relation as all the
> ChangeVarNode work is handled in the build_* functions, which have the info
> necessary.
>

No, the lists of policies built and returned by
get_policies_for_relation() should not be appended to any other lists.
That would have the effect of OR'ing the SELECT policy quals with the
UPDATE/DELETE policy quals, which is wrong. The user needs to have
both SELECT and UPDATE/DELETE permissions on each row, so the OR'ed
set of permissive SELECT quals needs to be AND'ed with the OR'ed set
of permissive UPDATE/DELETE quals. The build_* functions do precisely
what is needed for that, and shouldn't need changing either (other
than the rename discussed previously).

So for example, build^H^H^Hadd_security_quals() takes 2 lists of
policies (permissive and restrictive), combines them in the right way
using OR and AND, does the necessary varno manipulation, and adds the
resulting quals to the passed-in list of security quals (thereby
implicitly AND'ing the new quals with any pre-existing ones), which is
precisely what's needed to support RETURNING.

>> Then it isn't necessary to modify get_policies_for_relation(),
>> build_security_quals() and build_with_check_options() to know anything
>> specific about returning. They're just another set of permissive and
>> restrictive policies to be fetched and added to the command.
>
>
> The ALL/SELECT policies need to be combined with OR's (just like all
> permissive sets of policies) and then added to the restrictive set of quals,
> to ensure that they are evaluated as a restriction and not just another set
> of permissive policies, which wouldn't provide the required filtering.
>

Right, and that's what the add_* functions do. The separation of
concerns between get_policies_for_relation() and the add_* functions
was intended to make just this kind of change trivial at a higher
level.

I think this should actually be 2 separate commits, since the
refactoring and the support for RETURNING are entirely different
things. It just happens that after the refactoring, the RETURNING
patch becomes trivial (4 new executable lines of code wrapped in a
couple of if statements, to fetch and then apply the new policies in
the necessary cases). At least that's the theory :-)

Regards,
Dean