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

On Tue, 2022-01-18 at 16:16 +0100, Christoph Heiss wrote:
> > I think that this should be a boolean reloption, for example "security_definer".
> > If unset or set to "off", you would get the current behavior.
>
> A boolean option would have been indeed the better choice, I agree.
> I haven't though of any specific other values for this enum, it was
> rather a decision following a off-list discussion.
>
> I've changed the option to be boolean and renamed it to
> "security_invoker". This puts it in line with how other systems (e.g.
> MySQL) name their equivalent feature, so I think this should be an
> appropriate choice.
>
>
> > Also, I don't think that the documentation on
> > RLS policies is the correct place for this.  It should be on a page dedicated to views
> > or permissions.
> >
> > The CREATE VIEW page already has a paragraph about this, starting with
> > "Access to tables referenced in the view is determined by permissions of the view owner."
> > This looks like the best place to me (and it would need to be adapted anyway).
> It makes sense to put it there, thanks for the pointer! I wasn't really
> that sure where to put the documentation to start with, and this seems
> like a more appropriate place.

I gave the new patch a spin, and got a surprising result:

CREATE TABLE tab (id integer);

CREATE ROLE duff LOGIN;

CREATE ROLE jock LOGIN;

GRANT INSERT, UPDATE, DELETE ON tab TO jock;

GRANT SELECT ON tab TO duff;

CREATE VIEW v WITH (security_invoker = TRUE) AS SELECT * FROM tab;

ALTER VIEW v OWNER TO jock;

GRANT SELECT, INSERT, UPDATE, DELETE ON v TO duff;

SET SESSION AUTHORIZATION duff;

SELECT * FROM v;
id
════
(0 rows)

That's ok, "duff" has permissions to read "tab".

INSERT INTO v VALUES (1);
INSERT 0 1

Huh? "duff" has no permission to insert into "tab"!

RESET SESSION AUTHORIZATION;

ALTER VIEW v SET (security_invoker = FALSE);

SET SESSION AUTHORIZATION duff;

SELECT * FROM v;
ERROR: permission denied for table tab

As expected.

INSERT INTO v VALUES (1);
INSERT 0 1

As expected.

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.

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

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

Yours,
Laurenz Albe