From 0cd3be31b047f62c181e57c491d09418e18607c0 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 25 Mar 2025 16:01:30 +0000 Subject: [PATCH 1/4] chore: disallow inbox as default method --- coderd/notifications/enqueuer.go | 14 ++++++++++++-- coderd/notifications/notifications_test.go | 16 ++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/coderd/notifications/enqueuer.go b/coderd/notifications/enqueuer.go index b93a05aa96a1e..b9c95bd066433 100644 --- a/coderd/notifications/enqueuer.go +++ b/coderd/notifications/enqueuer.go @@ -3,6 +3,7 @@ package notifications import ( "context" "encoding/json" + "fmt" "slices" "strings" "text/template" @@ -25,6 +26,14 @@ var ( ErrDuplicate = xerrors.New("duplicate notification") ) +type InvalidNotificationMethodError struct { + Method string +} + +func (e InvalidNotificationMethodError) Error() string { + return fmt.Sprintf("given notification method %q is invalid", e.Method) +} + type StoreEnqueuer struct { store Store log slog.Logger @@ -43,8 +52,9 @@ type StoreEnqueuer struct { // NewStoreEnqueuer creates an Enqueuer implementation which can persist notification messages in the store. func NewStoreEnqueuer(cfg codersdk.NotificationsConfig, store Store, helpers template.FuncMap, log slog.Logger, clock quartz.Clock) (*StoreEnqueuer, error) { var method database.NotificationMethod - if err := method.Scan(cfg.Method.String()); err != nil { - return nil, xerrors.Errorf("given notification method %q is invalid", cfg.Method) + // We do not allow setting Coder Inbox as the default method. + if err := method.Scan(cfg.Method.String()); err != nil || method == database.NotificationMethodInbox { + return nil, InvalidNotificationMethodError{Method: cfg.Method.String()} } return &StoreEnqueuer{ diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index 348185ef516f1..a91cfb97c7f1f 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -1856,6 +1856,22 @@ func TestNotificationDuplicates(t *testing.T) { require.NoError(t, err) } +func TestNotificationMethodCannotDefaultToInbox(t *testing.T) { + t.Parallel() + + store, _ := dbtestutil.NewDB(t) + logger := testutil.Logger(t) + + cfg := defaultNotificationsConfig(database.NotificationMethodInbox) + + // Set the time to a known value. + mClock := quartz.NewMock(t) + mClock.Set(time.Date(2024, 1, 15, 9, 0, 0, 0, time.UTC)) + + _, err := notifications.NewStoreEnqueuer(cfg, store, defaultHelpers(), logger.Named("enqueuer"), mClock) + require.ErrorIs(t, err, notifications.InvalidNotificationMethodError{Method: string(database.NotificationMethodInbox)}) +} + func TestNotificationTargetMatrix(t *testing.T) { t.Parallel() From c9df480302cbac610c7cc970c6fb120a6df11ea8 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 25 Mar 2025 17:52:58 +0000 Subject: [PATCH 2/4] chore: update error type name --- coderd/notifications/enqueuer.go | 8 ++++---- coderd/notifications/notifications_test.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/coderd/notifications/enqueuer.go b/coderd/notifications/enqueuer.go index b9c95bd066433..3e8bafa33679e 100644 --- a/coderd/notifications/enqueuer.go +++ b/coderd/notifications/enqueuer.go @@ -26,12 +26,12 @@ var ( ErrDuplicate = xerrors.New("duplicate notification") ) -type InvalidNotificationMethodError struct { +type InvalidDefaultNotificationMethodError struct { Method string } -func (e InvalidNotificationMethodError) Error() string { - return fmt.Sprintf("given notification method %q is invalid", e.Method) +func (e InvalidDefaultNotificationMethodError) Error() string { + return fmt.Sprintf("given default notification method %q is invalid", e.Method) } type StoreEnqueuer struct { @@ -54,7 +54,7 @@ func NewStoreEnqueuer(cfg codersdk.NotificationsConfig, store Store, helpers tem var method database.NotificationMethod // We do not allow setting Coder Inbox as the default method. if err := method.Scan(cfg.Method.String()); err != nil || method == database.NotificationMethodInbox { - return nil, InvalidNotificationMethodError{Method: cfg.Method.String()} + return nil, InvalidDefaultNotificationMethodError{Method: cfg.Method.String()} } return &StoreEnqueuer{ diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index a91cfb97c7f1f..ca93212add460 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -1869,7 +1869,7 @@ func TestNotificationMethodCannotDefaultToInbox(t *testing.T) { mClock.Set(time.Date(2024, 1, 15, 9, 0, 0, 0, time.UTC)) _, err := notifications.NewStoreEnqueuer(cfg, store, defaultHelpers(), logger.Named("enqueuer"), mClock) - require.ErrorIs(t, err, notifications.InvalidNotificationMethodError{Method: string(database.NotificationMethodInbox)}) + require.ErrorIs(t, err, notifications.InvalidDefaultNotificationMethodError{Method: string(database.NotificationMethodInbox)}) } func TestNotificationTargetMatrix(t *testing.T) { From d7f64b72d7d5133585297466935c57592e181ad0 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 25 Mar 2025 17:53:11 +0000 Subject: [PATCH 3/4] chore: add todo comment --- coderd/notifications/enqueuer.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/coderd/notifications/enqueuer.go b/coderd/notifications/enqueuer.go index 3e8bafa33679e..7692bbd85ce07 100644 --- a/coderd/notifications/enqueuer.go +++ b/coderd/notifications/enqueuer.go @@ -52,7 +52,11 @@ type StoreEnqueuer struct { // NewStoreEnqueuer creates an Enqueuer implementation which can persist notification messages in the store. func NewStoreEnqueuer(cfg codersdk.NotificationsConfig, store Store, helpers template.FuncMap, log slog.Logger, clock quartz.Clock) (*StoreEnqueuer, error) { var method database.NotificationMethod - // We do not allow setting Coder Inbox as the default method. + // TODO(DanielleMaywood): + // Currently we do not want to allow setting `inbox` as the default notification method. + // As of 2025-03-25, setting this to `inbox` would cause a crash on the deployment + // notification settings page. Until we make a future decision on this we want to disallow + // setting it. if err := method.Scan(cfg.Method.String()); err != nil || method == database.NotificationMethodInbox { return nil, InvalidDefaultNotificationMethodError{Method: cfg.Method.String()} } From 3c26ced5e092d7afb4a03625735504d099cafff9 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 25 Mar 2025 17:53:51 +0000 Subject: [PATCH 4/4] chore: remove old test code --- coderd/notifications/notifications_test.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index ca93212add460..9bf31384234ed 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -1864,11 +1864,7 @@ func TestNotificationMethodCannotDefaultToInbox(t *testing.T) { cfg := defaultNotificationsConfig(database.NotificationMethodInbox) - // Set the time to a known value. - mClock := quartz.NewMock(t) - mClock.Set(time.Date(2024, 1, 15, 9, 0, 0, 0, time.UTC)) - - _, err := notifications.NewStoreEnqueuer(cfg, store, defaultHelpers(), logger.Named("enqueuer"), mClock) + _, err := notifications.NewStoreEnqueuer(cfg, store, defaultHelpers(), logger.Named("enqueuer"), quartz.NewMock(t)) require.ErrorIs(t, err, notifications.InvalidDefaultNotificationMethodError{Method: string(database.NotificationMethodInbox)}) }