Re: restructuring "alter table" privilege checks - Mailing list pgsql-hackers

(2010/01/24 11:27), Robert Haas wrote:
> 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.

Yes, most of the ALTER TABLE options runs ATPrepCmd() except for RENAME
TO and SET SCHEMA, so it is a good candidate to apply less error-prone
permission checks.

The reason why I introduced this alternative idea is from the perspective
of simple basis/concept about where we should apply permission checks,
although it needs larger number of entrypoints compared to head of the
ATPrepCmd().

If we put the new ATSimplePermissions() with all the needed information
just after gathering them at the execution stage, we don't need to have
some of exceptions which takes additional checks except for ownership
on the relation to be altered.

Of course, code simpleness is important. Likewise, I think simpleness in
basis/concept (less number of special treatments) is also important.


>> I'd like to introduce it using a pseudo code.  :
>> 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.

Hmm, indeed, here is no more benefit except for eliminating one self recursion.

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