[PATCH] DefaultACLs

Hi Petr,

this is first public version of our DefaultACLs patch as described on
http://wiki.postgresql.org/wiki/DefaultACL .

I have been assigned by Robert to do an initial review of your "GRANT
ON ALL" patch mentioned here
(http://archives.postgresql.org/pgsql-hackers/2009-07/msg00207.php)

Does this new DefaultACL patch nullify this earlier one? Or it is
different and should be looked at first since it was added to the
commitfest before the defaultACL patch? It is a bit confusing. Please
clarify.

Regards,
Nikhils

It allows GRANT/REVOKE permissions to be inherited by objects based on
schema permissions at create type by use of ALTER SCHEMA foo SET DEFAULT
PRIVILEGES ON TABLE SELECT TO bar syntax. There is also ADD and DROP for
appending and removing those default privileges. It works for tables, views,
sequences and functions. More info about syntax and some previous discussion
is on wiki.

There is also GRANT DEFAULT PRIVILEGES ON tablename which *replaces* current
object privileges with the default ones. Only owner can do both of those
commands (ALTER SCHEMA can be done only by schema owner and GRANT can be
done only by object owner).

It adds new catalog table which stores the default permissions for given
schema and object type. We didn't add syscache entry for that as Stephen
Frost didn't feel we should do that (yet). Three functions were also
exported from aclchk.c because most of the ALTER SCHEMA stuff is done in
schemacmds.c.

The current version is fully working and includes some regression tests.
There is however no documentation at this moment.
Patch is against current Git HEAD (it is context diff).

--
Regards
Petr Jelinek (PJMODOS)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
http://www.enterprisedb.com

Nikhil Sontakke wrote:

Does this new DefaultACL patch nullify this earlier one? Or it is
different and should be looked at first since it was added to the
commitfest before the defaultACL patch? It is a bit confusing. Please
clarify.

No, DefaultACLs applies to objects created in the future while GRANT ON
ALL affects existing objects.
DefaultACLs is more important functionality so it should probably take
precedence in review process.

There is however one thing that needs some attention. Both patches add
distinction between VIEW and TABLE objects for acls into parser and they
both do it differently. GRANT ON ALL works by adding ACL_OBJECT_VIEW and
tracks that object type in code (that was my original method in both
patches) while DefaultACLs uses method suggested by Stephen Frost which
is creating new enum with relation, view, function and sequence members
(those are object types for which both DefaultACLs and GRANT ON ALL are
applicable). The second method has advantage of minimal changes to
existing code.
It's pointless to use both methods so one of the patches will have to be
adjusted. The problem is that most people seem to dislike the addition
of ACL_OBJECT_VIEW but on the other hand I don't like the idea of adding
another object type variable into GrantStmt struct which would be needed
if we adjusted GRANT ON ALL to Stephen Frost's method.

--
Regards
Petr Jelinek (PJMODOS)

Hi,

No, DefaultACLs applies to objects created in the future while GRANT ON ALL
affects existing objects.

I see.

DefaultACLs is more important functionality so it should probably take
precedence in review process.

There is however one thing that needs some attention. Both patches add
distinction between VIEW and TABLE objects for acls into parser and they
both do it differently. GRANT ON ALL works by adding ACL_OBJECT_VIEW and
tracks that object type in code (that was my original method in both
patches) while DefaultACLs uses method suggested by Stephen Frost which is
creating new enum with relation, view, function and sequence members (those
are object types for which both DefaultACLs and GRANT ON ALL are
applicable). The second method has advantage of minimal changes to existing
code.

I briefly looked at the DefaultACLs patch. Can you not re-use the
GrantStmt structure for the defaults purpose too? You might have to
introduce an "is_default" boolean similar to the "is_schema" boolean
that you have added in the "GRANT ON ALL" patch. If you think you can
re-use the GrantStmt structure, then we might as well stick with the
existing object type code and not add the enums in the DefaultACLs
patch too..

Regards,
Nikhils
--
http://www.enterprisedb.com

Nikhil Sontakke wrote:

There is however one thing that needs some attention. Both patches add
distinction between VIEW and TABLE objects for acls into parser and they
both do it differently. GRANT ON ALL works by adding ACL_OBJECT_VIEW and
tracks that object type in code (that was my original method in both
patches) while DefaultACLs uses method suggested by Stephen Frost which is
creating new enum with relation, view, function and sequence members (those
are object types for which both DefaultACLs and GRANT ON ALL are
applicable). The second method has advantage of minimal changes to existing
code.

I briefly looked at the DefaultACLs patch. Can you not re-use the
GrantStmt structure for the defaults purpose too? You might have to
introduce an "is_default" boolean similar to the "is_schema" boolean
that you have added in the "GRANT ON ALL" patch. If you think you can
re-use the GrantStmt structure, then we might as well stick with the
existing object type code and not add the enums in the DefaultACLs
patch too..

No we can't use the GrantStmt and I wasn't talking about using it. I was
talking about the change in GrantObjectType and differentiating VIEW and
TABLE in some code inside aclchk.c which people didn't like. We can use
the changed GrantObjectType in DefaultACLs and filter inapplicable types
inside C code as I do in GRANT ON ALL patch and it's what I did
originally, but submitted version of DefaultACLs behaves differently.

--
Regards
Petr Jelinek (PJMODOS)

Nikhil,

* Nikhil Sontakke (nikhil.sontakke@enterprisedb.com) wrote:

I briefly looked at the DefaultACLs patch. Can you not re-use the
GrantStmt structure for the defaults purpose too? You might have to
introduce an "is_default" boolean similar to the "is_schema" boolean
that you have added in the "GRANT ON ALL" patch. If you think you can
re-use the GrantStmt structure, then we might as well stick with the
existing object type code and not add the enums in the DefaultACLs
patch too..

Petr and I discussed this. Part of the problem is that the regular
grant enums don't distinguish between TABLE and VIEW because they don't
need to. We need to with DefaultACL because users see those as distinct
types of objects even though we track them in the same catalog.
Splitting up RELATION into TABLE and VIEW in the grant enum would
increase the changes quite a bit in otherwise unrelated paths.
Additionally, not all of the grantable types are applicable for
DefaultACL since DefaultACLs are associated with objects in schemas
(eg: database permissions, schema permissions, etc).

We can certainly do it either way, but I don't see much downside to
having a new enum and a number of downsides with modifying the existing
grant enums.

Thanks,

Stephen

Hi,

I briefly looked at the DefaultACLs patch. Can you not re-use the
GrantStmt structure for the defaults purpose too? You might have to
introduce an "is_default" boolean similar to the "is_schema" boolean
that  you have added in the "GRANT ON ALL" patch. If you think you can
re-use the GrantStmt structure, then we might as well stick with the
existing object type code and not add the enums in the DefaultACLs
patch too..

Petr and I discussed this.  Part of the problem is that the regular
grant enums don't distinguish between TABLE and VIEW because they don't
need to.  We need to with DefaultACL because users see those as distinct
types of objects even though we track them in the same catalog.
Splitting up RELATION into TABLE and VIEW in the grant enum would
increase the changes quite a bit in otherwise unrelated paths.
Additionally, not all of the grantable types are applicable for
DefaultACL since DefaultACLs are associated with objects in schemas
(eg: database permissions, schema permissions, etc).

Ok.

We can certainly do it either way, but I don't see much downside to
having a new enum and a number of downsides with modifying the existing
grant enums.

Sure, I understand. But if we want to go the DefaultACLs way, then we
need to change the "GRANT ON ALL" patch a bit too for the sake of
uniformity - don't we? There is indeed benefit in managing ACLs for
existing objects, so that patch has some value too.

Regards,
Nikhils
--
http://www.enterprisedb.com

Hey,

* Nikhil Sontakke (nikhil.sontakke@enterprisedb.com) wrote:

We can certainly do it either way, but I don't see much downside to
having a new enum and a number of downsides with modifying the existing
grant enums.

Sure, I understand. But if we want to go the DefaultACLs way, then we
need to change the "GRANT ON ALL" patch a bit too for the sake of
uniformity - don't we? There is indeed benefit in managing ACLs for
existing objects, so that patch has some value too.

I agree that they should be consistant. The GRANT ON ALL shares alot
more of the syntax with GRANT than DefaultACL though, which makes it a
more interesting question there. I can understand not wanting to
duplicate the GRANT syntax. I think my suggestion would be to add a
field to the structure passed around by GRANT which indicates if 'VIEW'
was requested or not in the command. This could be used both for GRANT
ON ALL and to allow 'GRANT ON VIEW blah' to verify that the relation
being granted on is a view.

Thanks,

Stephen

Stephen Frost wrote:

I agree that they should be consistant. The GRANT ON ALL shares alot
more of the syntax with GRANT than DefaultACL though, which makes it a
more interesting question there. I can understand not wanting to
duplicate the GRANT syntax. I think my suggestion would be to add a
field to the structure passed around by GRANT which indicates if 'VIEW'
was requested or not in the command. This could be used both for GRANT
ON ALL and to allow 'GRANT ON VIEW blah' to verify that the relation
being granted on is a view.

I arrived into this conclusion too, but it adds a lot of clutter in
gram.y (setting that flag to false or something in many places, just to
use in in one place).

Originally I thought adding ACL_OBJECT_VIEW wasn't such a bad idea. But
after I looked more closely at the code, it it seems to me that having
same object type for VIEW and TABLE seems like the only logical reason
why GRANT uses separate object type enum at all (instead of using subset
of ObjectType like other commands do). If we went this path of
separating VIEW and TABLE in GRANT code it might be cleaner to remove
GrantObjectType and use ObjectType, but I don't think we want to do that.

--
Regards
Petr Jelinek (PJMODOS)

On Tue, Jul 14, 2009 at 11:10:00PM +0200, Petr Jelinek wrote:

Hello,

this is first public version of our DefaultACLs patch as described on
http://wiki.postgresql.org/wiki/DefaultACL .

I've been asked to review this. I can't get it to build, because of a missing
semicolon on line 1608. I fixed it in my version, and it applied cleanly to
head (with some offset hunks in gram.y). I've not yet finished building and
testing; results to follow later.

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

On Tue, Jul 14, 2009 at 11:10:00PM +0200, Petr Jelinek wrote:

Hello,

this is first public version of our DefaultACLs patch as described on
http://wiki.postgresql.org/wiki/DefaultACL .

Ok, here's my first crack at a comprehensive review. There's more I need to
look at, eventually. Some of these are very minor stylistic comments, and some
are probably just because I've much less of a clue, in general, than I'd like
to think I have.

First, as you've already pointed out, this needs documentation.

Once I added the missing semicolon mentioned earlier, it applies and builds
fine. The regression tests, however, seem to assume that they'll be run as the
postgres user, and the privileges test failed. Here's part of a diff between
expected/privileges.out and results/privileges.out as an example of what I
mean:

ALTER SCHEMA regressns DROP DEFAULT PRIVILEGES ON TABLE ALL FROM
regressuser2;
***************
*** 895,903 ****
(1 row)

SELECT relname, relacl FROM pg_class WHERE relname = 'acltest2';
! relname | relacl
! ----------+------------------------------------------------------
! acltest2 | {postgres=arwdDxt/postgres,regressgroup1=r/postgres}
(1 row)

  CREATE FUNCTION regressns.testfunc1() RETURNS int AS 'select 1;' LANGUAGE
sql;
--- 895,903 ----
  (1 row)

SELECT relname, relacl FROM pg_class WHERE relname = 'acltest2';
! relname | relacl
! ----------+------------------------------------------
! acltest2 | {josh=arwdDxt/josh,regressgroup1=r/josh}
(1 row)

CREATE FUNCTION regressns.testfunc1() RETURNS int AS 'select 1;' LANGUAGE
sql;

Very minor stylistic or comment issues:

* There's a stray newline added in pg_class.h (no other changes were made to
that file by this patch)
* It feels to me like the comment "Continue with standard grant" in aclchk.c
interrupts the flow of the code, though such a comment was likely useful
when the patch was being written.
* pg_namespace_default_acl.h:71 should read "objects stored *in* pg_class"
* The comment at the beginning of InsertPgClassTuple() in catalog/heap.c
should probably be updated to say that relation's ACLs aren't always NULL by
default
* copy_from in gram.y got changed to to_from, but to_from isn't ever used in
the default ACL grammar. I wondered if this was changed so you could use the
same TO/FROM code as COPY uses, and then you decided to hardcode TO and FROM?

In my perusal of the patch, I didn't see any code that screamed at me as
though it were a bad idea; quite likely there weren't any really bad ideas but
I can't say with confidence I'd have spotted them if there were. The addition
of both the NSPDEFACLOBJ_* defines alongside the NSP_ACL_OBJ_* defines kinda
made me think there were too many sets of constants that had to be kept in
sync, but I'm not sure that's much of an issue in reality, given how unlikely
it is that schema object types to which default ACLs should apply are likely
to be added or removed.

I don't know how patches that require catalog version changes are generally
handled; should the patch include that change?

More testing to follow.

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

Joshua Tolley wrote:

I don't know how patches that require catalog version changes are generally
handled; should the patch include that change?

The committer should handle that.

cheers

andrew

Joshua Tolley wrote:

First, as you've already pointed out, this needs documentation.

/me looks at Stephen ;)

Once I added the missing semicolon mentioned earlier, it applies and builds

As we discussed outside of list, my compiler didn't picked that one
because I didn't use --enable-cassert .

fine. The regression tests, however, seem to assume that they'll be run as the
postgres user, and the privileges test failed.

Oh I thought they are always run under *database user* postgres, my bad,
I reworked them and the are probably better as a result.

Very minor stylistic or comment issues:

* There's a stray newline added in pg_class.h (no other changes were made to
that file by this patch)

Fixed, probably I either pressed enter by mistake while viewing that
file or it was some merging problem when updating my working copy.

* It feels to me like the comment "Continue with standard grant" in aclchk.c
interrupts the flow of the code, though such a comment was likely useful
when the patch was being written.

Ok I removed that comment.

* pg_namespace_default_acl.h:71 should read "objects stored *in* pg_class"

Fixed.

* The comment at the beginning of InsertPgClassTuple() in catalog/heap.c
should probably be updated to say that relation's ACLs aren't always NULL by
default

Done.

* copy_from in gram.y got changed to to_from, but to_from isn't ever used in
the default ACL grammar. I wondered if this was changed so you could use the
same TO/FROM code as COPY uses, and then you decided to hardcode TO and FROM?

Yes, that's more or less what happened.

In my perusal of the patch, I didn't see any code that screamed at me as
though it were a bad idea; quite likely there weren't any really bad ideas but
I can't say with confidence I'd have spotted them if there were. The addition
of both the NSPDEFACLOBJ_* defines alongside the NSP_ACL_OBJ_* defines kinda
made me think there were too many sets of constants that had to be kept in
sync, but I'm not sure that's much of an issue in reality, given how unlikely
it is that schema object types to which default ACLs should apply are likely
to be added or removed.

Well the problem you see is not really a problem IMHO because most of
code I've seen does not use actual catalog values inside gram.y/parser
so I didn't use them either.

But there is one problem there that also affects GRANT ON ALL patch and
that was discussed previously (see
http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php and
the rest of the thread from there). One (or both) of those patches will
have to be adjusted but only commiter can decide that IMHO (I am happy
to make any necessary changes but I really don't know which of the 3
solutions is better).

I don't know how patches that require catalog version changes are generally
handled; should the patch include that change?

Than can reasonably be done only at commit time so it's up to commiter.

I attached updated version of the patch per your comments. Let's hope I
didn't introduce new problems :)

Thanks for your time.

--
Regards
Petr Jelinek (PJMODOS)

Hello,

while writing some basic docs I found bug in dependency handling when
doing SET on object type that already had some default privileges.
Attached patch fixes it, it also fixes thinko in parser (DROPing GRANT
OPTION behaves like REVOKE now). And there is also initial version of
those basic docs included (but you have to pardon my english as I didn't
pass it to Stephen for proofreading due to discovery of that bug).

--
Regards
Petr Jelinek (PJMODOS)

On Fri, Jul 17, 2009 at 9:46 PM, Joshua Tolley<eggyknap@gmail.com> wrote:

On Tue, Jul 14, 2009 at 11:10:00PM +0200, Petr Jelinek wrote:

Hello,

this is first public version of our DefaultACLs patch as described on
http://wiki.postgresql.org/wiki/DefaultACL .

Ok, here's my first crack at a comprehensive review. There's more I need to
look at, eventually. Some of these are very minor stylistic comments, and some
are probably just because I've much less of a clue, in general, than I'd like
to think I have.

First, as you've already pointed out, this needs documentation.

Once I added the missing semicolon mentioned earlier, it applies and builds
fine. The regression tests, however, seem to assume that they'll be run as the
postgres user, and the privileges test failed. Here's part of a diff between
expected/privileges.out and results/privileges.out as an example of what I
mean:

 ALTER SCHEMA regressns DROP DEFAULT PRIVILEGES ON TABLE ALL FROM
regressuser2;
***************
*** 895,903 ****
 (1 row)

 SELECT relname, relacl FROM pg_class WHERE relname = 'acltest2';
!  relname  |                        relacl
! ----------+------------------------------------------------------
!  acltest2 | {postgres=arwdDxt/postgres,regressgroup1=r/postgres}
 (1 row)

 CREATE FUNCTION regressns.testfunc1() RETURNS int AS 'select 1;' LANGUAGE
sql;
--- 895,903 ----
 (1 row)

 SELECT relname, relacl FROM pg_class WHERE relname = 'acltest2';
!  relname  |                  relacl
! ----------+------------------------------------------
!  acltest2 | {josh=arwdDxt/josh,regressgroup1=r/josh}
 (1 row)

 CREATE FUNCTION regressns.testfunc1() RETURNS int AS 'select 1;' LANGUAGE
sql;

Very minor stylistic or comment issues:

* There's a stray newline added in pg_class.h (no other changes were made to
 that file by this patch)
* It feels to me like the comment "Continue with standard grant" in aclchk.c
 interrupts the flow of the code, though such a comment was likely useful
 when the patch was being written.
* pg_namespace_default_acl.h:71 should read "objects stored *in* pg_class"
* The comment at the beginning of InsertPgClassTuple() in catalog/heap.c
 should probably be updated to say that relation's ACLs aren't always NULL by
 default
* copy_from in gram.y got changed to to_from, but to_from isn't ever used in
 the default ACL grammar. I wondered if this was changed so you could use the
 same TO/FROM code as COPY uses, and then you decided to hardcode TO and FROM?

In my perusal of the patch, I didn't see any code that screamed at me as
though it were a bad idea; quite likely there weren't any really bad ideas but
I can't say with confidence I'd have spotted them if there were. The addition
of both the NSPDEFACLOBJ_* defines alongside the NSP_ACL_OBJ_* defines kinda
made me think there were too many sets of constants that had to be kept in
sync, but I'm not sure that's much of an issue in reality, given how unlikely
it is that schema object types to which default ACLs should apply are likely
to be added or removed.

I don't know how patches that require catalog version changes are generally
handled; should the patch include that change?

More testing to follow.

So are these warts fixed in the latest revision of this patch?

http://archives.postgresql.org/pgsql-hackers/2009-07/msg01216.php

I am gathering that this patch is still a bit of a WIP. I think it
might be best to mark it returned with feedback and let Petr resubmit
for the next CommitFest when it is closer to being done. But I don't
want to do that if it's really already to go now.

I am also a bit unsure as to whether Josh Tolley is still conducting a
more in-depth review. Josh?

Thanks,

...Robert

On Wed, Jul 22, 2009 at 08:54:19PM -0400, Robert Haas wrote:

I am gathering that this patch is still a bit of a WIP.

I don't consider it a WIP. Petr posted a patch a couple of days ago, but I've
not been able to verify its changes or perform some additional testing I had
in mind, because of my own schedule. I probably should have made that clear a
while ago. I consider the ball entirely in my court on this one, and plan to
complete my review using the latest version of the patch as soon as my time
permits; I expect this will happen before Saturday.

I am also a bit unsure as to whether Josh Tolley is still conducting a
more in-depth review. Josh?

Yes, I am, but if you've read this far you know that already :)

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

Robert Haas wrote:

So are these warts fixed in the latest revision of this patch?

http://archives.postgresql.org/pgsql-hackers/2009-07/msg01216.php

I am gathering that this patch is still a bit of a WIP. I think it
might be best to mark it returned with feedback and let Petr resubmit
for the next CommitFest when it is closer to being done. But I don't
want to do that if it's really already to go now.

See http://archives.postgresql.org/pgsql-hackers/2009-07/msg01151.php
(the revision before the one you pasted).
The docs are not complete but that's up to Stephen, otherwise the patch
should be finished. But I am not the reviewer :)

Anyway, while this patch might not necessary get commited in this commit
fest, I'd still like to have opinion from one of the commiters on "the
VIEW problem" which also affects grant on all patch ( see
http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php ) and
I fear "returned with feedback" might prevent that until next commit fest.

--
Regards
Petr Jelinek (PJMODOS)

On Wed, Jul 22, 2009 at 11:21 PM, Joshua Tolley<eggyknap@gmail.com> wrote:

On Wed, Jul 22, 2009 at 08:54:19PM -0400, Robert Haas wrote:

I am gathering that this patch is still a bit of a WIP.

I don't consider it a WIP. Petr posted a patch a couple of days ago, but I've
not been able to verify its changes or perform some additional testing I had
in mind, because of my own schedule. I probably should have made that clear a
while ago. I consider the ball entirely in my court on this one, and plan to
complete my review using the latest version of the patch as soon as my time
permits; I expect this will happen before Saturday.

OK, I stand corrected. Thanks for the update.

...Robert

On Wed, Jul 22, 2009 at 11:26 PM, Petr Jelinek<pjmodos@pjmodos.net> wrote:

Robert Haas wrote:

So are these warts fixed in the latest revision of this patch?

http://archives.postgresql.org/pgsql-hackers/2009-07/msg01216.php

I am gathering that this patch is still a bit of a WIP.  I think it
might be best to mark it returned with feedback and let Petr resubmit
for the next CommitFest when it is closer to being done.  But I don't
want to do that if it's really already to go now.

See http://archives.postgresql.org/pgsql-hackers/2009-07/msg01151.php (the
revision before the one you pasted).

Ah, woops. Sorry I missed that. Actually, now that I read it, I
remember that I saw it before, but I didn't remember that before
sending my previous email, of course...

The docs are not complete but that's up to Stephen, otherwise the patch
should be finished. But I am not the reviewer :)

Well, perhaps we had better prod Stephen then, since complete docs are
a requirement for commit.

Anyway, while this patch might not necessary get commited in this commit
fest, I'd still like to have opinion from one of the commiters on "the VIEW
problem" which also affects grant on all patch ( see
http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php ) and I
fear "returned with feedback" might prevent that until next commit fest.

OK, hopefully one of them will chime in with an opinion. As I say, I
would like to see this get committed if it's done, I just wasn't sure
it was. But now it seems that was due to insufficiently careful
reading of the thread, with the exception of this issue and the need
to finish the docs.

Thanks,

...Robert

* Robert Haas (robertmhaas@gmail.com) wrote:

On Wed, Jul 22, 2009 at 11:26 PM, Petr Jelinek<pjmodos@pjmodos.net> wrote:

The docs are not complete but that's up to Stephen, otherwise the patch
should be finished. But I am not the reviewer :)

Well, perhaps we had better prod Stephen then, since complete docs are
a requirement for commit.

I didn't think the docs posted were terrible, but I am working on
improving them.

Anyway, while this patch might not necessary get commited in this commit
fest, I'd still like to have opinion from one of the commiters on "the VIEW
problem" which also affects grant on all patch ( see
http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php ) and I
fear "returned with feedback" might prevent that until next commit fest.

OK, hopefully one of them will chime in with an opinion. As I say, I
would like to see this get committed if it's done, I just wasn't sure
it was. But now it seems that was due to insufficiently careful
reading of the thread, with the exception of this issue and the need
to finish the docs.

Working on it... Not that we've never committed stuff with less than
complete docs before.. ;)

Thanks,

Stephen