Clarifying docs on nuance of select and update policies

From: Chandler Gonzales <jcgsville(at)gmail(dot)com>
To: pgsql-docs(at)lists(dot)postgresql(dot)org
Subject: Clarifying docs on nuance of select and update policies
Date: 2022-09-16 18:57:25
Message-ID: CAHBb8c5OHwnuJ_dhRi_qVuG-0cWd8Z_Rb4fmnGwAD3hrfQsZZw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-docs pgsql-hackers

Hi y'all, I've got a proposed clarification to the documentation on the
nuances of RLS behavior for update policies, and maybe a (humble) request
for a change in behavior to make it more intuitive. I am starting with
pgsql-docs since I think the documentation change is a good starting point.

We use RLS policies to hide "soft deleted" objects from certain DB roles.
We recently tried to add the ability to let a user "soft delete" a row.
Ideally, we can write an RLS policy that allows a user to "soft delete" a
row, but then hides the row from that same user once it is soft deleted.

Here's the setup on a fresh Postgres db for my example. I'm executing these
queries as the database owner:
```
create role some_user_type;
create table foo(id int primary key, soft_deleted_at timestamptz,
some_other_field text);
alter table foo enable row level security;
grant select on table foo to some_user_type;
grant update(soft_deleted_at) on table foo to some_user_type;
insert into foo(id) values (1);
insert into foo(id, soft_deleted_at) values (2, now());
```

The behavior I'm trying to encode in RLS is that users with the role
some_user_type can see all rows where soft_deleted_at is null, can update
rows where BOTH the original soft_deleted_at is null AND the updated row
has a non-null soft_deleted_at. Basically, the only thing this user can do
to this row is to soft delete it.

We'll use a restrictive policy to get better error messages when we do an
update later on:
```
create policy pol_1 on foo for select to some_user_type using (true);
create policy pol_1_res on foo as restrictive for select to some_user_type
using (soft_deleted_at is null);
```

And just to verify it's working:
```
chandler(at)localhost:chandler> begin; set local role some_user_type; select *
from foo; rollback;
BEGIN
SET
╒════╤═════════════════╤══════════════════╕
│ id │ soft_deleted_at │ some_other_field │
╞════╪═════════════════╪══════════════════╡
│ 1 │ ¤ │ ¤ │
╘════╧═════════════════╧══════════════════╛
SELECT 1
ROLLBACK
```

Now the important bit, the update policy:
```
create policy pol_2 on foo for update to some_user_type using
(soft_deleted_at is null) with check (soft_deleted_at is not null);
```

If we update a row without a where clause that touches the row, the update
succeeds:
```
chandler(at)localhost:chandler> begin; set local role some_user_type; update
foo set soft_deleted_at = now() where true; rollback;
BEGIN
SET
UPDATE 1
ROLLBACK
```

But if we update a row with a where clause that uses the existing row, the
update fails:
```
chandler(at)localhost:chandler> begin; set local role some_user_type; update
foo set soft_deleted_at = now() where id = 1; rollback;
BEGIN
SET
new row violates row-level security policy "pol_1_res" for table "foo"
```

This was very unintuitive to me. My understanding is that when USING and
WITH CHECK are both used for an update policy, the USING is tested against
the original row, and the WITH CHECK clause against the new row. This is
stated in the [documentation](
https://www.postgresql.org/docs/current/sql-createpolicy.html):

> The USING expression determines which records the UPDATE command will see
to operate against, while the WITH CHECK expression defines which modified
rows are allowed to be stored back into the relation

So why is it that the addition of where id = 1 violates the select policy?
Later in the same doc:

> Typically an UPDATE command also needs to read data from columns in the
relation being updated (e.g., in a WHERE clause or a RETURNING clause, or
in an expression on the right hand side of the SET clause). In this case,
SELECT rights are also required on the relation being updated, and the
appropriate SELECT or ALL policies will be applied in addition to the
UPDATE policies. Thus the user must have access to the row(s) being updated
through a SELECT or ALL policy in addition to being granted permission to
update the row(s) via an UPDATE or ALL policy.

If you read this documentation perfectly literally, it does describe the
behavior shown my examples above, but it is a bit ambiguous. I think a more
clear explanation of this behavior would be the following:

> Typically an UPDATE command also needs to read data from columns in the
relation being updated (e.g., in a WHERE clause or a RETURNING clause, or
in an expression on the right hand side of the SET clause). In this case,
SELECT rights are also required on the relation being updated, and the
appropriate SELECT or ALL policies will be applied in addition to the
UPDATE policies. Thus the user must have access to the row(s) operated
against and the rows being stored back into the relation via a SELECT or
ALL policy in addition to being granted permission to update the row(s) via
an UPDATE or ALL policy.

However, it seems like there is an opportunity to change the behavior to be
more intuitive, and to support the use case I am attempting. It seems like
use of a WHERE clause that uses the existing row should result in the
select policies only being applied to the row being operated against, not
the row being stored back into the relation. If the caller adds a RETURNING
clause, the existing behavior makes sense, because you are asking to access
the row being stored back into the relation.

Am I thinking about this correctly? I know this would be a breaking change,
albeit a small one. I'm not familiar with Postgres's approach to breaking
changes to know if it would even be considered. At a minimum, does my
proposed documentation change seem reasonable? It would have saved us lots
of time in discovering the behavior.

If this behavior is intentional, I'm also just curious about the rationale
behind it if someone happens to know? Maybe there's a use case that this is
built for that I'm not thinking of.

Responses

Browse pgsql-docs by date

  From Date Subject
Next Message PG Doc comments form 2022-09-16 20:08:04 JSON/JSONB documentation, aggregate functions
Previous Message Laurenz Albe 2022-09-14 16:48:46 Re: suggest about bpchar data type in the document

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2022-09-16 19:24:03 Re: Reducing the WAL overhead of freezing in VACUUM by deduplicating per-tuple freeze plans
Previous Message a.rybakina 2022-09-16 18:51:01 RFC: Logging plan of the running query