Re: [PATCH] Add reloption for views to enable RLS

Thanks for reviewing!

On 2/25/22 19:22, Dean Rasheed wrote:
> Re-reading this thread, I think I preferred the name
> "security_invoker". The main objection seemed to come from the
> potential confusion with SECURITY INVOKER/DEFINER functions, but I
> think that's really a different thing. As long as the documentation
> for the default behaviour is clear (which I think it was), then it
> should be easy to explain how a security invoker view behaves
> differently. Also, there's value in using the same terminology as
> other databases, because many users will already be familiar with the
> feature from those databases.

That is also the main reason I preferred naming it "security_invoker" -
it is consistent with other databases and eases transition from such
systems.

I kept "check_permissions_owner" for now. Constantly changing it around
with each iteration doesn't really bring any value IMHO, I'd rather have
a final consensus on how to name the option and *then* change it for good.

>
> Some other review comments:
>
> 1). This new comment:
>
> + <para>
> + Be aware that <literal>USAGE</literal> privileges on schemas containing
> + the underlying base relations are <emphasis>not</emphasis> checked.
> + </para>
>
> is not entirely accurate. It's more accurate to say that a user
> creating or replacing a view must have CREATE privileges on the schema
> containing the view and USAGE privileges on any schemas referred to in
> the view query, whereas a user using the view only needs USAGE
> privileges on the schema containing the view.
>
> (Note that, for the view creator, USAGE is required on any schema
> referred to in the query -- e.g., schemas containing functions as well
> as base relations.)

Improved in the attached v9.

>
> 2). The patch is adding a new field to RangeTblEntry which seems to be
> unnecessary -- it's set, and copied around, but never read, so it
> should just be removed.

I removed that field in v9 since it is indeed completely unused. I
initially added it to be consistent with the "security_barrier"
implementation and than somewhat forgot about it.

>
> 3). Looking at this change:
>
> [..]
>
> I think it should call setRuleCheckAsUser() in all cases. It might be
> true that the rule fetched has checkAsUser set to InvalidOid
> throughout its action and quals, but it seems unwise to rely on that
> -- better to code defensively and explicitly set it in all cases.

It probably doesn't really matter, but I agree that coding defensively
is always a good thing.
Changed that in v9 to call setRuleCheckAsUser() either with ->relowner
or InvalidOid.

>
> 4). In the same code block, I think the new behaviour should be
> applied to SELECT rules only. The view may have other non-SELECT rules
> (just as a table may have non-SELECT rules), created using CREATE
> RULE, but their actions are independent of the view definition.
> Currently their permissions are checked as the view/table owner, and
> if anyone wanted to change that, it should be an option on the rule,
> not the view (just as triggers can be made security definer or
> invoker, depending on how the trigger function is defined).
>

Good catch, I added a additional check for rule->event and a test for
that in v9.
[ I also had to add a missing DROP statement to some previous test, just
a heads up. ]

It makes sense to mimic the behavior of triggers and further,
user-created rules otherwise might behave differently for tables and
views, depending on the view definition.
[ But I'm not _that_ familiar with CREATE RULE, FWIW. ]

>
> 5). In the same function, the block of code that fetches rules and
> triggers has been moved. I think it would be worth adding a comment to
> explain why it's now important to extract the reloptions *before*
> fetching the relation's rules and triggers.

Added a small comment explaining that in v9.

>
> 6). The second set of tests added to rowsecurity.sql seem to have
> nothing to do with RLS, and probably belong in updatable_views.sql,
> and I think it would be worth adding a few more tests for things like
> views on top of views.

Seems reasonable to move them into updatable_views.sql, done that for
v9. Further I added two (simple) tests for chained views as you
mentioned, hope they reflect what you had in mind.

Thanks,
Christoph