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

On Thu, Dec 17, 2009 at 10:09 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>>>> 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.

> [ a plan for rearranging ALTER TABLE's checks ]

Works for me. All I asked for was plan first, code second, and you've
satisfied the precondition.

(I would venture that changing this stuff is probably 9.1 material at
this point, though.)

regards, tom lane

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

On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)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.

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

I don't understand what you're saying here.

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

And just as importantly, in a consistent place.

...Robert

(2010/01/24 9:08), Robert Haas wrote:
> On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)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.

In your plan, where the new ATSimplePermissions() should be called?

If we still call it from the ATPrepCmd() stage, it needs to apply
special treatments some of operations with part-B logic.

One other candidate of the entrypoint is the head of each ATExecXXXX()
functions. In this case, we have already gathered enough information
in B2(ATExecDropColumn, ATExecDropConstraint), B3(ATExecAddInherit)
and B4(ATPrepSetTablespace). In B1(ATAddForeignKeyConstrants), we
will need a bit more steps to collect information, but we can call
it just after all the needed information.
We can also have enough information at the head of B5(ATExecAddIndex).
However, my preference is to apply checks at the DefineIndex() when
"check_rights" equals true, because it also allows to consolidate
permission checks in ProcessUtility() with T_IndexStmt.

>>> 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.
>
> I don't understand what you're saying here.

I'd like to introduce it using a pseudo code.

Currently, in ATPrepCmd(),

| case AT_AddConstraint: /* ADD CONSTRAINT */
| ATSimplePermissions(rel, false);
| /* Recursion occurs during execution phase */
| /* No command-specific prep needed except saving recurse flag */
| if (recurse)
| cmd->subtype = AT_AddConstraintRecurse;
| pass = AT_PASS_ADD_CONSTR;
| break;

What I said is:

| case AT_AddConstraint: /* ADD CONSTRAINT */
| {
| Constraint *newCons = (Constraint *)cmd->def;
| if (newCons->contype == CONSTR_CHECK)
| {
| ATSimplePermissions(rel, false);
| if (recurse)
| ATSimpleRecursion(wqueue, rel, cmd, recurse);
| cmd->subtype = AT_AddCheckContraint;
| }
| else if (newCond->contype == CONSTR_FOREIGN)
| {
| /* Permission checks are applied during execution stage */
| /* And, it never recurse */
| cmd->subtype = AT_AddFKConstraint;
| }
| else
| elog(ERROR, "unrecognized constraint type");
|
| pass = AT_PASS_ADD_CONSTR;
| break;
| }

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

>> 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.
>
> And just as importantly, in a consistent place.

I agree.

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

On Sat, Jan 23, 2010 at 8:33 PM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> (2010/01/24 9:08), Robert Haas wrote:
>>
>> On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)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.

>>>> 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.
>>
>> I don't understand what you're saying here.
>
> I'd like to introduce it using a pseudo code.
>
> Currently, in ATPrepCmd(),
>
> |    case AT_AddConstraint:  /* ADD CONSTRAINT */
> |        ATSimplePermissions(rel, false);
> |        /* Recursion occurs during execution phase */
> |        /* No command-specific prep needed except saving recurse flag */
> |        if (recurse)
> |            cmd->subtype = AT_AddConstraintRecurse;
> |        pass = AT_PASS_ADD_CONSTR;
> |        break;
>
> What I said is:
>
> |    case AT_AddConstraint:  /* ADD CONSTRAINT */
> |    {
> |        Constraint   *newCons = (Constraint *)cmd->def;
> |        if (newCons->contype == CONSTR_CHECK)
> |        {
> |            ATSimplePermissions(rel, false);
> |            if (recurse)
> |                ATSimpleRecursion(wqueue, rel, cmd, recurse);
> |            cmd->subtype = AT_AddCheckContraint;
> |        }
> |        else if (newCond->contype == CONSTR_FOREIGN)
> |        {
> |            /* Permission checks are applied during execution stage */
> |            /* And, it never recurse */
> |            cmd->subtype = AT_AddFKConstraint;
> |        }
> |        else
> |            elog(ERROR, "unrecognized constraint type");
> |
> |        pass = AT_PASS_ADD_CONSTR;
> |        break;
> |    }
>
> 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.

...Robert

(2010/01/24 11:27), Robert Haas wrote:
> On Sat, Jan 23, 2010 at 8:33 PM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> (2010/01/24 9:08), Robert Haas wrote:
>>>
>>> On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Sat, Jan 23, 2010 at 10:11 PM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 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.

Maybe I'm still not understanding, but I don't see how you're going to
do this without a massive pile of spaghetti code and a function with
about 12 parameters. Feel free to show some code, but I think this is
a non-starter.

...Robert

(2010/01/24 12:16), Robert Haas wrote:
> On Sat, Jan 23, 2010 at 10:11 PM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 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.
>
> Maybe I'm still not understanding, but I don't see how you're going to
> do this without a massive pile of spaghetti code and a function with
> about 12 parameters. Feel free to show some code, but I think this is
> a non-starter.

Hhmmm,...

Indeed, indeed, if a single function tries to handle all the ALTER TABLE
options, it needs many function arguments, because ALTER TABLE is one of
the most functional statement in PostgreSQL...

If the basis is head of the execution phase, it does not need a big switch
... case branch in ATSimplePermissions, because it is already branched in
ATExecCmd(). However, it also has tradeoff that we have multiple minor version
of functions to check ALTER TABLE permissions.

Perhaps, it may be a good idea to make two conceptual patches both head of
the ATPrepCmd() and ATExec*(). They will make clear good/bad points between
two approaches.

Is it waste of efforts?

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

(2010/01/24 12:40), KaiGai Kohei wrote:
> Perhaps, it may be a good idea to make two conceptual patches both head of
> the ATPrepCmd() and ATExec*(). They will make clear good/bad points between
> two approaches.

I tried to make two conceptual patches.

* pgsql-at-rework-prep.1.patch

It adds ATPermCmd(Relation rel, AlterTableCmd *cmd) that is called from the
head of ATPrepCmd(). This function enables to divide the logic of permission
checks depending on cmd->subtype from ATPrepCmd().
In most of subcommand (it does not check permission except for ownership of
the relation to be altered), it calls ATSimplePermissions() or similar.
Or, it does not anything at the stage for rest of exceptions.

Good: Here is only one entrypoint to call ATPermCmd().
Bad: Although most of logics are consolidated into ATPremCmd(), we need to
put individual checks on some of exception cases.

Was it matching with what you suggested? Or, am I missing something?

* pgsql-at-rework-exec.2.patch

It moves permission checks into the head (or just after all needed information
was gathered) of ATExec*() functions. The ATPrepCmd() checks only correctness
of relkind and ensure the relation is not system catalog.
This basis/concept can be applied to ALTER TABLE RENAME TO/SET SCHEMA cases also.

Good: Concept is clear, and less exceptions.
Good: We can apply this concept (just before execution) on other database
objects which does not have explicit preparation stage.
Bad: All the ATExec*() function has to call the permission checks.

My preference is the later approach. Indeed, it has larger number of entrypoints
compared to the ATPermCom() functions, but its concept is clear and we can also
apply same basis on the code path that does not go through ATPrepCmd().

P.S In the right way, this patch should also touch CheckRelationOwnership() or
DefineIndex() logic, but I omitted them because of simplifies.

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