Horton

Thread

Commits

  1. adjust ACL owners for REASSIGN and ALTER OWNER TO

  1. Re: BUG #9923: "reassign owned" does not change permissions grantor

    Alexey Bashtanov <bashtanov@imap.cc> — 2014-04-09T07:35:09Z

    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

  2. Re: BUG #9923: "reassign owned" does not change permissions grantor

    Bruce Momjian <bruce@momjian.us> — 2015-01-09T18:19:48Z

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

  3. Re: BUG #9923: "reassign owned" does not change permissions grantor

    Bruce Momjian <bruce@momjian.us> — 2015-01-22T17:37:56Z

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

  4. Re: BUG #9923: "reassign owned" does not change permissions grantor

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2015-12-16T22:40:05Z

    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

  5. Re: BUG #9923: "reassign owned" does not change permissions grantor

    Bruce Momjian <bruce@momjian.us> — 2015-12-17T20:14:53Z

    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                             +

  6. Re: BUG #9923: "reassign owned" does not change permissions grantor

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-12-17T20:21:46Z

    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

  7. Re: BUG #9923: "reassign owned" does not change permissions grantor

    Bruce Momjian <bruce@momjian.us> — 2015-12-17T20:23:04Z

    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                             +

  8. Re: BUG #9923: "reassign owned" does not change permissions grantor

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2015-12-21T22:54:50Z

    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

  9. Re: BUG #9923: "reassign owned" does not change permissions grantor

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-12-21T23:46:25Z

    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