Re: unclear about row-level security USING vs. CHECK

Good morning

> -----Original Message-----
> From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Stephen Frost
> Sent: Montag, 28. September 2015 21:16
> To: Peter Eisentraut <peter_e@gmx.net>; Robert Haas <robertmhaas@gmail.com>
> Cc: pgsql-hackers <pgsql-hackers@postgresql.org>; Charles Clavadetscher <clavadetscher@swisspug.org>
> Subject: Re: [HACKERS] unclear about row-level security USING vs. CHECK
> 
> * Peter Eisentraut (peter_e@gmx.net) wrote:
> > I see.  But it is a bit odd to hide this very fundamental behavior
> > somewhere in a paragraph that starts out with something about roles.
> 
> I'm happy to change that.  You're right, it should be a paragraph by
> itself.
> 
> > There is also a mistake, I believe: DELETE policies also take both a
> > CHECK and a USING clause.
> 
> DELETE never adds records and therefore does not take a CHECK clause,
> only a USING clause:
> 
> =*# create policy p1 on t1 for delete using (c1 > 5) with check (c1 > 10);
> ERROR:  WITH CHECK cannot be applied to SELECT or DELETE
> 
> There has been some discussion about changing that, but that would be a
> future change and not for 9.5.
> 
> > I still find something about this weird, but I'm not sure what.  It's
> > not clear to me at what level this USING->CHECK mapping is applied.  I
> > can write FOR ALL USING and it will be mapped to CHECK for all actions,
> > including INSERT, but when I write FOR INSERT USING it complains.  Why
> > doesn't it do the mapping that case, too?
> 
> INSERT is only adding records and therefore only the CHECK policy
> applies:
> 
> =*# create policy p1 on t1 for insert using (c1 > 5) with check (c1 > 10);
> ERROR:  only WITH CHECK expression allowed for INSERT
> 
> The USING clause is for existing records while the CHECK option is for
> new records, which is why DELETE only has a USING clause and INSERT only
> has a WITH CHECK clause.  ALL allows you to specify clauses for all
> commands, which is why it accepts both.  The only other case which
> allows both is UPDATE, where records are both retrived and added.
> 
> > >> (Btw., what's the meaning of a policy for DELETE?)
> > >
> > > The DELETE policy controls what records a user is able to delete.
> >
> > That needs to be documented somewhere.
> 
> This is included in the CREATE POLICY documentation:
> 
> DELETE
> 
>     Using DELETE for a policy means that it will apply to DELETE
>     commands. Only rows which pass this policy will be seen by a DELETE
>     command. Rows may be visible through a SELECT which are not seen by
>     a DELETE, as they do not pass the USING expression for the DELETE,
>     and rows which are not visible through the SELECT policy may be
>     deleted if they pass the DELETE USING policy. The DELETE policy only
>     accepts the USING expression as it only ever applies in cases where
>     records are being extracted from the relation for deletion.
> 
> I'm certainly all for improving the documentation, of course.  What
> about the above isn't clear regarding what DELETE policies do?  Or is
> the issue that it wasn't covered in ddl-rowsecurity?  Perhaps we should
> simply move much of the CREATE POLICY documentation into ddl-rowsecurity
> instead, since that's where people seem to be looking for this
> information?

I think that many people will look first into ddl-rowsecurity to get an understanding what it can do and how it can be
used.
Detailed information is then in the CREATE POLICY doc. So it could make sense to move parts that contribute to
understandthe
 
mechanics as a whole from the CREATE POLICY doc to ddl-rowsecurity. As an alternative, when it comes to the
characteristicsof a
 
specific command, a link to the place in CREATE POLICY doc may be enough. Just no duplicated information. That would be
difficultto
 
keep in sync.

> * Robert Haas (robertmhaas@gmail.com) wrote:
> > On Sat, Sep 26, 2015 at 9:46 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> > > On 9/23/15 3:41 PM, Stephen Frost wrote:
> > > I see.  But it is a bit odd to hide this very fundamental behavior
> > > somewhere in a paragraph that starts out with something about roles.
> > >
> > > There is also a mistake, I believe: DELETE policies also take both a
> > > CHECK and a USING clause.
> > >
> > > I still find something about this weird, but I'm not sure what.  It's
> > > not clear to me at what level this USING->CHECK mapping is applied.  I
> > > can write FOR ALL USING and it will be mapped to CHECK for all actions,
> > > including INSERT, but when I write FOR INSERT USING it complains.  Why
> > > doesn't it do the mapping that case, too?
> >
> > We are really pushing our luck only hammering this stuff out now.  But
> > I think I agree with Peter's concerns, FWIW.
> 
> I listed out the various alternatives but didn't end up getting any
> responses to it.  I'm still of the opinion that the documentation is the
> main thing which needs improving here, but we can also change CREATE
> POLICY, et al, to require an explicit WITH CHECK clause for the commands
> where that makes sense if that's the consensus.

True, sorry.

1) keep it as-is
2) make WITH CHECK mandatory
3) keep WITH CHECK optional, but default it to 'false' instead
4) new grammar: USING AND WITH CHECK (<expression>) (suggested by Peter Eisentraut)

My first thought is that the whole statement should not just help, but also force people to think what they are doing.

The improvements to the documentation should be enough to keep it as-is (option 1). Making a WITH CHECK mandatory also
forcases
 
that don't really make sense would be more confusing than helping. My second suitable candidate would be 3, because I
thinkthat
 
restrictions that are not expressed explicitly should not be more permissive than the one expressed. Option 4 is nice
asa short
 
form when <expression> is the same and maybe even less confusing. Since this ends up being the same as omitting WITH
CHECKin the
 
current implementation, it may lead again to confusion, unless it becomes mandatory to declare both USING and WITH
CHECKfor ALL and
 
UPDATE. So, option 4 only together with mandatory WITH CHECK.

As everybody else, howevere, I will welcome what consensus brings.

Bye
Charles