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