Re: [HACKERS] Row Level Security Documentation

[Stephen Frost]

Dean,

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> On 26 September 2017 at 00:42, Stephen Frost  wrote:
> > That's a relatively minor point, however, and I do feel that this patch
> > is a definite improvement.  Were you thinking of just applying this for
> > v10, or back-patching all or part of it..?
> 
> I was planning on back-patching it to 9.5, taking out the parts
> relating the restrictive policies as appropriate. Currently the 9.5
> and 9.6 docs are identical, as are 10 and HEAD, and 9.5/9.6 only
> differs from 10/HEAD in a couple of places where they mention
> restrictive policies. IMO we should stick to that, making any
> improvements available in the back-branches. I was also thinking the
> same about the new summary table, although I haven't properly reviewed
> that yet.

Makes sense to me.

+1

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security Documentation

[Dean Rasheed]

On 26 September 2017 at 00:42, Stephen Frost  wrote:
> * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
>> Attached is a proposed patch...
>
> I've taken a look through this and generally agree with it.

Thanks for looking at this.

> the bits inside  ...  tags should be
> consistently capitalized (you had one case of 'All' that I noticed)

Yes, that's a typo. Will fix.

> I wonder if it'd be better to have the "simple" description *first*
> instead of later on, eg, where you have "Thus the overall result is
> that" we might want to try and reword things to decribe it as "Overall,
> it works thusly, ..., and the specifics are ...".

OK, that makes sense. I'll try it that way round.

> That's a relatively minor point, however, and I do feel that this patch
> is a definite improvement.  Were you thinking of just applying this for
> v10, or back-patching all or part of it..?

I was planning on back-patching it to 9.5, taking out the parts
relating the restrictive policies as appropriate. Currently the 9.5
and 9.6 docs are identical, as are 10 and HEAD, and 9.5/9.6 only
differs from 10/HEAD in a couple of places where they mention
restrictive policies. IMO we should stick to that, making any
improvements available in the back-branches. I was also thinking the
same about the new summary table, although I haven't properly reviewed
that yet.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Row Level Security Documentation

[Stephen Frost]

Dean,

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> On 5 August 2017 at 10:03, Fabien COELHO  wrote:
> > Patch applies cleanly, make html ok, new table looks good to me.
> 
> So I started looking at this patch, but before even considering the
> new table proposed, I think there are multiple issues that need to be
> resolved with the current docs:
> 
> Firstly, there are 4 separate places in the current CREATE POLICY docs
> that say that multiple policies are combined using OR, which is of
> course no longer correct, since PG10 added RESTRICTIVE policy support.

Ah, indeed, you're right there, those should have been updated to
indicate that it was the default or perhaps clarify the difference, but
I agree that doing so all over the place isn't ideal.

> In fact, it wasn't even strictly correct in 9.5/9.6, because if
> multiple different types of policy apply to a single command (e.g.
> SELECT and UPDATE policies, being applied to an UPDATE command), then
> they are combined using AND. Rather than trying to make this correct
> in 4 different places, I think there should be a single new section of
> the docs that explains how multiple policies are combined. This will
> also reduce the number of places where the pre- and post-v10 docs
> differ, making them easier to maintain.

Agreed, that makes a lot of sense to me.

> The proposed patch distinguishes between UPDATE/DELETE commands that
> have WHERE or RETURNING clauses, and those that don't, claiming that
> the former require SELECT permissions and the latter don't. That's
> based on a couple of statements from the current docs, but it's not
> entirely accurate. For example, "UPDATE foo SET a=NULL WHERE true
> RETURNING now()" does not require SELECT permissions despite having
> both WHERE and RETURNING clauses (OK, I admit that's a rather
> contrived example, but still...), and conversely (a more realistic
> example) "UPDATE foo SET a=b+c" does require SELECT permissions
> despite not having WHERE or RETURNING clauses. I think we need to try
> to be more precise about when SELECT permissions are required.

Agreed.

> In the notes section, there is a note about there needing to be at
> least one permissive policy for restrictive policies to take effect.
> That's well worth pointing out, however, I fear that this note is
> buried so far down the page (amongst some other very wordy notes) that
> readers will miss it. I suggest moving it to the parameters section,
> where permissive and restrictive policies are first introduced,
> because it's a pretty crucial fact to be aware of when using these new
> types of policy.

Ok.

> Attached is a proposed patch to address these issues, along with a few
> other minor tidy-ups.

I've taken a look through this and generally agree with it.  I'll note
that the bits inside  ...  tags should be
consistently capitalized (you had one case of 'All' that I noticed) and
I wonder if it'd be better to have the "simple" description *first*
instead of later on, eg, where you have "Thus the overall result is
that" we might want to try and reword things to decribe it as "Overall,
it works thusly, ..., and the specifics are ...".

That's a relatively minor point, however, and I do feel that this patch
is a definite improvement.  Were you thinking of just applying this for
v10, or back-patching all or part of it..?  For my 2c, at least, I'm
pretty open to clarifying things in the back-branches (and we have
technically had restrictive policies for a while, they just required
using an extension, so even those pieces are relevant for older
versions, but might need additional caveats...).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security Documentation

[Dean Rasheed]

On 5 August 2017 at 10:03, Fabien COELHO  wrote:
> Patch applies cleanly, make html ok, new table looks good to me.
>

So I started looking at this patch, but before even considering the
new table proposed, I think there are multiple issues that need to be
resolved with the current docs:

Firstly, there are 4 separate places in the current CREATE POLICY docs
that say that multiple policies are combined using OR, which is of
course no longer correct, since PG10 added RESTRICTIVE policy support.
In fact, it wasn't even strictly correct in 9.5/9.6, because if
multiple different types of policy apply to a single command (e.g.
SELECT and UPDATE policies, being applied to an UPDATE command), then
they are combined using AND. Rather than trying to make this correct
in 4 different places, I think there should be a single new section of
the docs that explains how multiple policies are combined. This will
also reduce the number of places where the pre- and post-v10 docs
differ, making them easier to maintain.

In the 6th paragraph of the description section, the terminology used
is mixed up and does not match the terminology used elsewhere
("command" instead of "policy" and "policy" instead of "expression").

The description of UPDATE policies lists the commands that the policy
applies to (UPDATE and INSERT ... ON CONFLICT DO UPDATE), but fails to
mention SELECT FOR UPDATE and SELECT FOR SHARE.

The proposed patch distinguishes between UPDATE/DELETE commands that
have WHERE or RETURNING clauses, and those that don't, claiming that
the former require SELECT permissions and the latter don't. That's
based on a couple of statements from the current docs, but it's not
entirely accurate. For example, "UPDATE foo SET a=NULL WHERE true
RETURNING now()" does not require SELECT permissions despite having
both WHERE and RETURNING clauses (OK, I admit that's a rather
contrived example, but still...), and conversely (a more realistic
example) "UPDATE foo SET a=b+c" does require SELECT permissions
despite not having WHERE or RETURNING clauses. I think we need to try
to be more precise about when SELECT permissions are required.

In the notes section, there is a note about there needing to be at
least one permissive policy for restrictive policies to take effect.
That's well worth pointing out, however, I fear that this note is
buried so far down the page (amongst some other very wordy notes) that
readers will miss it. I suggest moving it to the parameters section,
where permissive and restrictive policies are first introduced,
because it's a pretty crucial fact to be aware of when using these new
types of policy.

Attached is a proposed patch to address these issues, along with a few
other minor tidy-ups.

Regards,
Dean


create-policy-docs.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Row Level Security Documentation

[Fabien COELHO]

Hello Rod,

Patch applies cleanly, make html ok, new table looks good to me.

I've turned it "Ready for Committer".

Thanks!

--
Fabien.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Row Level Security Documentation

[Rod Taylor]

On Thu, Jul 13, 2017 at 5:49 AM, Fabien COELHO  wrote:

>
> Hello Rod,
>
> This version of the table attempts to stipulate which section of the
>> process the rule applies to.
>>
>
> The table should be referenced from the description, something like "Table
> xxx summarizes the ..."
>

Added the below which seemed consistent with other "see something else"
messages.

A summary of the application of policies to a command is found in .



> ISTM that it would be clearer to split the Policy column into "FOR xxx
> ..." and "USING" or "WITH CHECK", and to merge the rows which have the same
> "FOR xxx ..." contents, something like:
>
>POLICY |
>   ---++-
>  | USING  | ...
>   FOR ALL ...++-
>  | WITH CHECK | ...
>   ---++-
>   FOR SELECT ... | USING  | ...
>
> So that it is clear that only ALL & UPDATE can get both USING & WITH
> CHECK. This can be done with "morerows=1" on an entry so that it spans more
> rows.
>

Done. I couldn't figure out a morecols=1 equivalent to keep everything
under the Policy heading without a full colspec.



> For empty cells, maybe a dash would be clearer. Not sure.


Looked cluttered to me. Tried N/A first which was even worse.

-- 
Rod Taylor
diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml
index c0dfe1ea4b..52a868e65d 100644
--- a/doc/src/sgml/ref/create_policy.sgml
+++ b/doc/src/sgml/ref/create_policy.sgml
@@ -94,6 +94,11 @@ CREATE POLICY name ON default deny policy is assumed, so that no rows will
be visible or updatable.
   
+
+  
+   A summary of the application of policies to a command is found
+   in .
+  
  
 
  
@@ -389,6 +394,80 @@ CREATE POLICY name ON 
 

+
+   Policies Applied During Statement
+
+ 
+ 
+ 
+ 
+ 
+ 
+ 
+  
+   Policy
+   SELECT
+   INSERT
+   UPDATE
+   DELETE
+  
+ 
+ 
+  
+   FOR ALL ...
+   USING
+   WHERE clause
+   RETURNING clause
+   WHERE and RETURNING clause
+   WHERE and RETURNING clause
+  
+  
+   WITH CHECK
+   
+   new tuple
+   new tuple
+   new tuple
+  
+  
+   FOR SELECT ... USING
+   WHERE clause
+   RETURNING clause
+   WHERE and RETURNING clause
+   WHERE and RETURNING clause
+  
+  
+   FOR INSERT ... WITH CHECK
+   
+   new tuple
+   
+   
+  
+  
+   FOR UPDATE ...
+   USING
+   
+   
+   WHERE clause
+   
+  
+  
+   WITH CHECK
+   
+   
+   new tuple
+   
+  
+  
+   FOR DELETE ... USING
+   
+   
+   
+   WHERE clause
+  
+ 
+
+   
+
   
  
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Row Level Security Documentation

[Fabien COELHO]

Hello Rod,


This version of the table attempts to stipulate which section of the
process the rule applies to.


A few comments about this patch. It applies cleanly, make html is ok.

It adds a summary table which shows for each case what happens. Although 
the information can be guessed/infered from the text, I think that a 
summary table is a good thing, at least because it breaks an otherwise 
dense presentation.


I would suggest the following changes:

The table should be referenced from the description, something like "Table 
xxx summarizes the ..."


ISTM that it would be clearer to split the Policy column into "FOR xxx 
..." and "USING" or "WITH CHECK", and to merge the rows which have the 
same "FOR xxx ..." contents, something like:


   POLICY |
  ---++-
 | USING  | ...
  FOR ALL ...++-
 | WITH CHECK | ...
  ---++-
  FOR SELECT ... | USING  | ...

So that it is clear that only ALL & UPDATE can get both USING & WITH 
CHECK. This can be done with "morerows=1" on an entry so that it spans 
more rows.


For empty cells, maybe a dash would be clearer. Not sure.

--
Fabien


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Row Level Security Documentation

[Rod Taylor]

Of course, better thoughts appear immediately after hitting the send button.

This version of the table attempts to stipulate which section of the
process the rule applies to.

On Thu, May 11, 2017 at 8:40 PM, Rod Taylor  wrote:

> I think the biggest piece missing is something to summarize the giant
> blocks of text.
>
> Attached is a table that has commands and policy types, and a "yes" if it
> applies.
>
> --
> Rod Taylor
>



-- 
Rod Taylor
diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml
index 3b24e5e95e..4c997a956d 100644
--- a/doc/src/sgml/ref/create_policy.sgml
+++ b/doc/src/sgml/ref/create_policy.sgml
@@ -389,6 +389,72 @@ CREATE POLICY name ON 
 

+
+   Policies Applied During Statement
+
+ 
+  
+   Policy
+   SELECT
+   INSERT
+   UPDATE
+   DELETE
+  
+ 
+ 
+  
+   FOR ALL ... USING
+   WHERE clause
+   RETURNING clause
+   WHERE and RETURNING clause
+   WHERE and RETURNING clause
+  
+  
+   FOR ALL ... WITH CHECK
+   
+   new tuple
+   new tuple
+   new tuple
+  
+  
+   FOR SELECT ... USING
+   WHERE clause
+   RETURNING clause
+   WHERE and RETURNING clause
+   WHERE and RETURNING clause
+  
+  
+   FOR INSERT ... WITH CHECK
+   
+   new tuple
+   
+   
+  
+  
+   FOR UPDATE ... USING
+   
+   
+   WHERE clause
+   
+  
+  
+   FOR UPDATE ... WITH CHECK
+   
+   
+   new tuple
+   
+  
+  
+   FOR DELETE ... USING
+   
+   
+   
+   WHERE clause
+  
+ 
+
+   
+
   
  
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers