On Mon, Mar 7, 2022 at 11:14 PM Mark Dilger <mark.dil...@enterprisedb.com> wrote: > > On Mar 7, 2022, at 12:16 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > > What would be > > lost if we drop it? > > I looked into this a bit. Removing that bit of code, the only regression > test changes for "check-world" are the expected ones, with nothing else > breaking. Running installcheck+pg_upgrade to the patched version of HEAD > from each of versions 11, 12, 13 and 14 doesn't turn up anything untoward.
I looked into this a bit, too. I attach a draft patch for removing the self-admin exception. I found that having is_admin_of_role() return true matters in three ways: (1) It lets you grant membership in the role to some other role. (2) It lets you revoke membership in the role from some other role. (3) It changes the return value of pg_role_aclcheck(), which is used in the implementation of various SQL-callable functions all invoked via the name pg_has_role(). We've mostly been discussing (2) as an issue, but (1) and (3) are pretty interesting too. Regarding (3), there is a comment in the code indicating that Noah considered the self-admin exception something of a wart as far as pg_has_role() is concerned. As to (1), I discovered that today you can do this: rhaas=# create user foo; CREATE ROLE rhaas=# create user bar; CREATE ROLE rhaas=# \q [rhaas ~]$ psql -U foo rhaas psql (15devel) Type "help" for help. rhaas=> grant foo to bar with admin option; GRANT ROLE I don't know why I didn't realize that before. It's a natural result of treating the logged-in user as if they had admin option. But it's weird that you can't even be granted WITH ADMIN OPTION on your own login role, but at the same time without having it you can grant it to someone else! I believe there are three other points worth some consideration here. First, in the course of my investigation I re-discovered what Tom already did a good job articulating: tgl> Having said that, one thing that I find fishy is that it's not clear tgl> where the admin privilege for a role originates. After "CREATE ROLE tgl> alice", alice has no members, therefore none that have admin privilege, tgl> therefore the only way that the first member could be added is via tgl> superuser deus ex machina. This does not seem clean. I agree with that, but I don't think it's a sufficient reason for keeping the self-admin exception, because the same problem exists for non-login roles. I don't even think it's the right idea conceptually to suppose that the power to administer a role originates from the role itself. If that were so, then it would be inherited by all members of the role along with all the rest of the role's privileges, which is so clearly not right that we've already prohibited a role from having WITH ADMIN OPTION on itself. In my opinion, the right to administer a role - regardless of whether or not it is a login role - most naturally vests in the role that created it, or something in that direction at least, if not that exact thing. Today, that means the superuser or a CREATEROLE user who could hack superuser if they wished. In the future, I hope for other alternatives, as recently argued on other threads. But we need not resolve the question of how that should work exactly in order to agree (as I hope we do) that doubling down on the self-administration exception is not the answer. Second, it occured to me to wonder what implications a change like this might have for dump and restore. If privilege restoration somehow relied on this behavior, then we'd have a problem. But I don't think it does, because (a) pg_dump can SET ROLE but can't change the session user without reconnecting, so it's unclear how we could be relying on it; (b) it wouldn't work for non-login roles, and it's unlikely that we would treat login and non-login roles different in terms of restoring privileges, and (c) when I execute the example shown above and then run pg_dump, there's no attempt to change the current user, it just dumps "GRANT foo TO bar WITH ADMIN OPTION GRANTED BY foo". Third, it occurred to me to wonder whether some users might be using and relying upon this behavior. It's certainly possible, and it does suck that we'd be removing it without providing a workable substitute. But it's probably not a LOT of users because most people who have commented on this topic on this mailing list seem to find granting membership in a login role a super-weird thing to do, because a lot of people really seem to want every role to be a user or a group, and a login role with members feels like it's blurring that line. I'm inclined to think that the small number of people who may be unhappy is an acceptable price to pay for removing this wart, but it's a judgement call and if someone has information to suggest that I'm wrong, it'd be good to hear about that. Thanks, -- Robert Haas EDB: http://www.enterprisedb.com
remove-self-admin.patch
Description: Binary data
