Re: New patch for Column-level privileges - Mailing list pgsql-hackers
From | KaiGai Kohei |
---|---|
Subject | Re: New patch for Column-level privileges |
Date | |
Msg-id | 496BF538.6070903@ak.jp.nec.com Whole thread Raw |
In response to | Re: New patch for Column-level privileges (Stephen Frost <sfrost@snowman.net>) |
Responses |
Re: New patch for Column-level privileges
(KaiGai Kohei <kaigai@ak.jp.nec.com>)
|
List | pgsql-hackers |
Stephen, The revised patch can reproduce the original matter, as follows: ---------------- postgres=# CREATE TABLE t1 (a int, b text); CREATE TABLE postgres=# CREATE TABLE t2 (x int, y text); CREATE TABLE postgres=# GRANT select(a) on t1 to ymj; GRANT postgres=# GRANT select(x,y) on t2 TO ymj; GRANT postgres=# \c - ymj psql (8.4devel) You are now connected to database "postgres" as user "ymj". postgres=> SELECT a, y FROM t1 JOIN t2 ON a = x; NOTICE: make_var: attribute 1 added on rte(relid=16398, rtekind=0) NOTICE: make_var: attribute 1 added on rte(relid=16404, rtekind=0) NOTICE: make_var: attribute 1 added on rte(relid=0, rtekind=2) NOTICE: make_var: attribute 4 added on rte(relid=0, rtekind=2) NOTICE: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002 NOTICE: pg_attribute_aclmask: t1.b required: 0002 allowed: 0000 ERROR: permission denied for relation t1 ---------------- I think it is not an essentiality whether it walks on query tree, or not. So, I also suppose putting this code on make_var(). However, it does not care the case when rte->relkind == RTE_JOIN, so it requires column-level privileges on whole of columns including unrefered ones. My suggestiong is case separations. If rte->relkind == RTE_RELATION, it can keep current behavior, as is. If rte->relkind == RTE_JOIN, it need to find the source relation recursively and marks required column. Please note that the source relation can be RTE_JOIN or others. Elsewhere, we don't need to care anymore. Thanks, Stephen Frost wrote: > Tom, et al, > > * Stephen Frost (sfrost@snowman.net) wrote: >>> applyColumnPrivs is misnamed as it's not "applying" any privileges nor >>> indeed doing much of anything directly to do with privileges. It should >>> probably be named something more like findReferencedColumns. And what's >>> with the special exception for SortGroupClause!? >> I'm not sure what the story with SortGroupClause is.. Might just have >> been a bit more difficult to do. KaiGai? > > This should be resolved now, since.. > >>> Actually, though, you probably shouldn't have applyColumnPrivsWalker at all. >>> It requires an additional traversal of the parse tree, and an additional RTE >>> search for each var, for no good reason. Can't we just mark the column >>> as referenced in make_var() and maybe a couple other places that already have >>> their hands on the RTE? >> I certainly like this idea and I'll look into it, but it might take me a >> bit longer to find the appropriate places beyond make_var(). > > I've implemented and tested these suggested changes, and have removed > applyColumnPrivs, etc. It passes all the regression tests, which had a > variety of tests, so I'm reasonably happy with this. > >>> pg_attribute_aclmask seems to need a rethink. I don't like the amount >>> of policy copied-and-pasted from pg_class_aclmask, nor the fact that >>> it will fail for system attributes (attnum < 0), nor the fact that it >>> insists on looping over the attributes even if the relation privileges >>> would provide what's needed. (Indeed, you shouldn't need that "merge >>> ACLs" operation at all -- you could be ORing a couple of bitmasks >>> instead, no?) >> I'll have to think of the entry points for pg_attribute_aclmask. In >> general, we shouldn't ever get to it if the relation privileges are >> sufficient but I think it's exposed to the user at some level, where >> we would be wrong to say a user doesn't have rights on the column >> when they do have the appropriate table-level rights. Unfortunately, >> aclmask() uses information beyond the bitmasks (who granted them, >> specifically) so I don't think we can just OR the bitmasks. >> >> With regard to looping through the attributes, I could split it up into >> two functions, would that be better? A function that handles >> all-attribute cases (either 'AND'd or 'OR'd together depending on what >> the caller wants) could be added pretty easily and then >> pg_attribute_aclmask could be reverted to just handling a single >> attribute at a time (which would fix it for system attributes as >> well..). > > I've modified the code to handle system attributes but otherwise kept it > pretty much the same (with the loop and the special values to indicate > how to handle it). I looked at creating a seperate function and it > really seemed like it would be alot of code duplication.. It might > still be possible to refactor it if you'd really like. I'm open to > other thoughts or suggestions you have based on my comments above. > > Updated patch attached. > > Thanks! > > Stephen -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
pgsql-hackers by date: