Re: New patch for Column-level privileges

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>