Re: [PATCH] remove redundant ownership checks

(2010/01/14 23:29), Stephen Frost wrote:
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> Some of ALTER TABLE operations take multiple permission checks, not only
>> ownership of the relation to be altered.
>
> Yes, exactly my point. Those places typically just presume that the
> owner check has already been done.
>
>> 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.
>
> Right. I would say that we should wait until we have all the necessary
> information to do the permissions checking, and then do it all at that
> point, similar to how we handle DML today.
>
>> In my understanding, Stephen suggests that we should, ideally, rip out
>> permission logic from the code closely-combined with the steps of
>> implementation.
>
> This might be a confusion due to language, but I think of the
> "implementation" as being "the work". The check on permissions on the
> tablespace that you're describing above is done right before "the work".
> For this case, specifically, ATPrepSetTableSpace() takes the action on
> line 6754: tab->newTableSpace = tablespaceId;
> Prior to that, it checks the tablespace permissions (but not the table
> permissions, since they've been checked already). I would suggest we
> add a call to ATSimplePermissions() in ATPrepSetTableSpace at line 6745-
> right after the /* Check its permissions */ comment (which would be
> changed to "check permissions for ALTER TABLE x SET TABLESPACE y").
>
>> Of course, it does not mean all the checks should be
>> moved just before simple_heap_update().
>
> No, I would have it earlier than simple_heap_update(), we don't need to
> go building the structures and whatnot needed to call
> simple_heap_update().

Sorry for this confusion. I used the "just before simple_heap_update()"
as a metaphor to mean "much deep stage in execution phase".
It does not mean we should put security checks after the catalog updates.

> For this specific case though, I'm a bit torn by
> the fact that the work associated with changing the tablespace can
> actually happen in two distinct places- either through
> ATExecSetTableSpace, or in ATRewriteTables directly.
> ATExecSetTableSpace would actually be a good candidate rather than in
> the 'prep' stage, if all tablespace changes were done there. The 'prep'
> stage worries me a bit since I'm not sure if all permissions checking
> is currently, or coulde be, done at that point, and I'd prefer that we
> use the same approach for permissions checking throughout the code- for
> example, it's either done in 'phase 3' (where we're going through the
> subcommands) or all done in 'phase 1/2', where we're setting things up.

It seems to me it is highly suggestive idea, and we should not ignore it.

Currently, ATPrepCmd() applies permission checks and set up recursion
for inherited tables, if necessary.

The following commands have its own variations:

* AT_AddColumn, AT_AddColumnToView, AT_AddOids
It eventually calls ATPrepAddColumn(), because ColumnDef->inhcount of
the child relation should be 1, not 0.

* AT_SetStatistics
ATPrepSetStatistics() does same job with ATSimplePermissionsRelationOrIndex()
except for it allows to alter system relation.

* AT_AddIndex
It check table's permission here, then, it eventually checks permission
to create a new index later.
The point is that whether the index is an individual object class, or
a property of the table. In fact, it has its own ownership, but it is
a copy from the relation to be indexed. Its namespace is also a copy.
In other word, it is equivalent to check properties of the relation.
IMO, we should move all the permission checks in DefineIndex() to the
caller side. In ALTER TABLE case, ATExecAddIndex() is a candidate.
(It is also reason why DefineIndex() takes 'check_right' argument.)

* AT_AlterColumnType
It calls ATPrepAlterColumnType() to check it is available, or not.

* AT_ChangeOwner
It does all the task in ATExecChangeOwner(), and it check permission
only when ownership is actually changed.

* AT_DropOids
It recursively calls ATPrepCmd() with pseudo AT_DropColumn with "oid".

* AT_SetTableSpace
ATPrepSetTableSpace() checks permission on tablespace, in addition to
the ownership of the relation checked in ATSimplePermissionsRelationOrIndex().

And, note that some of AT_* command already checks table's permission due to
the recursion of inheritance tree.

Example)
static void
ATExecDropColumn(List **wqueue, Relation rel, const char *colName, ...)
{
:
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
ATSimplePermissions(rel, false);
:

In addition, we need pay mention ATSimplePermissions() is a multi-functional
function.
(1) Ensure that it is a relation (or possibly a view)
(2) Ensure this user is the owner
(3) Ensure that it is not a system table

If we move the permission checks at the head of ATExecXXX() routines,
these checks should be broken out. At least, (1) and (3) are independent
from security model.

In the result of this modification, we will come on a clear plan to apply
permission checks on ALTER TABLE, namely, we should apply permission checks
at the head of ATExecXXXX phase (basically; exception is OWNER TO option).

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