Thread: [HACKERS] Row Level Security UPDATE Confusion
I'm a little confused on why a SELECT policy fires against the NEW record for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem to have that restriction.
--
DROP TABLE IF EXISTS t;
CREATE USER simple;
CREATE USER split;
CREATE TABLE t(value int);
grant select, update on table t to simple, split;
INSERT INTO t values (1), (2);
ALTER TABLE t ENABLE ROW LEVEL SECURITY;
CREATE POLICY simple_all ON t TO simple USING (value > 0) WITH CHECK (true);
CREATE POLICY split_select ON t FOR SELECT TO split USING (value > 0);
CREATE POLICY split_update ON t FOR UPDATE TO split USING (true) WITH CHECK (true);
SET session authorization simple;
SELECT * FROM t;
UPDATE t SET value = value * -1 WHERE value = 1;
SET session authorization split;
SELECT * FROM t;
UPDATE t SET value = value * -1 WHERE value = 2;
CREATE USER simple;
CREATE USER split;
CREATE TABLE t(value int);
grant select, update on table t to simple, split;
INSERT INTO t values (1), (2);
ALTER TABLE t ENABLE ROW LEVEL SECURITY;
CREATE POLICY simple_all ON t TO simple USING (value > 0) WITH CHECK (true);
CREATE POLICY split_select ON t FOR SELECT TO split USING (value > 0);
CREATE POLICY split_update ON t FOR UPDATE TO split USING (true) WITH CHECK (true);
SET session authorization simple;
SELECT * FROM t;
UPDATE t SET value = value * -1 WHERE value = 1;
SET session authorization split;
SELECT * FROM t;
UPDATE t SET value = value * -1 WHERE value = 2;
/* UPDATE fails with below error:
psql:/tmp/t.sql:24: ERROR: 42501: new row violates row-level security policy for table "t"
LOCATION: ExecWithCheckOptions, execMain.c:2045
*/
SET session authorization default;
SELECT * FROM t;
LOCATION: ExecWithCheckOptions, execMain.c:2045
*/
SET session authorization default;
SELECT * FROM t;
This seems consistent in both Pg 9.5 and upcoming Pg 10.
--
Rod Taylor
On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor <rod.taylor@gmail.com> wrote: > I'm a little confused on why a SELECT policy fires against the NEW record > for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem > to have that restriction. My guess is that you have found a bug. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote: > On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor <rod.taylor@gmail.com> wrote: > > I'm a little confused on why a SELECT policy fires against the NEW record > > for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem > > to have that restriction. > > My guess is that you have found a bug. Indeed. Joe's been looking into it and I'm hoping to find some time to dig into it shortly. Thanks! Stephen
On 04/13/2017 01:31 PM, Stephen Frost wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: >> On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor <rod.taylor@gmail.com> wrote: >> > I'm a little confused on why a SELECT policy fires against the NEW record >> > for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem >> > to have that restriction. >> >> My guess is that you have found a bug. > > Indeed. Joe's been looking into it and I'm hoping to find some time to > dig into it shortly. >> CREATE POLICY split_select ON t FOR SELECT TO split >> USING (value > 0); >> CREATE POLICY split_update ON t FOR UPDATE TO split >> USING (true) WITH CHECK (true); Yes -- from what I can see in gdb: 1) add_with_check_options() adds both (value > 0) and (true) to withCheckOptions -- this seems correct as the USING expression is used for WITH CHECK when the latter is not specified 2) ExecWithCheckOptions() checks (value > 0) which fails, and it immediately throws an ERROR, i.e. it never checks the (true) expression and therefore never ORs the results -- this seems incorrect, it uses restrictive not permissive Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Rod, all, * Joe Conway (mail@joeconway.com) wrote: > On 04/13/2017 01:31 PM, Stephen Frost wrote: > > * Robert Haas (robertmhaas@gmail.com) wrote: > >> On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor <rod.taylor@gmail.com> wrote: > >> > I'm a little confused on why a SELECT policy fires against the NEW record > >> > for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem > >> > to have that restriction. > >> > >> My guess is that you have found a bug. > > > > Indeed. Joe's been looking into it and I'm hoping to find some time to > > dig into it shortly. > > >> CREATE POLICY split_select ON t FOR SELECT TO split > >> USING (value > 0); > >> CREATE POLICY split_update ON t FOR UPDATE TO split > >> USING (true) WITH CHECK (true); > > Yes -- from what I can see in gdb: Actually, looking at this again, the complaint appears to be that you can't "give away" records. That was a topic of much discussion and I'm reasonably sure that was what we ended up deciding made the most sense. You have to be able to see records to be able to update them (you can't update records you can't see), and you have to be able to see the result of your update. I don't doubt that we could improve the documentation around this (and apparently the code comments, according to Joe..). Thanks! Stephen
On Thu, Apr 13, 2017 at 5:31 PM, Stephen Frost <sfrost@snowman.net> wrote:
-- No error and value is -1 at the end.Rod, all,
* Joe Conway (mail@joeconway.com) wrote:
> On 04/13/2017 01:31 PM, Stephen Frost wrote:
> > * Robert Haas (robertmhaas@gmail.com) wrote:
> >> On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor <rod.taylor@gmail.com> wrote:
> >> > I'm a little confused on why a SELECT policy fires against the NEW record
> >> > for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem
> >> > to have that restriction.
> >>
> >> My guess is that you have found a bug.
> >
> > Indeed. Joe's been looking into it and I'm hoping to find some time to
> > dig into it shortly.
>
> >> CREATE POLICY split_select ON t FOR SELECT TO split
> >> USING (value > 0);
> >> CREATE POLICY split_update ON t FOR UPDATE TO split
> >> USING (true) WITH CHECK (true);
>
> Yes -- from what I can see in gdb:
Actually, looking at this again, the complaint appears to be that you
can't "give away" records. That was a topic of much discussion and I'm
reasonably sure that was what we ended up deciding made the most sense.
You have to be able to see records to be able to update them (you can't
update records you can't see), and you have to be able to see the result
of your update. I don't doubt that we could improve the documentation
around this (and apparently the code comments, according to Joe..).
Then there is a bug in the simpler statement which happily lets you give away records.
CREATE POLICY simple_all ON t TO simple USING (value > 0) WITH CHECK (true);
SET session authorization simple;
SELECT * FROM t;
UPDATE t SET value = value * -1 WHERE value = 1;
CREATE POLICY simple_all ON t TO simple USING (value > 0) WITH CHECK (true);
SET session authorization simple;
SELECT * FROM t;
UPDATE t SET value = value * -1 WHERE value = 1;
--
Rod Taylor
Rod, * Rod Taylor (rod.taylor@gmail.com) wrote: > Then there is a bug in the simpler statement which happily lets you give > away records. > > CREATE POLICY simple_all ON t TO simple USING (value > 0) WITH CHECK (true); > > SET session authorization simple; > SELECT * FROM t; > UPDATE t SET value = value * -1 WHERE value = 1; > -- No error and value is -1 at the end. Hm, that does seem like it's not matching up with the intent, likely because it's an 'ALL' policy instead of individual policies. Out of curiosity, is there a particular use-case here that you're working towards, or just testing it out to see how it works? Thanks! Stephen
On Fri, Apr 14, 2017 at 7:41 AM, Rod Taylor <rod.taylor@gmail.com> wrote:
On Thu, Apr 13, 2017 at 5:31 PM, Stephen Frost <sfrost@snowman.net> wrote:-- No error and value is -1 at the end.Rod, all,
* Joe Conway (mail@joeconway.com) wrote:
> On 04/13/2017 01:31 PM, Stephen Frost wrote:
> > * Robert Haas (robertmhaas@gmail.com) wrote:
> >> On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor <rod.taylor@gmail.com> wrote:
> >> > I'm a little confused on why a SELECT policy fires against the NEW record
> >> > for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem
> >> > to have that restriction.
> >>
> >> My guess is that you have found a bug.
> >
> > Indeed. Joe's been looking into it and I'm hoping to find some time to
> > dig into it shortly.
>
> >> CREATE POLICY split_select ON t FOR SELECT TO split
> >> USING (value > 0);
> >> CREATE POLICY split_update ON t FOR UPDATE TO split
> >> USING (true) WITH CHECK (true);
>
> Yes -- from what I can see in gdb:
Actually, looking at this again, the complaint appears to be that you
can't "give away" records. That was a topic of much discussion and I'm
reasonably sure that was what we ended up deciding made the most sense.
You have to be able to see records to be able to update them (you can't
update records you can't see), and you have to be able to see the result
of your update. I don't doubt that we could improve the documentation
around this (and apparently the code comments, according to Joe..).Then there is a bug in the simpler statement which happily lets you give away records.
CREATE POLICY simple_all ON t TO simple USING (value > 0) WITH CHECK (true);
SET session authorization simple;
SELECT * FROM t;
UPDATE t SET value = value * -1 WHERE value = 1;
My actual use-case involves a range. Most users can see and manipulate the record when CURRENT_TIMESTAMP is within active_period. Some users (staff/account admins) can see recently dead records too. And a 3rd group (senior staff) have no time restriction, though there are a few customers they cannot see due to their information being a touch more sensitive. I've simplified the below rules to just deal with active_period and the majority of user view (@> CURRENT_TIMESTAMP).
NOTE: the active_period range is '[)' by default, so records with upper() = CURRENT_TIMESTAMP are not visible with @> CURRENT_TIMESTAMP restriction.
The below policy is allowed but requires that 1ms slop to accommodate the wi
Updated record invisible to USING but requires a trigger to enforce specific upper
and starting values. I have a trigger enforcing specific upper/lower values for the range
for specific ROLEs. So I had the thought that I might move ROLE specific trigger logic into
the RLS mechanism.
I tried to tighten the above rules, so INSERT must have upper of NULL and UPDATE must
set upper to exactly CURRENT_TIMESTAMP. Clearly I can achieve this using triggers for
enforcement but I tried to abuse RLS instead because it is a role specific restriction.
I was surprised when hide_old_select->USING was preventing the UPDATE when the simple single policy version let it through.
I'm happy to help with testing and documentation but first I need to understand what the intended functionality was. Right now it seems inconsistent between the simple single policy version and the multi policy version; the docs imply the single policy version is correct (it only seems to mention SELECT checks on RETURNING clauses).
NOTE: the active_period range is '[)' by default, so records with upper() = CURRENT_TIMESTAMP are not visible with @> CURRENT_TIMESTAMP restriction.
CREATE A TABLE t (id integer, active_period tstzrange NOT NULL DEFAULT tstzrange(current_timestamp, NULL));
The below policy is allowed but requires that 1ms slop to accommodate the wi
Updated record invisible to USING but requires a trigger to enforce specific upper
and starting values. I have a trigger enforcing specific upper/lower values for the range
for specific ROLEs. So I had the thought that I might move ROLE specific trigger logic into
the RLS mechanism.
CREATE POLICY hide_old ON t TO s;
USING ( active_period @> CURRENT_TIMESTAMP)
WITH CHECK ( active_period && tstzrange(current_timestamp - interval '0.001 seconds', current_timestamp, '[]'));
-- This is effectively a delete for the above policy. It becomes immediately invisible due to USING restriction.
UPDATE t SET active_period = tstzrange(lower(active_period), CURRENT_TIMESTAMP);
SELECT count(*) FROM t; -- 0 records
USING ( active_period @> CURRENT_TIMESTAMP)
WITH CHECK ( active_period && tstzrange(current_timestamp - interval '0.001 seconds', current_timestamp, '[]'));
-- This is effectively a delete for the above policy. It becomes immediately invisible due to USING restriction.
UPDATE t SET active_period = tstzrange(lower(active_period), CURRENT_TIMESTAMP);
SELECT count(*) FROM t; -- 0 records
I tried to tighten the above rules, so INSERT must have upper of NULL and UPDATE must
set upper to exactly CURRENT_TIMESTAMP. Clearly I can achieve this using triggers for
enforcement but I tried to abuse RLS instead because it is a role specific restriction.
I was surprised when hide_old_select->USING was preventing the UPDATE when the simple single policy version let it through.
CREATE POLICY hide_old_select ON t FOR SELECT TO s
USING ( active_period @> CURRENT_TIMESTAMP);
CREATE POLICY hide_old_insert ON t FOR INSERT to s
WITH CHECK ( lower(active_period) = CURRENT_TIMESTAMP AND upper(active_period) IS NULL);
CREATE POLICY hide_old_update ON t FOR UPDATE TO s
USING ( active_period @> CURRENT_TIMESTAMP)
WITH CHECK ( upper(active_period) = CURRENT_TIMESTAMP);
USING ( active_period @> CURRENT_TIMESTAMP);
CREATE POLICY hide_old_insert ON t FOR INSERT to s
WITH CHECK ( lower(active_period) = CURRENT_TIMESTAMP AND upper(active_period) IS NULL);
CREATE POLICY hide_old_update ON t FOR UPDATE TO s
USING ( active_period @> CURRENT_TIMESTAMP)
WITH CHECK ( upper(active_period) = CURRENT_TIMESTAMP);
-- Disallowed due to hide_old_select policy.
UPDATE t SET active_period = tstzrange(lower(active_period), CURRENT_TIMESTAMP);
I'm happy to help with testing and documentation but first I need to understand what the intended functionality was. Right now it seems inconsistent between the simple single policy version and the multi policy version; the docs imply the single policy version is correct (it only seems to mention SELECT checks on RETURNING clauses).
--
Rod Taylor
Rod, * Rod Taylor (rod.taylor@gmail.com) wrote: > My actual use-case involves a range. Most users can see and manipulate the > record when CURRENT_TIMESTAMP is within active_period. Some users > (staff/account admins) can see recently dead records too. And a 3rd group > (senior staff) have no time restriction, though there are a few customers > they cannot see due to their information being a touch more sensitive. > I've simplified the below rules to just deal with active_period and the > majority of user view (@> CURRENT_TIMESTAMP). Interesting. > NOTE: the active_period range is '[)' by default, so records with upper() = > CURRENT_TIMESTAMP are not visible with @> CURRENT_TIMESTAMP restriction. Is that really what you intend/want though? For records with upper() = CURRENT_TIMESTAMP to not be visible? You are able to change the range returned from tstzrange by specifying what you want, eg: select tstzrange(current_timestamp, current_timestamp, '[]'); > CREATE A TABLE t (id integer, active_period tstzrange NOT NULL DEFAULT > tstzrange(current_timestamp, NULL)); Why NULL instead of 'infinity'...? > -- Disallowed due to hide_old_select policy. > UPDATE t SET active_period = tstzrange(lower(active_period), > CURRENT_TIMESTAMP); Guess I'm still trying to figure out if you really intend for this to make the records invisible to the 'most users' case. > I'm happy to help with testing and documentation but first I need to > understand what the intended functionality was. Right now it seems > inconsistent between the simple single policy version and the multi policy > version; the docs imply the single policy version is correct (it only seems > to mention SELECT checks on RETURNING clauses). I agree that the documentation could be improved here. Thanks! Stephen
On Fri, Apr 14, 2017 at 9:09 AM, Stephen Frost <sfrost@snowman.net> wrote: > I agree that the documentation could be improved here. This does not seem like it's only a documentation problem. There seems to be no principled reason why a grant for ALL shouldn't have exactly the same effect as one grant per relevant operation type. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Fri, Apr 14, 2017 at 9:09 AM, Stephen Frost <sfrost@snowman.net> wrote: > > I agree that the documentation could be improved here. > > This does not seem like it's only a documentation problem. There > seems to be no principled reason why a grant for ALL shouldn't have > exactly the same effect as one grant per relevant operation type. I agreed already up-thread that there's an issue there and will be looking to fix it. That comment was simply replying to Rod's point that the documentation could also be improved. Thanks! Stephen
On Fri, Apr 14, 2017 at 9:09 AM, Stephen Frost <sfrost@snowman.net> wrote:
Rod,
* Rod Taylor (rod.taylor@gmail.com) wrote:
> My actual use-case involves a range. Most users can see and manipulate the
> record when CURRENT_TIMESTAMP is within active_period. Some users
> (staff/account admins) can see recently dead records too. And a 3rd group
> (senior staff) have no time restriction, though there are a few customers
> they cannot see due to their information being a touch more sensitive.
> I've simplified the below rules to just deal with active_period and the
> majority of user view (@> CURRENT_TIMESTAMP).
Interesting.
> NOTE: the active_period range is '[)' by default, so records with upper() =
> CURRENT_TIMESTAMP are not visible with @> CURRENT_TIMESTAMP restriction.
Is that really what you intend/want though? For records with
upper() = CURRENT_TIMESTAMP to not be visible? You are able to change
the range returned from tstzrange by specifying what you want, eg:
Yeah, think of it like a delete. Once a record is deleted you want it to disappear. From the users point of view, who doesn't have time-travel privileges, an UPDATE to upper() = CURRENT_TIMESTAMP should disappear from any actions that take place later in the transaction.
A more common way of implementing this is an archive table. Have a DELETE trigger and shuffle the record to another storage area but with many dependent tuples via foreign key this can be very time consuming. Flipping a time period is consistently fast with the caveat that SELECT pays a price.
If you decide Pg shouldn't allow a user to make a tuple disappear, I would probably make a DO INSTEAD SECURITY DEFINER function that triggers on DELETE for that role only and changes the time range. Reality is after about 1 week for customers to contact their account administrator and say "I accidentally deleted X" it would likely be moved to an archive structure.
select tstzrange(current_timestamp, current_timestamp, '[]');
> CREATE A TABLE t (id integer, active_period tstzrange NOT NULL DEFAULT
> tstzrange(current_timestamp, NULL));
Why NULL instead of 'infinity'...?
Diskspace. NULL works (almost) the same as infinity but the storage is quite a bit smaller.
> -- Disallowed due to hide_old_select policy.
> UPDATE t SET active_period = tstzrange(lower(active_period),
> CURRENT_TIMESTAMP);
Guess I'm still trying to figure out if you really intend for this to
make the records invisible to the 'most users' case.
Yep. It's equivalent to a DELETE or DEACTIVATE. RLS may not be the right facility but it was very close to working exactly the way I wanted in FOR ALL mode.
--
Rod Taylor
Rod, * Rod Taylor (rod.taylor@gmail.com) wrote: > Yep. It's equivalent to a DELETE or DEACTIVATE. RLS may not be the right > facility but it was very close to working exactly the way I wanted in FOR > ALL mode. Turning an UPDATE into, effectively, a DELETE, does seem like it's beyond the mandate of RLS. Using an on-delete trigger strikes me as a good approach. The base premise of not allowing rows to be 'given away', similar to how we don't allow full objects to be given away, should be enforced for the 'ALL' policy case, just as it is for the individual-command case. I'll get that addressed before the next set of minor releases and will also see about improving the documentation and code comments to make it more clear. Thanks! Stephen
On Fri, Apr 14, 2017 at 9:16 AM, Stephen Frost <sfrost@snowman.net> wrote: > I agreed already up-thread that there's an issue there and will be > looking to fix it. That comment was simply replying to Rod's point that > the documentation could also be improved. OK, thanks. The wrap for the next set of minor releases is, according to my understanding, scheduled for Monday, so you'd better jump on this soon if you're hoping to get a fix out this time around. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Fri, Apr 14, 2017 at 9:16 AM, Stephen Frost <sfrost@snowman.net> wrote: > > I agreed already up-thread that there's an issue there and will be > > looking to fix it. That comment was simply replying to Rod's point that > > the documentation could also be improved. > > OK, thanks. The wrap for the next set of minor releases is, according > to my understanding, scheduled for Monday, so you'd better jump on > this soon if you're hoping to get a fix out this time around. Thanks for the reminder. I do know the release is looming and I am hoping to have a patch tomorrow or Thursday. Thanks again! Stephen
Robert, all, * Robert Haas (robertmhaas@gmail.com) wrote: > On Fri, Apr 14, 2017 at 9:16 AM, Stephen Frost <sfrost@snowman.net> wrote: > > I agreed already up-thread that there's an issue there and will be > > looking to fix it. That comment was simply replying to Rod's point that > > the documentation could also be improved. > > OK, thanks. The wrap for the next set of minor releases is, according > to my understanding, scheduled for Monday, so you'd better jump on > this soon if you're hoping to get a fix out this time around. I've worked out what's happening here and it's because the ALL policy has both USING and WITH CHECK that it's not acting the same as the SELECT policy (which can only have USING). add_with_check_quals() is what determines if the WITH CHECK policy or the USING policy should be used (through a bit of a grotty #define, if you ask me..). I've been considering how best to fix it. The two main options are to use a different WCOKind and then track that through, which might be nice as we might be able to provide a more useful error message in that case, or to just add an additional flag to add_with_check_quals() to say "always add the USING clause when this flag is true." Either way, I expect to wrap this up either later tonight or tomorrow. Thanks! Stephen
Rod, Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Fri, Apr 14, 2017 at 9:16 AM, Stephen Frost <sfrost@snowman.net> wrote: > > I agreed already up-thread that there's an issue there and will be > > looking to fix it. That comment was simply replying to Rod's point that > > the documentation could also be improved. > > OK, thanks. The wrap for the next set of minor releases is, according > to my understanding, scheduled for Monday, so you'd better jump on > this soon if you're hoping to get a fix out this time around. The attached patch against master fixes this issue. Rod, if you get a chance, would be great for you to check that you no longer see a difference between the single ALL policy and the split SELECT/UPDATE policies. Thanks! Stephen
Attachment
Hmm.
CREATE POLICY split_select ON t FOR SELECT TO split USING (value > 0);
CREATE POLICY split_update ON t FOR UPDATE TO split USING (value < 10) WITH CHECK (value > 2);
SET session authorization split;
update t set value = 100 where value = 4; -- 1 record changed
CREATE POLICY split_update ON t FOR UPDATE TO split USING (value < 10) WITH CHECK (value > 2);
SET session authorization split;
update t set value = 100 where value = 4; -- 1 record changed
update t set value = 5 where value = 100; -- 0 records changed
However, despite INSERT also functioning the same for both styles of commands it's definitely not obeying the `cannot give away records` rule:
CREATE USER simple;
CREATE USER split;
CREATE TABLE t(value int);
grant select, update, insert, delete on table t to simple, split;
INSERT INTO t values (1), (2);
ALTER TABLE t ENABLE ROW LEVEL SECURITY;
CREATE POLICY simple_all ON t TO simple USING (value > 0) WITH CHECK (true);
CREATE POLICY split_select ON t FOR SELECT TO split USING (value > 0);
CREATE POLICY split_insert ON t FOR INSERT TO split WITH CHECK (true);
SET session authorization simple;
INSERT INTO t VALUES (3), (-3);
SELECT * FROM t;
value
-------
1
2
3
(3 rows)
SET session authorization split;
INSERT INTO t VALUES (4), (-4);
SELECT * FROM t;
value
-------
1
2
3
4
(4 rows)
SET session authorization default;
SELECT * FROM t;
value
-------
1
2
3
-3
4
-4
(6 rows)
regards,CREATE USER split;
CREATE TABLE t(value int);
grant select, update, insert, delete on table t to simple, split;
INSERT INTO t values (1), (2);
ALTER TABLE t ENABLE ROW LEVEL SECURITY;
CREATE POLICY simple_all ON t TO simple USING (value > 0) WITH CHECK (true);
CREATE POLICY split_select ON t FOR SELECT TO split USING (value > 0);
CREATE POLICY split_insert ON t FOR INSERT TO split WITH CHECK (true);
SET session authorization simple;
INSERT INTO t VALUES (3), (-3);
SELECT * FROM t;
value
-------
1
2
3
(3 rows)
SET session authorization split;
INSERT INTO t VALUES (4), (-4);
SELECT * FROM t;
value
-------
1
2
3
4
(4 rows)
SET session authorization default;
SELECT * FROM t;
value
-------
1
2
3
-3
4
-4
(6 rows)
Rod
On Fri, May 5, 2017 at 12:10 PM, Stephen Frost <sfrost@snowman.net> wrote:
Rod, Robert,
* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Apr 14, 2017 at 9:16 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > I agreed already up-thread that there's an issue there and will be
> > looking to fix it. That comment was simply replying to Rod's point that
> > the documentation could also be improved.
>
> OK, thanks. The wrap for the next set of minor releases is, according
> to my understanding, scheduled for Monday, so you'd better jump on
> this soon if you're hoping to get a fix out this time around.
The attached patch against master fixes this issue. Rod, if you get a
chance, would be great for you to check that you no longer see a
difference between the single ALL policy and the split SELECT/UPDATE
policies.
Thanks!
Stephen
--
Rod Taylor
Rod, * Rod Taylor (rod.taylor@gmail.com) wrote: > UPDATE seems to work as described (unable to create records you cannot > select); both the single rule version and multi-rule version seem to work > the same. I'm glad they work the same now, as they were always intended to. Regarding the "rules", they're actually based on what the ACL rules are. In particular, an UPDATE with a WHERE clause requires SELECT rights- and so the code mandates that the UPDATE can see both the row-to-be-updated and the row-post-updating, but you're able to INSERT records which you can't then see, as an INSERT doesn't generally require SELECT privileges. > This combination works too though it seems funny that UPDATE can create an > entry it cannot reverse. I can set the value to 100 (going to -1 fails) but > the UPDATE cannot see the record to set it back. I can see use cases for > it, for example you might be able to promote someone to manager but not > demote a manager to front-desk. We also allow INSERT on tables you cannot > delete from, so it's not inconsistent. > > CREATE POLICY split_select ON t FOR SELECT TO split USING (value > 0); > CREATE POLICY split_update ON t FOR UPDATE TO split USING (value < 10) WITH > CHECK (value > 2); > SET session authorization split; > update t set value = 100 where value = 4; -- 1 record changed > update t set value = 5 where value = 100; -- 0 records changed Being able to UPDATE records to change them in a way that you're not able to subsequently UPDATE them seems reasonable to me, at least. > However, despite INSERT also functioning the same for both styles of > commands it's definitely not obeying the `cannot give away records` rule: Right, this is because INSERT doesn't generally require SELECT privileges, and therefore doesn't require the USING clause to be checked also. We could certainly debate if the approach of applying policies based on the privileges required for the command is the "correct" approach, but it's definitely what was intended and generally works well, based on what I've seen thus far in terms of actual usage. Admittedly, it's a bit of an edge case which doesn't come up very often either, so perhaps we should consider changing this down the road, but for now we should go ahead and fix the obvious unintentional bug in the code around ALL policies and back-patch that as a bug fix, we can then consider if changes should be made here in future releases independently. Assuming there aren't objections to that, I'll plan to push this fix later tonight or tomorrow. Thanks! Stephen
Rod, * Rod Taylor (rod.taylor@gmail.com) wrote: > UPDATE seems to work as described (unable to create records you cannot > select); both the single rule version and multi-rule version seem to work > the same. Thanks for reporting this and helping to test the fix. I've now pushed the fix and it'll be included in the next round of patch releases. I'd definitely like to chat further at some point regarding what really makes sense here and if we should be considering a change, and, further, how we can improve the documentation to be more clear. Thanks! Stephen