> 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 Tue, 22 Apr 2025 at 20:18, David G. Johnston <david.g.johns...@gmail.com>
wrote:
> On Tuesday, April 22, 2025, David G. Johnston <david.g.johns...@gmail.com>
> wrote:
>
>> On Tuesday, April 22, 2025, David G. Johnston <david.g.johns...@gmail.com>
>> wrote:
>>
>>> On Tuesday, April 22, 2025, Steve Chavez <st...@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.
>
>
From e9e294a638d55458adb6d08c9d2ce22e0c41f267 Mon Sep 17 00:00:00 2001
From: steve-chavez <stevechavez...@gmail.com>
Date: Sun, 20 Apr 2025 19:46:00 -0500
Subject: [PATCH v3] Allow regular users to create event triggers
* fix bad drop table
* remove alter owner test
* add test for not allowing member to drop evtrig
---
src/backend/commands/event_trigger.c | 33 +++-------
src/backend/utils/cache/evtcache.c | 1 +
src/include/utils/evtcache.h | 1 +
src/test/regress/expected/event_trigger.out | 13 +---
.../expected/event_trigger_nosuper.out | 60 +++++++++++++++++++
src/test/regress/parallel_schedule | 4 ++
src/test/regress/sql/event_trigger.sql | 11 +---
.../regress/sql/event_trigger_nosuper.sql | 53 ++++++++++++++++
8 files changed, 130 insertions(+), 46 deletions(-)
create mode 100644 src/test/regress/expected/event_trigger_nosuper.out
create mode 100644 src/test/regress/sql/event_trigger_nosuper.sql
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index edc2c988e29..7ae0636f7c2 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -126,18 +126,6 @@ CreateEventTrigger(CreateEventTrigStmt *stmt)
ListCell *lc;
List *tags = NULL;
- /*
- * It would be nice to allow database owners or even regular users to do
- * this, but there are obvious privilege escalation risks which would have
- * to somehow be plugged first.
- */
- if (!superuser())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to create event trigger \"%s\"",
- stmt->trigname),
- errhint("Must be superuser to create an event trigger.")));
-
/* Validate event name. */
if (strcmp(stmt->eventname, "ddl_command_start") != 0 &&
strcmp(stmt->eventname, "ddl_command_end") != 0 &&
@@ -545,14 +533,6 @@ AlterEventTriggerOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_EVENT_TRIGGER,
NameStr(form->evtname));
- /* New owner must be a superuser */
- if (!superuser_arg(newOwnerId))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to change owner of event trigger \"%s\"",
- NameStr(form->evtname)),
- errhint("The owner of an event trigger must be a superuser.")));
-
form->evtowner = newOwnerId;
CatalogTupleUpdate(rel, &tup->t_self, tup);
@@ -698,7 +678,7 @@ EventTriggerCommonSetup(Node *parsetree,
if (unfiltered || filter_event_trigger(tag, item))
{
/* We must plan to fire this trigger. */
- runlist = lappend_oid(runlist, item->fnoid);
+ runlist = lappend(runlist, item);
}
}
@@ -1084,11 +1064,16 @@ EventTriggerInvoke(List *fn_oid_list, EventTriggerData *trigdata)
foreach(lc, fn_oid_list)
{
LOCAL_FCINFO(fcinfo, 0);
- Oid fnoid = lfirst_oid(lc);
+ EventTriggerCacheItem *item = lfirst(lc);
FmgrInfo flinfo;
PgStat_FunctionCallUsage fcusage;
+ Oid current_user = GetUserId();
+
+ if (!is_member_of_role_nosuper(current_user, item->owneroid)) {
+ continue;
+ }
- elog(DEBUG1, "EventTriggerInvoke %u", fnoid);
+ elog(DEBUG1, "EventTriggerInvoke %u", item->fnoid);
/*
* We want each event trigger to be able to see the results of the
@@ -1102,7 +1087,7 @@ EventTriggerInvoke(List *fn_oid_list, EventTriggerData *trigdata)
CommandCounterIncrement();
/* Look up the function */
- fmgr_info(fnoid, &flinfo);
+ fmgr_info(item->fnoid, &flinfo);
/* Call the function, passing no arguments but setting a context. */
InitFunctionCallInfoData(*fcinfo, &flinfo, 0,
diff --git a/src/backend/utils/cache/evtcache.c b/src/backend/utils/cache/evtcache.c
index ce596bf5638..1dc9a864034 100644
--- a/src/backend/utils/cache/evtcache.c
+++ b/src/backend/utils/cache/evtcache.c
@@ -175,6 +175,7 @@ BuildEventTriggerCache(void)
item = palloc0(sizeof(EventTriggerCacheItem));
item->fnoid = form->evtfoid;
item->enabled = form->evtenabled;
+ item->owneroid = form->evtowner;
/* Decode and sort tags array. */
evttags = heap_getattr(tup, Anum_pg_event_trigger_evttags,
diff --git a/src/include/utils/evtcache.h b/src/include/utils/evtcache.h
index 9d9fcb8657b..0ecd6a54c5f 100644
--- a/src/include/utils/evtcache.h
+++ b/src/include/utils/evtcache.h
@@ -31,6 +31,7 @@ typedef struct
Oid fnoid; /* function to be called */
char enabled; /* as SESSION_REPLICATION_ROLE_* */
Bitmapset *tagset; /* command tags, or NULL if empty */
+ Oid owneroid; /* owner of the event trigger */
} EventTriggerCacheItem;
extern List *EventCacheLookup(EventTriggerEvent event);
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 7b2198eac6f..0ca16ec5e38 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -84,14 +84,6 @@ create event trigger regress_event_trigger2 on ddl_command_start
execute procedure test_event_trigger();
-- OK
comment on event trigger regress_event_trigger is 'test comment';
--- drop as non-superuser should fail
-create role regress_evt_user;
-set role regress_evt_user;
-create event trigger regress_event_trigger_noperms on ddl_command_start
- execute procedure test_event_trigger();
-ERROR: permission denied to create event trigger "regress_event_trigger_noperms"
-HINT: Must be superuser to create an event trigger.
-reset role;
-- test enabling and disabling
alter event trigger regress_event_trigger disable;
-- fires _trigger2 and _trigger_end should fire, but not _trigger
@@ -168,15 +160,12 @@ create foreign data wrapper useless;
NOTICE: test_event_trigger: ddl_command_end CREATE FOREIGN DATA WRAPPER
create server useless_server foreign data wrapper useless;
NOTICE: test_event_trigger: ddl_command_end CREATE SERVER
+create role regress_evt_user;
create user mapping for regress_evt_user server useless_server;
NOTICE: test_event_trigger: ddl_command_end CREATE USER MAPPING
alter default privileges for role regress_evt_user
revoke delete on tables from regress_evt_user;
NOTICE: test_event_trigger: ddl_command_end ALTER DEFAULT PRIVILEGES
--- alter owner to non-superuser should fail
-alter event trigger regress_event_trigger owner to regress_evt_user;
-ERROR: permission denied to change owner of event trigger "regress_event_trigger"
-HINT: The owner of an event trigger must be a superuser.
-- alter owner to superuser should work
alter role regress_evt_user superuser;
alter event trigger regress_event_trigger owner to regress_evt_user;
diff --git a/src/test/regress/expected/event_trigger_nosuper.out b/src/test/regress/expected/event_trigger_nosuper.out
new file mode 100644
index 00000000000..86d8f8b2da3
--- /dev/null
+++ b/src/test/regress/expected/event_trigger_nosuper.out
@@ -0,0 +1,60 @@
+-- setup roles and privileges
+create role evtrig_owner;
+create role member;
+grant evtrig_owner to member with inherit false;
+create role non_member;
+grant all on schema public to evtrig_owner, member, non_member;
+\echo
+
+-- create nonsuper event trigger
+set role evtrig_owner;
+create function show_current_user()
+ returns event_trigger
+ language plpgsql as
+$$
+begin
+ raise notice 'the event trigger is executed for %', current_user;
+end;
+$$;
+create event trigger evtrig_show_current_user_1
+ on ddl_command_start
+ execute procedure show_current_user();
+reset role;
+\echo
+
+-- evtrig should not fire for superusers
+select current_setting('is_superuser');
+ current_setting
+-----------------
+ on
+(1 row)
+
+create table evtrig_quux();
+\echo
+
+-- evtrig should fire for member
+set role member;
+create table evtrig_foo();
+NOTICE: the event trigger is executed for member
+\echo
+
+-- member is not allowed to drop the evtrig
+drop event trigger evtrig_show_current_user_1;
+ERROR: must be owner of event trigger evtrig_show_current_user_1
+\echo
+
+-- evtrig should not fire for non-member
+set role non_member;
+create table evtrig_qux();
+\echo
+
+-- cleanup
+reset role;
+drop table evtrig_qux;
+drop table evtrig_foo;
+revoke all on schema public from member, non_member, evtrig_owner;
+drop event trigger evtrig_show_current_user_1;
+drop function show_current_user();
+drop role member;
+drop role non_member;
+drop role evtrig_owner;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index a424be2a6bf..fd3ef719f5e 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -130,6 +130,10 @@ test: partition_join partition_prune reloptions hash_part indexing partition_agg
# oidjoins is read-only, though, and should run late for best coverage
test: oidjoins event_trigger
+# event_trigger_nosuper cannot run concurrently
+# with other tests that runs DDL
+test: event_trigger_nosuper
+
# event_trigger_login cannot run concurrently with any other tests because
# on-login event handling could catch connection of a concurrent test.
test: event_trigger_login
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index 013546b8305..6833e991762 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -85,13 +85,6 @@ create event trigger regress_event_trigger2 on ddl_command_start
-- OK
comment on event trigger regress_event_trigger is 'test comment';
--- drop as non-superuser should fail
-create role regress_evt_user;
-set role regress_evt_user;
-create event trigger regress_event_trigger_noperms on ddl_command_start
- execute procedure test_event_trigger();
-reset role;
-
-- test enabling and disabling
alter event trigger regress_event_trigger disable;
-- fires _trigger2 and _trigger_end should fire, but not _trigger
@@ -139,13 +132,11 @@ revoke all on table event_trigger_fire1 from public;
drop table event_trigger_fire1;
create foreign data wrapper useless;
create server useless_server foreign data wrapper useless;
+create role regress_evt_user;
create user mapping for regress_evt_user server useless_server;
alter default privileges for role regress_evt_user
revoke delete on tables from regress_evt_user;
--- alter owner to non-superuser should fail
-alter event trigger regress_event_trigger owner to regress_evt_user;
-
-- alter owner to superuser should work
alter role regress_evt_user superuser;
alter event trigger regress_event_trigger owner to regress_evt_user;
diff --git a/src/test/regress/sql/event_trigger_nosuper.sql b/src/test/regress/sql/event_trigger_nosuper.sql
new file mode 100644
index 00000000000..70e5e82a7cb
--- /dev/null
+++ b/src/test/regress/sql/event_trigger_nosuper.sql
@@ -0,0 +1,53 @@
+-- setup roles and privileges
+create role evtrig_owner;
+create role member;
+grant evtrig_owner to member with inherit false;
+create role non_member;
+grant all on schema public to evtrig_owner, member, non_member;
+\echo
+
+-- create nonsuper event trigger
+set role evtrig_owner;
+create function show_current_user()
+ returns event_trigger
+ language plpgsql as
+$$
+begin
+ raise notice 'the event trigger is executed for %', current_user;
+end;
+$$;
+create event trigger evtrig_show_current_user_1
+ on ddl_command_start
+ execute procedure show_current_user();
+reset role;
+\echo
+
+-- evtrig should not fire for superusers
+select current_setting('is_superuser');
+create table evtrig_quux();
+\echo
+
+-- evtrig should fire for member
+set role member;
+create table evtrig_foo();
+\echo
+
+-- member is not allowed to drop the evtrig
+drop event trigger evtrig_show_current_user_1;
+\echo
+
+-- evtrig should not fire for non-member
+set role non_member;
+create table evtrig_qux();
+\echo
+
+-- cleanup
+reset role;
+drop table evtrig_qux;
+drop table evtrig_foo;
+revoke all on schema public from member, non_member, evtrig_owner;
+drop event trigger evtrig_show_current_user_1;
+drop function show_current_user();
+drop role member;
+drop role non_member;
+drop role evtrig_owner;
--
2.42.0
