Re: Proposal: allow database-specific role memberships

Rebased yet again...

On Mon, Jul 4, 2022 at 1:17 PM Kenaniah Cerny <kenaniah(at)gmail(dot)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 <ah(at)cybertec(dot)at> wrote:
>
>> Kenaniah Cerny <kenaniah(at)gmail(dot)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
>>
>