> 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.
> Actually, leave the other member around, but not granted ownership, and
both create tables, to demonstrate that a non-superuser and non-owner is
unaffected by the trigger.
I've updated the tests accordingly. Please let me know if that's what you
had in mind.
Best regards,
Steve Chavez
On Sun, 20 Apr 2025 at 23:13, David G. Johnston <david.g.johns...@gmail.com>
wrote:
> On Sunday, April 20, 2025, Steve Chavez <st...@supabase.io> wrote:
>
>> > Also, this looks unconventional…
>> > EventTriggerCacheItem *item = (EventTriggerCacheItem*) lfirst_oid(lc);
>>
>> Just noticed the mistake there, I would have expected a compilation
>> error. New patch attached with the following change:
>>
>> EventTriggerCacheItem *item = lfirst(lc);
>>
>> On Sun, 20 Apr 2025 at 22:55, Steve Chavez <st...@supabase.io> wrote:
>>
>>> Sorry, attached the output file.
>>>
>>>
> You can remove role member_1 and trigger..1 and “create table foo” from
> the nosuper script without any loss of test coverage. Or member2 trigger2
> table_bar along with the alter event trigger command which doesn’t need to
> be exercised here. Ownership is all that matters. Whether come to
> directly or via alter.
>
> Actually, leave the other member around, but not granted ownership, and
> both create tables, to demonstrate that a non-superuser and non-owner is
> unaffected by the trigger.
>
> David J.
>
>
From e9a73cda8145a04b8375d21e37a6e713af1bed34 Mon Sep 17 00:00:00 2001
From: steve-chavez <stevechavez...@gmail.com>
Date: Sun, 20 Apr 2025 19:46:00 -0500
Subject: [PATCH v2] Allow regular users to create event triggers
---
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 | 71 +++++++++++++++++++
src/test/regress/parallel_schedule | 4 ++
src/test/regress/sql/event_trigger.sql | 11 +--
.../regress/sql/event_trigger_nosuper.sql | 59 +++++++++++++++
8 files changed, 147 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..e662d5b4f11
--- /dev/null
+++ b/src/test/regress/expected/event_trigger_nosuper.out
@@ -0,0 +1,71 @@
+-- setup roles and privileges
+create role evtrig_owner;
+create role member;
+grant evtrig_owner to member;
+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
+
+-- evtrig should not fire for non-member
+set role non_member;
+create table evtrig_qux();
+\echo
+
+-- altering the event trigger owner to a regular user should work
+reset role;
+select current_setting('is_superuser');
+ current_setting
+-----------------
+ on
+(1 row)
+
+create event trigger evtrig_show_current_user_2
+ on ddl_command_end
+ execute procedure show_current_user();
+alter event trigger evtrig_show_current_user_2 owner to evtrig_owner;
+\echo
+
+-- cleanup
+drop table evtrig_qux;
+drop table evtrig_bar;
+ERROR: table "evtrig_bar" does not exist
+drop table evtrig_foo;
+revoke all on schema public from member, non_member, evtrig_owner;
+drop event trigger evtrig_show_current_user_1;
+drop event trigger evtrig_show_current_user_2;
+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..0ba41089f08
--- /dev/null
+++ b/src/test/regress/sql/event_trigger_nosuper.sql
@@ -0,0 +1,59 @@
+-- setup roles and privileges
+create role evtrig_owner;
+create role member;
+grant evtrig_owner to member;
+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
+
+-- evtrig should not fire for non-member
+set role non_member;
+create table evtrig_qux();
+\echo
+
+-- altering the event trigger owner to a regular user should work
+reset role;
+select current_setting('is_superuser');
+create event trigger evtrig_show_current_user_2
+ on ddl_command_end
+ execute procedure show_current_user();
+alter event trigger evtrig_show_current_user_2 owner to evtrig_owner;
+\echo
+
+-- cleanup
+drop table evtrig_qux;
+drop table evtrig_bar;
+drop table evtrig_foo;
+revoke all on schema public from member, non_member, evtrig_owner;
+drop event trigger evtrig_show_current_user_1;
+drop event trigger evtrig_show_current_user_2;
+drop function show_current_user();
+drop role member;
+drop role non_member;
+drop role evtrig_owner;
--
2.40.1
