Skip site navigation (1) Skip section navigation (2)

Re: New patch for Column-level privileges

From: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, sfrost(at)snowman(dot)net
Cc: Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)ec>, Markus Wanner <markus(at)bluegap(dot)ch>, Alex Hunsaker <badalex(at)gmail(dot)com>, PostgreSQL-development Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: New patch for Column-level privileges
Date: 2009-01-13 03:54:12
Message-ID: 496C1064.8030700@ak.jp.nec.com (view raw or flat)
Thread:
Lists: 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(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>

In response to

Responses

pgsql-hackers by date

Next:From: Robert HaasDate: 2009-01-13 04:14:01
Subject: Re: Recovery Test Framework
Previous:From: KaiGai KoheiDate: 2009-01-13 01:58:16
Subject: Re: New patch for Column-level privileges

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group