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
