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

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

I agree with the top half of this proposition. ALTER TABLE has most
various kind of options in PostgreSQL, so some of options are not
feasible to complete all the needed checks in ATPrepCmd() stage,
such as AT_AddIndex.

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?

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

I have a different plan to clean up these differences, especially A1 and A2.

The existing ATSimplePermissions() can be also divided into three parts,
as source code comments mentioned.
Aa) It ensures the relation has an expected relkind
Ab) It ensures ownership of the relation to be altered
Ac) It ensures the relation is not system catalog, if not allowSystemTableMods.

If we provide these three checks in separated helper function, like:
Aa) ATCheckRelkind(Relation rel, bool viewOK, bool indexOK)
Ab) ATCheckOwnership(Relation rel)
Ac) ATCheckNoCatalog(Relation rel)

The above A1 and A2 can be rewritten using combination of them, then we can
eliminate these functions to apply special treatments.
For example, the A2 can be replaced by ATCheckRelkind(rel, false, true) and
ATCheckOwnership(rel).

I think it allows to put a logic to decide whether we should apply permission
checks at the ATPrepCmd() stage, or not, in the existing swich ... case branch.

For some of exceptions, we can apply checks for them in the later stage
after the code gathered enough information to make access control decision.

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

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.

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

They calls CheckRelationOwnership() just before RenameRelation(),
renameatt() and renametrig(). It applies same checks except for sanity
checks in relkind.
But here is no reason why we cannot rewrite them using ATCheckOwnership()
and ATCheckNoCatalog() in tablecmd.c, if ATSimplePermissions() are divided.

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

I agree.

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

It is a good idea to consolidate permission check into tablecmd.c from
various kind of core routines.
And, it also makes the basis clear. The permission check should be applied
after the code path gathered enough information to make its access control
decision as early as possible.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>