From 456255730ab590b0826197856ab1600c443e6259 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Tue, 6 Jan 2015 16:32:21 -0800 Subject: [PATCH 5/8] RLS support for ON CONFLICT UPDATE Row-Level Security policies may apply to UPDATE commands or INSERT commands only. UPDATE RLS policies can have both USING() security barrier quals, and CHECK options (INSERT RLS policies may only have CHECK options, though). It is necessary to carefully consider the behavior of RLS policies in the context of INSERT with ON CONFLICT UPDATE, since ON CONFLICT UPDATE is more or less a new top-level command, conceptually quite different to two separate statements (an INSERT and an UPDATE). The approach taken is to "bunch together" both sets of policies, and to enforce them in 3 different places against three different slots (3 different stages of query processing in the executor). Note that UPDATE policy USING() barrier quals are always treated as CHECK options. It is thought that silently failing when USING() barrier quals are not satisfied is a more surprising outcome, even if it is closer to the existing behavior of UPDATE statements. This is because the user's intent to UPDATE one particular row based on simple criteria is quite clear with ON CONFLICT UPDATE. The 3 places that RLS policies are enforced are: * Against row actually inserted, after insertion proceeds successfully (INSERT-applicable policies only). * Against row in target table that caused conflict. The implementation is careful not to leak the contents of that row in diagnostic messages (INSERT-applicable *and* UPDATE-applicable policies). * Against the version of the row added by to the relation after ExecUpdate() is called (INSERT-applicable *and* UPDATE-applicable policies). Documentation and tests follow in later commits. --- src/backend/executor/execMain.c | 25 ++++++--- src/backend/executor/nodeModifyTable.c | 53 ++++++++++++++++++- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/equalfuncs.c | 1 + src/backend/nodes/outfuncs.c | 1 + src/backend/nodes/readfuncs.c | 1 + src/backend/rewrite/rewriteHandler.c | 2 + src/backend/rewrite/rowsecurity.c | 94 +++++++++++++++++++++++++++++----- src/include/executor/executor.h | 3 +- src/include/nodes/parsenodes.h | 1 + 10 files changed, 158 insertions(+), 24 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 36251f0..53cecd7 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1709,7 +1709,8 @@ ExecConstraints(ResultRelInfo *resultRelInfo, */ void ExecWithCheckOptions(ResultRelInfo *resultRelInfo, - TupleTableSlot *slot, EState *estate) + TupleTableSlot *slot, bool detail, + bool onlyInsert, EState *estate) { Relation rel = resultRelInfo->ri_RelationDesc; TupleDesc tupdesc = RelationGetDescr(rel); @@ -1734,6 +1735,15 @@ ExecWithCheckOptions(ResultRelInfo *resultRelInfo, ExprState *wcoExpr = (ExprState *) lfirst(l2); /* + * INSERT ... ON CONFLICT UPDATE callers may require that not all WITH + * CHECK OPTIONs associated with resultRelInfo are enforced at all + * stages of query processing. (UPDATE-related policies are not + * enforced in respect of a successfully inserted tuple). + */ + if (onlyInsert && wco->commandType == CMD_UPDATE) + continue; + + /* * WITH CHECK OPTION checks are intended to ensure that the new tuple * is visible (in the case of a view) or that it passes the * 'with-check' policy (in the case of row security). @@ -1744,16 +1754,17 @@ ExecWithCheckOptions(ResultRelInfo *resultRelInfo, */ if (!ExecQual((List *) wcoExpr, econtext, false)) { - char *val_desc; + char *val_desc = NULL; Bitmapset *modifiedCols; modifiedCols = GetUpdatedColumns(resultRelInfo, estate); modifiedCols = bms_union(modifiedCols, GetInsertedColumns(resultRelInfo, estate)); - val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel), - slot, - tupdesc, - modifiedCols, - 64); + if (detail) + val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel), + slot, + tupdesc, + modifiedCols, + 64); ereport(ERROR, (errcode(ERRCODE_WITH_CHECK_OPTION_VIOLATION), diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 1603c45..90236ce 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -458,7 +458,8 @@ vlock: /* Check any WITH CHECK OPTION constraints */ if (resultRelInfo->ri_WithCheckOptions != NIL) - ExecWithCheckOptions(resultRelInfo, slot, estate); + ExecWithCheckOptions(resultRelInfo, slot, true, spec == SPEC_INSERT, + estate); /* Process RETURNING if present */ if (resultRelInfo->ri_projectReturning) @@ -952,7 +953,7 @@ lreplace:; /* Check any WITH CHECK OPTION constraints */ if (resultRelInfo->ri_WithCheckOptions != NIL) - ExecWithCheckOptions(resultRelInfo, slot, estate); + ExecWithCheckOptions(resultRelInfo, slot, true, false, estate); /* Process RETURNING if present */ if (resultRelInfo->ri_projectReturning) @@ -1148,6 +1149,54 @@ ExecLockUpdateTuple(ResultRelInfo *resultRelInfo, slot = EvalPlanQualNext(&onConflict->mt_epqstate); + /* + * For RLS with ON CONFLICT UPDATE, security quals are always + * treated as WITH CHECK options, even when there were separate + * security quals and explicit WITH CHECK options (ordinarily, + * security quals are only treated as WITH CHECK options when there + * are no explicit WITH CHECK options). Also, CHECK OPTIONs + * (originating either explicitly, or implicitly as security quals) + * for both UPDATE and INSERT policies (or ALL policies) are + * checked (as CHECK OPTIONs) at three different points for three + * distinct but related tuples/slots in the context of ON CONFLICT + * UPDATE. There are three relevant ExecWithCheckOptions() calls: + * + * * After successful insertion, within ExecInsert(), against the + * inserted tuple. This only includes INSERT-applicable policies. + * + * * Here, after row locking but before calling ExecUpdate(), on + * the existing tuple in the target relation (which we cannot leak + * details of). This is conceptually like a security barrier qual + * for the purposes of the auxiliary update, although unlike + * regular updates that require security barrier quals we prefer to + * raise an error (by treating the security barrier quals as CHECK + * OPTIONS) rather than silently not affect rows, because the + * intent to update seems clear and unambiguous for ON CONFLICT + * UPDATE. This includes both INSERT-applicable and + * UPDATE-applicable policies. + * + * * On the final tuple created by the update within ExecUpdate (if + * any). This is also subject to INSERT policy enforcement, unlike + * conventional ExecUpdate() calls for UPDATE statements -- it + * includes both INSERT-applicable and UPDATE-applicable policies. + */ + if (resultRelInfo->ri_WithCheckOptions != NIL) + { + TupleTableSlot *opts; + + /* Construct temp slot for locked tuple from target */ + opts = MakeSingleTupleTableSlot(slot->tts_tupleDescriptor); + ExecStoreTuple(copyTuple, opts, InvalidBuffer, false); + + /* + * Check, but without leaking contents of tuple; user only + * supplied one conflicting value or composition of values, and + * not the entire tuple. + */ + ExecWithCheckOptions(resultRelInfo, opts, false, false, + estate); + } + if (!TupIsNull(slot)) *returning = ExecUpdate(&tuple.t_data->t_ctid, NULL, slot, planSlot, &onConflict->mt_epqstate, diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index df611d2..5c091e1 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -2074,6 +2074,7 @@ _copyWithCheckOption(const WithCheckOption *from) COPY_STRING_FIELD(viewname); COPY_NODE_FIELD(qual); + COPY_SCALAR_FIELD(commandType); COPY_SCALAR_FIELD(cascaded); return newnode; diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index 24e58fa..4057c27 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -2384,6 +2384,7 @@ _equalWithCheckOption(const WithCheckOption *a, const WithCheckOption *b) { COMPARE_STRING_FIELD(viewname); COMPARE_NODE_FIELD(qual); + COMPARE_SCALAR_FIELD(commandType); COMPARE_SCALAR_FIELD(cascaded); return true; diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 34e9163..d077882 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -2336,6 +2336,7 @@ _outWithCheckOption(StringInfo str, const WithCheckOption *node) WRITE_STRING_FIELD(viewname); WRITE_NODE_FIELD(qual); + WRITE_ENUM_FIELD(commandType, CmdType); WRITE_BOOL_FIELD(cascaded); } diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index b471bbf..30b0eca 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -272,6 +272,7 @@ _readWithCheckOption(void) READ_STRING_FIELD(viewname); READ_NODE_FIELD(qual); + READ_ENUM_FIELD(commandType, CmdType); READ_BOOL_FIELD(cascaded); READ_DONE(); diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index f37760b..a2cc4f3 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -1767,6 +1767,7 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown) List *quals = NIL; wco = (WithCheckOption *) makeNode(WithCheckOption); + wco->commandType = parsetree->commandType; quals = lcons(wco->qual, quals); activeRIRs = lcons_oid(RelationGetRelid(rel), activeRIRs); @@ -2935,6 +2936,7 @@ rewriteTargetView(Query *parsetree, Relation view) wco->viewname = pstrdup(RelationGetRelationName(view)); wco->qual = NULL; wco->cascaded = cascaded; + wco->commandType = viewquery->commandType; parsetree->withCheckOptions = lcons(wco, parsetree->withCheckOptions); diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c index 7669130..09f1ac3 100644 --- a/src/backend/rewrite/rowsecurity.c +++ b/src/backend/rewrite/rowsecurity.c @@ -56,12 +56,14 @@ #include "utils/syscache.h" #include "tcop/utility.h" -static List *pull_row_security_policies(CmdType cmd, Relation relation, - Oid user_id); +static List *pull_row_security_policies(CmdType cmd, bool onConflict, + Relation relation, Oid user_id); static void process_policies(List *policies, int rt_index, Expr **final_qual, Expr **final_with_check_qual, - bool *hassublinks); + bool *hassublinks, + Expr **spec_with_check_eval, + bool onConflict); static bool check_role_for_policy(ArrayType *policy_roles, Oid user_id); /* @@ -88,6 +90,7 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index) Expr *rowsec_with_check_expr = NULL; Expr *hook_expr = NULL; Expr *hook_with_check_expr = NULL; + Expr *hook_spec_with_check_expr = NULL; List *rowsec_policies; List *hook_policies = NIL; @@ -149,8 +152,9 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index) /* Grab the built-in policies which should be applied to this relation. */ rel = heap_open(rte->relid, NoLock); - rowsec_policies = pull_row_security_policies(root->commandType, rel, - user_id); + rowsec_policies = pull_row_security_policies(root->commandType, + root->specClause == SPEC_INSERT, + rel, user_id); /* * Check if this is only the default-deny policy. @@ -168,7 +172,9 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index) /* Now that we have our policies, build the expressions from them. */ process_policies(rowsec_policies, rt_index, &rowsec_expr, - &rowsec_with_check_expr, &hassublinks); + &rowsec_with_check_expr, &hassublinks, + &hook_spec_with_check_expr, + root->specClause == SPEC_INSERT); /* * Also, allow extensions to add their own policies. @@ -198,7 +204,9 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index) /* Build the expression from any policies returned. */ process_policies(hook_policies, rt_index, &hook_expr, - &hook_with_check_expr, &hassublinks); + &hook_with_check_expr, &hassublinks, + &hook_spec_with_check_expr, + root->specClause == SPEC_INSERT); } /* @@ -230,6 +238,7 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index) wco->viewname = RelationGetRelationName(rel); wco->qual = (Node *) rowsec_with_check_expr; wco->cascaded = false; + wco->commandType = root->commandType; root->withCheckOptions = lcons(wco, root->withCheckOptions); } @@ -244,6 +253,23 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index) wco->viewname = RelationGetRelationName(rel); wco->qual = (Node *) hook_with_check_expr; wco->cascaded = false; + wco->commandType = root->commandType; + root->withCheckOptions = lcons(wco, root->withCheckOptions); + } + + /* + * Also add the expression, if any, returned from the extension that + * applies to auxiliary UPDATE within ON CONFLICT UPDATE. + */ + if (hook_spec_with_check_expr) + { + WithCheckOption *wco; + + wco = (WithCheckOption *) makeNode(WithCheckOption); + wco->viewname = RelationGetRelationName(rel); + wco->qual = (Node *) hook_spec_with_check_expr; + wco->cascaded = false; + wco->commandType = CMD_UPDATE; root->withCheckOptions = lcons(wco, root->withCheckOptions); } } @@ -288,7 +314,8 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index) * */ static List * -pull_row_security_policies(CmdType cmd, Relation relation, Oid user_id) +pull_row_security_policies(CmdType cmd, bool onConflict, Relation relation, + Oid user_id) { List *policies = NIL; ListCell *item; @@ -322,7 +349,9 @@ pull_row_security_policies(CmdType cmd, Relation relation, Oid user_id) if (policy->polcmd == ACL_INSERT_CHR && check_role_for_policy(policy->roles, user_id)) policies = lcons(policy, policies); - break; + if (!onConflict) + break; + /* FALL THRU */ case CMD_UPDATE: if (policy->polcmd == ACL_UPDATE_CHR && check_role_for_policy(policy->roles, user_id)) @@ -384,26 +413,41 @@ pull_row_security_policies(CmdType cmd, Relation relation, Oid user_id) */ static void process_policies(List *policies, int rt_index, Expr **qual_eval, - Expr **with_check_eval, bool *hassublinks) + Expr **with_check_eval, bool *hassublinks, + Expr **spec_with_check_eval, bool onConflict) { ListCell *item; List *quals = NIL; List *with_check_quals = NIL; + List *conflict_update_quals = NIL; /* * Extract the USING and WITH CHECK quals from each of the policies - * and add them to our lists. + * and add them to our lists. CONFLICT UPDATE quals are always treated + * as CHECK OPTIONS. */ foreach(item, policies) { RowSecurityPolicy *policy = (RowSecurityPolicy *) lfirst(item); if (policy->qual != NULL) - quals = lcons(copyObject(policy->qual), quals); + { + if (!onConflict || policy->polcmd != ACL_UPDATE_CHR) + quals = lcons(copyObject(policy->qual), quals); + else + conflict_update_quals = lcons(copyObject(policy->qual), quals); + } if (policy->with_check_qual != NULL) - with_check_quals = lcons(copyObject(policy->with_check_qual), - with_check_quals); + { + if (!onConflict || policy->polcmd != ACL_UPDATE_CHR) + with_check_quals = lcons(copyObject(policy->with_check_qual), + with_check_quals); + else + conflict_update_quals = + lcons(copyObject(policy->with_check_qual), + conflict_update_quals); + } if (policy->hassublinks) *hassublinks = true; @@ -420,6 +464,10 @@ 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. + * + * For the INSERT with ON CONFLICT UPDATE case, we always enforce that the + * UPDATE's USING quals are treated like WITH CHECK quals, enforced against + * the target relation's tuple in multiple places. */ if (with_check_quals == NIL) with_check_quals = copyObject(quals); @@ -453,6 +501,24 @@ process_policies(List *policies, int rt_index, Expr **qual_eval, else *with_check_eval = (Expr*) linitial(with_check_quals); + /* + * For INSERT with ON CONFLICT UPDATE, *both* sets of WITH CHECK options + * (from any INSERT policy and any UPDATE policy) are enforced. + * + * These are handled separately because enforcement of each type of WITH + * CHECK option is based on the point in query processing of INSERT ... ON + * CONFLICT UPDATE. The INSERT path does not enforce UPDATE related CHECK + * OPTIONs. + */ + if (conflict_update_quals != NIL) + { + if (list_length(conflict_update_quals) > 1) + *spec_with_check_eval = makeBoolExpr(AND_EXPR, + conflict_update_quals, -1); + else + *spec_with_check_eval = (Expr*) linitial(conflict_update_quals); + } + return; } diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index 9400801..6c535da 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -195,7 +195,8 @@ extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids); extern void ExecConstraints(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate); extern void ExecWithCheckOptions(ResultRelInfo *resultRelInfo, - TupleTableSlot *slot, EState *estate); + TupleTableSlot *slot, bool detail, bool onlyInsert, + EState *estate); extern ExecRowMark *ExecFindRowMark(EState *estate, Index rti); extern ExecAuxRowMark *ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist); extern TupleTableSlot *EvalPlanQual(EState *estate, EPQState *epqstate, diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 9ae3bb5..6447f45 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -868,6 +868,7 @@ typedef struct WithCheckOption NodeTag type; char *viewname; /* name of view that specified the WCO */ Node *qual; /* constraint qual to check */ + CmdType commandType; /* select|insert|update|delete */ bool cascaded; /* true = WITH CASCADED CHECK OPTION */ } WithCheckOption; -- 1.9.1