Re: Column level privileges was:(Re: [HACKERS] Extending grant insert on tables to sequences)
Jaime, * Stephen Frost ([EMAIL PROTECTED]) wrote: * Jaime Casanova ([EMAIL PROTECTED]) wrote: On 7/25/08, Stephen Frost [EMAIL PROTECTED] wrote: Yes, I'm working on it hi, any work on it? may i help? If you look at the commitfest, I've posted my WIP so far there. Most of the grammer, parser, and catalog changes are there. There's a couple of bugs in that code that I'm working to run down but otherwise I think it's pretty good. I do need to add in the dependency tracking as well though, and that's what I'm planning to work on next. I've now added dependency tracking and worked out a few kinks in the code, both existing previously and from adding the dep tracking. I'd really like to simplify things in aclchk.c, perhaps by factoring out more common bits into functional pieces, but it's been kind of a bear so far. The dependency tracking is being done by continuing to treat the table as a single entity and just figuring out the total set (including all column-level permissions) of roles for the entire table, rather than introducing the sub-object concept. This requires a bit of extra effort when doing DDLs and GRANTs but simplifies the dependency tracking itself, especially since we have to keep track of both table-level permissions and column-level permissions seperately. I'm open to other suggestions/comments. If people feel the sub-object is a better approach, it would get somewhat more awkward because we'd have to handle the relation-level dependencies as well as the column-level ones. Not impossible to do, of course, but a bit more complicated than how it was done originally. A piece which can be broken off pretty easily is adding support to track the columns used through to the executor so we can check the permissions in the right place. Jamie, have you had a chance to work on this? It's next on my list and I'll start working on it tonight unless you've had a chance to get to it. Please let me know. Thanks, Stephen signature.asc Description: Digital signature
Re: Column level privileges was:(Re: [HACKERS] Extending grant insert on tables to sequences)
On 9/17/08, Stephen Frost [EMAIL PROTECTED] wrote: A piece which can be broken off pretty easily is adding support to track the columns used through to the executor so we can check the permissions in the right place. Jamie, have you had a chance to work on this? It's next on my list and I'll start working on it tonight unless you've had a chance to get to it. Please let me know. not really, i start to read the code... but was interrupted for a new task... (if we only could send kill -9 signals to work tasks ;) -- regards, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- 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] Extending grant insert on tables to sequences
On Wed, Sep 3, 2008 at 7:03 PM, Tom Lane [EMAIL PROTECTED] wrote: In short, this patch isn't much more ready to commit than it was in the last fest. Just for the record, i put this updated patch just because there were an entry for Extending grant insert on tables to sequences for this Commit Fest without being an updated patch -- regards, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. (593) 87171157 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Column level privileges was:(Re: [HACKERS] Extending grant insert on tables to sequences)
Jaime, * Jaime Casanova ([EMAIL PROTECTED]) wrote: On 7/25/08, Stephen Frost [EMAIL PROTECTED] wrote: Yes, I'm working on it hi, any work on it? may i help? If you look at the commitfest, I've posted my WIP so far there. Most of the grammer, parser, and catalog changes are there. There's a couple of bugs in that code that I'm working to run down but otherwise I think it's pretty good. I do need to add in the dependency tracking as well though, and that's what I'm planning to work on next. A piece which can be broken off pretty easily is adding support to track the columns used through to the executor so we can check the permissions in the right place. You should review Tom's #2 comment here: http://archives.postgresql.org/pgsql-patches/2008-05/msg00111.php Let me know if you'll be able to work on this or not. If not then I'll get to it after I'm happy with the other pieces of the patch. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Extending grant insert on tables to sequences
Stephen Frost [EMAIL PROTECTED] writes: * Jaime Casanova ([EMAIL PROTECTED]) wrote: updating the patch with one that only extends inserts. though, i haven't look at the col level privs patch yet. At least initially I wasn't planning to support column-level privileges for sequences, so I don't think it will affect you much. Do people think it makes sense to try and support that? USAGE certainly wouldn't be column-level in any case --- it'd be a privilege on the sequence as such. That end of it isn't the problem; the problem is that column-level privileges on the table make it hard to decide when to grant rights on the sequence, as I pointed out last time round: http://archives.postgresql.org/pgsql-hackers/2008-07/msg00624.php As your patch appears more ready-for-commit than the column-level privileges patch, I wouldn't worry about what code might have to move around, that'll be for me to deal with in a re-sync with HEAD once your patch is committed. I think that's backwards. The above message raises serious concerns about whether the USAGE-granting patch can be implemented at all in the presence of column-level privileges. I think the right thing is to get column privileges in and then see if it's possible to implement USAGE-granting compatibly. I don't want to commit a patch that is clearly going to be broken when (not if) column privileges arrive. I note also that no response was given to my worries about pg_dump behavior. In short, this patch isn't much more ready to commit than it was in the last fest. 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] Extending grant insert on tables to sequences
* Tom Lane ([EMAIL PROTECTED]) wrote: Stephen Frost [EMAIL PROTECTED] writes: * Jaime Casanova ([EMAIL PROTECTED]) wrote: updating the patch with one that only extends inserts. though, i haven't look at the col level privs patch yet. At least initially I wasn't planning to support column-level privileges for sequences, so I don't think it will affect you much. Do people think it makes sense to try and support that? USAGE certainly wouldn't be column-level in any case --- it'd be a privilege on the sequence as such. That end of it isn't the problem; the problem is that column-level privileges on the table make it hard to decide when to grant rights on the sequence, as I pointed out last time round: http://archives.postgresql.org/pgsql-hackers/2008-07/msg00624.php Ah, obviously I hadn't read far enough back about this patch. I agree that sequence USAGE should be granted when insert is granted on any column. One suggestion is that as the SQL spec indicates that a table-level revoke implies a revoke on all columns, we could have the revokation of the sequence permissisons done only on table-level revokation of insert and not on any individual column-level insert, even if that was the last column which insert rights were granted on. I have to admit that I'm not a big fan of that though because a given state on the table wouldn't imply a particular state for the sequence- it would depend on how you got there. The way the code is currently laid out for the column-level privileges, it wouldn't be that difficult to go through all of the other columns and check if this was the last insert being revoked, but I don't particularly like that either, and it strikes me as 99% of the time being wasted effort. I guess if we could check for and only go through that effort when there is a sequence in place with implicit grants it might not be too bad. As your patch appears more ready-for-commit than the column-level privileges patch, I wouldn't worry about what code might have to move around, that'll be for me to deal with in a re-sync with HEAD once your patch is committed. I think that's backwards. The above message raises serious concerns about whether the USAGE-granting patch can be implemented at all in the presence of column-level privileges. I think the right thing is to get column privileges in and then see if it's possible to implement USAGE-granting compatibly. I don't want to commit a patch that is clearly going to be broken when (not if) column privileges arrive. Now that I understand the situation better, I agree with you on this. I hadn't realized this patch was about implicit grants on sequnces. Sorry for the noise. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Extending grant insert on tables to sequences
On Fri, Aug 22, 2008 at 10:19 PM, Bruce Momjian [EMAIL PROTECTED] wrote:
Added to September commit fest.
updating the patch with one that only extends inserts. though, i
haven't look at the col level privs patch yet.
--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. (593) 87171157
Index: doc/src/sgml/ref/grant.sgml
===
RCS file: /var/lib/postgresql/CVSREPO/pgsql/doc/src/sgml/ref/grant.sgml,v
retrieving revision 1.70
diff -c -r1.70 grant.sgml
*** doc/src/sgml/ref/grant.sgml 3 Jul 2008 15:59:55 - 1.70
--- doc/src/sgml/ref/grant.sgml 1 Sep 2008 06:43:08 -
***
*** 401,410
/para
para
! Granting permission on a table does not automatically extend
! permissions to any sequences used by the table, including
! sequences tied to typeSERIAL/ columns. Permissions on
! sequence must be set separately.
/para
para
--- 401,410
/para
para
! Granting INSERT permissions on a table automatically extends it
! to any sequences owned by the table, including sequences tied to
! typeSERIAL/ columns. All other permissions must be set
! separately.
/para
para
Index: src/backend/catalog/aclchk.c
===
RCS file: /var/lib/postgresql/CVSREPO/pgsql/src/backend/catalog/aclchk.c,v
retrieving revision 1.147
diff -c -r1.147 aclchk.c
*** src/backend/catalog/aclchk.c 19 Jun 2008 00:46:03 - 1.147
--- src/backend/catalog/aclchk.c 1 Sep 2008 06:06:26 -
***
*** 361,366
--- 361,396
}
ExecGrantStmt_oids(istmt);
+
+ /*
+ * If the objtype is a relation and the privileges includes INSERT
+ * then extend it as USAGE to the sequences owned by the
+ * relation
+ */
+ if (istmt.objtype == ACL_OBJECT_RELATION
+ (istmt.all_privs || (istmt.privileges ACL_INSERT)))
+ {
+ InternalGrant istmt_seq;
+
+ istmt_seq.is_grant = istmt.is_grant;
+ istmt_seq.objtype = ACL_OBJECT_SEQUENCE;
+ istmt_seq.grantees = istmt.grantees;
+ istmt_seq.grant_option = istmt.grant_option;
+ istmt_seq.behavior = istmt.behavior;
+
+ istmt_seq.all_privs = false;
+ istmt_seq.privileges = ACL_NO_RIGHTS;
+
+ istmt_seq.privileges |= ACL_USAGE;
+
+ istmt_seq.objects = NIL;
+ foreach(cell, istmt.objects)
+ istmt_seq.objects = list_concat(istmt_seq.objects,
+ getOwnedSequences(lfirst_oid(cell)));
+
+ if (istmt_seq.objects != NIL)
+ ExecGrantStmt_oids(istmt_seq);
+ }
}
/*
Index: src/test/regress/expected/dependency.out
===
RCS file: /var/lib/postgresql/CVSREPO/pgsql/src/test/regress/expected/dependency.out,v
retrieving revision 1.7
diff -c -r1.7 dependency.out
*** src/test/regress/expected/dependency.out 3 Jul 2008 15:59:55 - 1.7
--- src/test/regress/expected/dependency.out 1 Sep 2008 06:18:43 -
***
*** 13,19
-- can't drop neither because they have privileges somewhere
DROP USER regression_user;
ERROR: role regression_user cannot be dropped because some objects depend on it
! DETAIL: access to table deptest
DROP GROUP regression_group;
ERROR: role regression_group cannot be dropped because some objects depend on it
DETAIL: access to table deptest
--- 13,20
-- can't drop neither because they have privileges somewhere
DROP USER regression_user;
ERROR: role regression_user cannot be dropped because some objects depend on it
! DETAIL: access to sequence deptest_f1_seq
! access to table deptest
DROP GROUP regression_group;
ERROR: role regression_group cannot be dropped because some objects depend on it
DETAIL: access to table deptest
--
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] Extending grant insert on tables to sequences
* Jaime Casanova ([EMAIL PROTECTED]) wrote: On Fri, Aug 22, 2008 at 10:19 PM, Bruce Momjian [EMAIL PROTECTED] wrote: Added to September commit fest. updating the patch with one that only extends inserts. though, i haven't look at the col level privs patch yet. At least initially I wasn't planning to support column-level privileges for sequences, so I don't think it will affect you much. Do people think it makes sense to try and support that? As your patch appears more ready-for-commit than the column-level privileges patch, I wouldn't worry about what code might have to move around, that'll be for me to deal with in a re-sync with HEAD once your patch is committed. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Extending grant insert on tables to sequences
On Fri, Aug 22, 2008 at 10:19 PM, Bruce Momjian [EMAIL PROTECTED] wrote: Added to September commit fest. why? there isn't a new patch yet... i haven't sent it because i want to see the column level privileges patch first because Tom's complaints -- regards, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. (593) 87171157 -- 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] Extending grant insert on tables to sequences
Added to September commit fest.
---
Abhijit Menon-Sen wrote:
At 2008-07-09 15:11:25 -0400, [EMAIL PROTECTED] wrote:
No, actually I meant having a lone list = lappend(list, newseq); in
the loop, so that ExecGrantStmt_oids is called only once.
Yes, I understand what you meant. I just phrased my agreement poorly.
Here's a more precise phrasing. ;-)
(I agree with Robert Treat that there seems to be no point granting
SELECT on the sequence. I don't *particularly* care about it, but I
tend towards wanting to drop that bit. This patch reflects that.)
Jaime: please feel free to use or ignore this, as you wish.
-- ams
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 15f5af0..8664203 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -361,6 +361,41 @@ ExecuteGrantStmt(GrantStmt *stmt)
}
ExecGrantStmt_oids(istmt);
+
+ /* If INSERT or UPDATE privileges are being granted or revoked on a
+ * relation, this extends the operation to include any sequences
+ * owned by the relation.
+ */
+
+ if (istmt.objtype == ACL_OBJECT_RELATION
+ (istmt.privileges (ACL_INSERT | ACL_UPDATE)))
+ {
+ InternalGrant istmt_seq;
+
+ istmt_seq.is_grant = istmt.is_grant;
+ istmt_seq.objtype = ACL_OBJECT_SEQUENCE;
+ istmt_seq.grantees = istmt.grantees;
+ istmt_seq.grant_option = istmt.grant_option;
+ istmt_seq.behavior = istmt.behavior;
+ istmt_seq.all_privs = false;
+
+ istmt_seq.privileges = ACL_NO_RIGHTS;
+ if (istmt.privileges ACL_INSERT)
+ istmt_seq.privileges |= ACL_USAGE;
+ if (istmt.privileges ACL_UPDATE)
+ istmt_seq.privileges |= ACL_UPDATE;
+
+ istmt_seq.objects = NIL;
+ foreach (cell, istmt.objects)
+ {
+ istmt_seq.objects =
+ list_concat(istmt_seq.objects,
+
getOwnedSequences(lfirst_oid(cell)));
+ }
+
+ if (istmt_seq.objects != NIL)
+ ExecGrantStmt_oids(istmt_seq);
+ }
}
/*
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Bruce Momjian [EMAIL PROTECTED]http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: Column level privileges was:(Re: [HACKERS] Extending grant insert on tables to sequences)
On 7/25/08, Stephen Frost [EMAIL PROTECTED] wrote: * Jaime Casanova ([EMAIL PROTECTED]) wrote: ok, seems this is the last one for column level patch http://archives.postgresql.org/pgsql-patches/2008-04/msg00417.php any one working it... Yes, I'm working on it hi, any work on it? may i help? -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Guayaquil - Ecuador Cel. (593) 87171157 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Column level privileges was:(Re: [HACKERS] Extending grant insert on tables to sequences)
* Jaime Casanova ([EMAIL PROTECTED]) wrote: ok, seems this is the last one for column level patch http://archives.postgresql.org/pgsql-patches/2008-04/msg00417.php any one working it... Yes, I'm working on it, but I'm not against having help, of course. The past couple weeks have been given over to commitfest though, so I havn't made much progress on it yet. My plan is to focus on it during August and have a good patch to submit for the September commitfest. Thanks, Stephen signature.asc Description: Digital signature
Re: Column level privileges was:(Re: [HACKERS] Extending grant insert on tables to sequences)
On Fri, Jul 25, 2008 at 4:51 AM, Stephen Frost [EMAIL PROTECTED] wrote: * Jaime Casanova ([EMAIL PROTECTED]) wrote: ok, seems this is the last one for column level patch http://archives.postgresql.org/pgsql-patches/2008-04/msg00417.php any one working it... Yes, I'm working on it, but I'm not against having help, of course. The past couple weeks have been given over to commitfest though, so I havn't made much progress on it yet. My plan is to focus on it during August and have a good patch to submit for the September commitfest. seems like a plan to me... do you have a repository for it? or can you send me the patch in early august? -- regards, Jaime Casanova Soporte y capacitación de PostgreSQL Guayaquil - Ecuador Cel. (593) 87171157 -- 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] Extending grant insert on tables to sequences
Sorry for the delay in the answer but i was busy with 2 projects and a talk... On Sat, Jul 12, 2008 at 3:50 PM, Tom Lane [EMAIL PROTECTED] wrote: I think it's probably reasonable as long as we keep the implicitly granted rights as narrow as possible. INSERT on the parent table would normally be hard to use correctly if you can't nextval() the sequence, so automatically allowing nextval() seems pretty reasonable. I think the case for granting anything more than that is weak --- even without considering backwards-compatibility arguments. ok. at least two more reviewers make questions against the SELECT permission... my point was that if keep INSERT and UPDATE permissions then keep SELECT as well... but let *only* INSERT's seems enough to me... A fairly important practical problem is whether this will keep pg_dump from correctly reproducing the state of a database. Assume that someone did revoke the implicitly-granted rights on the sequence --- would a dump and reload correctly preserve that state? It'd depend on the order in which pg_dump issued the GRANTs, and I'm not at all sure pg_dump could be relied on to get that right. (Even if we fixed it to account for the issue today, what of older dump scripts?) good point! a simple test make me think that yes, i will try some complex cases to be sure (actually i think it should be a problem here) Another issue is the interaction with the planned column-level GRANT feature. Although that is a feature we want, is a WIP one... do we stop patches because it can conflict with a project we don't know will be applied soon? In any case, i will review that patch to see where we are on that and to try make those two compatible... I thought for a bit about abandoning the proposed implementation and instead having nextval/currval check at runtime: IOW, if the check for ACL_USAGE on the sequence fails, look to see if the sequence is owned and if so look to see if the user has ACL_INSERT on the parent table. (This seems a bit slow but maybe it wouldn't be a problem, or maybe we could arrange to cache the lookup results.) This would avoid the action at a distance behavior in GRANT and thereby cure both of the problems mentioned above. However, it would mean that it'd be impossible to grant INSERT without effectively granting sequence USAGE --- revoking USAGE on the sequence wouldn't stop anything. Plus, \z on the sequence would fail to tell you about those implicitly held rights. seems like a hackish... do we want this? comments? i will work on this patch for the next days... -- regards, Jaime Casanova Soporte y capacitación de PostgreSQL Guayaquil - Ecuador Cel. (593) 87171157 -- 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] Extending grant insert on tables to sequences
Jaime Casanova [EMAIL PROTECTED] writes: Another issue is the interaction with the planned column-level GRANT feature. Although that is a feature we want, is a WIP one... do we stop patches because it can conflict with a project we don't know will be applied soon? Well, considering that that one is implementing a feature required by SQL spec, your feature will lose any tug-of-war ;-). So yeah, you ought to consider how to make yours play nice when (not if) that happens. 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
Column level privileges was:(Re: [HACKERS] Extending grant insert on tables to sequences)
On Thu, Jul 24, 2008 at 12:09 PM, Tom Lane [EMAIL PROTECTED] wrote: Jaime Casanova [EMAIL PROTECTED] writes: Another issue is the interaction with the planned column-level GRANT feature. Although that is a feature we want, is a WIP one... do we stop patches because it can conflict with a project we don't know will be applied soon? Well, considering that that one is implementing a feature required by SQL spec, your feature will lose any tug-of-war ;-). i knew the answer already but... ok, seems this is the last one for column level patch http://archives.postgresql.org/pgsql-patches/2008-04/msg00417.php any one working it... -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Guayaquil - Ecuador Cel. (593) 87171157 -- 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] Extending grant insert on tables to sequences
At 2008-07-11 11:57:37 -0500, [EMAIL PROTECTED] wrote:
attached is a new version of the patch, it implements Alvaro's
suggestion and fix a bug i found (it wasn't managing GRANT ALL) :(
Looks good to me.
About the SELECT issue, AFAIU Robert doesn't complaint he just asked
what is the use case... if people think it should be removed ok, but
OTOH: why? i don't think that affects anything...
As I said, I don't feel too strongly about it.
para
! Granting permission on a table automatically extend
! permissions to any sequences owned by the table, including
! sequences tied to typeSERIAL/ columns.
/para
Should be Granting permissions on a table automatically extends those
permissions to
+ if ((istmt.objtype == ACL_OBJECT_RELATION) (istmt.all_privs ||
+ (istmt.privileges (ACL_INSERT | ACL_UPDATE | ACL_SELECT
+ {
The parentheses around the first comparison can go away, and also the
ones around the ACL_* here:
+ if (istmt.privileges (ACL_INSERT))
+ istmt_seq.privileges |= ACL_USAGE;
+ if (istmt.privileges (ACL_UPDATE))
+ istmt_seq.privileges |= ACL_UPDATE;
+ if (istmt.privileges (ACL_SELECT))
+ istmt_seq.privileges |= ACL_SELECT;
-- ams
--
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] Extending grant insert on tables to sequences
On Sat, Jul 12, 2008 at 6:30 AM, Abhijit Menon-Sen [EMAIL PROTECTED] wrote:
para
! Granting permission on a table automatically extend
! permissions to any sequences owned by the table, including
! sequences tied to typeSERIAL/ columns.
/para
Should be Granting permissions on a table automatically extends those
permissions to
what about extends them to...
+ if ((istmt.objtype == ACL_OBJECT_RELATION) (istmt.all_privs ||
+ (istmt.privileges (ACL_INSERT | ACL_UPDATE | ACL_SELECT
+ {
The parentheses around the first comparison can go away, and also the
ones around the ACL_* here:
ok
--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 87171157
Index: doc/src/sgml/ref/grant.sgml
===
RCS file: /home/postgres/cvshome/pgsql/doc/src/sgml/ref/grant.sgml,v
retrieving revision 1.70
diff -c -r1.70 grant.sgml
*** doc/src/sgml/ref/grant.sgml 3 Jul 2008 15:59:55 - 1.70
--- doc/src/sgml/ref/grant.sgml 12 Jul 2008 19:22:01 -
***
*** 401,410
/para
para
! Granting permission on a table does not automatically extend
! permissions to any sequences used by the table, including
! sequences tied to typeSERIAL/ columns. Permissions on
! sequence must be set separately.
/para
para
--- 401,409
/para
para
! Granting permissions on a table automatically extend
! them to any sequences owned by the table, including
! sequences tied to typeSERIAL/ columns.
/para
para
Index: src/backend/catalog/aclchk.c
===
RCS file: /home/postgres/cvshome/pgsql/src/backend/catalog/aclchk.c,v
retrieving revision 1.147
diff -c -r1.147 aclchk.c
*** src/backend/catalog/aclchk.c 19 Jun 2008 00:46:03 - 1.147
--- src/backend/catalog/aclchk.c 12 Jul 2008 18:39:40 -
***
*** 361,366
--- 361,406
}
ExecGrantStmt_oids(istmt);
+
+ /*
+ * If the objtype is a relation and the privileges includes INSERT, UPDATE
+ * or SELECT then extends the GRANT/REVOKE to the sequences owned by the
+ * relation
+ */
+ if (istmt.objtype == ACL_OBJECT_RELATION (istmt.all_privs ||
+ (istmt.privileges (ACL_INSERT | ACL_UPDATE | ACL_SELECT
+ {
+ InternalGrant istmt_seq;
+
+ istmt_seq.is_grant = istmt.is_grant;
+ istmt_seq.objtype = ACL_OBJECT_SEQUENCE;
+ istmt_seq.grantees = istmt.grantees;
+ istmt_seq.grant_option = istmt.grant_option;
+ istmt_seq.behavior = istmt.behavior;
+
+ istmt_seq.all_privs = false;
+ istmt_seq.privileges = ACL_NO_RIGHTS;
+
+ if (istmt.all_privs)
+ istmt_seq.all_privs = true;
+ else
+ {
+ if (istmt.privileges ACL_INSERT)
+ istmt_seq.privileges |= ACL_USAGE;
+ if (istmt.privileges ACL_UPDATE)
+ istmt_seq.privileges |= ACL_UPDATE;
+ if (istmt.privileges ACL_SELECT)
+ istmt_seq.privileges |= ACL_SELECT;
+ }
+
+ istmt_seq.objects = NIL;
+ foreach(cell, istmt.objects)
+ istmt_seq.objects = list_concat(istmt_seq.objects,
+ getOwnedSequences(lfirst_oid(cell)));
+
+ if (istmt_seq.objects != NIL)
+ ExecGrantStmt_oids(istmt_seq);
+ }
}
/*
Index: src/test/regress/expected/dependency.out
===
RCS file: /home/postgres/cvshome/pgsql/src/test/regress/expected/dependency.out,v
retrieving revision 1.7
diff -c -r1.7 dependency.out
*** src/test/regress/expected/dependency.out 3 Jul 2008 15:59:55 - 1.7
--- src/test/regress/expected/dependency.out 11 Jul 2008 16:53:14 -
***
*** 13,22
-- can't drop neither because they have privileges somewhere
DROP USER regression_user;
ERROR: role regression_user cannot be dropped because some objects depend on it
! DETAIL: access to table deptest
DROP GROUP regression_group;
ERROR: role regression_group cannot be dropped because some objects depend on it
! DETAIL: access to table deptest
-- if we revoke the privileges we can drop the group
REVOKE SELECT ON deptest FROM GROUP regression_group;
DROP GROUP regression_group;
--- 13,24
-- can't drop neither because they have privileges somewhere
DROP USER regression_user;
ERROR: role regression_user cannot be dropped because some objects depend on it
! DETAIL: access to sequence deptest_f1_seq
! access to table deptest
DROP GROUP regression_group;
ERROR: role regression_group cannot be dropped because some objects depend on it
! DETAIL: access to sequence deptest_f1_seq
! access to table deptest
-- if we revoke the privileges we can drop the group
REVOKE SELECT ON deptest FROM GROUP regression_group;
DROP GROUP regression_group;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
Re: [HACKERS] Extending grant insert on tables to sequences
At 2008-07-12 14:32:03 -0500, [EMAIL PROTECTED] wrote: Should be Granting permissions on a table automatically extends those permissions to what about extends them to... Yes, sounds fine, thanks. But I notice that nobody else has commented on whether they want this feature or not. Does anyone particularly dislike the idea? -- ams -- 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] Extending grant insert on tables to sequences
Jaime Casanova [EMAIL PROTECTED] writes:
+ if (istmt.all_privs)
+ istmt_seq.all_privs = true;
+ else
+ {
+ if (istmt.privileges ACL_INSERT)
+ istmt_seq.privileges |= ACL_USAGE;
+ if (istmt.privileges ACL_UPDATE)
+ istmt_seq.privileges |= ACL_UPDATE;
+ if (istmt.privileges ACL_SELECT)
+ istmt_seq.privileges |= ACL_SELECT;
+ }
This definition of the derived rights seems pretty arbitrary and
unprincipled. If you can't explain it precisely in a few words in the
documentation (and I notice you didn't even attempt to explain it at
all), then you probably need to think harder.
In particular, I don't see why having UPDATE on the parent table should
grant the right to use setval() on the sequence. If you had INSERT and
DELETE as well, implying the right to make arbitrary changes in the
parent table, then maybe setval() would be sensible to allow --- but
this code can't really tell that, since it doesn't have a global view
of the privileges previously granted.
What I think makes sense is just to have parent INSERT privilege lead to
USAGE on the sequence (thus granting nextval/currval rights, which are
what you'd typically need in association with trying to do inserts).
I don't see any need to automatically grant more than that. Selects and
updates on the parent table don't need to touch the sequence, so why are
those privileges granting anything?
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] Extending grant insert on tables to sequences
Abhijit Menon-Sen [EMAIL PROTECTED] writes: But I notice that nobody else has commented on whether they want this feature or not. Does anyone particularly dislike the idea? I think it's probably reasonable as long as we keep the implicitly granted rights as narrow as possible. INSERT on the parent table would normally be hard to use correctly if you can't nextval() the sequence, so automatically allowing nextval() seems pretty reasonable. I think the case for granting anything more than that is weak --- even without considering backwards-compatibility arguments. A fairly important practical problem is whether this will keep pg_dump from correctly reproducing the state of a database. Assume that someone did revoke the implicitly-granted rights on the sequence --- would a dump and reload correctly preserve that state? It'd depend on the order in which pg_dump issued the GRANTs, and I'm not at all sure pg_dump could be relied on to get that right. (Even if we fixed it to account for the issue today, what of older dump scripts?) Another issue is the interaction with the planned column-level GRANT feature. AFAICS, the obvious-sounding rule that usage of the sequence should be granted consequent to granting INSERT on the owning column would be exactly backwards. It's when you have *not* got INSERT on that column that you *must* rely on the default for it, and hence you'd better have the ability to do nextval() or your alleged insert privileges on other columns are worthless. So it seems that sequence usage should be granted if any column INSERT is granted, and revoked only when all column INSERT privileges are revoked --- and that latter rule is going to be hard to implement with this type of patch, because it doesn't know what column privileges are going to remain. I thought for a bit about abandoning the proposed implementation and instead having nextval/currval check at runtime: IOW, if the check for ACL_USAGE on the sequence fails, look to see if the sequence is owned and if so look to see if the user has ACL_INSERT on the parent table. (This seems a bit slow but maybe it wouldn't be a problem, or maybe we could arrange to cache the lookup results.) This would avoid the action at a distance behavior in GRANT and thereby cure both of the problems mentioned above. However, it would mean that it'd be impossible to grant INSERT without effectively granting sequence USAGE --- revoking USAGE on the sequence wouldn't stop anything. Plus, \z on the sequence would fail to tell you about those implicitly held rights. So I'm not sure I like this way any better. 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] Extending grant insert on tables to sequences
On Wed, Jul 9, 2008 at 10:11 PM, Abhijit Menon-Sen [EMAIL PROTECTED] wrote:
At 2008-07-09 15:11:25 -0400, [EMAIL PROTECTED] wrote:
No, actually I meant having a lone list = lappend(list, newseq); in
the loop, so that ExecGrantStmt_oids is called only once.
Yes, I understand what you meant. I just phrased my agreement poorly.
Here's a more precise phrasing. ;-)
(I agree with Robert Treat that there seems to be no point granting
SELECT on the sequence. I don't *particularly* care about it, but I
tend towards wanting to drop that bit. This patch reflects that.)
Hi,
sorry for the delay i was busy...
attached is a new version of the patch, it implements Alvaro's
suggestion and fix a bug i found (it wasn't managing GRANT ALL) :(
About the SELECT issue, AFAIU Robert doesn't complaint he just asked
what is the use case... if people think it should be removed ok, but
OTOH: why? i don't think that affects anything...
--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 87171157
Index: doc/src/sgml/ref/grant.sgml
===
RCS file: /home/postgres/cvshome/pgsql/doc/src/sgml/ref/grant.sgml,v
retrieving revision 1.70
diff -c -r1.70 grant.sgml
*** doc/src/sgml/ref/grant.sgml 3 Jul 2008 15:59:55 - 1.70
--- doc/src/sgml/ref/grant.sgml 11 Jul 2008 16:29:52 -
***
*** 401,410
/para
para
! Granting permission on a table does not automatically extend
! permissions to any sequences used by the table, including
! sequences tied to typeSERIAL/ columns. Permissions on
! sequence must be set separately.
/para
para
--- 401,409
/para
para
! Granting permission on a table automatically extend
! permissions to any sequences owned by the table, including
! sequences tied to typeSERIAL/ columns.
/para
para
Index: src/backend/catalog/aclchk.c
===
RCS file: /home/postgres/cvshome/pgsql/src/backend/catalog/aclchk.c,v
retrieving revision 1.147
diff -c -r1.147 aclchk.c
*** src/backend/catalog/aclchk.c 19 Jun 2008 00:46:03 - 1.147
--- src/backend/catalog/aclchk.c 11 Jul 2008 16:37:24 -
***
*** 361,366
--- 361,406
}
ExecGrantStmt_oids(istmt);
+
+ /*
+ * If the objtype is a relation and the privileges includes INSERT, UPDATE
+ * or SELECT then extends the GRANT/REVOKE to the sequences owned by the
+ * relation
+ */
+ if ((istmt.objtype == ACL_OBJECT_RELATION) (istmt.all_privs ||
+ (istmt.privileges (ACL_INSERT | ACL_UPDATE | ACL_SELECT
+ {
+ InternalGrant istmt_seq;
+
+ istmt_seq.is_grant = istmt.is_grant;
+ istmt_seq.objtype = ACL_OBJECT_SEQUENCE;
+ istmt_seq.grantees = istmt.grantees;
+ istmt_seq.grant_option = istmt.grant_option;
+ istmt_seq.behavior = istmt.behavior;
+
+ istmt_seq.all_privs = false;
+ istmt_seq.privileges = ACL_NO_RIGHTS;
+
+ if (istmt.all_privs)
+ istmt_seq.all_privs = true;
+ else
+ {
+ if (istmt.privileges (ACL_INSERT))
+ istmt_seq.privileges |= ACL_USAGE;
+ if (istmt.privileges (ACL_UPDATE))
+ istmt_seq.privileges |= ACL_UPDATE;
+ if (istmt.privileges (ACL_SELECT))
+ istmt_seq.privileges |= ACL_SELECT;
+ }
+
+ istmt_seq.objects = NIL;
+ foreach(cell, istmt.objects)
+ istmt_seq.objects = list_concat(istmt_seq.objects,
+ getOwnedSequences(lfirst_oid(cell)));
+
+ if (istmt_seq.objects != NIL)
+ ExecGrantStmt_oids(istmt_seq);
+ }
}
/*
Index: src/test/regress/expected/dependency.out
===
RCS file: /home/postgres/cvshome/pgsql/src/test/regress/expected/dependency.out,v
retrieving revision 1.7
diff -c -r1.7 dependency.out
*** src/test/regress/expected/dependency.out 3 Jul 2008 15:59:55 - 1.7
--- src/test/regress/expected/dependency.out 11 Jul 2008 16:53:14 -
***
*** 13,22
-- can't drop neither because they have privileges somewhere
DROP USER regression_user;
ERROR: role regression_user cannot be dropped because some objects depend on it
! DETAIL: access to table deptest
DROP GROUP regression_group;
ERROR: role regression_group cannot be dropped because some objects depend on it
! DETAIL: access to table deptest
-- if we revoke the privileges we can drop the group
REVOKE SELECT ON deptest FROM GROUP regression_group;
DROP GROUP regression_group;
--- 13,24
-- can't drop neither because they have privileges somewhere
DROP USER regression_user;
ERROR: role regression_user cannot be dropped because some objects depend on it
! DETAIL: access to sequence deptest_f1_seq
! access to table deptest
DROP GROUP regression_group;
ERROR: role regression_group cannot be dropped because some objects depend on it
! DETAIL: access to sequence deptest_f1_seq
! access to
Re: [HACKERS] Extending grant insert on tables to sequences
At 2008-07-08 09:32:44 -0400, [EMAIL PROTECTED] wrote: The idea of this patch is to avoid the need to make explicit grants on sequences owned by tables. [...] I had a look at this patch and it looks good. The only thing that's not clear to me is whether we have agreed we want this to be the default behavior? For what it's worth, I quite like the idea. (I looked at the patch, and it looks good to me too.) Wouldn't it be clearer to build a list with all the sequences owned by the tables in istmt.objects, and then call ExecGrantStmt_oids() a single time with the big list? i.e., to hoist most of the istmt_seq initialisation out of the loop, right? Yes, that makes sense. -- ams -- 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] Extending grant insert on tables to sequences
Abhijit Menon-Sen escribió: At 2008-07-08 09:32:44 -0400, [EMAIL PROTECTED] wrote: Wouldn't it be clearer to build a list with all the sequences owned by the tables in istmt.objects, and then call ExecGrantStmt_oids() a single time with the big list? i.e., to hoist most of the istmt_seq initialisation out of the loop, right? Yes, that makes sense. No, actually I meant having a lone list = lappend(list, newseq); in the loop, so that ExecGrantStmt_oids is called only once. The initialization is done only once too, of course. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] Extending grant insert on tables to sequences
At 2008-07-09 15:11:25 -0400, [EMAIL PROTECTED] wrote:
No, actually I meant having a lone list = lappend(list, newseq); in
the loop, so that ExecGrantStmt_oids is called only once.
Yes, I understand what you meant. I just phrased my agreement poorly.
Here's a more precise phrasing. ;-)
(I agree with Robert Treat that there seems to be no point granting
SELECT on the sequence. I don't *particularly* care about it, but I
tend towards wanting to drop that bit. This patch reflects that.)
Jaime: please feel free to use or ignore this, as you wish.
-- ams
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 15f5af0..8664203 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -361,6 +361,41 @@ ExecuteGrantStmt(GrantStmt *stmt)
}
ExecGrantStmt_oids(istmt);
+
+ /* If INSERT or UPDATE privileges are being granted or revoked on a
+* relation, this extends the operation to include any sequences
+* owned by the relation.
+*/
+
+ if (istmt.objtype == ACL_OBJECT_RELATION
+ (istmt.privileges (ACL_INSERT | ACL_UPDATE)))
+ {
+ InternalGrant istmt_seq;
+
+ istmt_seq.is_grant = istmt.is_grant;
+ istmt_seq.objtype = ACL_OBJECT_SEQUENCE;
+ istmt_seq.grantees = istmt.grantees;
+ istmt_seq.grant_option = istmt.grant_option;
+ istmt_seq.behavior = istmt.behavior;
+ istmt_seq.all_privs = false;
+
+ istmt_seq.privileges = ACL_NO_RIGHTS;
+ if (istmt.privileges ACL_INSERT)
+ istmt_seq.privileges |= ACL_USAGE;
+ if (istmt.privileges ACL_UPDATE)
+ istmt_seq.privileges |= ACL_UPDATE;
+
+ istmt_seq.objects = NIL;
+ foreach (cell, istmt.objects)
+ {
+ istmt_seq.objects =
+ list_concat(istmt_seq.objects,
+
getOwnedSequences(lfirst_oid(cell)));
+ }
+
+ if (istmt_seq.objects != NIL)
+ ExecGrantStmt_oids(istmt_seq);
+ }
}
/*
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
