Re: replacing role-level NOINHERIT with a grant-level option

On Mon, Aug 29, 2022 at 03:38:57PM -0400, Robert Haas wrote:
> Argh, I found a bug, and one that I should have caught during testing,
> too. I modelled the new function select_best_grantor() on
> is_admin_of_role(), but it differs in that it calls
> roles_is_member_of() with ROLERECURSE_PRIVS rather than
> ROLECURSE_MEMBERS. Sadly, roles_is_member_of() handles
> ROLERECURSE_PRIVS by completely ignoring non-inherited grants, which
> is wrong, because then calls to select_best_grantor() treat a member
> of a role with INHERIT FALSE, ADMIN TRUE is if they were not an admin
> at all, which is incorrect.

I've been staring at this stuff for a while, and I'm still rather confused.

* You mention that you modelled the "new" function select_best_grantor() on
is_admin_of_role(), but it looks like that function was introduced in 2005.
IIUC you meant select_best_admin(), which was added much more recently.

* I don't see why we should ignore inheritance until after checking admin
option. Prior to your commit (e3ce2de), we skipped checking for admin
option if the role had [NO]INHERIT, and AFAICT your commit aimed to retain
that behavior. The comment above check_role_grantor() even mentions
"inheriting the privileges of a role which does have ADMIN OPTION." Within
the function, I see the following comment:

* Otherwise, the grantor must either have ADMIN OPTION on the role or
* inherit the privileges of a role which does. In the former case,
* record the grantor as the current user; in the latter, pick one of
* the roles that is "most directly" inherited by the current role
* (i.e. fewest "hops").

So, IIUC, the intent is for ADMIN OPTION to apply even if for non-inherited
grants, but we still choose to recurse in roles_is_member_of() based on
inheritance. This means that you can inherit the ADMIN OPTION to some
degree, but if you have membership WITH INHERIT FALSE to a role that has
ADMIN OPTION on another role, the current role effectively does not have
ADMIN OPTION on the other role. Is this correct?

As I'm writing this down, I think it's beginning to make more sense to me,
so this might just be a matter of under-caffeination. But perhaps some
extra comments or documentation wouldn't be out of the question...

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com