On 14 January 2015 at 08:43, Dean Rasheed <dean.a.rash...@gmail.com> wrote:
> On 12 January 2015 at 14:24, Stephen Frost <sfr...@snowman.net> wrote:
>> Interesting, thanks for the work!  I had been suspicious that there was
>> an issue with the recursion handling.
>>
>
> So continuing to review the RLS code, I spotted the following in
> prepend_row_security_policies():
>
>     /*
>      * 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;
>
> which looked suspicious. I assume that's just a hang-up from an
> earlier attempt to prevent infinite recursion in RLS expansion, but
> actually it's wrong because in the case of an UPDATE to a security
> barrier view on top of a table with RLS enabled, the view's
> securityQuals will get added to the RTE first, and that shouldn't
> prevent the underlying table's RLS securityQuals from being added.
>
> AFAICT, it should be safe to just remove the above code. I can't see
> how prepend_row_security_policies() could end up getting called more
> than once for the same RTE.
>

Turns out it wasn't as simple as that. prepend_row_security_policies()
really could get called multiple times for the same RTE, because the
call to query_tree_walker() at the end of fireRIRrules() would descend
into the just-added quals again. The simplest fix seems to be to
process RLS in a separate loop at the end, so that it can have it's
own infinite recursion detection, which is different from that needed
for pre-existing security quals and with check options from security
barrier views. This approach simplifies things a bit, and ensures that
we only try to expand RLS once for each RTE.

> Also, I'm thinking that it would be better to refactor things a bit
> and have prepend_row_security_policies() just return the new
> securityQuals and withCheckOptions to add. Then fireRIRrules() would
> only have to recurse into the new quals being added, not the
> already-processed quals.
>

Turns out that refactoring actually became necessary in order to fix
this bug, but I think it makes things cleaner and more efficient.

Here's an updated patch with a new test for this bug. I've been
developing the fixes for these RLS issues as one big patch, but I
suppose it would be easy to split up, if that's preferred.

Regards,
Dean
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
new file mode 100644
index 9cbbcfb..34ddf41
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*************** inheritance_planner(PlannerInfo *root)
*** 790,795 ****
--- 790,796 ----
  {
  	Query	   *parse = root->parse;
  	int			parentRTindex = parse->resultRelation;
+ 	Bitmapset  *resultRTindexes = NULL;
  	List	   *final_rtable = NIL;
  	int			save_rel_array_size = 0;
  	RelOptInfo **save_rel_array = NULL;
*************** inheritance_planner(PlannerInfo *root)
*** 814,820 ****
--- 815,835 ----
  	 * (1) would result in a rangetable of length O(N^2) for N targets, with
  	 * at least O(N^3) work expended here; and (2) would greatly complicate
  	 * management of the rowMarks list.
+ 	 *
+ 	 * Note that any RTEs with security barrier quals will be turned into
+ 	 * subqueries during planning, and so we must create copies of them too,
+ 	 * except where they are target relations, which will each only be used
+ 	 * in a single plan.
  	 */
+ 	resultRTindexes = bms_add_member(resultRTindexes, parentRTindex);
+ 	foreach(lc, root->append_rel_list)
+ 	{
+ 		AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
+ 		if (appinfo->parent_relid == parentRTindex)
+ 			resultRTindexes = bms_add_member(resultRTindexes,
+ 											 appinfo->child_relid);
+ 	}
+ 
  	foreach(lc, root->append_rel_list)
  	{
  		AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
*************** inheritance_planner(PlannerInfo *root)
*** 885,905 ****
  			{
  				RangeTblEntry *rte = (RangeTblEntry *) lfirst(lr);
  
! 				if (rte->rtekind == RTE_SUBQUERY)
  				{
  					Index		newrti;
  
  					/*
  					 * The RTE can't contain any references to its own RT
! 					 * index, so we can save a few cycles by applying
! 					 * ChangeVarNodes before we append the RTE to the
! 					 * rangetable.
  					 */
  					newrti = list_length(subroot.parse->rtable) + 1;
  					ChangeVarNodes((Node *) subroot.parse, rti, newrti, 0);
  					ChangeVarNodes((Node *) subroot.rowMarks, rti, newrti, 0);
  					ChangeVarNodes((Node *) subroot.append_rel_list, rti, newrti, 0);
  					rte = copyObject(rte);
  					subroot.parse->rtable = lappend(subroot.parse->rtable,
  													rte);
  				}
--- 900,928 ----
  			{
  				RangeTblEntry *rte = (RangeTblEntry *) lfirst(lr);
  
! 				/*
! 				 * Copy subquery RTEs and RTEs with security barrier quals
! 				 * that will be turned into subqueries, except those that are
! 				 * target relations.
! 				 */
! 				if (rte->rtekind == RTE_SUBQUERY ||
! 					(rte->securityQuals != NIL &&
! 					 !bms_is_member(rti, resultRTindexes)))
  				{
  					Index		newrti;
  
  					/*
  					 * The RTE can't contain any references to its own RT
! 					 * index, except in the security barrier quals, so we can
! 					 * save a few cycles by applying ChangeVarNodes before we
! 					 * append the RTE to the rangetable.
  					 */
  					newrti = list_length(subroot.parse->rtable) + 1;
  					ChangeVarNodes((Node *) subroot.parse, rti, newrti, 0);
  					ChangeVarNodes((Node *) subroot.rowMarks, rti, newrti, 0);
  					ChangeVarNodes((Node *) subroot.append_rel_list, rti, newrti, 0);
  					rte = copyObject(rte);
+ 					ChangeVarNodes((Node *) rte->securityQuals, rti, newrti, 0);
  					subroot.parse->rtable = lappend(subroot.parse->rtable,
  													rte);
  				}
*************** preprocess_rowmarks(PlannerInfo *root)
*** 2254,2261 ****
  		newrc = makeNode(PlanRowMark);
  		newrc->rti = newrc->prti = i;
  		newrc->rowmarkId = ++(root->glob->lastRowMarkId);
! 		/* real tables support REFERENCE, anything else needs COPY */
  		if (rte->rtekind == RTE_RELATION &&
  			rte->relkind != RELKIND_FOREIGN_TABLE)
  			newrc->markType = ROW_MARK_REFERENCE;
  		else
--- 2277,2289 ----
  		newrc = makeNode(PlanRowMark);
  		newrc->rti = newrc->prti = i;
  		newrc->rowmarkId = ++(root->glob->lastRowMarkId);
! 		/*
! 		 * Real tables support REFERENCE, anything else needs COPY.  Since
! 		 * RTEs with security barrier quals will become subqueries, they also
! 		 * need COPY.
! 		 */
  		if (rte->rtekind == RTE_RELATION &&
+ 			rte->securityQuals == NIL &&
  			rte->relkind != RELKIND_FOREIGN_TABLE)
  			newrc->markType = ROW_MARK_REFERENCE;
  		else
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index b8e6e7a..149191e
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
*************** fireRIRrules(Query *parsetree, List *act
*** 1714,1764 ****
  				activeRIRs = list_delete_first(activeRIRs);
  			}
  		}
- 		/*
- 		 * If the RTE has row security quals, apply them and recurse into the
- 		 * securityQuals.
- 		 */
- 		if (prepend_row_security_policies(parsetree, rte, rt_index))
- 		{
- 			/*
- 			 * We applied security quals, check for infinite recursion and
- 			 * then expand any nested queries.
- 			 */
- 			if (list_member_oid(activeRIRs, RelationGetRelid(rel)))
- 					ereport(ERROR,
- 							(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- 							 errmsg("infinite recursion detected in policy for relation \"%s\"",
- 									RelationGetRelationName(rel))));
- 
- 			/*
- 			 * Make sure we check for recursion in either securityQuals or
- 			 * WITH CHECK quals.
- 			 */
- 			if (rte->securityQuals != NIL)
- 			{
- 				activeRIRs = lcons_oid(RelationGetRelid(rel), activeRIRs);
- 
- 				expression_tree_walker( (Node*) rte->securityQuals,
- 										fireRIRonSubLink, (void*)activeRIRs );
- 
- 				activeRIRs = list_delete_first(activeRIRs);
- 			}
- 
- 			if (parsetree->withCheckOptions != NIL)
- 			{
- 				WithCheckOption    *wco;
- 				List			   *quals = NIL;
- 
- 				wco = (WithCheckOption *) makeNode(WithCheckOption);
- 				quals = lcons(wco->qual, quals);
- 
- 				activeRIRs = lcons_oid(RelationGetRelid(rel), activeRIRs);
- 
- 				expression_tree_walker( (Node*) quals, fireRIRonSubLink,
- 									   (void*)activeRIRs);
- 			}
- 
- 		}
  
  		heap_close(rel, NoLock);
  	}
--- 1714,1719 ----
*************** fireRIRrules(Query *parsetree, List *act
*** 1780,1785 ****
--- 1735,1822 ----
  		query_tree_walker(parsetree, fireRIRonSubLink, (void *) activeRIRs,
  						  QTW_IGNORE_RC_SUBQUERIES);
  
+ 	/*
+ 	 * Apply any row level security policies.  We do this last because it
+ 	 * requires special recursion detection if the new quals have sublink
+ 	 * subqueries, and if we did it in the loop above query_tree_walker
+ 	 * would then recurse into those quals a second time.
+ 	 */
+ 	rt_index = 0;
+ 	foreach(lc, parsetree->rtable)
+ 	{
+ 		RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
+ 		Relation	rel;
+ 		List	   *securityQuals;
+ 		List	   *withCheckOptions;
+ 		bool		hasRowSecurity;
+ 		bool		hasSubLinks;
+ 
+ 		++rt_index;
+ 
+ 		/* Only normal relations can have RLS policies */
+ 		if (rte->rtekind != RTE_RELATION ||
+ 			rte->relkind != RELKIND_RELATION)
+ 			continue;
+ 
+ 		rel = heap_open(rte->relid, NoLock);
+ 
+ 		/*
+ 		 * Fetch any new security quals that must be applied to this RTE.
+ 		 */
+ 		get_row_security_policies(parsetree, rte, rt_index,
+ 								  &securityQuals, &withCheckOptions,
+ 								  &hasRowSecurity, &hasSubLinks);
+ 
+ 		if (securityQuals != NIL || withCheckOptions != NIL)
+ 		{
+ 			if (hasSubLinks)
+ 			{
+ 				/*
+ 				 * Recursively process the new quals, checking for infinite
+ 				 * recursion.
+ 				 */
+ 				if (list_member_oid(activeRIRs, RelationGetRelid(rel)))
+ 					ereport(ERROR,
+ 							(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ 							 errmsg("infinite recursion detected in policy for relation \"%s\"",
+ 									RelationGetRelationName(rel))));
+ 
+ 				activeRIRs = lcons_oid(RelationGetRelid(rel), activeRIRs);
+ 
+ 				expression_tree_walker( (Node*) securityQuals,
+ 										fireRIRonSubLink, (void*)activeRIRs );
+ 
+ 				expression_tree_walker( (Node*) withCheckOptions,
+ 										fireRIRonSubLink, (void*)activeRIRs );
+ 
+ 				activeRIRs = list_delete_first(activeRIRs);
+ 			}
+ 
+ 			/*
+ 			 * Add the new security quals to the start of the RTE's list so
+ 			 * that they get applied before any existing security quals (which
+ 			 * might have come from a user-written security barrier view, and
+ 			 * might contain malicious code).
+ 			 */
+ 			rte->securityQuals = list_concat(securityQuals,
+ 											 rte->securityQuals);
+ 
+ 			parsetree->withCheckOptions = list_concat(withCheckOptions,
+ 													  parsetree->withCheckOptions);
+ 		}
+ 
+ 		/*
+ 		 * Make sure the query is marked correctly if row level security
+ 		 * applies, or if the new quals had sublinks.
+ 		 */
+ 		if (hasRowSecurity)
+ 			parsetree->hasRowSecurity = true;
+ 		if (hasSubLinks)
+ 			parsetree->hasSubLinks = true;
+ 
+ 		heap_close(rel, NoLock);
+ 	}
+ 
  	return parsetree;
  }
  
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
new file mode 100644
index 35790a9..bb47429
*** a/src/backend/rewrite/rowsecurity.c
--- b/src/backend/rewrite/rowsecurity.c
***************
*** 57,63 ****
  
  static List *pull_row_security_policies(CmdType cmd, Relation relation,
  										Oid user_id);
! static void process_policies(List *policies, int rt_index,
  							 Expr **final_qual,
  							 Expr **final_with_check_qual,
  							 bool *hassublinks);
--- 57,63 ----
  
  static List *pull_row_security_policies(CmdType cmd, Relation relation,
  										Oid user_id);
! static void process_policies(Query* root, List *policies, int rt_index,
  							 Expr **final_qual,
  							 Expr **final_with_check_qual,
  							 bool *hassublinks);
*************** static bool check_role_for_policy(ArrayT
*** 72,87 ****
  row_security_policy_hook_type	row_security_policy_hook = 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.
   *
!  * 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.
   */
! bool
! prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
  {
  	Expr			   *rowsec_expr = NULL;
  	Expr			   *rowsec_with_check_expr = NULL;
--- 72,88 ----
  row_security_policy_hook_type	row_security_policy_hook = NULL;
  
  /*
!  * Get any row security quals and check quals that should be applied to the
!  * specified RTE.
   *
!  * 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.
   */
! 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;
*************** prepend_row_security_policies(Query* roo
*** 95,102 ****
  	Oid					user_id;
  	int					sec_context;
  	int					rls_status;
! 	bool				defaultDeny = true;
! 	bool				hassublinks = false;
  
  	/* This is just to get the security context */
  	GetUserIdAndSecContext(&user_id, &sec_context);
--- 96,108 ----
  	Oid					user_id;
  	int					sec_context;
  	int					rls_status;
! 	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);
*************** prepend_row_security_policies(Query* roo
*** 112,125 ****
  	if (rte->relid < FirstNormalObjectId
  		|| rte->relkind != RELKIND_RELATION
  		|| (sec_context & SECURITY_ROW_LEVEL_DISABLED))
! 		return false;
  
  	/* Determine the state of RLS for this, pass checkAsUser explicitly */
  	rls_status = check_enable_rls(rte->relid, rte->checkAsUser);
  
  	/* If there is no RLS on this table at all, nothing to do */
  	if (rls_status == RLS_NONE)
! 		return false;
  
  	/*
  	 * RLS_NONE_ENV means we are not doing any RLS now, but that may change
--- 118,131 ----
  	if (rte->relid < FirstNormalObjectId
  		|| rte->relkind != RELKIND_RELATION
  		|| (sec_context & SECURITY_ROW_LEVEL_DISABLED))
! 		return;
  
  	/* Determine the state of RLS for this, pass checkAsUser explicitly */
  	rls_status = check_enable_rls(rte->relid, rte->checkAsUser);
  
  	/* If there is no RLS on this table at all, nothing to do */
  	if (rls_status == RLS_NONE)
! 		return;
  
  	/*
  	 * RLS_NONE_ENV means we are not doing any RLS now, but that may change
*************** prepend_row_security_policies(Query* roo
*** 133,150 ****
  		 * be replanned if the environment changes (GUCs, role), but we
  		 * are not adding anything here.
  		 */
! 		root->hasRowSecurity = true;
  
! 		return false;
  	}
  
- 	/*
- 	 * 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);
  
--- 139,149 ----
  		 * be replanned if the environment changes (GUCs, role), but we
  		 * are not adding anything here.
  		 */
! 		*hasRowSecurity = true;
  
! 		return;
  	}
  
  	/* Grab the built-in policies which should be applied to this relation. */
  	rel = heap_open(rte->relid, NoLock);
  
*************** prepend_row_security_policies(Query* roo
*** 166,173 ****
  		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);
  
  	/*
  	 * Also, allow extensions to add their own policies.
--- 165,172 ----
  		defaultDeny = true;
  
  	/* Now that we have our policies, build the expressions from them. */
! 	process_policies(root, rowsec_policies, rt_index, &rowsec_expr,
! 					 &rowsec_with_check_expr, hasSubLinks);
  
  	/*
  	 * Also, allow extensions to add their own policies.
*************** prepend_row_security_policies(Query* roo
*** 190,203 ****
  	 * enabled on the table, then we will ignore the internally-generated
  	 * default-deny policy and use only the policies returned by the
  	 * extension.
  	 */
  	if (row_security_policy_hook)
  	{
  		hook_policies = (*row_security_policy_hook)(root->commandType, rel);
  
  		/* Build the expression from any policies returned. */
! 		process_policies(hook_policies, rt_index, &hook_expr,
! 						 &hook_with_check_expr, &hassublinks);
  	}
  
  	/*
--- 189,205 ----
  	 * enabled on the table, then we will ignore the internally-generated
  	 * default-deny policy and use only the policies returned by the
  	 * extension.
+ 	 *
+ 	 * Any extension policies are applied after any internally-generated
+ 	 * policies, so that extensions cannot bypass built-in RLS.
  	 */
  	if (row_security_policy_hook)
  	{
  		hook_policies = (*row_security_policy_hook)(root->commandType, rel);
  
  		/* Build the expression from any policies returned. */
! 		process_policies(root, hook_policies, rt_index, &hook_expr,
! 						 &hook_with_check_expr, hasSubLinks);
  	}
  
  	/*
*************** prepend_row_security_policies(Query* roo
*** 229,235 ****
  			wco->viewname = RelationGetRelationName(rel);
  			wco->qual = (Node *) rowsec_with_check_expr;
  			wco->cascaded = false;
! 			root->withCheckOptions = lcons(wco, root->withCheckOptions);
  		}
  
  		/*
--- 231,237 ----
  			wco->viewname = RelationGetRelationName(rel);
  			wco->qual = (Node *) rowsec_with_check_expr;
  			wco->cascaded = false;
! 			*withCheckOptions = lappend(*withCheckOptions, wco);
  		}
  
  		/*
*************** prepend_row_security_policies(Query* roo
*** 243,249 ****
  			wco->viewname = RelationGetRelationName(rel);
  			wco->qual = (Node *) hook_with_check_expr;
  			wco->cascaded = false;
! 			root->withCheckOptions = lcons(wco, root->withCheckOptions);
  		}
  	}
  
--- 245,251 ----
  			wco->viewname = RelationGetRelationName(rel);
  			wco->qual = (Node *) hook_with_check_expr;
  			wco->cascaded = false;
! 			*withCheckOptions = lappend(*withCheckOptions, wco);
  		}
  	}
  
*************** prepend_row_security_policies(Query* roo
*** 253,283 ****
  		|| root->commandType == CMD_DELETE)
  	{
  		if (rowsec_expr)
! 			rte->securityQuals = lcons(rowsec_expr, rte->securityQuals);
  
  		if (hook_expr)
! 			rte->securityQuals = lcons(hook_expr,
! 									   rte->securityQuals);
  	}
  
  	heap_close(rel, NoLock);
  
  	/*
! 	 * Mark this query as having row security, so plancache can invalidate
! 	 * it when necessary (eg: role changes)
! 	 */
! 	root->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.
  	 */
! 	if (hassublinks)
! 		root->hasSubLinks = hassublinks;
! 
! 	/* If we got this far, we must have added quals */
! 	return true;
  }
  
  /*
--- 255,273 ----
  		|| root->commandType == CMD_DELETE)
  	{
  		if (rowsec_expr)
! 			*securityQuals = lappend(*securityQuals, rowsec_expr);
  
  		if (hook_expr)
! 			*securityQuals = lappend(*securityQuals, hook_expr);
  	}
  
  	heap_close(rel, NoLock);
  
  	/*
! 	 * The query has row security, so plancache must invalidate it when
! 	 * necessary (eg: role changes)
  	 */
! 	*hasRowSecurity = true;
  }
  
  /*
*************** pull_row_security_policies(CmdType cmd,
*** 383,389 ****
   * 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)
  {
  	ListCell		   *item;
--- 373,379 ----
   * qual_eval, with_check_eval, and hassublinks are output variables
   */
  static void
! process_policies(Query* root, List *policies, int rt_index, Expr **qual_eval,
  				 Expr **with_check_eval, bool *hassublinks)
  {
  	ListCell		   *item;
*************** process_policies(List *policies, int rt_
*** 392,398 ****
  
  	/*
  	 * Extract the USING and WITH CHECK quals from each of the policies
! 	 * and add them to our lists.
  	 */
  	foreach(item, policies)
  	{
--- 382,389 ----
  
  	/*
  	 * Extract the USING and WITH CHECK quals from each of the policies
! 	 * and add them to our lists.  We only want WITH CHECK quals if this
! 	 * RTE is the query's result relation.
  	 */
  	foreach(item, policies)
  	{
*************** process_policies(List *policies, int rt_
*** 401,407 ****
  		if (policy->qual != NULL)
  			quals = lcons(copyObject(policy->qual), quals);
  
! 		if (policy->with_check_qual != NULL)
  			with_check_quals = lcons(copyObject(policy->with_check_qual),
  									 with_check_quals);
  
--- 392,399 ----
  		if (policy->qual != NULL)
  			quals = lcons(copyObject(policy->qual), quals);
  
! 		if (policy->with_check_qual != NULL &&
! 			rt_index == root->resultRelation)
  			with_check_quals = lcons(copyObject(policy->with_check_qual),
  									 with_check_quals);
  
*************** process_policies(List *policies, int rt_
*** 421,427 ****
  	 * 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);
  
  	/*
--- 413,419 ----
  	 * If we end up with only USING quals, then use those as
  	 * WITH CHECK quals also.
  	 */
! 	if (with_check_quals == NIL && rt_index == root->resultRelation)
  		with_check_quals = copyObject(quals);
  
  	/*
*************** process_policies(List *policies, int rt_
*** 450,457 ****
  	 */
  	if (list_length(with_check_quals) > 1)
  		*with_check_eval = makeBoolExpr(OR_EXPR, with_check_quals, -1);
! 	else
  		*with_check_eval = (Expr*) linitial(with_check_quals);
  
  	return;
  }
--- 442,451 ----
  	 */
  	if (list_length(with_check_quals) > 1)
  		*with_check_eval = makeBoolExpr(OR_EXPR, with_check_quals, -1);
! 	else if (with_check_quals != NIL)
  		*with_check_eval = (Expr*) linitial(with_check_quals);
+ 	else
+ 		*with_check_eval = NULL;
  
  	return;
  }
diff --git a/src/include/rewrite/rowsecurity.h b/src/include/rewrite/rowsecurity.h
new file mode 100644
index aa1b45b..43f528d
*** a/src/include/rewrite/rowsecurity.h
--- b/src/include/rewrite/rowsecurity.h
*************** typedef List *(*row_security_policy_hook
*** 73,80 ****
  
  extern PGDLLIMPORT row_security_policy_hook_type row_security_policy_hook;
  
! extern bool prepend_row_security_policies(Query* root, RangeTblEntry* rte,
! 									   int rt_index);
  
  extern int check_enable_rls(Oid relid, Oid checkAsUser);
  
--- 73,81 ----
  
  extern PGDLLIMPORT row_security_policy_hook_type row_security_policy_hook;
  
! extern void get_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index,
! 						  List **securityQuals, List **withCheckOptions,
! 						  bool *hasRowSecurity, bool *hasSubLinks);
  
  extern int check_enable_rls(Oid relid, Oid checkAsUser);
  
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
new file mode 100644
index 430da55..81f4095
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*************** NOTICE:  f_leak => yyyyyy
*** 1111,1116 ****
--- 1111,1185 ----
   302 | 2 | yyyyyy      | (2,yyyyyy)
  (5 rows)
  
+ -- update with from clause self join
+ EXPLAIN (COSTS OFF) UPDATE t1 t1_1 SET b = t1_2.b FROM t1 t1_2
+ WHERE t1_1.a = 4 AND t1_2.a = t1_1.a AND t1_2.b = t1_1.b
+ AND f_leak(t1_1.b) AND f_leak(t1_2.b) RETURNING *, t1_1, t1_2;
+                           QUERY PLAN                           
+ ---------------------------------------------------------------
+  Update on t1 t1_1_3
+    ->  Nested Loop
+          Join Filter: (t1_1.b = t1_2.b)
+          ->  Subquery Scan on t1_1
+                Filter: f_leak(t1_1.b)
+                ->  Seq Scan on t1 t1_1_4
+                      Filter: ((a = 4) AND ((a % 2) = 0))
+          ->  Subquery Scan on t1_2
+                Filter: f_leak(t1_2.b)
+                ->  Append
+                      ->  Seq Scan on t1 t1_2_3
+                            Filter: ((a = 4) AND ((a % 2) = 0))
+                      ->  Seq Scan on t2 t1_2_4
+                            Filter: ((a = 4) AND ((a % 2) = 0))
+                      ->  Seq Scan on t3 t1_2_5
+                            Filter: ((a = 4) AND ((a % 2) = 0))
+    ->  Nested Loop
+          Join Filter: (t1_1_1.b = t1_2_1.b)
+          ->  Subquery Scan on t1_1_1
+                Filter: f_leak(t1_1_1.b)
+                ->  Seq Scan on t2 t1_1_5
+                      Filter: ((a = 4) AND ((a % 2) = 0))
+          ->  Subquery Scan on t1_2_1
+                Filter: f_leak(t1_2_1.b)
+                ->  Append
+                      ->  Seq Scan on t1 t1_2_6
+                            Filter: ((a = 4) AND ((a % 2) = 0))
+                      ->  Seq Scan on t2 t1_2_7
+                            Filter: ((a = 4) AND ((a % 2) = 0))
+                      ->  Seq Scan on t3 t1_2_8
+                            Filter: ((a = 4) AND ((a % 2) = 0))
+    ->  Nested Loop
+          Join Filter: (t1_1_2.b = t1_2_2.b)
+          ->  Subquery Scan on t1_1_2
+                Filter: f_leak(t1_1_2.b)
+                ->  Seq Scan on t3 t1_1_6
+                      Filter: ((a = 4) AND ((a % 2) = 0))
+          ->  Subquery Scan on t1_2_2
+                Filter: f_leak(t1_2_2.b)
+                ->  Append
+                      ->  Seq Scan on t1 t1_2_9
+                            Filter: ((a = 4) AND ((a % 2) = 0))
+                      ->  Seq Scan on t2 t1_2_10
+                            Filter: ((a = 4) AND ((a % 2) = 0))
+                      ->  Seq Scan on t3 t1_2_11
+                            Filter: ((a = 4) AND ((a % 2) = 0))
+ (46 rows)
+ 
+ UPDATE t1 t1_1 SET b = t1_2.b FROM t1 t1_2
+ WHERE t1_1.a = 4 AND t1_2.a = t1_1.a AND t1_2.b = t1_1.b
+ AND f_leak(t1_1.b) AND f_leak(t1_2.b) RETURNING *, t1_1, t1_2;
+ NOTICE:  f_leak => dddddd_updt
+ NOTICE:  f_leak => dddddd_updt
+ NOTICE:  f_leak => defdef
+ NOTICE:  f_leak => defdef
+ NOTICE:  f_leak => dddddd_updt
+ NOTICE:  f_leak => defdef
+  a |      b      | a |      b      |      t1_1       |      t1_2       
+ ---+-------------+---+-------------+-----------------+-----------------
+  4 | dddddd_updt | 4 | dddddd_updt | (4,dddddd_updt) | (4,dddddd_updt)
+  4 | defdef      | 4 | defdef      | (4,defdef)      | (4,defdef)
+ (2 rows)
+ 
  RESET SESSION AUTHORIZATION;
  SET row_security TO OFF;
  SELECT * FROM t1;
*************** NOTICE:  f_leak => yyyyyy
*** 1180,1185 ****
--- 1249,1349 ----
  (3 rows)
  
  --
+ -- S.b. view on top of Row-level security
+ --
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ CREATE TABLE b1 (a int, b text);
+ INSERT INTO b1 (SELECT x, md5(x::text) FROM generate_series(-10,10) x);
+ CREATE POLICY p1 ON b1 USING (a % 2 = 0);
+ ALTER TABLE b1 ENABLE ROW LEVEL SECURITY;
+ GRANT ALL ON b1 TO rls_regress_user1;
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ CREATE VIEW bv1 WITH (security_barrier) AS SELECT * FROM b1 WHERE a > 0 WITH CHECK OPTION;
+ GRANT ALL ON bv1 TO rls_regress_user2;
+ SET SESSION AUTHORIZATION rls_regress_user2;
+ EXPLAIN (COSTS OFF) SELECT * FROM bv1 WHERE f_leak(b);
+                  QUERY PLAN                  
+ ---------------------------------------------
+  Subquery Scan on bv1
+    Filter: f_leak(bv1.b)
+    ->  Seq Scan on b1
+          Filter: ((a > 0) AND ((a % 2) = 0))
+ (4 rows)
+ 
+ SELECT * FROM bv1 WHERE f_leak(b);
+ NOTICE:  f_leak => c81e728d9d4c2f636f067f89cc14862c
+ NOTICE:  f_leak => a87ff679a2f3e71d9181a67b7542122c
+ NOTICE:  f_leak => 1679091c5a880faf6fb5e6087eb1b2dc
+ NOTICE:  f_leak => c9f0f895fb98ab9159f51fd0297e236d
+ NOTICE:  f_leak => d3d9446802a44259755d38e6d163e820
+  a  |                b                 
+ ----+----------------------------------
+   2 | c81e728d9d4c2f636f067f89cc14862c
+   4 | a87ff679a2f3e71d9181a67b7542122c
+   6 | 1679091c5a880faf6fb5e6087eb1b2dc
+   8 | c9f0f895fb98ab9159f51fd0297e236d
+  10 | d3d9446802a44259755d38e6d163e820
+ (5 rows)
+ 
+ INSERT INTO bv1 VALUES (-1, 'xxx'); -- should fail view WCO
+ ERROR:  new row violates WITH CHECK OPTION for "b1"
+ DETAIL:  Failing row contains (-1, xxx).
+ INSERT INTO bv1 VALUES (11, 'xxx'); -- should fail RLS check
+ ERROR:  new row violates WITH CHECK OPTION for "b1"
+ DETAIL:  Failing row contains (11, xxx).
+ INSERT INTO bv1 VALUES (12, 'xxx'); -- ok
+ EXPLAIN (COSTS OFF) UPDATE bv1 SET b = 'yyy' WHERE a = 4 AND f_leak(b);
+                           QUERY PLAN                           
+ ---------------------------------------------------------------
+  Update on b1 b1_1
+    ->  Subquery Scan on b1
+          Filter: f_leak(b1.b)
+          ->  Seq Scan on b1 b1_2
+                Filter: ((a > 0) AND (a = 4) AND ((a % 2) = 0))
+ (5 rows)
+ 
+ UPDATE bv1 SET b = 'yyy' WHERE a = 4 AND f_leak(b);
+ NOTICE:  f_leak => a87ff679a2f3e71d9181a67b7542122c
+ EXPLAIN (COSTS OFF) DELETE FROM bv1 WHERE a = 6 AND f_leak(b);
+                           QUERY PLAN                           
+ ---------------------------------------------------------------
+  Delete on b1 b1_1
+    ->  Subquery Scan on b1
+          Filter: f_leak(b1.b)
+          ->  Seq Scan on b1 b1_2
+                Filter: ((a > 0) AND (a = 6) AND ((a % 2) = 0))
+ (5 rows)
+ 
+ DELETE FROM bv1 WHERE a = 6 AND f_leak(b);
+ NOTICE:  f_leak => 1679091c5a880faf6fb5e6087eb1b2dc
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ SELECT * FROM b1;
+   a  |                b                 
+ -----+----------------------------------
+  -10 | 1b0fd9efa5279c4203b7c70233f86dbf
+   -9 | 252e691406782824eec43d7eadc3d256
+   -8 | a8d2ec85eaf98407310b72eb73dda247
+   -7 | 74687a12d3915d3c4d83f1af7b3683d5
+   -6 | 596a3d04481816330f07e4f97510c28f
+   -5 | 47c1b025fa18ea96c33fbb6718688c0f
+   -4 | 0267aaf632e87a63288a08331f22c7c3
+   -3 | b3149ecea4628efd23d2f86e5a723472
+   -2 | 5d7b9adcbe1c629ec722529dd12e5129
+   -1 | 6bb61e3b7bce0931da574d19d1d82c88
+    0 | cfcd208495d565ef66e7dff9f98764da
+    1 | c4ca4238a0b923820dcc509a6f75849b
+    2 | c81e728d9d4c2f636f067f89cc14862c
+    3 | eccbc87e4b5ce2fe28308fd9f2a7baf3
+    5 | e4da3b7fbbce2345d7772b0674a318d5
+    7 | 8f14e45fceea167a5a36dedd4bea2543
+    8 | c9f0f895fb98ab9159f51fd0297e236d
+    9 | 45c48cce2e2d7fbdea1afc51c7c6ad26
+   10 | d3d9446802a44259755d38e6d163e820
+   12 | xxx
+    4 | yyy
+ (21 rows)
+ 
+ --
  -- ROLE/GROUP
  --
  SET SESSION AUTHORIZATION rls_regress_user0;
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
new file mode 100644
index ee28a2c..49b3201
*** a/src/test/regress/sql/rowsecurity.sql
--- b/src/test/regress/sql/rowsecurity.sql
*************** UPDATE only t1 SET b = b WHERE f_leak(b)
*** 423,428 ****
--- 423,437 ----
  UPDATE t1 SET b = b WHERE f_leak(b) RETURNING *;
  UPDATE t1 SET b = b WHERE f_leak(b) RETURNING oid, *, t1;
  
+ -- update with from clause self join
+ EXPLAIN (COSTS OFF) UPDATE t1 t1_1 SET b = t1_2.b FROM t1 t1_2
+ WHERE t1_1.a = 4 AND t1_2.a = t1_1.a AND t1_2.b = t1_1.b
+ AND f_leak(t1_1.b) AND f_leak(t1_2.b) RETURNING *, t1_1, t1_2;
+ 
+ UPDATE t1 t1_1 SET b = t1_2.b FROM t1 t1_2
+ WHERE t1_1.a = 4 AND t1_2.a = t1_1.a AND t1_2.b = t1_1.b
+ AND f_leak(t1_1.b) AND f_leak(t1_2.b) RETURNING *, t1_1, t1_2;
+ 
  RESET SESSION AUTHORIZATION;
  SET row_security TO OFF;
  SELECT * FROM t1;
*************** DELETE FROM only t1 WHERE f_leak(b) RETU
*** 436,441 ****
--- 445,483 ----
  DELETE FROM t1 WHERE f_leak(b) RETURNING oid, *, t1;
  
  --
+ -- S.b. view on top of Row-level security
+ --
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ CREATE TABLE b1 (a int, b text);
+ INSERT INTO b1 (SELECT x, md5(x::text) FROM generate_series(-10,10) x);
+ 
+ CREATE POLICY p1 ON b1 USING (a % 2 = 0);
+ ALTER TABLE b1 ENABLE ROW LEVEL SECURITY;
+ GRANT ALL ON b1 TO rls_regress_user1;
+ 
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ CREATE VIEW bv1 WITH (security_barrier) AS SELECT * FROM b1 WHERE a > 0 WITH CHECK OPTION;
+ GRANT ALL ON bv1 TO rls_regress_user2;
+ 
+ SET SESSION AUTHORIZATION rls_regress_user2;
+ 
+ EXPLAIN (COSTS OFF) SELECT * FROM bv1 WHERE f_leak(b);
+ SELECT * FROM bv1 WHERE f_leak(b);
+ 
+ INSERT INTO bv1 VALUES (-1, 'xxx'); -- should fail view WCO
+ INSERT INTO bv1 VALUES (11, 'xxx'); -- should fail RLS check
+ INSERT INTO bv1 VALUES (12, 'xxx'); -- ok
+ 
+ EXPLAIN (COSTS OFF) UPDATE bv1 SET b = 'yyy' WHERE a = 4 AND f_leak(b);
+ UPDATE bv1 SET b = 'yyy' WHERE a = 4 AND f_leak(b);
+ 
+ EXPLAIN (COSTS OFF) DELETE FROM bv1 WHERE a = 6 AND f_leak(b);
+ DELETE FROM bv1 WHERE a = 6 AND f_leak(b);
+ 
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ SELECT * FROM b1;
+ 
+ --
  -- ROLE/GROUP
  --
  SET SESSION AUTHORIZATION rls_regress_user0;
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to