On Mon, Jul 25, 2022 at 4:03 AM Kenaniah Cerny <kenan...@gmail.com> wrote:
> Hi Antonin, > > Thank you again for the detailed review and questions. It was encouraging > to see the increasing level of nuance in this latest round. > > It's not clear from the explanation of the GRANT ... IN DATABASE ... / >> GRANT >> ... IN CURRENT DATABASE ... that, even if "membership in ... will be >> effective only when the recipient is connected to the database ...", the >> ADMIN option might not be "fully effective". > > > While I'm not entirely sure what you mean by fully effective, it sounds > like you may have expected a database-specific WITH ADMIN OPTION grant to > be able to take effect when connected to a different database (such as > being able to use db_4's database-specific grants when connected to db_3). > The documentation updated in this patch specifies that membership (for > database-specific grants) would be effective only when the grantee is > connected to the same database that the grant was issued for. > > In the case of attempting to make a role grant to db_4 from within db_3, > the user would need to have a cluster-wide admin option for the role being > granted, as the test case you referenced in your example aims to verify. > > I have added a couple of lines to the documentation included with this > patch in order to clarify. > > >> Specifically on the regression tests: >> >> * The function check_memberships() has no parameters - is there a >> reason not to use a view? >> > > I believe a view would work just as well -- this was an implementation > detail that was fashioned to match the pre-existing rolenames.sql file's > test format. > > >> * I'm not sure if the pg_auth_members catalog can contain InvalidOid >> in >> other columns than dbid. Thus I think that the query in >> check_memberships() only needs an outer JOIN for the pg_database >> table, >> while the other joins can be inner. >> > > This is probably true. The tests run just as well using inner joins for > pg_roles, as this latest version of the patch reflects. > > >> * In this part >> >> SET SESSION AUTHORIZATION role_read_12_noinherit; >> SELECT * FROM data; -- error >> SET ROLE role_read_12; -- error >> SELECT * FROM data; -- error >> >> I think you don't need to query the table again if the SET ROLE >> statement >> failed and the same query had been executed before the SET ROLE. > > > I left that last query in place as a sanity check to ensure that > role_read_12's privileges were indeed not in effect after the call to SET > ROLE. > > As we appear to now be working through the minutiae, it is my hope that > this will soon be ready for merge. > > - Kenaniah > The patch requires a rebase, please do that. Hunk #5 succeeded at 454 (offset 28 lines). 1 out of 5 hunks FAILED -- saving rejects to file doc/src/sgml/ref/grant.sgml.rej ... ... -- Ibrar Ahmed
