Re: New patch for Column-level privileges

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(at)snowman(dot)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(at)ak(dot)jp(dot)nec(dot)com>