On Tue, Aug 30, 2022 at 1:30 PM Nathan Bossart <nathandboss...@gmail.com> wrote: > 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.
Well, a combination of the two. It's modeled after is_admin_of_role() in the sense that it also calls roles_is_member_of() with a non-InvalidOid third argument and a non-NULL fourth argument. All other callers pass InvalidOid/NULL. > * 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? I think it's easiest if I get an example. Suppose role 'william' inherits 'charles' and 'diana' and role 'charles' in turn inherits from roles 'elizabeth' and 'philip'. Furthermore, 'william' has ADMIN OPTION on 'cambridge', 'charles' on 'wales', and 'elizabeth' on 'uk'. Now, because 'william' has ADMIN OPTION on role 'cambridge', he can add members to that role and remove the ones he added. He can also grant admin out option to others and later remove it (and all dependent grants that those others have made). When he does this, he is acting as himself, on his own authority. Because he inherits the privileges of charles directly and of elizabeth via charles, he can also add members to wales and uk. But if does this, he is not acting as himself, because he himself does not possess the ADMIN OPTION on either of those roles. Rather, he is acting on authority delegated from some other role which does possess admin option on those roles. Therefore, if 'william' adds a member to 'uk', the grantor MUST be recorded as 'elizabeth', not 'william', because the grantor of record must possess admin option on the role. Now let's add another layer of complexity. Suppose that the 'uk' role possesses ADMIN OPTION on some other role, let's say 'parliament'. Can 'william' use the fact that he inherits from 'elizabeth' to grant membership in 'parliament'? That all depends on whether 'uk' was granted to 'elizabeth' WITH INHERIT TRUE or WITH INHERIT FALSE. If it was granted WITH INHERIT TRUE, then elizabeth freely exercises all the powers of parliament, william freely exercises all of elizabeth's powers, and thus william can grant membership in parliament. Such a membership will be recorded as having been granted by uk, the role that actually holds ADMIN OPTION on parliament. However, if elizabeth was granted uk WITH INHERIT FALSE, then she can't mess with the membership of parliament, and thus neither can william. At least, not without first executing "SET ROLE uk", which in this scenario, either could do, but perhaps the use of SET ROLE is logged, audited, and very carefully scrutinized. So, what does all of this mean for select_best_grantor() and its helper function roles_is_member_of()? Well, first, we have to recurse. william can act on his own behalf when administering cambridge, and on behalf of charles or elizabeth when administering wales or uk respectively, and there's no way to get that behavior without recursing. Second, we have to stop the recursion when we reach a grant that is not inherited. Otherwise, william might be able to act on behalf of uk and administer membership in parliament even if uk is granted to elizabeth WITH INHERIT FALSE. Third, and this is where it gets tricky, we have to stop recursion *only after checking whether we've found a valid grantor*. william is allowed to administer membership cambridge whether or not it is granted to william WITH INHERIT TRUE or WITH INHERIT FALSE. Either way, he possess ADMIN OPTION on that role, and may administer membership in it. Likewise, he can administer membership in wales or uk as charles or elizabeth, respectively, because he definitely inherits the privileges of roles which definitely have ADMIN OPTION on those roles. Maybe a clearer way to look at is this: we're going to transit a series of role-membership links going from william (who attempts some operation) to the role in which he's trying to grant (or revoke) membership. For the operation to succeed, we need every link but the last in the chain to have INHERIT TRUE, and we need the last link in the chain to have ADMIN TRUE. We don't care whether the links other than the last one have ADMIN, and we don't care whether the last link has INHERIT. Thus: - william => cambridge is OK because the one and only grant involved has ADMIN, whether or not it has INHERIT - william => charles => wales is OK because the first hop has INHERIT and the second hop has ADMIN - william => charles => elizabeth => uk is OK because the first two hops have ADMIN and the last hop has INHERIT - william => charles => elizabeth => uk => parliament is probably not OK because although the first two hops have ADMIN and the last has INHERIT, the third hop probably lacks INHERIT I hope this makes it clearer. select_best_grantor() can't completely disregard links without INHERIT, because if it does, it can't pay any attention to the last grant in the chain, which only needs to have ADMIN, not INHERIT. But it must pay some attention to them, because every earlier link in the chain does need to have INHERIT. By moving the if-test up, the patch makes it behave just that way, or at least, I think it does. > 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... I do not rule that out! -- Robert Haas EDB: http://www.enterprisedb.com
