From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> |
Cc: | Stephen Frost <sfrost(at)snowman(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: restructuring "alter table" privilege checks (was: remove redundant ownership checks) |
Date: | 2010-01-24 02:27:30 |
Message-ID: | 603c8f071001231827u6c1d2e50y797e697e30db0270@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Sat, Jan 23, 2010 at 8:33 PM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> (2010/01/24 9:08), Robert Haas wrote:
>>
>> On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2010-01-24 02:28:52 | Re: commit fests |
Previous Message | Alex Hunsaker | 2010-01-24 01:40:03 | Re: Miscellaneous changes to plperl [PATCH] |