Re: CREATE POLICY and RETURNING

On 8 June 2015 at 16:53, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> I definitely don't like the idea of returning a portion of the data in a
> RETURNING clause. Returning an error if the RETURNING clause ends up
> not passing the SELECT policy is an interesting idea, but I do have
> doubts about how often that will be a useful exercise. Another issue to
> note is that, currently, SELECT policies never cause errors to be
> returned, and this would change that.
>

True. Before UPSERT was added, it was the case that USING clauses from
all kinds of policies didn't cause errors, and CHECK clauses did, but
that has now changed for UPDATE, and I don't think it's necessarily a
problem to change it for SELECT, if we decide that it makes sense to
apply SELECT policies to rows returned by RETURNING clauses.

> There was discussion about a VISIBILITY policy instead of a SELECT
> policy at one point. What I think we're seeing here is confusion about
> the various policies which exist, because the USING policy of an UPDATE
> is precisely its VISIBILITY policy, in my view, which is why I wasn't
> bothered by the RETURNING clause being able to be used in that case. I
> recall working on the documentation to make this clear, but it clearly
> needs work.
>

Yeah, perhaps there's scope for improving the documentation,
regardless of whether or not we change the current behaviour. One
place that we could add additional documentation is the pages
describing the INSERT, UPDATE and DELETE commands. These currently
each have a paragraph in their main description sections describing
what privileges they require, so perhaps we should add similar
paragraphs describing what RLS policies are checked, if RLS is enabled
on the table.

> The primary use-case for having a different VISIBILITY for UPDATE (or
> DELETE) than for SELECT is that you wish to allow users to only modify a
> subset of the rows in the table which they can see. I agree that the
> current arrangement is that this can be used to allow users to UPDATE
> records which they can't see (except through a RETURNING). Perhaps
> that's a mistake and we should, instead, force the SELECT policy to be
> included for the UPDATE and DELETE policies and have the USING clauses
> from those be AND'd with the SELECT policy. That strikes me as a much
> simpler approach than applying the SELECT policy against the RETURNING
> clause and then throwing an error if it fails,

I don't think that quite addresses the concern with RETURNING though
because the resulting clauses would only be applied to the old
(pre-update) data, whereas a RETURNING clause might still return new
data that you couldn't otherwise see.

> though this would remove
> a bit of flexibility in the system since we would no longer allow blind
> UPDATEs or DELETEs. I'm not sure if that's really an issue though- to
> compare it to our GRANT-based system, the only blind UPDATE allowed
> there is one which goes across the entire table- any WHERE clause used
> results in a check to ensure you have SELECT rights.
>

That's not quite the extent of it. Column-level privileges might allow
you to SELECT the PK column of a table, and then you might be able to
UPDATE or DELETE specific rows despite not being able to see their
entire contents. I can see cases where that might be useful, but I
have to admit that I'm struggling to think of any case where the
row-level equivalent would make sense (although that, by itself is not
a good argument for not allowing it).

> Ultimately, we're trying to provide as much flexibility as possible
> while having the system be easily understandable by the policy authors
> and allowing them to write simple policies to enforce the necessary
> constraints. Perhaps allowing the open-ended USING clauses for UPDATE
> and DELETE is simply making for a more complicated system as policy
> authors then have to make sure they embed whatever SELECT policy they
> have into their UPDATE and DELETE policies explicitly- and the result if
> they don't do that correctly is that users may be able to update/delete
> things they really shouldn't be able to. Now, as we've discussed
> previously, users should be expected to test their systems and make sure
> that they are behaving as they expect, but we don't want to put
> landmines in either or have ways which they can be easily tripped up.
>

Well I'm all for keeping this as simple and easily understandable as
possible. Right now the USING clauses of UPDATE policies control which
existing rows can be updated and the CHECK clauses control what new
data can go in. That's about as simple as it could be IMO, and trying
to merge SELECT policies with either of those would only make it more
complicated, and hence make it more likely for users to get it wrong.

OTOH, when you tag a RETURNING clause onto the UPDATE, it's kind of
like tying an UPDATE and a SELECT together, and it does make sense to
me that the new rows returned by the SELECT part of the command be
constrained by any SELECT policies on the table. That's consistent
with what happens with GRANT-based privileges, so I don't think that
it should be that surprising or difficult for users to understand.

I also think that if it ever were the case that a row resulting from
an UPDATE were not visible to the user who made that update, then that
would most likely indicate that the policies were poorly-defined.
Ideally the UPDATE policy's CHECK clause would prevent the user from
making such an update. Having a RETURNING clause that was checked
against the table's SELECT policy would then actually provide the user
with a way to validate the consistency of their policy clauses.
Hitting this new RETURNING-SELECT-POLICY error would be a sign that
perhaps the WITH CHECK clauses on your policies aren't properly
constraining new data.

So I actually think this additional check would increase the chances
of policies being defined correctly, and I think that consistency with
GRANT-based privileges will make it easier to understand.

Regards,
Dean