Hi, On 2023-01-27 16:35:11 -0500, Robert Haas wrote: > > Maybe a daft question: > > > > Have we considered using a "normal grant", e.g. on the database, instead of > > a > > role? Could it e.g. be useful to grant a user the permission to create a > > subscription in one database, but not in another? > > Potentially, but I didn't think we'd want to burn through permissions > bits that fast, even given 7b378237aa805711353075de142021b1d40ff3b0. > Still, if the consensus is otherwise, I can change it.
I don't really have an opinion on what's better. I looked briefly whether there was discussion around ithis but I didn't see anything. pg_create_subcription feels a bit different than most of the other pg_* roles. For most of those there is no schema object to tie permissions to. But here there is. But I think there's good arguments against a GRANT approach, too. GRANT ALL ON DATABASE would suddenly be dangerous. How does it interact with database ownership? Etc. > Then I guess we'd end up with GRANT CREATE ON DATABASE and GRANT CREATE > SUBSCRIPTION ON DATABASE, which I'm sure wouldn't be confusing at all. Heh. I guess it could just be GRANT SUBSCRIBE. > Or, another thought, maybe this should be checking for CREATE on the > current database + also pg_create_subscription. That seems like it > might be the right idea, actually. Yes, that seems like a good idea. > > The subscription code already does ownership checks before locking and now > > there's also the passwordrequired before. Is it possible that this could > > open > > up some sort of race? Could e.g. the user change the ownership to the > > superuser in one session, do an ALTER in the other? > > > > It looks like your change won't increase the danger of that, as the > > superuser() check just checks the current users permissions. > > I'm not entirely clear whether there's a hazard there. I'm not at all either. It's just a code pattern that makes me anxious - I suspect there's a few places it makes us more vulnerable. > If there is, I think we could fix it by moving the LockSharedObject call up > higher, above object_ownercheck. The only problem with that is it lets you > lock an object on which you have no permissions: see > 2ad36c4e44c8b513f6155656e1b7a8d26715bb94. To really fix that, we'd need an > analogue of RangeVarGetRelidExtended. Yea, we really should have something like RangeVarGetRelidExtended() for other kinds of objects. It'd take a fair bit of work / time to use it widely, but it'll take even longer if we start in 5 years ;) Perhaps the bulk of RangeVarGetRelidExtended() could be generalized by having a separate name->oid lookup callback? Greetings, Andres Freund
