Re: restructuring "alter table" privilege checks (was: remove redundant ownership checks)

On Sat, Jan 23, 2010 at 8:33 PM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote:
> (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.

Yeah, that's what I thought, too.

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

From ATPrepCmd(), just before the big switch statement.

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

Not sure if I understand what you mean.  ATSimplePermissions() won't
be responsible for applying permissions checks related to other
objects upon which the command is operating (e.g. the other table, if
adding a foreign key).  It will however be responsible for knowing
everything about which permission checks to apply to the main table
involved, which will require some special-case logic for certain
command types.

> One other candidate of the entrypoint is the head of each ATExecXXXX()
> functions. [...snip...]

I don't think this is a good idea.  Calling it in just one place seems
less error-prone and easier to audit.

>>>> 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().

I don't really see that this gains us anything.

...Robert