Rebased yet again... On Mon, Jul 4, 2022 at 1:17 PM Kenaniah Cerny <kenan...@gmail.com> wrote:
> Hi Antonin, > > First of all, thank you so much for taking the time to review my patch. > I'll answer your questions in reverse order: > > The "unsafe_tests" directory is where the pre-existing role tests were > located. According to the readme of the "unsafe_tests" directory, the tests > contained within are not run during "make installcheck" because they could > have side-effects that seem undesirable for a production installation. This > seemed like a reasonable location as the new tests that this patch > introduces also modifies the "state" of the database cluster by adding, > modifying, and removing roles & databases (including template1). > > Regarding roles_is_member_of(), the nuance is that role "A" in your > example would only be considered a member of role "B" (and by extension > role "C") when connected to the database in which "A" was granted > database-specific membership to "B". Conversely, when connected to any > other database, "A" would not be considered to be a member of "B". > > This patch is designed to solve the scenarios in which one may want to > grant constrained access to a broader set of privileges. For example, > membership in "pg_read_all_data" effectively grants SELECT and USAGE rights > on everything (implicitly cluster-wide in today's implementation). By > granting a role membership to "pg_read_all_data" within the context of a > specific database, the grantee's read-everything privilege is effectively > constrained to just that specific database (as membership within > "pg_read_all_data" would not otherwise be held). > > A rebased version is attached. > > Thanks again! > > - Kenaniah > > On Wed, Jun 29, 2022 at 6:45 AM Antonin Houska <a...@cybertec.at> wrote: > >> Kenaniah Cerny <kenan...@gmail.com> wrote: >> >> > Attached is a newly-rebased patch -- would love to get a review from >> someone whenever possible. >> >> I've picked this patch for a review. The patch currently does not apply >> to the >> master branch, so I could only read the diff. Following are my comments: >> >> * I think that roles_is_member_of() deserves a comment explaining why the >> code >> that you moved into append_role_memberships() needs to be called twice, >> i.e. once for global memberships and once for the database-specific >> ones. >> >> I think the reason is that if, for example, role "A" is a >> database-specific >> member of role "B" and "B" is a "global" member of role "C", then "A" >> should >> not be considered a member of "C", unless "A" is granted "C" >> explicitly. Is >> this behavior intended? >> >> Note that in this example, the "C" members are a superset of "B" >> members, >> and thus "C" should have weaker permissions on database objects than >> "B". What's then the reason to not consider "A" a member of "C"? If "C" >> gives its members some permissions of "B" (e.g. "pg_write_all_data"), >> then I >> think the roles hierarchy is poorly designed. >> >> A counter-example might help me to understand. >> >> * Why do you think that "unsafe_tests" is the appropriate name for the >> directory that contains regression tests? >> >> I can spend more time on the review if the patch gets rebased. >> >> -- >> Antonin Houska >> Web: https://www.cybertec-postgresql.com >> >
database-role-memberships-v10.patch
Description: Binary data
