Re: RLS Design

On Fri, Aug 29, 2014 at 8:16 PM, Brightwell, Adam
<adam.brightwell@crunchydatasolutions.com> wrote:
> Attached is a patch for RLS that was create against master at
> 01363beae52700c7425cb2d2452177133dad3e93 and is ready for review.
>
> Overview:
>
> This patch provides the capability to create multiple named row level
> security policies for a table on a per command basis and assign them to be
> applied to specific roles/users.
>
> It contains the following changes:
>
> * Syntax:
>
> CREATE POLICY <name> ON <table>
>     [ FOR { ALL | SELECT | INSERT | UPDATE | DELETE } ]
>     [ TO { PUBLIC | <role> [, <role> ] } ]
>     USING (<condition>)
>
> Creates a RLS policy named <name> on <table>.  Specifying a command is
> optional, but the default is ALL.  Specifying a role is options, but the
> default is PUBLIC.  If PUBLIC and other roles are specified, ONLY PUBLIC is
> applied and a warning is raised.
>
> ALTER POLICY <name> ON <table>
>     [ FOR { ALL | SELECT | INSERT | UPDATE | DELETE } ]
>     [ TO { PUBLIC | <role> [, <role> ] } ]
>     USING (<condition>)
>
> Alter a RLS policy named <name> on <table>.  Specifying a command is
> optional, if provided then the policy's command is changed otherwise it is
> left as-is.  Specifying a role is optional, if provided then the policy's
> role is changed otherwise it is left as-is.  The <condition> must always be
> provided and is therefore always replaced.

This is not a full review of this patch; as we're mid-CommitFest, I
assume this will get added to the next CommitFest.

In earlier discussions, it was proposed (and I thought the proposal
was viewed favorably) that when enabling row-level security for a
table (i.e. before doing CREATE POLICY), you'd have to first flip the
table to a default-deny mode:

ALTER TABLE <name> ENABLE ROW LEVEL SECURITY;

+         elog(ERROR, "Table \"%s\" already has a policy named \"%s\"."
+             " Use a different name for the policy or to modify this policy"
+             " use ALTER POLICY %s ON %s USING (qual)",
+             RelationGetRelationName(target_table), stmt->policy_name,
+             RelationGetRelationName(target_table), stmt->policy_name);
+

That needs to be an ereport, be capitalized properly, and the hint, if
it's to be included at all, needs to go into errhint().

+                          errhint("all roles are considered members
of public")));

Wrong message style for a hint.  Also, not sure that's actually
appropriate for a hint.

+         case EXPR_KIND_ROW_SECURITY:
+             return "ROW SECURITY";

This is quite simply bizarre.  That's not the SQL syntax of anything.

+             | ROW SECURITY row_security_option
+                 {
+                     VariableSetStmt *n = makeNode(VariableSetStmt);
+                     n->kind = VAR_SET_VALUE;
+                     n->name = "row_security";
+                     n->args = list_make1(makeStringConst($3, @3));
+                     $$ = n;
+                 }

I object to this.  There's no reason that we should bloat the parser
to allow SET ROW SECURITY in lieu of SET row_security unless this is a
standard-mandated syntax with standard-mandated semantics, which I bet
it isn't.

  /*
+  * Although only "on" and"off" are documented, we accept all likely
variants of
+  * "on" and "off".
+  */
+ static const struct config_enum_entry row_security_options[] = {
+     {"off", ROW_SECURITY_OFF, false},
+     {"on", ROW_SECURITY_ON, false},
+     {"true", ROW_SECURITY_ON, true},
+     {"false", ROW_SECURITY_OFF, true},
+     {"yes", ROW_SECURITY_ON, true},
+     {"no", ROW_SECURITY_OFF, true},
+     {"1", ROW_SECURITY_ON, true},
+     {"0", ROW_SECURITY_OFF, true},
+     {NULL, 0, false}
+ };

Just make it a bool and you get all this for free.

+     if (AuthenticatedUserIsSuperuser)
+         SetConfigOption("row_security", "off", PGC_INTERNAL, PGC_S_OVERRIDE);

Injecting this kind of magic into InitializeSessionUserId(),
SetSessionAuthorization(), and SetCurrentRoleId() seems 100%
unacceptable to me.