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

On Tue, 2022-03-08 at 18:17 +0100, Christoph Heiss wrote:
> Since there don't seem to be any more objections to "security_invoker" I 
> attached v10 renaming it again.
> 
> I've tried to better clarify the whole invoker vs. definer thing in the 
> CREATE VIEW documentation by explicitly mentioning that 
> "security_invoker=false" is _not_ the same as "security definer", based 
> on the earlier discussions.
> 
> This should hopefully avoid any implicit associations.

I have only some minor comments:

> --- a/doc/src/sgml/ref/create_view.sgml
> +++ b/doc/src/sgml/ref/create_view.sgml
> @@ -387,10 +430,17 @@ CREATE VIEW vista AS SELECT text 'Hello World' AS hello;
>     <para>
>      Note that the user performing the insert, update or delete on the view
>      must have the corresponding insert, update or delete privilege on the
> -    view.  In addition the view's owner must have the relevant privileges on
> -    the underlying base relations, but the user performing the update does
> -    not need any permissions on the underlying base relations (see
> -    <xref linkend="rules-privileges"/>).
> +    view.
> +   </para>
> +
> +   <para>
> +    Additionally, by default the view's owner must have the relevant privileges
> +    on the underlying base relations, but the user performing the update does
> +    not need any permissions on the underlying base relations. (see
> +    <xref linkend="rules-privileges"/>)  If the view has the
> +    <literal>security_invoker</literal> property is set to
> +    <literal>true</literal>, the invoking user will need to have the relevant
> +    privileges rather than the view owner.
>     </para>
>    </refsect2>
>   </refsect1>

This paragraph contains a couple of grammatical errors.
How about

  <para>
   Note that the user performing the insert, update or delete on the view
   must have the corresponding insert, update or delete privilege on the
   view.  Unless <literal>security_invoker</literal> is set to
   <literal>true</literal>, the view's owner must additionally have the
   relevant privileges on the underlying base relations, but the user
   performing the update does not need any permissions on the underlying
   base relations (see <xref linkend="rules-privileges"/>).
   If <literal>security_invoker</literal> is set to <literal>true</literal>,
   it is the invoking user rather than the view owner that must have the
   relevant privileges on the underlying base relations.
  </para>

Also, this:

> --- a/src/backend/utils/cache/relcache.c
> +++ b/src/backend/utils/cache/relcache.c
> @@ -838,8 +846,18 @@ RelationBuildRuleLock(Relation relation)
>          * the rule tree during load is relatively cheap (compared to
>          * constructing it in the first place), so we do it here.
>          */
> -       setRuleCheckAsUser((Node *) rule->actions, relation->rd_rel->relowner);
> -       setRuleCheckAsUser(rule->qual, relation->rd_rel->relowner);
> +       if (rule->event == CMD_SELECT
> +           && relation->rd_rel->relkind == RELKIND_VIEW
> +           && RelationHasSecurityInvoker(relation))
> +       {
> +           setRuleCheckAsUser((Node *) rule->actions, InvalidOid);
> +           setRuleCheckAsUser(rule->qual, InvalidOid);
> +       }
> +       else
> +       {
> +           setRuleCheckAsUser((Node *) rule->actions, relation->rd_rel->relowner);
> +           setRuleCheckAsUser(rule->qual, relation->rd_rel->relowner);
> +       }

could be written like this (introducing a new variable):

  if (rule->event == CMD_SELECT
      && relation->rd_rel->relkind == RELKIND_VIEW
      && RelationHasSecurityInvoker(relation))
      user_for_check = InvalidOid;
  else
      user_for_check = relation->rd_rel->relowner;

  setRuleCheckAsUser((Node *) rule->actions, user_for_check);
  setRuleCheckAsUser(rule->qual, user_for_check);

This might be easier to read.


Yours,
Laurenz Albe