Horton

Thread

Commits

  1. Add test for REVOKE ADMIN OPTION

  1. Add regression coverage for REVOKE ADMIN OPTION

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-15T23:58:34Z

    Hackers,

While working on a fix for dangling references to dropped roles in the pg_auth_members.grantor field, I happened to notice we entirely lack regression test coverage of the REVOKE ADMIN OPTION FOR form of the RevokeRoleStmt.  I am unaware of any bugs in the current implementation, but future work on roles may benefit if we close the testing gap.

For consideration:

  2. Re: Add regression coverage for REVOKE ADMIN OPTION

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-16T14:31:18Z

    > On 16 Nov 2021, at 00:58, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

> While working on a fix for dangling references to dropped roles in the pg_auth_members.grantor field, I happened to notice we entirely lack regression test coverage of the REVOKE ADMIN OPTION FOR form of the RevokeRoleStmt.  I am unaware of any bugs in the current implementation, but future work on roles may benefit if we close the testing gap.

LGTM.  Reading this I realized that the GRANTED BY keyword for RevokeRoleStmt
isn't working as documented, it's not checking the role at all.  I've sent a
diff for that with tests on the relevant thread, but I think it would be a good
to get this in too to boost coverage.

--
Daniel Gustafsson		https://vmware.com/

  3. Re: Add regression coverage for REVOKE ADMIN OPTION

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-16T14:32:15Z

    
> On Nov 16, 2021, at 6:31 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
>> On 16 Nov 2021, at 00:58, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
>> While working on a fix for dangling references to dropped roles in the pg_auth_members.grantor field, I happened to notice we entirely lack regression test coverage of the REVOKE ADMIN OPTION FOR form of the RevokeRoleStmt.  I am unaware of any bugs in the current implementation, but future work on roles may benefit if we close the testing gap.
> 
> LGTM.  Reading this I realized that the GRANTED BY keyword for RevokeRoleStmt
> isn't working as documented, it's not checking the role at all.  I've sent a
> diff for that with tests on the relevant thread, but I think it would be a good
> to get this in too to boost coverage.

Thanks for the review!

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  4. Re: Add regression coverage for REVOKE ADMIN OPTION

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-26T13:17:54Z

    > On 16 Nov 2021, at 15:32, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

> Thanks for the review!

Pushed to master along with the bugfix it helped uncover, thanks!

--
Daniel Gustafsson		https://vmware.com/