Re: WITH CHECK and Column-Level Privileges

Note the first comment in this hunk was not update to talk about NULL
instead of "":

> @@ -163,13 +168,63 @@ BuildIndexValueDescription(Relation indexRelation,
> Datum *values, bool *isnull)
> {
> StringInfoData buf;
> + Form_pg_index idxrec;
> + HeapTuple ht_idx;
> int natts = indexRelation->rd_rel->relnatts;
> int i;
> + int keyno;
> + Oid indexrelid = RelationGetRelid(indexRelation);
> + Oid indrelid;
> + AclResult aclresult;
> +
> + /*
> + * Check permissions- if the users does not have access to view the
> + * data in the key columns, we return "" instead, which callers can
> + * test for and use or not accordingly.
> + *
> + * First we need to check table-level SELECT access and then, if
> + * there is no access there, check column-level permissions.
> + */

[hunk continues]

> + /* Table-level SELECT is enough, if the user has it */
> + aclresult = pg_class_aclcheck(indrelid, GetUserId(), ACL_SELECT);
> + if (aclresult != ACLCHECK_OK)
> + {
> + /*
> + * No table-level access, so step through the columns in the
> + * index and make sure the user has SELECT rights on all of them.
> + */
> + for (keyno = 0; keyno < idxrec->indnatts; keyno++)
> + {
> + AttrNumber attnum = idxrec->indkey.values[keyno];
> +
> + aclresult = pg_attribute_aclcheck(indrelid, attnum, GetUserId(),
> + ACL_SELECT);
> +
> + if (aclresult != ACLCHECK_OK)
> + {
> + /* No access, so clean up and return */
> + ReleaseSysCache(ht_idx);
> + return NULL;
> + }
> + }
> + }

Hm, this is a bit odd. I thought you were going to return the subset of
columns that the user had permission to read, not empty if any of them
was inaccesible. Did I misunderstand?

> diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
> index 4c55551..20acf04 100644
> --- a/src/backend/executor/execMain.c
> +++ b/src/backend/executor/execMain.c
> @@ -82,12 +82,17 @@ static void ExecutePlan(EState *estate, PlanState *planstate,
> DestReceiver *dest);
> static bool ExecCheckRTEPerms(RangeTblEntry *rte);
> static void ExecCheckXactReadOnly(PlannedStmt *plannedstmt);
> -static char *ExecBuildSlotValueDescription(TupleTableSlot *slot,
> +static char *ExecBuildSlotValueDescription(Oid reloid,
> + TupleTableSlot *slot,
> TupleDesc tupdesc,
> + Bitmapset *modifiedCols,
> int maxfieldlen);
> static void EvalPlanQualStart(EPQState *epqstate, EState *parentestate,
> Plan *planTree);
>
> +#define GetModifiedColumns(relinfo, estate) \
> + (rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->modifiedCols)

I assume you are aware that this GetModifiedColumns() macro is a
duplicate of another one found elsewhere. However I think this is not
such a hot idea; the UPSERT patch series has a preparatory patch that
changes that other macro definition, as far as I recall; probably it'd
be a good idea to move it elsewhere, to avoid a future divergence.

> @@ -1689,25 +1722,54 @@ ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
> * dropped columns. We used to use the slot's tuple descriptor to decode the
> * data, but the slot's descriptor doesn't identify dropped columns, so we
> * now need to be passed the relation's descriptor.
> + *
> + * Note that, like BuildIndexValueDescription, if the user does not have
> + * permission to view any of the columns involved, a NULL is returned. Unlike
> + * BuildIndexValueDescription, if the user has access to view a subset of the
> + * column involved, that subset will be returned with a key identifying which
> + * columns they are.
> */

Ah, I now see that you are aware of the NULL-or-nothing nature of
BuildIndexValueDescription ... but the comments there don't explain the
reason. Perhaps a comment in BuildIndexValueDescription is in order
rather than a code change?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services