RLS fixes, new hooks, and new test module

In prepend_row_security_policies(), defaultDeny was always true, so if
there were any hook policies, the RLS policies on the table would just
get discarded.  Fixed to start off with defaultDeny as false and then
properly set later if we detect that only the default deny policy exists
for the internal policies.

The infinite recursion detection in fireRIRrules() didn't properly
manage the activeRIRs list in the case of WCOs, so it would incorrectly
report infinite recusion if the same relation with RLS appeared more
than once in the rtable, for example "UPDATE t ... FROM t ...".

Further, the RLS expansion code in fireRIRrules() was handling RLS in
the main loop through the rtable, which lead to RTEs being visited twice
if they contained sublink subqueries, which
prepend_row_security_policies() attempted to handle by exiting early if
the RTE already had securityQuals.  That doesn't work, however, since
if the query involved a security barrier view on top of a table with
RLS, the RTE would already have securityQuals (from the view) by the
time fireRIRrules() was invoked, and so the table's RLS policies would
be ignored.  This is fixed in fireRIRrules() by handling RLS in a
separate loop at the end, after dealing with any other sublink
subqueries, thus ensuring that each RTE is only visited once for RLS
expansion.

The inheritance planner code didn't correctly handle non-target
relations with RLS, which would get turned into subqueries during
planning. Thus an update of the form "UPDATE t1 ... FROM t2 ..." where
t1 has inheritance and t2 has RLS quals would fail.  Fix by making sure
to copy in and update the securityQuals when they exist for non-target
relations.

process_policies() was adding WCOs to non-target relations, which is
unnecessary, and could lead to a lot of wasted time in the rewriter and
the planner. Fix by only adding WCO policies when working on the result
relation.  Also in process_policies, we should be copying the USING
policies to the WITH CHECK policies on a per-policy basis, fix by moving
the copying up into the per-policy loop.

Lastly, as noted by Dean, we were simply adding policies returned by the
hook provided to the list of quals being AND'd, meaning that they would
actually restrict records returned and there was no option to have
internal policies and hook-based policies work together permissively (as
all internal policies currently work).  Instead, explicitly add support
for both permissive and restrictive policies by having a hook for each
and combining the results appropriately.  To ensure this is all done
correctly, add a new test module (test_rls_hooks) to test the various
combinations of internal, permissive, and restrictive hook policies.

Largely from Dean Rasheed (thanks!):

CAEZATCVmFUfUOwwhnBTcgi6AquyjQ0-1fyKd0T3xBWJvn+xsFA@mail.gmail.com

Author: Dean Rasheed, though I added the new hooks and test module.

1 files changed, 175 insertions, 85 deletions

diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index 7669130e2b6..bad166ac3ad 100644
--- a/src/backend/rewrite/rowsecurity.c
+++ b/src/backend/rewrite/rowsecurity.c
@@ -58,46 +58,63 @@
 
 static List *pull_row_security_policies(CmdType cmd, Relation relation,
 										Oid user_id);
-static void process_policies(List *policies, int rt_index,
+static void process_policies(Query* root, List *policies, int rt_index,
 							 Expr **final_qual,
 							 Expr **final_with_check_qual,
-							 bool *hassublinks);
+							 bool *hassublinks,
+							 BoolExprType boolop);
 static bool check_role_for_policy(ArrayType *policy_roles, Oid user_id);
 
 /*
- * hook to allow extensions to apply their own security policy
+ * 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.
+ *
+ * row_security_policy_hook_restrictive can be used to add policies which
+ * are enforced, regardless of other policies (they are "AND"d).
  *
  * See below where the hook is called in prepend_row_security_policies for
  * insight into how to use this hook.
  */
-row_security_policy_hook_type	row_security_policy_hook = NULL;
+row_security_policy_hook_type	row_security_policy_hook_permissive = NULL;
+row_security_policy_hook_type	row_security_policy_hook_restrictive = NULL;
 
 /*
- * Check the given RTE to see whether it's already had row security quals
- * expanded and, if not, prepend any row security rules from built-in or
- * plug-in sources to the securityQuals. The security quals are rewritten (for
- * view expansion, etc) before being added to the RTE.
+ * Get any row security quals and check quals that should be applied to the
+ * specified RTE.
  *
- * Returns true if any quals were added. Note that quals may have been found
- * but not added if user rights make the user exempt from row security.
+ * In addition, hasRowSecurity is set to true if row level security is enabled
+ * (even if this RTE doesn't have any row security quals), and hasSubLinks is
+ * set to true if any of the quals returned contain sublinks.
  */
-bool
-prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
+void
+get_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index,
+						  List **securityQuals, List **withCheckOptions,
+						  bool *hasRowSecurity, bool *hasSubLinks)
 {
 	Expr			   *rowsec_expr = NULL;
 	Expr			   *rowsec_with_check_expr = NULL;
-	Expr			   *hook_expr = NULL;
-	Expr			   *hook_with_check_expr = NULL;
+	Expr			   *hook_expr_restrictive = NULL;
+	Expr			   *hook_with_check_expr_restrictive = NULL;
+	Expr			   *hook_expr_permissive = NULL;
+	Expr			   *hook_with_check_expr_permissive = NULL;
 
 	List			   *rowsec_policies;
-	List			   *hook_policies = NIL;
+	List			   *hook_policies_restrictive = NIL;
+	List			   *hook_policies_permissive = NIL;
 
 	Relation 			rel;
 	Oid					user_id;
 	int					sec_context;
 	int					rls_status;
-	bool				defaultDeny = true;
-	bool				hassublinks = false;
+	bool				defaultDeny = false;
+
+	/* Defaults for the return values */
+	*securityQuals = NIL;
+	*withCheckOptions = NIL;
+	*hasRowSecurity = false;
+	*hasSubLinks = false;
 
 	/* This is just to get the security context */
 	GetUserIdAndSecContext(&user_id, &sec_context);
@@ -113,14 +130,14 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
 	if (rte->relid < FirstNormalObjectId
 		|| rte->relkind != RELKIND_RELATION
 		|| (sec_context & SECURITY_ROW_LEVEL_DISABLED))
-		return false;
+		return;
 
 	/* Determine the state of RLS for this, pass checkAsUser explicitly */
 	rls_status = check_enable_rls(rte->relid, rte->checkAsUser, false);
 
 	/* If there is no RLS on this table at all, nothing to do */
 	if (rls_status == RLS_NONE)
-		return false;
+		return;
 
 	/*
 	 * RLS_NONE_ENV means we are not doing any RLS now, but that may change
@@ -134,18 +151,11 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
 		 * be replanned if the environment changes (GUCs, role), but we
 		 * are not adding anything here.
 		 */
-		root->hasRowSecurity = true;
+		*hasRowSecurity = true;
 
-		return false;
+		return;
 	}
 
-	/*
-	 * We may end up getting called multiple times for the same RTE, so check
-	 * to make sure we aren't doing double-work.
-	 */
-	if (rte->securityQuals != NIL)
-		return false;
-
 	/* Grab the built-in policies which should be applied to this relation. */
 	rel = heap_open(rte->relid, NoLock);
 
@@ -167,20 +177,17 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
 		defaultDeny = true;
 
 	/* Now that we have our policies, build the expressions from them. */
-	process_policies(rowsec_policies, rt_index, &rowsec_expr,
-					 &rowsec_with_check_expr, &hassublinks);
+	process_policies(root, rowsec_policies, rt_index, &rowsec_expr,
+					 &rowsec_with_check_expr, hasSubLinks, OR_EXPR);
 
 	/*
 	 * Also, allow extensions to add their own policies.
 	 *
+	 * extensions can add either permissive or restrictive policies.
+	 *
 	 * Note that, as with the internal policies, if multiple policies are
 	 * returned then they will be combined into a single expression with
-	 * all of them OR'd together.  However, to avoid the situation of an
-	 * extension granting more access to a table than the internal policies
-	 * would allow, the extension's policies are AND'd with the internal
-	 * policies.  In other words - extensions can only provide further
-	 * filtering of the result set (or further reduce the set of records
-	 * allowed to be added).
+	 * all of them OR'd (for permissive) or AND'd (for restrictive) together.
 	 *
 	 * If only a USING policy is returned by the extension then it will be
 	 * used for WITH CHECK as well, similar to how internal policies are
@@ -192,23 +199,42 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
 	 * default-deny policy and use only the policies returned by the
 	 * extension.
 	 */
-	if (row_security_policy_hook)
+	if (row_security_policy_hook_restrictive)
 	{
-		hook_policies = (*row_security_policy_hook)(root->commandType, rel);
+		hook_policies_restrictive = (*row_security_policy_hook_restrictive)(root->commandType, rel);
 
 		/* Build the expression from any policies returned. */
-		process_policies(hook_policies, rt_index, &hook_expr,
-						 &hook_with_check_expr, &hassublinks);
+		if (hook_policies_restrictive != NIL)
+			process_policies(root, hook_policies_restrictive, rt_index,
+							 &hook_expr_restrictive,
+							 &hook_with_check_expr_restrictive,
+							 hasSubLinks,
+							 AND_EXPR);
+	}
+
+	if (row_security_policy_hook_permissive)
+	{
+		hook_policies_permissive = (*row_security_policy_hook_permissive)(root->commandType, rel);
+
+		/* Build the expression from any policies returned. */
+		if (hook_policies_permissive != NIL)
+			process_policies(root, hook_policies_permissive, rt_index,
+							 &hook_expr_permissive,
+							 &hook_with_check_expr_permissive, hasSubLinks,
+							 OR_EXPR);
 	}
 
 	/*
 	 * If the only built-in policy is the default-deny one, and hook
 	 * policies exist, then use the hook policies only and do not apply
-	 * the default-deny policy.  Otherwise, apply both sets (AND'd
-	 * together).
+	 * the default-deny policy.  Otherwise, we will apply both sets below.
 	 */
-	if (defaultDeny && hook_policies != NIL)
+	if (defaultDeny &&
+		(hook_policies_restrictive != NIL || hook_policies_permissive != NIL))
+	{
 		rowsec_expr = NULL;
+		rowsec_with_check_expr = NULL;
+	}
 
 	/*
 	 * For INSERT or UPDATE, we need to add the WITH CHECK quals to
@@ -222,29 +248,65 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
 		 * WITH CHECK OPTIONS wants a WCO node which wraps each Expr, so
 		 * create them as necessary.
 		 */
-		if (rowsec_with_check_expr)
+
+		/*
+		 * Handle any restrictive policies first.
+		 *
+		 * They can simply be added.
+		 */
+		if (hook_with_check_expr_restrictive)
 		{
 			WithCheckOption	   *wco;
 
 			wco = (WithCheckOption *) makeNode(WithCheckOption);
 			wco->viewname = RelationGetRelationName(rel);
-			wco->qual = (Node *) rowsec_with_check_expr;
+			wco->qual = (Node *) hook_with_check_expr_restrictive;
 			wco->cascaded = false;
-			root->withCheckOptions = lcons(wco, root->withCheckOptions);
+			*withCheckOptions = lappend(*withCheckOptions, wco);
 		}
 
 		/*
-		 * Ditto for the expression, if any, returned from the extension.
+		 * Handle built-in policies, if there are no permissive
+		 * policies from the hook.
 		 */
-		if (hook_with_check_expr)
+		if (rowsec_with_check_expr && !hook_with_check_expr_permissive)
+		{
+			WithCheckOption	   *wco;
+
+			wco = (WithCheckOption *) makeNode(WithCheckOption);
+			wco->viewname = RelationGetRelationName(rel);
+			wco->qual = (Node *) rowsec_with_check_expr;
+			wco->cascaded = false;
+			*withCheckOptions = lappend(*withCheckOptions, wco);
+		}
+		/* Handle the hook policies, if there are no built-in ones. */
+		else if (!rowsec_with_check_expr && hook_with_check_expr_permissive)
 		{
 			WithCheckOption	   *wco;
 
 			wco = (WithCheckOption *) makeNode(WithCheckOption);
 			wco->viewname = RelationGetRelationName(rel);
-			wco->qual = (Node *) hook_with_check_expr;
+			wco->qual = (Node *) hook_with_check_expr_permissive;
 			wco->cascaded = false;
-			root->withCheckOptions = lcons(wco, root->withCheckOptions);
+			*withCheckOptions = lappend(*withCheckOptions, wco);
+		}
+		/* Handle the case where there are both. */
+		else if (rowsec_with_check_expr && hook_with_check_expr_permissive)
+		{
+			WithCheckOption	   *wco;
+			List			   *combined_quals = NIL;
+			Expr			   *combined_qual_eval;
+
+			combined_quals = lcons(copyObject(rowsec_with_check_expr), combined_quals);
+			combined_quals = lcons(copyObject(hook_with_check_expr_permissive), combined_quals);
+
+			combined_qual_eval = makeBoolExpr(OR_EXPR, combined_quals, -1);
+
+			wco = (WithCheckOption *) makeNode(WithCheckOption);
+			wco->viewname = RelationGetRelationName(rel);
+			wco->qual = (Node *) combined_qual_eval;
+			wco->cascaded = false;
+			*withCheckOptions = lappend(*withCheckOptions, wco);
 		}
 	}
 
@@ -253,12 +315,29 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
 		|| root->commandType == CMD_UPDATE
 		|| root->commandType == CMD_DELETE)
 	{
-		if (rowsec_expr)
-			rte->securityQuals = lcons(rowsec_expr, rte->securityQuals);
+		/* restrictive policies can simply be added to the list first */
+		if (hook_expr_restrictive)
+			*securityQuals = lappend(*securityQuals, hook_expr_restrictive);
+
+		/* If we only have internal permissive, then just add those */
+		if (rowsec_expr && !hook_expr_permissive)
+			*securityQuals = lappend(*securityQuals, rowsec_expr);
+		/* .. and if we have only permissive policies from the hook */
+		else if (!rowsec_expr && hook_expr_permissive)
+			*securityQuals = lappend(*securityQuals, hook_expr_permissive);
+		/* if we have both, we have to combine them with an OR */
+		else if (rowsec_expr && hook_expr_permissive)
+		{
+			List   *combined_quals = NIL;
+			Expr   *combined_qual_eval;
 
-		if (hook_expr)
-			rte->securityQuals = lcons(hook_expr,
-									   rte->securityQuals);
+			combined_quals = lcons(copyObject(rowsec_expr), combined_quals);
+			combined_quals = lcons(copyObject(hook_expr_permissive), combined_quals);
+
+			combined_qual_eval = makeBoolExpr(OR_EXPR, combined_quals, -1);
+
+			*securityQuals = lappend(*securityQuals, combined_qual_eval);
+		}
 	}
 
 	heap_close(rel, NoLock);
@@ -267,17 +346,9 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
 	 * Mark this query as having row security, so plancache can invalidate
 	 * it when necessary (eg: role changes)
 	 */
-	root->hasRowSecurity = true;
+	*hasRowSecurity = true;
 
-	/*
-	 * If we have sublinks added because of the policies being added to the
-	 * query, then set hasSubLinks on the Query to force subLinks to be
-	 * properly expanded.
-	 */
-	root->hasSubLinks |= hassublinks;
-
-	/* If we got this far, we must have added quals */
-	return true;
+	return;
 }
 
 /*
@@ -292,7 +363,6 @@ pull_row_security_policies(CmdType cmd, Relation relation, Oid user_id)
 {
 	List			   *policies = NIL;
 	ListCell		   *item;
-	RowSecurityPolicy  *policy;
 
 	/*
 	 * Row security is enabled for the relation and the row security GUC is
@@ -302,7 +372,7 @@ pull_row_security_policies(CmdType cmd, Relation relation, Oid user_id)
 	 */
 	foreach(item, relation->rd_rsdesc->policies)
 	{
-		policy = (RowSecurityPolicy *) lfirst(item);
+		RowSecurityPolicy  *policy = (RowSecurityPolicy *) lfirst(item);
 
 		/* Always add ALL policies, if they exist. */
 		if (policy->polcmd == '*' &&
@@ -383,8 +453,9 @@ pull_row_security_policies(CmdType cmd, Relation relation, Oid user_id)
  * qual_eval, with_check_eval, and hassublinks are output variables
  */
 static void
-process_policies(List *policies, int rt_index, Expr **qual_eval,
-				 Expr **with_check_eval, bool *hassublinks)
+process_policies(Query* root, List *policies, int rt_index, Expr **qual_eval,
+				 Expr **with_check_eval, bool *hassublinks,
+				 BoolExprType boolop)
 {
 	ListCell		   *item;
 	List			   *quals = NIL;
@@ -392,7 +463,8 @@ process_policies(List *policies, int rt_index, Expr **qual_eval,
 
 	/*
 	 * Extract the USING and WITH CHECK quals from each of the policies
-	 * and add them to our lists.
+	 * and add them to our lists.  We only want WITH CHECK quals if this
+	 * RTE is the query's result relation.
 	 */
 	foreach(item, policies)
 	{
@@ -401,10 +473,22 @@ process_policies(List *policies, int rt_index, Expr **qual_eval,
 		if (policy->qual != NULL)
 			quals = lcons(copyObject(policy->qual), quals);
 
-		if (policy->with_check_qual != NULL)
+		if (policy->with_check_qual != NULL &&
+			rt_index == root->resultRelation)
 			with_check_quals = lcons(copyObject(policy->with_check_qual),
 									 with_check_quals);
 
+		/*
+		 * For each policy, if there is only a USING clause then copy/use it for
+		 * the WITH CHECK policy also, if this RTE is the query's result
+		 * relation.
+		 */
+		if (policy->qual != NULL && policy->with_check_qual == NULL &&
+			rt_index == root->resultRelation)
+			with_check_quals = lcons(copyObject(policy->qual),
+									 with_check_quals);
+
+
 		if (policy->hassublinks)
 			*hassublinks = true;
 	}
@@ -418,40 +502,46 @@ process_policies(List *policies, int rt_index, Expr **qual_eval,
 								BoolGetDatum(false), false, true), quals);
 
 	/*
-	 * If we end up with only USING quals, then use those as
-	 * WITH CHECK quals also.
-	 */
-	if (with_check_quals == NIL)
-		with_check_quals = copyObject(quals);
-
-	/*
 	 * Row security quals always have the target table as varno 1, as no
 	 * joins are permitted in row security expressions. We must walk the
 	 * expression, updating any references to varno 1 to the varno
 	 * the table has in the outer query.
 	 *
 	 * We rewrite the expression in-place.
+	 *
+	 * We must have some quals at this point; the default-deny policy, if
+	 * nothing else.  Note that we might not have any WITH CHECK quals-
+	 * that's fine, as this might not be the resultRelation.
 	 */
+	Assert(quals != NIL);
+
 	ChangeVarNodes((Node *) quals, 1, rt_index, 0);
-	ChangeVarNodes((Node *) with_check_quals, 1, rt_index, 0);
+
+	if (with_check_quals != NIL)
+		ChangeVarNodes((Node *) with_check_quals, 1, rt_index, 0);
 
 	/*
 	 * If more than one security qual is returned, then they need to be
-	 * OR'ed together.
+	 * combined together.
 	 */
 	if (list_length(quals) > 1)
-		*qual_eval = makeBoolExpr(OR_EXPR, quals, -1);
+		*qual_eval = makeBoolExpr(boolop, quals, -1);
 	else
 		*qual_eval = (Expr*) linitial(quals);
 
 	/*
-	 * If more than one WITH CHECK qual is returned, then they need to
-	 * be OR'ed together.
+	 * Similairly, if more than one WITH CHECK qual is returned, then
+	 * they need to be combined together.
+	 *
+	 * with_check_quals is allowed to be NIL here since this might not be the
+	 * resultRelation (see above).
 	 */
 	if (list_length(with_check_quals) > 1)
-		*with_check_eval = makeBoolExpr(OR_EXPR, with_check_quals, -1);
-	else
+		*with_check_eval = makeBoolExpr(boolop, with_check_quals, -1);
+	else if (with_check_quals != NIL)
 		*with_check_eval = (Expr*) linitial(with_check_quals);
+	else
+		*with_check_eval = NULL;
 
 	return;
 }