Re: restructuring "alter table" privilege checks (was: remove redundant ownership checks) - Mailing list pgsql-hackers

(2010/01/24 9:08), Robert Haas wrote:
> On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei<kaigai@kaigai.gr.jp>  wrote:
>> However, it is unclear for me whether the revised ATSimplePermissions()
>> provide cleaner code than currently we have, because it also needs
>> a big switch ... case statement within.
>>
>> Am I misunderstanding something?
>
> Well, not everyone is going to agree on what "cleaner code" means in
> every case, but the reason that I like my design better is because it
> moves all of the decision making out of ATPrepCmd() into
> ATSimplePermissions().  What you're proposing would mean that
> ATPrepCmd() would basically continue to know everything about which
> operations need which permissions checks, which I don't think is going
> to scale very well to alternative security providers, if we eventually
> decide to support such a thing.

Hmm. Indeed, the existing ATPrepCmd() closely combines the permission
checks and controls of code path (inheritance recursion and AT_PASS_*),
and it is worthwhile to divide these independent logic into two.

In your plan, where the new ATSimplePermissions() should be called?

If we still call it from the ATPrepCmd() stage, it needs to apply
special treatments some of operations with part-B logic.

One other candidate of the entrypoint is the head of each ATExecXXXX()
functions. In this case, we have already gathered enough information
in B2(ATExecDropColumn, ATExecDropConstraint), B3(ATExecAddInherit)
and B4(ATPrepSetTablespace). In B1(ATAddForeignKeyConstrants), we
will need a bit more steps to collect information, but we can call
it just after all the needed information.
We can also have enough information at the head of B5(ATExecAddIndex).
However, my preference is to apply checks at the DefineIndex() when
"check_rights" equals true, because it also allows to consolidate
permission checks in ProcessUtility() with T_IndexStmt.

>>> It may also be worth refactoring is ATAddCheckConstraint(), which
>>> currently does its own recursion only so that the constraint name at
>>> the top of the inheritance hierarchy propagates all the way down
>>> unchanged.  I wonder if it would be cleaner/possible to work out the
>>> constraint name earlier and then just use the standard recursion
>>> mechanism.
>>
>> Isn't it possible to check whether the given constraint is CHECK()
>> or FOREIGN KEY in the ATPrepCmd() stage, and assign individual
>> cmd->subtype? If CONSTR_FOREIGN, it will never recursion.
>>
>> In this case, we can apply checks in ATPrepCmd() stage for CONSTR_CHECK.
>
> I don't understand what you're saying here.

I'd like to introduce it using a pseudo code.

Currently, in ATPrepCmd(),

|    case AT_AddConstraint:  /* ADD CONSTRAINT */
|        ATSimplePermissions(rel, false);
|        /* Recursion occurs during execution phase */
|        /* No command-specific prep needed except saving recurse flag */
|        if (recurse)
|            cmd->subtype = AT_AddConstraintRecurse;
|        pass = AT_PASS_ADD_CONSTR;
|        break;

What I said is:

|    case AT_AddConstraint:  /* ADD CONSTRAINT */
|    {
|        Constraint   *newCons = (Constraint *)cmd->def;
|        if (newCons->contype == CONSTR_CHECK)
|        {
|            ATSimplePermissions(rel, false);
|            if (recurse)
|                ATSimpleRecursion(wqueue, rel, cmd, recurse);
|            cmd->subtype = AT_AddCheckContraint;
|        }
|        else if (newCond->contype == CONSTR_FOREIGN)
|        {
|            /* Permission checks are applied during execution stage */
|            /* And, it never recurse */
|            cmd->subtype = AT_AddFKConstraint;
|        }
|        else
|            elog(ERROR, "unrecognized constraint type");
|
|        pass = AT_PASS_ADD_CONSTR;
|        break;
|    }

It will allow to eliminate self recursion in ATAddCheckConstraint() and
to apply permission checks for new CHECK constraint in ATPrepCmd() phase.

Perhaps, it may be consolidated to ATPrepAddConstraint().

>> It is a good idea to consolidate permission check into tablecmd.c from
>> various kind of core routines.
>> And, it also makes the basis clear. The permission check should be applied
>> after the code path gathered enough information to make its access control
>> decision as early as possible.
>
> And just as importantly, in a consistent place.

I agree.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>