Thread: [HACKERS] Row Level Security Documentation

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

Hello Rod,

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

I've turned it "Ready for Committer".

Thanks!

-- 
Fabien.

On 5 August 2017 at 10:03, Fabien COELHO <coelho@cri.ensmp.fr> 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

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

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 5 August 2017 at 10:03, Fabien COELHO <coelho@cri.ensmp.fr> 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 <literal> ... </literal> 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

On 26 September 2017 at 00:42, Stephen Frost <sfrost@snowman.net> wrote:
> * Dean Rasheed (dean.a.rasheed@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 <literal> ... </literal> 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

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 26 September 2017 at 00:42, Stephen Frost <sfrost@snowman.net> 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

On 5 August 2017 at 10:03, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
> Hello Rod,
>
> Patch applies cleanly, make html ok, new table looks good to me.
>
> I've turned it "Ready for Committer".
>

I didn't really finish committing this in the last commitfest, before
getting distracted by a security issue, so returning to it now...

I like Rod's original idea of a summary table here, to save reading
through a lot of text, and I think it's handy to be able to see at a
glance which polices apply to which commands, and vice versa. However,
I'm not entirely convinced by the contents of the table, as proposed.

The meaning of the contents of the table cells isn't entirely clear.
For example, the SELECT/FOR ALL USING cell contains "WHERE clause",
which I presume means the policy expressions are added to the query's
WHERE clause. But the INSERT/FOR ALL USING cell contains "RETURNING
clause", meaning the policy is used if there is a RETURNING clause.
Then the INSERT/FOR ALL WITH CHECK cell contains "new tuple". So the
table cells seem to be providing answers to different orthogonal
questions.

I think the contents of each cell should provide the answer to a
single question, or a consistent set of questions. IMO the principal
question is "does this policy apply to this command?", although that
can be expanded to include "... and if so, which tuple is tested?" so
then the principal content of each cell would be "Existing row" or
"New row", or blank if the policy doesn't apply.

There's a minor complication that some cases like SELECT policies for
UPDATE commands don't always apply. I think that can be covered by a
suitable footnote.

The set of table columns (commands) currently only includes SELECT,
INSERT, UPDATE and DELETE. I think that should be expanded to include
SELECT FOR UPDATE/SHARE and ON CONFLICT DO UPDATE. I think it also
makes sense to include INSERT ... RETURNING as a separate command,
since it almost always requires SELECT permissions, whereas a bare
INSERT never does. I don't think it's worth including the RETURNING
variants of the other commands, since they typically require SELECT
permissions even without a RETURNING clause.

The set of table rows (policy types) currently includes FOR ALL as a
separate policy type. Whilst that might be technically correct, I
think it's more useful to think of the set of policy types as
SELECT/ALL, INSERT/ALL, UPDATE/ALL and DELETE/ALL because ALL policies
effectively just behave like one or more of the other policy types
depending on the context. Doing it that way then ties in better with
the way multiple policies are combined. For example, for an UPDATE
command, there are 2 sets of policies that get applied (combined using
AND) -- the SELECT/ALL policies and the UPDATE/ALL policies. It's not
so useful to regards ALL as a separate type, and it would be wrong to
AND together the ALL policies with the SELECT policies and the UPDATE
policies.

With the above changes, there are more command types than policy
types, so it also makes sense to swap the orientation of the table.

Here's an updated patch, with the table done that way. Comments?

Regards,
Dean