On 10 January 2015 at 15:12, Stephen Frost <sfr...@snowman.net> wrote: > * Dean Rasheed (dean.a.rash...@gmail.com) wrote: >> Currently we're applying RLS CHECKs after the INSERT or UPDATE, like >> WITH CHECK OPTIONs on views. The SQL spec says that WITH CHECK OPTIONs >> on views have to be applied after the INSERT/UPDATE on the base >> relation, but we're free to do something different for RLS CHECKs if >> that makes more sense. If we want RLS to be more like column-level >> privilege checking, then it does make sense to do the checks sooner, >> so perhaps we should be checking the RLS policies before the >> INSERT/UPDATE, like CHECK constraints. > > Were you thinking about working up a patch for such a change? If not, > I'll see about finding time to do it, unless someone else wants to > volunteer. :) >
Attached is a patch to make RLS checks run before attempting to insert/update any data rather than afterwards. In the end I decided not to create a new structure for RLS checks because most of the code that handles them treats them the same as WCOs. Instead, I just added a new 'kind' enum field to the existing structure and renamed/reworded things a bit. The patch also changes the error message for a RLS check violation, to make the cause of the error clearer. One thing I'm not sure about is what sqlstate code to use for this error, but I don't think that using WITH_CHECK_OPTION_VIOLATION is appropriate, because that seems to be specifically intended for views. Regards, Dean
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c new file mode 100644 index 5b70cc9..6762b63 *** a/src/backend/executor/execMain.c --- b/src/backend/executor/execMain.c *************** ExecConstraints(ResultRelInfo *resultRel *** 1638,1646 **** /* * ExecWithCheckOptions -- check that tuple satisfies any WITH CHECK OPTIONs */ void ! ExecWithCheckOptions(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate) { ExprContext *econtext; --- 1638,1647 ---- /* * ExecWithCheckOptions -- check that tuple satisfies any WITH CHECK OPTIONs + * of the specified kind. */ void ! ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate) { ExprContext *econtext; *************** ExecWithCheckOptions(ResultRelInfo *resu *** 1663,1668 **** --- 1664,1672 ---- WithCheckOption *wco = (WithCheckOption *) lfirst(l1); ExprState *wcoExpr = (ExprState *) lfirst(l2); + if (wco->kind != kind) + 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 *************** ExecWithCheckOptions(ResultRelInfo *resu *** 1673,1686 **** * case (the opposite of what we do above for CHECK constraints). */ if (!ExecQual((List *) wcoExpr, econtext, false)) ! ereport(ERROR, ! (errcode(ERRCODE_WITH_CHECK_OPTION_VIOLATION), ! errmsg("new row violates WITH CHECK OPTION for \"%s\"", ! wco->viewname), ! errdetail("Failing row contains %s.", ! ExecBuildSlotValueDescription(slot, ! RelationGetDescr(resultRelInfo->ri_RelationDesc), ! 64)))); } } --- 1677,1711 ---- * case (the opposite of what we do above for CHECK constraints). */ if (!ExecQual((List *) wcoExpr, econtext, false)) ! { ! switch (wco->kind) ! { ! case WCO_VIEW_CHECK: ! ereport(ERROR, ! (errcode(ERRCODE_WITH_CHECK_OPTION_VIOLATION), ! errmsg("new row violates WITH CHECK OPTION for \"%s\"", ! wco->relname), ! errdetail("Failing row contains %s.", ! ExecBuildSlotValueDescription(slot, ! RelationGetDescr(resultRelInfo->ri_RelationDesc), ! 64)))); ! break; ! case WCO_RLS_INSERT_CHECK: ! case WCO_RLS_UPDATE_CHECK: ! ereport(ERROR, ! (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), ! errmsg("new row violates row level security policy for \"%s\"", ! wco->relname), ! errdetail("Failing row contains %s.", ! ExecBuildSlotValueDescription(slot, ! RelationGetDescr(resultRelInfo->ri_RelationDesc), ! 64)))); ! break; ! default: ! elog(ERROR, "unrecognized WCO kind: %u", wco->kind); ! break; ! } ! } } } diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c new file mode 100644 index f96fb24..be879a4 *** a/src/backend/executor/nodeModifyTable.c --- b/src/backend/executor/nodeModifyTable.c *************** ExecInsert(TupleTableSlot *slot, *** 253,258 **** --- 253,265 ---- tuple->t_tableOid = RelationGetRelid(resultRelationDesc); /* + * Check any RLS INSERT WITH CHECK policies + */ + if (resultRelInfo->ri_WithCheckOptions != NIL) + ExecWithCheckOptions(WCO_RLS_INSERT_CHECK, + resultRelInfo, slot, estate); + + /* * Check the constraints of the tuple */ if (resultRelationDesc->rd_att->constr) *************** ExecInsert(TupleTableSlot *slot, *** 287,295 **** list_free(recheckIndexes); ! /* Check any WITH CHECK OPTION constraints */ if (resultRelInfo->ri_WithCheckOptions != NIL) ! ExecWithCheckOptions(resultRelInfo, slot, estate); /* Process RETURNING if present */ if (resultRelInfo->ri_projectReturning) --- 294,302 ---- list_free(recheckIndexes); ! /* Check any WITH CHECK OPTION constraints from parent views */ if (resultRelInfo->ri_WithCheckOptions != NIL) ! ExecWithCheckOptions(WCO_VIEW_CHECK, resultRelInfo, slot, estate); /* Process RETURNING if present */ if (resultRelInfo->ri_projectReturning) *************** ExecUpdate(ItemPointer tupleid, *** 653,667 **** tuple->t_tableOid = RelationGetRelid(resultRelationDesc); /* ! * Check the constraints of the tuple * * If we generate a new candidate tuple after EvalPlanQual testing, we ! * must loop back here and recheck constraints. (We don't need to ! * redo triggers, however. If there are any BEFORE triggers then ! * trigger.c will have done heap_lock_tuple to lock the correct tuple, ! * so there's no need to do them again.) */ lreplace:; if (resultRelationDesc->rd_att->constr) ExecConstraints(resultRelInfo, slot, estate); --- 660,681 ---- tuple->t_tableOid = RelationGetRelid(resultRelationDesc); /* ! * Check any RLS UPDATE WITH CHECK policies * * If we generate a new candidate tuple after EvalPlanQual testing, we ! * must loop back here and recheck any RLS policies and constraints. ! * (We don't need to redo triggers, however. If there are any BEFORE ! * triggers then trigger.c will have done heap_lock_tuple to lock the ! * correct tuple, so there's no need to do them again.) */ lreplace:; + if (resultRelInfo->ri_WithCheckOptions != NIL) + ExecWithCheckOptions(WCO_RLS_UPDATE_CHECK, + resultRelInfo, slot, estate); + + /* + * Check the constraints of the tuple + */ if (resultRelationDesc->rd_att->constr) ExecConstraints(resultRelInfo, slot, estate); *************** lreplace:; *** 780,788 **** list_free(recheckIndexes); ! /* Check any WITH CHECK OPTION constraints */ if (resultRelInfo->ri_WithCheckOptions != NIL) ! ExecWithCheckOptions(resultRelInfo, slot, estate); /* Process RETURNING if present */ if (resultRelInfo->ri_projectReturning) --- 794,802 ---- list_free(recheckIndexes); ! /* Check any WITH CHECK OPTION constraints from parent views */ if (resultRelInfo->ri_WithCheckOptions != NIL) ! ExecWithCheckOptions(WCO_VIEW_CHECK, resultRelInfo, slot, estate); /* Process RETURNING if present */ if (resultRelInfo->ri_projectReturning) diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c new file mode 100644 index f1a24f5..777df58 *** a/src/backend/nodes/copyfuncs.c --- b/src/backend/nodes/copyfuncs.c *************** _copyWithCheckOption(const WithCheckOpti *** 2055,2061 **** { WithCheckOption *newnode = makeNode(WithCheckOption); ! COPY_STRING_FIELD(viewname); COPY_NODE_FIELD(qual); COPY_SCALAR_FIELD(cascaded); --- 2055,2062 ---- { WithCheckOption *newnode = makeNode(WithCheckOption); ! COPY_SCALAR_FIELD(kind); ! COPY_STRING_FIELD(relname); COPY_NODE_FIELD(qual); COPY_SCALAR_FIELD(cascaded); diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c new file mode 100644 index 6e8b308..0b88c84 *** a/src/backend/nodes/equalfuncs.c --- b/src/backend/nodes/equalfuncs.c *************** _equalRangeTblFunction(const RangeTblFun *** 2368,2374 **** static bool _equalWithCheckOption(const WithCheckOption *a, const WithCheckOption *b) { ! COMPARE_STRING_FIELD(viewname); COMPARE_NODE_FIELD(qual); COMPARE_SCALAR_FIELD(cascaded); --- 2368,2375 ---- static bool _equalWithCheckOption(const WithCheckOption *a, const WithCheckOption *b) { ! COMPARE_SCALAR_FIELD(kind); ! COMPARE_STRING_FIELD(relname); COMPARE_NODE_FIELD(qual); COMPARE_SCALAR_FIELD(cascaded); diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c new file mode 100644 index dd1278b..f514388 *** a/src/backend/nodes/outfuncs.c --- b/src/backend/nodes/outfuncs.c *************** _outWithCheckOption(StringInfo str, cons *** 2319,2325 **** { WRITE_NODE_TYPE("WITHCHECKOPTION"); ! WRITE_STRING_FIELD(viewname); WRITE_NODE_FIELD(qual); WRITE_BOOL_FIELD(cascaded); } --- 2319,2326 ---- { WRITE_NODE_TYPE("WITHCHECKOPTION"); ! WRITE_ENUM_FIELD(kind, WCOKind); ! WRITE_STRING_FIELD(relname); WRITE_NODE_FIELD(qual); WRITE_BOOL_FIELD(cascaded); } diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c new file mode 100644 index ae24d05..86d1d1f *** a/src/backend/nodes/readfuncs.c --- b/src/backend/nodes/readfuncs.c *************** _readWithCheckOption(void) *** 266,272 **** { READ_LOCALS(WithCheckOption); ! READ_STRING_FIELD(viewname); READ_NODE_FIELD(qual); READ_BOOL_FIELD(cascaded); --- 266,273 ---- { READ_LOCALS(WithCheckOption); ! READ_ENUM_FIELD(kind, WCOKind); ! READ_STRING_FIELD(relname); READ_NODE_FIELD(qual); READ_BOOL_FIELD(cascaded); diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c new file mode 100644 index b8e6e7a..2ce7c23 *** a/src/backend/rewrite/rewriteHandler.c --- b/src/backend/rewrite/rewriteHandler.c *************** rewriteTargetView(Query *parsetree, Rela *** 2910,2916 **** WithCheckOption *wco; wco = makeNode(WithCheckOption); ! wco->viewname = pstrdup(RelationGetRelationName(view)); wco->qual = NULL; wco->cascaded = cascaded; --- 2910,2917 ---- WithCheckOption *wco; wco = makeNode(WithCheckOption); ! wco->kind = WCO_VIEW_CHECK; ! wco->relname = pstrdup(RelationGetRelationName(view)); wco->qual = NULL; wco->cascaded = cascaded; diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c new file mode 100644 index 35790a9..e55f8fe *** a/src/backend/rewrite/rowsecurity.c --- b/src/backend/rewrite/rowsecurity.c *************** prepend_row_security_policies(Query* roo *** 226,232 **** WithCheckOption *wco; wco = (WithCheckOption *) makeNode(WithCheckOption); ! wco->viewname = RelationGetRelationName(rel); wco->qual = (Node *) rowsec_with_check_expr; wco->cascaded = false; root->withCheckOptions = lcons(wco, root->withCheckOptions); --- 226,234 ---- WithCheckOption *wco; wco = (WithCheckOption *) makeNode(WithCheckOption); ! wco->kind = root->commandType == CMD_INSERT ? ! WCO_RLS_INSERT_CHECK : WCO_RLS_UPDATE_CHECK; ! wco->relname = RelationGetRelationName(rel); wco->qual = (Node *) rowsec_with_check_expr; wco->cascaded = false; root->withCheckOptions = lcons(wco, root->withCheckOptions); *************** prepend_row_security_policies(Query* roo *** 240,246 **** WithCheckOption *wco; wco = (WithCheckOption *) makeNode(WithCheckOption); ! wco->viewname = RelationGetRelationName(rel); wco->qual = (Node *) hook_with_check_expr; wco->cascaded = false; root->withCheckOptions = lcons(wco, root->withCheckOptions); --- 242,250 ---- WithCheckOption *wco; wco = (WithCheckOption *) makeNode(WithCheckOption); ! wco->kind = root->commandType == CMD_INSERT ? ! WCO_RLS_INSERT_CHECK : WCO_RLS_UPDATE_CHECK; ! wco->relname = RelationGetRelationName(rel); wco->qual = (Node *) hook_with_check_expr; wco->cascaded = false; root->withCheckOptions = lcons(wco, root->withCheckOptions); diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h new file mode 100644 index 40fde83..cb4f158 *** a/src/include/executor/executor.h --- b/src/include/executor/executor.h *************** extern ResultRelInfo *ExecGetTriggerResu *** 194,200 **** 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); extern ExecRowMark *ExecFindRowMark(EState *estate, Index rti); extern ExecAuxRowMark *ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist); --- 194,200 ---- extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids); extern void ExecConstraints(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate); ! extern void ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate); extern ExecRowMark *ExecFindRowMark(EState *estate, Index rti); extern ExecAuxRowMark *ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist); diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h new file mode 100644 index 41288ed..791343c *** a/src/include/nodes/execnodes.h --- b/src/include/nodes/execnodes.h *************** typedef struct JunkFilter *** 303,309 **** * TrigInstrument optional runtime measurements for triggers * FdwRoutine FDW callback functions, if foreign table * FdwState available to save private state of FDW ! * WithCheckOptions list of WithCheckOption's for views * WithCheckOptionExprs list of WithCheckOption expr states * ConstraintExprs array of constraint-checking expr states * junkFilter for removing junk attributes from tuples --- 303,309 ---- * TrigInstrument optional runtime measurements for triggers * FdwRoutine FDW callback functions, if foreign table * FdwState available to save private state of FDW ! * WithCheckOptions list of WithCheckOption's to be checked * WithCheckOptionExprs list of WithCheckOption expr states * ConstraintExprs array of constraint-checking expr states * junkFilter for removing junk attributes from tuples diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h new file mode 100644 index b1dfa85..3fadb7d *** a/src/include/nodes/parsenodes.h --- b/src/include/nodes/parsenodes.h *************** typedef struct RangeTblFunction *** 854,867 **** /* * WithCheckOption - * representation of WITH CHECK OPTION checks to be applied to new tuples ! * when inserting/updating an auto-updatable view. */ typedef struct WithCheckOption { NodeTag type; ! char *viewname; /* name of view that specified the WCO */ Node *qual; /* constraint qual to check */ ! bool cascaded; /* true = WITH CASCADED CHECK OPTION */ } WithCheckOption; /* --- 854,876 ---- /* * WithCheckOption - * representation of WITH CHECK OPTION checks to be applied to new tuples ! * when inserting/updating an auto-updatable view, or RLS WITH CHECK ! * policies to be applied when inserting/updating a relation with RLS. */ + typedef enum WCOKind + { + WCO_VIEW_CHECK, /* WCO on an auto-updatable view */ + WCO_RLS_INSERT_CHECK, /* RLS INSERT WITH CHECK policy */ + WCO_RLS_UPDATE_CHECK /* RLS UPDATE WITH CHECK policy */ + } WCOKind; + typedef struct WithCheckOption { NodeTag type; ! WCOKind kind; /* kind of WCO */ ! char *relname; /* name of relation that specified the WCO */ Node *qual; /* constraint qual to check */ ! bool cascaded; /* true for a cascaded WCO on a view */ } WithCheckOption; /* diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out new file mode 100644 index 430da55..e7777e6 *** a/src/test/regress/expected/rowsecurity.out --- b/src/test/regress/expected/rowsecurity.out *************** SELECT * FROM document WHERE did = 8; -- *** 301,306 **** --- 301,313 ---- -----+-----+--------+---------+-------- (0 rows) + -- RLS policies are checked before constraints + INSERT INTO document VALUES (8, 44, 1, 'rls_regress_user2', 'my third manga'); -- Should fail with RLS check violation, not duplicate key violation + ERROR: new row violates row level security policy for "document" + DETAIL: Failing row contains (8, 44, 1, rls_regress_user2, my third manga). + UPDATE document SET did = 8, dauthor = 'rls_regress_user2' WHERE did = 5; -- Should fail with RLS check violation, not duplicate key violation + ERROR: new row violates row level security policy for "document" + DETAIL: Failing row contains (8, 44, 2, rls_regress_user2, my second manga). -- database superuser does bypass RLS policy when enabled RESET SESSION AUTHORIZATION; SET row_security TO ON; *************** EXPLAIN (COSTS OFF) WITH cte1 AS (SELECT *** 1682,1688 **** (6 rows) WITH cte1 AS (UPDATE t1 SET a = a + 1 RETURNING *) SELECT * FROM cte1; --fail ! ERROR: new row violates WITH CHECK OPTION for "t1" DETAIL: Failing row contains (1, cfcd208495d565ef66e7dff9f98764da). WITH cte1 AS (UPDATE t1 SET a = a RETURNING *) SELECT * FROM cte1; --ok a | b --- 1689,1695 ---- (6 rows) WITH cte1 AS (UPDATE t1 SET a = a + 1 RETURNING *) SELECT * FROM cte1; --fail ! ERROR: new row violates row level security policy for "t1" DETAIL: Failing row contains (1, cfcd208495d565ef66e7dff9f98764da). WITH cte1 AS (UPDATE t1 SET a = a RETURNING *) SELECT * FROM cte1; --ok a | b *************** WITH cte1 AS (UPDATE t1 SET a = a RETURN *** 1701,1707 **** (11 rows) WITH cte1 AS (INSERT INTO t1 VALUES (21, 'Fail') RETURNING *) SELECT * FROM cte1; --fail ! ERROR: new row violates WITH CHECK OPTION for "t1" DETAIL: Failing row contains (21, Fail). WITH cte1 AS (INSERT INTO t1 VALUES (20, 'Success') RETURNING *) SELECT * FROM cte1; --ok a | b --- 1708,1714 ---- (11 rows) WITH cte1 AS (INSERT INTO t1 VALUES (21, 'Fail') RETURNING *) SELECT * FROM cte1; --fail ! ERROR: new row violates row level security policy for "t1" DETAIL: Failing row contains (21, Fail). WITH cte1 AS (INSERT INTO t1 VALUES (20, 'Success') RETURNING *) SELECT * FROM cte1; --ok a | b diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql new file mode 100644 index ee28a2c..9652dda *** a/src/test/regress/sql/rowsecurity.sql --- b/src/test/regress/sql/rowsecurity.sql *************** SET SESSION AUTHORIZATION rls_regress_us *** 146,151 **** --- 146,155 ---- INSERT INTO document VALUES (8, 44, 1, 'rls_regress_user1', 'my third manga'); -- Must fail with unique violation, revealing presence of did we can't see SELECT * FROM document WHERE did = 8; -- and confirm we can't see it + -- RLS policies are checked before constraints + INSERT INTO document VALUES (8, 44, 1, 'rls_regress_user2', 'my third manga'); -- Should fail with RLS check violation, not duplicate key violation + UPDATE document SET did = 8, dauthor = 'rls_regress_user2' WHERE did = 5; -- Should fail with RLS check violation, not duplicate key violation + -- database superuser does bypass RLS policy when enabled RESET SESSION AUTHORIZATION; SET row_security TO ON;
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers