Re: [PATCH] DefaultACLs

Hello,

this is first public version of our DefaultACLs patch as described on
http://wiki.postgresql.org/wiki/DefaultACL .
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)

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(at)postgresql(dot)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(dot)sontakke(at)enterprisedb(dot)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(dot)sontakke(at)enterprisedb(dot)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(at)gmail(dot)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(at)gmail(dot)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(at)pjmodos(dot)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(at)gmail(dot)com) wrote:
> On Wed, Jul 22, 2009 at 11:26 PM, Petr Jelinek<pjmodos(at)pjmodos(dot)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

Hi,

>
>> 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.
>

IMO, the committers should have a go at the "GRANT ON ALL" (simpler
than DefaultACLs) patch first. Whatever consensus is arrived for the
VIEW issue as part of its review can then spill over to the
DefaultACLs patch too.

As mentioned by Petr, he does not seem to be in a hurry to get the
DefaultACLs patch in, in this commitfest..

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

On Thursday 23 July 2009 06:26:05 Petr Jelinek wrote:
> 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.

I see potential for confusion in that GRANT ON TABLE x works if x is a base
table or a view, but GRANT ON ALL TABLES would not affect views. Maybe you
need to make up a different syntax to affect only base tables, e.g., GRANT ON
ALL BASE TABLES.

Peter Eisentraut wrote:
> On Thursday 23 July 2009 06:26:05 Petr Jelinek wrote:
>
>> 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.
>>
>
> I see potential for confusion in that GRANT ON TABLE x works if x is a base
> table or a view, but GRANT ON ALL TABLES would not affect views. Maybe you
> need to make up a different syntax to affect only base tables, e.g., GRANT ON
> ALL BASE TABLES.
>

That's not what I mean the problem is what is the best way of handling
the views in implementation itself (there were IIRC 3 possible solutions
devised and I don't think we have consensus on which is better).
In short,
1. add ACL_OBJECT_VIEW into GrantObjectType enum and track that inside code
2. create new enum with table, view, function and sequence objects in it
(that works well for DefaultACLs but not for GRANT ON ALL)
3. add some boolean into GrantStmt that would indicate that relation is
a view (that works for GRANT ON ALL but does not solve anything for
DefaultACLs)

Currently DefaultACLs patch uses method 2 (because Stephen does not like
method 1) and GRANT ON ALL patch uses method 1 and it might be better if
both patches uses only one of those.
If we went with method 1 we probably should just ditch GrantObjectType
alltogether and work with subset of ObjectType as other commands do (I
haven't found any reason for GrantObjectType to exist other than having
single object type for both TABLE and VIEW).
And If we choose not to use method 1 then we should probably go with 2
for DefaultACLs and 3 for GRANT ON ALL. That is unless somebody has a
better solution.

--
Regards
Petr Jelinek (PJMODOS)

Hi,

> 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.
>
>
> I see potential for confusion in that GRANT ON TABLE x works if x is a base
> table or a view, but GRANT ON ALL TABLES would not affect views. Maybe you
> need to make up a different syntax to affect only base tables, e.g., GRANT
> ON
> ALL BASE TABLES.
>
>
> That's not what I mean the problem is what is the best way of handling the
> views in implementation itself (there were IIRC 3 possible solutions devised
> and I don't think we have consensus on which is better).

Peter is raising a good question here and it's not related to the
implementation. What he is saying is that in your new implementation
if "GRANT ON ALL TABLES" is invoked, it will affect only RELKIND_TABLE
objects. Whereas the GRANT ON TABLE affects both RELKIND_TABLE and
RELKIND_VIEW types of objects (with and without your patch).

We could have brought in the differentiation with this patch to treat
views and tables separately. So a GRANT ON TABLE would just affect
tables. But I guess that will break existing user scripts which assume
it works against VIEWS too.

I don't know how acceptable the "ON ALL BASE TABLES" sounds to all.

Regards,
Nikhils

> In short,
> 1. add ACL_OBJECT_VIEW into GrantObjectType enum and track that inside code
> 2. create new enum with table, view, function and sequence objects in it
> (that works well for DefaultACLs but not for GRANT ON ALL)
> 3. add some boolean into GrantStmt that would indicate that relation is a
> view (that works for GRANT ON ALL but does not solve anything for
> DefaultACLs)
>
> Currently DefaultACLs patch uses method 2 (because Stephen does not like
> method 1) and GRANT ON ALL patch uses method 1 and it might be better if
> both patches uses only one of those.
> If we went with method 1 we probably should just ditch GrantObjectType
> alltogether and work with subset of ObjectType as other commands do (I
> haven't found any reason for GrantObjectType to exist other than having
> single object type for both TABLE and VIEW).
> And If we choose not to use method 1 then we should probably go with 2 for
> DefaultACLs and 3 for GRANT ON ALL. That is unless somebody has a better
> solution.
>
> --
> Regards
> Petr Jelinek (PJMODOS)

--
http://www.enterprisedb.com

On Sun, Jul 19, 2009 at 06:13:32PM +0200, Petr Jelinek wrote:
> 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).

Am I the only one that gets this on make check, with this version (from
src/test/regress/log/initdb.log):

selecting default shared_buffers ... 32MB
creating configuration files ... ok
creating template1 database in /home/josh/devel/pgsrc/pg85/src/test/regress/./tmp_check/data/base/1 ... FATAL: relation "pg_namespace_default_acl" already exists
child process exited with exit code 1
initdb: data directory "/home/josh/devel/pgsrc/pg85/src/test/regress/./tmp_check/data" not removed at user's request

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

Joshua Tolley wrote:
> Am I the only one that gets this on make check, with this version (from
> src/test/regress/log/initdb.log):
>
> selecting default shared_buffers ... 32MB
> creating configuration files ... ok
> creating template1 database in /home/josh/devel/pgsrc/pg85/src/test/regress/./tmp_check/data/base/1 ... FATAL: relation "pg_namespace_default_acl" already exists
> child process exited with exit code 1
> initdb: data directory "/home/josh/devel/pgsrc/pg85/src/test/regress/./tmp_check/data" not removed at user's request
>
Certainly never happened to me. Are you using any special parameters or
something (altho I don't have the slightest idea what could cause that)
? I run make check on patches using gcc under debian and msvc on vista
before sending them.

--
Regards
Petr Jelinek (PJMODOS)

On Sat, Jul 25, 2009 at 11:14:19AM +0200, Petr Jelinek wrote:
> Joshua Tolley wrote:
>> Am I the only one that gets this on make check, with this version (from
>> src/test/regress/log/initdb.log):
>>
>> selecting default shared_buffers ... 32MB
>> creating configuration files ... ok
>> creating template1 database in /home/josh/devel/pgsrc/pg85/src/test/regress/./tmp_check/data/base/1 ... FATAL: relation "pg_namespace_default_acl" already exists
>> child process exited with exit code 1
>> initdb: data directory "/home/josh/devel/pgsrc/pg85/src/test/regress/./tmp_check/data" not removed at user's request
>>
> Certainly never happened to me. Are you using any special parameters or
> something (altho I don't have the slightest idea what could cause that)
> ? I run make check on patches using gcc under debian and msvc on vista
> before sending them.

I figured as much. I can't seem to get past this, despite a make distclean.
Suggestions, anyone?

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

Joshua Tolley wrote:
> I figured as much. I can't seem to get past this, despite a make distclean.
> Suggestions, anyone?
>
>
>

try a fresh checkout and reapply the patch?

cheers

andrew

On Sat, Jul 25, 2009 at 03:50:06PM -0400, Andrew Dunstan wrote:
> Joshua Tolley wrote:
>> I figured as much. I can't seem to get past this, despite a make distclean.
>> Suggestions, anyone?
>>
> try a fresh checkout and reapply the patch?

[ a couple git clean, git reset, make clean, etc. commands later... ]

Yeah, well, it works now. What's more, the problems I had with make check the
first time I tried this are no longer.

I've done all the looking I can think to do at the patch in its existing form,
and don't have any complaints. I realize, though, that there are open
questions about how this should work with, given the GRANT ON ALL patch. I'm
not sure I have comments in that regard, but I'll try to come up with some, or
at least to convince myself I don't have any for a better reason than just
that I wouldn't know what I was talking about. In the meantime, I think this
one is ready to be marked as ... something else. Ready for committer?

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

On Sat, Jul 25, 2009 at 7:45 PM, Joshua Tolley<eggyknap(at)gmail(dot)com> wrote:
> that I wouldn't know what I was talking about. In the meantime, I think this
> one is ready to be marked as ... something else. Ready for committer?

Sounds right to me.

...Robert

On Sun, Jul 19, 2009 at 06:13:32PM +0200, Petr Jelinek wrote:
> 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).

Immediately after concluding I was done with my review, I realized I'd
completely forgotten to look at the docs. I've made a few changes based solely
on my opinions of what sounds better and what's more consistent with the
existing documentation. Do with them as you see fit. :)

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

On Sat, Jul 25, 2009 at 8:39 PM, Joshua Tolley<eggyknap(at)gmail(dot)com> wrote:
> On Sun, Jul 19, 2009 at 06:13:32PM +0200, Petr Jelinek wrote:
>> 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).
>
> Immediately after concluding I was done with my review, I realized I'd
> completely forgotten to look at the docs. I've made a few changes based solely
> on my opinions of what sounds better and what's more consistent with the
> existing documentation. Do with them as you see fit. :)

Did you intend to attach something to this email?

...Robert

On Sat, Jul 25, 2009 at 08:41:12PM -0400, Robert Haas wrote:
> On Sat, Jul 25, 2009 at 8:39 PM, Joshua Tolley<eggyknap(at)gmail(dot)com> wrote:
> > On Sun, Jul 19, 2009 at 06:13:32PM +0200, Petr Jelinek wrote:
> >> 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).
> >
> > Immediately after concluding I was done with my review, I realized I'd
> > completely forgotten to look at the docs. I've made a few changes based solely
> > on my opinions of what sounds better and what's more consistent with the
> > existing documentation. Do with them as you see fit. :)
>
> Did you intend to attach something to this email?
>
> ...Robert

Well, yes, now that you mention it :) Trying again...

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

Joshua Tolley weote:
> On Sat, Jul 25, 2009 at 08:41:12PM -0400, Robert Haas wrote:
>
>> On Sat, Jul 25, 2009 at 8:39 PM, Joshua Tolley<eggyknap(at)gmail(dot)com> wrote:
>>
>>> Immediately after concluding I was done with my review, I realized I'd
>>> completely forgotten to look at the docs. I've made a few changes based solely
>>> on my opinions of what sounds better and what's more consistent with the
>>> existing documentation. Do with them as you see fit. :)
>>>
Applied with minor adjustments, attached updated patch.

--
Regards
Petr Jelinek (PJMODOS)

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > On Wed, Jul 22, 2009 at 11:26 PM, Petr Jelinek<pjmodos(at)pjmodos(dot)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.

Thanks to Joshua, there weren't really many changes I found for the
docs. Here they are anyway:

grant.sgml:

+ Replace current privileges with the ones specified using
+ <xref linkend="sql-alterschema" endterm="sql-alterschema-title">.
+ The <literal>WITH GRANT OPTION</literal> parameter is not applicable
+ because it is copied from default privileges.
+ Note: this can actually <emphasis role="bold">revoke</emphasis> some
+ privileges because it clears all existing privileges object has and
+ replaces them with the default ones for the schema in which this object
+ resides.

How about:

Replaces current privileges with the default privileges, as set using
<xref linkend="sql-alterschema" endterm="sql-alterschema-title">, for
this object type in its current schema.
The <literal>WITH GRANT OPTION</literal> parameter is not applicable
because it is copied from default privileges.
Note: This can <emphasis role="bold">revoke</emphasis> privileges! This
will clear all existing privileges first! If no default privilege has
been set, the object will have all privileges revoked!

Otherwise, I thought the docs looked pretty good.

Thanks,

Stephen

On Tue, Aug 04, 2009 at 01:28:00PM -0400, Stephen Frost wrote:
> Thanks to Joshua, there weren't really many changes I found for the
> docs. Here they are anyway:

Yay, I was useful! :)

> How about:
>
> Replaces current privileges with the default privileges, as set using
> <xref linkend="sql-alterschema" endterm="sql-alterschema-title">, for
> this object type in its current schema.
> The <literal>WITH GRANT OPTION</literal> parameter is not applicable
> because it is copied from default privileges.
> Note: This can <emphasis role="bold">revoke</emphasis> privileges! This
> will clear all existing privileges first! If no default privilege has
> been set, the object will have all privileges revoked!
>
> Otherwise, I thought the docs looked pretty good.

No complaints here, FWIW.

- Josh

I looked at this patch a bit. I haven't actually read any of the code
yet, just reviewed the on-list discussions and the docs, but I think I can
make a few comments at the definitional level.

First: there's already been some discussion about the VIEW problem.
I understand that the patch adds a "GRANT ON VIEW" syntax that works
like "GRANT ON SEQUENCE" in that if you say VIEW it insists the object
really is a view, but without taking away the existing laxness that
allows you to still say GRANT ON TABLE for a view. This seems reasonable
in isolation, but it creates a big problem for both this patch and the
GRANT ON ALL patch, in that it's unclear how to treat views if they
could be considered either tables or views. Unfortunately I think
we have no choice but to live with the "existing laxness", because
(a) to do otherwise will break pretty nearly every pg_dump script, and
(b) the SQL standard says we have to accept "GRANT ON TABLE view".
In fact "GRANT ON VIEW" is not in the SQL standard and there is no good
reason to expect applications will adopt it at all.

So my feeling is that adding GRANT ON VIEW is a bad idea. The main
argument for doing it seemed to be that the author wanted to be able
to grant different default privileges for tables and views, but I'm
unconvinced that there's a strong use-case for that. You could very
easily have one set of default privileges for all of tables, views,
and sequences, simply ignoring any bits that weren't relevant for
particular objects. It seems to me that that would handle common
cases just fine. For complicated cases not so much, but it's not
clear to me that filtering by the object subtype is all that useful
for complicated cases anyway. People will want more functionality
than that. Which leads into my next item.

Second: both this patch and GRANT ON ALL are built on the assumption
that the only way to filter/classify objects is by schema membership.
Now I don't object to that as an initial implementation restriction,
but I don't like hard-wiring it into the syntax. It is very clear to me
that we'll want other filter rules in the future --- an immediate example
is being able to say that new children of an inheritance parent table
should inherit its GRANTs. So to my mind, designing the syntax around
ALTER SCHEMA is right out. Maybe we could do something like
ALTER DEFAULT PRIVILEGES ON TABLES IN SCHEMA foo GRANT ...
where the "IN SCHEMA foo" part would be subject to generalization later.
This also matches up a bit better with the proposed syntax for GRANT
ON ALL (which also uses "IN SCHEMA foo").

Third: speaking of syntax, I don't like the way that this is gratituously
different from GRANT/REVOKE. I don't like using ADD/DROP instead of
GRANT/REVOKE, nor the unnecessary AND business. I think we should
minimize confusion by using something that is spelled as nearly like
GRANT/REVOKE as possible.

Fourth: the system's existing treatment of default permissions is
owner-dependent, that is the implied set of permissions is typically
GRANT ALL TO <owner> (from himself, with grant option). I do not
understand how schema-level default ACLs will play nicely with that,
except in the special case where the schema owner also owns every
contained object. If you copy the schema-level ACL directly to a
contained object with a different owner it will definitely be the wrong
thing, but if you try to translate the ownership it will confuse people
too. And confusion in a security-related behavior is a Bad Thing.
Furthermore, having the schema owner able to control the grants made
for objects not owned by him is a huge security hole.

What I suggest as a way to resolve this last point is that a default ACL
should apply only to objects owned by the user who creates/modifies the
default ACL. In this view, the question of which schema the objects are
in is just an additional filter condition, not the primary determinant
of which objects a default ACL applies to. Every user has his own set
of default ACLs. (This is another reason not to design the syntax
around ALTER SCHEMA.)

regards, tom lane

Tom Lane wrote:
> So my feeling is that adding GRANT ON VIEW is a bad idea. The main
> argument for doing it seemed to be that the author wanted to be able
> to grant different default privileges for tables and views, but I'm
> unconvinced that there's a strong use-case for that. You could very
>
Yes that was the intention. I do have users in my databases with access
privileges to VIEWs but not to underlying TABLEs so it seemed like a
good idea to be able to do that with DefaultACLs and GRANT ON ALL.

> Second: both this patch and GRANT ON ALL are built on the assumption
> that the only way to filter/classify objects is by schema membership.
> Now I don't object to that as an initial implementation restriction,
> but I don't like hard-wiring it into the syntax. It is very clear to me
> that we'll want other filter rules in the future --- an immediate example
> is being able to say that new children of an inheritance parent table
> should inherit its GRANTs. So to my mind, designing the syntax around
> ALTER SCHEMA is right out. Maybe we could do something like
> ALTER DEFAULT PRIVILEGES ON TABLES IN SCHEMA foo GRANT ...
> where the "IN SCHEMA foo" part would be subject to generalization later.
> This also matches up a bit better with the proposed syntax for GRANT
> ON ALL (which also uses "IN SCHEMA foo").
>
Actually I was planning to extend GRANT ON ALL - if it was accepted - to
include more filters (something like OWNED BY for example).

> Third: speaking of syntax, I don't like the way that this is gratituously
> different from GRANT/REVOKE. I don't like using ADD/DROP instead of
> GRANT/REVOKE, nor the unnecessary AND business. I think we should
> minimize confusion by using something that is spelled as nearly like
> GRANT/REVOKE as possible.
>
ADD/DROP was side product of having SET and that "unnecessary AND business".
If we went with that syntax you proposed we could just put exact same
syntax as GRANT and REVOKE after the filtering option, that should be
close enough :). I remember Stephen being against having GRANT in ALTER
SCHEMA but I doubt he would be against having it in completely new ALTER
DEFAULT PRIVILEGES statement.

> Fourth: the system's existing treatment of default permissions is
> owner-dependent, that is the implied set of permissions is typically
> GRANT ALL TO <owner> (from himself, with grant option). I do not
> understand how schema-level default ACLs will play nicely with that,
> except in the special case where the schema owner also owns every
> contained object. If you copy the schema-level ACL directly to a
> contained object with a different owner it will definitely be the wrong
> thing, but if you try to translate the ownership it will confuse people
> too. And confusion in a security-related behavior is a Bad Thing.
> Furthermore, having the schema owner able to control the grants made
> for objects not owned by him is a huge security hole.
>
We were actually discussing this with Stephen yesterday as something
similar occurred to me too. The patch as submitted just does the copy,
after the discussion I added post-processing on object creation which
translates everything to owner but I guess there is no point in
submitting that now.

> What I suggest as a way to resolve this last point is that a default ACL
> should apply only to objects owned by the user who creates/modifies the
> default ACL. In this view, the question of which schema the objects are
> in is just an additional filter condition, not the primary determinant
> of which objects a default ACL applies to. Every user has his own set
> of default ACLs.
>
We could certainly do that. I wonder what we should do about inheritance
of default privileges between the roles if we did this - should it just
be what I set is mine and my parent roles do not affect me or should it
get default privs from parent roles and merge them with mine when I
create the object ? Also when creating new default privileges entry
should we use some template which would give owner all privileges like
GRANT does when there are no existing privileges on object or should we
just use blank and leave it to user to grant himself default privileges
on objects he will create ?

I don't think there is any point in you looking at code since
DefaultACLs might need serious rewriting.

GRANT ON ALL is a bit different story, there I can just remove all that
VIEW stuff, although I would very much like to have the ability to
affect only VIEWs. On the other hand removing VIEW as separate object
type would remove my biggest problem with how both patches are
implemented (I mentioned it few times in the previous discussion) so
from that standpoint it might be a good thing.

--
Regards
Petr Jelinek (PJMODOS)

Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
> Tom Lane wrote:
>> What I suggest as a way to resolve this last point is that a default ACL
>> should apply only to objects owned by the user who creates/modifies the
>> default ACL. In this view, the question of which schema the objects are
>> in is just an additional filter condition, not the primary determinant
>> of which objects a default ACL applies to. Every user has his own set
>> of default ACLs.
>>
> We could certainly do that. I wonder what we should do about inheritance
> of default privileges between the roles if we did this - should it just
> be what I set is mine and my parent roles do not affect me or should it
> get default privs from parent roles and merge them with mine when I
> create the object ?

I don't believe there is any "inheritance" needed or involved. A
default ACL would only be looked up for use at the instant of creating
an object, and what you'd look for is one owned by the same userID that
is going to own the object being created. Anything else will be too
complicated to be understandable. The commands that actually
create/alter a default ACL would work on those belonging to whatever
the effective userID is.

> Also when creating new default privileges entry
> should we use some template which would give owner all privileges like
> GRANT does when there are no existing privileges on object or should we
> just use blank and leave it to user to grant himself default privileges
> on objects he will create ?

It should start from the same initial state you'd have if you didn't
have a default ACL. Anything else violates the principle of least
astonishment.

regards, tom lane

I had some time to work on this patch, and I implemented the ALTER
DEFAULT PRIVILEGES syntax as proposed by Tom and adjusted some other
stuff, but before I can submit the new patch for commitfest there is
still this fundamental issue about how it should behave.

The situation is as following. Josh's and Stephen's idea was basically
to solve something like this: you are a dba, you give some users
privileges to create tables and you want those new tables to have same
privileges no matter who created them.
But if I understood Tom's suggestions correctly then his approach does
not solve this at all since every one of those users with CREATE TABLE
privileges would have to also set same DEFAULT PRIVILEGES and the dba
would have no say in the matter.

I personally can see use cases for both but I don't really see any
reasonable way to have both at the same time.

--
Regards
Petr Jelinek (PJMODOS)

Petr,

> But if I understood Tom's suggestions correctly then his approach does
> not solve this at all since every one of those users with CREATE TABLE
> privileges would have to also set same DEFAULT PRIVILEGES and the dba
> would have no say in the matter.

This latter approach benefits nobody. If default's can't be set by the
DBA centrally, the feature is useless.

--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com

Josh Berkus wrote:
>> But if I understood Tom's suggestions correctly then his approach does
>> not solve this at all since every one of those users with CREATE TABLE
>> privileges would have to also set same DEFAULT PRIVILEGES and the dba
>> would have no say in the matter.
>>
>
> This latter approach benefits nobody. If default's can't be set by the
> DBA centrally, the feature is useless.
>
I agree, however I assume I understood Tom properly since he didn't reply.

So I've been working on solution with which I am happy with (does not
mean anybody else will be also though).
I created a new version with syntax devised by Tom and one which is user
centric, but also one with which DBA can control default privileges.
The attached patch adds syntax in this format:
ALTER DEFAULT PRIVILEGES [ IN SCHEMA schema_name(s) ] [ FOR ROLE
role_name(s) ] GRANT privileges ON object_type TO role(s);
Obviously it sets default privileges for new objects of given object
type created by role(s) specified using FOR ROLE (is that syntax ok?)
clause and inside specified schema(s).
If user omits IN SCHEMA it applies database wide. Database wide settings
are used only if there is nothing specified for current schema (ie no
cascading/inheritance).
If FOR ROLE is omitted then the privileges are set for current role.
Only superusers and users with ADMIN privilege (we might want to add
specific privilege for this but ADMIN seems suitable to me) granted on
the role can use FOR ROLE clause.
The order of FOR ROLE and IN SCHEMA clauses does not matter.

Some of my thoughts on the changed behavior of the patch:
There is no need to be schema owner anymore in this implementation since
the privileges are handled quite differently.
There are no longer issues about who should be grantor (discussed on IRC
only, there was problem that schema owner as grantor didn't seem logical
and we didn't know owner at the time we created default privileges).
Also there is no longer a problem with what should be template for
privileges because we now know the owner of the object at default
privileges creation time (which we didn't before as it was all schema
based) so we can use standard template as used by GRANT.
The whole thing is more consistent with GRANT.
The patch is also a bit smaller :)
It's not as easy to do the original idea of setting default privileges
for schema for all users with CREATE privilege on schema but it can
still be done, one just have to update default privileges every time
somebody is granted that privilege, and DBA can still have control over
it all.

Hopefully this will at least inspire some more discussion on the matter.

--
Regards
Petr Jelinek (PJMODOS)

Petr,

> It's not as easy to do the original idea of setting default privileges
> for schema for all users with CREATE privilege on schema but it can
> still be done, one just have to update default privileges every time
> somebody is granted that privilege, and DBA can still have control over
> it all.

Sounds like a good solution. Thanks for persisting with this.

--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com

Petr Jelinek wrote:
> So I've been working on solution with which I am happy with (does not
> mean anybody else will be also though).

Hi Petr,

I'm reviewing this patch and after reading it I have some comments.
Unfortunately, when I got to the compiling part, it turned out that the
attached patch is missing src/include/catalog/pg_default_acls.h (or is
it just me?).

I'll continue reviewing while waiting for a new patch with that header
file and in any case will send the full review tommorrow.

Cheers,
Jan

Jan Urbański napsal(a):
> Petr Jelinek wrote:
>
>> So I've been working on solution with which I am happy with (does not
>> mean anybody else will be also though).
>>
>
> Hi Petr,
>
> I'm reviewing this patch and after reading it I have some comments.
> Unfortunately, when I got to the compiling part, it turned out that the
> attached patch is missing src/include/catalog/pg_default_acls.h (or is
> it just me?).
>
> I'll continue reviewing while waiting for a new patch with that header
> file and in any case will send the full review tommorrow.
>
Hmm looks like I forgot to git add that file in the new branch, sorry
about that.
Attached patch should have the missing file (otherwise it's same).

--
Regards
Petr Jelinek (PJMODOS)

Hi,

here's a (late, sorry about that) review:

== Trivia ==

Patch applies cleanly with a few 1 line offsets.

It's unified, not context, but that's trivial.

The patch adds some trailing whitespace, which is not good (git diff
shows it in red, it's easy to spot it). There's also one
hunk that's just an addition of a newline (in
src/backend/catalog/aclchk.c, -270,6 +291,7)

== Code ==

There's a few places where the following pattern is used:
if (!stmt->grantees)
whereas I think the project prefers:
stmt->grantees != NIL
Same for if (schemas) => if (schemas != NULL)

I'm not sure if this pattern in src/backend/catalog/aclchk.c is the best
option:
if (rolenames == NIL)
rolenames = lappend(rolenames, makeString(pstrdup("")));
if (nspnames == NIL)
nspnames = lappend(nspnames, makeString(pstrdup("")));
Appending an empty string and then checking in strlen of the option
value is 0
is ugly.

In SetDefaultACLs the OidIsValid(roleId) is not necessary, maybe better
put in
assert(oidIsValid). The caller always puts a valid rolename in there. Or
maybe
even better: make the caller pass an InvalidOid for the role if no is
specified. This way the handling arguments is more uniform between
SetDefaultACLs and ExecDefaultACLsStmt.

The logic in get_default_acl and pg_namespace_object_default_acl could be
improved, for instance get_default_acl checks if the result of the scan
is null
and if is, returns NULL. At the same time, the only calling function,
pg_namespace_object_default_acl checks the isNull flag instead of just
checking
if the result is NULL.

Also, pg_namespace_object_default_acl could just do without the isNull out
parameter and the same goes for get_default_acl. Just return NULL to
indicate
an invalid result and declare a isNull in get_default_acl locally to use
it in
heap_getattr. This also saves some lines in InsertPgClassTuple.

Also, in InsertPgClassTuple change the order of the branches:
+ if (isNull)
+ nulls[Anum_pg_class_relacl - 1] = true;
+ else
+ values[Anum_pg_class_relacl - 1] = PointerGetDatum(relacl);
to have the same pattern as the next if statement.

Also, changing tests like if (!isNull) to if (relacl) makes it more
natural to
see sequences like if (relacl) { do-stuff; pfree(relacl); }

In ExecDefaultACLsStmt this fragment:
else if (strcmp(defel->defname, "roles") == 0)
{
if (rolenames)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
drolenames = defel;
}
Should test if (drolenames), because currently it's possible to do:

alter default privileges for role test for role test grant select, insert on
table to test;

Maybe add a unit test for that.

A comment in dependency.c function getDefaultACLDescription with
something like
"shouldn't get here" in the default: switch branch could be useful,
cf. getRelationDescription.

In ExecGrantDefaults_Relation there's a hunk:
+ if (isNull)
+ elog(ERROR, "no DEFAULT PRIVILEGES for relation");
Maybe you could move it higher, no need to do other stuff if it's going
to fail
afterwards anyway.

ExecGrantDefaults_Function and ExecGrantDefaults_Relation could maybe share
code? They look quite similar, although it might not be so easy to
factor out
common functionality.

The "unrecognized GrantStmt.objtype: %d" error message needs better
wording I
think.

No code patch removes rows from pg_default_acls, so it might accumulate
cruft. Maybe a drop default privileges? Or maybe revoking all would delete
the row instead of setting it? It has the same meaning, I guess...

== Compiling and installing ==

My gcc complains about

gram.y: In function ‘base_yyparse’:
gram.y:1128: warning: assignment from incompatible pointer type
gram.y:1135: warning: passing argument 1 of ‘lappend’ from incompatible
pointer type
gram.y:1135: warning: assignment from incompatible pointer type
gram.y:1136: warning: assignment from incompatible pointer type

Regression tests fail because of the username mismatch

! DETAIL: drop cascades to default acls for role postgres on new
relation in namespace regressns
--- 951,957 ----
! DETAIL: drop cascades to default acls for role wulczer on new
relation in namespace regressns

== Testing ==

Tab completion is not up to speed - annoying ;)

The functionality worked as advertised, although I was able to do the
following:

postgres=# create role test login;
CREATE ROLE
postgres=# \c - test
psql (8.5devel)
You are now connected to database "postgres" as user "test".
postgres=> alter default privileges grant select on table to test;
ALTER DEFAULT PRIVILEGES
postgres=> \c - wulczer
psql (8.5devel)
You are now connected to database "postgres" as user "wulczer".
postgres=# drop role test;
DROP ROLE
postgres=# select * from pg_default_acls ;
defaclrole | defaclnamespace | defaclobjtype | defacllist
------------+-----------------+---------------+-----------------------
16384 | 0 | r | {16384=arwdDxt/16384}

The numeric defaclrole and defacllist don't look good.

While testing I also got a "unexpected object type: 1248" from
src/backend/catalog/pg_shdepend.c, but was unable to reproduce it.
This happened after I did a combination of DROP OWNED BY and REASSIGN
OWNED for
a role that has membership in other roles and that all had default
privileges
in a schema.

== Docs ==

Need to document that FOR USER also works (as a synonym for FOR ROLE) in the
synopsis of ALTER DEFAULT PRIVILEGES.

Add examples of usage of the FOR USER/FOR ROLE syntax and explain what
they do.

In the ALTER DEFAULT PRIVILEGES synopsis there's a copy-paste-o because
it says
you can do:
ALTER DEFAULT PRIVILEGES IN SCHEMA s GRANT DEFAULT PRIVILEGES ON TABLE
TO test;

The wording of "with grant options parameter is not applicable" in the
sql-grant page should mentiont that you also can't do:
GRANT DEFAULT PRIVILEGES ON s.test2 TO test;
so it should be indicated the "TO rolename" is also not applicable.

== Other ==

I'm not really sure what's the use of GRANT DEFAULT PRIVILEGES... Is it for
backward-sanitizing existing databases?

I haven't tested performance at all, I thinks it doesn't make sense for
a patch
like this.

Don't know about the relationship with the spec, I suspect there's no such
thing in there. But the feature has been reworked to match what the
community
wanted and I think that the syntax is quite good and the use cases are
there.

Not tested on Windows, probably not important.

Having said all that, I'm moving the patch to "Waiting on author".

Best,
Jan

Jan Urbański napsal(a):
> Hi,
>
> here's a (late, sorry about that) review:
>
Thanks for the comprehensive review!

> It's unified, not context, but that's trivial.
>
It's not, I have git configured to produce context diffs (see
http://wiki.postgresql.org/wiki/Working_with_Git#Context_diffs_with_Git ).

> == Code ==
>
> There's a few places where the following pattern is used:
> if (!stmt->grantees)
> whereas I think the project prefers:
> stmt->grantees != NIL
>

Ok changed that.

> I'm not sure if this pattern in src/backend/catalog/aclchk.c is the best
> option:
> if (rolenames == NIL)
> rolenames = lappend(rolenames, makeString(pstrdup("")));
> if (nspnames == NIL)
> nspnames = lappend(nspnames, makeString(pstrdup("")));
> Appending an empty string and then checking in strlen of the option
> value is 0
> is ugly.
>

I changed that, I had to change the way SetDefaultACLs is called a bit
anyway (to fix the problem with dependencies you mentioned below).

> In SetDefaultACLs the OidIsValid(roleId) is not necessary, maybe better
> put in
> assert(oidIsValid). The caller always puts a valid rolename in there. Or
> maybe
> even better: make the caller pass an InvalidOid for the role if no is
> specified. This way the handling arguments is more uniform between
> SetDefaultACLs and ExecDefaultACLsStmt.
>

Same as above.

> The logic in get_default_acl and pg_namespace_object_default_acl could be
> improved, for instance get_default_acl checks if the result of the scan
> is null
> and if is, returns NULL. At the same time, the only calling function,
> pg_namespace_object_default_acl checks the isNull flag instead of just
> checking
> if the result is NULL.
>
> Also, pg_namespace_object_default_acl could just do without the isNull out
> parameter and the same goes for get_default_acl. Just return NULL to
> indicate
> an invalid result and declare a isNull in get_default_acl locally to use
> it in
> heap_getattr. This also saves some lines in InsertPgClassTuple.
>
I agree, btw it actually does not save anything in InsertPgClassTuple,
but it does save few lines in ProcedureCreate.

> Also, in InsertPgClassTuple change the order of the branches:
> + if (isNull)
> + nulls[Anum_pg_class_relacl - 1] = true;
> + else
> + values[Anum_pg_class_relacl - 1] = PointerGetDatum(relacl);
> to have the same pattern as the next if statement.
>
Done.

> In ExecDefaultACLsStmt this fragment:
> else if (strcmp(defel->defname, "roles") == 0)
> {
> if (rolenames)
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("conflicting or redundant options")));
> drolenames = defel;
> }
> Should test if (drolenames), because currently it's possible to do:
>
Typo, fixed.

> A comment in dependency.c function getDefaultACLDescription with
> something like
> "shouldn't get here" in the default: switch branch could be useful,
> cf. getRelationDescription.
>
Ok.

> In ExecGrantDefaults_Relation there's a hunk:
> + if (isNull)
> + elog(ERROR, "no DEFAULT PRIVILEGES for relation");
> Maybe you could move it higher, no need to do other stuff if it's going
> to fail
> afterwards anyway.
>
Ok, I moved it just after owner check.

> ExecGrantDefaults_Function and ExecGrantDefaults_Relation could maybe share
> code? They look quite similar, although it might not be so easy to
> factor out
> common functionality.
>
They don't really, they operate on different tables and the only partly
shared part is the indexes and acl dependency update.

> The "unrecognized GrantStmt.objtype: %d" error message needs better
> wording I
> think.
>
Well, that exact same message is in other two places in original code, I
just copy pasted it.

> No code patch removes rows from pg_default_acls, so it might accumulate
> cruft. Maybe a drop default privileges? Or maybe revoking all would delete
> the row instead of setting it? It has the same meaning, I guess...
>
Now that I fixed DefaultACLs removal on role drop this is no longer true
(previously it was only dropped with schema a leftover from original
design).
Also revoking everything is not same as having no DefaultACLs, with no
DefaultACLs current behavior is used (owner gets all privs and public
might get something depending on object type).

> == Compiling and installing ==
>
> My gcc complains about
>
> gram.y: In function ‘base_yyparse’:
> gram.y:1128: warning: assignment from incompatible pointer type
> gram.y:1135: warning: passing argument 1 of ‘lappend’ from incompatible
> pointer type
> gram.y:1135: warning: assignment from incompatible pointer type
> gram.y:1136: warning: assignment from incompatible pointer type
>
Fixed.

> Regression tests fail because of the username mismatch
>
> ! DETAIL: drop cascades to default acls for role postgres on new
> relation in namespace regressns
> --- 951,957 ----
> ! DETAIL: drop cascades to default acls for role wulczer on new
> relation in namespace regressns
>
Removed that part from regression.

> == Testing ==
>
> The functionality worked as advertised, although I was able to do the
> following:
>
> postgres=# create role test login;
> CREATE ROLE
> postgres=# \c - test
> psql (8.5devel)
> You are now connected to database "postgres" as user "test".
> postgres=> alter default privileges grant select on table to test;
> ALTER DEFAULT PRIVILEGES
> postgres=> \c - wulczer
> psql (8.5devel)
> You are now connected to database "postgres" as user "wulczer".
> postgres=# drop role test;
> DROP ROLE
> postgres=# select * from pg_default_acls ;
> defaclrole | defaclnamespace | defaclobjtype | defacllist
> ------------+-----------------+---------------+-----------------------
> 16384 | 0 | r | {16384=arwdDxt/16384}
>
> The numeric defaclrole and defacllist don't look good.
>
Fixed, DefaultACLs for role are now dropped in DropRole.

> While testing I also got a "unexpected object type: 1248" from
> src/backend/catalog/pg_shdepend.c, but was unable to reproduce it.
> This happened after I did a combination of DROP OWNED BY and REASSIGN
> OWNED for
> a role that has membership in other roles and that all had default
> privileges
> in a schema.
>

Yeah that was because I was adding ACL dependencies and wasn't removing
them in DROP OWNED BY. Fixed. This is why I had to change the way
SetDefaultACLs is called, so it could be called from DROP OWNED BY.

> == Docs ==
>
> Need to document that FOR USER also works (as a synonym for FOR ROLE) in the
> synopsis of ALTER DEFAULT PRIVILEGES.
>
Added.

> Add examples of usage of the FOR USER/FOR ROLE syntax and explain what
> they do.
>

Added simple example.

> In the ALTER DEFAULT PRIVILEGES synopsis there's a copy-paste-o because
> it says
> you can do:
> ALTER DEFAULT PRIVILEGES IN SCHEMA s GRANT DEFAULT PRIVILEGES ON TABLE
> TO test;
>
Eh, right, fixed.

> The wording of "with grant options parameter is not applicable" in the
> sql-grant page should mentiont that you also can't do:
> GRANT DEFAULT PRIVILEGES ON s.test2 TO test;
> so it should be indicated the "TO rolename" is also not applicable.
>
Done.

> == Other ==
>
> I'm not really sure what's the use of GRANT DEFAULT PRIVILEGES... Is it for
> backward-sanitizing existing databases?
>
Yes (especially in combination with GRANT ON ALL if it gets in).

> Not tested on Windows, probably not important.
>

I did test it on Windows ;)

> Having said all that, I'm moving the patch to "Waiting on author".
>
I'll changed it back to "needs review" since I made quite a few changes
(per your review).

--
Regards
Petr Jelinek (PJMODOS)

I made some more small adjustments - mainly renaming stuff after Tom's
comment on anonymous code blocks patch and removed one unused shared
dependency.

--
Regards
Petr Jelinek (PJMODOS)

Petr Jelinek wrote:
> I made some more small adjustments - mainly renaming stuff after Tom's
> comment on anonymous code blocks patch and removed one unused shared
> dependency.

Hi,

the patch still has some issues with dependency handling:

postgres=# create role test;
CREATE ROLE
postgres=# create role test2;
CREATE ROLE
postgres=# create schema s;
CREATE SCHEMA
postgres=# alter default privileges in schema s for user test2 grant
insert on table to test;
ALTER DEFAULT PRIVILEGES
postgres=# drop role test2;
DROP ROLE
postgres=# drop schema s;
ERROR: could not find tuple for default acls 16387
postgres=#

At this moment pg_default_acls is empty and schema s is undroppable... I
got an unexpected server exit after that once, when I executed \ds from
psql, but unfortunately don't have a backtrace (forgot to ulimit -c
unlimited). The next time I tried to provoke that backend crash I
failed: after the "ERROR: could not find tuple for default acls 16387"
I'm only stuck with an undroppable schema, but the rest of the system
works normally.

Apart from that all my complains from the previous review seem to be
addressed, except for the tab completion... Not sure if it's mandatory
for commit, but it sure would be useful.

Marking as "Waiting on Author".

Cheers,
Jan

Jan Urbański napsal(a):
> Petr Jelinek wrote:
>
>> I made some more small adjustments - mainly renaming stuff after Tom's
>> comment on anonymous code blocks patch and removed one unused shared
>> dependency.
>>
>
> Hi,
>
> the patch still has some issues with dependency handling:
>
> postgres=# create role test;
> CREATE ROLE
> postgres=# create role test2;
> CREATE ROLE
> postgres=# create schema s;
> CREATE SCHEMA
> postgres=# alter default privileges in schema s for user test2 grant
> insert on table to test;
> ALTER DEFAULT PRIVILEGES
> postgres=# drop role test2;
> DROP ROLE
> postgres=# drop schema s;
> ERROR: could not find tuple for default acls 16387
> postgres=#
>

Fixed and added regression test for dependency handling.

--
Regards
Petr Jelinek (PJMODOS)

OK, the previous problem went away, but I can still do something like that:

postgres=# create role test;
CREATE ROLE
postgres=# create role test2;
CREATE ROLE
postgres=# create database db;
CREATE DATABASE
postgres=# \c db
psql (8.5devel)
You are now connected to database "db".
db=# alter default privileges for role test2 grant insert on table to test;
ALTER DEFAULT PRIVILEGES
db=# \c postgres
psql (8.5devel)
You are now connected to database "postgres".
postgres=# drop role test2;
DROP ROLE
postgres=# \c db
psql (8.5devel)
You are now connected to database "db".
db=# select * from pg_default_acls ;
defaclrole | defaclnamespace | defaclobjtype | defacllist
------------+-----------------+---------------+--------------------------------------
16385 | 0 | r |
{16385=arwdDxt/16385,test=a/wulczer}
(1 row)

db=#

Dependencies suck, I know..

Jan

Jan Urbański napsal(a):
> Dependencies suck, I know..
>

Cross-database dependencies do.

I had to make target role owner of the default acls which adds some side
effects like the fact that it blocks DROP ROLE so DROP OWNED BY has to
be used.
As for REASSIGN OWNED, it does not reassign anything (I don't think it's
a good idea to reassign default acls) it just spits warning with hint
what to do if user plans to drop the role.

--
Regards
Petr Jelinek (PJMODOS)

Petr Jelinek wrote:
> Jan Urbański napsal(a):
>> Dependencies suck, I know..
>>
>
> Cross-database dependencies do.
>
> I had to make target role owner of the default acls which adds some side
> effects like the fact that it blocks DROP ROLE so DROP OWNED BY has to
> be used.
> As for REASSIGN OWNED, it does not reassign anything (I don't think it's
> a good idea to reassign default acls) it just spits warning with hint
> what to do if user plans to drop the role.

OK, so that addresses my last gripe.

It seems that when you try to drop a role to which you have granted some
privileges before, you can't, and when you REASSIGN OWNED, it doesn't
help. So maybe it's not even necessary to give a warning when REASSIGN
OWNED is called on default ACLs.

Only loose end is tab completion which can probably be added later on.

I'm also not sure if we wouldn't like to have ALTER DEFAULT PRIVILEGES
FOR ALL ROLES (or something similar), so you won't have to ALTER DEFAULT
PRIVILEGES for each developer you gave a DB login to.

Petr told me that was the previous design but has been shot down - I
found references to that on the mailing list, but most complains were
about tying the syntax to ALTER SCHEMA. Since we now have ALTER DEFAULT
PRIVILEGES I think it might make sense to introduce a way to set the
default privileges for all roles (and give superusers the right to do it).

This can be added later on, but maybe we could make the syntax work so
when you do ALTER DEFAULT PRIVILEGES without FOR ROLE you set them for
all roles in the current DB and if you want to set them for yourself,
you need to specify FOR ROLE <yourrole>. That'd be a minor change in the
patch.

Setting to "Ready for Committer" (and leaving to the committer the
decision whether to support "FOR ALL ROLES" and what to do about the
warning).

Jan

Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
> [ latest version of DefaultACLs patch ]

I started looking through this patch, but found that it's not nearly
ready to commit :-(. The big missing piece is that there's no pg_dump
support for default ACLs. That's a bigger chunk of code than I have
time/interest to write, and I don't think I want to commit the feature
without it. (I'm willing to commit without tab completion or any
psql \d command to show the defaults, but pg_dump just isn't optional.)

There is another large problem, too. The patch seems to have
only half-baked support for global defaults (those not tied to a
specific schema) --- it looks like you can put them in, but half
of the code will ignore them or else fail while trying to use them.
This isn't just a matter of a few missed cases while coding, I think.
The generic issue that the code doesn't even think about addressing
is which default should apply when there's potentially more than one
applicable default? As long as there's only global and per-schema
defaults, it's not too hard to decide that the latter take precedence
over the former; but I have no idea what we're going to do in order
to add any other features. This seems like a sufficiently big
conceptual issue that it had better be resolved now, even if the first
version of the patch doesn't really need to deal with it.

Also, the GRANT DEFAULT PRIVILEGES thing just seems completely bizarre,
and I'm not convinced it has a sufficient use-case to justify such a
strange wart on GRANT. I think we should drop it. Or at least it needs
to be proposed and discussed as a separate feature. Maybe it would seem
less strange if the syntax was "RESET PRIVILEGES ON object".

A couple of minor gripes:

Per recent discussion, names of system catalogs shouldn't be plural.

elog() is not suitable for user-facing errors. For example in
ExecGrantStmt_Defaults, the grammar does not prohibit the unsupported
target object types, so you need to throw a nicer error there. Or
else reject them in the grammar. There seem to be a number of other
places where elog is used but the error is perfectly likely to be caused
by a user mistake.

regards, tom lane

Tom Lane wrote:
> Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
>
>> [ latest version of DefaultACLs patch ]
>>
>
> I started looking through this patch, but found that it's not nearly
> ready to commit :-(. The big missing piece is that there's no pg_dump
> support for default ACLs. That's a bigger chunk of code than I have
> time/interest to write, and I don't think I want to commit the feature
> without it. (I'm willing to commit without tab completion or any
> psql \d command to show the defaults, but pg_dump just isn't optional.)
>

Yeah I completely forgot about pg_dump just like I did with anonymous
code blocks :-(

> There is another large problem, too. The patch seems to have
> only half-baked support for global defaults (those not tied to a
> specific schema) --- it looks like you can put them in, but half
> of the code will ignore them or else fail while trying to use them.
> This isn't just a matter of a few missed cases while coding, I think.
> The generic issue that the code doesn't even think about addressing
> is which default should apply when there's potentially more than one
> applicable default? As long as there's only global and per-schema
> defaults, it's not too hard to decide that the latter take precedence
> over the former; but I have no idea what we're going to do in order
> to add any other features. This seems like a sufficiently big
> conceptual issue that it had better be resolved now, even if the first
> version of the patch doesn't really need to deal with it.
>

Half of the code will ignore them ? They are ignored if schema specific
defaults were set.
Yes I haven't tried to solve the problem of having non-hierarchical
filters for defaults and if we require that then this patch is dead for
(at least) this commitfest, because at the moment I don't even know
where to begin solving this.

> Also, the GRANT DEFAULT PRIVILEGES thing just seems completely bizarre,
> and I'm not convinced it has a sufficient use-case to justify such a
> strange wart on GRANT. I think we should drop it. Or at least it needs
> to be proposed and discussed as a separate feature. Maybe it would seem
> less strange if the syntax was "RESET PRIVILEGES ON object".
>

I vote for dropping it then.

--
Regards
Petr Jelinek (PJMODOS)

> There is another large problem, too. The patch seems to have
> only half-baked support for global defaults (those not tied to a
> specific schema) --- it looks like you can put them in, but half
> of the code will ignore them or else fail while trying to use them.
> This isn't just a matter of a few missed cases while coding, I think.
> The generic issue that the code doesn't even think about addressing
> is which default should apply when there's potentially more than one
> applicable default?

I thought the idea was to simply avoid that situation. Maybe we want to
forget about global defaults if that's the case, and just do the ROLE
defaults.

I thought we were trying to keep this solution as simple as possible.
It's meant to be a simple feature for simple use cases. I know we all
love making stuff as ornate and complex as possible around here, but
that kind of defeats the purpose of having DefaultACLs, as well as
setting the bar unreasonably high for Petr. Asking him to
future-filter-proof the feature assumes that there will be future
filters, which I'm not convinced there will.

I certainly haven't seen any good use case for having multiple
conflicting defaults. In fact, I thought we'd agreed that any complex
cases would be better handled by PL scripts.

pg_dump support is required though.

--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> This isn't just a matter of a few missed cases while coding, I think.
>> The generic issue that the code doesn't even think about addressing
>> is which default should apply when there's potentially more than one
>> applicable default?

> I thought the idea was to simply avoid that situation. Maybe we want to
> forget about global defaults if that's the case, and just do the ROLE
> defaults.

That seems like a pretty dead-end design.

> I thought we were trying to keep this solution as simple as possible.
> It's meant to be a simple feature for simple use cases. I know we all
> love making stuff as ornate and complex as possible around here, but
> that kind of defeats the purpose of having DefaultACLs, as well as
> setting the bar unreasonably high for Petr. Asking him to
> future-filter-proof the feature assumes that there will be future
> filters, which I'm not convinced there will.

I already mentioned one case that there's longstanding demand for, which
is to instantiate the correct permissions on new partition child tables.

But more generally, this is a fairly large and complicated patch in
comparison to the reward, if the intention is that it will never support
anything more than the one case of "IN SCHEMA foo" filtering.

regards, tom lane

Tom,

>> I thought the idea was to simply avoid that situation. Maybe we want to
>> forget about global defaults if that's the case, and just do the ROLE
>> defaults.
>
> That seems like a pretty dead-end design.

Well, the whole purpose for DefaultACLs is to simplify administration
for the simplest use cases. If we add a large host of conflicting
options, we haven't simplified stuff very much.

> I already mentioned one case that there's longstanding demand for, which
> is to instantiate the correct permissions on new partition child tables.

Wouldn't that be handled by inheritance?

> But more generally, this is a fairly large and complicated patch in
> comparison to the reward, if the intention is that it will never support
> anything more than the one case of "IN SCHEMA foo" filtering.

I thought we were doing ROLEs?

--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> But more generally, this is a fairly large and complicated patch in
>> comparison to the reward, if the intention is that it will never support
>> anything more than the one case of "IN SCHEMA foo" filtering.

> I thought we were doing ROLEs?

The owning-ROLE match is required, else you have issues with exactly
what the ACL really means. What we're discussing is what other filters
might exist to determine which objects are affected. The patch already
tries to handle the cases of "all owned objects" and "all owned objects
in schema X", and I think it's inevitable that people will want other
cases.

regards, tom lane

Tom,

> The owning-ROLE match is required, else you have issues with exactly
> what the ACL really means. What we're discussing is what other filters
> might exist to determine which objects are affected. The patch already
> tries to handle the cases of "all owned objects" and "all owned objects
> in schema X", and I think it's inevitable that people will want other
> cases.

Yeah, I'm thinking we should back off from filters for 8.5; we could do
them for 8.6, maybe. I'm one of the people who prefers a schema-based
system, but I'll do without one if it means we can keep things *simple*
(and get the feature in in 8.5).

I think trying to make this patch a panacea in the first iteration is
liable to backfire. Especially since we're doing GRANT ALL ON at the
same time.

--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> I think trying to make this patch a panacea in the first iteration is
> liable to backfire.

I did not suggest any such thing --- the current scope of functionality
is fine by me for a first cut. What I *am* saying is that we had better
have some idea of how we are going to extend it when (not if) we try
to extend it. Otherwise we are likely to find we painted ourselves
into a corner.

> Especially since we're doing GRANT ALL ON at the same time.

That's another patch that has an *excellent* chance of getting rejected
on pretty much the same grounds.

regards, tom lane

On 9/28/09, Josh Berkus <josh(at)agliodbs(dot)com> wrote
>
> > I already mentioned one case that there's longstanding demand for, which
> > is to instantiate the correct permissions on new partition child tables.
>
> Wouldn't that be handled by inheritance?

I wish, but no:
http://www.postgresql.org/docs/current/interactive/ddl-inherit.html
The first paragraph under "5.8.1 Caveats":
Table access permissions are not automatically inherited. Therefore, a user
attempting to access a parent table must either have permissions to do the
same operation on all its child tables as well, or must use the
ONLYnotation. When adding a new child table to an existing inheritance
hierarchy, be careful to grant all the needed permissions on it.

On Mon, Sep 28, 2009 at 1:32 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> The generic issue that the code doesn't even think about addressing
> is which default should apply when there's potentially more than one
> applicable default?  As long as there's only global and per-schema
> defaults, it's not too hard to decide that the latter take precedence
> over the former; but I have no idea what we're going to do in order
> to add any other features.  This seems like a sufficiently big
> conceptual issue that it had better be resolved now, even if the first
> version of the patch doesn't really need to deal with it.

I haven't read the patch, but it seems like one possible solution to
this problem would be to declare that any any DEFAULT PRIVILEGES you
set are cumulative. If you configure a global default, a per-schema
default, a default for tables whose names begin with the letter q, and
a default for tables created between midnight and 4am, then a table
called quux created in that schema at 2:30 in the morning will get the
union of all four sets of privileges.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I haven't read the patch, but it seems like one possible solution to
> this problem would be to declare that any any DEFAULT PRIVILEGES you
> set are cumulative. If you configure a global default, a per-schema
> default, a default for tables whose names begin with the letter q, and
> a default for tables created between midnight and 4am, then a table
> called quux created in that schema at 2:30 in the morning will get the
> union of all four sets of privileges.

Hmm ... interesting proposal. Simple to understand and simple to
implement, which are both to the good. I'm not clear though on whether
this behavior would be useful in practice. Any comments from those
who've been asking for default ACLs?

One potential trouble spot is that presumably the built-in default
privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate
with user-specified defaults. So you'd have a behavior where a
function would not get PUBLIC EXECUTE automatically if it matched
any of the available defaults, but would get it if it managed to
miss matching them all. I am not sure if that's bad or not, but
it seems kind of inconsistent.

regards, tom lane

On Mon, Sep 28, 2009 at 10:44 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I haven't read the patch, but it seems like one possible solution to
>> this problem would be to declare that any any DEFAULT PRIVILEGES you
>> set are cumulative.  If you configure a global default, a per-schema
>> default, a default for tables whose names begin with the letter q, and
>> a default for tables created between midnight and 4am, then a table
>> called quux created in that schema at 2:30 in the morning will get the
>> union of all four sets of privileges.
>
> Hmm ... interesting proposal.  Simple to understand and simple to
> implement, which are both to the good.  I'm not clear though on whether
> this behavior would be useful in practice.  Any comments from those
> who've been asking for default ACLs?
>
> One potential trouble spot is that presumably the built-in default
> privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate
> with user-specified defaults.

Why not?

...Robert

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > One potential trouble spot is that presumably the built-in default
> > privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate
> > with user-specified defaults.
>
> Why not?

How would you have a default that says "I *don't* want public execute on
my new functions"?

Stephen

Tom Lane napsal(a):
>> I thought we were trying to keep this solution as simple as possible.
>> It's meant to be a simple feature for simple use cases. I know we all
>> love making stuff as ornate and complex as possible around here, but
>> that kind of defeats the purpose of having DefaultACLs, as well as
>> setting the bar unreasonably high for Petr. Asking him to
>> future-filter-proof the feature assumes that there will be future
>> filters, which I'm not convinced there will.
>>
>
> I already mentioned one case that there's longstanding demand for, which
> is to instantiate the correct permissions on new partition child tables.
>

Just FYI, this one is still hierarchical (database->schema->parent table).

--
Regards
Petr Jelinek (PJMODOS)

Stephen Frost napsal(a):
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>
>>> One potential trouble spot is that presumably the built-in default
>>> privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate
>>> with user-specified defaults.
>>>
>> Why not?
>>
>
> How would you have a default that says "I *don't* want public execute on
> my new functions"?
>

This is actually problem that applies to whole Robert's proposal. How
would you define you don\t want insert on new tables in schema when you
granted it for whole database. I don't think any kind of mixing of
different default privileges is a good idea. I was thinking about
rejecting creation of conflicting default privileges but that would be
impossible to detect before object creation which is too late.

--
Regards
Petr Jelinek (PJMODOS)

Tom Lane napsal(a):
>> Especially since we're doing GRANT ALL ON at the same time.
>>
>
> That's another patch that has an *excellent* chance of getting rejected
> on pretty much the same grounds.
>

I don't really see how this applies to GRANT ON ALL. You don't have to
predict future there so if you have conflict in multiple filters user
specified you simply throw error.

--
Regards
Petr Jelinek (PJMODOS)

On Mon, Sep 28, 2009 at 11:47 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> > One potential trouble spot is that presumably the built-in default
>> > privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate
>> > with user-specified defaults.
>>
>> Why not?
>
> How would you have a default that says "I *don't* want public execute on
> my new functions"?

Hmm...

Maybe instead of having built-in default privileges, we could view
each user as having their global default ACL pre-initialized to that
same set of privileges (of course we needn't store it unless and until
they modify it). Then they could add to those or take away from them,
plus add additional privileges at other levels.

...Robert

Robert Haas napsal(a):
> On Mon, Sep 28, 2009 at 11:47 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
>> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>>
>>>> One potential trouble spot is that presumably the built-in default
>>>> privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate
>>>> with user-specified defaults.
>>>>
>>> Why not?
>>>
>> How would you have a default that says "I *don't* want public execute on
>> my new functions"?
>>
>
> Hmm...
>
> Maybe instead of having built-in default privileges, we could view
> each user as having their global default ACL pre-initialized to that
> same set of privileges (of course we needn't store it unless and until
> they modify it). Then they could add to those or take away from them,
> plus add additional privileges at other levels.
>

That's how it works now actually, the problem is that when you grant
something in the chain you can't revoke it anywhere else in the chain
when you are merging privileges as you proposed.

--
Regards
Petr Jelinek (PJMODOS)

Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
> That's how it works now actually, the problem is that when you grant
> something in the chain you can't revoke it anywhere else in the chain
> when you are merging privileges as you proposed.

To allow that, you have to have some notion of a priority order among
the available defaults, so that you can sensibly say that A should
override B. Which is easy as long as they've got hierarchical scopes,
but that doesn't seem like a restriction that will hold good for future
extensions.

regards, tom lane

On Tue, Sep 29, 2009 at 11:05 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
>> That's how it works now actually, the problem is that when you grant
>> something in the chain you can't revoke it anywhere else in the chain
>> when you are merging privileges as you proposed.
>
> To allow that, you have to have some notion of a priority order among
> the available defaults, so that you can sensibly say that A should
> override B.  Which is easy as long as they've got hierarchical scopes,
> but that doesn't seem like a restriction that will hold good for future
> extensions.

Yeah. Right now AIUI we have these:

grant default privileges across the board
grant default privileges to objects in schema X

In the future maybe we might have:

grant default privileges to all objects EXCEPT those in schemas A, B, C.

I'm not real sure whether this sensible and I'm not real sure whether
it's too baroque to be worthwhile, but I am fairly confident that it
doesn't create any nasty semantic problems, which can't be said for
any approach that involves permissions for schemas "overrriding" the
global permissions, which will break down horribly as soon as we want
to do anything orthogonal.

...Robert

Tom Lane napsal(a):
> Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
>
>> That's how it works now actually, the problem is that when you grant
>> something in the chain you can't revoke it anywhere else in the chain
>> when you are merging privileges as you proposed.
>>
>
> To allow that, you have to have some notion of a priority order among
> the available defaults, so that you can sensibly say that A should
> override B. Which is easy as long as they've got hierarchical scopes,
> but that doesn't seem like a restriction that will hold good for future
> extensions.
>

I am aware, I knew all that has been said so far at the time I sent in
the patch actually. That's why I am very skeptical about having those
future non-hierarchical filters, I just don't see a way to make it happen.
Also when you go to some insane complexity of default privileges that
don't respect your database structure then you either want to handle it
programatically as Josh said or you want to create new subroles what
have create something privilege and different default privileges instead
of hoping that the database will somehow magically do the right thing
about default acls conflicts.

--
Regards
Petr Jelinek (PJMODOS)

Tom,

> Hmm ... interesting proposal. Simple to understand and simple to
> implement, which are both to the good. I'm not clear though on whether
> this behavior would be useful in practice. Any comments from those
> who've been asking for default ACLs?

I'd be fine with it. My goals here are to make my life easier by
eliminating the need for complex GRANT scripts in the simplest cases,
and (more importantly) to get web developers to use database object
permissions *at all* by making them easy to manage. Robert's suggestion
fulfills this.

Again, for people who have complex or high security cases, you'd still
need a script. That's fine; object permissions are an NP-complete
problem anyway, and no feature we adopt is going to cover everyone. But
right now we have an issue that probably 98% of Postgres users don't use
object permissions *at all* because they're "too difficult to manage".

If I had a dollar for every person I saw running Postgres in production
as the "postgres" user, or with full public permissions on everything, I
could shut down PGX and go back to pottery.

> One potential trouble spot is that presumably the built-in default
> privileges (eg, PUBLIC EXECUTE for functions) would *not* cumulate
> with user-specified defaults. So you'd have a behavior where a
> function would not get PUBLIC EXECUTE automatically if it matched
> any of the available defaults, but would get it if it managed to
> miss matching them all. I am not sure if that's bad or not, but
> it seems kind of inconsistent.

Yeah, but the fact that functions have PUBLIC EXECUTE by default is
inconsistent. All DefaultACLs would be doing is inheriting this
inconsistency. Again, I'm OK with that.

--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com

Hi all,

because it seems like merging privileges seems to be acceptable for most
(although I am not sure I like it, but I don't have better solution for
managing conflicts), I changed the patch to do just that.

Also I think I addressed all other problems pointed out by Tom. Namely I
added pg_dump support (hopefully) and \ddp command for psql. Still no
tab competition though. I removed that GRANT DEFAULT PRIVILEGES
statement, changed user facing elog()s to ereport()s, and I changed
catalog name to singular.

--
Regards
Petr Jelinek (PJMODOS)

Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
> because it seems like merging privileges seems to be acceptable for most
> (although I am not sure I like it, but I don't have better solution for
> managing conflicts), I changed the patch to do just that.

It's not clear to me whether we have consensus on this approach.
Last chance for objections, anyone?

The main argument I can see against doing it this way is that it doesn't
provide a means for overriding the hard-wired public grants for object
types that have such (principally functions). I think that a reasonable
way to address that issue would be for a follow-on patch that allows
changing the hard-wired default privileges for object types. It might
well be that no one cares enough for it to matter, though. I think that
in most simple cases what's needed is a way to add privileges, not
subtract them --- and we're already agreed that this mechanism is only
meant to simplify simple cases.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
> > because it seems like merging privileges seems to be acceptable for most
> > (although I am not sure I like it, but I don't have better solution for
> > managing conflicts), I changed the patch to do just that.
>
> It's not clear to me whether we have consensus on this approach.
> Last chance for objections, anyone?

I don't like it, but at the same time I'd rather have it with this than
not have anything.

> The main argument I can see against doing it this way is that it doesn't
> provide a means for overriding the hard-wired public grants for object
> types that have such (principally functions). I think that a reasonable
> way to address that issue would be for a follow-on patch that allows
> changing the hard-wired default privileges for object types. It might
> well be that no one cares enough for it to matter, though. I think that
> in most simple cases what's needed is a way to add privileges, not
> subtract them --- and we're already agreed that this mechanism is only
> meant to simplify simple cases.

This doesn't actually address the entire problem. How about
schema-level default grants which you want to override with per-role
default grants? Or the other way around? Is it always only more
permissive with more defaults? Even when the grantee is the same?

I dunno, I'll probably just ignore the per-role stuff, personally, but
it seems more complex without sufficient definition about what's going
to happen in each case.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> This doesn't actually address the entire problem. How about
> schema-level default grants which you want to override with per-role
> default grants? Or the other way around? Is it always only more
> permissive with more defaults? Even when the grantee is the same?

Well, bear in mind that we're *only* going to allow these things
per-role, so as to avoid the problem of translating ACLs to a different
grantor. So the main case that's not being solved is "I'd like to
grant privileges XYZ everywhere except in this schema". I'm willing to
write that off as not being within the scope of a simple mechanism.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > This doesn't actually address the entire problem. How about
> > schema-level default grants which you want to override with per-role
> > default grants? Or the other way around? Is it always only more
> > permissive with more defaults? Even when the grantee is the same?
>
> Well, bear in mind that we're *only* going to allow these things
> per-role, so as to avoid the problem of translating ACLs to a different
> grantor. So the main case that's not being solved is "I'd like to
> grant privileges XYZ everywhere except in this schema". I'm willing to
> write that off as not being within the scope of a simple mechanism.

Erm, wait, we're going to drop the only piece of this that outside folks
have actually been asking for? Specifically, having per-schema default
ACLs? Big -1 for even bothering to add the complexity if we're not
going to address what end users are actually looking for. Perhaps I
missed where the issue with assigning the right grantor was, but that
feels very much like an implementation detail we can certainly solve and
document which way we decided to solve it (either schema owner, which
would be my preference, or object creator, which would be acceptable as
well).

That's my thoughts on it.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> Erm, wait, we're going to drop the only piece of this that outside folks
> have actually been asking for? Specifically, having per-schema default
> ACLs?

They are per-schema for the objects belonging to the granting user.
Otherwise you have a bunch of nasty issues, including the prospect
of non-superusers being able to control the privileges granted on
objects that don't belong to them.

regards, tom lane

Tom Lane wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>
>> Erm, wait, we're going to drop the only piece of this that outside folks
>> have actually been asking for? Specifically, having per-schema default
>> ACLs?
>>
>
> They are per-schema for the objects belonging to the granting user.
> Otherwise you have a bunch of nasty issues, including the prospect
> of non-superusers being able to control the privileges granted on
> objects that don't belong to them.
>
Also it's not really a problem since superuser or role admin can set
these for other roles.

--
Regards
Petr Jelinek (PJMODOS)

On Thu, Oct 1, 2009 at 1:37 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
>> because it seems like merging privileges seems to be acceptable for most
>> (although I am not sure I like it, but I don't have better solution for
>> managing conflicts), I changed the patch to do just that.
>
> It's not clear to me whether we have consensus on this approach.
> Last chance for objections, anyone?
>
> The main argument I can see against doing it this way is that it doesn't
> provide a means for overriding the hard-wired public grants for object
> types that have such (principally functions).  I think that a reasonable
> way to address that issue would be for a follow-on patch that allows
> changing the hard-wired default privileges for object types.  It might
> well be that no one cares enough for it to matter, though.  I think that
> in most simple cases what's needed is a way to add privileges, not
> subtract them --- and we're already agreed that this mechanism is only
> meant to simplify simple cases.

I'm going to reiterate what I suggested upthread... let's let the
default, global default ACL contain the hard-wired privileges, instead
of making them hardwired. Then your objects will get those privileges
not because they are hard-wired, but because you haven't changed your
global default ACL to not contain them.

...Robert

Robert Haas napsal(a):
> On Thu, Oct 1, 2009 at 1:37 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
>>
>>> because it seems like merging privileges seems to be acceptable for most
>>> (although I am not sure I like it, but I don't have better solution for
>>> managing conflicts), I changed the patch to do just that.
>>>
>> It's not clear to me whether we have consensus on this approach.
>> Last chance for objections, anyone?
>>
>> The main argument I can see against doing it this way is that it doesn't
>> provide a means for overriding the hard-wired public grants for object
>> types that have such (principally functions). I think that a reasonable
>> way to address that issue would be for a follow-on patch that allows
>> changing the hard-wired default privileges for object types. It might
>> well be that no one cares enough for it to matter, though. I think that
>> in most simple cases what's needed is a way to add privileges, not
>> subtract them --- and we're already agreed that this mechanism is only
>> meant to simplify simple cases.
>>
>
> I'm going to reiterate what I suggested upthread... let's let the
> default, global default ACL contain the hard-wired privileges, instead
> of making them hardwired. Then your objects will get those privileges
> not because they are hard-wired, but because you haven't changed your
> global default ACL to not contain them.
>

That's somewhat how I implemented it although not just on global level
but in any single filter, what we now have as defaults (before this
patch) is used as template for default acls and you can revoke it. You
just can't revoke anything you granted anywhere in the default acls chain.

--
Regards
Petr Jelinek (PJMODOS)

Petr Jelinek napsal(a):
> Robert Haas napsal(a):
>> I'm going to reiterate what I suggested upthread... let's let the
>> default, global default ACL contain the hard-wired privileges, instead
>> of making them hardwired. Then your objects will get those privileges
>> not because they are hard-wired, but because you haven't changed your
>> global default ACL to not contain them.
>
> That's somewhat how I implemented it although not just on global level
> but in any single filter, what we now have as defaults (before this
> patch) is used as template for default acls and you can revoke it. You
> just can't revoke anything you granted anywhere in the default acls chain.

Reminds me I forgot to adjust the docs. Attached patch fixes that (no
other changes).

--
Regards
Petr Jelinek (PJMODOS)

Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
> [ latest default-ACLs patch ]

Applied with a fair amount of editorial polishing. Notably I changed
the permissions requirements a bit:

* for IN SCHEMA, the *target* role has to have CREATE permission on the
target schema. Without this, the command is a bit pointless since the
permissions can never be used. The original coding checked whether the
*calling* role had USAGE, which seems rather irrelevant.

* I simplified the target-role permission test to is_member_of. The
original check for ADMIN seemed pointlessly strong, because if you're a
member of the role you can just become the role and set owned objects'
permissions however you like. I didn't see the point of the CREATEROLE
exemption either, and am generally suspicious of anything that would let
people change permissions on stuff they didn't own.

One thing that seems like it's likely to be an annoyance in practice
is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl
entries for a role to be dropped. But I can't see any very good way
around that, since the entries might be in some other database. One
thing that might at least reduce the number of keystrokes is to have
REASSIGN OWNED act as DROP OWNED BY for default ACLs. I can't convince
myself whether that's a good idea though, so I left it as-is for the
moment.

regards, tom lane

2009/10/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Applied with a fair amount of editorial polishing.  Notably I changed
> the permissions requirements a bit:
>

Thanks and congratulations! I'm really looking forward to this feature.

I pulled the latest sources and gave it a whirl. Things worked as
expected in psql, but I was a little surprised when I headed into the
documentation. The first place I visited was Chapter 20 - Database
Roles and Privileges, but there was no mention of the default ACLs
feature in there.

If you head into the SQL Reference, it's all there under ALTER DEFAULT
PRIVILEGES, but that's only helpful if you already know that it's
there and what the command is called. At the moment the only way
you'd find out about Default ACLs via the documentation is if you
noticed the link several paragraphs into the reference for GRANT.

Perhaps we should have something in Chapter 20 along the lines of
"Rather than tediously assigning privileges to objects every time you
create them, you can set default privileges on a schema" etc.

IMO we should be actively pointing people, especially Postgres/DBA
newbies, to this very useful functionality.

Cheers,
BJ

Tom Lane napsal(a):
> Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
>
>> [ latest default-ACLs patch ]
>>
>
> Applied with a fair amount of editorial polishing. Notably I changed
> the permissions requirements a bit:
>

Thank you very much Tom.

> One thing that seems like it's likely to be an annoyance in practice
> is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl
> entries for a role to be dropped. But I can't see any very good way
> around that, since the entries might be in some other database. One
> thing that might at least reduce the number of keystrokes is to have
> REASSIGN OWNED act as DROP OWNED BY for default ACLs. I can't convince
> myself whether that's a good idea though, so I left it as-is for the
> moment.
>

Yeah I am not happy about this either but there is not much we can do
about it. Btw I think in the version I sent in REASSIGN OWNED acted as
DROP OWNED for default ACLs.

--
Regards
Petr Jelinek (PJMODOS)

Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
> Tom Lane napsal(a):
>> One thing that seems like it's likely to be an annoyance in practice
>> is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl
>> entries for a role to be dropped.

> Yeah I am not happy about this either but there is not much we can do
> about it. Btw I think in the version I sent in REASSIGN OWNED acted as
> DROP OWNED for default ACLs.

IIRC it just threw a warning, which didn't seem tremendously useful to
me.

regards, tom lane

Brendan Jurd <direvus(at)gmail(dot)com> writes:
> I pulled the latest sources and gave it a whirl. Things worked as
> expected in psql, but I was a little surprised when I headed into the
> documentation. The first place I visited was Chapter 20 - Database
> Roles and Privileges, but there was no mention of the default ACLs
> feature in there.

I looked at that (as well as the material in section 5.6) and concluded
that it was a once-over-lightly presentation that didn't really need to
delve into this. But if you want to work it over, feel free to send in
a docs patch. Personally I'd like to see less duplication between 5.6
and 20.3, but I'm not quite sure how to refactor it --- the material is
arguably somewhat relevant in both places.

regards, tom lane

Tom Lane napsal(a):
> Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
>
>> Tom Lane napsal(a):
>>
>>> One thing that seems like it's likely to be an annoyance in practice
>>> is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl
>>> entries for a role to be dropped.
>>>
>> Yeah I am not happy about this either but there is not much we can do
>> about it. Btw I think in the version I sent in REASSIGN OWNED acted as
>> DROP OWNED for default ACLs.
>>
>
> IIRC it just threw a warning, which didn't seem tremendously useful to
> me.
>

Oh did it ? Then I must have discarded that idea for some reason. I
probably didn't want to be too pushy there.

--
Regards
Petr Jelinek (PJMODOS)

2009/10/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Brendan Jurd <direvus(at)gmail(dot)com> writes:
>> I pulled the latest sources and gave it a whirl.  Things worked as
>> expected in psql, but I was a little surprised when I headed into the
>> documentation.  The first place I visited was Chapter 20 - Database
>> Roles and Privileges, but there was no mention of the default ACLs
>> feature in there.
>
> I looked at that (as well as the material in section 5.6) and concluded
> that it was a once-over-lightly presentation that didn't really need to
> delve into this.

Well 5.6 goes as far as to give mention about WITH GRANT OPTION, so I
don't think we'd be going too deep to give a pointer regarding default
ACLs as well.

I don't think we need a fully detailed explanation of default ACLs
here, just a brief mention and a pointer to the reference page.

> But if you want to work it over, feel free to send in
> a docs patch.

I'll have a play around with it and see if I can come up with wording
that I like.

> Personally I'd like to see less duplication between 5.6
> and 20.3, but I'm not quite sure how to refactor it --- the material is
> arguably somewhat relevant in both places.
>

I hear you about the duplication. 20.3 is so brief that I'm quite
tempted to just remove it and instead have a paragraph referencing
GRANT and REVOKE somewhere in 20.1.

Cheers,
BJ

I tried to check the default ACL behavior.

postgres=# \c - ymj
psql (8.5devel)
You are now connected to database "postgres" as user "ymj".
postgres=> ALTER DEFAULT PRIVILEGES REVOKE INSERT ON TABLE FROM ymj;
ALTER DEFAULT PRIVILEGES
postgres=> CREATE TABLE t2 (x int, y text);
CREATE TABLE
postgres=> SELECT relname, relacl FROM pg_class WHERE oid = 't2'::regclass;
relname | relacl
---------+------------------
t2 | {ymj=rwdDxt/ymj}
(1 row)

postgres=> INSERT INTO t2 VALUES (1, 'aaa');
ERROR: permission denied for relation t2

It works for me fine, good, but ...

postgres=> SELECT * INTO t3 FROM t1;
SELECT
postgres=> SELECT * FROM t3;
a | b
---+-----
1 | aaa
2 | bbb
(2 rows)

postgres=> INSERT INTO t3 VALUES (3,'ccc');
ERROR: permission denied for relation t3

In this case, the new table t3 is created with the default ACL which does not
allow to insert any values by the owner of the relation.

SELECT INTO does not check ACL_INSERT on the newly created tables, because
we had been able to assume the table owner always has privilege to insert
values into the new table.
So, OpenIntoRel() didn't check this obvious privilege.

But the default ACL feature breaks this assumption. The table owner may not
have privilege to insert values into new tables.
So, it is necessary to put actual access controls on the OpenIntoRel().

Thanks,

Tom Lane wrote:
> Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
>> [ latest default-ACLs patch ]
>
> Applied with a fair amount of editorial polishing. Notably I changed
> the permissions requirements a bit:
>
> * for IN SCHEMA, the *target* role has to have CREATE permission on the
> target schema. Without this, the command is a bit pointless since the
> permissions can never be used. The original coding checked whether the
> *calling* role had USAGE, which seems rather irrelevant.
>
> * I simplified the target-role permission test to is_member_of. The
> original check for ADMIN seemed pointlessly strong, because if you're a
> member of the role you can just become the role and set owned objects'
> permissions however you like. I didn't see the point of the CREATEROLE
> exemption either, and am generally suspicious of anything that would let
> people change permissions on stuff they didn't own.
>
> One thing that seems like it's likely to be an annoyance in practice
> is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl
> entries for a role to be dropped. But I can't see any very good way
> around that, since the entries might be in some other database. One
> thing that might at least reduce the number of keystrokes is to have
> REASSIGN OWNED act as DROP OWNED BY for default ACLs. I can't convince
> myself whether that's a good idea though, so I left it as-is for the
> moment.
>
> regards, tom lane
>

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei napsal(a):
> I tried to check the default ACL behavior.
>
> It works for me fine, good, but ...
>
> postgres=> SELECT * INTO t3 FROM t1;
> SELECT
> postgres=> SELECT * FROM t3;
> a | b
> ---+-----
> 1 | aaa
> 2 | bbb
> (2 rows)
>
> postgres=> INSERT INTO t3 VALUES (3,'ccc');
> ERROR: permission denied for relation t3
>
> In this case, the new table t3 is created with the default ACL which does not
> allow to insert any values by the owner of the relation.
>
> SELECT INTO does not check ACL_INSERT on the newly created tables, because
> we had been able to assume the table owner always has privilege to insert
> values into the new table.
> So, OpenIntoRel() didn't check this obvious privilege.
>
> But the default ACL feature breaks this assumption. The table owner may not
> have privilege to insert values into new tables.
> So, it is necessary to put actual access controls on the OpenIntoRel().
>

That's strange behavior I agree. However I don't see how default ACLs
changed it in any way, owner could REVOKE his privileges before.

--
Regards
Petr Jelinek (PJMODOS)

Petr Jelinek napsal(a):
> Tom Lane napsal(a):
>> Petr Jelinek <pjmodos(at)pjmodos(dot)net> <mailto:pjmodos(at)pjmodos(dot)net> writes:
>>
>>> Tom Lane napsal(a):
>>>
>>>> One thing that seems like it's likely to be an annoyance in practice
>>>> is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl
>>>> entries for a role to be dropped.
>>>>
>>> Yeah I am not happy about this either but there is not much we can do
>>> about it. Btw I think in the version I sent in REASSIGN OWNED acted as
>>> DROP OWNED for default ACLs.
>>>
>> IIRC it just threw a warning, which didn't seem tremendously useful to
>> me.
>>
>
> Oh did it ? Then I must have discarded that idea for some reason. I
> probably didn't want to be too pushy there.
>

Now I remember why - consistency with ACLs on object. REASSIGN OWNED
does not drop any GRANTed ACLs on any object, so it seemed appropriate
to only drop default ACLs in DROP OWNED BY along with ACLs on objects.

--
Regards
Petr Jelinek (PJMODOS)

Petr Jelinek wrote:
> KaiGai Kohei napsal(a):
>> I tried to check the default ACL behavior.
>>
>> It works for me fine, good, but ...
>>
>> postgres=> SELECT * INTO t3 FROM t1;
>> SELECT
>> postgres=> SELECT * FROM t3;
>> a | b
>> ---+-----
>> 1 | aaa
>> 2 | bbb
>> (2 rows)
>>
>> postgres=> INSERT INTO t3 VALUES (3,'ccc');
>> ERROR: permission denied for relation t3
>>
>> In this case, the new table t3 is created with the default ACL which does not
>> allow to insert any values by the owner of the relation.
>>
>> SELECT INTO does not check ACL_INSERT on the newly created tables, because
>> we had been able to assume the table owner always has privilege to insert
>> values into the new table.
>> So, OpenIntoRel() didn't check this obvious privilege.
>>
>> But the default ACL feature breaks this assumption. The table owner may not
>> have privilege to insert values into new tables.
>> So, it is necessary to put actual access controls on the OpenIntoRel().
>>
>
> That's strange behavior I agree. However I don't see how default ACLs
> changed it in any way, owner could REVOKE his privileges before.
>
I don't think the default ACL feature should do something ad-hoc here.

Is there anything necessary more than adding permission checks to insert
values into the new table?

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Petr Jelinek escribió:
> Petr Jelinek napsal(a):
> >Tom Lane napsal(a):
> >>Petr Jelinek <pjmodos(at)pjmodos(dot)net> <mailto:pjmodos(at)pjmodos(dot)net> writes:
> >>>Tom Lane napsal(a):
> >>>>One thing that seems like it's likely to be an annoyance in practice
> >>>>is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl
> >>>>entries for a role to be dropped.
> >>>Yeah I am not happy about this either but there is not much we
> >>>can do about it. Btw I think in the version I sent in REASSIGN
> >>>OWNED acted as DROP OWNED for default ACLs.
> >>IIRC it just threw a warning, which didn't seem tremendously useful to
> >>me.
> >
> >Oh did it ? Then I must have discarded that idea for some reason.
> >I probably didn't want to be too pushy there.
>
> Now I remember why - consistency with ACLs on object. REASSIGN OWNED
> does not drop any GRANTed ACLs on any object, so it seemed
> appropriate to only drop default ACLs in DROP OWNED BY along with
> ACLs on objects.

That seems reasonable to me too ...

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
> KaiGai Kohei napsal(a):
>> SELECT INTO does not check ACL_INSERT on the newly created tables, because
>> we had been able to assume the table owner always has privilege to insert
>> values into the new table.
>> So, OpenIntoRel() didn't check this obvious privilege.
>>
>> But the default ACL feature breaks this assumption. The table owner may not
>> have privilege to insert values into new tables.
>> So, it is necessary to put actual access controls on the OpenIntoRel().

> That's strange behavior I agree. However I don't see how default ACLs
> changed it in any way, owner could REVOKE his privileges before.

The point is that some rows got into the new table, which should have
been disallowed if CREATE TABLE AS is considered to represent a CREATE
followed by an INSERT. However, I disagree that this necessarily
represents a bug in the permissions checks. We could perfectly well
define that INSERT means the right to insert into the table *after it is
created*, and that CREATE TABLE AS does not involve this right. I think
that that is a reasonable and potentially useful thing to do, whereas
defining it as KaiGai-san suggests would have no usefulness whatsoever.
What's more, CREATE TABLE AS is specified in the SQL standard, and I see
nothing in the standard mentioning that it requires INSERT privilege.

regards, tom lane

Tom Lane wrote:
> Petr Jelinek <pjmodos(at)pjmodos(dot)net> writes:
>> KaiGai Kohei napsal(a):
>>> SELECT INTO does not check ACL_INSERT on the newly created tables, because
>>> we had been able to assume the table owner always has privilege to insert
>>> values into the new table.
>>> So, OpenIntoRel() didn't check this obvious privilege.
>>>
>>> But the default ACL feature breaks this assumption. The table owner may not
>>> have privilege to insert values into new tables.
>>> So, it is necessary to put actual access controls on the OpenIntoRel().
>
>> That's strange behavior I agree. However I don't see how default ACLs
>> changed it in any way, owner could REVOKE his privileges before.
>
> The point is that some rows got into the new table, which should have
> been disallowed if CREATE TABLE AS is considered to represent a CREATE
> followed by an INSERT. However, I disagree that this necessarily
> represents a bug in the permissions checks. We could perfectly well
> define that INSERT means the right to insert into the table *after it is
> created*, and that CREATE TABLE AS does not involve this right. I think
> that that is a reasonable and potentially useful thing to do, whereas
> defining it as KaiGai-san suggests would have no usefulness whatsoever.
> What's more, CREATE TABLE AS is specified in the SQL standard, and I see
> nothing in the standard mentioning that it requires INSERT privilege.

It is also an issue due to the differences in perspectives and security models.

My major concern is come from the viewpoint of MAC which tries to control
data flows based on sensitivity levels.
So, if the default PG model considers that "CREATE TABLE AS" is an atomic
operation, not a pair of CREATE and INSERTs, I think it is an approach to
understand this behavior.
In my preference, this perspective should be documented somewhere.
(source code comments or official documentation?)

BTW, in the MAC model, we must strictly prevent a user with classified
security level to write any data into objects with unclassified security
level. It is called "write-down", being equivalent to information leaks.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>