Re: Possible typo in create_policy.sgml

Hi,

Following is perhaps a typo:

- qualifications of queries which are run against the table the policy
is on,
+ qualifications of queries which are run against the table if the
policy is on,

Attached fixes it if so.

Thanks,
Amit

On Tue, Jan 6, 2015 at 12:26 AM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Following is perhaps a typo:
>
> - qualifications of queries which are run against the table the policy
> is on,
> + qualifications of queries which are run against the table if the
> policy is on,
>
> Attached fixes it if so.

I don't think that's a typo, although it's not particularly
well-worded IMHO. I might rewrite the whole paragraph like this:

A policy limits the ability to SELECT, INSERT, UPDATE, or DELETE rows
in a table to those rows which match the relevant policy expression.
Existing table rows are checked against the expression specified via
USING, while new rows that would be created via INSERT or UPDATE are
checked against the expression specified via WITH CHECK. Generally,
the system will enforce filter conditions imposed using security
policies prior to qualifications that appear in the query itself, in
order to the prevent the inadvertent exposure of the protected data to
user-defined functions which might not be trustworthy. However,
functions and operators marked by the system (or the system
administrator) as LEAKPROOF may be evaluated before policy
expressions, as they are assumed to be trustworthy.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert, Amit,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> I don't think that's a typo, although it's not particularly
> well-worded IMHO. I might rewrite the whole paragraph like this:
>
> A policy limits the ability to SELECT, INSERT, UPDATE, or DELETE rows
> in a table to those rows which match the relevant policy expression.
> Existing table rows are checked against the expression specified via
> USING, while new rows that would be created via INSERT or UPDATE are
> checked against the expression specified via WITH CHECK. Generally,
> the system will enforce filter conditions imposed using security
> policies prior to qualifications that appear in the query itself, in
> order to the prevent the inadvertent exposure of the protected data to
> user-defined functions which might not be trustworthy. However,
> functions and operators marked by the system (or the system
> administrator) as LEAKPROOF may be evaluated before policy
> expressions, as they are assumed to be trustworthy.

Looks reasonable to me. Amit, does this read better for you? If so, I
can handle making the change to the docs.

Thanks!

Stephen

On Tue, Jan 6, 2015 at 11:25 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Looks reasonable to me. Amit, does this read better for you? If so, I
> can handle making the change to the docs.

The docs also prominently say:

"The security-barrier qualifications will always be evaluated prior to
any user-defined functions or user-provided WHERE clauses, while the
with-check expression will be evaluated against the rows which are
going to be added to the table. By adding policies to a table, a user
can limit the rows which a given user can select, insert, update, or
delete. This capability is also known as Row Level Security or RLS."

I would prefer it if it was clearer based on the syntax description
which qual is which. The security barrier qual "expression" should
have an identifier/name in the syntax description that is more
suggestive of "security barrier qual", emphasizing its distinctness
from "check_expression". For example, I think "barrier_expression"
would be clearer.
--
Peter Geoghegan

On Tue, Jan 6, 2015 at 2:48 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Tue, Jan 6, 2015 at 11:25 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> Looks reasonable to me. Amit, does this read better for you? If so, I
>> can handle making the change to the docs.
>
> The docs also prominently say:
>
> "The security-barrier qualifications will always be evaluated prior to
> any user-defined functions or user-provided WHERE clauses, while the
> with-check expression will be evaluated against the rows which are
> going to be added to the table. By adding policies to a table, a user
> can limit the rows which a given user can select, insert, update, or
> delete. This capability is also known as Row Level Security or RLS."
>
> I would prefer it if it was clearer based on the syntax description
> which qual is which. The security barrier qual "expression" should
> have an identifier/name in the syntax description that is more
> suggestive of "security barrier qual", emphasizing its distinctness
> from "check_expression". For example, I think "barrier_expression"
> would be clearer.

I thought my rewrite clarified this distinction pretty well. Maybe
I'm wrong? We're talking about the same paragraph.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jan 6, 2015 at 1:03 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I thought my rewrite clarified this distinction pretty well. Maybe
> I'm wrong? We're talking about the same paragraph.

Sorry, I didn't express myself clearly. I think that you did get it
right, but I would like to see that distinction also reflected in the
actual sgml PARAMETER class tag. "expression" is way too generic here.

--
Peter Geoghegan

I also don't see this behavior documented (this is from process_policies()):

/*
* If we end up with only USING quals, then use those as
* WITH CHECK quals also.
*/
if (with_check_quals == NIL)
with_check_quals = copyObject(quals);

Now, I do see a reference to it under "Per-Command policies - ALL". It says:

"If an INSERT or UPDATE command attempts to add rows to the table
which do not pass the ALL WITH CHECK (or USING, if no WITH CHECK
expression is defined) expression, the command will error."

But is that really the right place for it? Does it not equally well
apply to FOR UPDATE policies, that can on their own have both barriers
quals and WITH CHECK options? Basically, that seems to me like a
*generic* property of policies (it's a generic thing that the WITH
CHECK options/expressions "shadow" the USING security barrier quals as
check options), and so should be documented as such.

--
Peter Geoghegan

On 07-01-2015 AM 04:25, Stephen Frost wrote:
> Robert, Amit,
>
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> I don't think that's a typo, although it's not particularly
>> well-worded IMHO. I might rewrite the whole paragraph like this:
>>
>> A policy limits the ability to SELECT, INSERT, UPDATE, or DELETE rows
>> in a table to those rows which match the relevant policy expression.
>> Existing table rows are checked against the expression specified via
>> USING, while new rows that would be created via INSERT or UPDATE are
>> checked against the expression specified via WITH CHECK. Generally,
>> the system will enforce filter conditions imposed using security
>> policies prior to qualifications that appear in the query itself, in
>> order to the prevent the inadvertent exposure of the protected data to
>> user-defined functions which might not be trustworthy. However,
>> functions and operators marked by the system (or the system
>> administrator) as LEAKPROOF may be evaluated before policy
>> expressions, as they are assumed to be trustworthy.
>
> Looks reasonable to me. Amit, does this read better for you? If so, I
> can handle making the change to the docs.
>

Yes, it looks reasonable to me to.

Thanks,
Amit

On Tue, Jan 6, 2015 at 4:07 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Tue, Jan 6, 2015 at 1:03 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I thought my rewrite clarified this distinction pretty well. Maybe
>> I'm wrong? We're talking about the same paragraph.
>
> Sorry, I didn't express myself clearly. I think that you did get it
> right, but I would like to see that distinction also reflected in the
> actual sgml PARAMETER class tag. "expression" is way too generic here.

I'm happy to see us change that if it makes sense, but I'm not sure
what that actually does.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, Jan 6, 2015 at 4:07 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> > On Tue, Jan 6, 2015 at 1:03 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >> I thought my rewrite clarified this distinction pretty well. Maybe
> >> I'm wrong? We're talking about the same paragraph.
> >
> > Sorry, I didn't express myself clearly. I think that you did get it
> > right, but I would like to see that distinction also reflected in the
> > actual sgml PARAMETER class tag. "expression" is way too generic here.
>
> I'm happy to see us change that if it makes sense, but I'm not sure
> what that actually does.

If I'm following correctly, Peter's specifically talking about:

[ USING ( <replaceable class="parameter">expression</replaceable> ) ]
[ WITH CHECK ( <replaceable class="parameter">check_expression</replaceable> ) ]

Where the USING parameter is 'expression' but the WITH CHECK parameter
is 'check_expression'. He makes a good point, I believe, as
"expression" is overly generic. I don't like the idea of using
"barrier_expression" though as that ends up introducing a new term- how
about "using_expression"?

This would need to be reflected below in the documentation which talks
about "expression" as the parameter to "USING", but I can certainly
handle cleaning that up.

Thanks!

Stephen

On Wed, Jan 7, 2015 at 3:06 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> If I'm following correctly, Peter's specifically talking about:
>
> [ USING ( <replaceable class="parameter">expression</replaceable> ) ]
> [ WITH CHECK ( <replaceable class="parameter">check_expression</replaceable> ) ]
>
> Where the USING parameter is 'expression' but the WITH CHECK parameter
> is 'check_expression'. He makes a good point, I believe, as
> "expression" is overly generic. I don't like the idea of using
> "barrier_expression" though as that ends up introducing a new term- how
> about "using_expression"?

Oh. Well, I guess we could change that. I don't think it's a
problem, myself. I thought he was talking about something in the SGML
markup.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Peter Geoghegan (pg(at)heroku(dot)com) wrote:
> I also don't see this behavior documented (this is from process_policies()):
>
> /*
> * If we end up with only USING quals, then use those as
> * WITH CHECK quals also.
> */
> if (with_check_quals == NIL)
> with_check_quals = copyObject(quals);
>
> Now, I do see a reference to it under "Per-Command policies - ALL". It says:
>
> "If an INSERT or UPDATE command attempts to add rows to the table
> which do not pass the ALL WITH CHECK (or USING, if no WITH CHECK
> expression is defined) expression, the command will error."
>
> But is that really the right place for it? Does it not equally well
> apply to FOR UPDATE policies, that can on their own have both barriers
> quals and WITH CHECK options? Basically, that seems to me like a
> *generic* property of policies (it's a generic thing that the WITH
> CHECK options/expressions "shadow" the USING security barrier quals as
> check options), and so should be documented as such.

Ah, yes, good point, I can add more documentation around that.

Thanks!

Stephen

On Wed, Jan 7, 2015 at 12:06 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Where the USING parameter is 'expression' but the WITH CHECK parameter
> is 'check_expression'. He makes a good point, I believe, as
> "expression" is overly generic. I don't like the idea of using
> "barrier_expression" though as that ends up introducing a new term- how
> about "using_expression"?

Yes, I think "using_expression" works best.

--
Peter Geoghegan

On 6 January 2015 at 19:25, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Robert, Amit,
>
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> I don't think that's a typo, although it's not particularly
>> well-worded IMHO. I might rewrite the whole paragraph like this:
>>
>> A policy limits the ability to SELECT, INSERT, UPDATE, or DELETE rows
>> in a table to those rows which match the relevant policy expression.
>> Existing table rows are checked against the expression specified via
>> USING, while new rows that would be created via INSERT or UPDATE are
>> checked against the expression specified via WITH CHECK. Generally,
>> the system will enforce filter conditions imposed using security
>> policies prior to qualifications that appear in the query itself, in
>> order to the prevent the inadvertent exposure of the protected data to
>> user-defined functions which might not be trustworthy. However,
>> functions and operators marked by the system (or the system
>> administrator) as LEAKPROOF may be evaluated before policy
>> expressions, as they are assumed to be trustworthy.
>
> Looks reasonable to me. Amit, does this read better for you? If so, I
> can handle making the change to the docs.
>

I have a wider concern about the wording on this page - both the
rewritten paragraph and elsewhere talk about policies in terms of
limiting access to or filtering out rows.

However, since policy expressions are OR'ed together and there is a
default-deny policy when RLS is enabled, I think it should be talking
about policies in terms of permitting access to tables that have row
security enabled.

Regards,
Dean

On 8 January 2015 at 08:30, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> I have a wider concern about the wording on this page - both the
> rewritten paragraph and elsewhere talk about policies in terms of
> limiting access to or filtering out rows.
>
> However, since policy expressions are OR'ed together and there is a
> default-deny policy when RLS is enabled, I think it should be talking
> about policies in terms of permitting access to tables that have row
> security enabled.
>

[There's also a typo further down -- "filter out the records which are
visible", should be "not visible"]

What do you think of the attached rewording?

Regards,
Dean

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> [There's also a typo further down -- "filter out the records which are
> visible", should be "not visible"]
>
> What do you think of the attached rewording?

Rewording it this way is a great idea. Hopefully that will help address
the confusion which we've seen. The only comment I have offhand is:
should we should add a sentence to this paragraph about the default-deny
policy? I feel like that would help explain why the policies are
allowing access to rows.

Thanks!

Stephen

On 8 January 2015 at 18:57, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> What do you think of the attached rewording?
>
> Rewording it this way is a great idea. Hopefully that will help address
> the confusion which we've seen. The only comment I have offhand is:
> should we should add a sentence to this paragraph about the default-deny
> policy?
>

Yes, good idea, although I think perhaps that sentence should be added
to the preceding paragraph, after noting that RLS has to be enabled on
the table for the policies to be applied:

The <command>CREATE POLICY</command> command defines a new policy for a
table. Note that row level security must also be enabled on the table using
<command>ALTER TABLE</command> in order for created policies to be applied.
Once row level security has been enabled, a default-deny policy is
used and no rows
in the table are visible unless permitted by a specific policy.

A policy permits SELECT, INSERT, UPDATE or DELETE commands to access rows
in a table that has row level security enabled. Access to existing table
rows is granted if they match a policy expression specified via USING,
while new rows that would be created via INSERT or UPDATE are checked
against policy expressions specified via WITH CHECK. For policy
expressions specified via USING which grant access to existing rows, the
system will generally test the policy expressions prior to any
qualifications that appear in the query itself, in order to the prevent the
inadvertent exposure of the protected data to user-defined functions which
might not be trustworthy. However, functions and operators marked by the
system (or the system administrator) as LEAKPROOF may be evaluated before
policy expressions, as they are assumed to be trustworthy.

Also, perhaps the "ALTER TABLE" in the first paragraph should be
turned into a link.

Regards,
Dean

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 8 January 2015 at 18:57, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >> What do you think of the attached rewording?
> >
> > Rewording it this way is a great idea. Hopefully that will help address
> > the confusion which we've seen. The only comment I have offhand is:
> > should we should add a sentence to this paragraph about the default-deny
> > policy?
>
> Yes, good idea, although I think perhaps that sentence should be added
> to the preceding paragraph, after noting that RLS has to be enabled on
> the table for the policies to be applied:

I'm a bit on the fence about these ending up as different paragraphs
then, but ignoring that for the moment, I'd suggest we further clarify
with:

The <command>CREATE POLICY</command> command defines a new policy for a
table. Note that row level security must also be enabled on the table using
<command>ALTER TABLE</command> in order for created policies to be applied.
Once row level security has been enabled, a default-deny policy is used and
no rows in the table are visible, except to the table owner or
superuser, unless permitted by a specific policy.

A policy permits SELECT, INSERT, UPDATE or DELETE commands to access rows
in a table that has row level security enabled. Access to existing table
rows is granted if they match a policy expression specified via USING,
while new rows that would be created via INSERT or UPDATE are checked
against policy expressions specified via WITH CHECK. For policy
expressions specified via USING which grant access to existing rows, the
system will generally test the policy expressions prior to any
qualifications that appear in the query itself, in order to the prevent the
inadvertent exposure of the protected data to user-defined functions which
might not be trustworthy. However, functions and operators marked by the
system (or the system administrator) as LEAKPROOF may be evaluated before
policy expressions, as they are assumed to be trustworthy.

> Also, perhaps the "ALTER TABLE" in the first paragraph should be
> turned into a link.

Ah, yes, agreed.

Thanks!

Stephen

On 9 January 2015 at 20:46, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> I'd suggest we further clarify
> with:
>
> The <command>CREATE POLICY</command> command defines a new policy for a
> table. Note that row level security must also be enabled on the table using
> <command>ALTER TABLE</command> in order for created policies to be applied.
> Once row level security has been enabled, a default-deny policy is used and
> no rows in the table are visible, except to the table owner or
> superuser, unless permitted by a specific policy.
>
> A policy permits SELECT, INSERT, UPDATE or DELETE commands to access rows
> in a table that has row level security enabled. Access to existing table
> rows is granted if they match a policy expression specified via USING,
> while new rows that would be created via INSERT or UPDATE are checked
> against policy expressions specified via WITH CHECK. For policy
> expressions specified via USING which grant access to existing rows, the
> system will generally test the policy expressions prior to any
> qualifications that appear in the query itself, in order to the prevent the
> inadvertent exposure of the protected data to user-defined functions which
> might not be trustworthy. However, functions and operators marked by the
> system (or the system administrator) as LEAKPROOF may be evaluated before
> policy expressions, as they are assumed to be trustworthy.
>

Looks good to me.

Regards,
Dean

On Fri, Jan 9, 2015 at 3:46 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> A policy permits SELECT, INSERT, UPDATE or DELETE commands to access rows
> in a table that has row level security enabled. Access to existing table
> rows is granted if they match a policy expression specified via USING,
> while new rows that would be created via INSERT or UPDATE are checked
> against policy expressions specified via WITH CHECK. For policy
> expressions specified via USING which grant access to existing rows, the
> system will generally test the policy expressions prior to any
> qualifications that appear in the query itself, in order to the prevent the
> inadvertent exposure of the protected data to user-defined functions which
> might not be trustworthy. However, functions and operators marked by the
> system (or the system administrator) as LEAKPROOF may be evaluated before
> policy expressions, as they are assumed to be trustworthy.

I think that sticking "while new rows that would be created via INSERT
or UPDATE are checked against policy expressions specified via WITH
CHECK" into the middle of this is horribly confusing, as it's a
completely separate mechanism from the rest of what's being discussed
here. I think there needs to be some initial language that clarifies
that USING expressions apply to old rows and WITH CHECK expressions to
new rows, and then you can go into more detail. But mentioning WITH
CHECK parenthetically in the middle of the rest of this I think will
not lead to clarity.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Peter,

* Peter Geoghegan (pg(at)heroku(dot)com) wrote:
> I also don't see this behavior documented (this is from process_policies()):
[...]
> But is that really the right place for it? Does it not equally well
> apply to FOR UPDATE policies, that can on their own have both barriers
> quals and WITH CHECK options? Basically, that seems to me like a
> *generic* property of policies (it's a generic thing that the WITH
> CHECK options/expressions "shadow" the USING security barrier quals as
> check options), and so should be documented as such.

Thanks, you're right, the documentation there can be improved. I've
pushed up a change to clarify that the USING quals will be used for the
WITH CHECK case if no WITH CHECK quals are specified. Hopefully that's
clear now, but please let me know if you feel further improvements to
this would help.

I do think we will be making additional changes in this area before too
long, but good to get it cleaned up now anyway, so we don't forget to
do so later.

Thanks!

Stephen

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Fri, Jan 9, 2015 at 3:46 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > A policy permits SELECT, INSERT, UPDATE or DELETE commands to access rows
> > in a table that has row level security enabled. Access to existing table
> > rows is granted if they match a policy expression specified via USING,
> > while new rows that would be created via INSERT or UPDATE are checked
> > against policy expressions specified via WITH CHECK. For policy
> > expressions specified via USING which grant access to existing rows, the
> > system will generally test the policy expressions prior to any
> > qualifications that appear in the query itself, in order to the prevent the
> > inadvertent exposure of the protected data to user-defined functions which
> > might not be trustworthy. However, functions and operators marked by the
> > system (or the system administrator) as LEAKPROOF may be evaluated before
> > policy expressions, as they are assumed to be trustworthy.
>
> I think that sticking "while new rows that would be created via INSERT
> or UPDATE are checked against policy expressions specified via WITH
> CHECK" into the middle of this is horribly confusing, as it's a
> completely separate mechanism from the rest of what's being discussed
> here. I think there needs to be some initial language that clarifies
> that USING expressions apply to old rows and WITH CHECK expressions to
> new rows, and then you can go into more detail. But mentioning WITH
> CHECK parenthetically in the middle of the rest of this I think will
> not lead to clarity.

I agree, especially after going back and re-reading this while fixing
the issue mentioned earlier by Peter (which was an orthogonal complaint
about the shadowing of WITH CHECK by USING, if WITH CHECK isn't
specified). We really need a paragraph on "USING" policies and another
on "WITH CHECK" policies. How about a reword along these lines:

When row level security is enabled on a table, all access to that
table by users other than the owner or a superuser must be through a
policy. This requirement applies to both selecting out existing rows
from the table and to adding rows to the table (through either INSERT
or UPDATE).

Granting access to existing rows in a table is done by specifying a
USING expression which will be added to queries which reference the
table. Every row in the table which a USING expression returns true
will be visible.

Granting access to add rows to a table is done by specifying a WITH
CHECK expressison. A WITH CHECK expression must return true for
every row being added to the table or an error will be returned and
the command will be aborted.

For policy expressions specified via USING which grant access to
existing rows, the system will generally test the policy expressions
prior to any qualifications that appear in the query itself, in order
to the prevent the inadvertent exposure of the protected data to
user-defined functions which might not be trustworthy. However,
functions and operators marked by the system (or the system
administrator) as LEAKPROOF may be evaluated before policy
expressions, as they are assumed to be trustworthy.

Thoughts?

Thanks!

Stephen

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Wed, Jan 7, 2015 at 3:06 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > If I'm following correctly, Peter's specifically talking about:
> >
> > [ USING ( <replaceable class="parameter">expression</replaceable> ) ]
> > [ WITH CHECK ( <replaceable class="parameter">check_expression</replaceable> ) ]
> >
> > Where the USING parameter is 'expression' but the WITH CHECK parameter
> > is 'check_expression'. He makes a good point, I believe, as
> > "expression" is overly generic. I don't like the idea of using
> > "barrier_expression" though as that ends up introducing a new term- how
> > about "using_expression"?
>
> Oh. Well, I guess we could change that. I don't think it's a
> problem, myself. I thought he was talking about something in the SGML
> markup.

I agree that it's not a big deal, but I agree with Peter that it's
worthwhile to clarify, especially since this will be seen in psql's \h
w/o the rest of the context of the CREATE POLICY documentation.

I've gone ahead and made this minor change.

Thanks!

Stephen

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> [There's also a typo further down -- "filter out the records which are
> visible", should be "not visible"]

I agree, that's not really worded quite right. I've reworded this along
the lines of what you suggested (though not exactly- if you get a chance
to review it, that'd be great) and pushed it, so we don't forget to do
so later.

> What do you think of the attached rewording?

Regarding the larger/earlier paragraph, Would be great if you could
comment on my latest suggestion.

Thanks!

Stephen

On 29 January 2015 at 04:00, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> On Wed, Jan 7, 2015 at 3:06 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> > If I'm following correctly, Peter's specifically talking about:
>> >
>> > [ USING ( <replaceable class="parameter">expression</replaceable> ) ]
>> > [ WITH CHECK ( <replaceable class="parameter">check_expression</replaceable> ) ]
>> >
>> > Where the USING parameter is 'expression' but the WITH CHECK parameter
>> > is 'check_expression'. He makes a good point, I believe, as
>> > "expression" is overly generic. I don't like the idea of using
>> > "barrier_expression" though as that ends up introducing a new term- how
>> > about "using_expression"?
>>
> I've gone ahead and made this minor change.
>

Don't forget the ALTER POLICY page. This and some of the other things
being discussed on this thread ought to be copied there too.

Regards,
Dean

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 29 January 2015 at 04:00, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> >> On Wed, Jan 7, 2015 at 3:06 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >> > If I'm following correctly, Peter's specifically talking about:
> >> >
> >> > [ USING ( <replaceable class="parameter">expression</replaceable> ) ]
> >> > [ WITH CHECK ( <replaceable class="parameter">check_expression</replaceable> ) ]
> >> >
> >> > Where the USING parameter is 'expression' but the WITH CHECK parameter
> >> > is 'check_expression'. He makes a good point, I believe, as
> >> > "expression" is overly generic. I don't like the idea of using
> >> > "barrier_expression" though as that ends up introducing a new term- how
> >> > about "using_expression"?
> >>
> > I've gone ahead and made this minor change.
>
> Don't forget the ALTER POLICY page. This and some of the other things
> being discussed on this thread ought to be copied there too.

Ah, yeah, good point, will address soon.

Thanks!

Stephen

On Wed, Jan 28, 2015 at 10:45 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> I agree, especially after going back and re-reading this while fixing
> the issue mentioned earlier by Peter (which was an orthogonal complaint
> about the shadowing of WITH CHECK by USING, if WITH CHECK isn't
> specified). We really need a paragraph on "USING" policies and another
> on "WITH CHECK" policies. How about a reword along these lines:
>
> When row level security is enabled on a table, all access to that
> table by users other than the owner or a superuser must be through a
> policy. This requirement applies to both selecting out existing rows
> from the table and to adding rows to the table (through either INSERT
> or UPDATE).
>
> Granting access to existing rows in a table is done by specifying a
> USING expression which will be added to queries which reference the
> table. Every row in the table which a USING expression returns true
> will be visible.
>
> Granting access to add rows to a table is done by specifying a WITH
> CHECK expressison. A WITH CHECK expression must return true for
> every row being added to the table or an error will be returned and
> the command will be aborted.

I hate to be a critic, but this existing sentence in the documentation
seems to explain nearly all of the above: "A policy limits the ability
to SELECT, INSERT, UPDATE, or DELETE rows in a table to those rows
which match the relevant policy expression. Existing table rows are
checked against the expression specified via USING, while new rows
that would be created via INSERT or UPDATE are checked against the
expression specified via WITH CHECK." The only thing I can see we
might need to add is a sentence that says something like "If the USING
clause returns false for a particular row, that row will not be
visible to the user; if a WITH CHECK expression does not return true,
an error occurs."

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Wed, Jan 28, 2015 at 10:45 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I agree, especially after going back and re-reading this while fixing
> > the issue mentioned earlier by Peter (which was an orthogonal complaint
> > about the shadowing of WITH CHECK by USING, if WITH CHECK isn't
> > specified). We really need a paragraph on "USING" policies and another
> > on "WITH CHECK" policies. How about a reword along these lines:
> >
> > When row level security is enabled on a table, all access to that
> > table by users other than the owner or a superuser must be through a
> > policy. This requirement applies to both selecting out existing rows
> > from the table and to adding rows to the table (through either INSERT
> > or UPDATE).
> >
> > Granting access to existing rows in a table is done by specifying a
> > USING expression which will be added to queries which reference the
> > table. Every row in the table which a USING expression returns true
> > will be visible.
> >
> > Granting access to add rows to a table is done by specifying a WITH
> > CHECK expressison. A WITH CHECK expression must return true for
> > every row being added to the table or an error will be returned and
> > the command will be aborted.
>
> I hate to be a critic, but this existing sentence in the documentation
> seems to explain nearly all of the above: "A policy limits the ability
> to SELECT, INSERT, UPDATE, or DELETE rows in a table to those rows
> which match the relevant policy expression. Existing table rows are
> checked against the expression specified via USING, while new rows
> that would be created via INSERT or UPDATE are checked against the
> expression specified via WITH CHECK." The only thing I can see we
> might need to add is a sentence that says something like "If the USING
> clause returns false for a particular row, that row will not be
> visible to the user; if a WITH CHECK expression does not return true,
> an error occurs."

Well, I agree (I suppose perhaps that I have to, since I'm pretty sure I
wrote that.. :D), but what Dean was suggesting was a reword that
approached it from the other direction- that is, talk about policies as
granting access, instead of limiting it. I thought that was an
excellent approach which will hopefully reduce confusion. To that end,
how about this reword:

-----------------
A policy grants the ability to SELECT, INSERT, UPDATE, or DELETE rows
which match the relevant policy expression. Existing table rows are
checked against the expression specified via USING, while new rows
that would be created via INSERT or UPDATE are checked against the
expression specified via WITH CHECK. When a USING expression returns
false for a given row, that row is not visible to the user. When a WITH
CHECK expression returns false for a row which is to be added, an error
occurs.
-----------------

This would need to be after we talk about row level security being
enabled for a table and the default-deny policy or it might not make
sense, but otherwise I'm thinking it works.

Thoughts?

Thanks!

Stephen

On Thu, Jan 29, 2015 at 9:04 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> A policy grants the ability to SELECT, INSERT, UPDATE, or DELETE rows
> which match the relevant policy expression. Existing table rows are
> checked against the expression specified via USING, while new rows
> that would be created via INSERT or UPDATE are checked against the
> expression specified via WITH CHECK. When a USING expression returns
> false for a given row, that row is not visible to the user. When a WITH
> CHECK expression returns false for a row which is to be added, an error
> occurs.

Yeah, that's not bad. I think it's an improvement, in fact.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Thu, Jan 29, 2015 at 9:04 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > A policy grants the ability to SELECT, INSERT, UPDATE, or DELETE rows
> > which match the relevant policy expression. Existing table rows are
> > checked against the expression specified via USING, while new rows
> > that would be created via INSERT or UPDATE are checked against the
> > expression specified via WITH CHECK. When a USING expression returns
> > false for a given row, that row is not visible to the user. When a WITH
> > CHECK expression returns false for a row which is to be added, an error
> > occurs.
>
> Yeah, that's not bad. I think it's an improvement, in fact.

Excellent, thanks for the feedback. I'll see about making those changes
tomorrow.

Thanks again!

Stephen

On 30 January 2015 at 03:40, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> On Thu, Jan 29, 2015 at 9:04 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> > A policy grants the ability to SELECT, INSERT, UPDATE, or DELETE rows
>> > which match the relevant policy expression. Existing table rows are
>> > checked against the expression specified via USING, while new rows
>> > that would be created via INSERT or UPDATE are checked against the
>> > expression specified via WITH CHECK. When a USING expression returns
>> > false for a given row, that row is not visible to the user. When a WITH
>> > CHECK expression returns false for a row which is to be added, an error
>> > occurs.
>>
>> Yeah, that's not bad. I think it's an improvement, in fact.
>

Yes I like that too. My main concern was that we should be describing
policies in terms of permitting access to the table, not limiting
access, because of the default-deny policy, and this new text clears
that up.

One additional quibble -- it's misleading to say "expression returns
false" here (and later in the check_expression parameter description)
because if the expression returns null, that's also a failure. So it
ought to be "false or null", but perhaps it could just be described in
terms of rows matching the expression, with a separate note to say
that a row only matches a policy expression if that expression returns
true, not false or null.

Regards,
Dean

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 30 January 2015 at 03:40, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> >> On Thu, Jan 29, 2015 at 9:04 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >> > A policy grants the ability to SELECT, INSERT, UPDATE, or DELETE rows
> >> > which match the relevant policy expression. Existing table rows are
> >> > checked against the expression specified via USING, while new rows
> >> > that would be created via INSERT or UPDATE are checked against the
> >> > expression specified via WITH CHECK. When a USING expression returns
> >> > false for a given row, that row is not visible to the user. When a WITH
> >> > CHECK expression returns false for a row which is to be added, an error
> >> > occurs.
> >>
> >> Yeah, that's not bad. I think it's an improvement, in fact.
>
> Yes I like that too. My main concern was that we should be describing
> policies in terms of permitting access to the table, not limiting
> access, because of the default-deny policy, and this new text clears
> that up.

Great, thanks, pushed.

> One additional quibble -- it's misleading to say "expression returns
> false" here (and later in the check_expression parameter description)
> because if the expression returns null, that's also a failure. So it
> ought to be "false or null", but perhaps it could just be described in
> terms of rows matching the expression, with a separate note to say
> that a row only matches a policy expression if that expression returns
> true, not false or null.

Good point, I've made a few minor changes to address that also, please
let me know if you see any issus.

Thanks!

Stephen

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> Don't forget the ALTER POLICY page. This and some of the other things
> being discussed on this thread ought to be copied there too.

Thanks, I've fixed this also.

Stephen