Re: running logical replication as the subscription owner

On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis <pgsql@j-davis.com> wrote:
> I say just take the special case out of 0001. If the trigger doesn't
> work as a SECURITY_RESTRICTED_OPERATION, and is also ENABLED ALWAYS,
> then the user can just use the new option in 0002 to get the old
> behavior. I don't see a reason to implicitly give them the old
> behavior, as 0001 does.

Mmm, I don't agree. Suppose A can SET ROLE to B or C, and B can SET
ROLE to A. With the patch as written, actions on B's tables are not
confined by the SECURITY_RESTRICTED_OPERATION flag, but actions on C's
tables are.

I think we want to do everything possible to avoid people feeling like
they need to turn on this new option. I'm not sure we'll ever be able
to get rid of it, but we certainly should avoid doing things that make
it more likely that it will be needed.

> > 0002 adds a run_as_owner option to subscriptions. This doesn't make
> > the updated regression tests fail, because they don't use it. If you
> > revert the changes to 027_nosuperuser.pl then you get failures (as
> > one
> > would hope) and if you then add WITH (run_as_owner = true) to the
> > CREATE SUBSCRIPTION command in 027_nosuperuser.pl then it passes
> > again. I need to spend some more time thinking about what the tests
> > actually ought to look like if we go this route -- I haven't looked
> > through what Jelte proposed yet -- and also the documentation would
> > need a bunch of updating.
>
> "run_as_owner" is ambiguous -- subscription owner or table owner?
>
> I would prefer something like "trust_table_owners". That would
> communicate that the user shouldn't choose it unless they know what
> they're doing.

I agree that the naming is somewhat problematic, but I don't like
trust_table_owners. It's not clear enough about what actually happens.
I want the name to describe behavior, not sentiment.

run_as_subscription_owner removes the ambiguity, but is long.

run_as_table_owner is a bit shorter, and we could do that with the
sense reversed. Is that equally clear? I'm not sure.

I can think of other alternatives, like user_switching or
switch_to_table_owner or no_user_switching or various other things,
but none of them seem very good to me.

Another idea could be to make the option non-Boolean. This is
comically long and I can't seriously recommend it, but just to
illustrate the point, if you type CREATE SUBSCRIPTION ... WITH
(execute_code_as_owner_of_which_object = subscription) then you
certainly should know what you've signed up for! If there were a
shorter version that were still clear, I could go for that, but I'm
having trouble coming up with exact wording.

I don't think run_as_owner is terrible, despite the ambiguity. It's
talking about the owner of the object on which the property is being
set. Isn't that the most natural interpretation? I'd be pretty
surprised if I set a property called run_as_owner on object A and it
ran as the owner of some other object B. I think our notion of how
ambiguous this is may be somewhat inflated by the fact that we've just
had a huge conversation about whether it should be the table owner or
the subscription owner, so those possibilities are etched in our mind
in a way that maybe they won't be for people coming at this fresh. But
it's hard to be sure what other people will think about something, and
I don't want to be too optimistic about the name I picked, either.

> If you are worried that *I* think 0002 would be a poor call, then no, I
> don't. Initially I didn't like the idea of supporting two behaviors
> (and who would?), but we probably can't avoid it at least for right
> now.

OK, good. Then we have a way forward that nobody's too upset about.

-- 
Robert Haas
EDB: http://www.enterprisedb.com