Re: INSERT ... ON CONFLICT UPDATE and RLS

On Wed, Jan 7, 2015 at 12:26 PM, Stephen Frost <sfrost@snowman.net> wrote:
> I agree with that up to a point- this case feels more like a POLA
> violation though rather than a run-of-the-mill "you need to test all
> that."  I'm a bit worried this might lead to cases where users can't use
> UPSERT because they don't know which set of policies will end up getting
> applied, although the above approach is strictly a subset of the
> approach I'm advocating, but at least with the 'throw it all together'
> case, you know exactly what you're getting with UPSERT.

I think that it makes sense to bunch the policies together, because
INSERT ... ON CONFLICT UPDATE is mostly an INSERT - informally
speaking, the "UPDATE part" is just enough to resolve a conflict -
this is ultimately just a new clause of INSERT. Besides, I think that
making the permissions as restrictive as possible is the least
surprising behavior. It's clearly the most secure behavior.

I think that we collectively lean towards actively throwing an error
as the preferred behavior - I think that's good, because the intent of
the user to UPDATE some particular tuple that they're not allowed to
UPDATE is clear and unambiguous. As long as we're doing that, clearly
writing a query that will fail based on the "wrong" path being taken
is wrong-headed. Now, you can construct an UPSERT case that a "bunch
together policies" approach doesn't serve well. I could be mistaken,
but I think that case is likely to be more than a little contrived.

We're now going to be testing the TARGET.* tuple before going to
update, having failed to insert (and after row locking the TARGET.*
tuple) - the target tuple is really how the update finds the existing
row to update, so it should be tested, without necessarily being
blamed directly (certainly, we cannot leak the TARGET.* tuple values,
so maybe we should try blaming the EXCLUDED.* tuple first for error
messaging reasons). If it fails because of the insert check after row
locking but before update, well, it was liable to anyway, when the
insert path was taken. Conversely, if it failed after actually
inserting where that would not happen with a vanilla INSERT (due to a
bunched-together update "USING()" security barrier qual or explicit
user-supplied CHECK OPTION), well, you'd have gotten the same thing if
you took the UPDATE path anyway.

That just leaves the regular ExecUpdate() call of
ExecWithCheckOptions(). When that is called with "bunched together"
policies, you're now testing the final tuple. So it might be a bit
confusing that the policy of final tuples originating from INSERTs is
also enforced here, for the final tuple of an "UPDATE". But in order
for even that to be the case, you'd have to have a final tuple that
was substantially different from the one originally proposed for
insertion that made the difference between a qual passing and not
passing. How likely is that in realistic use cases? And besides, isn't
the policy for INSERTs still quite relevant even here? We're talking
about a final tuple that originated from an INSERT statement, after
all.

-- 
Peter Geoghegan