Re: New patch for Column-level privileges - Mailing list pgsql-hackers

I can find a matter on JOIN.

Please make sure the following function invocation chain:

transformSelectStmt()-> transformFromClause() -> transformFromClauseItem()  -> expandRTE(), if JoinExpr   ->
expandRelation(),if rte->rtekind == RTE_RELATION    -> expandTupleDesc()
 

expandTupleDesc() set rte->cols_sel for all the user columns.
It seems to me unexpected behavior.

Maybe, the point to mark rte->cols_sel has to be moved.
However, I wonder whether it is actually proper manner to put
a code depending its invocation context on existing parser.

I can understand Tom's concern, but does it make maintenance
difficulty in the future version?
For example, when a new future invokes expandRTE(), it also
polutes rte->cols_sel implicitly.
I reconsidered the previous walker implementation independent
from other parser codes is more simple and better.

Stephen, Tom, what is your opinion?

Thanks,


KaiGai Kohei wrote:
> 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>