On 26 February 2015 at 09:50, Dean Rasheed <dean.a.rash...@gmail.com> wrote:
> On 26 February 2015 at 05:43, Stephen Frost <sfr...@snowman.net> wrote:
>> I wonder if there are some documentation updates which need to be done
>> for this also? I'm planning to look as I vauguely recall mentioning the
>> ordering of operations somewhere along the way.
>>
I couldn't find any mention of the timing of the check in the existing
documentation, although it does vaguely imply that the check is done
before inserting any new data. There is an existing paragraph
describing the timing of USING conditions, so I added a new paragraph
immediately after that to explain when CHECK expressions are enforced,
since that seemed like the best place for it.
>> I also addressed the bitrot from the column-priv leak patch. Would be
>> great to have you take a look at the latest and let me know if you have
>> any further comments or suggestions.
I looked it over again, and I'm happy with these updates, except there
was a missing "break" in the switch statement in
ExecWithCheckOptions().
Here's an updated patch.
Regards,
Dean
diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml
new file mode 100644
index 868a6c1..49eaadc
*** a/doc/src/sgml/ref/create_policy.sgml
--- b/doc/src/sgml/ref/create_policy.sgml
*************** CREATE POLICY <replaceable class="parame
*** 61,66 ****
--- 61,74 ----
</para>
<para>
+ For INSERT and UPDATE queries, WITH CHECK expressions are enforced after
+ BEFORE triggers are fired, and before any data modifications are made.
+ Thus a BEFORE ROW trigger may modify the data to be inserted, affecting
+ the result of the security policy check. WITH CHECK expressions are
+ enforced before any other constraints.
+ </para>
+
+ <para>
Policy names are per-table, therefore one policy name can be used for many
different tables and have a definition for each table which is appropriate to
that table.
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
new file mode 100644
index 33b172b..8dfb952
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
*************** ExecConstraints(ResultRelInfo *resultRel
*** 1667,1675 ****
/*
* ExecWithCheckOptions -- check that tuple satisfies any WITH CHECK OPTIONs
*/
void
! ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate)
{
Relation rel = resultRelInfo->ri_RelationDesc;
--- 1667,1676 ----
/*
* ExecWithCheckOptions -- check that tuple satisfies any WITH CHECK OPTIONs
+ * of the specified kind.
*/
void
! ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate)
{
Relation rel = resultRelInfo->ri_RelationDesc;
*************** ExecWithCheckOptions(ResultRelInfo *resu
*** 1694,1699 ****
--- 1695,1703 ----
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
*** 1715,1726 ****
modifiedCols,
64);
! ereport(ERROR,
! (errcode(ERRCODE_WITH_CHECK_OPTION_VIOLATION),
! errmsg("new row violates WITH CHECK OPTION for \"%s\"",
! wco->viewname),
! val_desc ? errdetail("Failing row contains %s.", val_desc) :
! 0));
}
}
}
--- 1719,1747 ----
modifiedCols,
64);
! 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),
! val_desc ? errdetail("Failing row contains %s.",
! val_desc) : 0));
! 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),
! val_desc ? errdetail("Failing row contains %s.",
! val_desc) : 0));
! 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 9fe8008..d49585a
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
*************** _copyWithCheckOption(const WithCheckOpti
*** 2059,2065 ****
{
WithCheckOption *newnode = makeNode(WithCheckOption);
! COPY_STRING_FIELD(viewname);
COPY_NODE_FIELD(qual);
COPY_SCALAR_FIELD(cascaded);
--- 2059,2066 ----
{
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 fe509b0..2cf1899
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
*************** _equalRangeTblFunction(const RangeTblFun
*** 2371,2377 ****
static bool
_equalWithCheckOption(const WithCheckOption *a, const WithCheckOption *b)
{
! COMPARE_STRING_FIELD(viewname);
COMPARE_NODE_FIELD(qual);
COMPARE_SCALAR_FIELD(cascaded);
--- 2371,2378 ----
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 775f482..f86086c
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
*************** _outWithCheckOption(StringInfo str, cons
*** 2326,2332 ****
{
WRITE_NODE_TYPE("WITHCHECKOPTION");
! WRITE_STRING_FIELD(viewname);
WRITE_NODE_FIELD(qual);
WRITE_BOOL_FIELD(cascaded);
}
--- 2326,2333 ----
{
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 563209c..b0cd95d
*** 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 9d2c280..9ea770b
*** 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 7669130..687c7f6
*** a/src/backend/rewrite/rowsecurity.c
--- b/src/backend/rewrite/rowsecurity.c
*************** prepend_row_security_policies(Query* roo
*** 227,233 ****
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);
--- 227,235 ----
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
*** 241,247 ****
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);
--- 243,251 ----
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 59b17f3..ee36a64
*** 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 ac13302..2c3bbce
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
*************** typedef struct RangeTblFunction
*** 861,874 ****
/*
* 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;
/*
--- 861,883 ----
/*
* 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 f41bef1..d318bf9
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*************** SELECT * FROM document WHERE did = 8; --
*** 300,305 ****
--- 300,310 ----
-----+-----+--------+---------+--------
(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"
+ 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"
-- database superuser does bypass RLS policy when enabled
RESET SESSION AUTHORIZATION;
SET row_security TO ON;
*************** EXPLAIN (COSTS OFF) WITH cte1 AS (SELECT
*** 1689,1695 ****
(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"
WITH cte1 AS (UPDATE t1 SET a = a RETURNING *) SELECT * FROM cte1; --ok
a | b
----+----------------------------------
--- 1694,1700 ----
(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"
WITH cte1 AS (UPDATE t1 SET a = a RETURNING *) SELECT * FROM cte1; --ok
a | b
----+----------------------------------
*************** WITH cte1 AS (UPDATE t1 SET a = a RETURN
*** 1707,1713 ****
(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"
WITH cte1 AS (INSERT INTO t1 VALUES (20, 'Success') RETURNING *) SELECT * FROM cte1; --ok
a | b
----+---------
--- 1712,1718 ----
(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"
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 ed7adbf..a9f4b83
*** 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
