> You have a bad drop table cleanup command, and I’d drop the entire alter event trigger owner test.
My bad, removed the bad drop table and also removed the alter owner test.
> The other thing I’m wondering, but haven’t gotten around to testing, is whether a role affected by the event trigger is able to just drop the trigger given this implementation. I always get member/member-of dynamics confused. Having member (possibly via set role…) trying to drop the trigger would be good to prove that it isn’t allowed.
So this one is a problem. If we do `grant evtrig_owner to member` then `member` will be able to drop the event trigger, which is no good. There are 2 option to solve this:
1. Do `grant evtrig_owner to member with inherit false`, then `member` will not be able to drop the event trigger. I've updated the tests to reflect that.
2. `create role member noinherit`, which won't let `member` drop the event trigger with a regular `grant evtrig_owner to member`. But this change is more invasive towards existing roles.
That being said, it's not a good default behavior to let evtrigger targets to drop the evtrigger. Should we enforce that only granting with inherit false will make the role members targets of the evtrigger? Are there any other options?
On Tuesday, April 22, 2025, David G. Johnston <david.g.johnston@gmail.com> wrote:
On Tuesday, April 22, 2025, David G. Johnston <david.g.johnston@gmail.com> wrote:
On Tuesday, April 22, 2025, Steve Chavez <steve@supabase.io> wrote:
> alter event trigger command which doesn’t need to be exercised here
That part does need to be tested, I modified `AlterEventTriggerOwner_internal` to allow altering owners to regular users. Before it was only restricted to superusers.
Ok. I missed this.
Sorry for the self-reply but this nagged at me.
It’s probably not a big deal either way, but the prior test existed to ensure that a superuser couldn’t do something they are otherwise are always permitted to do - assign object to whomever they wish. So event_trigger.sql had a test that errored showing this anomaly. You moved the test and now are proving it doesn’t error. But it is not expected to error; and immediately above you already show that a non-superuser can be an owner. We don’t need a test to show a superuser demonstrating their normal abilities.
IOW, select test cases around the feature as it is implemented now, not its history. A personal one-off test to ensure that no super-user prohibitions remained will suffice to make sure all such code that needed to be removed is gone.
David J.
