Row security violation error is misleading

Started by Craig Ringeralmost 11 years ago23 messages
#1Craig Ringer
craig@2ndquadrant.com

When attempting to insert a row that violates a row security policy that
applies to writes, the error message emitted references WITH CHECK OPTION,
even though (as far as the user knows) there's no such thing involved.
If you understand the internals you'd know that row security re-uses the
same logic as WITH CHECK OPTION, but it's going to be confusing for users.

postgres=> INSERT INTO clients (account_name, account_manager) VALUES
('peters', 'peter'), ('johannas', 'johanna');
ERROR: 44000: new row violates WITH CHECK OPTION for "clients"
DETAIL: Failing row contains (7, johannas, johanna).
LOCATION: ExecWithCheckOptions, execMain.c:1683

... yet "clients" is a table, not a view, and cannot have a WITH CHECK
OPTION clause.

There is no reference to the policy being violated or to the fact that it's
row security involved.

I think this is going to be very confusing for users. I was expecting to
see something more like:

ERROR: 44000: new row in table 'clients' violates row level security
policy 'just_own_clients'

Re-using the SQLSTATE 44000 is a bit iffy too. We should probably define
something to differentiate this, like:

44P01 ROW SECURITY WRITE POLICY VIOLATION

(I've finally found some time to try to review the user-facing side of the
row security patch as-committed).

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

#2Stephen Frost
sfrost@snowman.net
In reply to: Craig Ringer (#1)
Re: Row security violation error is misleading

Craig,

* Craig Ringer (craig@2ndquadrant.com) wrote:

When attempting to insert a row that violates a row security policy that
applies to writes, the error message emitted references WITH CHECK OPTION,
even though (as far as the user knows) there's no such thing involved.
If you understand the internals you'd know that row security re-uses the
same logic as WITH CHECK OPTION, but it's going to be confusing for users.

Agreed and we actually have a patch from Dean already to address this,
it's just been waiting on me (with a couple of other ones). It'd
certainly be great if you have time to take a look at those, though,
generally speaking, I feel prety happy about where those are and believe
they really just need to be reviewed/tested and maybe a bit of word-
smithing around the docs.

Thanks!

Stephen

#3Peter Geoghegan
pg@heroku.com
In reply to: Craig Ringer (#1)
Re: Row security violation error is misleading

On Tue, Apr 7, 2015 at 5:11 AM, Craig Ringer <craig@2ndquadrant.com> wrote:

postgres=> INSERT INTO clients (account_name, account_manager) VALUES
('peters', 'peter'), ('johannas', 'johanna');
ERROR: 44000: new row violates WITH CHECK OPTION for "clients"
DETAIL: Failing row contains (7, johannas, johanna).
LOCATION: ExecWithCheckOptions, execMain.c:1683

... yet "clients" is a table, not a view, and cannot have a WITH CHECK
OPTION clause.

There is no reference to the policy being violated or to the fact that it's
row security involved.

FWIW, I also think that this is very confusing.

--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Craig Ringer (#1)
Re: Row security violation error is misleading

On 7 April 2015 at 13:11, Craig Ringer <craig@2ndquadrant.com> wrote:

When attempting to insert a row that violates a row security policy that
applies to writes, the error message emitted references WITH CHECK OPTION,
even though (as far as the user knows) there's no such thing involved.
If you understand the internals you'd know that row security re-uses the
same logic as WITH CHECK OPTION, but it's going to be confusing for users.

postgres=> INSERT INTO clients (account_name, account_manager) VALUES
('peters', 'peter'), ('johannas', 'johanna');
ERROR: 44000: new row violates WITH CHECK OPTION for "clients"
DETAIL: Failing row contains (7, johannas, johanna).
LOCATION: ExecWithCheckOptions, execMain.c:1683

... yet "clients" is a table, not a view, and cannot have a WITH CHECK
OPTION clause.

There is no reference to the policy being violated or to the fact that it's
row security involved.

I think this is going to be very confusing for users. I was expecting to see
something more like:

ERROR: 44000: new row in table 'clients' violates row level security policy
'just_own_clients'

Yes, I agree - that's a bit confusing.

Note that it doesn't make sense to ask which RLS policy was violated.
There is a default deny policy in place, and each defined policy's
quals are combined using OR, so when there are multiple policies this
error indicates that none of the policies passed (in a sense, they
were all violated).

Re-using the SQLSTATE 44000 is a bit iffy too. We should probably define
something to differentiate this, like:

44P01 ROW SECURITY WRITE POLICY VIOLATION

Yes, that sounds sensible.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Stephen Frost (#2)
1 attachment(s)
Re: Row security violation error is misleading

On 7 April 2015 at 16:21, Stephen Frost <sfrost@snowman.net> wrote:

Agreed and we actually have a patch from Dean already to address this,
it's just been waiting on me (with a couple of other ones). It'd
certainly be great if you have time to take a look at those, though,
generally speaking, I feel prety happy about where those are and believe
they really just need to be reviewed/tested and maybe a bit of word-
smithing around the docs.

The first of those patches [1]/messages/by-id/CAEZATCVoaNiy5qLGda4K-nmP7GbJJgpVGucBAtA1ckVm-uWvgg@mail.gmail.com has bit-rotted somewhat, due to the
recent changes to the way rowmarks are handled, so here's an updated
version. It wasn't a trivial merge, because commit
cb1ca4d800621dcae67ca6c799006de99fa4f0a5 made a change to
ExecBuildAuxRowMark() without a matching change to
preprocess_targetlist(), and one of the new RLS-with-inheritance tests
fell over that.

This is not a complete review of RLS, but it does fix a number of issues:

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

2). In prepend_row_security_policies(), I think it is better to have
any table RLS policies applied before any hook policies, so that a
hook cannot be used to bypass built-in RLS.

3). 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 ...".

4). The RLS expansion code in fireRIRrules() was handling RLS in the
main loop through the rtable. This 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 didn'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 most easily 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.

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

6). 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. WCOs are only needed on the result relation.

The second patch [2]/messages/by-id/CAEZATCVz1u3dyjdzXN3F26rxks2BYXDz--_2rTZfRhuU0zfWSw@mail.gmail.com is the one that is actually relevant to this
thread. This patch is primarily to apply the RLS checks earlier,
before an update is attempted, more like a regular permissions check.
This adds a new enum to classify the kinds of WCO, a side benefit of
which is that it allows different error messages when RLS checks are
violated, as opposed to WITH CHECK OPTIONs on views.

I actually re-used the sql status code 42501 -
ERRCODE_INSUFFICIENT_PRIVILEGE for a RLS check failure because of the
parallel with permissions checks, but I quite like Craig's idea of
inventing a new status code for this, so that it can be more easily
distinguished from a lack of GRANTed privileges.

There's another side benefit to this patch, which is that the new enum
could be extended to include a new kind of WCO for a check of the
USING quals of a RLS UPDATE policy on the update path of an INSERT..ON
CONFLICT UPDATE. As I pointed out on that thread, I think these quals
need to be treated differently from the WITH CHECK quals of a RLS
UPDATE policy, since they ought to apply to different tuples.
Therefore classifying the WCOs by command type is insufficient to
distinguish the 2 cases.

Regards,
Dean

[1]: /messages/by-id/CAEZATCVoaNiy5qLGda4K-nmP7GbJJgpVGucBAtA1ckVm-uWvgg@mail.gmail.com

[2]: /messages/by-id/CAEZATCVz1u3dyjdzXN3F26rxks2BYXDz--_2rTZfRhuU0zfWSw@mail.gmail.com

Attachments:

rls.v6.patchtext/x-diff; charset=US-ASCII; name=rls.v6.patchDownload
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
new file mode 100644
index 876a87f..ea4d4c5
*** 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;
  	int			nominalRelation = -1;
  	List	   *final_rtable = NIL;
  	int			save_rel_array_size = 0;
*************** inheritance_planner(PlannerInfo *root)
*** 815,821 ****
--- 816,836 ----
  	 * (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)
*** 886,906 ****
  			{
  				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);
  				}
--- 901,929 ----
  			{
  				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);
  				}
*************** select_rowmark_type(RangeTblEntry *rte,
*** 2283,2289 ****
  		switch (strength)
  		{
  			case LCS_NONE:
! 				/* don't need tuple lock, only ability to re-fetch the row */
  				return ROW_MARK_REFERENCE;
  				break;
  			case LCS_FORKEYSHARE:
--- 2306,2324 ----
  		switch (strength)
  		{
  			case LCS_NONE:
! 				/*
! 				 * We don't need a tuple lock, only the ability to re-fetch
! 				 * the row.  Regular tables support ROW_MARK_REFERENCE, but if
! 				 * this RTE has security barrier quals, it will be turned into
! 				 * a subquery during planning, so use ROW_MARK_COPY.
! 				 *
! 				 * This is only necessary for LCS_NONE, since real tuple locks
! 				 * on an RTE with security barrier quals are supported by
! 				 * pushing the lock down into the subquery --- see
! 				 * expand_security_qual.
! 				 */
! 				if (rte->securityQuals != NIL)
! 					return ROW_MARK_COPY;
  				return ROW_MARK_REFERENCE;
  				break;
  			case LCS_FORKEYSHARE:
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
new file mode 100644
index 08e7c44..779befd
*** a/src/backend/optimizer/prep/preptlist.c
--- b/src/backend/optimizer/prep/preptlist.c
*************** preprocess_targetlist(PlannerInfo *root,
*** 107,129 ****
  								  pstrdup(resname),
  								  true);
  			tlist = lappend(tlist, tle);
- 
- 			/* if parent of inheritance tree, need the tableoid too */
- 			if (rc->isParent)
- 			{
- 				var = makeVar(rc->rti,
- 							  TableOidAttributeNumber,
- 							  OIDOID,
- 							  -1,
- 							  InvalidOid,
- 							  0);
- 				snprintf(resname, sizeof(resname), "tableoid%u", rc->rowmarkId);
- 				tle = makeTargetEntry((Expr *) var,
- 									  list_length(tlist) + 1,
- 									  pstrdup(resname),
- 									  true);
- 				tlist = lappend(tlist, tle);
- 			}
  		}
  		if (rc->allMarkTypes & (1 << ROW_MARK_COPY))
  		{
--- 107,112 ----
*************** preprocess_targetlist(PlannerInfo *root,
*** 136,141 ****
--- 119,141 ----
  			tle = makeTargetEntry((Expr *) var,
  								  list_length(tlist) + 1,
  								  pstrdup(resname),
+ 								  true);
+ 			tlist = lappend(tlist, tle);
+ 		}
+ 
+ 		/* If parent of inheritance tree, need to fetch the tableoid */
+ 		if (rc->isParent)
+ 		{
+ 			var = makeVar(rc->rti,
+ 						  TableOidAttributeNumber,
+ 						  OIDOID,
+ 						  -1,
+ 						  InvalidOid,
+ 						  0);
+ 			snprintf(resname, sizeof(resname), "tableoid%u", rc->rowmarkId);
+ 			tle = makeTargetEntry((Expr *) var,
+ 								  list_length(tlist) + 1,
+ 								  pstrdup(resname),
  								  true);
  			tlist = lappend(tlist, tle);
  		}
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index 9d2c280..08ec13c
*** 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 7669130..73a5794
*** a/src/backend/rewrite/rowsecurity.c
--- b/src/backend/rewrite/rowsecurity.c
***************
*** 58,64 ****
  
  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);
--- 58,64 ----
  
  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
*** 73,88 ****
  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;
--- 73,89 ----
  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
*** 96,103 ****
  	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);
--- 97,109 ----
  	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
*** 113,126 ****
  	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, false);
  
  	/* 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
--- 119,132 ----
  	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, false);
  
  	/* 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
*** 134,151 ****
  		 * 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);
  
--- 140,150 ----
  		 * 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
*** 167,174 ****
  		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.
--- 166,173 ----
  		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
*** 191,204 ****
  	 * 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);
  	}
  
  	/*
--- 190,206 ----
  	 * 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
*** 230,236 ****
  			wco->viewname = RelationGetRelationName(rel);
  			wco->qual = (Node *) rowsec_with_check_expr;
  			wco->cascaded = false;
! 			root->withCheckOptions = lcons(wco, root->withCheckOptions);
  		}
  
  		/*
--- 232,238 ----
  			wco->viewname = RelationGetRelationName(rel);
  			wco->qual = (Node *) rowsec_with_check_expr;
  			wco->cascaded = false;
! 			*withCheckOptions = lappend(*withCheckOptions, wco);
  		}
  
  		/*
*************** prepend_row_security_policies(Query* roo
*** 244,250 ****
  			wco->viewname = RelationGetRelationName(rel);
  			wco->qual = (Node *) hook_with_check_expr;
  			wco->cascaded = false;
! 			root->withCheckOptions = lcons(wco, root->withCheckOptions);
  		}
  	}
  
--- 246,252 ----
  			wco->viewname = RelationGetRelationName(rel);
  			wco->qual = (Node *) hook_with_check_expr;
  			wco->cascaded = false;
! 			*withCheckOptions = lappend(*withCheckOptions, wco);
  		}
  	}
  
*************** prepend_row_security_policies(Query* roo
*** 254,264 ****
  		|| 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);
--- 256,265 ----
  		|| root->commandType == CMD_DELETE)
  	{
  		if (rowsec_expr)
! 			*securityQuals = lappend(*securityQuals, rowsec_expr);
  
  		if (hook_expr)
! 			*securityQuals = lappend(*securityQuals, hook_expr);
  	}
  
  	heap_close(rel, NoLock);
*************** prepend_row_security_policies(Query* roo
*** 267,283 ****
  	 * 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.
! 	 */
! 	root->hasSubLinks |= hassublinks;
  
! 	/* If we got this far, we must have added quals */
! 	return true;
  }
  
  /*
--- 268,276 ----
  	 * Mark this query as having row security, so plancache can invalidate
  	 * it when necessary (eg: role changes)
  	 */
! 	*hasRowSecurity = true;
  
! 	return;
  }
  
  /*
*************** 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;
--- 376,382 ----
   * 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)
  	{
--- 385,392 ----
  
  	/*
  	 * 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);
  
--- 395,402 ----
  		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);
  
  	/*
--- 416,422 ----
  	 * 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;
  }
--- 445,454 ----
  	 */
  	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 8d19bfd..ac72427
*** a/src/include/rewrite/rowsecurity.h
--- b/src/include/rewrite/rowsecurity.h
*************** typedef List *(*row_security_policy_hook
*** 39,45 ****
  
  extern PGDLLIMPORT row_security_policy_hook_type row_security_policy_hook;
  
! extern bool prepend_row_security_policies(Query* root, RangeTblEntry* rte,
! 									   int rt_index);
  
  #endif	/* ROWSECURITY_H */
--- 39,46 ----
  
  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);
  
  #endif	/* ROWSECURITY_H */
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
new file mode 100644
index 44e8dab..5676079
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*************** ALTER TABLE t1 DROP COLUMN junk1;    --
*** 466,474 ****
--- 466,476 ----
  GRANT ALL ON t1 TO public;
  COPY t1 FROM stdin WITH (oids);
  CREATE TABLE t2 (c float) INHERITS (t1);
+ GRANT ALL ON t2 TO public;
  COPY t2 FROM stdin WITH (oids);
  CREATE TABLE t3 (c text, b text, a int) WITH OIDS;
  ALTER TABLE t3 INHERIT t1;
+ GRANT ALL ON t3 TO public;
  COPY t3(a,b,c) FROM stdin WITH (oids);
  CREATE POLICY p1 ON t1 FOR ALL TO PUBLIC USING (a % 2 = 0); -- be even number
  CREATE POLICY p2 ON t2 FOR ALL TO PUBLIC USING (a % 2 = 1); -- be odd number
*************** NOTICE:  f_leak => yyyyyy
*** 1117,1138 ****
   302 | 2 | yyyyyy      | (2,yyyyyy)
  (5 rows)
  
  RESET SESSION AUTHORIZATION;
  SET row_security TO OFF;
! SELECT * FROM t1;
   a |      b      
  ---+-------------
   1 | aaa
-  3 | ccc
-  2 | bbbbbb_updt
-  4 | dddddd_updt
   1 | abc
-  3 | cde
-  2 | bcdbcd
-  4 | defdef
   1 | xxx
!  3 | zzz
   2 | yyyyyy
  (11 rows)
  
  SET SESSION AUTHORIZATION rls_regress_user1;
--- 1119,1334 ----
   302 | 2 | yyyyyy      | (2,yyyyyy)
  (5 rows)
  
+ -- updates with from clause
+ EXPLAIN (COSTS OFF) UPDATE t2 SET b=t2.b FROM t3
+ WHERE t2.a = 3 and t3.a = 2 AND f_leak(t2.b) AND f_leak(t3.b);
+                           QUERY PLAN                           
+ ---------------------------------------------------------------
+  Update on t2 t2_1
+    ->  Nested Loop
+          ->  Subquery Scan on t2
+                Filter: f_leak(t2.b)
+                ->  LockRows
+                      ->  Seq Scan on t2 t2_2
+                            Filter: ((a = 3) AND ((a % 2) = 1))
+          ->  Seq Scan on t3
+                Filter: (f_leak(b) AND (a = 2))
+ (9 rows)
+ 
+ UPDATE t2 SET b=t2.b FROM t3
+ WHERE t2.a = 3 and t3.a = 2 AND f_leak(t2.b) AND f_leak(t3.b);
+ NOTICE:  f_leak => cde
+ NOTICE:  f_leak => xxx
+ NOTICE:  f_leak => zzz
+ NOTICE:  f_leak => yyyyyy
+ EXPLAIN (COSTS OFF) UPDATE t1 SET b=t1.b FROM t2
+ WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+                           QUERY PLAN                           
+ ---------------------------------------------------------------
+  Update on t1 t1_3
+    Update on t1 t1_3
+    Update on t2 t1
+    Update on t3 t1
+    ->  Nested Loop
+          ->  Subquery Scan on t1
+                Filter: f_leak(t1.b)
+                ->  LockRows
+                      ->  Seq Scan on t1 t1_4
+                            Filter: ((a = 3) AND ((a % 2) = 0))
+          ->  Subquery Scan on t2
+                Filter: f_leak(t2.b)
+                ->  Seq Scan on t2 t2_3
+                      Filter: ((a = 3) AND ((a % 2) = 1))
+    ->  Nested Loop
+          ->  Subquery Scan on t1_1
+                Filter: f_leak(t1_1.b)
+                ->  LockRows
+                      ->  Seq Scan on t2 t2_4
+                            Filter: ((a = 3) AND ((a % 2) = 0))
+          ->  Subquery Scan on t2_1
+                Filter: f_leak(t2_1.b)
+                ->  Seq Scan on t2 t2_5
+                      Filter: ((a = 3) AND ((a % 2) = 1))
+    ->  Nested Loop
+          ->  Subquery Scan on t1_2
+                Filter: f_leak(t1_2.b)
+                ->  LockRows
+                      ->  Seq Scan on t3
+                            Filter: ((a = 3) AND ((a % 2) = 0))
+          ->  Subquery Scan on t2_2
+                Filter: f_leak(t2_2.b)
+                ->  Seq Scan on t2 t2_6
+                      Filter: ((a = 3) AND ((a % 2) = 1))
+ (34 rows)
+ 
+ UPDATE t1 SET b=t1.b FROM t2
+ WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+ EXPLAIN (COSTS OFF) UPDATE t2 SET b=t2.b FROM t1
+ WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+                              QUERY PLAN                              
+ ---------------------------------------------------------------------
+  Update on t2 t2_1
+    ->  Nested Loop
+          ->  Subquery Scan on t2
+                Filter: f_leak(t2.b)
+                ->  LockRows
+                      ->  Seq Scan on t2 t2_2
+                            Filter: ((a = 3) AND ((a % 2) = 1))
+          ->  Subquery Scan on t1
+                Filter: f_leak(t1.b)
+                ->  Result
+                      ->  Append
+                            ->  Seq Scan on t1 t1_1
+                                  Filter: ((a = 3) AND ((a % 2) = 0))
+                            ->  Seq Scan on t2 t2_3
+                                  Filter: ((a = 3) AND ((a % 2) = 0))
+                            ->  Seq Scan on t3
+                                  Filter: ((a = 3) AND ((a % 2) = 0))
+ (17 rows)
+ 
+ UPDATE t2 SET b=t2.b FROM t1
+ WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+ NOTICE:  f_leak => cde
+ -- updates with from clause self join
+ EXPLAIN (COSTS OFF) UPDATE t2 t2_1 SET b = t2_2.b FROM t2 t2_2
+ WHERE t2_1.a = 3 AND t2_2.a = t2_1.a AND t2_2.b = t2_1.b
+ AND f_leak(t2_1.b) AND f_leak(t2_2.b) RETURNING *, t2_1, t2_2;
+                           QUERY PLAN                           
+ ---------------------------------------------------------------
+  Update on t2 t2_1_1
+    ->  Nested Loop
+          Join Filter: (t2_1.b = t2_2.b)
+          ->  Subquery Scan on t2_1
+                Filter: f_leak(t2_1.b)
+                ->  LockRows
+                      ->  Seq Scan on t2 t2_1_2
+                            Filter: ((a = 3) AND ((a % 2) = 1))
+          ->  Subquery Scan on t2_2
+                Filter: f_leak(t2_2.b)
+                ->  Seq Scan on t2 t2_2_1
+                      Filter: ((a = 3) AND ((a % 2) = 1))
+ (12 rows)
+ 
+ UPDATE t2 t2_1 SET b = t2_2.b FROM t2 t2_2
+ WHERE t2_1.a = 3 AND t2_2.a = t2_1.a AND t2_2.b = t2_1.b
+ AND f_leak(t2_1.b) AND f_leak(t2_2.b) RETURNING *, t2_1, t2_2;
+ NOTICE:  f_leak => cde
+ NOTICE:  f_leak => cde
+  a |  b  |  c  | a |  b  |  c  |    t2_1     |    t2_2     
+ ---+-----+-----+---+-----+-----+-------------+-------------
+  3 | cde | 3.3 | 3 | cde | 3.3 | (3,cde,3.3) | (3,cde,3.3)
+ (1 row)
+ 
+ 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
+    Update on t1 t1_1_3
+    Update on t2 t1_1
+    Update on t3 t1_1
+    ->  Nested Loop
+          Join Filter: (t1_1.b = t1_2.b)
+          ->  Subquery Scan on t1_1
+                Filter: f_leak(t1_1.b)
+                ->  LockRows
+                      ->  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)
+                ->  LockRows
+                      ->  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)
+                ->  LockRows
+                      ->  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))
+ (52 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 ORDER BY a,b;
   a |      b      
  ---+-------------
   1 | aaa
   1 | abc
   1 | xxx
!  2 | bbbbbb_updt
!  2 | bcdbcd
   2 | yyyyyy
+  3 | ccc
+  3 | cde
+  3 | zzz
+  4 | dddddd_updt
+  4 | defdef
  (11 rows)
  
  SET SESSION AUTHORIZATION rls_regress_user1;
*************** NOTICE:  f_leak => yyyyyy
*** 1193,1198 ****
--- 1389,1491 ----
  (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"
+ INSERT INTO bv1 VALUES (11, 'xxx'); -- should fail RLS check
+ ERROR:  new row violates WITH CHECK OPTION for "b1"
+ 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)
+          ->  Subquery Scan on b1_2
+                ->  LockRows
+                      ->  Seq Scan on b1 b1_3
+                            Filter: ((a > 0) AND (a = 4) AND ((a % 2) = 0))
+ (7 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)
+          ->  Subquery Scan on b1_2
+                ->  LockRows
+                      ->  Seq Scan on b1 b1_3
+                            Filter: ((a > 0) AND (a = 6) AND ((a % 2) = 0))
+ (7 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 ed7adbf..4da5035
*** a/src/test/regress/sql/rowsecurity.sql
--- b/src/test/regress/sql/rowsecurity.sql
*************** COPY t1 FROM stdin WITH (oids);
*** 207,212 ****
--- 207,214 ----
  \.
  
  CREATE TABLE t2 (c float) INHERITS (t1);
+ GRANT ALL ON t2 TO public;
+ 
  COPY t2 FROM stdin WITH (oids);
  201	1	abc	1.1
  202	2	bcd	2.2
*************** COPY t2 FROM stdin WITH (oids);
*** 216,221 ****
--- 218,225 ----
  
  CREATE TABLE t3 (c text, b text, a int) WITH OIDS;
  ALTER TABLE t3 INHERIT t1;
+ GRANT ALL ON t3 TO public;
+ 
  COPY t3(a,b,c) FROM stdin WITH (oids);
  301	1	xxx	X
  302	2	yyy	Y
*************** UPDATE only t1 SET b = b WHERE f_leak(b)
*** 423,431 ****
  UPDATE t1 SET b = b WHERE f_leak(b) RETURNING *;
  UPDATE t1 SET b = b WHERE f_leak(b) RETURNING oid, *, t1;
  
  RESET SESSION AUTHORIZATION;
  SET row_security TO OFF;
! SELECT * FROM t1;
  
  SET SESSION AUTHORIZATION rls_regress_user1;
  SET row_security TO ON;
--- 427,471 ----
  UPDATE t1 SET b = b WHERE f_leak(b) RETURNING *;
  UPDATE t1 SET b = b WHERE f_leak(b) RETURNING oid, *, t1;
  
+ -- updates with from clause
+ EXPLAIN (COSTS OFF) UPDATE t2 SET b=t2.b FROM t3
+ WHERE t2.a = 3 and t3.a = 2 AND f_leak(t2.b) AND f_leak(t3.b);
+ 
+ UPDATE t2 SET b=t2.b FROM t3
+ WHERE t2.a = 3 and t3.a = 2 AND f_leak(t2.b) AND f_leak(t3.b);
+ 
+ EXPLAIN (COSTS OFF) UPDATE t1 SET b=t1.b FROM t2
+ WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+ 
+ UPDATE t1 SET b=t1.b FROM t2
+ WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+ 
+ EXPLAIN (COSTS OFF) UPDATE t2 SET b=t2.b FROM t1
+ WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+ 
+ UPDATE t2 SET b=t2.b FROM t1
+ WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+ 
+ -- updates with from clause self join
+ EXPLAIN (COSTS OFF) UPDATE t2 t2_1 SET b = t2_2.b FROM t2 t2_2
+ WHERE t2_1.a = 3 AND t2_2.a = t2_1.a AND t2_2.b = t2_1.b
+ AND f_leak(t2_1.b) AND f_leak(t2_2.b) RETURNING *, t2_1, t2_2;
+ 
+ UPDATE t2 t2_1 SET b = t2_2.b FROM t2 t2_2
+ WHERE t2_1.a = 3 AND t2_2.a = t2_1.a AND t2_2.b = t2_1.b
+ AND f_leak(t2_1.b) AND f_leak(t2_2.b) RETURNING *, t2_1, t2_2;
+ 
+ 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 ORDER BY a,b;
  
  SET SESSION AUTHORIZATION rls_regress_user1;
  SET row_security TO ON;
*************** DELETE FROM only t1 WHERE f_leak(b) RETU
*** 436,441 ****
--- 476,514 ----
  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;
#6Kevin Grittner
kgrittn@ymail.com
In reply to: Dean Rasheed (#4)
Re: Row security violation error is misleading

Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

Re-using the SQLSTATE 44000 is a bit iffy too. We should
probably define something to differentiate this, like:

44P01 ROW SECURITY WRITE POLICY VIOLATION

Yes, that sounds sensible.

I would be more inclined to use:

42501 ERRCODE_INSUFFICIENT_PRIVILEGE

I know this is used 173 other places where a user attempts to do
something they are not authorized to do, so you would not be able
to differentiate the specific cause based on SQLSTATE if this is
used -- but why don't we feel that way about the other 173 causes?
Why does this security violation require a separate SQLSTATE?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Stephen Frost
sfrost@snowman.net
In reply to: Kevin Grittner (#6)
Re: Row security violation error is misleading

* Kevin Grittner (kgrittn@ymail.com) wrote:

Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

Re-using the SQLSTATE 44000 is a bit iffy too. We should
probably define something to differentiate this, like:

44P01 ROW SECURITY WRITE POLICY VIOLATION

Yes, that sounds sensible.

I would be more inclined to use:

42501 ERRCODE_INSUFFICIENT_PRIVILEGE

I know this is used 173 other places where a user attempts to do
something they are not authorized to do, so you would not be able
to differentiate the specific cause based on SQLSTATE if this is
used -- but why don't we feel that way about the other 173 causes?
Why does this security violation require a separate SQLSTATE?

I tend to agree with this and it feels more consistent. SQLSTATE is
already a very generic response system and knowing that it's a policy
violation instead of a GRANT violations strikes me as unlikely to be
terribly interesting at the level where you're just looking at the
SQLSTATE code.

Thanks!

Stephen

#8Stephen Frost
sfrost@snowman.net
In reply to: Dean Rasheed (#5)
Re: Row security violation error is misleading

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

On 7 April 2015 at 16:21, Stephen Frost <sfrost@snowman.net> wrote:

Agreed and we actually have a patch from Dean already to address this,
it's just been waiting on me (with a couple of other ones). It'd
certainly be great if you have time to take a look at those, though,
generally speaking, I feel prety happy about where those are and believe
they really just need to be reviewed/tested and maybe a bit of word-
smithing around the docs.

The first of those patches [1] has bit-rotted somewhat, due to the
recent changes to the way rowmarks are handled, so here's an updated
version. It wasn't a trivial merge, because commit
cb1ca4d800621dcae67ca6c799006de99fa4f0a5 made a change to
ExecBuildAuxRowMark() without a matching change to
preprocess_targetlist(), and one of the new RLS-with-inheritance tests
fell over that.

Thanks!

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

That was certainly unintentional (and wasn't the case at one point.. I
recall specifically checking that), thanks for addressing it.

2). In prepend_row_security_policies(), I think it is better to have
any table RLS policies applied before any hook policies, so that a
hook cannot be used to bypass built-in RLS.

While I agree that we want to include the RLS policies defined against
the table prior to any policies which are added by the hook, I don't
quite follow what you mean by "cannot be used to bypass built-in RLS".
If we allow, as intended, extensions to define their own policies then
it's entirely possible for the extension to return a "allow all" policy,
and I believe that's perfectly fine.

The idea has come up a couple of times to also have "restrictive"
policies and I agree that's an interesting idea and, once implemented,
we would want to allow extensions to define both kinds and make sure
that we apply "restrictive" policies defined in table-level policies
in addition to policies from extensions, but we're not quite there yet.

3). 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 ...".

Right, thanks for working through that and providing a fix.

4). The RLS expansion code in fireRIRrules() was handling RLS in the
main loop through the rtable. This 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 didn'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 most easily 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.

Agreed.

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

Urgh. Thanks for the testing and fix for that.

6). 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. WCOs are only needed on the result relation.

Ah, yes, that makes sense.

The second patch [2] is the one that is actually relevant to this
thread. This patch is primarily to apply the RLS checks earlier,
before an update is attempted, more like a regular permissions check.
This adds a new enum to classify the kinds of WCO, a side benefit of
which is that it allows different error messages when RLS checks are
violated, as opposed to WITH CHECK OPTIONs on views.

Right.

I actually re-used the sql status code 42501 -
ERRCODE_INSUFFICIENT_PRIVILEGE for a RLS check failure because of the
parallel with permissions checks, but I quite like Craig's idea of
inventing a new status code for this, so that it can be more easily
distinguished from a lack of GRANTed privileges.

As I mentioned to Kevin, I'm not sure that this is really a useful
distinction. I'm quite curious if other systems provide that
distinction between grant violations and policy violations. If they do
then that would certainly bolster the argument to provide the
distinction in PG.

There's another side benefit to this patch, which is that the new enum
could be extended to include a new kind of WCO for a check of the
USING quals of a RLS UPDATE policy on the update path of an INSERT..ON
CONFLICT UPDATE. As I pointed out on that thread, I think these quals
need to be treated differently from the WITH CHECK quals of a RLS
UPDATE policy, since they ought to apply to different tuples.
Therefore classifying the WCOs by command type is insufficient to
distinguish the 2 cases.

Good point.

Thanks!

Stephen

#9Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Stephen Frost (#8)
Re: Row security violation error is misleading

On 8 April 2015 at 16:27, Stephen Frost <sfrost@snowman.net> wrote:

2). In prepend_row_security_policies(), I think it is better to have
any table RLS policies applied before any hook policies, so that a
hook cannot be used to bypass built-in RLS.

While I agree that we want to include the RLS policies defined against
the table prior to any policies which are added by the hook, I don't
quite follow what you mean by "cannot be used to bypass built-in RLS".
If we allow, as intended, extensions to define their own policies then
it's entirely possible for the extension to return a "allow all" policy,
and I believe that's perfectly fine.

That doesn't match what the code currently does:

* Also, allow extensions to add their own 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).

which seems reasonable, and means that if there are both internal and
external policies, an "allow all" external policy would be a no-op.

All the patch does is to ensure that the quals from the external
policies are on the outer security barrier, so that if they contain
leaky functions they cannot leak data that doesn't pass the internal
policies (like a SB view on top of another SB view).

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10Craig Ringer
craig@2ndquadrant.com
In reply to: Dean Rasheed (#5)
Re: Row security violation error is misleading

On 8 April 2015 at 19:52, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

2). In prepend_row_security_policies(), I think it is better to have
any table RLS policies applied before any hook policies, so that a
hook cannot be used to bypass built-in RLS.

A hook really has to be able to ensure that built-in RLS cannot bypass the
hook's policies, too, i.e. the hook policy *must* return true for the row
to be visible.

This is necessary for mandatory access control hooks, which need to be able
to say "permit if and only if..."

I'll take a closer look at this.

3). 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 ...".

I'm impressed you found that one.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

#11Craig Ringer
craig@2ndquadrant.com
In reply to: Dean Rasheed (#9)
Re: Row security violation error is misleading

On 9 April 2015 at 01:30, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

That doesn't match what the code currently does:

* Also, allow extensions to add their own 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).

which seems reasonable, and means that if there are both internal and
external policies, an "allow all" external policy would be a no-op.

Great, I'm glad to see that they're ANDed now.

I wasn't caught up with the current state of this. At some earlier point
policies from hooks were being ORed, which made mandatory access control
extensions impossible.

(I need to finish reading threads before replying).

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

#12Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Stephen Frost (#8)
Re: Row security violation error is misleading

On 8 April 2015 at 16:27, Stephen Frost <sfrost@snowman.net> wrote:

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

I actually re-used the sql status code 42501 -
ERRCODE_INSUFFICIENT_PRIVILEGE for a RLS check failure because of the
parallel with permissions checks, but I quite like Craig's idea of
inventing a new status code for this, so that it can be more easily
distinguished from a lack of GRANTed privileges.

As I mentioned to Kevin, I'm not sure that this is really a useful
distinction. I'm quite curious if other systems provide that
distinction between grant violations and policy violations. If they do
then that would certainly bolster the argument to provide the
distinction in PG.

OK, on further reflection I think that's probably right.

ERRCODE_INSUFFICIENT_PRIVILEGE is certainly more appropriate than
anything based on a WCO violation, because it reflects the fact that
the current user isn't allowed to perform the insert/update, but
another user might be allowed, so this is a privilege problem, not a
data error.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#13Stephen Frost
sfrost@snowman.net
In reply to: Craig Ringer (#11)
Re: Row security violation error is misleading

* Craig Ringer (craig@2ndquadrant.com) wrote:

On 9 April 2015 at 01:30, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

That doesn't match what the code currently does:

Ah, right.

* Also, allow extensions to add their own 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).

which seems reasonable, and means that if there are both internal and
external policies, an "allow all" external policy would be a no-op.

Great, I'm glad to see that they're ANDed now.

I wasn't caught up with the current state of this. At some earlier point
policies from hooks were being ORed, which made mandatory access control
extensions impossible.

That's what I had been recalling also. I'm certainly on-board with
wanting to support MAC, but I'm wondering what we're going to do when we
add support for "restrictive" policies. We'd certainly want extensions
to be able to provide both kinds and we will need to make sure they are
added correctly, with all of the restrictive policies being combined
and applied together, and then the permissive policies similairly
combined (but with OR's).

Thoughts?

Thanks!

Stephen

#14Craig Ringer
craig@2ndquadrant.com
In reply to: Dean Rasheed (#12)
Re: Row security violation error is misleading

On 9 April 2015 at 14:56, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

On 8 April 2015 at 16:27, Stephen Frost <sfrost@snowman.net> wrote:

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

I actually re-used the sql status code 42501 -
ERRCODE_INSUFFICIENT_PRIVILEGE for a RLS check failure because of the
parallel with permissions checks, but I quite like Craig's idea of
inventing a new status code for this, so that it can be more easily
distinguished from a lack of GRANTed privileges.

As I mentioned to Kevin, I'm not sure that this is really a useful
distinction. I'm quite curious if other systems provide that
distinction between grant violations and policy violations. If they do
then that would certainly bolster the argument to provide the
distinction in PG.

OK, on further reflection I think that's probably right.

ERRCODE_INSUFFICIENT_PRIVILEGE is certainly more appropriate than
anything based on a WCO violation, because it reflects the fact that
the current user isn't allowed to perform the insert/update, but
another user might be allowed, so this is a privilege problem, not a
data error.

I'd be OK with that too. Reusing WCO's code for something that isn't really
"with check option" at all was my concern, really.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

#15Stephen Frost
sfrost@snowman.net
In reply to: Dean Rasheed (#5)
1 attachment(s)
Re: Row security violation error is misleading

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

On 7 April 2015 at 16:21, Stephen Frost <sfrost@snowman.net> wrote:

Agreed and we actually have a patch from Dean already to address this,
it's just been waiting on me (with a couple of other ones). It'd
certainly be great if you have time to take a look at those, though,
generally speaking, I feel prety happy about where those are and believe
they really just need to be reviewed/tested and maybe a bit of word-
smithing around the docs.

The first of those patches [1] has bit-rotted somewhat, due to the
recent changes to the way rowmarks are handled, so here's an updated
version. It wasn't a trivial merge, because commit
cb1ca4d800621dcae67ca6c799006de99fa4f0a5 made a change to
ExecBuildAuxRowMark() without a matching change to
preprocess_targetlist(), and one of the new RLS-with-inheritance tests
fell over that.

Thanks a lot for this. Please take a look at the attached. It still
includes the preprocess_targetlist() changes, so the regression tests
don't fail, but that'll be committed independently (hopefully soon).

I've taken an initial look at the second patch (actually, a few times)
and plan to do a thorough review soon. I'd definitely like to get these
both committed and done with very shortly. Again, apologies about the
delays; this past weekend was quite a bit busier than I originally
anticipated.

Thanks!

Stephen

Attachments:

rls-spf.v7.patchtext/x-diff; charset=us-asciiDownload
From 33cbd926f935dbf3de2302c6c5bb5babebceaacf Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Sun, 19 Apr 2015 18:58:02 -0400
Subject: [PATCH] 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.

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.
---
 src/backend/optimizer/plan/planner.c               |  45 ++-
 src/backend/optimizer/prep/preptlist.c             |  34 +--
 src/backend/rewrite/rewriteHandler.c               | 127 ++++++---
 src/backend/rewrite/rowsecurity.c                  | 248 +++++++++++------
 src/include/rewrite/rowsecurity.h                  |   9 +-
 src/test/modules/Makefile                          |   1 +
 src/test/modules/test_rls_hooks/.gitignore         |   4 +
 src/test/modules/test_rls_hooks/Makefile           |  22 ++
 src/test/modules/test_rls_hooks/README             |  16 ++
 .../test_rls_hooks/expected/test_rls_hooks.out     | 193 +++++++++++++
 src/test/modules/test_rls_hooks/rls_hooks.conf     |   1 +
 .../modules/test_rls_hooks/sql/test_rls_hooks.sql  | 157 +++++++++++
 src/test/modules/test_rls_hooks/test_rls_hooks.c   | 170 ++++++++++++
 .../modules/test_rls_hooks/test_rls_hooks.control  |   4 +
 src/test/modules/test_rls_hooks/test_rls_hooks.h   |  25 ++
 src/test/regress/expected/rowsecurity.out          | 309 ++++++++++++++++++++-
 src/test/regress/sql/rowsecurity.sql               |  75 ++++-
 17 files changed, 1281 insertions(+), 159 deletions(-)
 create mode 100644 src/test/modules/test_rls_hooks/.gitignore
 create mode 100644 src/test/modules/test_rls_hooks/Makefile
 create mode 100644 src/test/modules/test_rls_hooks/README
 create mode 100644 src/test/modules/test_rls_hooks/expected/test_rls_hooks.out
 create mode 100644 src/test/modules/test_rls_hooks/rls_hooks.conf
 create mode 100644 src/test/modules/test_rls_hooks/sql/test_rls_hooks.sql
 create mode 100644 src/test/modules/test_rls_hooks/test_rls_hooks.c
 create mode 100644 src/test/modules/test_rls_hooks/test_rls_hooks.control
 create mode 100644 src/test/modules/test_rls_hooks/test_rls_hooks.h

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 876a87f..ea4d4c5 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -790,6 +790,7 @@ inheritance_planner(PlannerInfo *root)
 {
 	Query	   *parse = root->parse;
 	int			parentRTindex = parse->resultRelation;
+	Bitmapset  *resultRTindexes = NULL;
 	int			nominalRelation = -1;
 	List	   *final_rtable = NIL;
 	int			save_rel_array_size = 0;
@@ -815,7 +816,21 @@ inheritance_planner(PlannerInfo *root)
 	 * (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);
@@ -886,21 +901,29 @@ inheritance_planner(PlannerInfo *root)
 			{
 				RangeTblEntry *rte = (RangeTblEntry *) lfirst(lr);
 
-				if (rte->rtekind == RTE_SUBQUERY)
+				/*
+				 * 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, so we can save a few cycles by applying
-					 * ChangeVarNodes before we append the RTE to the
-					 * rangetable.
+					 * 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);
 				}
@@ -2283,7 +2306,19 @@ select_rowmark_type(RangeTblEntry *rte, LockClauseStrength strength)
 		switch (strength)
 		{
 			case LCS_NONE:
-				/* don't need tuple lock, only ability to re-fetch the row */
+				/*
+				 * We don't need a tuple lock, only the ability to re-fetch
+				 * the row.  Regular tables support ROW_MARK_REFERENCE, but if
+				 * this RTE has security barrier quals, it will be turned into
+				 * a subquery during planning, so use ROW_MARK_COPY.
+				 *
+				 * This is only necessary for LCS_NONE, since real tuple locks
+				 * on an RTE with security barrier quals are supported by
+				 * pushing the lock down into the subquery --- see
+				 * expand_security_qual.
+				 */
+				if (rte->securityQuals != NIL)
+					return ROW_MARK_COPY;
 				return ROW_MARK_REFERENCE;
 				break;
 			case LCS_FORKEYSHARE:
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index 08e7c44..580c846 100644
--- a/src/backend/optimizer/prep/preptlist.c
+++ b/src/backend/optimizer/prep/preptlist.c
@@ -107,23 +107,6 @@ preprocess_targetlist(PlannerInfo *root, List *tlist)
 								  pstrdup(resname),
 								  true);
 			tlist = lappend(tlist, tle);
-
-			/* if parent of inheritance tree, need the tableoid too */
-			if (rc->isParent)
-			{
-				var = makeVar(rc->rti,
-							  TableOidAttributeNumber,
-							  OIDOID,
-							  -1,
-							  InvalidOid,
-							  0);
-				snprintf(resname, sizeof(resname), "tableoid%u", rc->rowmarkId);
-				tle = makeTargetEntry((Expr *) var,
-									  list_length(tlist) + 1,
-									  pstrdup(resname),
-									  true);
-				tlist = lappend(tlist, tle);
-			}
 		}
 		if (rc->allMarkTypes & (1 << ROW_MARK_COPY))
 		{
@@ -139,6 +122,23 @@ preprocess_targetlist(PlannerInfo *root, List *tlist)
 								  true);
 			tlist = lappend(tlist, tle);
 		}
+
+		/* If parent of inheritance tree, always fetch the tableoid too. */
+		if (rc->isParent)
+		{
+			var = makeVar(rc->rti,
+						  TableOidAttributeNumber,
+						  OIDOID,
+						  -1,
+						  InvalidOid,
+						  0);
+			snprintf(resname, sizeof(resname), "tableoid%u", rc->rowmarkId);
+			tle = makeTargetEntry((Expr *) var,
+								  list_length(tlist) + 1,
+								  pstrdup(resname),
+								  true);
+			tlist = lappend(tlist, tle);
+		}
 	}
 
 	/*
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 9d2c280..08ec13c 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1714,51 +1714,6 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
 				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);
 	}
@@ -1780,6 +1735,88 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
 		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
index 7669130..aafef90 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 *) hook_with_check_expr;
+			wco->qual = (Node *) rowsec_with_check_expr;
 			wco->cascaded = false;
-			root->withCheckOptions = lcons(wco, root->withCheckOptions);
+			*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_permissive;
+			wco->cascaded = false;
+			*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;
+
+			combined_quals = lcons(copyObject(rowsec_expr), combined_quals);
+			combined_quals = lcons(copyObject(hook_expr_permissive), combined_quals);
 
-		if (hook_expr)
-			rte->securityQuals = lcons(hook_expr,
-									   rte->securityQuals);
+			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;
-
-	/*
-	 * 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;
+	*hasRowSecurity = true;
 
-	/* 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,7 +473,8 @@ 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);
 
@@ -419,9 +492,11 @@ process_policies(List *policies, int rt_index, Expr **qual_eval,
 
 	/*
 	 * If we end up with only USING quals, then use those as
-	 * WITH CHECK quals also.
+	 * WITH CHECK quals also.  WITH CHECK only applies to the resultRelation
+	 * case (where we're adding rows to a table), no sense adding them
+	 * otherwise.
 	 */
-	if (with_check_quals == NIL)
+	if (with_check_quals == NIL && rt_index == root->resultRelation)
 		with_check_quals = copyObject(quals);
 
 	/*
@@ -431,27 +506,40 @@ process_policies(List *policies, int rt_index, Expr **qual_eval,
 	 * 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;
 }
diff --git a/src/include/rewrite/rowsecurity.h b/src/include/rewrite/rowsecurity.h
index 8d19bfd..115c9a8 100644
--- a/src/include/rewrite/rowsecurity.h
+++ b/src/include/rewrite/rowsecurity.h
@@ -37,9 +37,12 @@ typedef struct RowSecurityDesc
 typedef List *(*row_security_policy_hook_type)(CmdType cmdtype,
 											   Relation relation);
 
-extern PGDLLIMPORT row_security_policy_hook_type row_security_policy_hook;
+extern PGDLLIMPORT row_security_policy_hook_type row_security_policy_hook_permissive;
 
-extern bool prepend_row_security_policies(Query* root, RangeTblEntry* rte,
-									   int rt_index);
+extern PGDLLIMPORT row_security_policy_hook_type row_security_policy_hook_restrictive;
+
+extern void get_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index,
+						  List **securityQuals, List **withCheckOptions,
+						  bool *hasRowSecurity, bool *hasSubLinks);
 
 #endif	/* ROWSECURITY_H */
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 93d93af..730fa75 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -8,6 +8,7 @@ SUBDIRS = \
 		  commit_ts \
 		  worker_spi \
 		  dummy_seclabel \
+		  test_rls_hooks \
 		  test_shm_mq \
 		  test_parser
 
diff --git a/src/test/modules/test_rls_hooks/.gitignore b/src/test/modules/test_rls_hooks/.gitignore
new file mode 100644
index 0000000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/test_rls_hooks/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_rls_hooks/Makefile b/src/test/modules/test_rls_hooks/Makefile
new file mode 100644
index 0000000..6b772c4
--- /dev/null
+++ b/src/test/modules/test_rls_hooks/Makefile
@@ -0,0 +1,22 @@
+# src/test/modules/test_rls_hooks/Makefile
+
+MODULE_big = test_rls_hooks
+OBJS = test_rls_hooks.o $(WIN32RES)
+PGFILEDESC = "test_rls_hooks - example use of RLS hooks"
+
+EXTENSION = test_rls_hooks
+# DATA = test_rls_hooks--1.0.sql
+
+REGRESS = test_rls_hooks
+REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_rls_hooks/rls_hooks.conf
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_rls_hooks
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_rls_hooks/README b/src/test/modules/test_rls_hooks/README
new file mode 100644
index 0000000..b61dace
--- /dev/null
+++ b/src/test/modules/test_rls_hooks/README
@@ -0,0 +1,16 @@
+test_rls_hooks is an example of how to use the hooks provided for RLS to
+define additional policies to be used.
+
+Functions
+=========
+test_rls_hook_permissive(CmdType cmdtype, Relation relation)
+    RETURNS List*
+
+Returns a list of policies which should be added to any existing
+policies on the relation, combined with OR.
+
+test_rls_hook_restrictive(CmdType cmdtype, Relation relation)
+    RETURNS List*
+
+Returns a list of policies which should be added to any existing
+policies on the relation, combined with AND.
diff --git a/src/test/modules/test_rls_hooks/expected/test_rls_hooks.out b/src/test/modules/test_rls_hooks/expected/test_rls_hooks.out
new file mode 100644
index 0000000..d55aea7
--- /dev/null
+++ b/src/test/modules/test_rls_hooks/expected/test_rls_hooks.out
@@ -0,0 +1,193 @@
+CREATE TABLE rls_test_permissive (
+    username        name,
+    supervisor      name,
+    data            integer
+);
+-- initial test data
+INSERT INTO rls_test_permissive VALUES ('r1','s1',4);
+INSERT INTO rls_test_permissive VALUES ('r2','s2',5);
+INSERT INTO rls_test_permissive VALUES ('r3','s3',6);
+CREATE TABLE rls_test_restrictive (
+    username        name,
+    supervisor      name,
+    data            integer
+);
+-- initial test data
+INSERT INTO rls_test_restrictive VALUES ('r1','s1',1);
+INSERT INTO rls_test_restrictive VALUES ('r2','s2',2);
+INSERT INTO rls_test_restrictive VALUES ('r3','s3',3);
+CREATE TABLE rls_test_both (
+    username        name,
+    supervisor      name,
+    data            integer
+);
+-- initial test data
+INSERT INTO rls_test_both VALUES ('r1','s1',7);
+INSERT INTO rls_test_both VALUES ('r2','s2',8);
+INSERT INTO rls_test_both VALUES ('r3','s3',9);
+ALTER TABLE rls_test_permissive ENABLE ROW LEVEL SECURITY;
+ALTER TABLE rls_test_restrictive ENABLE ROW LEVEL SECURITY;
+ALTER TABLE rls_test_both ENABLE ROW LEVEL SECURITY;
+CREATE ROLE r1;
+CREATE ROLE s1;
+GRANT SELECT,INSERT ON rls_test_permissive TO r1;
+GRANT SELECT,INSERT ON rls_test_restrictive TO r1;
+GRANT SELECT,INSERT ON rls_test_both TO r1;
+GRANT SELECT,INSERT ON rls_test_permissive TO s1;
+GRANT SELECT,INSERT ON rls_test_restrictive TO s1;
+GRANT SELECT,INSERT ON rls_test_both TO s1;
+SET ROLE r1;
+-- With only the hook's policies, permissive
+-- hook's policy is current_user = username
+EXPLAIN (costs off) SELECT * FROM rls_test_permissive;
+               QUERY PLAN                
+-----------------------------------------
+ Seq Scan on rls_test_permissive
+   Filter: ("current_user"() = username)
+(2 rows)
+
+SELECT * FROM rls_test_permissive;
+ username | supervisor | data 
+----------+------------+------
+ r1       | s1         |    4
+(1 row)
+
+-- success
+INSERT INTO rls_test_permissive VALUES ('r1','s1',10);
+-- failure
+INSERT INTO rls_test_permissive VALUES ('r4','s4',10);
+ERROR:  new row violates WITH CHECK OPTION for "rls_test_permissive"
+SET ROLE s1;
+-- With only the hook's policies, restrictive
+-- hook's policy is current_user = supervisor
+EXPLAIN (costs off) SELECT * FROM rls_test_restrictive;
+                QUERY PLAN                 
+-------------------------------------------
+ Seq Scan on rls_test_restrictive
+   Filter: ("current_user"() = supervisor)
+(2 rows)
+
+SELECT * FROM rls_test_restrictive;
+ username | supervisor | data 
+----------+------------+------
+ r1       | s1         |    1
+(1 row)
+
+-- success
+INSERT INTO rls_test_restrictive VALUES ('r1','s1',10);
+-- failure
+INSERT INTO rls_test_restrictive VALUES ('r4','s4',10);
+ERROR:  new row violates WITH CHECK OPTION for "rls_test_restrictive"
+SET ROLE s1;
+-- With only the hook's policies, both
+-- permissive hook's policy is current_user = username
+-- restrictive hook's policy is current_user = superuser
+-- combined with AND, results in nothing being allowed
+EXPLAIN (costs off) SELECT * FROM rls_test_both;
+                      QUERY PLAN                       
+-------------------------------------------------------
+ Subquery Scan on rls_test_both
+   Filter: ("current_user"() = rls_test_both.username)
+   ->  Seq Scan on rls_test_both rls_test_both_1
+         Filter: ("current_user"() = supervisor)
+(4 rows)
+
+SELECT * FROM rls_test_both;
+ username | supervisor | data 
+----------+------------+------
+(0 rows)
+
+-- failure
+INSERT INTO rls_test_both VALUES ('r1','s1',10);
+ERROR:  new row violates WITH CHECK OPTION for "rls_test_both"
+-- failure
+INSERT INTO rls_test_both VALUES ('r4','s1',10);
+ERROR:  new row violates WITH CHECK OPTION for "rls_test_both"
+-- failure
+INSERT INTO rls_test_both VALUES ('r4','s4',10);
+ERROR:  new row violates WITH CHECK OPTION for "rls_test_both"
+RESET ROLE;
+-- Create "internal" policies, to check that the policies from
+-- the hooks are combined correctly.
+CREATE POLICY p1 ON rls_test_permissive USING (data % 2 = 0);
+CREATE POLICY p1 ON rls_test_restrictive USING (data % 2 = 0);
+CREATE POLICY p1 ON rls_test_both USING (data % 2 = 0);
+SET ROLE r1;
+-- With both internal and hook policies, permissive
+EXPLAIN (costs off) SELECT * FROM rls_test_permissive;
+                          QUERY PLAN                           
+---------------------------------------------------------------
+ Seq Scan on rls_test_permissive
+   Filter: (("current_user"() = username) OR ((data % 2) = 0))
+(2 rows)
+
+SELECT * FROM rls_test_permissive;
+ username | supervisor | data 
+----------+------------+------
+ r1       | s1         |    4
+ r3       | s3         |    6
+ r1       | s1         |   10
+(3 rows)
+
+-- success
+INSERT INTO rls_test_permissive VALUES ('r1','s1',7);
+-- success
+INSERT INTO rls_test_permissive VALUES ('r3','s3',10);
+-- failure
+INSERT INTO rls_test_permissive VALUES ('r4','s4',7);
+ERROR:  new row violates WITH CHECK OPTION for "rls_test_permissive"
+SET ROLE s1;
+-- With both internal and hook policies, restrictive
+EXPLAIN (costs off) SELECT * FROM rls_test_restrictive;
+                          QUERY PLAN                           
+---------------------------------------------------------------
+ Subquery Scan on rls_test_restrictive
+   Filter: ((rls_test_restrictive.data % 2) = 0)
+   ->  Seq Scan on rls_test_restrictive rls_test_restrictive_1
+         Filter: ("current_user"() = supervisor)
+(4 rows)
+
+SELECT * FROM rls_test_restrictive;
+ username | supervisor | data 
+----------+------------+------
+ r1       | s1         |   10
+(1 row)
+
+-- success
+INSERT INTO rls_test_restrictive VALUES ('r1','s1',8);
+-- failure
+INSERT INTO rls_test_restrictive VALUES ('r3','s3',10);
+ERROR:  new row violates WITH CHECK OPTION for "rls_test_restrictive"
+-- failure
+INSERT INTO rls_test_restrictive VALUES ('r1','s1',7);
+ERROR:  new row violates WITH CHECK OPTION for "rls_test_restrictive"
+-- failure
+INSERT INTO rls_test_restrictive VALUES ('r4','s4',7);
+ERROR:  new row violates WITH CHECK OPTION for "rls_test_restrictive"
+-- With both internal and hook policies, both permissive
+-- and restrictive hook policies
+EXPLAIN (costs off) SELECT * FROM rls_test_both;
+                                        QUERY PLAN                                         
+-------------------------------------------------------------------------------------------
+ Subquery Scan on rls_test_both
+   Filter: (("current_user"() = rls_test_both.username) OR ((rls_test_both.data % 2) = 0))
+   ->  Seq Scan on rls_test_both rls_test_both_1
+         Filter: ("current_user"() = supervisor)
+(4 rows)
+
+SELECT * FROM rls_test_both;
+ username | supervisor | data 
+----------+------------+------
+(0 rows)
+
+-- success
+INSERT INTO rls_test_both VALUES ('r1','s1',8);
+-- failure
+INSERT INTO rls_test_both VALUES ('r3','s3',10);
+ERROR:  new row violates WITH CHECK OPTION for "rls_test_both"
+-- failure
+INSERT INTO rls_test_both VALUES ('r1','s1',7);
+ERROR:  new row violates WITH CHECK OPTION for "rls_test_both"
+-- failure
+INSERT INTO rls_test_both VALUES ('r4','s4',7);
+ERROR:  new row violates WITH CHECK OPTION for "rls_test_both"
diff --git a/src/test/modules/test_rls_hooks/rls_hooks.conf b/src/test/modules/test_rls_hooks/rls_hooks.conf
new file mode 100644
index 0000000..a522c0e
--- /dev/null
+++ b/src/test/modules/test_rls_hooks/rls_hooks.conf
@@ -0,0 +1 @@
+shared_preload_libraries = test_rls_hooks
diff --git a/src/test/modules/test_rls_hooks/sql/test_rls_hooks.sql b/src/test/modules/test_rls_hooks/sql/test_rls_hooks.sql
new file mode 100644
index 0000000..be13ab4
--- /dev/null
+++ b/src/test/modules/test_rls_hooks/sql/test_rls_hooks.sql
@@ -0,0 +1,157 @@
+CREATE TABLE rls_test_permissive (
+    username        name,
+    supervisor      name,
+    data            integer
+);
+
+-- initial test data
+INSERT INTO rls_test_permissive VALUES ('r1','s1',4);
+INSERT INTO rls_test_permissive VALUES ('r2','s2',5);
+INSERT INTO rls_test_permissive VALUES ('r3','s3',6);
+
+CREATE TABLE rls_test_restrictive (
+    username        name,
+    supervisor      name,
+    data            integer
+);
+
+-- initial test data
+INSERT INTO rls_test_restrictive VALUES ('r1','s1',1);
+INSERT INTO rls_test_restrictive VALUES ('r2','s2',2);
+INSERT INTO rls_test_restrictive VALUES ('r3','s3',3);
+
+CREATE TABLE rls_test_both (
+    username        name,
+    supervisor      name,
+    data            integer
+);
+
+-- initial test data
+INSERT INTO rls_test_both VALUES ('r1','s1',7);
+INSERT INTO rls_test_both VALUES ('r2','s2',8);
+INSERT INTO rls_test_both VALUES ('r3','s3',9);
+
+ALTER TABLE rls_test_permissive ENABLE ROW LEVEL SECURITY;
+ALTER TABLE rls_test_restrictive ENABLE ROW LEVEL SECURITY;
+ALTER TABLE rls_test_both ENABLE ROW LEVEL SECURITY;
+
+CREATE ROLE r1;
+CREATE ROLE s1;
+
+GRANT SELECT,INSERT ON rls_test_permissive TO r1;
+GRANT SELECT,INSERT ON rls_test_restrictive TO r1;
+GRANT SELECT,INSERT ON rls_test_both TO r1;
+
+GRANT SELECT,INSERT ON rls_test_permissive TO s1;
+GRANT SELECT,INSERT ON rls_test_restrictive TO s1;
+GRANT SELECT,INSERT ON rls_test_both TO s1;
+
+SET ROLE r1;
+
+-- With only the hook's policies, permissive
+-- hook's policy is current_user = username
+EXPLAIN (costs off) SELECT * FROM rls_test_permissive;
+
+SELECT * FROM rls_test_permissive;
+
+-- success
+INSERT INTO rls_test_permissive VALUES ('r1','s1',10);
+
+-- failure
+INSERT INTO rls_test_permissive VALUES ('r4','s4',10);
+
+SET ROLE s1;
+
+-- With only the hook's policies, restrictive
+-- hook's policy is current_user = supervisor
+EXPLAIN (costs off) SELECT * FROM rls_test_restrictive;
+
+SELECT * FROM rls_test_restrictive;
+
+-- success
+INSERT INTO rls_test_restrictive VALUES ('r1','s1',10);
+
+-- failure
+INSERT INTO rls_test_restrictive VALUES ('r4','s4',10);
+
+SET ROLE s1;
+
+-- With only the hook's policies, both
+-- permissive hook's policy is current_user = username
+-- restrictive hook's policy is current_user = superuser
+-- combined with AND, results in nothing being allowed
+EXPLAIN (costs off) SELECT * FROM rls_test_both;
+
+SELECT * FROM rls_test_both;
+
+-- failure
+INSERT INTO rls_test_both VALUES ('r1','s1',10);
+
+-- failure
+INSERT INTO rls_test_both VALUES ('r4','s1',10);
+
+-- failure
+INSERT INTO rls_test_both VALUES ('r4','s4',10);
+
+RESET ROLE;
+
+-- Create "internal" policies, to check that the policies from
+-- the hooks are combined correctly.
+CREATE POLICY p1 ON rls_test_permissive USING (data % 2 = 0);
+
+CREATE POLICY p1 ON rls_test_restrictive USING (data % 2 = 0);
+
+CREATE POLICY p1 ON rls_test_both USING (data % 2 = 0);
+
+SET ROLE r1;
+
+-- With both internal and hook policies, permissive
+EXPLAIN (costs off) SELECT * FROM rls_test_permissive;
+
+SELECT * FROM rls_test_permissive;
+
+-- success
+INSERT INTO rls_test_permissive VALUES ('r1','s1',7);
+
+-- success
+INSERT INTO rls_test_permissive VALUES ('r3','s3',10);
+
+-- failure
+INSERT INTO rls_test_permissive VALUES ('r4','s4',7);
+
+SET ROLE s1;
+
+-- With both internal and hook policies, restrictive
+EXPLAIN (costs off) SELECT * FROM rls_test_restrictive;
+
+SELECT * FROM rls_test_restrictive;
+
+-- success
+INSERT INTO rls_test_restrictive VALUES ('r1','s1',8);
+
+-- failure
+INSERT INTO rls_test_restrictive VALUES ('r3','s3',10);
+
+-- failure
+INSERT INTO rls_test_restrictive VALUES ('r1','s1',7);
+
+-- failure
+INSERT INTO rls_test_restrictive VALUES ('r4','s4',7);
+
+-- With both internal and hook policies, both permissive
+-- and restrictive hook policies
+EXPLAIN (costs off) SELECT * FROM rls_test_both;
+
+SELECT * FROM rls_test_both;
+
+-- success
+INSERT INTO rls_test_both VALUES ('r1','s1',8);
+
+-- failure
+INSERT INTO rls_test_both VALUES ('r3','s3',10);
+
+-- failure
+INSERT INTO rls_test_both VALUES ('r1','s1',7);
+
+-- failure
+INSERT INTO rls_test_both VALUES ('r4','s4',7);
diff --git a/src/test/modules/test_rls_hooks/test_rls_hooks.c b/src/test/modules/test_rls_hooks/test_rls_hooks.c
new file mode 100644
index 0000000..f2f87c3
--- /dev/null
+++ b/src/test/modules/test_rls_hooks/test_rls_hooks.c
@@ -0,0 +1,170 @@
+/*--------------------------------------------------------------------------
+ *
+ * test.c
+ *		Test harness code for shared memory message queues.
+ *
+ * Copyright (C) 2015, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		src/test/modules/test_rls_hooks/test_rls_hooks.c
+ *
+ * -------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "miscadmin.h"
+
+#include "test_rls_hooks.h"
+
+#include <catalog/pg_type.h>
+#include <nodes/makefuncs.h>
+#include <nodes/makefuncs.h>
+#include <parser/parse_clause.h>
+#include <parser/parse_node.h>
+#include <parser/parse_relation.h>
+#include <rewrite/rowsecurity.h>
+#include <utils/acl.h>
+#include <utils/rel.h>
+#include <utils/relcache.h>
+
+PG_MODULE_MAGIC;
+
+/* Saved hook values in case of unload */
+static row_security_policy_hook_type prev_row_security_policy_hook_permissive = NULL;
+static row_security_policy_hook_type prev_row_security_policy_hook_restrictive = NULL;
+
+void        _PG_init(void);
+void        _PG_fini(void);
+
+/* Install hooks */
+void		_PG_init(void)
+{
+	/* Save values for unload  */
+	prev_row_security_policy_hook_permissive = row_security_policy_hook_permissive;
+	prev_row_security_policy_hook_restrictive = row_security_policy_hook_restrictive;
+
+	/* Set our hooks */
+	row_security_policy_hook_permissive = test_rls_hooks_permissive;
+	row_security_policy_hook_restrictive = test_rls_hooks_restrictive;
+}
+
+/* Uninstall hooks */
+void        _PG_fini(void)
+{
+	row_security_policy_hook_permissive = prev_row_security_policy_hook_permissive;
+	row_security_policy_hook_restrictive = prev_row_security_policy_hook_restrictive;
+}
+
+/*
+ * Return permissive policies to be added
+ */
+List*
+test_rls_hooks_permissive(CmdType cmdtype, Relation relation)
+{
+	List			   *policies = NIL;
+	RowSecurityPolicy  *policy = palloc0(sizeof(RowSecurityPolicy));
+	Datum				role;
+	FuncCall		   *n;
+	Node			   *e;
+	ColumnRef		   *c;
+	ParseState		   *qual_pstate;
+	RangeTblEntry	   *rte;
+
+	if (strcmp(RelationGetRelationName(relation),"rls_test_permissive")
+			&& strcmp(RelationGetRelationName(relation),"rls_test_both"))
+		return NIL;
+
+	qual_pstate = make_parsestate(NULL);
+
+	rte = addRangeTableEntryForRelation(qual_pstate, relation, NULL, false,
+										false);
+	addRTEtoQuery(qual_pstate, rte, false, true, true);
+
+	role = ObjectIdGetDatum(ACL_ID_PUBLIC);
+
+	policy->policy_name = pstrdup("extension policy");
+	policy->policy_id = InvalidOid;
+	policy->polcmd = '*';
+	policy->roles = construct_array(&role, 1, OIDOID, sizeof(Oid), true, 'i');
+	/*
+	policy->qual = (Expr *) makeConst(BOOLOID, -1, InvalidOid,
+									  sizeof(bool), BoolGetDatum(true),
+									  false, true);
+									  */
+
+	n = makeFuncCall(list_make2(makeString("pg_catalog"),
+								makeString("current_user")), NIL, 0);
+
+	c = makeNode(ColumnRef);
+	c->fields = list_make1(makeString("username"));
+	c->location = 0;
+
+	e = (Node*) makeSimpleA_Expr(AEXPR_OP, "=", (Node*) n, (Node*) c, 0);
+
+	policy->qual = (Expr*) transformWhereClause(qual_pstate, copyObject(e),
+												EXPR_KIND_WHERE,
+												"POLICY");
+
+	policy->with_check_qual = copyObject(policy->qual);
+	policy->hassublinks = false;
+
+	policies = list_make1(policy);
+
+	return policies;
+}
+
+/*
+ * Return restrictive policies to be added
+ */
+List*
+test_rls_hooks_restrictive(CmdType cmdtype, Relation relation)
+{
+	List			   *policies = NIL;
+	RowSecurityPolicy  *policy = palloc0(sizeof(RowSecurityPolicy));
+	Datum				role;
+	FuncCall		   *n;
+	Node			   *e;
+	ColumnRef		   *c;
+	ParseState		   *qual_pstate;
+	RangeTblEntry	   *rte;
+
+
+	if (strcmp(RelationGetRelationName(relation),"rls_test_restrictive")
+			&& strcmp(RelationGetRelationName(relation),"rls_test_both"))
+		return NIL;
+
+	qual_pstate = make_parsestate(NULL);
+
+	rte = addRangeTableEntryForRelation(qual_pstate, relation, NULL, false,
+										false);
+	addRTEtoQuery(qual_pstate, rte, false, true, true);
+
+	role = ObjectIdGetDatum(ACL_ID_PUBLIC);
+
+	policy->policy_name = pstrdup("extension policy");
+	policy->policy_id = InvalidOid;
+	policy->polcmd = '*';
+	policy->roles = construct_array(&role, 1, OIDOID, sizeof(Oid), true, 'i');
+
+	n = makeFuncCall(list_make2(makeString("pg_catalog"),
+								makeString("current_user")), NIL, 0);
+
+	c = makeNode(ColumnRef);
+	c->fields = list_make1(makeString("supervisor"));
+	c->location = 0;
+
+	e = (Node*) makeSimpleA_Expr(AEXPR_OP, "=", (Node*) n, (Node*) c, 0);
+
+	policy->qual = (Expr*) transformWhereClause(qual_pstate, copyObject(e),
+												EXPR_KIND_WHERE,
+												"POLICY");
+
+	policy->with_check_qual = copyObject(policy->qual);
+	policy->hassublinks = false;
+
+	policies = list_make1(policy);
+
+	return policies;
+}
diff --git a/src/test/modules/test_rls_hooks/test_rls_hooks.control b/src/test/modules/test_rls_hooks/test_rls_hooks.control
new file mode 100644
index 0000000..9f9f13f
--- /dev/null
+++ b/src/test/modules/test_rls_hooks/test_rls_hooks.control
@@ -0,0 +1,4 @@
+comment = 'Test code for RLS hooks'
+default_version = '1.0'
+module_pathname = '$libdir/test_rls_hooks'
+relocatable = true
diff --git a/src/test/modules/test_rls_hooks/test_rls_hooks.h b/src/test/modules/test_rls_hooks/test_rls_hooks.h
new file mode 100644
index 0000000..c8a0330
--- /dev/null
+++ b/src/test/modules/test_rls_hooks/test_rls_hooks.h
@@ -0,0 +1,25 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_rls_hooks.h
+ *		Definitions for RLS hooks
+ *
+ * Copyright (C) 2015, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		src/test/modules/test_rls_hooks/test_rls_hooks.h
+ *
+ * -------------------------------------------------------------------------
+ */
+
+#ifndef TEST_RLS_HOOKS_H
+#define TEST_RLS_HOOKS_H
+
+#include <rewrite/rowsecurity.h>
+
+/* Return set of permissive hooks based on CmdType and Relation */
+extern List *test_rls_hooks_permissive(CmdType cmdtype, Relation relation);
+
+/* Return set of restrictive hooks based on CmdType and Relation */
+extern List *test_rls_hooks_restrictive(CmdType cmdtype, Relation relation);
+
+#endif
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 44e8dab..5676079 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -466,9 +466,11 @@ ALTER TABLE t1 DROP COLUMN junk1;    -- just a disturbing factor
 GRANT ALL ON t1 TO public;
 COPY t1 FROM stdin WITH (oids);
 CREATE TABLE t2 (c float) INHERITS (t1);
+GRANT ALL ON t2 TO public;
 COPY t2 FROM stdin WITH (oids);
 CREATE TABLE t3 (c text, b text, a int) WITH OIDS;
 ALTER TABLE t3 INHERIT t1;
+GRANT ALL ON t3 TO public;
 COPY t3(a,b,c) FROM stdin WITH (oids);
 CREATE POLICY p1 ON t1 FOR ALL TO PUBLIC USING (a % 2 = 0); -- be even number
 CREATE POLICY p2 ON t2 FOR ALL TO PUBLIC USING (a % 2 = 1); -- be odd number
@@ -1117,22 +1119,216 @@ NOTICE:  f_leak => yyyyyy
  302 | 2 | yyyyyy      | (2,yyyyyy)
 (5 rows)
 
+-- updates with from clause
+EXPLAIN (COSTS OFF) UPDATE t2 SET b=t2.b FROM t3
+WHERE t2.a = 3 and t3.a = 2 AND f_leak(t2.b) AND f_leak(t3.b);
+                          QUERY PLAN                           
+---------------------------------------------------------------
+ Update on t2 t2_1
+   ->  Nested Loop
+         ->  Subquery Scan on t2
+               Filter: f_leak(t2.b)
+               ->  LockRows
+                     ->  Seq Scan on t2 t2_2
+                           Filter: ((a = 3) AND ((a % 2) = 1))
+         ->  Seq Scan on t3
+               Filter: (f_leak(b) AND (a = 2))
+(9 rows)
+
+UPDATE t2 SET b=t2.b FROM t3
+WHERE t2.a = 3 and t3.a = 2 AND f_leak(t2.b) AND f_leak(t3.b);
+NOTICE:  f_leak => cde
+NOTICE:  f_leak => xxx
+NOTICE:  f_leak => zzz
+NOTICE:  f_leak => yyyyyy
+EXPLAIN (COSTS OFF) UPDATE t1 SET b=t1.b FROM t2
+WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+                          QUERY PLAN                           
+---------------------------------------------------------------
+ Update on t1 t1_3
+   Update on t1 t1_3
+   Update on t2 t1
+   Update on t3 t1
+   ->  Nested Loop
+         ->  Subquery Scan on t1
+               Filter: f_leak(t1.b)
+               ->  LockRows
+                     ->  Seq Scan on t1 t1_4
+                           Filter: ((a = 3) AND ((a % 2) = 0))
+         ->  Subquery Scan on t2
+               Filter: f_leak(t2.b)
+               ->  Seq Scan on t2 t2_3
+                     Filter: ((a = 3) AND ((a % 2) = 1))
+   ->  Nested Loop
+         ->  Subquery Scan on t1_1
+               Filter: f_leak(t1_1.b)
+               ->  LockRows
+                     ->  Seq Scan on t2 t2_4
+                           Filter: ((a = 3) AND ((a % 2) = 0))
+         ->  Subquery Scan on t2_1
+               Filter: f_leak(t2_1.b)
+               ->  Seq Scan on t2 t2_5
+                     Filter: ((a = 3) AND ((a % 2) = 1))
+   ->  Nested Loop
+         ->  Subquery Scan on t1_2
+               Filter: f_leak(t1_2.b)
+               ->  LockRows
+                     ->  Seq Scan on t3
+                           Filter: ((a = 3) AND ((a % 2) = 0))
+         ->  Subquery Scan on t2_2
+               Filter: f_leak(t2_2.b)
+               ->  Seq Scan on t2 t2_6
+                     Filter: ((a = 3) AND ((a % 2) = 1))
+(34 rows)
+
+UPDATE t1 SET b=t1.b FROM t2
+WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+EXPLAIN (COSTS OFF) UPDATE t2 SET b=t2.b FROM t1
+WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+                             QUERY PLAN                              
+---------------------------------------------------------------------
+ Update on t2 t2_1
+   ->  Nested Loop
+         ->  Subquery Scan on t2
+               Filter: f_leak(t2.b)
+               ->  LockRows
+                     ->  Seq Scan on t2 t2_2
+                           Filter: ((a = 3) AND ((a % 2) = 1))
+         ->  Subquery Scan on t1
+               Filter: f_leak(t1.b)
+               ->  Result
+                     ->  Append
+                           ->  Seq Scan on t1 t1_1
+                                 Filter: ((a = 3) AND ((a % 2) = 0))
+                           ->  Seq Scan on t2 t2_3
+                                 Filter: ((a = 3) AND ((a % 2) = 0))
+                           ->  Seq Scan on t3
+                                 Filter: ((a = 3) AND ((a % 2) = 0))
+(17 rows)
+
+UPDATE t2 SET b=t2.b FROM t1
+WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+NOTICE:  f_leak => cde
+-- updates with from clause self join
+EXPLAIN (COSTS OFF) UPDATE t2 t2_1 SET b = t2_2.b FROM t2 t2_2
+WHERE t2_1.a = 3 AND t2_2.a = t2_1.a AND t2_2.b = t2_1.b
+AND f_leak(t2_1.b) AND f_leak(t2_2.b) RETURNING *, t2_1, t2_2;
+                          QUERY PLAN                           
+---------------------------------------------------------------
+ Update on t2 t2_1_1
+   ->  Nested Loop
+         Join Filter: (t2_1.b = t2_2.b)
+         ->  Subquery Scan on t2_1
+               Filter: f_leak(t2_1.b)
+               ->  LockRows
+                     ->  Seq Scan on t2 t2_1_2
+                           Filter: ((a = 3) AND ((a % 2) = 1))
+         ->  Subquery Scan on t2_2
+               Filter: f_leak(t2_2.b)
+               ->  Seq Scan on t2 t2_2_1
+                     Filter: ((a = 3) AND ((a % 2) = 1))
+(12 rows)
+
+UPDATE t2 t2_1 SET b = t2_2.b FROM t2 t2_2
+WHERE t2_1.a = 3 AND t2_2.a = t2_1.a AND t2_2.b = t2_1.b
+AND f_leak(t2_1.b) AND f_leak(t2_2.b) RETURNING *, t2_1, t2_2;
+NOTICE:  f_leak => cde
+NOTICE:  f_leak => cde
+ a |  b  |  c  | a |  b  |  c  |    t2_1     |    t2_2     
+---+-----+-----+---+-----+-----+-------------+-------------
+ 3 | cde | 3.3 | 3 | cde | 3.3 | (3,cde,3.3) | (3,cde,3.3)
+(1 row)
+
+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
+   Update on t1 t1_1_3
+   Update on t2 t1_1
+   Update on t3 t1_1
+   ->  Nested Loop
+         Join Filter: (t1_1.b = t1_2.b)
+         ->  Subquery Scan on t1_1
+               Filter: f_leak(t1_1.b)
+               ->  LockRows
+                     ->  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)
+               ->  LockRows
+                     ->  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)
+               ->  LockRows
+                     ->  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))
+(52 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;
+SELECT * FROM t1 ORDER BY a,b;
  a |      b      
 ---+-------------
  1 | aaa
- 3 | ccc
- 2 | bbbbbb_updt
- 4 | dddddd_updt
  1 | abc
- 3 | cde
- 2 | bcdbcd
- 4 | defdef
  1 | xxx
- 3 | zzz
+ 2 | bbbbbb_updt
+ 2 | bcdbcd
  2 | yyyyyy
+ 3 | ccc
+ 3 | cde
+ 3 | zzz
+ 4 | dddddd_updt
+ 4 | defdef
 (11 rows)
 
 SET SESSION AUTHORIZATION rls_regress_user1;
@@ -1193,6 +1389,103 @@ NOTICE:  f_leak => yyyyyy
 (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"
+INSERT INTO bv1 VALUES (11, 'xxx'); -- should fail RLS check
+ERROR:  new row violates WITH CHECK OPTION for "b1"
+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)
+         ->  Subquery Scan on b1_2
+               ->  LockRows
+                     ->  Seq Scan on b1 b1_3
+                           Filter: ((a > 0) AND (a = 4) AND ((a % 2) = 0))
+(7 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)
+         ->  Subquery Scan on b1_2
+               ->  LockRows
+                     ->  Seq Scan on b1 b1_3
+                           Filter: ((a > 0) AND (a = 6) AND ((a % 2) = 0))
+(7 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
index ed7adbf..4da5035 100644
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -207,6 +207,8 @@ COPY t1 FROM stdin WITH (oids);
 \.
 
 CREATE TABLE t2 (c float) INHERITS (t1);
+GRANT ALL ON t2 TO public;
+
 COPY t2 FROM stdin WITH (oids);
 201	1	abc	1.1
 202	2	bcd	2.2
@@ -216,6 +218,8 @@ COPY t2 FROM stdin WITH (oids);
 
 CREATE TABLE t3 (c text, b text, a int) WITH OIDS;
 ALTER TABLE t3 INHERIT t1;
+GRANT ALL ON t3 TO public;
+
 COPY t3(a,b,c) FROM stdin WITH (oids);
 301	1	xxx	X
 302	2	yyy	Y
@@ -423,9 +427,45 @@ UPDATE only t1 SET b = b WHERE f_leak(b) RETURNING oid, *, t1;
 UPDATE t1 SET b = b WHERE f_leak(b) RETURNING *;
 UPDATE t1 SET b = b WHERE f_leak(b) RETURNING oid, *, t1;
 
+-- updates with from clause
+EXPLAIN (COSTS OFF) UPDATE t2 SET b=t2.b FROM t3
+WHERE t2.a = 3 and t3.a = 2 AND f_leak(t2.b) AND f_leak(t3.b);
+
+UPDATE t2 SET b=t2.b FROM t3
+WHERE t2.a = 3 and t3.a = 2 AND f_leak(t2.b) AND f_leak(t3.b);
+
+EXPLAIN (COSTS OFF) UPDATE t1 SET b=t1.b FROM t2
+WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+
+UPDATE t1 SET b=t1.b FROM t2
+WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+
+EXPLAIN (COSTS OFF) UPDATE t2 SET b=t2.b FROM t1
+WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+
+UPDATE t2 SET b=t2.b FROM t1
+WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
+
+-- updates with from clause self join
+EXPLAIN (COSTS OFF) UPDATE t2 t2_1 SET b = t2_2.b FROM t2 t2_2
+WHERE t2_1.a = 3 AND t2_2.a = t2_1.a AND t2_2.b = t2_1.b
+AND f_leak(t2_1.b) AND f_leak(t2_2.b) RETURNING *, t2_1, t2_2;
+
+UPDATE t2 t2_1 SET b = t2_2.b FROM t2 t2_2
+WHERE t2_1.a = 3 AND t2_2.a = t2_1.a AND t2_2.b = t2_1.b
+AND f_leak(t2_1.b) AND f_leak(t2_2.b) RETURNING *, t2_1, t2_2;
+
+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;
+SELECT * FROM t1 ORDER BY a,b;
 
 SET SESSION AUTHORIZATION rls_regress_user1;
 SET row_security TO ON;
@@ -436,6 +476,39 @@ DELETE FROM only t1 WHERE f_leak(b) RETURNING oid, *, t1;
 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;
-- 
1.9.1

#16Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Stephen Frost (#15)
Re: Row security violation error is misleading

On 21 April 2015 at 20:50, Stephen Frost <sfrost@snowman.net> wrote:

Thanks a lot for this. Please take a look at the attached.

I've given this a quick read-through, and it looks good to me. The
interaction of permissive and restrictive policies from hooks matches
my expections, and it's a definite improvement having tests for RLS
hooks.

The only thing I spotted was that the file comment for
test_rls_hooks.c needs updating.

Is there any documentation for hooks? If not, perhaps that's something
we should be considering too.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#17Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Dean Rasheed (#16)
Re: Row security violation error is misleading

On 21 April 2015 at 22:21, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

On 21 April 2015 at 20:50, Stephen Frost <sfrost@snowman.net> wrote:

Thanks a lot for this. Please take a look at the attached.

I've given this a quick read-through, and it looks good to me. The
interaction of permissive and restrictive policies from hooks matches
my expections, and it's a definite improvement having tests for RLS
hooks.

The only thing I spotted was that the file comment for
test_rls_hooks.c needs updating.

So re-reading this, I spotted what looks like another (pre-existing)
bug. In process_policies() there's a loop over all the policies,
collecting quals and with_check_quals, then a test at the end to use
the USING quals for the WITH CHECK quals if there were no
with_check_quals. I think we want to instead do that test inside the
loop -- i.e., for each policy, if there is no with_check_qual *for
that policy*, use it's USING qual instead.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#18Stephen Frost
sfrost@snowman.net
In reply to: Dean Rasheed (#17)
Re: Row security violation error is misleading

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

On 21 April 2015 at 22:21, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

On 21 April 2015 at 20:50, Stephen Frost <sfrost@snowman.net> wrote:

Thanks a lot for this. Please take a look at the attached.

I've given this a quick read-through, and it looks good to me. The
interaction of permissive and restrictive policies from hooks matches
my expections, and it's a definite improvement having tests for RLS
hooks.

The only thing I spotted was that the file comment for
test_rls_hooks.c needs updating.

So re-reading this, I spotted what looks like another (pre-existing)
bug. In process_policies() there's a loop over all the policies,
collecting quals and with_check_quals, then a test at the end to use
the USING quals for the WITH CHECK quals if there were no
with_check_quals. I think we want to instead do that test inside the
loop -- i.e., for each policy, if there is no with_check_qual *for
that policy*, use it's USING qual instead.

Agreed, the USING -> WITH CHECK copy should be happening for all
policies independently, not wholesale at the end.

I've updated my tree and am testing.

Thanks!

Stephen

#19Stephen Frost
sfrost@snowman.net
In reply to: Dean Rasheed (#17)
Re: Row security violation error is misleading

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

On 21 April 2015 at 22:21, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

On 21 April 2015 at 20:50, Stephen Frost <sfrost@snowman.net> wrote:

Thanks a lot for this. Please take a look at the attached.

I've given this a quick read-through, and it looks good to me. The
interaction of permissive and restrictive policies from hooks matches
my expections, and it's a definite improvement having tests for RLS
hooks.

The only thing I spotted was that the file comment for
test_rls_hooks.c needs updating.

So re-reading this, I spotted what looks like another (pre-existing)
bug. In process_policies() there's a loop over all the policies,
collecting quals and with_check_quals, then a test at the end to use
the USING quals for the WITH CHECK quals if there were no
with_check_quals. I think we want to instead do that test inside the
loop -- i.e., for each policy, if there is no with_check_qual *for
that policy*, use it's USING qual instead.

Pushed with those changes, please take a look and test!

Thanks again for all of your help with this. I'm going to be looking
over that second patch with an eye towards getting it in very soon, it's
been kicking around for far longer than it should have been and that's
my fault, apologies about that.

Stephen

#20Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Stephen Frost (#19)
Re: Row security violation error is misleading

On 22 April 2015 at 17:02, Stephen Frost <sfrost@snowman.net> wrote:

Pushed with those changes, please take a look and test!

Excellent, thanks! Will test.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#21Stephen Frost
sfrost@snowman.net
In reply to: Dean Rasheed (#16)
Re: Row security violation error is misleading

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

On 21 April 2015 at 20:50, Stephen Frost <sfrost@snowman.net> wrote:

Thanks a lot for this. Please take a look at the attached.

I've given this a quick read-through, and it looks good to me. The
interaction of permissive and restrictive policies from hooks matches
my expections, and it's a definite improvement having tests for RLS
hooks.

The only thing I spotted was that the file comment for
test_rls_hooks.c needs updating.

Fixed!

Is there any documentation for hooks? If not, perhaps that's something
we should be considering too.

We don't generally document hooks beyond in the source code.. I'm happy
to expand on what's there, if anyone feels it'd be helpful to do so.
Having the test module is a form of documentation also, of course.

Thanks!

Stephen

#22Stephen Frost
sfrost@snowman.net
In reply to: Dean Rasheed (#5)
Re: Row security violation error is misleading

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

The second patch [2] is the one that is actually relevant to this
thread. This patch is primarily to apply the RLS checks earlier,
before an update is attempted, more like a regular permissions check.
This adds a new enum to classify the kinds of WCO, a side benefit of
which is that it allows different error messages when RLS checks are
violated, as opposed to WITH CHECK OPTIONs on views.

I've gone ahead and pushed this, please take a look and test it and let
me know if you see any issues or have any questions or concerns.

Thanks!

Stephen

#23Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Stephen Frost (#22)
Re: Row security violation error is misleading

On 25 April 2015 at 01:52, Stephen Frost <sfrost@snowman.net> wrote:

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

The second patch [2] is the one that is actually relevant to this
thread. This patch is primarily to apply the RLS checks earlier,
before an update is attempted, more like a regular permissions check.
This adds a new enum to classify the kinds of WCO, a side benefit of
which is that it allows different error messages when RLS checks are
violated, as opposed to WITH CHECK OPTIONs on views.

I've gone ahead and pushed this, please take a look and test it and let
me know if you see any issues or have any questions or concerns.

Brilliant, thanks! I gave it a quick read-through and all the changes
look good, so thanks for all your work on this.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers