Re: CREATE POLICY and RETURNING

On Fri, Oct 17, 2014 at 5:34 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-10-17 14:57:03 +0800, Craig Ringer wrote:
>> On 10/17/2014 02:49 AM, Robert Haas wrote:
>> > I think you could probably make the DELETE policy control what can get
>> > deleted, but then have the SELECT policy further filter what gets
>> > returned.
>>
>> That seems like the worst of both worlds to me.
>>
>> Suddenly DELETE ... RETURNING might delete more rows than it reports a
>> resultset for. As well as being potentially dangerous for people using
>> it in wCTEs, etc, to me that's the most astonishing possible outcome of all.
>>
>> I'd be much happier with even:
>>
>> ERROR: RETURNING not permitted with SELECT row-security policy
>
> FWIW, that doesn't sound acceptable to me.

This is more or less what ended up happening with UPSERT and USING
security barrier quals on UPDATE/ALL policies. Realistically, the
large majority of use cases don't involve a user being able to
INSERT/DELETE tuples, but not SELECT them, and those that do should
not be surprised to have a RETURNING fail (it's an odd enough union of
different features that this seems acceptable to me).

Like Fujii, I think that RETURNING with RLS should not get to avoid
SELECT policies. I agree with the concern about not seeing affected
rows with a DELETE (which, as I said, is very similar to UPSERT +
WCO_RLS_CONFLICT_CHECK policies), so an error seems like the only
alternative.

The argument against not requiring SELECT *column* privilege on the
EXCLUDED.* pseudo relation for UPSERT might have been: "well, what can
be the harm of allowing the user to see what they themselves might
have inserted?". But that would have been a bad argument then had
anyone made it, because RETURNING with a (vanilla) INSERT requires
SELECT privilege, and that's also what the user then actually inserted
(as distinct from what the user *would have* inserted had the insert
path been taking, representing as the EXCLUDED.* pseudo relation --
for security purposes, ISTM that this is really no distinction at
all). Consider before row insert triggers that can modify EXCLUDED.*
tuples in a privileged way.

So, the only logical reason that INSERT with RETURNING requires SELECT
column privilege that I can see is that a before row INSERT trigger
could modify the tuple inserted in a way that the inserter role should
not know the details of. This long standing convention was reason
enough to mandate that SELECT column privilege be required for the
EXCLUDED.* pseudo relation for UPSERT. And so, I think it isn't too
much of a jump to also say that we should do the same for RLS (for
INSERTs for the reason I state, but also for UPDATEs and DELETEs for a
far more obvious reason: the *existing* tuple can be projected, and
the updater/deleter might well have no business seeing its contents).

In short: I think we should be tracking a new WCOKind (perhaps
WCO_RLS_RETURNING_CHECK?), that independently holds the security
barrier quals as WCO-style checks when that's recognized as being
necessary. For INSERT, these WCOs must be enforced against the target
tuple projected by RETURNING. For UPDATEs and DELETEs, FROM/USING
relations must also have SELECT privilege enforced against the
projected target tuple, as well as the non-target relation --
apparently the latter isn't currently happening, although Dean has
tried to address this with his recent patch [1]. That is, even
non-target relations (UPDATE ... FROM relations, or DELETE ... USING
relations) do not have SELECT policy enforcement, but rather have
UPDATE or DELETE policy enforcement only. I must admit that I was
rather surprised at that; it has to be a bug.

[1] http://www.postgresql.org/message-id/CAEZATCVE7hdtfZGCJN-oevVaWBtBGG8-fBCh9VhDBHuZrsWY5w@mail.gmail.com
--
Peter Geoghegan