Thread: BUG #9923: "reassign owned" does not change permissions grantor
The following bug has been logged on the website:
Bug reference: 9923
Logged by: Alexey Bashtanov
Email address: bashtanov@imap.cc
PostgreSQL version: 9.3.1
Operating system: CentOS 6.4
Description:
Hello!
"Alter ... owner to ..." statement changes the permissions grantor but
"reassign owned ..." does not.
[ACTIONS]
1. create a type, give it some permissions. Get something like this:
test_bash_20140408=# select (select rolname from pg_roles where oid =
typowner), typacl from pg_type where oid = 'foo'::regtype;
rolname | typacl
---------+---------
ro | {=U/ro}
(1 row)
2. reassign owned by its owner to some other user:
test_bash_20140408=# reassign owned by ro to test_ui;
REASSIGN OWNED
[EXPECTED]
new permissions grantor is the new owner:
test_bash_20140408=# select (select rolname from pg_roles where oid =
typowner), typacl from pg_type where oid = 'foo'::regtype;
rolname | typacl
---------+---------
test_ui | {=U/test_ui}
(1 row)
[RECIEVED]
permissions grantor was left the same:
test_bash_20140408=# select (select rolname from pg_roles where oid =
typowner), typacl from pg_type where oid = 'foo'::regtype;
rolname | typacl
---------+---------
test_ui | {=U/ro}
(1 row)
Additionally, these ACLs cannot be pg_restored after pg_dumped
BTW is the expected behavior documented? I could not find this in docs.
Regards,
Alexey Bashtanov
after a series of tests and source code reading I realized that 1) the bug is not fixed in last git repository version 2) the bug could be reproduced on types and foreign servers, maybe also on foreign data wrappers, triggers, but not on any other objects 3) it does not matter if we assign owner using "reassign owned" or using "alter .. owner to ..." 4) there is a problem on revoking such incorrect grants: a workaround is to reassign back to old owner, then revoke, than reassign once again 5) to fix the bug we need to perform aclnewowner call in AlterForeignServerOwner_internal and AlterTypeOwner (including the typtype == TYPTYPE_COMPOSITE case, cause we pass recursing=true to ATExecChangeOwner) and maybe in AlterForeignDataWrapperOwner_internal and AlterEventTriggerOwner_internal sorry I do not provide the exact patch Regards, Alexey Bashtanov
On Wed, Apr 9, 2014 at 11:35:09AM +0400, Alexey Bashtanov wrote: > after a series of tests and source code reading I realized that > 1) the bug is not fixed in last git repository version Confirmed. > 2) the bug could be reproduced on types and foreign servers, maybe > also on foreign data wrappers, triggers, but not on any other > objects Triggers don't have acl lists, but all the others are accurate. > 3) it does not matter if we assign owner using "reassign owned" or > using "alter .. owner to ..." Confirmed. > 4) there is a problem on revoking such incorrect grants: a > workaround is to reassign back to old owner, then revoke, than > reassign once again > 5) to fix the bug we need to perform aclnewowner call in > AlterForeignServerOwner_internal and AlterTypeOwner (including the > typtype == TYPTYPE_COMPOSITE case, cause we pass recursing=true to > ATExecChangeOwner) > and maybe in AlterForeignDataWrapperOwner_internal and > AlterEventTriggerOwner_internal I can confirm this bug report from April, and your analysis of the fixes --- we were missing calls to aclnewowner() for types, foreign servers, and foreign data wrappers, for both REASSIGN and ALTER OWNER TO. With the attached SQL script you can see the ACL fields properly changing to match the object owner (attached). Without the patch, only the table's ACL changes. The patch also changes the regression output --- I think that is because the object ownership changes remove certain duplicates from the ACL list. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Attachment
On Fri, Jan 9, 2015 at 01:19:48PM -0500, Bruce Momjian wrote: > I can confirm this bug report from April, and your analysis of the fixes > --- we were missing calls to aclnewowner() for types, foreign servers, > and foreign data wrappers, for both REASSIGN and ALTER OWNER TO. > > With the attached SQL script you can see the ACL fields properly > changing to match the object owner (attached). Without the patch, only > the table's ACL changes. > > The patch also changes the regression output --- I think that is because > the object ownership changes remove certain duplicates from the ACL > list. Patch applied. Thank you for the excellent bug report. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian wrote:
> On Fri, Jan 9, 2015 at 01:19:48PM -0500, Bruce Momjian wrote:
> > I can confirm this bug report from April, and your analysis of the fixes
> > --- we were missing calls to aclnewowner() for types, foreign servers,
> > and foreign data wrappers, for both REASSIGN and ALTER OWNER TO.
> >
> > With the attached SQL script you can see the ACL fields properly
> > changing to match the object owner (attached). Without the patch, only
> > the table's ACL changes.
> >
> > The patch also changes the regression output --- I think that is because
> > the object ownership changes remove certain duplicates from the ACL
> > list.
>
> Patch applied. Thank you for the excellent bug report.
I just realized that you didn't backpatch this bug fix, and therefore my
fix for bug #13666 fails to cherry-pick sanely on 9.4 and earlier.
I think this should be back-patched.
This is the changelog entry:
Author: Bruce Momjian <bruce@momjian.us>
Branch: master Release: REL9_5_BR [59367fdf9] 2015-01-22 12:36:55 -0500
adjust ACL owners for REASSIGN and ALTER OWNER TO
When REASSIGN and ALTER OWNER TO are used, both the object owner and ACL
list should be changed from the old owner to the new owner. This patch
fixes types, foreign data wrappers, and foreign servers to change their
ACL list properly; they already changed owners properly.
BACKWARD INCOMPATIBILITY?
Report by Alexey Bashtanov
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Dec 16, 2015 at 07:40:05PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > On Fri, Jan 9, 2015 at 01:19:48PM -0500, Bruce Momjian wrote:
> > > I can confirm this bug report from April, and your analysis of the fixes
> > > --- we were missing calls to aclnewowner() for types, foreign servers,
> > > and foreign data wrappers, for both REASSIGN and ALTER OWNER TO.
> > >
> > > With the attached SQL script you can see the ACL fields properly
> > > changing to match the object owner (attached). Without the patch, only
> > > the table's ACL changes.
> > >
> > > The patch also changes the regression output --- I think that is because
> > > the object ownership changes remove certain duplicates from the ACL
> > > list.
> >
> > Patch applied. Thank you for the excellent bug report.
>
> I just realized that you didn't backpatch this bug fix, and therefore my
> fix for bug #13666 fails to cherry-pick sanely on 9.4 and earlier.
>
> I think this should be back-patched.
>
> This is the changelog entry:
>
> Author: Bruce Momjian <bruce@momjian.us>
> Branch: master Release: REL9_5_BR [59367fdf9] 2015-01-22 12:36:55 -0500
>
> adjust ACL owners for REASSIGN and ALTER OWNER TO
>
> When REASSIGN and ALTER OWNER TO are used, both the object owner and ACL
> list should be changed from the old owner to the new owner. This patch
> fixes types, foreign data wrappers, and foreign servers to change their
> ACL list properly; they already changed owners properly.
>
> BACKWARD INCOMPATIBILITY?
Backpatching seems fine to me. I was just concerned if anyone was
relying on the existing buggy behavior. We do list this item as a 9.5
incompatibility, so the question is whether we can add an
incompatibility to back branches:
Fix <link linkend="SQL-REASSIGN-OWNED"><command>REASSIGN OWNED</></>
and <link linkend="SQL-ALTERTYPE"><command>ALTER OWNER TO</></>
to properly update permissions lists (ACLs) when changing ownership of
types, foreign data wrappers, and foreign servers (Bruce Momjian)
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +
Bruce Momjian <bruce@momjian.us> writes:
> Backpatching seems fine to me. I was just concerned if anyone was
> relying on the existing buggy behavior.
Seems unlikely. Even if they are, the potential for catalog corruption
later seems to trump that argument.
> We do list this item as a 9.5
> incompatibility, so the question is whether we can add an
> incompatibility to back branches:
> Fix <link linkend="SQL-REASSIGN-OWNED"><command>REASSIGN OWNED</></>
> and <link linkend="SQL-ALTERTYPE"><command>ALTER OWNER TO</></>
> to properly update permissions lists (ACLs) when changing ownership of
> types, foreign data wrappers, and foreign servers (Bruce Momjian)
Actually, I guess we should take out that 9.5 release note item altogether
if we back-patch this.
regards, tom lane
On Thu, Dec 17, 2015 at 03:21:46PM -0500, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Backpatching seems fine to me. I was just concerned if anyone was > > relying on the existing buggy behavior. > > Seems unlikely. Even if they are, the potential for catalog corruption > later seems to trump that argument. > > > We do list this item as a 9.5 > > incompatibility, so the question is whether we can add an > > incompatibility to back branches: > > > Fix <link linkend="SQL-REASSIGN-OWNED"><command>REASSIGN OWNED</></> > > and <link linkend="SQL-ALTERTYPE"><command>ALTER OWNER TO</></> > > to properly update permissions lists (ACLs) when changing ownership of > > types, foreign data wrappers, and foreign servers (Bruce Momjian) > > Actually, I guess we should take out that 9.5 release note item altogether > if we back-patch this. Yes, that was one of my points. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription +
Bruce Momjian wrote: > On Thu, Dec 17, 2015 at 03:21:46PM -0500, Tom Lane wrote: > > Bruce Momjian <bruce@momjian.us> writes: > > > Backpatching seems fine to me. I was just concerned if anyone was > > > relying on the existing buggy behavior. > > > > Seems unlikely. Even if they are, the potential for catalog corruption > > later seems to trump that argument. > > > > > We do list this item as a 9.5 > > > incompatibility, so the question is whether we can add an > > > incompatibility to back branches: > > > > > Fix <link linkend="SQL-REASSIGN-OWNED"><command>REASSIGN OWNED</></> > > > and <link linkend="SQL-ALTERTYPE"><command>ALTER OWNER TO</></> > > > to properly update permissions lists (ACLs) when changing ownership of > > > types, foreign data wrappers, and foreign servers (Bruce Momjian) > > > > Actually, I guess we should take out that 9.5 release note item altogether > > if we back-patch this. > > Yes, that was one of my points. Backpatched all the way back to 9.1. Are you, Bruce, on the hook for removing the 9.5 release note entry, or should I do that? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Backpatched all the way back to 9.1. Are you, Bruce, on the hook for
> removing the 9.5 release note entry, or should I do that?
You can remove it if you feel like, but either Bruce or I will be doing
another pass over the release notes before 9.5.0, and we should catch
it then.
regards, tom lane