Re: [PATCH] remove redundant ownership checks

(2010/01/14 4:27), Stephen Frost wrote:
> * Jaime Casanova (jcasanov(at)systemguards(dot)com(dot)ec) wrote:
>> if it's not broken, then it doesn't need a fix...
>> if that code is causing a hole in security then i said remove it, if
>> not... what's the problem in let it be until we know exactly what the
>> plan is?
>
> The chances of getting concensus on an overarching big plan are slim
> to none, which essentially means it's not going to get changed. I've
> suggested an approach and had no feedback on it. What's probably
> needed, to get attention anyway, is a patch which starts to move us in
> the right direction. That's going to be more than a 6-line patch, but
> doesn't have to be the whole thing either.
>
> For my 2c, I think a patch which attacks 'AT_PrepCmd' and rips out all
> of the owner checks from it and moves them to appropriate places (that
> is to say, per my proposal, very shortly before the 'work' is actually
> done, which is also often where the *other* permission checks are done,
> for those pieces which require more than just a simple owner check)
> would probably be the first step.
>
> Of course, the code between AT_PrepCmd and where the permissions checks
> are moved to would need to be reviewed and vetted to make sure there
> isn't anything being done that shouldn't be done without permission,
> but, honestly, I don't recall seeing much of that. We're actually
> pretty good about having a "gather info" stage followed by a "implement
> change" stage.

Some of ALTER TABLE operations take multiple permission checks, not only
ownership of the relation to be altered.
For example, ALTER TABLE with SET TABLESPACE option also need ACL_CREATE
permission on the new tablespace, not only ownership of the relation.
It means we need to gather two information before whole of the permission
checks. (1) OID of the relation to be altered. (2) OID of the tablespace
to be set.

In my understanding, Stephen suggests that we should, ideally, rip out
permission logic from the code closely-combined with the steps of
implementation. Of course, it does not mean all the checks should be
moved just before simple_heap_update().

However, it is a separate topic for this patch.
This patch just tries to remove redundant checks, and integrate them
into a common place where most of ALTER TABLE option applies its
permission checks on.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>