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

On Fri, 18 Feb 2022 at 14:57, Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at> wrote:
>
> Here is a new version, with improved documentation and the option renamed
> to "check_permissions_owner". I just prefer the shorter form.
>

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.

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.)

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.

3). Looking at this change:

- setRuleCheckAsUser((Node *) rule->actions, relation->rd_rel->relowner);
- setRuleCheckAsUser(rule->qual, relation->rd_rel->relowner);
+ if (!(relation->rd_rel->relkind == RELKIND_VIEW
+ && !RelationSubqueryCheckPermsOwner(relation)))
+ {
+ setRuleCheckAsUser((Node *) rule->actions,
relation->rd_rel->relowner);
+ setRuleCheckAsUser(rule->qual, relation->rd_rel->relowner);
+ }

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.

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).

(Note: I'm not suggesting that anyone actually spend any time adding
such an option to rules. Given all the pitfalls associated with rules,
I think their use should be discouraged, and no development effort
should be expended enhancing them.)

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.

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.

Regards,
Dean