Cleanup of rewriter and planner handling of Query.hasRowSecurity flag.

Be sure to pull up the subquery's hasRowSecurity flag when flattening a
subquery in pull_up_simple_subquery().  This isn't a bug today because
we don't look at the hasRowSecurity flag during planning, but it could
easily be a bug tomorrow.

Likewise, make rewriteRuleAction() pull up the hasRowSecurity flag when
absorbing RTEs from a rule action.  This isn't a bug either, for the
opposite reason: the flag should never be set yet.  But again, it seems
like good future proofing.

Add a comment explaining why rewriteTargetView() should *not* set
hasRowSecurity when adding stuff to securityQuals.

Improve some nearby comments about securityQuals processing, and document
that field more completely in parsenodes.h.

Patch by me, analysis by Dean Rasheed.

Discussion: <CAEZATCXZ8tb2DV6f=bkhsMV6u_gRcZ0CZBw2J-qU84RxSukZog@mail.gmail.com>

diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 878db9b4ab76cb8ecb3bc587e5731e0583a78aac..b68123034983ca03f26c0dc0489886405768e976 100644 (file)
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -1187,6 +1187,9 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
         */
        parse->hasSubLinks |= subquery->hasSubLinks;
 
+       /* If subquery had any RLS conditions, now main query does too */
+       parse->hasRowSecurity |= subquery->hasRowSecurity;
+
        /*
         * subquery won't be pulled up if it hasAggs, hasWindowFuncs, or
         * hasTargetSRFs, so no work needed on those flags

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index b828e3cb0779f84dd5c9784c8862e52350bb733d..65c3d6e081405c42153b2064fec01b85bfe53bc7 100644 (file)
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -446,6 +446,15 @@ rewriteRuleAction(Query *parsetree,
                }
        }
 
+       /*
+        * Also, we might have absorbed some RTEs with RLS conditions into the
+        * sub_action.  If so, mark it as hasRowSecurity, whether or not those
+        * RTEs will be referenced after we finish rewriting.  (Note: currently
+        * this is a no-op because RLS conditions aren't added till later, but it
+        * seems like good future-proofing to do this anyway.)
+        */
+       sub_action->hasRowSecurity |= parsetree->hasRowSecurity;
+
        /*
         * Each rule action's jointree should be the main parsetree's jointree
         * plus that rule's jointree, but usually *without* the original rtindex
@@ -1835,10 +1844,10 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
                        }
 
                        /*
-                        * 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).
+                        * Add the new security barrier quals to the start of the RTE's
+                        * list so that they get applied before any existing barrier quals
+                        * (which would have come from a security-barrier view, and should
+                        * get lower priority than RLS conditions on the table itself).
                         */
                        rte->securityQuals = list_concat(securityQuals,
                                                                                         rte->securityQuals);
@@ -2957,9 +2966,9 @@ rewriteTargetView(Query *parsetree, Relation view)
         * only adjust their varnos to reference the new target (just the same as
         * we did with the view targetlist).
         *
-        * Note that there is special-case handling for the quals of a security
-        * barrier view, since they need to be kept separate from any
-        * user-supplied quals, so these quals are kept on the new target RTE.
+        * If it's a security-barrier view, its WHERE quals must be applied before
+        * quals from the outer query, so we attach them to the RTE as security
+        * barrier quals rather than adding them to the main WHERE clause.
         *
         * For INSERT, the view's quals can be ignored in the main query.
         */
@@ -2980,12 +2989,21 @@ rewriteTargetView(Query *parsetree, Relation view)
                if (RelationIsSecurityView(view))
                {
                        /*
+                        * The view's quals go in front of existing barrier quals: those
+                        * would have come from an outer level of security-barrier view,
+                        * and so must get evaluated later.
+                        *
                         * Note: the parsetree has been mutated, so the new_rte pointer is
                         * stale and needs to be re-computed.
                         */
                        new_rte = rt_fetch(new_rt_index, parsetree->rtable);
                        new_rte->securityQuals = lcons(viewqual, new_rte->securityQuals);
 
+                       /*
+                        * Do not set parsetree->hasRowSecurity, because these aren't RLS
+                        * conditions (they aren't affected by enabling/disabling RLS).
+                        */
+
                        /*
                         * Make sure that the query is marked correctly if the added qual
                         * has sublinks.

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 9b600a5f76df6fca566218ad3a76f477187330cc..04b1c2f2d436eddf94c64e2ef52135feb773b81f 100644 (file)
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -775,6 +775,13 @@ typedef struct XmlSerialize
  *       FirstLowInvalidHeapAttributeNumber from column numbers before storing
  *       them in these fields.  A whole-row Var reference is represented by
  *       setting the bit for InvalidAttrNumber.
+ *
+ *       securityQuals is a list of security barrier quals (boolean expressions),
+ *       to be tested in the listed order before returning a row from the
+ *       relation.  It is always NIL in parser output.  Entries are added by the
+ *       rewriter to implement security-barrier views and/or row-level security.
+ *       Note that the planner turns each boolean expression into an implicitly
+ *       AND'ed sublist, as is its usual habit with qualification expressions.
  *--------------------
  */
 typedef enum RTEKind
@@ -872,7 +879,7 @@ typedef struct RangeTblEntry
        Bitmapset  *selectedCols;       /* columns needing SELECT permission */
        Bitmapset  *insertedCols;       /* columns needing INSERT permission */
        Bitmapset  *updatedCols;        /* columns needing UPDATE permission */
-       List       *securityQuals;      /* any security barrier quals to apply */
+       List       *securityQuals;      /* security barrier quals to apply, if any */
 } RangeTblEntry;
 
 /*