Re: [HACKERS] [PATCH] DefaultACLs
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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek napsal(a): Tom Lane napsal(a): Petr Jelinek pjmo...@pjmodos.net mailto:pjmo...@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. -- Regards Petr Jelinek (PJMODOS)
Re: [HACKERS] [PATCH] DefaultACLs
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 kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek escribió: Petr Jelinek napsal(a): Tom Lane napsal(a): Petr Jelinek pjmo...@pjmodos.net mailto:pjmo...@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 Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek pjmo...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Tom Lane wrote: Petr Jelinek pjmo...@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 kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek pjmo...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Josh Berkus j...@agliodbs.com writes: On 10/3/09 8:09 AM, Kevin Grittner wrote: Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
2009/10/6 Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Tom Lane napsal(a): Petr Jelinek pjmo...@pjmodos.net writes: [ latest default-ACLs patch ] Applied with a fair amount of editorial polishing. Notably I changed the permissions requirements a bit: Thank you very much Tom. 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. 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. -- Regards Petr Jelinek (PJMODOS)
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek pjmo...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Brendan Jurd dire...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Tom Lane napsal(a): Petr Jelinek pjmo...@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. -- Regards Petr Jelinek (PJMODOS)
Re: [HACKERS] [PATCH] DefaultACLs
2009/10/6 Tom Lane t...@sss.pgh.pa.us: Brendan Jurd dire...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 DEFAULT PRIVILEGES REVOKE INSERT ON TABLE FROM ymj;
ALTER DEFAULT PRIVILEGES
postgres= CREATE TABLE t2 (x int, y text);
CREATE TABLE
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 pjmo...@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 kai...@ak.jp.nec.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
On 10/3/09 8:09 AM, Kevin Grittner wrote: Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Robert Haas robertmh...@gmail.com wrote: let's let the default, global default ACL contain the hard-wired privileges, instead of making them hardwired. +1 -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Robert Haas napsal(a): On Thu, Oct 1, 2009 at 1:37 PM, Tom Lane t...@sss.pgh.pa.us wrote: Petr Jelinek pjmo...@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. 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. -- Regards Petr Jelinek (PJMODOS)
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek napsal(a): 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) defacl-2009-10-02.diff.gz Description: Unix tar archive -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek pjmo...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
* Tom Lane (t...@sss.pgh.pa.us) wrote: Petr Jelinek pjmo...@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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
Stephen Frost sfr...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
Stephen Frost sfr...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Tom Lane wrote: Stephen Frost sfr...@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. Also it's not really a problem since superuser or role admin can set these for other roles. -- Regards Petr Jelinek (PJMODOS)
Re: [HACKERS] [PATCH] DefaultACLs
On Thu, Oct 1, 2009 at 1:37 PM, Tom Lane t...@sss.pgh.pa.us wrote: Petr Jelinek pjmo...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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) defacl-2009-09-30.diff.gz Description: Unix tar archive -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Tom Lane napsal(a): 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. Just FYI, this one is still hierarchical (database-schema-parent table). -- Regards Petr Jelinek (PJMODOS)
Re: [HACKERS] [PATCH] DefaultACLs
Stephen Frost napsal(a): * Robert Haas (robertmh...@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? This is actually problem that applies to whole Robert's proposal. How would you define you don\t want insert on new tables in schema when you granted it for whole database. I don't think any kind of mixing of different default privileges is a good idea. I was thinking about rejecting creation of conflicting default privileges but that would be impossible to detect before object creation which is too late. -- Regards Petr Jelinek (PJMODOS)
Re: [HACKERS] [PATCH] DefaultACLs
Tom Lane napsal(a): 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. I don't really see how this applies to GRANT ON ALL. You don't have to predict future there so if you have conflict in multiple filters user specified you simply throw error. -- Regards Petr Jelinek (PJMODOS)
Re: [HACKERS] [PATCH] DefaultACLs
On Mon, Sep 28, 2009 at 11:47 PM, Stephen Frost sfr...@snowman.net wrote: * Robert Haas (robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Robert Haas napsal(a): On Mon, Sep 28, 2009 at 11:47 PM, Stephen Frost sfr...@snowman.net wrote: * Robert Haas (robertmh...@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. 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. -- Regards Petr Jelinek (PJMODOS)
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek pjmo...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
On Tue, Sep 29, 2009 at 11:05 AM, Tom Lane t...@sss.pgh.pa.us wrote: Petr Jelinek pjmo...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Tom Lane napsal(a): Petr Jelinek pjmo...@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. 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 make it happen. Also when you go to some insane complexity of default privileges that don't respect your database structure then you either want to handle it programatically as Josh said or you want to create new subroles what have create something privilege and different default privileges instead of hoping that the database will somehow magically do the right thing about default acls conflicts. -- Regards Petr Jelinek (PJMODOS)
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek pjmo...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Tom Lane wrote: Petr Jelinek pjmo...@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.) Yeah I completely forgot about pg_dump just like I did with anonymous code blocks :-( 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. Half of the code will ignore them ? They are ignored if schema specific defaults were set. Yes I haven't tried to solve the problem of having non-hierarchical filters for defaults and if we require that then this patch is dead for (at least) this commitfest, because at the moment I don't even know where to begin solving this. 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. I vote for dropping it then. -- Regards Petr Jelinek (PJMODOS)
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Josh Berkus j...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Josh Berkus j...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Josh Berkus j...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
On 9/28/09, Josh Berkus j...@agliodbs.com wrote 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 ONLYnotation. When adding a new child table to an existing inheritance hierarchy, be careful to grant all the needed permissions on it.
Re: [HACKERS] [PATCH] DefaultACLs
On Mon, Sep 28, 2009 at 1:32 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
On Mon, Sep 28, 2009 at 10:44 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
* Robert Haas (robertmh...@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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Jan Urbański napsal(a): 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) defacl-2009-09-24.diff.gz Description: Unix tar archive -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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) defacl-2009-09-24-2.diff.gz Description: Unix tar archive -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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) defacl-2009-09-23.diff.gz Description: Unix tar archive -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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
Re: [HACKERS] [PATCH] DefaultACLs
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 or redundant 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}
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Jan Urbański napsal(a): 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. Hmm looks like I forgot to git add that file in the new branch, sorry about that. Attached patch should have the missing file (otherwise it's same). -- Regards Petr Jelinek (PJMODOS) defacl-2009-09-18.diff.gz Description: Unix tar archive -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Josh Berkus wrote: 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. I agree, however I assume I understood Tom properly since he didn't reply. 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) defacl-2009-09-14.diff.gz Description: Unix tar archive -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek pjmo...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
* Stephen Frost (sfr...@snowman.net) wrote: * Robert Haas (robertmh...@gmail.com) wrote: On Wed, Jul 22, 2009 at 11:26 PM, Petr Jelinekpjmo...@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 literalWITH GRANT OPTION/literal parameter is not applicable + because it is copied from default privileges. + Note: this can actually emphasis role=boldrevoke/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 literalWITH GRANT OPTION/literal parameter is not applicable because it is copied from default privileges. Note: This can emphasis role=boldrevoke/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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
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 literalWITH GRANT OPTION/literal parameter is not applicable because it is copied from default privileges. Note: This can emphasis role=boldrevoke/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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
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 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). 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 Tolleyeggyk...@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) defaultacls-2009-07-26.diff.gz Description: Unix tar archive -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
On Sat, Jul 25, 2009 at 7:45 PM, Joshua Tolleyeggyk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
On Sat, Jul 25, 2009 at 8:39 PM, Joshua Tolleyeggyk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
On Sat, Jul 25, 2009 at 08:41:12PM -0400, Robert Haas wrote:
On Sat, Jul 25, 2009 at 8:39 PM, Joshua Tolleyeggyk...@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
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 34679d8..3eb92a4 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -3130,6 +3130,70 @@
/sect1
+ sect1 id=catalog-pg-namespace-default-acl
+ titlestructnamepg_namespace_default_acl/structname/title
+
+ indexterm zone=catalog-pg-namespace-default-acl
+ primarypg_namespace_default_acl/primary
+ /indexterm
+
+ para
+ The catalog structnamepg_namespace_default_acl/ stores default
+ privileges for newly created objects inside the schema.
+ /para
+
+ table
+ titlestructnamepg_namespace/ Columns/title
+
+ tgroup cols=4
+thead
+ row
+ entryName/entry
+ entryType/entry
+ entryReferences/entry
+ entryDescription/entry
+ /row
+/thead
+
+tbody
+ row
+ entrystructfielddefaclnamespace/structfield/entry
+ entrytypeoid/type/entry
+ entryliterallink linkend=catalog-pg-namespacestructnamepg_namespace/structname/link.oid/literal/entry
+ entryThe OID of the namespace associated with this entry/entry
+ /row
+
+ row
+ entrystructfielddefaclgrantobjtype/structfield/entry
+ entrytypechar/type/entry
+ entry/entry
+ entry
+ literalr/ = table, literalv/ = view,
+ literalf/ = function, literalS/ = sequence
+ /entry
+ /row
+
+ row
+ entrystructfielddefacllist/structfield/entry
+ entrytypeaclitem[]/type/entry
+ entry/entry
+ entry
+ Access privileges that the object should have on creation.
+ This is NOT a mask, it's exactly what the object will get.
+ See
+ xref linkend=sql-alterschema endterm=sql-alterschema-title,
+ xref linkend=sql-grant endterm=sql-grant-title and
+ xref linkend=sql-revoke endterm=sql-revoke-title
+ for details.
+ /entry
+ /row
+/tbody
+ /tgroup
+ /table
+
+ /sect1
+
+
sect1 id=catalog-pg-opclass
titlestructnamepg_opclass/structname/title
diff --git a/doc/src/sgml/ref/alter_schema.sgml b/doc/src/sgml/ref/alter_schema.sgml
index 2458d19..62f4c2a 100644
--- a/doc/src/sgml/ref/alter_schema.sgml
+++ b/doc/src/sgml/ref/alter_schema.sgml
@@ -23,18 +23,46 @@ PostgreSQL documentation
synopsis
ALTER SCHEMA replaceablename/replaceable RENAME TO replaceablenewname/replaceable
ALTER SCHEMA replaceablename/replaceable OWNER TO replaceablenewowner/replaceable
+
+ALTER SCHEMA replaceablename/replaceable { SET | ADD } DEFAULT PRIVILEGES { { ON default_privileges
+ TO { [ GROUP ] replaceable class=PARAMETERrolename/replaceable | PUBLIC } [, ...] [ WITH GRANT OPTION ] } [AND ...] } [...]
+
+where replaceable class=PARAMETERdefault_privileges/replaceable is:
+
+{ { TABLE | VIEW } { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
+[,...] | ALL [ PRIVILEGES ] } |
+ SEQUENCE { { USAGE | SELECT | UPDATE }
+[,...] | ALL [ PRIVILEGES ] } |
+ FUNCTION { EXECUTE | ALL [ PRIVILEGES ] } }
+
+ALTER SCHEMA replaceablename/replaceable DROP DEFAULT PRIVILEGES { { ON drop_default_privileges
+ FROM { [ GROUP ] replaceable class=PARAMETERrolename/replaceable | PUBLIC } [, ...] } [AND ...] } [...]
+
+where replaceable class=PARAMETERdrop_default_privileges/replaceable is:
+
+{ { TABLE | VIEW } [ GRANT OPTION FOR ]
+ { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
+[,...] | ALL [ PRIVILEGES ] } |
+ SEQUENCE [ GRANT OPTION FOR ]
+ { { USAGE | SELECT | UPDATE } [,...] | ALL [ PRIVILEGES ] } |
+ FUNCTION [ GRANT OPTION FOR ]
+ { EXECUTE | ALL [ PRIVILEGES ] } }
/synopsis
/refsynopsisdiv
- refsect1
+ refsect1 id=sql-alterschema-description
titleDescription/title
para
- commandALTER SCHEMA/command changes the definition of a schema.
+ You must own the schema to use commandALTER SCHEMA/.
/para
+ /refsect1
+
+ refsect1
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Peter Eisentraut wrote: 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. 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). 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)
Re: [HACKERS] [PATCH] DefaultACLs
On Fri, Jul 17, 2009 at 9:46 PM, Joshua Tolleyeggyk...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
On Wed, Jul 22, 2009 at 11:21 PM, Joshua Tolleyeggyk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
On Wed, Jul 22, 2009 at 11:26 PM, Petr Jelinekpjmo...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
* Robert Haas (robertmh...@gmail.com) wrote: On Wed, Jul 22, 2009 at 11:26 PM, Petr Jelinekpjmo...@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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
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) defaultacls-2009-07-19.diff.gz Description: Unix tar archive -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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) defaultacls.diff.gz Description: Unix tar archive -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Nikhil, * Nikhil Sontakke (nikhil.sonta...@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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Hey, * Nikhil Sontakke (nikhil.sonta...@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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
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.
--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com
signature.asc
Description: Digital signature
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
