Thread: restructuring "alter table" privilege checks (was: remove redundant ownership checks)
restructuring "alter table" privilege checks (was: remove redundant ownership checks)
From
Robert Haas
Date:
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
Re: restructuring "alter table" privilege checks (was: remove redundant ownership checks)
From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes: >> * Tom Lane (tgl@sss.pgh.pa.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
Re: restructuring "alter table" privilege checks (was: remove redundant ownership checks)
From
KaiGai Kohei
Date:
(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 relationto 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@kaigai.gr.jp>
Re: restructuring "alter table" privilege checks (was: remove redundant ownership checks)
From
Robert Haas
Date:
On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei <kaigai@kaigai.gr.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
Re: restructuring "alter table" privilege checks (was: remove redundant ownership checks)
From
KaiGai Kohei
Date:
(2010/01/24 9:08), Robert Haas wrote: > On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei<kaigai@kaigai.gr.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@kaigai.gr.jp>
Re: restructuring "alter table" privilege checks (was: remove redundant ownership checks)
From
Robert Haas
Date:
On Sat, Jan 23, 2010 at 8:33 PM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote: > (2010/01/24 9:08), Robert Haas wrote: >> >> On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei<kaigai@kaigai.gr.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@kaigai.gr.jp> wrote: >> (2010/01/24 9:08), Robert Haas wrote: >>> >>> On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei<kaigai@kaigai.gr.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@kaigai.gr.jp>
On Sat, Jan 23, 2010 at 10:11 PM, KaiGai Kohei <kaigai@kaigai.gr.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@kaigai.gr.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@kaigai.gr.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@kaigai.gr.jp>