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

On Thu, Dec 17, 2009 at 10:09 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
>> > [ patch to remove EnableDisableRule's permissions check ]
>>
>> I don't particularly like this patch, mainly because I disagree with
>> randomly removing permissions checks without any sort of plan about
>> where they ought to go.
>
> The goal of this was to increase consistancy with the rest of the code,
> in particular, ATPrepCmd checks ownership rights on the table, and
> anything which wants to check permissions beyond that has to do it
> independently.  Do I like that?  No, not really.

After you posted this, I agreed with you a time or two that this was
kooky, but I spent some time yesterday and today reading through
tablecmds.c, and I've somewhat revised my opinion.  It seems to me
that the ALTER TABLE permissions checks can be basically divided into
two parts: (A) checks on the relation that is being altered, and (B)
checks on other objects that are involved in whatever operation is
being performed.

Right now, the part-A logic is partially centralized in ATPrepCmd()
and partially spread throughout the rest of the code, and the part-B
logic is all over the place.  If we want ALL the permissions checking
for any given ALTER TABLE command to happen in ONE place, we're going
to have to do one of two things, neither of which is currently making
sense to me.  One, we could move all of the checks that are now done
in ATPrepCmd() down into the constituent ATPrep* functions and do all
the checks at once at the point when we have enough information.  But
that almost seems like it's going backwards - we're spreading out
checks that are now (kind of) centralized.  And, as Tom has pointed
out, it means doing more work before we decide whether we even had
permission to take the relation lock.  Two, we could try to pull all
the permission checks that are NOT in ATPrepCmd() back in.  But that
hardly seems feasible - we don't have enough information at that
point.

So, what if we treat these two cases separately?  The part-B checks -
on the other operations involved in ALTER TABLE - are by definition
idiosyncratic.  What type of object we're checking and what permission
we're checking for is inextricably bound up with what kind of ALTER
TABLE operation we're trying to perform.  So, that's going to be hard
to centralize.  But the part-A checks - on the relation itself - seem
like they could be consolidated.  The reason why they are spread out
right now is mostly because of relatively minor deviations from what
ATSimplePermissions does.

A1. ATSimplePermissionsRelationOrIndex(), which a few of the ALTER
TABLE subcommands use, is exactly the same as ATSimplePermissions(),
except that it allows indices as well.
A2. ATSetStatistics() and ATSetDistinct() are also similar, but they
both allow indices and also skip the defenses against system table
modification.
A3. ATExecChangeOwner() emits a warning indices and then performs no
action (for backwards compatibility), rather than emitting an error as
ATSimplePermissions() would do.  It also doesn't check permission if
the old and new owner are the same.
A4. ATExecEnableDisableTrigger() and ATExecEnableDisableRule() call,
respectively, EnableDisableTrigger and EnableDisableRule, each of
which does a redundant permissions check.

I believe that everything else just calls ATSimplePermissions(),
though it's possible I've missed something.  It strikes me that if we
changed the signature for ATSimplePermissions, we could eliminate
A1-A3 (and A4 is trivial):

static void ATSimplePermissions(Relation rel, AlterTableCmd *cmd);

The plan would be to move the knowledge of which operations require
special treatment (allowing indices, system tables, etc.) into
ATSimplePermissions() and then just calling it unconditionally for ALL
object types.  ATSimplePermissionsRelationOrIndex() would go away.
ATExecChangeOwner() would require some refactoring, but I think it
would end up being simpler than it is now.  I also think it would be
more clear which checks are being applied to which object types.

Just to enumerate the part-B permissions checks, that is, permissions
checks on objects other than the table to which ALTER TABLE is being
directly applied, the ones I found were:

B1. ATAddForeignKeyConstrants() checks for REFERENCES permission on
the two tables involved.
B2. ATExecDropColumn() and ATExecDropConstraint() check for permission
to perform the drop on the child tables to which they decide to
recurse.
B3. ATExecAddInherit() checks permissions on the new parent.
B4. ATPrepSetTablespace() checks for permission to use the new tablespace.
B5. ATExecAddIndex() calls DefineIndex(), which also checks for rights
on the new namespace.

B2 and B3 are actually implemented at present using
ATSimplePermissions, and I think we could keep it that way under the
proposed signature change, with only minor refactoring.  The others
are basically all special cases, but there aren't actually that many
of them.

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.

We also have a few bits and pieces of ALTER TABLE that do not go
through ATPrepCmd() at all and therefore need to be considered
separately from the above.  It looks like ALTER TABLE ... RENAME and
ALTER TABLE ... RENAME COLUMN go through ExecRenameStmt().
ExecRenameStmt() generally leaves permissions-checking to the
RenameWhatever() functions, but for RenameRelation(), renameatt(), and
renametrig() it makes an exception and does some of the work here.  I
think we should push that logic back down into the constituent
functions so that this goes back to being just a dispatch function;
while we're at it, I think we should change renameatt() and
renametrig() to use the same naming convention as the rest of these
functions.  It's not really buying us anything to do the check here;
it's just making it harder to find all the checks - for example, it
looks to me like the check done here is redundant with one being
performed in renameatt() anyway.  Interestingly, renameatt() also has
a comment that says "this would normally be done in utility.c, but
this particular routine is recursive".  That comment doesn't reflect
the way things are actually laid out, though.

ALTER TABLE ... SET SCHEMA has the same issue:
ExecAlterObjectSchemaStmt normally just calls
AlterWhateverNamespace(), but in the case where Whatever is Table, it
first calls CheckRelationOwnership().  So I think we should go ahead
and push this check down too, to match all the others.

Why do this?  Well, I think the end game is that it would be nice to
provide a finite number of well-defined control points that need to be
modified to add or change permission checks.  Of course, a lot more
work will be needed to do that in its entirety - this email only
addresses the ALTER TABLE stuff, and I'm not even 100% positive that
I've found everything.  But I think that's part of the problem too -
as we clean this up, it will become feasible to actually list what the
control points for different permissions checks are.

Thoughts?  Comments?  Blistering flames of agonizing death?

...Robert