Re: New patch for Column-level privileges

Markus,

* Markus Wanner (markus(at)bluegap(dot)ch) wrote:
> Stephen Frost wrote:
> > ..in the attached patch.
>
> Thanks, I've reviewed this patch again.

Thanks!

> > Currently,
> > column-level privileges are not honored when JOINs are involved (you
> > must have the necessary table-level privileges, as you do today). It
> > would really be great to have that working and mainly involves
> > modifying the rewriter to add on to the appropriate range table column
> > list entries the columns which are used in the joins and output from
> > joins.
>
> Understood. Do you plan to work on that? Or do you think the patch
> should go into 8.4 even without support for JOINs?

I'm going to look into it but it's a bit complicated. I was hoping
someone who's more familiar with those parts would be able to look at
it.

> Experimenting with relation vs column level privileges, I've discovered
> a strange behavior:
>
> test=# GRANT SELECT ON test TO joe;
> GRANT
> test=# GRANT SELECT(id) ON test TO joe;
> GRANT
> test=# REVOKE SELECT ON test FROM joe;
> ERROR: tuple already updated by self
>
> That's odd. Maybe you need to increment the command counter in between
> two updates of that tuple?

I don't think that's the right approach, but I'll look into it. I ran
into a similiar issue though, and I don't believe it's too hard to fix
(the issue here is that the REVOKE needs to remove the column-level grant
as well). I'll try and look into it tonight or tomorrow.

> Also note, that I'm unsure about what to expect from this REVOKE. Is it
> intended to remove the column level privilege as well or not?

Yes, the SQL spec requires that a table-level REVOKE also revoke all
column-level grants as well.

> Removing the column level privilege first, then the relation level one
> works:
>
> test=# REVOKE SELECT(id) ON test FROM joe;
> REVOKE
> test=# REVOKE SELECT ON test FROM joe;
> REVOKE

This is essentially what should happen automagically.

> I've also tested column level privileges on hidden attributes (xmin,
> xmax, ctid, tableoid, cmin), which works fine for SELECTs. However, you
> might want to filter out INSERT and UPDATE privileges on those, as those
> don't make much sense:
>
> test=# GRANT UPDATE(xmin) ON test TO joe;
> GRANT
> test=# GRANT INSERT(xmin) ON test TO joe;
> GRANT

Hmm, ok, that's easy enough to fix.

> [ Note that user joe can INSERT or UPDATE tuples of relation test even
> without those column level privileges, as long as he is allowed to
> INSERT or UPDATE the affected non-hidden columns. ]

Right, that's correct. You don't need table-level permissions so long
as you have permissions on the columns you're trying to select/modify.

> Some minor nit-picks: some lines exceed 80 columns, multi-line comments
> don't follow coding standards.

Hrmpf. I'll go back and review the coding standards.. I don't recall
that 80 column was a fixed limit.

> BTW: how are long constant strings expected to be formatted? Are those
> allowed to exceed 80 columns, or are they expected to be split like so
> (i.e. for errmsg):
>
> "Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed "
> "do eiusmod tempor incididunt ut labore et dolore magna aliqua."

Honestly, I think I've seen both done. I can do it either way, of
course.

> Nice work! I'd like to see this shipping with 8.4. The above mentioned
> bugs (the "updated by self" and the hidden columns) should be easy
> enough to fix, I think. Respecting columns level privileges for JOINs is
> probably going to be more work, but is required as well, IMO.

Thanks for the review!

Stephen