Thread: [PATCH] DefaultACLs
Hello, this is first public version of our DefaultACLs patch as described on http://wiki.postgresql.org/wiki/DefaultACL . It allows GRANT/REVOKE permissions to be inherited by objects based on schema permissions at create type by use of ALTER SCHEMA foo SET DEFAULT PRIVILEGES ON TABLE SELECT TO bar syntax. There is also ADD and DROP for appending and removing those default privileges. It works for tables, views, sequences and functions. More info about syntax and some previous discussion is on wiki. There is also GRANT DEFAULT PRIVILEGES ON tablename which *replaces* current object privileges with the default ones. Only owner can do both of those commands (ALTER SCHEMA can be done only by schema owner and GRANT can be done only by object owner). It adds new catalog table which stores the default permissions for given schema and object type. We didn't add syscache entry for that as Stephen Frost didn't feel we should do that (yet). Three functions were also exported from aclchk.c because most of the ALTER SCHEMA stuff is done in schemacmds.c. The current version is fully working and includes some regression tests. There is however no documentation at this moment. Patch is against current Git HEAD (it is context diff). -- Regards Petr Jelinek (PJMODOS)
Attachment
Hi Petr, > this is first public version of our DefaultACLs patch as described on > http://wiki.postgresql.org/wiki/DefaultACL . I have been assigned by Robert to do an initial review of your "GRANT ON ALL" patch mentioned here (http://archives.postgresql.org/pgsql-hackers/2009-07/msg00207.php) Does this new DefaultACL patch nullify this earlier one? Or it is different and should be looked at first since it was added to the commitfest before the defaultACL patch? It is a bit confusing. Please clarify. Regards, Nikhils > It allows GRANT/REVOKE permissions to be inherited by objects based on > schema permissions at create type by use of ALTER SCHEMA foo SET DEFAULT > PRIVILEGES ON TABLE SELECT TO bar syntax. There is also ADD and DROP for > appending and removing those default privileges. It works for tables, views, > sequences and functions. More info about syntax and some previous discussion > is on wiki. > > There is also GRANT DEFAULT PRIVILEGES ON tablename which *replaces* current > object privileges with the default ones. Only owner can do both of those > commands (ALTER SCHEMA can be done only by schema owner and GRANT can be > done only by object owner). > > It adds new catalog table which stores the default permissions for given > schema and object type. We didn't add syscache entry for that as Stephen > Frost didn't feel we should do that (yet). Three functions were also > exported from aclchk.c because most of the ALTER SCHEMA stuff is done in > schemacmds.c. > > The current version is fully working and includes some regression tests. > There is however no documentation at this moment. > Patch is against current Git HEAD (it is context diff). > > -- > Regards > Petr Jelinek (PJMODOS) > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > > -- http://www.enterprisedb.com
Nikhil Sontakke wrote: > Does this new DefaultACL patch nullify this earlier one? Or it is > different and should be looked at first since it was added to the > commitfest before the defaultACL patch? It is a bit confusing. Please > clarify. > No, DefaultACLs applies to objects created in the future while GRANT ON ALL affects existing objects. DefaultACLs is more important functionality so it should probably take precedence in review process. There is however one thing that needs some attention. Both patches add distinction between VIEW and TABLE objects for acls into parser and they both do it differently. GRANT ON ALL works by adding ACL_OBJECT_VIEW and tracks that object type in code (that was my original method in both patches) while DefaultACLs uses method suggested by Stephen Frost which is creating new enum with relation, view, function and sequence members (those are object types for which both DefaultACLs and GRANT ON ALL are applicable). The second method has advantage of minimal changes to existing code. It's pointless to use both methods so one of the patches will have to be adjusted. The problem is that most people seem to dislike the addition of ACL_OBJECT_VIEW but on the other hand I don't like the idea of adding another object type variable into GrantStmt struct which would be needed if we adjusted GRANT ON ALL to Stephen Frost's method. -- Regards Petr Jelinek (PJMODOS)
Hi, >> > > No, DefaultACLs applies to objects created in the future while GRANT ON ALL > affects existing objects. I see. > DefaultACLs is more important functionality so it should probably take > precedence in review process. > > There is however one thing that needs some attention. Both patches add > distinction between VIEW and TABLE objects for acls into parser and they > both do it differently. GRANT ON ALL works by adding ACL_OBJECT_VIEW and > tracks that object type in code (that was my original method in both > patches) while DefaultACLs uses method suggested by Stephen Frost which is > creating new enum with relation, view, function and sequence members (those > are object types for which both DefaultACLs and GRANT ON ALL are > applicable). The second method has advantage of minimal changes to existing > code. I briefly looked at the DefaultACLs patch. Can you not re-use the GrantStmt structure for the defaults purpose too? You might have to introduce an "is_default" boolean similar to the "is_schema" boolean that you have added in the "GRANT ON ALL" patch. If you think you can re-use the GrantStmt structure, then we might as well stick with the existing object type code and not add the enums in the DefaultACLs patch too.. Regards, Nikhils -- http://www.enterprisedb.com
Nikhil Sontakke wrote: >> There is however one thing that needs some attention. Both patches add >> distinction between VIEW and TABLE objects for acls into parser and they >> both do it differently. GRANT ON ALL works by adding ACL_OBJECT_VIEW and >> tracks that object type in code (that was my original method in both >> patches) while DefaultACLs uses method suggested by Stephen Frost which is >> creating new enum with relation, view, function and sequence members (those >> are object types for which both DefaultACLs and GRANT ON ALL are >> applicable). The second method has advantage of minimal changes to existing >> code. >> > I briefly looked at the DefaultACLs patch. Can you not re-use the > GrantStmt structure for the defaults purpose too? You might have to > introduce an "is_default" boolean similar to the "is_schema" boolean > that you have added in the "GRANT ON ALL" patch. If you think you can > re-use the GrantStmt structure, then we might as well stick with the > existing object type code and not add the enums in the DefaultACLs > patch too.. > No we can't use the GrantStmt and I wasn't talking about using it. I was talking about the change in GrantObjectType and differentiating VIEW and TABLE in some code inside aclchk.c which people didn't like. We can use the changed GrantObjectType in DefaultACLs and filter inapplicable types inside C code as I do in GRANT ON ALL patch and it's what I did originally, but submitted version of DefaultACLs behaves differently. -- Regards Petr Jelinek (PJMODOS)
Nikhil,
* Nikhil Sontakke (nikhil.sontakke@enterprisedb.com) wrote:
> I briefly looked at the DefaultACLs patch. Can you not re-use the
> GrantStmt structure for the defaults purpose too? You might have to
> introduce an "is_default" boolean similar to the "is_schema" boolean
> that you have added in the "GRANT ON ALL" patch. If you think you can
> re-use the GrantStmt structure, then we might as well stick with the
> existing object type code and not add the enums in the DefaultACLs
> patch too..
Petr and I discussed this. Part of the problem is that the regular
grant enums don't distinguish between TABLE and VIEW because they don't
need to. We need to with DefaultACL because users see those as distinct
types of objects even though we track them in the same catalog.
Splitting up RELATION into TABLE and VIEW in the grant enum would
increase the changes quite a bit in otherwise unrelated paths.
Additionally, not all of the grantable types are applicable for
DefaultACL since DefaultACLs are associated with objects in schemas
(eg: database permissions, schema permissions, etc).
We can certainly do it either way, but I don't see much downside to
having a new enum and a number of downsides with modifying the existing
grant enums.
Thanks,
Stephen
Hi, >> I briefly looked at the DefaultACLs patch. Can you not re-use the >> GrantStmt structure for the defaults purpose too? You might have to >> introduce an "is_default" boolean similar to the "is_schema" boolean >> that you have added in the "GRANT ON ALL" patch. If you think you can >> re-use the GrantStmt structure, then we might as well stick with the >> existing object type code and not add the enums in the DefaultACLs >> patch too.. > > Petr and I discussed this. Part of the problem is that the regular > grant enums don't distinguish between TABLE and VIEW because they don't > need to. We need to with DefaultACL because users see those as distinct > types of objects even though we track them in the same catalog. > Splitting up RELATION into TABLE and VIEW in the grant enum would > increase the changes quite a bit in otherwise unrelated paths. > Additionally, not all of the grantable types are applicable for > DefaultACL since DefaultACLs are associated with objects in schemas > (eg: database permissions, schema permissions, etc). > Ok. > We can certainly do it either way, but I don't see much downside to > having a new enum and a number of downsides with modifying the existing > grant enums. > Sure, I understand. But if we want to go the DefaultACLs way, then we need to change the "GRANT ON ALL" patch a bit too for the sake of uniformity - don't we? There is indeed benefit in managing ACLs for existing objects, so that patch has some value too. Regards, Nikhils -- http://www.enterprisedb.com
Hey,
* Nikhil Sontakke (nikhil.sontakke@enterprisedb.com) wrote:
> > We can certainly do it either way, but I don't see much downside to
> > having a new enum and a number of downsides with modifying the existing
> > grant enums.
>
> Sure, I understand. But if we want to go the DefaultACLs way, then we
> need to change the "GRANT ON ALL" patch a bit too for the sake of
> uniformity - don't we? There is indeed benefit in managing ACLs for
> existing objects, so that patch has some value too.
I agree that they should be consistant. The GRANT ON ALL shares alot
more of the syntax with GRANT than DefaultACL though, which makes it a
more interesting question there. I can understand not wanting to
duplicate the GRANT syntax. I think my suggestion would be to add a
field to the structure passed around by GRANT which indicates if 'VIEW'
was requested or not in the command. This could be used both for GRANT
ON ALL and to allow 'GRANT ON VIEW blah' to verify that the relation
being granted on is a view.
Thanks,
Stephen
Stephen Frost wrote: > > I agree that they should be consistant. The GRANT ON ALL shares alot > more of the syntax with GRANT than DefaultACL though, which makes it a > more interesting question there. I can understand not wanting to > duplicate the GRANT syntax. I think my suggestion would be to add a > field to the structure passed around by GRANT which indicates if 'VIEW' > was requested or not in the command. This could be used both for GRANT > ON ALL and to allow 'GRANT ON VIEW blah' to verify that the relation > being granted on is a view. > I arrived into this conclusion too, but it adds a lot of clutter in gram.y (setting that flag to false or something in many places, just to use in in one place). Originally I thought adding ACL_OBJECT_VIEW wasn't such a bad idea. But after I looked more closely at the code, it it seems to me that having same object type for VIEW and TABLE seems like the only logical reason why GRANT uses separate object type enum at all (instead of using subset of ObjectType like other commands do). If we went this path of separating VIEW and TABLE in GRANT code it might be cleaner to remove GrantObjectType and use ObjectType, but I don't think we want to do that. -- Regards Petr Jelinek (PJMODOS)
On Tue, Jul 14, 2009 at 11:10:00PM +0200, Petr Jelinek wrote: > Hello, > > this is first public version of our DefaultACLs patch as described on > http://wiki.postgresql.org/wiki/DefaultACL . I've been asked to review this. I can't get it to build, because of a missing semicolon on line 1608. I fixed it in my version, and it applied cleanly to head (with some offset hunks in gram.y). I've not yet finished building and testing; results to follow later. -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com
On Tue, Jul 14, 2009 at 11:10:00PM +0200, Petr Jelinek wrote: > Hello, > > this is first public version of our DefaultACLs patch as described on > http://wiki.postgresql.org/wiki/DefaultACL . Ok, here's my first crack at a comprehensive review. There's more I need to look at, eventually. Some of these are very minor stylistic comments, and some are probably just because I've much less of a clue, in general, than I'd like to think I have. First, as you've already pointed out, this needs documentation. Once I added the missing semicolon mentioned earlier, it applies and builds fine. The regression tests, however, seem to assume that they'll be run as the postgres user, and the privileges test failed. Here's part of a diff between expected/privileges.out and results/privileges.out as an example of what I mean: ALTER SCHEMA regressns DROP DEFAULT PRIVILEGES ON TABLE ALL FROM regressuser2; *************** *** 895,903 **** (1 row) SELECT relname, relacl FROM pg_class WHERE relname = 'acltest2'; ! relname | relacl ! ----------+------------------------------------------------------ ! acltest2 | {postgres=arwdDxt/postgres,regressgroup1=r/postgres} (1 row) CREATE FUNCTION regressns.testfunc1() RETURNSint AS 'select 1;' LANGUAGE sql; --- 895,903 ---- (1 row) SELECT relname, relacl FROM pg_class WHERE relname = 'acltest2'; ! relname | relacl ! ----------+------------------------------------------ ! acltest2 | {josh=arwdDxt/josh,regressgroup1=r/josh} (1 row) CREATE FUNCTION regressns.testfunc1() RETURNS int AS 'select1;' LANGUAGE sql; Very minor stylistic or comment issues: * There's a stray newline added in pg_class.h (no other changes were made to that file by this patch) * It feels to me like the comment "Continue with standard grant" in aclchk.c interrupts the flow of the code, though sucha comment was likely useful when the patch was being written. * pg_namespace_default_acl.h:71 should read "objects stored *in* pg_class" * The comment at the beginning of InsertPgClassTuple() in catalog/heap.c should probably be updated to say that relation'sACLs aren't always NULL by default * copy_from in gram.y got changed to to_from, but to_from isn't ever used in the default ACL grammar. I wondered if thiswas changed so you could use the same TO/FROM code as COPY uses, and then you decided to hardcode TO and FROM? In my perusal of the patch, I didn't see any code that screamed at me as though it were a bad idea; quite likely there weren't any really bad ideas but I can't say with confidence I'd have spotted them if there were. The addition of both the NSPDEFACLOBJ_* defines alongside the NSP_ACL_OBJ_* defines kinda made me think there were too many sets of constants that had to be kept in sync, but I'm not sure that's much of an issue in reality, given how unlikely it is that schema object types to which default ACLs should apply are likely to be added or removed. I don't know how patches that require catalog version changes are generally handled; should the patch include that change? More testing to follow. -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com
Joshua Tolley wrote: > I don't know how patches that require catalog version changes are generally > handled; should the patch include that change? > > The committer should handle that. cheers andrew
Joshua Tolley wrote: > First, as you've already pointed out, this needs documentation. > /me looks at Stephen ;) > Once I added the missing semicolon mentioned earlier, it applies and builds > As we discussed outside of list, my compiler didn't picked that one because I didn't use --enable-cassert . > fine. The regression tests, however, seem to assume that they'll be run as the > postgres user, and the privileges test failed. Oh I thought they are always run under *database user* postgres, my bad, I reworked them and the are probably better as a result. > Very minor stylistic or comment issues: > > * There's a stray newline added in pg_class.h (no other changes were made to > that file by this patch) > Fixed, probably I either pressed enter by mistake while viewing that file or it was some merging problem when updating my working copy. > * It feels to me like the comment "Continue with standard grant" in aclchk.c > interrupts the flow of the code, though such a comment was likely useful > when the patch was being written. > Ok I removed that comment. > * pg_namespace_default_acl.h:71 should read "objects stored *in* pg_class" > Fixed. > * The comment at the beginning of InsertPgClassTuple() in catalog/heap.c > should probably be updated to say that relation's ACLs aren't always NULL by > default > Done. > * copy_from in gram.y got changed to to_from, but to_from isn't ever used in > the default ACL grammar. I wondered if this was changed so you could use the > same TO/FROM code as COPY uses, and then you decided to hardcode TO and FROM? > Yes, that's more or less what happened. > In my perusal of the patch, I didn't see any code that screamed at me as > though it were a bad idea; quite likely there weren't any really bad ideas but > I can't say with confidence I'd have spotted them if there were. The addition > of both the NSPDEFACLOBJ_* defines alongside the NSP_ACL_OBJ_* defines kinda > made me think there were too many sets of constants that had to be kept in > sync, but I'm not sure that's much of an issue in reality, given how unlikely > it is that schema object types to which default ACLs should apply are likely > to be added or removed. > Well the problem you see is not really a problem IMHO because most of code I've seen does not use actual catalog values inside gram.y/parser so I didn't use them either. But there is one problem there that also affects GRANT ON ALL patch and that was discussed previously (see http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php and the rest of the thread from there). One (or both) of those patches will have to be adjusted but only commiter can decide that IMHO (I am happy to make any necessary changes but I really don't know which of the 3 solutions is better). > I don't know how patches that require catalog version changes are generally > handled; should the patch include that change? > Than can reasonably be done only at commit time so it's up to commiter. I attached updated version of the patch per your comments. Let's hope I didn't introduce new problems :) Thanks for your time. -- Regards Petr Jelinek (PJMODOS)
Attachment
Hello, while writing some basic docs I found bug in dependency handling when doing SET on object type that already had some default privileges. Attached patch fixes it, it also fixes thinko in parser (DROPing GRANT OPTION behaves like REVOKE now). And there is also initial version of those basic docs included (but you have to pardon my english as I didn't pass it to Stephen for proofreading due to discovery of that bug). -- Regards Petr Jelinek (PJMODOS)
Attachment
On Fri, Jul 17, 2009 at 9:46 PM, Joshua Tolley<eggyknap@gmail.com> wrote: > On Tue, Jul 14, 2009 at 11:10:00PM +0200, Petr Jelinek wrote: >> Hello, >> >> this is first public version of our DefaultACLs patch as described on >> http://wiki.postgresql.org/wiki/DefaultACL . > > Ok, here's my first crack at a comprehensive review. There's more I need to > look at, eventually. Some of these are very minor stylistic comments, and some > are probably just because I've much less of a clue, in general, than I'd like > to think I have. > > First, as you've already pointed out, this needs documentation. > > Once I added the missing semicolon mentioned earlier, it applies and builds > fine. The regression tests, however, seem to assume that they'll be run as the > postgres user, and the privileges test failed. Here's part of a diff between > expected/privileges.out and results/privileges.out as an example of what I > mean: > > ALTER SCHEMA regressns DROP DEFAULT PRIVILEGES ON TABLE ALL FROM > regressuser2; > *************** > *** 895,903 **** > (1 row) > > SELECT relname, relacl FROM pg_class WHERE relname = 'acltest2'; > ! relname | relacl > ! ----------+------------------------------------------------------ > ! acltest2 | {postgres=arwdDxt/postgres,regressgroup1=r/postgres} > (1 row) > > CREATE FUNCTION regressns.testfunc1() RETURNS int AS 'select 1;' LANGUAGE > sql; > --- 895,903 ---- > (1 row) > > SELECT relname, relacl FROM pg_class WHERE relname = 'acltest2'; > ! relname | relacl > ! ----------+------------------------------------------ > ! acltest2 | {josh=arwdDxt/josh,regressgroup1=r/josh} > (1 row) > > CREATE FUNCTION regressns.testfunc1() RETURNS int AS 'select 1;' LANGUAGE > sql; > > Very minor stylistic or comment issues: > > * There's a stray newline added in pg_class.h (no other changes were made to > that file by this patch) > * It feels to me like the comment "Continue with standard grant" in aclchk.c > interrupts the flow of the code, though such a comment was likely useful > when the patch was being written. > * pg_namespace_default_acl.h:71 should read "objects stored *in* pg_class" > * The comment at the beginning of InsertPgClassTuple() in catalog/heap.c > should probably be updated to say that relation's ACLs aren't always NULL by > default > * copy_from in gram.y got changed to to_from, but to_from isn't ever used in > the default ACL grammar. I wondered if this was changed so you could use the > same TO/FROM code as COPY uses, and then you decided to hardcode TO and FROM? > > In my perusal of the patch, I didn't see any code that screamed at me as > though it were a bad idea; quite likely there weren't any really bad ideas but > I can't say with confidence I'd have spotted them if there were. The addition > of both the NSPDEFACLOBJ_* defines alongside the NSP_ACL_OBJ_* defines kinda > made me think there were too many sets of constants that had to be kept in > sync, but I'm not sure that's much of an issue in reality, given how unlikely > it is that schema object types to which default ACLs should apply are likely > to be added or removed. > > I don't know how patches that require catalog version changes are generally > handled; should the patch include that change? > > More testing to follow. So are these warts fixed in the latest revision of this patch? http://archives.postgresql.org/pgsql-hackers/2009-07/msg01216.php I am gathering that this patch is still a bit of a WIP. I think it might be best to mark it returned with feedback and let Petr resubmit for the next CommitFest when it is closer to being done. But I don't want to do that if it's really already to go now. I am also a bit unsure as to whether Josh Tolley is still conducting a more in-depth review. Josh? Thanks, ...Robert
On Wed, Jul 22, 2009 at 08:54:19PM -0400, Robert Haas wrote: > I am gathering that this patch is still a bit of a WIP. I don't consider it a WIP. Petr posted a patch a couple of days ago, but I've not been able to verify its changes or perform some additional testing I had in mind, because of my own schedule. I probably should have made that clear a while ago. I consider the ball entirely in my court on this one, and plan to complete my review using the latest version of the patch as soon as my time permits; I expect this will happen before Saturday. > I am also a bit unsure as to whether Josh Tolley is still conducting a > more in-depth review. Josh? Yes, I am, but if you've read this far you know that already :) -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com
Robert Haas wrote: > So are these warts fixed in the latest revision of this patch? > > http://archives.postgresql.org/pgsql-hackers/2009-07/msg01216.php > > I am gathering that this patch is still a bit of a WIP. I think it > might be best to mark it returned with feedback and let Petr resubmit > for the next CommitFest when it is closer to being done. But I don't > want to do that if it's really already to go now. > See http://archives.postgresql.org/pgsql-hackers/2009-07/msg01151.php (the revision before the one you pasted). The docs are not complete but that's up to Stephen, otherwise the patch should be finished. But I am not the reviewer :) Anyway, while this patch might not necessary get commited in this commit fest, I'd still like to have opinion from one of the commiters on "the VIEW problem" which also affects grant on all patch ( see http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php ) and I fear "returned with feedback" might prevent that until next commit fest. -- Regards Petr Jelinek (PJMODOS)
On Wed, Jul 22, 2009 at 11:21 PM, Joshua Tolley<eggyknap@gmail.com> wrote: > On Wed, Jul 22, 2009 at 08:54:19PM -0400, Robert Haas wrote: >> I am gathering that this patch is still a bit of a WIP. > > I don't consider it a WIP. Petr posted a patch a couple of days ago, but I've > not been able to verify its changes or perform some additional testing I had > in mind, because of my own schedule. I probably should have made that clear a > while ago. I consider the ball entirely in my court on this one, and plan to > complete my review using the latest version of the patch as soon as my time > permits; I expect this will happen before Saturday. OK, I stand corrected. Thanks for the update. ...Robert
On Wed, Jul 22, 2009 at 11:26 PM, Petr Jelinek<pjmodos@pjmodos.net> wrote: > Robert Haas wrote: >> >> So are these warts fixed in the latest revision of this patch? >> >> http://archives.postgresql.org/pgsql-hackers/2009-07/msg01216.php >> >> I am gathering that this patch is still a bit of a WIP. I think it >> might be best to mark it returned with feedback and let Petr resubmit >> for the next CommitFest when it is closer to being done. But I don't >> want to do that if it's really already to go now. >> > > See http://archives.postgresql.org/pgsql-hackers/2009-07/msg01151.php (the > revision before the one you pasted). Ah, woops. Sorry I missed that. Actually, now that I read it, I remember that I saw it before, but I didn't remember that before sending my previous email, of course... > The docs are not complete but that's up to Stephen, otherwise the patch > should be finished. But I am not the reviewer :) Well, perhaps we had better prod Stephen then, since complete docs are a requirement for commit. > Anyway, while this patch might not necessary get commited in this commit > fest, I'd still like to have opinion from one of the commiters on "the VIEW > problem" which also affects grant on all patch ( see > http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php ) and I > fear "returned with feedback" might prevent that until next commit fest. OK, hopefully one of them will chime in with an opinion. As I say, I would like to see this get committed if it's done, I just wasn't sure it was. But now it seems that was due to insufficiently careful reading of the thread, with the exception of this issue and the need to finish the docs. Thanks, ...Robert
* Robert Haas (robertmhaas@gmail.com) wrote: > On Wed, Jul 22, 2009 at 11:26 PM, Petr Jelinek<pjmodos@pjmodos.net> wrote: > > The docs are not complete but that's up to Stephen, otherwise the patch > > should be finished. But I am not the reviewer :) > > Well, perhaps we had better prod Stephen then, since complete docs are > a requirement for commit. I didn't think the docs posted were terrible, but I am working on improving them. > > Anyway, while this patch might not necessary get commited in this commit > > fest, I'd still like to have opinion from one of the commiters on "the VIEW > > problem" which also affects grant on all patch ( see > > http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php ) and I > > fear "returned with feedback" might prevent that until next commit fest. > > OK, hopefully one of them will chime in with an opinion. As I say, I > would like to see this get committed if it's done, I just wasn't sure > it was. But now it seems that was due to insufficiently careful > reading of the thread, with the exception of this issue and the need > to finish the docs. Working on it... Not that we've never committed stuff with less than complete docs before.. ;) Thanks, Stephen
Hi, > >> Anyway, while this patch might not necessary get commited in this commit >> fest, I'd still like to have opinion from one of the commiters on "the VIEW >> problem" which also affects grant on all patch ( see >> http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php ) and I >> fear "returned with feedback" might prevent that until next commit fest. > > OK, hopefully one of them will chime in with an opinion. As I say, I > would like to see this get committed if it's done, I just wasn't sure > it was. But now it seems that was due to insufficiently careful > reading of the thread, with the exception of this issue and the need > to finish the docs. > IMO, the committers should have a go at the "GRANT ON ALL" (simpler than DefaultACLs) patch first. Whatever consensus is arrived for the VIEW issue as part of its review can then spill over to the DefaultACLs patch too. As mentioned by Petr, he does not seem to be in a hurry to get the DefaultACLs patch in, in this commitfest.. Regards, Nikhils -- http://www.enterprisedb.com
On Thursday 23 July 2009 06:26:05 Petr Jelinek wrote: > I'd still like to have opinion from one of the commiters on "the > VIEW problem" which also affects grant on all patch ( see > http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php ) and > I fear "returned with feedback" might prevent that until next commit fest. I see potential for confusion in that GRANT ON TABLE x works if x is a base table or a view, but GRANT ON ALL TABLES would not affect views. Maybe you need to make up a different syntax to affect only base tables, e.g., GRANT ON ALL BASE TABLES.
Peter Eisentraut wrote: <blockquote cite="mid:200907231254.45451.peter_e@gmx.net" type="cite"><pre wrap="">On Thursday 23July 2009 06:26:05 Petr Jelinek wrote: </pre><blockquote type="cite"><pre wrap="">I'd still like to have opinion from oneof the commiters on "the VIEW problem" which also affects grant on all patch ( see <a class="moz-txt-link-freetext" href="http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php">http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php</a> )and I fear "returned with feedback" might prevent that until next commit fest. </pre></blockquote><pre wrap=""> I see potential for confusion in that GRANT ON TABLE x works if x is a base table or a view, but GRANT ON ALL TABLES would not affect views. Maybe you need to make up a different syntax to affect only base tables, e.g., GRANT ON ALL BASE TABLES. </pre></blockquote><br /> That's not what I mean the problem is what is the best way of handling the viewsin implementation itself (there were IIRC 3 possible solutions devised and I don't think we have consensus on whichis better).<br /> In short, <br /> 1. add ACL_OBJECT_VIEW into GrantObjectType enum and track that inside code<br />2. create new enum with table, view, function and sequence objects in it (that works well for DefaultACLs but not for GRANTON ALL)<br /> 3. add some boolean into GrantStmt that would indicate that relation is a view (that works for GRANT ONALL but does not solve anything for DefaultACLs)<br /><br /> Currently DefaultACLs patch uses method 2 (because Stephendoes not like method 1) and GRANT ON ALL patch uses method 1 and it might be better if both patches uses only oneof those.<br /> If we went with method 1 we probably should just ditch GrantObjectType alltogether and work with subsetof ObjectType as other commands do (I haven't found any reason for GrantObjectType to exist other than having singleobject type for both TABLE and VIEW).<br /> And If we choose not to use method 1 then we should probably go with 2for DefaultACLs and 3 for GRANT ON ALL. That is unless somebody has a better solution.<br /><br /><pre class="moz-signature"cols="72">-- Regards Petr Jelinek (PJMODOS)</pre>
Hi, > I'd still like to have opinion from one of the commiters on "the > VIEW problem" which also affects grant on all patch ( see > http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php ) and > I fear "returned with feedback" might prevent that until next commit fest. > > > I see potential for confusion in that GRANT ON TABLE x works if x is a base > table or a view, but GRANT ON ALL TABLES would not affect views. Maybe you > need to make up a different syntax to affect only base tables, e.g., GRANT > ON > ALL BASE TABLES. > > > That's not what I mean the problem is what is the best way of handling the > views in implementation itself (there were IIRC 3 possible solutions devised > and I don't think we have consensus on which is better). Peter is raising a good question here and it's not related to the implementation. What he is saying is that in your new implementation if "GRANT ON ALL TABLES" is invoked, it will affect only RELKIND_TABLE objects. Whereas the GRANT ON TABLE affects both RELKIND_TABLE and RELKIND_VIEW types of objects (with and without your patch). We could have brought in the differentiation with this patch to treat views and tables separately. So a GRANT ON TABLE would just affect tables. But I guess that will break existing user scripts which assume it works against VIEWS too. I don't know how acceptable the "ON ALL BASE TABLES" sounds to all. Regards, Nikhils > In short, > 1. add ACL_OBJECT_VIEW into GrantObjectType enum and track that inside code > 2. create new enum with table, view, function and sequence objects in it > (that works well for DefaultACLs but not for GRANT ON ALL) > 3. add some boolean into GrantStmt that would indicate that relation is a > view (that works for GRANT ON ALL but does not solve anything for > DefaultACLs) > > Currently DefaultACLs patch uses method 2 (because Stephen does not like > method 1) and GRANT ON ALL patch uses method 1 and it might be better if > both patches uses only one of those. > If we went with method 1 we probably should just ditch GrantObjectType > alltogether and work with subset of ObjectType as other commands do (I > haven't found any reason for GrantObjectType to exist other than having > single object type for both TABLE and VIEW). > And If we choose not to use method 1 then we should probably go with 2 for > DefaultACLs and 3 for GRANT ON ALL. That is unless somebody has a better > solution. > > -- > Regards > Petr Jelinek (PJMODOS) -- http://www.enterprisedb.com
On Sun, Jul 19, 2009 at 06:13:32PM +0200, Petr Jelinek wrote: > Hello, > > while writing some basic docs I found bug in dependency handling when > doing SET on object type that already had some default privileges. > Attached patch fixes it, it also fixes thinko in parser (DROPing GRANT > OPTION behaves like REVOKE now). And there is also initial version of > those basic docs included (but you have to pardon my english as I didn't > pass it to Stephen for proofreading due to discovery of that bug). Am I the only one that gets this on make check, with this version (from src/test/regress/log/initdb.log): selecting default shared_buffers ... 32MB creating configuration files ... ok creating template1 database in /home/josh/devel/pgsrc/pg85/src/test/regress/./tmp_check/data/base/1 ... FATAL: relation"pg_namespace_default_acl" already exists child process exited with exit code 1 initdb: data directory "/home/josh/devel/pgsrc/pg85/src/test/regress/./tmp_check/data" not removed at user's request -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com
Joshua Tolley wrote: > Am I the only one that gets this on make check, with this version (from > src/test/regress/log/initdb.log): > > selecting default shared_buffers ... 32MB > creating configuration files ... ok > creating template1 database in /home/josh/devel/pgsrc/pg85/src/test/regress/./tmp_check/data/base/1 ... FATAL: relation"pg_namespace_default_acl" already exists > child process exited with exit code 1 > initdb: data directory "/home/josh/devel/pgsrc/pg85/src/test/regress/./tmp_check/data" not removed at user's request > Certainly never happened to me. Are you using any special parameters or something (altho I don't have the slightest idea what could cause that) ? I run make check on patches using gcc under debian and msvc on vista before sending them. -- Regards Petr Jelinek (PJMODOS)
On Sat, Jul 25, 2009 at 11:14:19AM +0200, Petr Jelinek wrote: > Joshua Tolley wrote: >> Am I the only one that gets this on make check, with this version (from >> src/test/regress/log/initdb.log): >> >> selecting default shared_buffers ... 32MB >> creating configuration files ... ok >> creating template1 database in /home/josh/devel/pgsrc/pg85/src/test/regress/./tmp_check/data/base/1 ... FATAL: relation"pg_namespace_default_acl" already exists >> child process exited with exit code 1 >> initdb: data directory "/home/josh/devel/pgsrc/pg85/src/test/regress/./tmp_check/data" not removed at user's request >> > Certainly never happened to me. Are you using any special parameters or > something (altho I don't have the slightest idea what could cause that) > ? I run make check on patches using gcc under debian and msvc on vista > before sending them. I figured as much. I can't seem to get past this, despite a make distclean. Suggestions, anyone? -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com
Joshua Tolley wrote: > I figured as much. I can't seem to get past this, despite a make distclean. > Suggestions, anyone? > > > try a fresh checkout and reapply the patch? cheers andrew
On Sat, Jul 25, 2009 at 03:50:06PM -0400, Andrew Dunstan wrote: > Joshua Tolley wrote: >> I figured as much. I can't seem to get past this, despite a make distclean. >> Suggestions, anyone? >> > try a fresh checkout and reapply the patch? [ a couple git clean, git reset, make clean, etc. commands later... ] Yeah, well, it works now. What's more, the problems I had with make check the first time I tried this are no longer. I've done all the looking I can think to do at the patch in its existing form, and don't have any complaints. I realize, though, that there are open questions about how this should work with, given the GRANT ON ALL patch. I'm not sure I have comments in that regard, but I'll try to come up with some, or at least to convince myself I don't have any for a better reason than just that I wouldn't know what I was talking about. In the meantime, I think this one is ready to be marked as ... something else. Ready for committer? -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com
On Sat, Jul 25, 2009 at 7:45 PM, Joshua Tolley<eggyknap@gmail.com> wrote: > that I wouldn't know what I was talking about. In the meantime, I think this > one is ready to be marked as ... something else. Ready for committer? Sounds right to me. ...Robert
On Sun, Jul 19, 2009 at 06:13:32PM +0200, Petr Jelinek wrote: > while writing some basic docs I found bug in dependency handling when > doing SET on object type that already had some default privileges. > Attached patch fixes it, it also fixes thinko in parser (DROPing GRANT > OPTION behaves like REVOKE now). And there is also initial version of > those basic docs included (but you have to pardon my english as I didn't > pass it to Stephen for proofreading due to discovery of that bug). Immediately after concluding I was done with my review, I realized I'd completely forgotten to look at the docs. I've made a few changes based solely on my opinions of what sounds better and what's more consistent with the existing documentation. Do with them as you see fit. :) -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com
On Sat, Jul 25, 2009 at 8:39 PM, Joshua Tolley<eggyknap@gmail.com> wrote: > On Sun, Jul 19, 2009 at 06:13:32PM +0200, Petr Jelinek wrote: >> while writing some basic docs I found bug in dependency handling when >> doing SET on object type that already had some default privileges. >> Attached patch fixes it, it also fixes thinko in parser (DROPing GRANT >> OPTION behaves like REVOKE now). And there is also initial version of >> those basic docs included (but you have to pardon my english as I didn't >> pass it to Stephen for proofreading due to discovery of that bug). > > Immediately after concluding I was done with my review, I realized I'd > completely forgotten to look at the docs. I've made a few changes based solely > on my opinions of what sounds better and what's more consistent with the > existing documentation. Do with them as you see fit. :) Did you intend to attach something to this email? ...Robert
On Sat, Jul 25, 2009 at 08:41:12PM -0400, Robert Haas wrote: > On Sat, Jul 25, 2009 at 8:39 PM, Joshua Tolley<eggyknap@gmail.com> wrote: > > On Sun, Jul 19, 2009 at 06:13:32PM +0200, Petr Jelinek wrote: > >> while writing some basic docs I found bug in dependency handling when > >> doing SET on object type that already had some default privileges. > >> Attached patch fixes it, it also fixes thinko in parser (DROPing GRANT > >> OPTION behaves like REVOKE now). And there is also initial version of > >> those basic docs included (but you have to pardon my english as I didn't > >> pass it to Stephen for proofreading due to discovery of that bug). > > > > Immediately after concluding I was done with my review, I realized I'd > > completely forgotten to look at the docs. I've made a few changes based solely > > on my opinions of what sounds better and what's more consistent with the > > existing documentation. Do with them as you see fit. :) > > Did you intend to attach something to this email? > > ...Robert Well, yes, now that you mention it :) Trying again... -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com
Attachment
Joshua Tolley weote: > On Sat, Jul 25, 2009 at 08:41:12PM -0400, Robert Haas wrote: > >> On Sat, Jul 25, 2009 at 8:39 PM, Joshua Tolley<eggyknap@gmail.com> wrote: >> >>> Immediately after concluding I was done with my review, I realized I'd >>> completely forgotten to look at the docs. I've made a few changes based solely >>> on my opinions of what sounds better and what's more consistent with the >>> existing documentation. Do with them as you see fit. :) >>> Applied with minor adjustments, attached updated patch. -- Regards Petr Jelinek (PJMODOS)
Attachment
* Stephen Frost (sfrost@snowman.net) wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
> > On Wed, Jul 22, 2009 at 11:26 PM, Petr Jelinek<pjmodos@pjmodos.net> wrote:
> > > The docs are not complete but that's up to Stephen, otherwise the patch
> > > should be finished. But I am not the reviewer :)
> >
> > Well, perhaps we had better prod Stephen then, since complete docs are
> > a requirement for commit.
>
> I didn't think the docs posted were terrible, but I am working on
> improving them.
Thanks to Joshua, there weren't really many changes I found for the
docs. Here they are anyway:
grant.sgml:
+ Replace current privileges with the ones specified using
+ <xref linkend="sql-alterschema" endterm="sql-alterschema-title">.
+ The <literal>WITH GRANT OPTION</literal> parameter is not applicable
+ because it is copied from default privileges.
+ Note: this can actually <emphasis role="bold">revoke</emphasis> some
+ privileges because it clears all existing privileges object has and
+ replaces them with the default ones for the schema in which this object
+ resides.
How about:
Replaces current privileges with the default privileges, as set using
<xref linkend="sql-alterschema" endterm="sql-alterschema-title">, for
this object type in its current schema.
The <literal>WITH GRANT OPTION</literal> parameter is not applicable
because it is copied from default privileges.
Note: This can <emphasis role="bold">revoke</emphasis> privileges! This
will clear all existing privileges first! If no default privilege has
been set, the object will have all privileges revoked!
Otherwise, I thought the docs looked pretty good.
Thanks,
Stephen
On Tue, Aug 04, 2009 at 01:28:00PM -0400, Stephen Frost wrote: > Thanks to Joshua, there weren't really many changes I found for the > docs. Here they are anyway: Yay, I was useful! :) > How about: > > Replaces current privileges with the default privileges, as set using > <xref linkend="sql-alterschema" endterm="sql-alterschema-title">, for > this object type in its current schema. > The <literal>WITH GRANT OPTION</literal> parameter is not applicable > because it is copied from default privileges. > Note: This can <emphasis role="bold">revoke</emphasis> privileges! This > will clear all existing privileges first! If no default privilege has > been set, the object will have all privileges revoked! > > Otherwise, I thought the docs looked pretty good. No complaints here, FWIW. - Josh
I looked at this patch a bit. I haven't actually read any of the code
yet, just reviewed the on-list discussions and the docs, but I think I can
make a few comments at the definitional level.
First: there's already been some discussion about the VIEW problem.
I understand that the patch adds a "GRANT ON VIEW" syntax that works
like "GRANT ON SEQUENCE" in that if you say VIEW it insists the object
really is a view, but without taking away the existing laxness that
allows you to still say GRANT ON TABLE for a view. This seems reasonable
in isolation, but it creates a big problem for both this patch and the
GRANT ON ALL patch, in that it's unclear how to treat views if they
could be considered either tables or views. Unfortunately I think
we have no choice but to live with the "existing laxness", because
(a) to do otherwise will break pretty nearly every pg_dump script, and
(b) the SQL standard says we have to accept "GRANT ON TABLE view".
In fact "GRANT ON VIEW" is not in the SQL standard and there is no good
reason to expect applications will adopt it at all.
So my feeling is that adding GRANT ON VIEW is a bad idea. The main
argument for doing it seemed to be that the author wanted to be able
to grant different default privileges for tables and views, but I'm
unconvinced that there's a strong use-case for that. You could very
easily have one set of default privileges for all of tables, views,
and sequences, simply ignoring any bits that weren't relevant for
particular objects. It seems to me that that would handle common
cases just fine. For complicated cases not so much, but it's not
clear to me that filtering by the object subtype is all that useful
for complicated cases anyway. People will want more functionality
than that. Which leads into my next item.
Second: both this patch and GRANT ON ALL are built on the assumption
that the only way to filter/classify objects is by schema membership.
Now I don't object to that as an initial implementation restriction,
but I don't like hard-wiring it into the syntax. It is very clear to me
that we'll want other filter rules in the future --- an immediate example
is being able to say that new children of an inheritance parent table
should inherit its GRANTs. So to my mind, designing the syntax around
ALTER SCHEMA is right out. Maybe we could do something likeALTER DEFAULT PRIVILEGES ON TABLES IN SCHEMA foo GRANT ...
where the "IN SCHEMA foo" part would be subject to generalization later.
This also matches up a bit better with the proposed syntax for GRANT
ON ALL (which also uses "IN SCHEMA foo").
Third: speaking of syntax, I don't like the way that this is gratituously
different from GRANT/REVOKE. I don't like using ADD/DROP instead of
GRANT/REVOKE, nor the unnecessary AND business. I think we should
minimize confusion by using something that is spelled as nearly like
GRANT/REVOKE as possible.
Fourth: the system's existing treatment of default permissions is
owner-dependent, that is the implied set of permissions is typically
GRANT ALL TO <owner> (from himself, with grant option). I do not
understand how schema-level default ACLs will play nicely with that,
except in the special case where the schema owner also owns every
contained object. If you copy the schema-level ACL directly to a
contained object with a different owner it will definitely be the wrong
thing, but if you try to translate the ownership it will confuse people
too. And confusion in a security-related behavior is a Bad Thing.
Furthermore, having the schema owner able to control the grants made
for objects not owned by him is a huge security hole.
What I suggest as a way to resolve this last point is that a default ACL
should apply only to objects owned by the user who creates/modifies the
default ACL. In this view, the question of which schema the objects are
in is just an additional filter condition, not the primary determinant
of which objects a default ACL applies to. Every user has his own set
of default ACLs. (This is another reason not to design the syntax
around ALTER SCHEMA.)
regards, tom lane
Tom Lane wrote: > So my feeling is that adding GRANT ON VIEW is a bad idea. The main > argument for doing it seemed to be that the author wanted to be able > to grant different default privileges for tables and views, but I'm > unconvinced that there's a strong use-case for that. You could very > Yes that was the intention. I do have users in my databases with access privileges to VIEWs but not to underlying TABLEs so it seemed like a good idea to be able to do that with DefaultACLs and GRANT ON ALL. > Second: both this patch and GRANT ON ALL are built on the assumption > that the only way to filter/classify objects is by schema membership. > Now I don't object to that as an initial implementation restriction, > but I don't like hard-wiring it into the syntax. It is very clear to me > that we'll want other filter rules in the future --- an immediate example > is being able to say that new children of an inheritance parent table > should inherit its GRANTs. So to my mind, designing the syntax around > ALTER SCHEMA is right out. Maybe we could do something like > ALTER DEFAULT PRIVILEGES ON TABLES IN SCHEMA foo GRANT ... > where the "IN SCHEMA foo" part would be subject to generalization later. > This also matches up a bit better with the proposed syntax for GRANT > ON ALL (which also uses "IN SCHEMA foo"). > Actually I was planning to extend GRANT ON ALL - if it was accepted - to include more filters (something like OWNED BY for example). > Third: speaking of syntax, I don't like the way that this is gratituously > different from GRANT/REVOKE. I don't like using ADD/DROP instead of > GRANT/REVOKE, nor the unnecessary AND business. I think we should > minimize confusion by using something that is spelled as nearly like > GRANT/REVOKE as possible. > ADD/DROP was side product of having SET and that "unnecessary AND business". If we went with that syntax you proposed we could just put exact same syntax as GRANT and REVOKE after the filtering option, that should be close enough :). I remember Stephen being against having GRANT in ALTER SCHEMA but I doubt he would be against having it in completely new ALTER DEFAULT PRIVILEGES statement. > Fourth: the system's existing treatment of default permissions is > owner-dependent, that is the implied set of permissions is typically > GRANT ALL TO <owner> (from himself, with grant option). I do not > understand how schema-level default ACLs will play nicely with that, > except in the special case where the schema owner also owns every > contained object. If you copy the schema-level ACL directly to a > contained object with a different owner it will definitely be the wrong > thing, but if you try to translate the ownership it will confuse people > too. And confusion in a security-related behavior is a Bad Thing. > Furthermore, having the schema owner able to control the grants made > for objects not owned by him is a huge security hole. > We were actually discussing this with Stephen yesterday as something similar occurred to me too. The patch as submitted just does the copy, after the discussion I added post-processing on object creation which translates everything to owner but I guess there is no point in submitting that now. > What I suggest as a way to resolve this last point is that a default ACL > should apply only to objects owned by the user who creates/modifies the > default ACL. In this view, the question of which schema the objects are > in is just an additional filter condition, not the primary determinant > of which objects a default ACL applies to. Every user has his own set > of default ACLs. > We could certainly do that. I wonder what we should do about inheritance of default privileges between the roles if we did this - should it just be what I set is mine and my parent roles do not affect me or should it get default privs from parent roles and merge them with mine when I create the object ? Also when creating new default privileges entry should we use some template which would give owner all privileges like GRANT does when there are no existing privileges on object or should we just use blank and leave it to user to grant himself default privileges on objects he will create ? I don't think there is any point in you looking at code since DefaultACLs might need serious rewriting. GRANT ON ALL is a bit different story, there I can just remove all that VIEW stuff, although I would very much like to have the ability to affect only VIEWs. On the other hand removing VIEW as separate object type would remove my biggest problem with how both patches are implemented (I mentioned it few times in the previous discussion) so from that standpoint it might be a good thing. -- Regards Petr Jelinek (PJMODOS)
Petr Jelinek <pjmodos@pjmodos.net> writes:
> Tom Lane wrote:
>> What I suggest as a way to resolve this last point is that a default ACL
>> should apply only to objects owned by the user who creates/modifies the
>> default ACL. In this view, the question of which schema the objects are
>> in is just an additional filter condition, not the primary determinant
>> of which objects a default ACL applies to. Every user has his own set
>> of default ACLs.
>>
> We could certainly do that. I wonder what we should do about inheritance
> of default privileges between the roles if we did this - should it just
> be what I set is mine and my parent roles do not affect me or should it
> get default privs from parent roles and merge them with mine when I
> create the object ?
I don't believe there is any "inheritance" needed or involved. A
default ACL would only be looked up for use at the instant of creating
an object, and what you'd look for is one owned by the same userID that
is going to own the object being created. Anything else will be too
complicated to be understandable. The commands that actually
create/alter a default ACL would work on those belonging to whatever
the effective userID is.
> Also when creating new default privileges entry
> should we use some template which would give owner all privileges like
> GRANT does when there are no existing privileges on object or should we
> just use blank and leave it to user to grant himself default privileges
> on objects he will create ?
It should start from the same initial state you'd have if you didn't
have a default ACL. Anything else violates the principle of least
astonishment.
regards, tom lane
I had some time to work on this patch, and I implemented the ALTER DEFAULT PRIVILEGES syntax as proposed by Tom and adjusted some other stuff, but before I can submit the new patch for commitfest there is still this fundamental issue about how it should behave. The situation is as following. Josh's and Stephen's idea was basically to solve something like this: you are a dba, you give some users privileges to create tables and you want those new tables to have same privileges no matter who created them. But if I understood Tom's suggestions correctly then his approach does not solve this at all since every one of those users with CREATE TABLE privileges would have to also set same DEFAULT PRIVILEGES and the dba would have no say in the matter. I personally can see use cases for both but I don't really see any reasonable way to have both at the same time. -- Regards Petr Jelinek (PJMODOS)
Petr, > But if I understood Tom's suggestions correctly then his approach does > not solve this at all since every one of those users with CREATE TABLE > privileges would have to also set same DEFAULT PRIVILEGES and the dba > would have no say in the matter. This latter approach benefits nobody. If default's can't be set by the DBA centrally, the feature is useless. -- Josh Berkus PostgreSQL Experts Inc. www.pgexperts.com
Josh Berkus wrote:
So I've been working on solution with which I am happy with (does not mean anybody else will be also though).
I created a new version with syntax devised by Tom and one which is user centric, but also one with which DBA can control default privileges.
The attached patch adds syntax in this format:
ALTER DEFAULT PRIVILEGES [ IN SCHEMA schema_name(s) ] [ FOR ROLE role_name(s) ] GRANT privileges ON object_type TO role(s);
Obviously it sets default privileges for new objects of given object type created by role(s) specified using FOR ROLE (is that syntax ok?) clause and inside specified schema(s).
If user omits IN SCHEMA it applies database wide. Database wide settings are used only if there is nothing specified for current schema (ie no cascading/inheritance).
If FOR ROLE is omitted then the privileges are set for current role. Only superusers and users with ADMIN privilege (we might want to add specific privilege for this but ADMIN seems suitable to me) granted on the role can use FOR ROLE clause.
The order of FOR ROLE and IN SCHEMA clauses does not matter.
Some of my thoughts on the changed behavior of the patch:
There is no need to be schema owner anymore in this implementation since the privileges are handled quite differently.
There are no longer issues about who should be grantor (discussed on IRC only, there was problem that schema owner as grantor didn't seem logical and we didn't know owner at the time we created default privileges).
Also there is no longer a problem with what should be template for privileges because we now know the owner of the object at default privileges creation time (which we didn't before as it was all schema based) so we can use standard template as used by GRANT.
The whole thing is more consistent with GRANT.
The patch is also a bit smaller :)
It's not as easy to do the original idea of setting default privileges for schema for all users with CREATE privilege on schema but it can still be done, one just have to update default privileges every time somebody is granted that privilege, and DBA can still have control over it all.
Hopefully this will at least inspire some more discussion on the matter.
I agree, however I assume I understood Tom properly since he didn't reply.But if I understood Tom's suggestions correctly then his approach does not solve this at all since every one of those users with CREATE TABLE privileges would have to also set same DEFAULT PRIVILEGES and the dba would have no say in the matter.This latter approach benefits nobody. If default's can't be set by the DBA centrally, the feature is useless.
So I've been working on solution with which I am happy with (does not mean anybody else will be also though).
I created a new version with syntax devised by Tom and one which is user centric, but also one with which DBA can control default privileges.
The attached patch adds syntax in this format:
ALTER DEFAULT PRIVILEGES [ IN SCHEMA schema_name(s) ] [ FOR ROLE role_name(s) ] GRANT privileges ON object_type TO role(s);
Obviously it sets default privileges for new objects of given object type created by role(s) specified using FOR ROLE (is that syntax ok?) clause and inside specified schema(s).
If user omits IN SCHEMA it applies database wide. Database wide settings are used only if there is nothing specified for current schema (ie no cascading/inheritance).
If FOR ROLE is omitted then the privileges are set for current role. Only superusers and users with ADMIN privilege (we might want to add specific privilege for this but ADMIN seems suitable to me) granted on the role can use FOR ROLE clause.
The order of FOR ROLE and IN SCHEMA clauses does not matter.
Some of my thoughts on the changed behavior of the patch:
There is no need to be schema owner anymore in this implementation since the privileges are handled quite differently.
There are no longer issues about who should be grantor (discussed on IRC only, there was problem that schema owner as grantor didn't seem logical and we didn't know owner at the time we created default privileges).
Also there is no longer a problem with what should be template for privileges because we now know the owner of the object at default privileges creation time (which we didn't before as it was all schema based) so we can use standard template as used by GRANT.
The whole thing is more consistent with GRANT.
The patch is also a bit smaller :)
It's not as easy to do the original idea of setting default privileges for schema for all users with CREATE privilege on schema but it can still be done, one just have to update default privileges every time somebody is granted that privilege, and DBA can still have control over it all.
Hopefully this will at least inspire some more discussion on the matter.
-- Regards Petr Jelinek (PJMODOS)
Attachment
Petr, > It's not as easy to do the original idea of setting default privileges > for schema for all users with CREATE privilege on schema but it can > still be done, one just have to update default privileges every time > somebody is granted that privilege, and DBA can still have control over > it all. Sounds like a good solution. Thanks for persisting with this. -- Josh Berkus PostgreSQL Experts Inc. www.pgexperts.com
Petr Jelinek wrote: > So I've been working on solution with which I am happy with (does not > mean anybody else will be also though). Hi Petr, I'm reviewing this patch and after reading it I have some comments. Unfortunately, when I got to the compiling part, it turned out that the attached patch is missing src/include/catalog/pg_default_acls.h (or is it just me?). I'll continue reviewing while waiting for a new patch with that header file and in any case will send the full review tommorrow. Cheers, Jan
Jan Urbański napsal(a):
Attached patch should have the missing file (otherwise it's same).
Hmm looks like I forgot to git add that file in the new branch, sorry about that.Petr Jelinek wrote:So I've been working on solution with which I am happy with (does not mean anybody else will be also though).Hi Petr, I'm reviewing this patch and after reading it I have some comments. Unfortunately, when I got to the compiling part, it turned out that the attached patch is missing src/include/catalog/pg_default_acls.h (or is it just me?). I'll continue reviewing while waiting for a new patch with that header file and in any case will send the full review tommorrow.
Attached patch should have the missing file (otherwise it's same).
-- Regards Petr Jelinek (PJMODOS)
Attachment
Hi,
here's a (late, sorry about that) review:
== Trivia ==
Patch applies cleanly with a few 1 line offsets.
It's unified, not context, but that's trivial.
The patch adds some trailing whitespace, which is not good (git diff
shows it in red, it's easy to spot it). There's also one
hunk that's just an addition of a newline (in
src/backend/catalog/aclchk.c, -270,6 +291,7)
== Code ==
There's a few places where the following pattern is used:
if (!stmt->grantees)
whereas I think the project prefers:
stmt->grantees != NIL
Same for if (schemas) => if (schemas != NULL)
I'm not sure if this pattern in src/backend/catalog/aclchk.c is the best
option:
if (rolenames == NIL) rolenames = lappend(rolenames, makeString(pstrdup("")));
if (nspnames == NIL) nspnames = lappend(nspnames, makeString(pstrdup("")));
Appending an empty string and then checking in strlen of the option
value is 0
is ugly.
In SetDefaultACLs the OidIsValid(roleId) is not necessary, maybe better
put in
assert(oidIsValid). The caller always puts a valid rolename in there. Or
maybe
even better: make the caller pass an InvalidOid for the role if no is
specified. This way the handling arguments is more uniform between
SetDefaultACLs and ExecDefaultACLsStmt.
The logic in get_default_acl and pg_namespace_object_default_acl could be
improved, for instance get_default_acl checks if the result of the scan
is null
and if is, returns NULL. At the same time, the only calling function,
pg_namespace_object_default_acl checks the isNull flag instead of just
checking
if the result is NULL.
Also, pg_namespace_object_default_acl could just do without the isNull out
parameter and the same goes for get_default_acl. Just return NULL to
indicate
an invalid result and declare a isNull in get_default_acl locally to use
it in
heap_getattr. This also saves some lines in InsertPgClassTuple.
Also, in InsertPgClassTuple change the order of the branches:
+ if (isNull)
+ nulls[Anum_pg_class_relacl - 1] = true;
+ else
+ values[Anum_pg_class_relacl - 1] = PointerGetDatum(relacl);
to have the same pattern as the next if statement.
Also, changing tests like if (!isNull) to if (relacl) makes it more
natural to
see sequences like if (relacl) { do-stuff; pfree(relacl); }
In ExecDefaultACLsStmt this fragment:
else if (strcmp(defel->defname, "roles") == 0)
{ if (rolenames) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting
orredundant options"))); drolenames = defel;
}
Should test if (drolenames), because currently it's possible to do:
alter default privileges for role test for role test grant select, insert on
table to test;
Maybe add a unit test for that.
A comment in dependency.c function getDefaultACLDescription with
something like
"shouldn't get here" in the default: switch branch could be useful,
cf. getRelationDescription.
In ExecGrantDefaults_Relation there's a hunk:
+ if (isNull)
+ elog(ERROR, "no DEFAULT PRIVILEGES for relation");
Maybe you could move it higher, no need to do other stuff if it's going
to fail
afterwards anyway.
ExecGrantDefaults_Function and ExecGrantDefaults_Relation could maybe share
code? They look quite similar, although it might not be so easy to
factor out
common functionality.
The "unrecognized GrantStmt.objtype: %d" error message needs better
wording I
think.
No code patch removes rows from pg_default_acls, so it might accumulate
cruft. Maybe a drop default privileges? Or maybe revoking all would delete
the row instead of setting it? It has the same meaning, I guess...
== Compiling and installing ==
My gcc complains about
gram.y: In function ‘base_yyparse’:
gram.y:1128: warning: assignment from incompatible pointer type
gram.y:1135: warning: passing argument 1 of ‘lappend’ from incompatible
pointer type
gram.y:1135: warning: assignment from incompatible pointer type
gram.y:1136: warning: assignment from incompatible pointer type
Regression tests fail because of the username mismatch
! DETAIL: drop cascades to default acls for role postgres on new
relation in namespace regressns
--- 951,957 ----
! DETAIL: drop cascades to default acls for role wulczer on new
relation in namespace regressns
== Testing ==
Tab completion is not up to speed - annoying ;)
The functionality worked as advertised, although I was able to do the
following:
postgres=# create role test login;
CREATE ROLE
postgres=# \c - test
psql (8.5devel)
You are now connected to database "postgres" as user "test".
postgres=> alter default privileges grant select on table to test;
ALTER DEFAULT PRIVILEGES
postgres=> \c - wulczer
psql (8.5devel)
You are now connected to database "postgres" as user "wulczer".
postgres=# drop role test;
DROP ROLE
postgres=# select * from pg_default_acls ;defaclrole | defaclnamespace | defaclobjtype | defacllist
------------+-----------------+---------------+----------------------- 16384 | 0 | r |
{16384=arwdDxt/16384}
The numeric defaclrole and defacllist don't look good.
While testing I also got a "unexpected object type: 1248" from
src/backend/catalog/pg_shdepend.c, but was unable to reproduce it.
This happened after I did a combination of DROP OWNED BY and REASSIGN
OWNED for
a role that has membership in other roles and that all had default
privileges
in a schema.
== Docs ==
Need to document that FOR USER also works (as a synonym for FOR ROLE) in the
synopsis of ALTER DEFAULT PRIVILEGES.
Add examples of usage of the FOR USER/FOR ROLE syntax and explain what
they do.
In the ALTER DEFAULT PRIVILEGES synopsis there's a copy-paste-o because
it says
you can do:
ALTER DEFAULT PRIVILEGES IN SCHEMA s GRANT DEFAULT PRIVILEGES ON TABLE
TO test;
The wording of "with grant options parameter is not applicable" in the
sql-grant page should mentiont that you also can't do:
GRANT DEFAULT PRIVILEGES ON s.test2 TO test;
so it should be indicated the "TO rolename" is also not applicable.
== Other ==
I'm not really sure what's the use of GRANT DEFAULT PRIVILEGES... Is it for
backward-sanitizing existing databases?
I haven't tested performance at all, I thinks it doesn't make sense for
a patch
like this.
Don't know about the relationship with the spec, I suspect there's no such
thing in there. But the feature has been reworked to match what the
community
wanted and I think that the syntax is quite good and the use cases are
there.
Not tested on Windows, probably not important.
Having said all that, I'm moving the patch to "Waiting on author".
Best,
Jan
Jan Urbański napsal(a): > Hi, > > here's a (late, sorry about that) review: > Thanks for the comprehensive review! > It's unified, not context, but that's trivial. > It's not, I have git configured to produce context diffs (see http://wiki.postgresql.org/wiki/Working_with_Git#Context_diffs_with_Git ). > == Code == > > There's a few places where the following pattern is used: > if (!stmt->grantees) > whereas I think the project prefers: > stmt->grantees != NIL > Ok changed that. > I'm not sure if this pattern in src/backend/catalog/aclchk.c is the best > option: > if (rolenames == NIL) > rolenames = lappend(rolenames, makeString(pstrdup(""))); > if (nspnames == NIL) > nspnames = lappend(nspnames, makeString(pstrdup(""))); > Appending an empty string and then checking in strlen of the option > value is 0 > is ugly. > I changed that, I had to change the way SetDefaultACLs is called a bit anyway (to fix the problem with dependencies you mentioned below). > In SetDefaultACLs the OidIsValid(roleId) is not necessary, maybe better > put in > assert(oidIsValid). The caller always puts a valid rolename in there. Or > maybe > even better: make the caller pass an InvalidOid for the role if no is > specified. This way the handling arguments is more uniform between > SetDefaultACLs and ExecDefaultACLsStmt. > Same as above. > The logic in get_default_acl and pg_namespace_object_default_acl could be > improved, for instance get_default_acl checks if the result of the scan > is null > and if is, returns NULL. At the same time, the only calling function, > pg_namespace_object_default_acl checks the isNull flag instead of just > checking > if the result is NULL. > > Also, pg_namespace_object_default_acl could just do without the isNull out > parameter and the same goes for get_default_acl. Just return NULL to > indicate > an invalid result and declare a isNull in get_default_acl locally to use > it in > heap_getattr. This also saves some lines in InsertPgClassTuple. > I agree, btw it actually does not save anything in InsertPgClassTuple, but it does save few lines in ProcedureCreate. > Also, in InsertPgClassTuple change the order of the branches: > + if (isNull) > + nulls[Anum_pg_class_relacl - 1] = true; > + else > + values[Anum_pg_class_relacl - 1] = PointerGetDatum(relacl); > to have the same pattern as the next if statement. > Done. > In ExecDefaultACLsStmt this fragment: > else if (strcmp(defel->defname, "roles") == 0) > { > if (rolenames) > ereport(ERROR, > (errcode(ERRCODE_SYNTAX_ERROR), > errmsg("conflicting or redundant options"))); > drolenames = defel; > } > Should test if (drolenames), because currently it's possible to do: > Typo, fixed. > A comment in dependency.c function getDefaultACLDescription with > something like > "shouldn't get here" in the default: switch branch could be useful, > cf. getRelationDescription. > Ok. > In ExecGrantDefaults_Relation there's a hunk: > + if (isNull) > + elog(ERROR, "no DEFAULT PRIVILEGES for relation"); > Maybe you could move it higher, no need to do other stuff if it's going > to fail > afterwards anyway. > Ok, I moved it just after owner check. > ExecGrantDefaults_Function and ExecGrantDefaults_Relation could maybe share > code? They look quite similar, although it might not be so easy to > factor out > common functionality. > They don't really, they operate on different tables and the only partly shared part is the indexes and acl dependency update. > The "unrecognized GrantStmt.objtype: %d" error message needs better > wording I > think. > Well, that exact same message is in other two places in original code, I just copy pasted it. > No code patch removes rows from pg_default_acls, so it might accumulate > cruft. Maybe a drop default privileges? Or maybe revoking all would delete > the row instead of setting it? It has the same meaning, I guess... > Now that I fixed DefaultACLs removal on role drop this is no longer true (previously it was only dropped with schema a leftover from original design). Also revoking everything is not same as having no DefaultACLs, with no DefaultACLs current behavior is used (owner gets all privs and public might get something depending on object type). > == Compiling and installing == > > My gcc complains about > > gram.y: In function ‘base_yyparse’: > gram.y:1128: warning: assignment from incompatible pointer type > gram.y:1135: warning: passing argument 1 of ‘lappend’ from incompatible > pointer type > gram.y:1135: warning: assignment from incompatible pointer type > gram.y:1136: warning: assignment from incompatible pointer type > Fixed. > Regression tests fail because of the username mismatch > > ! DETAIL: drop cascades to default acls for role postgres on new > relation in namespace regressns > --- 951,957 ---- > ! DETAIL: drop cascades to default acls for role wulczer on new > relation in namespace regressns > Removed that part from regression. > == Testing == > > The functionality worked as advertised, although I was able to do the > following: > > postgres=# create role test login; > CREATE ROLE > postgres=# \c - test > psql (8.5devel) > You are now connected to database "postgres" as user "test". > postgres=> alter default privileges grant select on table to test; > ALTER DEFAULT PRIVILEGES > postgres=> \c - wulczer > psql (8.5devel) > You are now connected to database "postgres" as user "wulczer". > postgres=# drop role test; > DROP ROLE > postgres=# select * from pg_default_acls ; > defaclrole | defaclnamespace | defaclobjtype | defacllist > ------------+-----------------+---------------+----------------------- > 16384 | 0 | r | {16384=arwdDxt/16384} > > The numeric defaclrole and defacllist don't look good. > Fixed, DefaultACLs for role are now dropped in DropRole. > While testing I also got a "unexpected object type: 1248" from > src/backend/catalog/pg_shdepend.c, but was unable to reproduce it. > This happened after I did a combination of DROP OWNED BY and REASSIGN > OWNED for > a role that has membership in other roles and that all had default > privileges > in a schema. > Yeah that was because I was adding ACL dependencies and wasn't removing them in DROP OWNED BY. Fixed. This is why I had to change the way SetDefaultACLs is called, so it could be called from DROP OWNED BY. > == Docs == > > Need to document that FOR USER also works (as a synonym for FOR ROLE) in the > synopsis of ALTER DEFAULT PRIVILEGES. > Added. > Add examples of usage of the FOR USER/FOR ROLE syntax and explain what > they do. > Added simple example. > In the ALTER DEFAULT PRIVILEGES synopsis there's a copy-paste-o because > it says > you can do: > ALTER DEFAULT PRIVILEGES IN SCHEMA s GRANT DEFAULT PRIVILEGES ON TABLE > TO test; > Eh, right, fixed. > The wording of "with grant options parameter is not applicable" in the > sql-grant page should mentiont that you also can't do: > GRANT DEFAULT PRIVILEGES ON s.test2 TO test; > so it should be indicated the "TO rolename" is also not applicable. > Done. > == Other == > > I'm not really sure what's the use of GRANT DEFAULT PRIVILEGES... Is it for > backward-sanitizing existing databases? > Yes (especially in combination with GRANT ON ALL if it gets in). > Not tested on Windows, probably not important. > I did test it on Windows ;) > Having said all that, I'm moving the patch to "Waiting on author". > I'll changed it back to "needs review" since I made quite a few changes (per your review). -- Regards Petr Jelinek (PJMODOS)
Attachment
I made some more small adjustments - mainly renaming stuff after Tom's comment on anonymous code blocks patch and removed one unused shared dependency. -- Regards Petr Jelinek (PJMODOS)
Attachment
Petr Jelinek wrote: > I made some more small adjustments - mainly renaming stuff after Tom's > comment on anonymous code blocks patch and removed one unused shared > dependency. Hi, the patch still has some issues with dependency handling: postgres=# create role test; CREATE ROLE postgres=# create role test2; CREATE ROLE postgres=# create schema s; CREATE SCHEMA postgres=# alter default privileges in schema s for user test2 grant insert on table to test; ALTER DEFAULT PRIVILEGES postgres=# drop role test2; DROP ROLE postgres=# drop schema s; ERROR: could not find tuple for default acls 16387 postgres=# At this moment pg_default_acls is empty and schema s is undroppable... I got an unexpected server exit after that once, when I executed \ds from psql, but unfortunately don't have a backtrace (forgot to ulimit -c unlimited). The next time I tried to provoke that backend crash I failed: after the "ERROR: could not find tuple for default acls 16387" I'm only stuck with an undroppable schema, but the rest of the system works normally. Apart from that all my complains from the previous review seem to be addressed, except for the tab completion... Not sure if it's mandatory for commit, but it sure would be useful. Marking as "Waiting on Author". Cheers, Jan
Jan Urbański napsal(a):
Fixed and added regression test for dependency handling.
Petr Jelinek wrote:I made some more small adjustments - mainly renaming stuff after Tom's comment on anonymous code blocks patch and removed one unused shared dependency.Hi, the patch still has some issues with dependency handling: postgres=# create role test; CREATE ROLE postgres=# create role test2; CREATE ROLE postgres=# create schema s; CREATE SCHEMA postgres=# alter default privileges in schema s for user test2 grant insert on table to test; ALTER DEFAULT PRIVILEGES postgres=# drop role test2; DROP ROLE postgres=# drop schema s; ERROR: could not find tuple for default acls 16387 postgres=#
Fixed and added regression test for dependency handling.
-- Regards Petr Jelinek (PJMODOS)
Attachment
OK, the previous problem went away, but I can still do something like that:
postgres=# create role test;
CREATE ROLE
postgres=# create role test2;
CREATE ROLE
postgres=# create database db;
CREATE DATABASE
postgres=# \c db
psql (8.5devel)
You are now connected to database "db".
db=# alter default privileges for role test2 grant insert on table to test;
ALTER DEFAULT PRIVILEGES
db=# \c postgres
psql (8.5devel)
You are now connected to database "postgres".
postgres=# drop role test2;
DROP ROLE
postgres=# \c db
psql (8.5devel)
You are now connected to database "db".
db=# select * from pg_default_acls ;defaclrole | defaclnamespace | defaclobjtype | defacllist
------------+-----------------+---------------+-------------------------------------- 16385 | 0 | r
|
{16385=arwdDxt/16385,test=a/wulczer}
(1 row)
db=#
Dependencies suck, I know..
Jan
Jan Urbański napsal(a): > Dependencies suck, I know.. > Cross-database dependencies do. I had to make target role owner of the default acls which adds some side effects like the fact that it blocks DROP ROLE so DROP OWNED BY has to be used. As for REASSIGN OWNED, it does not reassign anything (I don't think it's a good idea to reassign default acls) it just spits warning with hint what to do if user plans to drop the role. -- Regards Petr Jelinek (PJMODOS)
Attachment
Petr Jelinek wrote: > Jan Urbański napsal(a): >> Dependencies suck, I know.. >> > > Cross-database dependencies do. > > I had to make target role owner of the default acls which adds some side > effects like the fact that it blocks DROP ROLE so DROP OWNED BY has to > be used. > As for REASSIGN OWNED, it does not reassign anything (I don't think it's > a good idea to reassign default acls) it just spits warning with hint > what to do if user plans to drop the role. OK, so that addresses my last gripe. It seems that when you try to drop a role to which you have granted some privileges before, you can't, and when you REASSIGN OWNED, it doesn't help. So maybe it's not even necessary to give a warning when REASSIGN OWNED is called on default ACLs. Only loose end is tab completion which can probably be added later on. I'm also not sure if we wouldn't like to have ALTER DEFAULT PRIVILEGES FOR ALL ROLES (or something similar), so you won't have to ALTER DEFAULT PRIVILEGES for each developer you gave a DB login to. Petr told me that was the previous design but has been shot down - I found references to that on the mailing list, but most complains were about tying the syntax to ALTER SCHEMA. Since we now have ALTER DEFAULT PRIVILEGES I think it might make sense to introduce a way to set the default privileges for all roles (and give superusers the right to do it). This can be added later on, but maybe we could make the syntax work so when you do ALTER DEFAULT PRIVILEGES without FOR ROLE you set them for all roles in the current DB and if you want to set them for yourself, you need to specify FOR ROLE <yourrole>. That'd be a minor change in the patch. Setting to "Ready for Committer" (and leaving to the committer the decision whether to support "FOR ALL ROLES" and what to do about the warning). Jan
Petr Jelinek <pjmodos@pjmodos.net> writes:
> [ latest version of DefaultACLs patch ]
I started looking through this patch, but found that it's not nearly
ready to commit :-(. The big missing piece is that there's no pg_dump
support for default ACLs. That's a bigger chunk of code than I have
time/interest to write, and I don't think I want to commit the feature
without it. (I'm willing to commit without tab completion or any
psql \d command to show the defaults, but pg_dump just isn't optional.)
There is another large problem, too. The patch seems to have
only half-baked support for global defaults (those not tied to a
specific schema) --- it looks like you can put them in, but half
of the code will ignore them or else fail while trying to use them.
This isn't just a matter of a few missed cases while coding, I think.
The generic issue that the code doesn't even think about addressing
is which default should apply when there's potentially more than one
applicable default? As long as there's only global and per-schema
defaults, it's not too hard to decide that the latter take precedence
over the former; but I have no idea what we're going to do in order
to add any other features. This seems like a sufficiently big
conceptual issue that it had better be resolved now, even if the first
version of the patch doesn't really need to deal with it.
Also, the GRANT DEFAULT PRIVILEGES thing just seems completely bizarre,
and I'm not convinced it has a sufficient use-case to justify such a
strange wart on GRANT. I think we should drop it. Or at least it needs
to be proposed and discussed as a separate feature. Maybe it would seem
less strange if the syntax was "RESET PRIVILEGES ON object".
A couple of minor gripes:
Per recent discussion, names of system catalogs shouldn't be plural.
elog() is not suitable for user-facing errors. For example in
ExecGrantStmt_Defaults, the grammar does not prohibit the unsupported
target object types, so you need to throw a nicer error there. Or
else reject them in the grammar. There seem to be a number of other
places where elog is used but the error is perfectly likely to be caused
by a user mistake.
regards, tom lane
Tom Lane wrote: <blockquote cite="mid:11380.1254159141@sss.pgh.pa.us" type="cite"><pre wrap="">Petr Jelinek <a class="moz-txt-link-rfc2396E"href="mailto:pjmodos@pjmodos.net"><pjmodos@pjmodos.net></a> writes: </pre><blockquotetype="cite"><pre wrap="">[ latest version of DefaultACLs patch ] </pre></blockquote><pre wrap=""> I started looking through this patch, but found that it's not nearly ready to commit :-(. The big missing piece is that there's no pg_dump support for default ACLs. That's a bigger chunk of code than I have time/interest to write, and I don't think I want to commit the feature without it. (I'm willing to commit without tab completion or any psql \d command to show the defaults, but pg_dump just isn't optional.) </pre></blockquote><br /> Yeah I completely forgotabout pg_dump just like I did with anonymous code blocks :-(<br /><br /><blockquote cite="mid:11380.1254159141@sss.pgh.pa.us"type="cite"><pre wrap="">There is another large problem, too. The patch seems tohave only half-baked support for global defaults (those not tied to a specific schema) --- it looks like you can put them in, but half of the code will ignore them or else fail while trying to use them. This isn't just a matter of a few missed cases while coding, I think. The generic issue that the code doesn't even think about addressing is which default should apply when there's potentially more than one applicable default? As long as there's only global and per-schema defaults, it's not too hard to decide that the latter take precedence over the former; but I have no idea what we're going to do in order to add any other features. This seems like a sufficiently big conceptual issue that it had better be resolved now, even if the first version of the patch doesn't really need to deal with it. </pre></blockquote><br /> Half of the code will ignore them ? Theyare ignored if schema specific defaults were set.<br /> Yes I haven't tried to solve the problem of having non-hierarchicalfilters for defaults and if we require that then this patch is dead for (at least) this commitfest, becauseat the moment I don't even know where to begin solving this.<br /><br /><blockquote cite="mid:11380.1254159141@sss.pgh.pa.us"type="cite"><pre wrap="">Also, the GRANT DEFAULT PRIVILEGES thing just seems completelybizarre, and I'm not convinced it has a sufficient use-case to justify such a strange wart on GRANT. I think we should drop it. Or at least it needs to be proposed and discussed as a separate feature. Maybe it would seem less strange if the syntax was "RESET PRIVILEGES ON object". </pre></blockquote><br /> I vote for dropping it then.<br /><br/><pre class="moz-signature" cols="72">-- Regards Petr Jelinek (PJMODOS)</pre>
> There is another large problem, too. The patch seems to have > only half-baked support for global defaults (those not tied to a > specific schema) --- it looks like you can put them in, but half > of the code will ignore them or else fail while trying to use them. > This isn't just a matter of a few missed cases while coding, I think. > The generic issue that the code doesn't even think about addressing > is which default should apply when there's potentially more than one > applicable default? I thought the idea was to simply avoid that situation. Maybe we want to forget about global defaults if that's the case, and just do the ROLE defaults. I thought we were trying to keep this solution as simple as possible. It's meant to be a simple feature for simple use cases. I know we all love making stuff as ornate and complex as possible around here, but that kind of defeats the purpose of having DefaultACLs, as well as setting the bar unreasonably high for Petr. Asking him to future-filter-proof the feature assumes that there will be future filters, which I'm not convinced there will. I certainly haven't seen any good use case for having multiple conflicting defaults. In fact, I thought we'd agreed that any complex cases would be better handled by PL scripts. pg_dump support is required though. -- Josh Berkus PostgreSQL Experts Inc. www.pgexperts.com
Josh Berkus <josh@agliodbs.com> writes:
>> This isn't just a matter of a few missed cases while coding, I think.
>> The generic issue that the code doesn't even think about addressing
>> is which default should apply when there's potentially more than one
>> applicable default?
> I thought the idea was to simply avoid that situation. Maybe we want to
> forget about global defaults if that's the case, and just do the ROLE
> defaults.
That seems like a pretty dead-end design.
> I thought we were trying to keep this solution as simple as possible.
> It's meant to be a simple feature for simple use cases. I know we all
> love making stuff as ornate and complex as possible around here, but
> that kind of defeats the purpose of having DefaultACLs, as well as
> setting the bar unreasonably high for Petr. Asking him to
> future-filter-proof the feature assumes that there will be future
> filters, which I'm not convinced there will.
I already mentioned one case that there's longstanding demand for, which
is to instantiate the correct permissions on new partition child tables.
But more generally, this is a fairly large and complicated patch in
comparison to the reward, if the intention is that it will never support
anything more than the one case of "IN SCHEMA foo" filtering.
regards, tom lane
Tom, >> I thought the idea was to simply avoid that situation. Maybe we want to >> forget about global defaults if that's the case, and just do the ROLE >> defaults. > > That seems like a pretty dead-end design. Well, the whole purpose for DefaultACLs is to simplify administration for the simplest use cases. If we add a large host of conflicting options, we haven't simplified stuff very much. > I already mentioned one case that there's longstanding demand for, which > is to instantiate the correct permissions on new partition child tables. Wouldn't that be handled by inheritance? > But more generally, this is a fairly large and complicated patch in > comparison to the reward, if the intention is that it will never support > anything more than the one case of "IN SCHEMA foo" filtering. I thought we were doing ROLEs? -- Josh Berkus PostgreSQL Experts Inc. www.pgexperts.com
Josh Berkus <josh@agliodbs.com> writes:
>> But more generally, this is a fairly large and complicated patch in
>> comparison to the reward, if the intention is that it will never support
>> anything more than the one case of "IN SCHEMA foo" filtering.
> I thought we were doing ROLEs?
The owning-ROLE match is required, else you have issues with exactly
what the ACL really means. What we're discussing is what other filters
might exist to determine which objects are affected. The patch already
tries to handle the cases of "all owned objects" and "all owned objects
in schema X", and I think it's inevitable that people will want other
cases.
regards, tom lane
Tom, > The owning-ROLE match is required, else you have issues with exactly > what the ACL really means. What we're discussing is what other filters > might exist to determine which objects are affected. The patch already > tries to handle the cases of "all owned objects" and "all owned objects > in schema X", and I think it's inevitable that people will want other > cases. Yeah, I'm thinking we should back off from filters for 8.5; we could do them for 8.6, maybe. I'm one of the people who prefers a schema-based system, but I'll do without one if it means we can keep things *simple* (and get the feature in in 8.5). I think trying to make this patch a panacea in the first iteration is liable to backfire. Especially since we're doing GRANT ALL ON at the same time. -- Josh Berkus PostgreSQL Experts Inc. www.pgexperts.com
Josh Berkus <josh@agliodbs.com> writes:
> I think trying to make this patch a panacea in the first iteration is
> liable to backfire.
I did not suggest any such thing --- the current scope of functionality
is fine by me for a first cut. What I *am* saying is that we had better
have some idea of how we are going to extend it when (not if) we try
to extend it. Otherwise we are likely to find we painted ourselves
into a corner.
> Especially since we're doing GRANT ALL ON at the same time.
That's another patch that has an *excellent* chance of getting rejected
on pretty much the same grounds.
regards, tom lane
On 9/28/09, Josh Berkus <josh@agliodbs.com> wrote
I wish, but no:
http://www.postgresql.org/docs/current/interactive/ddl-inherit.html
The first paragraph under "5.8.1 Caveats":
Table access permissions are not automatically inherited. Therefore, a user attempting to access a parent table must either have permissions to do the same operation on all its child tables as well, or must use the ONLY notation. When adding a new child table to an existing inheritance hierarchy, be careful to grant all the needed permissions on it.
> I already mentioned one case that there's longstanding demand for, which
> is to instantiate the correct permissions on new partition child tables.
Wouldn't that be handled by inheritance?
I wish, but no:
http://www.postgresql.org/docs/current/interactive/ddl-inherit.html
The first paragraph under "5.8.1 Caveats":
Table access permissions are not automatically inherited. Therefore, a user attempting to access a parent table must either have permissions to do the same operation on all its child tables as well, or must use the ONLY notation. When adding a new child table to an existing inheritance hierarchy, be careful to grant all the needed permissions on it.
On Mon, Sep 28, 2009 at 1:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > The generic issue that the code doesn't even think about addressing > is which default should apply when there's potentially more than one > applicable default? As long as there's only global and per-schema > defaults, it's not too hard to decide that the latter take precedence > over the former; but I have no idea what we're going to do in order > to add any other features. This seems like a sufficiently big > conceptual issue that it had better be resolved now, even if the first > version of the patch doesn't really need to deal with it. I haven't read the patch, but it seems like one possible solution to this problem would be to declare that any any DEFAULT PRIVILEGES you set are cumulative. If you configure a global default, a per-schema default, a default for tables whose names begin with the letter q, and a default for tables created between midnight and 4am, then a table called quux created in that schema at 2:30 in the morning will get the union of all four sets of privileges. ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> I haven't read the patch, but it seems like one possible solution to
> this problem would be to declare that any any DEFAULT PRIVILEGES you
> set are cumulative. If you configure a global default, a per-schema
> default, a default for tables whose names begin with the letter q, and
> a default for tables created between midnight and 4am, then a table
> called quux created in that schema at 2:30 in the morning will get the
> union of all four sets of privileges.
Hmm ... interesting proposal. Simple to understand and simple to
implement, which are both to the good. I'm not clear though on whether
this behavior would be useful in practice. Any comments from those
who've been asking for default ACLs?
One potential trouble spot is that presumably the built-in default
privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate
with user-specified defaults. So you'd have a behavior where a
function would not get PUBLIC EXECUTE automatically if it matched
any of the available defaults, but would get it if it managed to
miss matching them all. I am not sure if that's bad or not, but
it seems kind of inconsistent.
regards, tom lane
On Mon, Sep 28, 2009 at 10:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I haven't read the patch, but it seems like one possible solution to >> this problem would be to declare that any any DEFAULT PRIVILEGES you >> set are cumulative. If you configure a global default, a per-schema >> default, a default for tables whose names begin with the letter q, and >> a default for tables created between midnight and 4am, then a table >> called quux created in that schema at 2:30 in the morning will get the >> union of all four sets of privileges. > > Hmm ... interesting proposal. Simple to understand and simple to > implement, which are both to the good. I'm not clear though on whether > this behavior would be useful in practice. Any comments from those > who've been asking for default ACLs? > > One potential trouble spot is that presumably the built-in default > privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate > with user-specified defaults. Why not? ...Robert
* Robert Haas (robertmhaas@gmail.com) wrote: > > One potential trouble spot is that presumably the built-in default > > privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate > > with user-specified defaults. > > Why not? How would you have a default that says "I *don't* want public execute on my new functions"? Stephen
Tom Lane napsal(a): <blockquote cite="mid:13288.1254166310@sss.pgh.pa.us" type="cite"><blockquote type="cite"><pre wrap="">Ithought we were trying to keep this solution as simple as possible. It's meant to be a simple feature for simple use cases. I know we all love making stuff as ornate and complex as possible around here, but that kind of defeats the purpose of having DefaultACLs, as well as setting the bar unreasonably high for Petr. Asking him to future-filter-proof the feature assumes that there will be future filters, which I'm not convinced there will. </pre></blockquote><pre wrap=""> I already mentioned one case that there's longstanding demand for, which is to instantiate the correct permissions on new partition child tables. </pre></blockquote><br /> Just FYI, this one isstill hierarchical (database->schema->parent table).<br /><br /><pre class="moz-signature" cols="72">-- Regards Petr Jelinek (PJMODOS)</pre>
Stephen Frost napsal(a): <blockquote cite="mid:20090929034709.GM17756@tamriel.snowman.net" type="cite"><pre wrap="">* RobertHaas (<a class="moz-txt-link-abbreviated" href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>) wrote: </pre><blockquotetype="cite"><blockquote type="cite"><pre wrap="">One potential trouble spot is that presumably the built-indefault privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate with user-specified defaults. </pre></blockquote><pre wrap="">Why not? </pre></blockquote><pre wrap=""> How would you have a default that says "I *don't* want public execute on my new functions"? </pre></blockquote><br /> This is actually problem that applies to whole Robert's proposal. How wouldyou define you don\t want insert on new tables in schema when you granted it for whole database. I don't think any kindof mixing of different default privileges is a good idea. I was thinking about rejecting creation of conflicting defaultprivileges but that would be impossible to detect before object creation which is too late.<br /><br /><pre class="moz-signature"cols="72">-- Regards Petr Jelinek (PJMODOS)</pre>
Tom Lane napsal(a): <blockquote cite="mid:16033.1254180225@sss.pgh.pa.us" type="cite"><blockquote type="cite"><pre wrap="">Especiallysince we're doing GRANT ALL ON at the same time. </pre></blockquote><pre wrap=""> That's another patch that has an *excellent* chance of getting rejected on pretty much the same grounds. </pre></blockquote><br /> I don't really see how this applies to GRANT ON ALL. You don'thave to predict future there so if you have conflict in multiple filters user specified you simply throw error.<br /><br/><pre class="moz-signature" cols="72">-- Regards Petr Jelinek (PJMODOS)</pre>
On Mon, Sep 28, 2009 at 11:47 PM, Stephen Frost <sfrost@snowman.net> wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: >> > One potential trouble spot is that presumably the built-in default >> > privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate >> > with user-specified defaults. >> >> Why not? > > How would you have a default that says "I *don't* want public execute on > my new functions"? Hmm... Maybe instead of having built-in default privileges, we could view each user as having their global default ACL pre-initialized to that same set of privileges (of course we needn't store it unless and until they modify it). Then they could add to those or take away from them, plus add additional privileges at other levels. ...Robert
Robert Haas napsal(a): <blockquote cite="mid:603c8f070909290640h4384e645r4a299dbd7f6dddbd@mail.gmail.com" type="cite"><prewrap="">On Mon, Sep 28, 2009 at 11:47 PM, Stephen Frost <a class="moz-txt-link-rfc2396E" href="mailto:sfrost@snowman.net"><sfrost@snowman.net></a>wrote: </pre><blockquote type="cite"><pre wrap="">* RobertHaas (<a class="moz-txt-link-abbreviated" href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>) wrote: </pre><blockquotetype="cite"><blockquote type="cite"><pre wrap="">One potential trouble spot is that presumably the built-indefault privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate with user-specified defaults. </pre></blockquote><pre wrap="">Why not? </pre></blockquote><pre wrap="">How wouldyou have a default that says "I *don't* want public execute on my new functions"? </pre></blockquote><pre wrap=""> Hmm... Maybe instead of having built-in default privileges, we could view each user as having their global default ACL pre-initialized to that same set of privileges (of course we needn't store it unless and until they modify it). Then they could add to those or take away from them, plus add additional privileges at other levels. </pre></blockquote><br /> That's how it works now actually, the problem isthat when you grant something in the chain you can't revoke it anywhere else in the chain when you are merging privilegesas you proposed.<br /><br /><pre class="moz-signature" cols="72">-- Regards Petr Jelinek (PJMODOS)</pre>
Petr Jelinek <pjmodos@pjmodos.net> writes:
> That's how it works now actually, the problem is that when you grant
> something in the chain you can't revoke it anywhere else in the chain
> when you are merging privileges as you proposed.
To allow that, you have to have some notion of a priority order among
the available defaults, so that you can sensibly say that A should
override B. Which is easy as long as they've got hierarchical scopes,
but that doesn't seem like a restriction that will hold good for future
extensions.
regards, tom lane
On Tue, Sep 29, 2009 at 11:05 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Petr Jelinek <pjmodos@pjmodos.net> writes: >> That's how it works now actually, the problem is that when you grant >> something in the chain you can't revoke it anywhere else in the chain >> when you are merging privileges as you proposed. > > To allow that, you have to have some notion of a priority order among > the available defaults, so that you can sensibly say that A should > override B. Which is easy as long as they've got hierarchical scopes, > but that doesn't seem like a restriction that will hold good for future > extensions. Yeah. Right now AIUI we have these: grant default privileges across the board grant default privileges to objects in schema X In the future maybe we might have: grant default privileges to all objects EXCEPT those in schemas A, B, C. I'm not real sure whether this sensible and I'm not real sure whether it's too baroque to be worthwhile, but I am fairly confident that it doesn't create any nasty semantic problems, which can't be said for any approach that involves permissions for schemas "overrriding" the global permissions, which will break down horribly as soon as we want to do anything orthogonal. ...Robert
Tom Lane napsal(a): <blockquote cite="mid:9440.1254236736@sss.pgh.pa.us" type="cite"><pre wrap="">Petr Jelinek <a class="moz-txt-link-rfc2396E"href="mailto:pjmodos@pjmodos.net"><pjmodos@pjmodos.net></a> writes: </pre><blockquotetype="cite"><pre wrap="">That's how it works now actually, the problem is that when you grant something in the chain you can't revoke it anywhere else in the chain when you are merging privileges as you proposed. </pre></blockquote><pre wrap=""> To allow that, you have to have some notion of a priority order among the available defaults, so that you can sensibly say that A should override B. Which is easy as long as they've got hierarchical scopes, but that doesn't seem like a restriction that will hold good for future extensions. </pre></blockquote><br /> I am aware, I knew all that has been said so far at the time I sent in the patch actually.That's why I am very skeptical about having those future non-hierarchical filters, I just don't see a way to makeit happen.<br /> Also when you go to some insane complexity of default privileges that don't respect your database structurethen you either want to handle it programatically as Josh said or you want to create new subroles what have createsomething privilege and different default privileges instead of hoping that the database will somehow magically dothe right thing about default acls conflicts.<br /><br /><pre class="moz-signature" cols="72">-- Regards Petr Jelinek (PJMODOS)</pre>
Tom, > Hmm ... interesting proposal. Simple to understand and simple to > implement, which are both to the good. I'm not clear though on whether > this behavior would be useful in practice. Any comments from those > who've been asking for default ACLs? I'd be fine with it. My goals here are to make my life easier by eliminating the need for complex GRANT scripts in the simplest cases, and (more importantly) to get web developers to use database object permissions *at all* by making them easy to manage. Robert's suggestion fulfills this. Again, for people who have complex or high security cases, you'd still need a script. That's fine; object permissions are an NP-complete problem anyway, and no feature we adopt is going to cover everyone. But right now we have an issue that probably 98% of Postgres users don't use object permissions *at all* because they're "too difficult to manage". If I had a dollar for every person I saw running Postgres in production as the "postgres" user, or with full public permissions on everything, I could shut down PGX and go back to pottery. > One potential trouble spot is that presumably the built-in default > privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate > with user-specified defaults. So you'd have a behavior where a > function would not get PUBLIC EXECUTE automatically if it matched > any of the available defaults, but would get it if it managed to > miss matching them all. I am not sure if that's bad or not, but > it seems kind of inconsistent. Yeah, but the fact that functions have PUBLIC EXECUTE by default is inconsistent. All DefaultACLs would be doing is inheriting this inconsistency. Again, I'm OK with that. -- Josh Berkus PostgreSQL Experts Inc. www.pgexperts.com
Hi all, because it seems like merging privileges seems to be acceptable for most (although I am not sure I like it, but I don't have better solution for managing conflicts), I changed the patch to do just that. Also I think I addressed all other problems pointed out by Tom. Namely I added pg_dump support (hopefully) and \ddp command for psql. Still no tab competition though. I removed that GRANT DEFAULT PRIVILEGES statement, changed user facing elog()s to ereport()s, and I changed catalog name to singular. -- Regards Petr Jelinek (PJMODOS)
Attachment
Petr Jelinek <pjmodos@pjmodos.net> writes:
> because it seems like merging privileges seems to be acceptable for most
> (although I am not sure I like it, but I don't have better solution for
> managing conflicts), I changed the patch to do just that.
It's not clear to me whether we have consensus on this approach.
Last chance for objections, anyone?
The main argument I can see against doing it this way is that it doesn't
provide a means for overriding the hard-wired public grants for object
types that have such (principally functions). I think that a reasonable
way to address that issue would be for a follow-on patch that allows
changing the hard-wired default privileges for object types. It might
well be that no one cares enough for it to matter, though. I think that
in most simple cases what's needed is a way to add privileges, not
subtract them --- and we're already agreed that this mechanism is only
meant to simplify simple cases.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Petr Jelinek <pjmodos@pjmodos.net> writes:
> > because it seems like merging privileges seems to be acceptable for most
> > (although I am not sure I like it, but I don't have better solution for
> > managing conflicts), I changed the patch to do just that.
>
> It's not clear to me whether we have consensus on this approach.
> Last chance for objections, anyone?
I don't like it, but at the same time I'd rather have it with this than
not have anything.
> The main argument I can see against doing it this way is that it doesn't
> provide a means for overriding the hard-wired public grants for object
> types that have such (principally functions). I think that a reasonable
> way to address that issue would be for a follow-on patch that allows
> changing the hard-wired default privileges for object types. It might
> well be that no one cares enough for it to matter, though. I think that
> in most simple cases what's needed is a way to add privileges, not
> subtract them --- and we're already agreed that this mechanism is only
> meant to simplify simple cases.
This doesn't actually address the entire problem. How about
schema-level default grants which you want to override with per-role
default grants? Or the other way around? Is it always only more
permissive with more defaults? Even when the grantee is the same?
I dunno, I'll probably just ignore the per-role stuff, personally, but
it seems more complex without sufficient definition about what's going
to happen in each case.
Thanks,
Stephen
Stephen Frost <sfrost@snowman.net> writes:
> This doesn't actually address the entire problem. How about
> schema-level default grants which you want to override with per-role
> default grants? Or the other way around? Is it always only more
> permissive with more defaults? Even when the grantee is the same?
Well, bear in mind that we're *only* going to allow these things
per-role, so as to avoid the problem of translating ACLs to a different
grantor. So the main case that's not being solved is "I'd like to
grant privileges XYZ everywhere except in this schema". I'm willing to
write that off as not being within the scope of a simple mechanism.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > This doesn't actually address the entire problem. How about
> > schema-level default grants which you want to override with per-role
> > default grants? Or the other way around? Is it always only more
> > permissive with more defaults? Even when the grantee is the same?
>
> Well, bear in mind that we're *only* going to allow these things
> per-role, so as to avoid the problem of translating ACLs to a different
> grantor. So the main case that's not being solved is "I'd like to
> grant privileges XYZ everywhere except in this schema". I'm willing to
> write that off as not being within the scope of a simple mechanism.
Erm, wait, we're going to drop the only piece of this that outside folks
have actually been asking for? Specifically, having per-schema default
ACLs? Big -1 for even bothering to add the complexity if we're not
going to address what end users are actually looking for. Perhaps I
missed where the issue with assigning the right grantor was, but that
feels very much like an implementation detail we can certainly solve and
document which way we decided to solve it (either schema owner, which
would be my preference, or object creator, which would be acceptable as
well).
That's my thoughts on it.
Thanks,
Stephen
Stephen Frost <sfrost@snowman.net> writes:
> Erm, wait, we're going to drop the only piece of this that outside folks
> have actually been asking for? Specifically, having per-schema default
> ACLs?
They are per-schema for the objects belonging to the granting user.
Otherwise you have a bunch of nasty issues, including the prospect
of non-superusers being able to control the privileges granted on
objects that don't belong to them.
regards, tom lane
Tom Lane wrote: <blockquote cite="mid:880.1254421285@sss.pgh.pa.us" type="cite"><pre wrap="">Stephen Frost <a class="moz-txt-link-rfc2396E"href="mailto:sfrost@snowman.net"><sfrost@snowman.net></a> writes: </pre><blockquote type="cite"><prewrap="">Erm, wait, we're going to drop the only piece of this that outside folks have actually been asking for? Specifically, having per-schema default ACLs? </pre></blockquote><pre wrap=""> They are per-schema for the objects belonging to the granting user. Otherwise you have a bunch of nasty issues, including the prospect of non-superusers being able to control the privileges granted on objects that don't belong to them. </pre></blockquote> Also it's not really a problem since superuser or role admin can setthese for other roles.<br /><br /><pre class="moz-signature" cols="72">-- Regards Petr Jelinek (PJMODOS)</pre>
On Thu, Oct 1, 2009 at 1:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Petr Jelinek <pjmodos@pjmodos.net> writes: >> because it seems like merging privileges seems to be acceptable for most >> (although I am not sure I like it, but I don't have better solution for >> managing conflicts), I changed the patch to do just that. > > It's not clear to me whether we have consensus on this approach. > Last chance for objections, anyone? > > The main argument I can see against doing it this way is that it doesn't > provide a means for overriding the hard-wired public grants for object > types that have such (principally functions). I think that a reasonable > way to address that issue would be for a follow-on patch that allows > changing the hard-wired default privileges for object types. It might > well be that no one cares enough for it to matter, though. I think that > in most simple cases what's needed is a way to add privileges, not > subtract them --- and we're already agreed that this mechanism is only > meant to simplify simple cases. I'm going to reiterate what I suggested upthread... let's let the default, global default ACL contain the hard-wired privileges, instead of making them hardwired. Then your objects will get those privileges not because they are hard-wired, but because you haven't changed your global default ACL to not contain them. ...Robert
Robert Haas napsal(a): <blockquote cite="mid:603c8f070910011820w5ed09055n399811239af4ba0c@mail.gmail.com" type="cite"><prewrap="">On Thu, Oct 1, 2009 at 1:37 PM, Tom Lane <a class="moz-txt-link-rfc2396E" href="mailto:tgl@sss.pgh.pa.us"><tgl@sss.pgh.pa.us></a>wrote: </pre><blockquote type="cite"><pre wrap="">Petr Jelinek<a class="moz-txt-link-rfc2396E" href="mailto:pjmodos@pjmodos.net"><pjmodos@pjmodos.net></a> writes: </pre><blockquotetype="cite"><pre wrap="">because it seems like merging privileges seems to be acceptable for most (although I am not sure I like it, but I don't have better solution for managing conflicts), I changed the patch to do just that. </pre></blockquote><pre wrap="">It's not clear to me whetherwe have consensus on this approach. Last chance for objections, anyone? The main argument I can see against doing it this way is that it doesn't provide a means for overriding the hard-wired public grants for object types that have such (principally functions). I think that a reasonable way to address that issue would be for a follow-on patch that allows changing the hard-wired default privileges for object types. It might well be that no one cares enough for it to matter, though. I think that in most simple cases what's needed is a way to add privileges, not subtract them --- and we're already agreed that this mechanism is only meant to simplify simple cases. </pre></blockquote><pre wrap=""> I'm going to reiterate what I suggested upthread... let's let the default, global default ACL contain the hard-wired privileges, instead of making them hardwired. Then your objects will get those privileges not because they are hard-wired, but because you haven't changed your global default ACL to not contain them. </pre></blockquote><br /> That's somewhat how I implemented it although not juston global level but in any single filter, what we now have as defaults (before this patch) is used as template for defaultacls and you can revoke it. You just can't revoke anything you granted anywhere in the default acls chain.<br /><br/><pre class="moz-signature" cols="72">-- Regards Petr Jelinek (PJMODOS)</pre>
Petr Jelinek napsal(a):
Reminds me I forgot to adjust the docs. Attached patch fixes that (no other changes).
Robert Haas napsal(a):I'm going to reiterate what I suggested upthread... let's let the default, global default ACL contain the hard-wired privileges, instead of making them hardwired. Then your objects will get those privileges not because they are hard-wired, but because you haven't changed your global default ACL to not contain them.
That's somewhat how I implemented it although not just on global level but in any single filter, what we now have as defaults (before this patch) is used as template for default acls and you can revoke it. You just can't revoke anything you granted anywhere in the default acls chain.
Reminds me I forgot to adjust the docs. Attached patch fixes that (no other changes).
-- Regards Petr Jelinek (PJMODOS)
Attachment
Robert Haas <robertmhaas@gmail.com> wrote:> let's let the default, global default ACL contain the hard-wired > privileges, instead of making them hardwired. +1 -Kevin
On 10/3/09 8:09 AM, Kevin Grittner wrote: > Robert Haas <robertmhaas@gmail.com> wrote: > > > let's let the default, global default ACL contain the hard-wired >> privileges, instead of making them hardwired. Wow, that would be great. It would meant that DBAs could change the global default permissions. --Josh
Petr Jelinek <pjmodos@pjmodos.net> writes:
> [ latest default-ACLs patch ]
Applied with a fair amount of editorial polishing. Notably I changed
the permissions requirements a bit:
* for IN SCHEMA, the *target* role has to have CREATE permission on the
target schema. Without this, the command is a bit pointless since the
permissions can never be used. The original coding checked whether the
*calling* role had USAGE, which seems rather irrelevant.
* I simplified the target-role permission test to is_member_of. The
original check for ADMIN seemed pointlessly strong, because if you're a
member of the role you can just become the role and set owned objects'
permissions however you like. I didn't see the point of the CREATEROLE
exemption either, and am generally suspicious of anything that would let
people change permissions on stuff they didn't own.
One thing that seems like it's likely to be an annoyance in practice
is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl
entries for a role to be dropped. But I can't see any very good way
around that, since the entries might be in some other database. One
thing that might at least reduce the number of keystrokes is to have
REASSIGN OWNED act as DROP OWNED BY for default ACLs. I can't convince
myself whether that's a good idea though, so I left it as-is for the
moment.
regards, tom lane
Josh Berkus <josh@agliodbs.com> writes:
> On 10/3/09 8:09 AM, Kevin Grittner wrote:
>> Robert Haas <robertmhaas@gmail.com> wrote:
> let's let the default, global default ACL contain the hard-wired
> privileges, instead of making them hardwired.
> Wow, that would be great. It would meant that DBAs could change the
> global default permissions.
Committed that way. One possible disadvantage down the road is that
people may now have the "default" privileges instantiated in their
databases, which will pose a hazard if we ever want to change the
default privilege sets. I imagine that we could have pg_upgrade go
through and modify the contents of pg_default_acl if we got in a bind
over this, but it's going to be something to think about.
regards, tom lane
2009/10/6 Tom Lane <tgl@sss.pgh.pa.us>: > Applied with a fair amount of editorial polishing. Notably I changed > the permissions requirements a bit: > Thanks and congratulations! I'm really looking forward to this feature. I pulled the latest sources and gave it a whirl. Things worked as expected in psql, but I was a little surprised when I headed into the documentation. The first place I visited was Chapter 20 - Database Roles and Privileges, but there was no mention of the default ACLs feature in there. If you head into the SQL Reference, it's all there under ALTER DEFAULT PRIVILEGES, but that's only helpful if you already know that it's there and what the command is called. At the moment the only way you'd find out about Default ACLs via the documentation is if you noticed the link several paragraphs into the reference for GRANT. Perhaps we should have something in Chapter 20 along the lines of "Rather than tediously assigning privileges to objects every time you create them, you can set default privileges on a schema" etc. IMO we should be actively pointing people, especially Postgres/DBA newbies, to this very useful functionality. Cheers, BJ
Tom Lane napsal(a): <blockquote cite="mid:18007.1254771245@sss.pgh.pa.us" type="cite"><pre wrap="">Petr Jelinek <a class="moz-txt-link-rfc2396E"href="mailto:pjmodos@pjmodos.net"><pjmodos@pjmodos.net></a> writes: </pre><blockquotetype="cite"><pre wrap="">[ latest default-ACLs patch ] </pre></blockquote><pre wrap=""> Applied with a fair amount of editorial polishing. Notably I changed the permissions requirements a bit: </pre></blockquote><br /> Thank you very much Tom.<br /><br /><blockquote cite="mid:18007.1254771245@sss.pgh.pa.us"type="cite"><pre wrap="">One thing that seems like it's likely to be an annoyancein practice is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl entries for a role to be dropped. But I can't see any very good way around that, since the entries might be in some other database. One thing that might at least reduce the number of keystrokes is to have REASSIGN OWNED act as DROP OWNED BY for default ACLs. I can't convince myself whether that's a good idea though, so I left it as-is for the moment. </pre></blockquote><br /> Yeah I am not happy about this either but there is not much we can do about it. Btw I thinkin the version I sent in REASSIGN OWNED acted as DROP OWNED for default ACLs.<br /><br /><pre class="moz-signature"cols="72">-- Regards Petr Jelinek (PJMODOS)</pre>
Petr Jelinek <pjmodos@pjmodos.net> writes:
> Tom Lane napsal(a):
>> One thing that seems like it's likely to be an annoyance in practice
>> is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl
>> entries for a role to be dropped.
> Yeah I am not happy about this either but there is not much we can do
> about it. Btw I think in the version I sent in REASSIGN OWNED acted as
> DROP OWNED for default ACLs.
IIRC it just threw a warning, which didn't seem tremendously useful to
me.
regards, tom lane
Brendan Jurd <direvus@gmail.com> writes:
> I pulled the latest sources and gave it a whirl. Things worked as
> expected in psql, but I was a little surprised when I headed into the
> documentation. The first place I visited was Chapter 20 - Database
> Roles and Privileges, but there was no mention of the default ACLs
> feature in there.
I looked at that (as well as the material in section 5.6) and concluded
that it was a once-over-lightly presentation that didn't really need to
delve into this. But if you want to work it over, feel free to send in
a docs patch. Personally I'd like to see less duplication between 5.6
and 20.3, but I'm not quite sure how to refactor it --- the material is
arguably somewhat relevant in both places.
regards, tom lane
Tom Lane napsal(a): <blockquote cite="mid:5876.1254782904@sss.pgh.pa.us" type="cite"><pre wrap="">Petr Jelinek <a class="moz-txt-link-rfc2396E"href="mailto:pjmodos@pjmodos.net"><pjmodos@pjmodos.net></a> writes: </pre><blockquotetype="cite"><pre wrap="">Tom Lane napsal(a): </pre><blockquote type="cite"><pre wrap="">One thing thatseems like it's likely to be an annoyance in practice is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl entries for a role to be dropped. </pre></blockquote></blockquote><blockquote type="cite"><pre wrap="">Yeah I am nothappy about this either but there is not much we can do about it. Btw I think in the version I sent in REASSIGN OWNED acted as DROP OWNED for default ACLs. </pre></blockquote><pre wrap=""> IIRC it just threw a warning, which didn't seem tremendously useful to me. </pre></blockquote><br /> Oh did it ? Then I must have discarded that idea for some reason. I probably didn't want tobe too pushy there.<br /><br /><pre class="moz-signature" cols="72">-- Regards Petr Jelinek (PJMODOS)</pre>
2009/10/6 Tom Lane <tgl@sss.pgh.pa.us>: > Brendan Jurd <direvus@gmail.com> writes: >> I pulled the latest sources and gave it a whirl. Things worked as >> expected in psql, but I was a little surprised when I headed into the >> documentation. The first place I visited was Chapter 20 - Database >> Roles and Privileges, but there was no mention of the default ACLs >> feature in there. > > I looked at that (as well as the material in section 5.6) and concluded > that it was a once-over-lightly presentation that didn't really need to > delve into this. Well 5.6 goes as far as to give mention about WITH GRANT OPTION, so I don't think we'd be going too deep to give a pointer regarding default ACLs as well. I don't think we need a fully detailed explanation of default ACLs here, just a brief mention and a pointer to the reference page. > But if you want to work it over, feel free to send in > a docs patch. I'll have a play around with it and see if I can come up with wording that I like. > Personally I'd like to see less duplication between 5.6 > and 20.3, but I'm not quite sure how to refactor it --- the material is > arguably somewhat relevant in both places. > I hear you about the duplication. 20.3 is so brief that I'm quite tempted to just remove it and instead have a paragraph referencing GRANT and REVOKE somewhere in 20.1. Cheers, BJ
I tried to check the default ACL behavior.
postgres=# \c - ymj psql (8.5devel) You are now connected to database "postgres" as user "ymj". postgres=> ALTER
DEFAULTPRIVILEGES REVOKE INSERT ON TABLE FROM ymj; ALTER DEFAULT PRIVILEGES postgres=> CREATE TABLE t2 (x int, y text);
CREATETABLE postgres=> SELECT relname, relacl FROM pg_class WHERE oid = 't2'::regclass; relname | relacl
---------+------------------ t2 | {ymj=rwdDxt/ymj} (1 row)
postgres=> INSERT INTO t2 VALUES (1, 'aaa'); ERROR: permission denied for relation t2
It works for me fine, good, but ...
postgres=> SELECT * INTO t3 FROM t1; SELECT postgres=> SELECT * FROM t3; a | b ---+----- 1 | aaa 2 | bbb (2 rows)
postgres=> INSERT INTO t3 VALUES (3,'ccc'); ERROR: permission denied for relation t3
In this case, the new table t3 is created with the default ACL which does not
allow to insert any values by the owner of the relation.
SELECT INTO does not check ACL_INSERT on the newly created tables, because
we had been able to assume the table owner always has privilege to insert
values into the new table.
So, OpenIntoRel() didn't check this obvious privilege.
But the default ACL feature breaks this assumption. The table owner may not
have privilege to insert values into new tables.
So, it is necessary to put actual access controls on the OpenIntoRel().
Thanks,
Tom Lane wrote:
> Petr Jelinek <pjmodos@pjmodos.net> writes:
>> [ latest default-ACLs patch ]
>
> Applied with a fair amount of editorial polishing. Notably I changed
> the permissions requirements a bit:
>
> * for IN SCHEMA, the *target* role has to have CREATE permission on the
> target schema. Without this, the command is a bit pointless since the
> permissions can never be used. The original coding checked whether the
> *calling* role had USAGE, which seems rather irrelevant.
>
> * I simplified the target-role permission test to is_member_of. The
> original check for ADMIN seemed pointlessly strong, because if you're a
> member of the role you can just become the role and set owned objects'
> permissions however you like. I didn't see the point of the CREATEROLE
> exemption either, and am generally suspicious of anything that would let
> people change permissions on stuff they didn't own.
>
> One thing that seems like it's likely to be an annoyance in practice
> is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl
> entries for a role to be dropped. But I can't see any very good way
> around that, since the entries might be in some other database. One
> thing that might at least reduce the number of keystrokes is to have
> REASSIGN OWNED act as DROP OWNED BY for default ACLs. I can't convince
> myself whether that's a good idea though, so I left it as-is for the
> moment.
>
> regards, tom lane
>
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei napsal(a): > I tried to check the default ACL behavior. > > It works for me fine, good, but ... > > postgres=> SELECT * INTO t3 FROM t1; > SELECT > postgres=> SELECT * FROM t3; > a | b > ---+----- > 1 | aaa > 2 | bbb > (2 rows) > > postgres=> INSERT INTO t3 VALUES (3,'ccc'); > ERROR: permission denied for relation t3 > > In this case, the new table t3 is created with the default ACL which does not > allow to insert any values by the owner of the relation. > > SELECT INTO does not check ACL_INSERT on the newly created tables, because > we had been able to assume the table owner always has privilege to insert > values into the new table. > So, OpenIntoRel() didn't check this obvious privilege. > > But the default ACL feature breaks this assumption. The table owner may not > have privilege to insert values into new tables. > So, it is necessary to put actual access controls on the OpenIntoRel(). > That's strange behavior I agree. However I don't see how default ACLs changed it in any way, owner could REVOKE his privileges before. -- Regards Petr Jelinek (PJMODOS)
Petr Jelinek napsal(a): <blockquote cite="mid:4ACA7962.7090804@pjmodos.net" type="cite"> Tom Lane napsal(a): <blockquotecite="mid:5876.1254782904@sss.pgh.pa.us" type="cite"><pre>Petr Jelinek <a href="mailto:pjmodos@pjmodos.net" moz-do-not-send="true"><pjmodos@pjmodos.net></a>writes: </pre><blockquote type="cite"><pre>Tom Lane napsal(a): </pre><blockquotetype="cite"><pre>One thing that seems like it's likely to be an annoyance in practice is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl entries for a role to be dropped. </pre></blockquote></blockquote><blockquote type="cite"><pre>Yeah I am not happy aboutthis either but there is not much we can do about it. Btw I think in the version I sent in REASSIGN OWNED acted as DROP OWNED for default ACLs. </pre></blockquote><pre>IIRC it just threw a warning, which didn't seem tremendously usefulto me. </pre></blockquote><br /> Oh did it ? Then I must have discarded that idea for some reason. I probably didn't want tobe too pushy there.<br /><br /></blockquote><br /> Now I remember why - consistency with ACLs on object. REASSIGN OWNEDdoes not drop any GRANTed ACLs on any object, so it seemed appropriate to only drop default ACLs in DROP OWNED BY alongwith ACLs on objects.<br /><br /><pre class="moz-signature" cols="72">-- Regards Petr Jelinek (PJMODOS)</pre>
Petr Jelinek wrote: > KaiGai Kohei napsal(a): >> I tried to check the default ACL behavior. >> >> It works for me fine, good, but ... >> >> postgres=> SELECT * INTO t3 FROM t1; >> SELECT >> postgres=> SELECT * FROM t3; >> a | b >> ---+----- >> 1 | aaa >> 2 | bbb >> (2 rows) >> >> postgres=> INSERT INTO t3 VALUES (3,'ccc'); >> ERROR: permission denied for relation t3 >> >> In this case, the new table t3 is created with the default ACL which does not >> allow to insert any values by the owner of the relation. >> >> SELECT INTO does not check ACL_INSERT on the newly created tables, because >> we had been able to assume the table owner always has privilege to insert >> values into the new table. >> So, OpenIntoRel() didn't check this obvious privilege. >> >> But the default ACL feature breaks this assumption. The table owner may not >> have privilege to insert values into new tables. >> So, it is necessary to put actual access controls on the OpenIntoRel(). >> > > That's strange behavior I agree. However I don't see how default ACLs > changed it in any way, owner could REVOKE his privileges before. > I don't think the default ACL feature should do something ad-hoc here. Is there anything necessary more than adding permission checks to insert values into the new table? Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Petr Jelinek escribió: > Petr Jelinek napsal(a): > >Tom Lane napsal(a): > >>Petr Jelinek <pjmodos@pjmodos.net> <mailto:pjmodos@pjmodos.net> writes: > >>>Tom Lane napsal(a): > >>>>One thing that seems like it's likely to be an annoyance in practice > >>>>is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl > >>>>entries for a role to be dropped. > >>>Yeah I am not happy about this either but there is not much we > >>>can do about it. Btw I think in the version I sent in REASSIGN > >>>OWNED acted as DROP OWNED for default ACLs. > >>IIRC it just threw a warning, which didn't seem tremendously useful to > >>me. > > > >Oh did it ? Then I must have discarded that idea for some reason. > >I probably didn't want to be too pushy there. > > Now I remember why - consistency with ACLs on object. REASSIGN OWNED > does not drop any GRANTed ACLs on any object, so it seemed > appropriate to only drop default ACLs in DROP OWNED BY along with > ACLs on objects. That seems reasonable to me too ... -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Petr Jelinek <pjmodos@pjmodos.net> writes:
> KaiGai Kohei napsal(a):
>> SELECT INTO does not check ACL_INSERT on the newly created tables, because
>> we had been able to assume the table owner always has privilege to insert
>> values into the new table.
>> So, OpenIntoRel() didn't check this obvious privilege.
>>
>> But the default ACL feature breaks this assumption. The table owner may not
>> have privilege to insert values into new tables.
>> So, it is necessary to put actual access controls on the OpenIntoRel().
> That's strange behavior I agree. However I don't see how default ACLs
> changed it in any way, owner could REVOKE his privileges before.
The point is that some rows got into the new table, which should have
been disallowed if CREATE TABLE AS is considered to represent a CREATE
followed by an INSERT. However, I disagree that this necessarily
represents a bug in the permissions checks. We could perfectly well
define that INSERT means the right to insert into the table *after it is
created*, and that CREATE TABLE AS does not involve this right. I think
that that is a reasonable and potentially useful thing to do, whereas
defining it as KaiGai-san suggests would have no usefulness whatsoever.
What's more, CREATE TABLE AS is specified in the SQL standard, and I see
nothing in the standard mentioning that it requires INSERT privilege.
regards, tom lane
Tom Lane wrote: > Petr Jelinek <pjmodos@pjmodos.net> writes: >> KaiGai Kohei napsal(a): >>> SELECT INTO does not check ACL_INSERT on the newly created tables, because >>> we had been able to assume the table owner always has privilege to insert >>> values into the new table. >>> So, OpenIntoRel() didn't check this obvious privilege. >>> >>> But the default ACL feature breaks this assumption. The table owner may not >>> have privilege to insert values into new tables. >>> So, it is necessary to put actual access controls on the OpenIntoRel(). > >> That's strange behavior I agree. However I don't see how default ACLs >> changed it in any way, owner could REVOKE his privileges before. > > The point is that some rows got into the new table, which should have > been disallowed if CREATE TABLE AS is considered to represent a CREATE > followed by an INSERT. However, I disagree that this necessarily > represents a bug in the permissions checks. We could perfectly well > define that INSERT means the right to insert into the table *after it is > created*, and that CREATE TABLE AS does not involve this right. I think > that that is a reasonable and potentially useful thing to do, whereas > defining it as KaiGai-san suggests would have no usefulness whatsoever. > What's more, CREATE TABLE AS is specified in the SQL standard, and I see > nothing in the standard mentioning that it requires INSERT privilege. It is also an issue due to the differences in perspectives and security models. My major concern is come from the viewpoint of MAC which tries to control data flows based on sensitivity levels. So, if the default PG model considers that "CREATE TABLE AS" is an atomic operation, not a pair of CREATE and INSERTs, I think it is an approach to understand this behavior. In my preference, this perspective should be documented somewhere. (source code comments or official documentation?) BTW, in the MAC model, we must strictly prevent a user with classified security level to write any data into objects with unclassified security level. It is called "write-down", being equivalent to information leaks. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>