Re: [HACKERS] Row Level Security UPDATE Confusion

Rod,

* Rod Taylor (rod.taylor@gmail.com) wrote:
> UPDATE seems to work as described (unable to create records you cannot
> select); both the single rule version and multi-rule version seem to work
> the same.

I'm glad they work the same now, as they were always intended to.

Regarding the "rules", they're actually based on what the ACL rules are.
In particular, an UPDATE with a WHERE clause requires SELECT rights- and
so the code mandates that the UPDATE can see both the row-to-be-updated
and the row-post-updating, but you're able to INSERT records which you
can't then see, as an INSERT doesn't generally require SELECT
privileges.

> This combination works too though it seems funny that UPDATE can create an
> entry it cannot reverse. I can set the value to 100 (going to -1 fails) but
> the UPDATE cannot see the record to set it back. I can see use cases for
> it, for example you might be able to promote someone to manager but not
> demote a manager to front-desk. We also allow INSERT on tables you cannot
> delete from, so it's not inconsistent.
>
> CREATE POLICY split_select ON t FOR SELECT TO split USING (value > 0);
> CREATE POLICY split_update ON t FOR UPDATE TO split USING (value < 10) WITH
> CHECK (value > 2);
> SET session authorization split;
> update t set value = 100 where value = 4; -- 1 record changed
> update t set value = 5 where value = 100; -- 0 records changed

Being able to UPDATE records to change them in a way that you're not
able to subsequently UPDATE them seems reasonable to me, at least.

> However, despite INSERT also functioning the same for both styles of
> commands it's definitely not obeying the `cannot give away records` rule:

Right, this is because INSERT doesn't generally require SELECT
privileges, and therefore doesn't require the USING clause to be
checked also.

We could certainly debate if the approach of applying policies based on
the privileges required for the command is the "correct" approach, but
it's definitely what was intended and generally works well, based on
what I've seen thus far in terms of actual usage.  Admittedly, it's a
bit of an edge case which doesn't come up very often either, so perhaps
we should consider changing this down the road, but for now we should go
ahead and fix the obvious unintentional bug in the code around ALL
policies and back-patch that as a bug fix, we can then consider if
changes should be made here in future releases independently.

Assuming there aren't objections to that, I'll plan to push this fix
later tonight or tomorrow.

Thanks!

Stephen