Re: RLS open items are vague and unactionable - Mailing list pgsql-hackers
| From | Stephen Frost |
|---|---|
| Subject | Re: RLS open items are vague and unactionable |
| Date | |
| Msg-id | CAOuzzgqJKtci=JpghE74z1ra273L+EFejn-XERYvYMq4O914tg@mail.gmail.com Whole thread Raw |
| In response to | Re: RLS open items are vague and unactionable (Dean Rasheed <dean.a.rasheed@gmail.com>) |
| Responses |
Re: RLS open items are vague and unactionable
(Dean Rasheed <dean.a.rasheed@gmail.com>)
|
| List | pgsql-hackers |
Dean,
On Sunday, September 13, 2015, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Sunday, September 13, 2015, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 13 September 2015 at 20:59, Stephen Frost <sfrost@snowman.net> wrote:
> I've been through this and definitely like it more than what we had
> previously, as I had mentioned previously. I've also added the
> RETURNING handling, per the discussion between you, Robert, Tom, and
> Kevin.
>
> Would be great to get your feedback both on the relatively minor changes
> which I made to your refactoring patch (mostly cosmetic) and how I added
> in the handling for the RETURNING case, which was made much simpler and
> cleaner by the refactoring. (Working through adding the RETURNING also
> helped my understanding of the refactoring, which I feel comfortable
> that I understand well now.)
>
Cool. I haven't looked in detail yet, but I expected the changes
necessary to support RETURNING to be much simpler than that. Isn't it
just a case of adding something like
if (root->returningList &&
commandType == CMD_UPDATE or CMD_DELETE)
{
get_policies_for_relation(rel, CMD_SELECT, ...)
build_security_quals(...)
}
and then something similar with build_with_check_options() for the upsert case?
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.
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.
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.
One change I thought about making was renaming build_security_quals()
and build_with_check_options() to add_security_quals() and
add_with_check_options(), because they're adding to lists rather than
returning lists, and in general they are called multiple times to
build up the complete lists.
That sounds reasonable to me.
Thanks!
Stephen
pgsql-hackers by date: