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

Hi Laurenz,

thank you again for the review!

On 1/20/22 15:20, Laurenz Albe wrote:
> [..]
> I gave the new patch a spin, and got a surprising result:
>
> [..]
>
> INSERT INTO v VALUES (1);
> INSERT 0 1
>
> Huh? "duff" has no permission to insert into "tab"!
That really should not happen, thanks for finding that and helping me
investigating on how to fix that!

This is now solved by checking the security_invoker property on the view
in rewriteTargetView().

I've also added a testcase for this in v4 to catch that in future.

>
> [..]
>
> About the documentation:
>
> --- a/doc/src/sgml/ref/create_view.sgml
> +++ b/doc/src/sgml/ref/create_view.sgml
> + <varlistentry>
> + <term><literal>security_invoker</literal> (<type>boolean</type>)</term>
> + <listitem>
> + <para>
> + If this option is set, it will cause all access to the underlying
> + tables to be checked as referenced by the invoking user, rather than
> + the view owner. This will only take effect when row level security is
> + enabled on the underlying tables (using <link linkend="sql-altertable">
> + <command>ALTER TABLE ... ENABLE ROW LEVEL SECURITY</command></link>).
> + </para>
>
> Why should this *only* take effect if (not "when") RLS is enabled?
> The above test shows that there is an effect even without RLS.
>
> + <para>This option can be changed on existing views using <link
> + linkend="sql-alterview"><command>ALTER VIEW</command></link>. See
> + <xref linkend="ddl-rowsecurity"/> for more details on row level security.
> + </para>
>
> I don't think that it is necessary to mention that this can be changed with
> ALTER VIEW - all storage parameters can be. I guess you copied that from
> the "check_option" documentation, but I would say it need not be mentioned
> there either.
Exactly, I tried to fit it in with the existing parameters.
I moved the link to ALTER VIEW to the end of the paragraph, as it
applies to all options anyways.

>
> + <para>
> + If the <firstterm>security_invoker</firstterm> option is set on the view,
> + access to tables is determined by permissions of the invoking user, rather
> + than the view owner. This can be used to provide stricter permission
> + checking to the underlying tables than by default.
> </para>
>
> Since you are talking about use cases here, RLS might deserve a mention.
Expanded upon a little bit in v4.

>
> --- a/src/backend/access/common/reloptions.c
> +++ b/src/backend/access/common/reloptions.c
> + {
> + {
> + "security_invoker",
> + "View subquery in invoked within the current security context.",
> + RELOPT_KIND_VIEW,
> + AccessExclusiveLock
> + },
> + false
> + },
>
> That doesn't seem to be proper English.
Yes, that happened when rewriting this for v1 -> v2.
Fixed.

Thanks,
Christoph Heiss