Re: running logical replication as the subscription owner

On Tue, May 23, 2023 at 8:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Thank you for updating the patch! Here are review comments:
> >
> > +       /*
> > +        * Make sure that the copy command runs as the table owner, unless
> > +        * the user has opted out of that behaviour.
> > +        */
> > +       run_as_owner = MySubscription->runasowner;
> > +       if (!run_as_owner)
> > +               SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt);
> > +
> >         /* Now do the initial data copy */
> >         PushActiveSnapshot(GetTransactionSnapshot());
> >
> > I think we should switch users before the acl check in
> > LogicalRepSyncTableStart().
> >
>
> Agreed, we should check acl with the user that is going to perform
> operations on the target table. BTW, is it okay to perform an
> operation on the system table with the changed user as that would be
> possible with your suggestion (see replorigin_create())?

Do you see any problem in particular?

As per the documentation, pg_replication_origin_create() is only
allowed to the superuser by default, but in CreateSubscription() a
non-superuser (who has pg_create_subscription privilege) can call
replorigin_create(). OTOH, we don't necessarily need to switch to the
table owner user for checking ACL and RLS. We can just pass either
table owner OID or subscription owner OID to pg_class_aclcheck() and
check_enable_rls() without actually switching the user.

>
> >
> > While this approach works, I'm not sure we really need a trigger for
> > this test. I've attached a patch for discussion that doesn't use
> > triggers for the regression tests. We create a new subscription owned
> > by a user who doesn't have the permission to the target table. The
> > test passes only if run_as_owner = true works.
> >
>
> Why in the test do you need to give additional permissions to
> regress_admin2 when the actual operation has to be performed by the
> table owner?

Good point. We need to give the ability to SET ROLE to regress_admin2
but other permissions are unnecessary.

>
> +# Because the initial data sync is working as the table owner, all
> +# dat should be copied.
>
> Typo. /dat/data

Will fix.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com