Re: WIP patch (v2) for updatable security barrier views

On 01/29/2014 08:29 PM, Dean Rasheed wrote:
> On 29 January 2014 11:34, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> On 01/23/2014 06:06 PM, Dean Rasheed wrote:
>>> On 21 January 2014 09:18, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>>> Yes, please review the patch from 09-Jan
>>>> (http://www.postgresql.org/message-id/CAEZATCUiKxOg=vOOvjA2S6G-sixzzxg18ToTggP8zOBq6QnQHQ@mail.gmail.com)
>>>>
>>>
>>> After further testing I found a bug --- it involves having a security
>>> barrier view on top of a base relation that has a rule that rewrites
>>> the query to have a different result relation, and possibly also a
>>> different command type, so that the securityQuals are no longer on the
>>> result relation, which is a code path not previously tested and the
>>> rowmark handling was wrong. That's probably a pretty obscure case in
>>> the context of security barrier views, but that code path would be
>>> used much more commonly if RLS were built on top of this. Fortunately
>>> the fix is trivial --- updated patch attached.
>>
>> This is the most recent patch I see, and the one I've been working on
>> top of.
>>
>> Are there any known tests that this patch fails?
>>
>
> None that I've been able to come up with.

I've found an issue. I'm not sure if it can be triggered from SQL, but
it affects in-code users who add their own securityQuals.

expand_security_quals fails to update any rowMarks that may point to a
relation being expanded. If the relation isn't the target relation, this
causes a rowmark to refer to a RTE with no relid, and thus failure in
the executor.

Relative patch against updatable s.b. views attached (for easier
reading), along with a new revision of updatable s.b. views that
incorporates the patch.

This isn't triggered by FOR SHARE / FOR UPDATE rowmark on a security
barrier view because the flattening to securityQuals and re-expansion in
the optimizer is only done for _target_ security barrier views. For
target views, different rowmark handling applies, so they don't trigger
it either.

This is triggered by adding securityQuals to a non-target relation
during row-security processing, though.

> Sounds sensible. Feel free to add any test cases you think up to the
> wiki page. Even if we don't like this design, any alternative must at
> least pass all the tests listed there.

Eh, much better to add directly to src/regress/ IMO. If they're on the
wiki they can get overlooked/forgotten.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services