Add support for restrictive RLS policies

We have had support for restrictive RLS policies since 9.5, but they
were only available through extensions which use the appropriate hooks.
This adds support into the grammer, catalog, psql and pg_dump for
restrictive RLS policies, thus reducing the cases where an extension is
necessary.

In passing, also move away from using "AND"d and "OR"d in comments.
As pointed out by Alvaro, it's not really appropriate to attempt
to make verbs out of "AND" and "OR", so reword those comments which
attempted to.

Reviewed By: Jeevan Chalke, Dean Rasheed
Discussion: https://postgr.es/m/20160901063404.GY4028@tamriel.snowman.net

6 files changed, 84 insertions, 30 deletions

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index e011af122c5..df59d1819ca 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -76,6 +76,12 @@ CREATE VIEW pg_policies AS
         C.relname AS tablename,
         pol.polname AS policyname,
         CASE
+            WHEN pol.polpermissive THEN
+                'PERMISSIVE'
+            ELSE
+                'RESTRICTIVE'
+        END AS permissive,
+        CASE
             WHEN pol.polroles = '{0}' THEN
                 string_to_array('public', '')
             ELSE
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index d694cf80be3..70e22c10000 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -235,6 +235,7 @@ RelationBuildRowSecurity(Relation relation)
 		{
 			Datum		value_datum;
 			char		cmd_value;
+			bool		permissive_value;
 			Datum		roles_datum;
 			char	   *qual_value;
 			Expr	   *qual_expr;
@@ -257,6 +258,12 @@ RelationBuildRowSecurity(Relation relation)
 			Assert(!isnull);
 			cmd_value = DatumGetChar(value_datum);
 
+			/* Get policy permissive or restrictive */
+			value_datum = heap_getattr(tuple, Anum_pg_policy_polpermissive,
+									   RelationGetDescr(catalog), &isnull);
+			Assert(!isnull);
+			permissive_value = DatumGetBool(value_datum);
+
 			/* Get policy name */
 			value_datum = heap_getattr(tuple, Anum_pg_policy_polname,
 									   RelationGetDescr(catalog), &isnull);
@@ -298,6 +305,7 @@ RelationBuildRowSecurity(Relation relation)
 			policy = palloc0(sizeof(RowSecurityPolicy));
 			policy->policy_name = pstrdup(policy_name_value);
 			policy->polcmd = cmd_value;
+			policy->permissive = permissive_value;
 			policy->roles = DatumGetArrayTypePCopy(roles_datum);
 			policy->qual = copyObject(qual_expr);
 			policy->with_check_qual = copyObject(with_check_qual);
@@ -796,6 +804,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
 	values[Anum_pg_policy_polname - 1] = DirectFunctionCall1(namein,
 										 CStringGetDatum(stmt->policy_name));
 	values[Anum_pg_policy_polcmd - 1] = CharGetDatum(polcmd);
+	values[Anum_pg_policy_polpermissive - 1] = BoolGetDatum(stmt->permissive);
 	values[Anum_pg_policy_polroles - 1] = PointerGetDatum(role_ids);
 
 	/* Add qual if present. */
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 04e49b77951..dd66adb0b24 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -4166,6 +4166,7 @@ _copyCreatePolicyStmt(const CreatePolicyStmt *from)
 	COPY_STRING_FIELD(policy_name);
 	COPY_NODE_FIELD(table);
 	COPY_STRING_FIELD(cmd_name);
+	COPY_SCALAR_FIELD(permissive);
 	COPY_NODE_FIELD(roles);
 	COPY_NODE_FIELD(qual);
 	COPY_NODE_FIELD(with_check);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 2eaf41c37f8..cad3aebecd5 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2125,6 +2125,7 @@ _equalCreatePolicyStmt(const CreatePolicyStmt *a, const CreatePolicyStmt *b)
 	COMPARE_STRING_FIELD(policy_name);
 	COMPARE_NODE_FIELD(table);
 	COMPARE_STRING_FIELD(cmd_name);
+	COMPARE_SCALAR_FIELD(permissive);
 	COMPARE_NODE_FIELD(roles);
 	COMPARE_NODE_FIELD(qual);
 	COMPARE_NODE_FIELD(with_check);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index d6274b49e7f..414348b95b4 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -332,6 +332,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type <str>		all_Op MathOp
 
 %type <str>		row_security_cmd RowSecurityDefaultForCmd
+%type <boolean> RowSecurityDefaultPermissive
 %type <node>	RowSecurityOptionalWithCheck RowSecurityOptionalExpr
 %type <list>	RowSecurityDefaultToRole RowSecurityOptionalToRole
 
@@ -4628,26 +4629,30 @@ AlterUserMappingStmt: ALTER USER MAPPING FOR auth_ident SERVER name alter_generi
 /*****************************************************************************
  *
  *		QUERIES:
- *				CREATE POLICY name ON table [FOR cmd] [TO role, ...]
- *					[USING (qual)] [WITH CHECK (with_check)]
+ *				CREATE POLICY name ON table
+ *					[AS { PERMISSIVE | RESTRICTIVE } ]
+ *					[FOR { SELECT | INSERT | UPDATE | DELETE } ]
+ *					[TO role, ...]
+ *					[USING (qual)] [WITH CHECK (with check qual)]
  *				ALTER POLICY name ON table [TO role, ...]
- *					[USING (qual)] [WITH CHECK (with_check)]
+ *					[USING (qual)] [WITH CHECK (with check qual)]
  *				DROP POLICY name ON table
  *
  *****************************************************************************/
 
 CreatePolicyStmt:
-			CREATE POLICY name ON qualified_name RowSecurityDefaultForCmd
-				RowSecurityDefaultToRole RowSecurityOptionalExpr
-				RowSecurityOptionalWithCheck
+			CREATE POLICY name ON qualified_name RowSecurityDefaultPermissive
+				RowSecurityDefaultForCmd RowSecurityDefaultToRole
+				RowSecurityOptionalExpr RowSecurityOptionalWithCheck
 				{
 					CreatePolicyStmt *n = makeNode(CreatePolicyStmt);
 					n->policy_name = $3;
 					n->table = $5;
-					n->cmd_name = $6;
-					n->roles = $7;
-					n->qual = $8;
-					n->with_check = $9;
+					n->permissive = $6;
+					n->cmd_name = $7;
+					n->roles = $8;
+					n->qual = $9;
+					n->with_check = $10;
 					$$ = (Node *) n;
 				}
 		;
@@ -4711,6 +4716,24 @@ RowSecurityOptionalToRole:
 			| /* EMPTY */			{ $$ = NULL; }
 		;
 
+RowSecurityDefaultPermissive:
+			AS IDENT
+				{
+					if (strcmp($2, "permissive") == 0)
+						$$ = true;
+					else if (strcmp($2, "restrictive") == 0)
+						$$ = false;
+					else
+						ereport(ERROR,
+								(errcode(ERRCODE_SYNTAX_ERROR),
+							 errmsg("unrecognized row security option \"%s\"", $2),
+								 errhint("Only PERMISSIVE or RESTRICTIVE policies are supported currently."),
+									 parser_errposition(@2)));
+
+				}
+			| /* EMPTY */			{ $$ = true; }
+		;
+
 RowSecurityDefaultForCmd:
 			FOR row_security_cmd	{ $$ = $2; }
 			| /* EMPTY */			{ $$ = "all"; }
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index e02911656a3..b7edefc7ddf 100644
--- a/src/backend/rewrite/rowsecurity.c
+++ b/src/backend/rewrite/rowsecurity.c
@@ -86,10 +86,10 @@ static bool check_role_for_policy(ArrayType *policy_roles, Oid user_id);
  * hooks to allow extensions to add their own security policies
  *
  * row_security_policy_hook_permissive can be used to add policies which
- * are included in the "OR"d set of policies.
+ * are combined with the other permissive policies, using OR.
  *
  * row_security_policy_hook_restrictive can be used to add policies which
- * are enforced, regardless of other policies (they are "AND"d).
+ * are enforced, regardless of other policies (they are combined using AND).
  */
 row_security_policy_hook_type row_security_policy_hook_permissive = NULL;
 row_security_policy_hook_type row_security_policy_hook_restrictive = NULL;
@@ -212,8 +212,8 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index,
 	/*
 	 * For SELECT, UPDATE and DELETE, add security quals to enforce the USING
 	 * policies.  These security quals control access to existing table rows.
-	 * Restrictive policies are "AND"d together, and permissive policies are
-	 * "OR"d together.
+	 * Restrictive policies are combined together using AND, and permissive
+	 * policies are combined together using OR.
 	 */
 
 	get_policies_for_relation(rel, commandType, user_id, &permissive_policies,
@@ -433,10 +433,21 @@ get_policies_for_relation(Relation relation, CmdType cmd, Oid user_id,
 		 * the specified role.
 		 */
 		if (cmd_matches && check_role_for_policy(policy->roles, user_id))
-			*permissive_policies = lappend(*permissive_policies, policy);
+		{
+			if (policy->permissive)
+				*permissive_policies = lappend(*permissive_policies, policy);
+			else
+				*restrictive_policies = lappend(*restrictive_policies, policy);
+		}
 	}
 
 	/*
+	 * We sort restrictive policies by name so that any WCOs they generate are
+	 * checked in a well-defined order.
+	 */
+	*restrictive_policies = sort_policies_by_name(*restrictive_policies);
+
+	/*
 	 * Then add any permissive or restrictive policies defined by extensions.
 	 * These are simply appended to the lists of internal policies, if they
 	 * apply to the specified role.
@@ -447,8 +458,10 @@ get_policies_for_relation(Relation relation, CmdType cmd, Oid user_id,
 		(*row_security_policy_hook_restrictive) (cmd, relation);
 
 		/*
-		 * We sort restrictive policies by name so that any WCOs they generate
-		 * are checked in a well-defined order.
+		 * As with built-in restrictive policies, we sort any hook-provided
+		 * restrictive policies by name also.  Note that we also intentionally
+		 * always check all built-in restrictive policies, in name order,
+		 * before checking restrictive policies added by hooks, in name order.
 		 */
 		hook_policies = sort_policies_by_name(hook_policies);
 
@@ -481,8 +494,8 @@ get_policies_for_relation(Relation relation, CmdType cmd, Oid user_id,
  *
  * This is only used for restrictive policies, ensuring that any
  * WithCheckOptions they generate are applied in a well-defined order.
- * This is not necessary for permissive policies, since they are all "OR"d
- * together into a single WithCheckOption check.
+ * This is not necessary for permissive policies, since they are all combined
+ * together using OR into a single WithCheckOption check.
  */
 static List *
 sort_policies_by_name(List *policies)
@@ -580,8 +593,8 @@ add_security_quals(int rt_index,
 		/*
 		 * We now know that permissive policies exist, so we can now add
 		 * security quals based on the USING clauses from the restrictive
-		 * policies.  Since these need to be "AND"d together, we can just add
-		 * them one at a time.
+		 * policies.  Since these need to be combined together using AND, we
+		 * can just add them one at a time.
 		 */
 		foreach(item, restrictive_policies)
 		{
@@ -599,8 +612,8 @@ add_security_quals(int rt_index,
 		}
 
 		/*
-		 * Then add a single security qual "OR"ing together the USING clauses
-		 * from all the permissive policies.
+		 * Then add a single security qual combining together the USING
+		 * clauses from all the permissive policies using OR.
 		 */
 		if (list_length(permissive_quals) == 1)
 			rowsec_expr = (Expr *) linitial(permissive_quals);
@@ -681,10 +694,11 @@ add_with_check_options(Relation rel,
 	if (permissive_quals != NIL)
 	{
 		/*
-		 * Add a single WithCheckOption for all the permissive policy clauses
-		 * "OR"d together.  This check has no policy name, since if the check
-		 * fails it means that no policy granted permission to perform the
-		 * update, rather than any particular policy being violated.
+		 * Add a single WithCheckOption for all the permissive policy clauses,
+		 * combining them together using OR.  This check has no policy name,
+		 * since if the check fails it means that no policy granted permission
+		 * to perform the update, rather than any particular policy being
+		 * violated.
 		 */
 		WithCheckOption *wco;
 
@@ -705,9 +719,9 @@ add_with_check_options(Relation rel,
 
 		/*
 		 * Now add WithCheckOptions for each of the restrictive policy clauses
-		 * (which will be "AND"d together).  We use a separate WithCheckOption
-		 * for each restrictive policy to allow the policy name to be included
-		 * in error reports if the policy is violated.
+		 * (which will be combined together using AND).  We use a separate
+		 * WithCheckOption for each restrictive policy to allow the policy
+		 * name to be included in error reports if the policy is violated.
 		 */
 		foreach(item, restrictive_policies)
 		{