Re: Clarifying docs on nuance of select and update policies - Mailing list pgsql-hackers

Thread moved to hackers since it involves complex queries.  Can someone
like Stephen comment on the existing docs and feature behavior?  Thanks.

---------------------------------------------------------------------------

On Fri, Sep 16, 2022 at 11:57:25AM -0700, Chandler Gonzales wrote:
> 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@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@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@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.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson