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;
 
 /*