Re: running logical replication as the subscription owner

On Thu, Jun 8, 2023 at 7:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Jun 8, 2023 at 6:32 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Thu, May 25, 2023 at 5:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > I've attached the updated patch. Please review it.
> > > >
> > >
> > > Few comments:
> > > 1.
> > > + /* get the owner for ACL and RLS checks */
> > > + run_as_owner = MySubscription->runasowner;
> > > + checkowner = run_as_owner ? MySubscription->owner : rel->rd_rel->relowner;
> > > +
> > >   /*
> > >   * Check that our table sync worker has permission to insert into the
> > >   * target table.
> > >   */
> > > - aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
> > > + aclresult = pg_class_aclcheck(RelationGetRelid(rel), checkowner,
> > >
> > > One thing that slightly worries me about this change is that we
> > > started to check the permission for relowner before even ensuring that
> > > we can switch to relowner. See checks in SwitchToUntrustedUser(). If
> > > we want to first ensure that we can switch to relowner then I think we
> > > should move this permission-checking code before we try to copy the
> > > table.
> >
> > Agreed. I thought it's better to do ACL and RLS checks before creating
> > the replication slot but it's not important. Rather checking them
> > after switching user would make sense since we do the same in
> > worker.c.
> >
>
> LGTM.

Thanks, pushed.

Regards,

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