Re: [HACKERS] WIP: Column-level Privileges
On Tue, Nov 25, 2008 at 4:03 PM, Stephen Frost [EMAIL PROTECTED] wrote: * Alvaro Herrera ([EMAIL PROTECTED]) wrote: I had a look at aclchk.c and didn't like your change to objectNamesToOids; seems rather baroque. I changed it per the attached patch. I've incorporated this change. Moreover I didn't very much like the way aclcheck_error_col is dealing with two or one % escapes. I think you should have a separate routine for the column case, and prepend a dummy string to no_priv_msg. I can do this, not really a big deal. Stephen, Are you sending an updated patch with these minor changes? ...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] WIP: Column-level Privileges
Alvaro, * Alvaro Herrera ([EMAIL PROTECTED]) wrote: I had a look at aclchk.c and didn't like your change to objectNamesToOids; seems rather baroque. I changed it per the attached patch. I've incorporated this change. Moreover I didn't very much like the way aclcheck_error_col is dealing with two or one % escapes. I think you should have a separate routine for the column case, and prepend a dummy string to no_priv_msg. I can do this, not really a big deal. Why is there a InternalGrantStmt.rel_level? Doesn't it suffice to check whether col_privs is NIL? No, a single statement can include both relation-level and column-level permission changes. The rel_level flag is there to indicate if there are any relation-level changes. Nothing else indicates that. Is there enough common code in ExecGrant_Relation to justify the way you have it? Can the common be refactored in a better way that separates the two cases more clearly? I've looked at this a couple of times and I've not been able to see a good way to do that. I agree that there's alot of common code and it seems like there should be a way to factor it out, but there are a number of differences that make it difficult. If you see something I'm missing, please let me know. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
Alvaro Herrera wrote:
I'll probably fix both things and submit an updated version tomorrow.
Here it is. This applies cleanly to current CVS HEAD and passes the
regression tests. Apart from fixing the conflicts, I updated psql's \z
with the new array aggregate, and changed the Schema_pg_* declarations
in pg_attribute.h to contain initializators for attacl (I'm not sure
that { 0 } is the correct initializator, but it seems better than
omitting it completely). Also tacked _null_ at the end of the DATA
lines.
I didn't check the rest of the code, so don't count this as a review.
--
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] WIP: Column-level Privileges
Alvaro Herrera wrote: Alvaro Herrera wrote: I'll probably fix both things and submit an updated version tomorrow. Here it is. Really attached this time. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. colprivs_wip.2008111401.patch.gz Description: Binary data -- 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] WIP: Column-level Privileges
Alvaro Herrera wrote:
I didn't check the rest of the code, so don't count this as a review.
I had a look at aclchk.c and didn't like your change to
objectNamesToOids; seems rather baroque. I changed it per the attached
patch.
Moreover I didn't very much like the way aclcheck_error_col is dealing
with two or one % escapes. I think you should have a separate routine
for the column case, and prepend a dummy string to no_priv_msg.
Why is there a InternalGrantStmt.rel_level? Doesn't it suffice to
check whether col_privs is NIL?
Is there enough common code in ExecGrant_Relation to justify the way you
have it? Can the common be refactored in a better way that separates
the two cases more clearly?
--
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
diff -u src/backend/catalog/aclchk.c src/backend/catalog/aclchk.c
--- src/backend/catalog/aclchk.c 14 Nov 2008 12:24:15 -
+++ src/backend/catalog/aclchk.c 14 Nov 2008 20:46:06 -
@@ -55,7 +55,8 @@
static void ExecGrant_Namespace(InternalGrant *grantStmt);
static void ExecGrant_Tablespace(InternalGrant *grantStmt);
-static List *objectNamesToOids(GrantObjectType objtype, List *objnames, Oid in_relOid);
+static List *objectNamesToOids(GrantObjectType objtype, List *objnames);
+static List *columnNamesToAttnums(List *colnames, Oid relid);
static AclMode string_to_privilege(const char *privname);
static const char *privilege_to_string(AclMode privilege);
static AclMode restrict_and_check_grant(bool is_grant, AclMode avail_goptions,
@@ -264,7 +265,7 @@
*/
istmt.is_grant = stmt-is_grant;
istmt.objtype = stmt-objtype;
- istmt.objects = objectNamesToOids(stmt-objtype, stmt-objects, 0);
+ istmt.objects = objectNamesToOids(stmt-objtype, stmt-objects);
/* all_privs to be filled below */
/* privileges to be filled below */
istmt.col_privs = NIL;
@@ -402,23 +403,24 @@
foreach(cell_obj, istmt.objects)
{
Oid relOid = lfirst_oid(cell_obj);
- List *cols_oids = NIL;
+ List *colnums;
ListCell *cell_coloid;
/* Get the attribute numbers for this relation for the columns affected */
- cols_oids = objectNamesToOids(ACL_OBJECT_COLUMN, privnode-cols, relOid);
+ colnums = columnNamesToAttnums(privnode-cols, relOid);
/* Loop through the columns listed for this privilege and
* add them to the col_privs list of privileges */
- foreach(cell_coloid, cols_oids)
+ foreach(cell_coloid, colnums)
{
- ListCell *cell_colprivs;
- int found = false;
- AttrNumber curr_attnum = lfirst_int(cell_coloid);
+ AttrNumber curr_attnum = lfirst_int(cell_coloid);
+ ListCell *cell_colprivs;
+ int found = false;
/* Check if we have already seen this column, and if
* so then just add to its aclmask. */
- foreach(cell_colprivs, istmt.col_privs) {
+ foreach(cell_colprivs, istmt.col_privs)
+ {
ColPrivs *col_privs = (ColPrivs *) lfirst(cell_colprivs);
if (col_privs-relOid == relOid col_privs-attnum == curr_attnum)
@@ -487,50 +489,18 @@
* objectNamesToOids
*
* Turn a list of object names of a given type into an Oid list.
- * For columns, turn a list of column names into a list of
- * attribute numbers, for a given relation in_relOid.
*/
static List *
-objectNamesToOids(GrantObjectType objtype, List *objnames, Oid in_relOid)
+objectNamesToOids(GrantObjectType objtype, List *objnames)
{
List *objects = NIL;
ListCell *cell;
Assert(objnames != NIL);
+ AssertArg(objtype != ACL_OBJECT_COLUMN);
switch (objtype)
{
- case ACL_OBJECT_COLUMN:
- foreach(cell, objnames)
- {
-AttrNumber attnum;
-
-attnum = get_attnum(in_relOid,strVal(lfirst(cell)));
-if (attnum == InvalidAttrNumber)
-{
- HeapTuple tuple;
- Form_pg_class pg_class_tuple;
-
- tuple = SearchSysCache(RELOID,
- ObjectIdGetDatum(in_relOid),
- 0, 0, 0);
-
- if (!HeapTupleIsValid(tuple))
- elog(ERROR, cache lookup failed for relation %u, in_relOid);
-
- pg_class_tuple = (Form_pg_class) GETSTRUCT(tuple);
-
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_COLUMN),
- errmsg(column \%s\ of relation %s does not exist.,
- strVal(lfirst(cell)), NameStr(pg_class_tuple-relname;
-
- ReleaseSysCache(tuple);
-}
-
-objects = lappend_int(objects, attnum);
- }
- break;
case ACL_OBJECT_RELATION:
case ACL_OBJECT_SEQUENCE:
foreach(cell, objnames)
@@ -647,6 +617,39 @@
}
/*
+ * columnNamesToAttnums
+ *
+ * Turn a list of column names into a list of attribute numbers, for the given
+ * relation.
+ */
+static List *
+columnNamesToAttnums(List *colnames, Oid relid)
+{
+ ListCell *cell;
+ List *colnums = NIL;
+
+ foreach(cell, colnames)
+ {
+ AttrNumber attnum;
+
+ attnum = get_attnum(relid, strVal(lfirst(cell)));
+ if (attnum == InvalidAttrNumber)
+
Re: [HACKERS] WIP: Column-level Privileges
Hello Stephen, Stephen Frost wrote: Attached patch has this fixed and still passes all regression tests, etc. Do you have an up-to-date patch laying around? The current one conflicts with some CVS tip changes. I didn't get around writing some docu, yet. Sorry. Regards Markus Wanner -- 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] WIP: Column-level Privileges
Markus, * Markus Wanner ([EMAIL PROTECTED]) wrote: Stephen Frost wrote: Attached patch has this fixed and still passes all regression tests, etc. Do you have an up-to-date patch laying around? The current one conflicts with some CVS tip changes. No, not yet. I suspect the array_agg patch is conflicting (which is a good thing, really) and the addition of array_agg by my patch can now be removed. Testing should be done to ensure nothing changed wrt output, of course, but I'm not too worried. I can probably update it this weekend, depending on how things are going with the newborn (she's only 4 days old at this point, after all.. :). Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
Stephen Frost wrote: Markus, * Markus Wanner ([EMAIL PROTECTED]) wrote: Stephen Frost wrote: Attached patch has this fixed and still passes all regression tests, etc. Do you have an up-to-date patch laying around? The current one conflicts with some CVS tip changes. No, not yet. I suspect the array_agg patch is conflicting (which is a good thing, really) and the addition of array_agg by my patch can now be removed. Testing should be done to ensure nothing changed wrt output, of course, but I'm not too worried. Yes, it has a conflict with the array_agg patch. I had some time on my hands today so I stole the part of the patch that dealt with pg_attribute tuples, tinkered with it a bit, and committed it. So now your patch conflicts with more stuff :-( I'll probably fix both things and submit an updated version tomorrow. -- 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] WIP: Column-level Privileges
Hello Stephen, Stephen Frost wrote: This has been fixed in the attached patch. Cool, thanks. If you could work on the documentation, that'd be great! I'll give it a try. Regards Markus Wanner -- 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] WIP: Column-level Privileges
* Tom Lane ([EMAIL PROTECTED]) wrote: Stephen Frost [EMAIL PROTECTED] writes: ... A case I just realized might be an issue is doing a 'select 1 from x;' where you have *no* rights on x, or any columns in it, would still get you the rowcount. Well, if you have table-level select on x, I would expect that to work, even if your privs on every column of x are revoked. If the patch doesn't get this right then it needs more work ... Table-level select on x is equivilant to having column-level select on every column, per the spec. The issue here, that I'm planning to fix shortly, is that you could get a rowcount without having table-level or column-level select rights on the table. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
* Tom Lane ([EMAIL PROTECTED]) wrote: Stephen Frost [EMAIL PROTECTED] writes: ... A case I just realized might be an issue is doing a 'select 1 from x;' where you have *no* rights on x, or any columns in it, would still get you the rowcount. Well, if you have table-level select on x, I would expect that to work, even if your privs on every column of x are revoked. If the patch doesn't get this right then it needs more work ... Attached patch has this fixed and still passes all regression tests, etc. Thanks, Stephen colprivs_wip.2008110201.diff.gz Description: Binary data signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
Markus, * Markus Wanner ([EMAIL PROTECTED]) wrote: Sorry, this took way longer than planned. Beleive me, I understand. :) testdb=# GRANT TRUNCATE (single_col) ON test TO malory; GRANT This has been fixed in the attached patch. Some privilege regression tests currently fail with your patch, but I think that's expected. All regression tests should pass now. Documentation and new regression tests for column level privileges are still missing. If you want, Stephen, I can work on that. If you could work on the documentation, that'd be great! I've updated the regression tests to include some testing of the column level privileges. Feel free to suggest or add to them though, and if you find anything not working as you'd expect, please let me know! There are a few outstanding items that I can think of- The error-reporting could be better (eg, give the specific column that you don't have rights on, rather than just saying you don't have rights on the relation), but I wasn't sure if people would be happy with the change to existing error messages that would imply. Basically, rather than getting told you don't have rights on the relation, you would be told you don't have rights on the first column in the relation that you don't have the necessary rights on. It's a simple change, if people are agreeable to it. Having it give the table-level message only when there aren't column-level privileges is possible, but makes the code rather ugly.. Documentation, of course. More testing, more review, etc, etc, making sure everything is working as expected, more complex queries than what I've done to make sure things happen correctly. Tom has me rather nervous based on his previous comments about the rewriter/optimizer causing problems, and I barely touched them.. I also wonder if you could use joins or something to extract information about columns you're not supposed to have access to, or where clauses, etc.. Anyhow, updated patch attached. Thanks, Stephen colprivs_wip.2008110102.diff.gz Description: Binary data signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
Markus, et al, * Stephen Frost ([EMAIL PROTECTED]) wrote: I also wonder if you could use joins or something to extract information about columns you're not supposed to have access to, or where clauses, etc.. welp, I've done some additional testing and there's good news and bad, I suppose. The good news is that when relations are join'd, they go through expandRelation, which adds all the columns in that relation to the 'required' set, so you have to have rights to all columns on a table to join against it in the normal way. On the other hand, you can just select out the columns you have access to in a subquery and then join against *that* and it works. updates with where clauses and inserts-with-selects seem to work correctly though, which is nice. A case I just realized might be an issue is doing a 'select 1 from x;' where you have *no* rights on x, or any columns in it, would still get you the rowcount. That might not be too hard to fix though, I'll look into it tomorrow sometime. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
Stephen Frost [EMAIL PROTECTED] writes: ... A case I just realized might be an issue is doing a 'select 1 from x;' where you have *no* rights on x, or any columns in it, would still get you the rowcount. Well, if you have table-level select on x, I would expect that to work, even if your privs on every column of x are revoked. If the patch doesn't get this right then it needs more work ... 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] WIP: Column-level Privileges
Hi, Markus Wanner wrote: As mentioned in above, regression tests, documentation updates, dependency handling, and actually implementing the permission checks all remain. What I'm looking for feedback on are the changes to the grammer, parser, catalog changes, psql output, etc. Aha, good. So I'm going to (try to) check these things and comment. Sorry, this took way longer than planned. The grammar and parser changes look fine to me. You've added a 'Priv' parser node which now stores a privilege string (like 'REFERENCES', 'SELECT' or 'CREATE') as well as a list of affected columns. I've been wondering about the use of 'ColId' instead of all the other options (i.e. 'UPDATE', 'DELETE', 'TRUNCATE', ...). But that has obviously been there before. Checking it is deferred to later giving an unrecognized privilege type error. I'm wondering why this is done that way. Seems to be related to some unreserved_keywords vs col_name_keyword vs reserved_keywords issue. However, the following is certainly bogus and needs to be prevented by the parser or later privilege type checking code: testdb=# GRANT TRUNCATE (single_col) ON test TO malory; GRANT Otherwise the syntax seems to match what my SQL 2008 draft is telling. MySQL does it the same way as well. The catalog changes have been discussed with Tom. Some privilege regression tests currently fail with your patch, but I think that's expected. Documentation and new regression tests for column level privileges are still missing. If you want, Stephen, I can work on that. Given the known-unfinished state of this patch I'm moving it to the November commit fest. Hope that's fine with you. I'm glad to help and review as updated patch, no matter what the commit fest state is. Regards Markus Wanner -- 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] WIP: Column-level Privileges
Hello Stephen, Stephen Frost wrote: Comments welcome, apologies for it not being ready by 9/1. I'm planning to continue working on it tomorrow, and throughout September as opportunity allows (ie: when Josh isn't keeping me busy). I'm trying to review this patch. I could at least compile it successfully for now. There have already been some comments from Tom [1]. You've mostly answered that you are going to fix these issues, but I'm pretty unclear on what the current status is (of patch 09/02). Can you please elaborate on what's done and what not? As this is still marked as WIP on the wiki page: what needs to be done until you consider it done? Regards Markus Wanner [1]: Comments from Tom: http://archives.postgresql.org/pgsql-patches/2008-05/msg00111.php -- 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] WIP: Column-level Privileges
Hi Markus, * Markus Wanner ([EMAIL PROTECTED]) wrote: Stephen Frost wrote: Comments welcome, apologies for it not being ready by 9/1. I'm planning to continue working on it tomorrow, and throughout September as opportunity allows (ie: when Josh isn't keeping me busy). I'm trying to review this patch. I could at least compile it successfully for now. That's a start at least. :) There have already been some comments from Tom [1]. You've mostly answered that you are going to fix these issues, but I'm pretty unclear on what the current status is (of patch 09/02). Can you please elaborate on what's done and what not? I would suggest you review the updated patch (linked off the wiki page) here: http://archives.postgresql.org/message-id/[EMAIL PROTECTED] It includes my comments about what's done and what's not. I reiterated much of it here as well: http://archives.postgresql.org/pgsql-hackers/2008-09/msg00263.php As this is still marked as WIP on the wiki page: what needs to be done until you consider it done? As mentioned in above, regression tests, documentation updates, dependency handling, and actually implementing the permission checks all remain. What I'm looking for feedback on are the changes to the grammer, parser, catalog changes, psql output, etc. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: Column-level Privileges
Hi, Stephen Frost wrote: I would suggest you review the updated patch (linked off the wiki page) here: http://archives.postgresql.org/message-id/[EMAIL PROTECTED] That's the patch I've been talking about: file colprivs_wip.20080902.diff.gz, dated Sept, 2nd. It includes my comments about what's done and what's not. I reiterated much of it here as well: http://archives.postgresql.org/pgsql-hackers/2008-09/msg00263.php Uh.. I've read that message as well, but didn't find it overly clear on what you were referring to. As mentioned in above, regression tests, documentation updates, dependency handling, and actually implementing the permission checks all remain. What I'm looking for feedback on are the changes to the grammer, parser, catalog changes, psql output, etc. Aha, good. So I'm going to (try to) check these things and comment. Regards Markus Wanner -- 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] WIP: Column-level Privileges
Greetings, * Stephen Frost ([EMAIL PROTECTED]) wrote: Comments welcome, apologies for it not being ready by 9/1. I'm planning to continue working on it tomorrow, and throughout September as opportunity allows (ie: when Josh isn't keeping me busy). Here is an updated patch. I've added column-level privilege information to the psql output as an additional column for \dp, but it depends on the array_accum() aggregate which isn't included (yet). This output matches more what I would expect, but I'm open to comments. An example of what it looks like is here, for the next few days: http://pgsql.privatepaste.com/871z3IheMr I havn't tested everything, but basic column-level grant/revoke should work now. This includes: grammer, parser, catalog changes. It also properly does the 'revoke all column-level when called as a table-level revoke' SQL spec requirement. It does not yet have proper dependency handling. Next I'm planning to work on adding some regression tests for grant/revoke commands and catalog updates and make sure those all work correctly. Then I'll probably go through the dependency handling, and last will be working on the changes to implement the permission checks. Thanks, Stephen colprivs_wip.20080902.diff.gz Description: Binary data signature.asc Description: Digital signature
