Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: support custom notifications#19751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ssncferreira merged 12 commits intomainfromssncferreira/support_custom_notifications
Sep 11, 2025
Merged
Show file tree
Hide file tree
Changes from1 commit
Commits
Show all changes
12 commits
Select commitHold shift + click to select a range
2ab1c4d
feat: support custom notifications
ssncferreiraSep 9, 2025
327ec0d
fix: block system users from sending custom notifications
ssncferreiraSep 10, 2025
0d1144a
fix: cli/notificatins_test.go imports
ssncferreiraSep 10, 2025
203668d
chore: add kind 'custom' type to notification template kind enum
ssncferreiraSep 10, 2025
d3ffd1a
chore: update custom notification request with a content object
ssncferreiraSep 10, 2025
938d010
chore: improve cli description
ssncferreiraSep 10, 2025
9a39693
chore: bypass the per-day notification dedupe
ssncferreiraSep 10, 2025
6b8636d
Merge remote-tracking branch 'origin/main' into ssncferreira/support_…
ssncferreiraSep 10, 2025
80c6b22
fix: fix migration numbers
ssncferreiraSep 10, 2025
657a230
chore: address comments
ssncferreiraSep 11, 2025
681a29f
chore: require both title and message
ssncferreiraSep 11, 2025
14e155e
Merge remote-tracking branch 'origin/main' into ssncferreira/support_…
ssncferreiraSep 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
PrevPrevious commit
NextNext commit
fix: block system users from sending custom notifications
  • Loading branch information
@ssncferreira
ssncferreira committedSep 10, 2025
commit327ec0d1929ab104a67b179253110c816fea1241
45 changes: 42 additions & 3 deletionscli/notifications_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -10,6 +10,9 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbgen"

"github.com/coder/coder/v2/cli/clitest"
"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/notifications"
Expand DownExpand Up@@ -175,11 +178,12 @@ func TestCustomNotifications(t *testing.T) {

notifyEnq := &notificationstest.FakeEnqueuer{}

// Given: A member user
ownerClient := coderdtest.New(t, &coderdtest.Options{
DeploymentValues: coderdtest.DeploymentValues(t),
NotificationsEnqueuer: notifyEnq,
})

// Given: A member user
ownerUser := coderdtest.CreateFirstUser(t, ownerClient)
memberClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, ownerUser.OrganizationID)

Expand All@@ -192,23 +196,58 @@ func TestCustomNotifications(t *testing.T) {
var sdkError *codersdk.Error
require.Error(t, err)
require.ErrorAsf(t, err, &sdkError, "error should be of type *codersdk.Error")
assert.Equal(t, http.StatusBadRequest, sdkError.StatusCode())
require.Equal(t, http.StatusBadRequest, sdkError.StatusCode())
require.Equal(t, "Invalid request body", sdkError.Message)

sent := notifyEnq.Sent(notificationstest.WithTemplateID(notifications.TemplateTestNotification))
require.Len(t, sent, 0)
})

t.Run("SystemUserNotAllowed", func(t *testing.T) {
t.Parallel()

notifyEnq := &notificationstest.FakeEnqueuer{}

ownerClient, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
DeploymentValues: coderdtest.DeploymentValues(t),
NotificationsEnqueuer: notifyEnq,
})

// Given: A system user (prebuilds system user)
_, token := dbgen.APIKey(t, db, database.APIKey{
UserID: database.PrebuildsSystemUserID,
LoginType: database.LoginTypeNone,
})
systemUserClient := codersdk.New(ownerClient.URL)
systemUserClient.SetSessionToken(token)

// When: The system user attempts to send a custom notification
inv, root := clitest.New(t, "notifications", "custom", "Custom Title", "Custom Message")
clitest.SetupConfig(t, systemUserClient, root)

// Then: an error is expected with no notifications sent
err := inv.Run()
var sdkError *codersdk.Error
require.Error(t, err)
require.ErrorAsf(t, err, &sdkError, "error should be of type *codersdk.Error")
require.Equal(t, http.StatusForbidden, sdkError.StatusCode())
require.Equal(t, "Forbidden", sdkError.Message)

sent := notifyEnq.Sent(notificationstest.WithTemplateID(notifications.TemplateTestNotification))
require.Len(t, sent, 0)
})

t.Run("Success", func(t *testing.T) {
t.Parallel()

notifyEnq := &notificationstest.FakeEnqueuer{}

// Given: A member user
ownerClient := coderdtest.New(t, &coderdtest.Options{
DeploymentValues: coderdtest.DeploymentValues(t),
NotificationsEnqueuer: notifyEnq,
})

// Given: A member user
ownerUser := coderdtest.CreateFirstUser(t, ownerClient)
memberClient, memberUser := coderdtest.CreateAnotherUser(t, ownerClient, ownerUser.OrganizationID)

Expand Down
6 changes: 6 additions & 0 deletionscoderd/apidoc/docs.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.

6 changes: 6 additions & 0 deletionscoderd/apidoc/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.

4 changes: 4 additions & 0 deletionscoderd/database/modelmethods.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -376,6 +376,10 @@ func (u User) RBACObject() rbac.Object {
return rbac.ResourceUserObject(u.ID)
}

func (u User) IsSystemUser() bool {
return u.IsSystem
}

func (u GetUsersRow) RBACObject() rbac.Object {
return rbac.ResourceUserObject(u.ID)
}
Expand Down
26 changes: 23 additions & 3 deletionscoderd/notifications.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -333,6 +333,7 @@ func (api *API) putUserNotificationPreferences(rw http.ResponseWriter, r *http.R
// @Param request body codersdk.CustomNotification true "Provide a non-empty title or message"
// @Success 204 "No Content"
// @Failure 400 {object} codersdk.Response "Invalid request body"
// @Failure 403 {object} codersdk.Response "System users cannot send custom notifications"
// @Failure 500 {object} codersdk.Response "Failed to send custom notification"
// @Router /notifications/custom [post]
func (api *API) postCustomNotification(rw http.ResponseWriter, r *http.Request) {
Expand All@@ -357,17 +358,36 @@ func (api *API) postCustomNotification(rw http.ResponseWriter, r *http.Request)
return
}

userID := apiKey.UserID
// Block system users from sending custom notifications
user, err := api.Database.GetUserByID(ctx, apiKey.UserID)
if err != nil {
api.Logger.Error(ctx, "send custom notification", slog.Error(err))
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Failed to send custom notification",
Detail: err.Error(),
})
return
}
if user.IsSystemUser() {
api.Logger.Error(ctx, "send custom notification: system user is not allowed",
slog.F("id", user.ID.String()), slog.F("name", user.Name))
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
Message: "Forbidden",
Detail: "System users cannot send custom notifications.",
})
return
}

if _, err := api.NotificationsEnqueuer.Enqueue(
Copy link
ContributorAuthor

@ssncferreirassncferreiraSep 10, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Note: Custom notifications may have the same issue as test notifications, if a user sends the same notification multiple times in a single day, our deduping will treat later sends as duplicates. Changing this could also mean circumventing a measure intended to reduce noise and prevent spam. For now this shouldn’t be a problem since users can only send notifications to themselves, but that might change in the future.

Should we allow multiple sends of the same custom notification within a single day, or keep the current deduping to limit noise/spam?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

That's a good point. I would suggest:

  • For requestors sending a notification to themselves, do not dedupe OR reduce the deduplication timeframe to something lower like 1 minute.
  • For requestors sending a notification to others, we should enforce similar notification limits to prevent noise.

ssncferreira reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If a user has some script that alerts them of completion of a task, it might make sense for their notification to have the same content each time. It would definitely be surprising for them tonot receive the second one.

If we keep the de-duplicate behavior then we definitely need to ensure this behavior is very obviously documented. We could document a workaround that the user can do by injecting a timestamp in the message.

I'm not opposed to us bypassing the de-duplication logic and just ensuring this endpoint has a rate limit? Would like to hear other peoples thoughts on that though.

ssncferreira reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@johnstcn@DanielleMaywood Thank you for the feedback. I agree that for the custom notification use case, it might be common to send the same message multiple times in a day, so it makes sense not to dedupe those. Since this feature will evolve to support multiple recipients, I like the idea of not deduping for the requesting user while deduping for other recipients.
For now, I updated the code to bypass deduplication by adding a timestamp to the payload (same as in test notifications).
We might be able to updatecompute_notification_message_dedupe_hash to produce a different hash when it’s a custom notification and the sender is also the recipient.

I created a follow-up issue to track multi-recipient support:#19768

//nolint:gocritic // We need to be notifier to send the notification.
dbauthz.AsNotifier(ctx),
userID,
user.ID,
notifications.TemplateCustomNotification,
map[string]string{
"custom_title": req.Title,
"custom_message": req.Message,
},
userID.String(),
user.ID.String(),
); err != nil {
api.Logger.Error(ctx, "send custom notification", slog.Error(err))
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Expand Down
40 changes: 40 additions & 0 deletionscoderd/notifications_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -11,6 +11,7 @@ import (

"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbgen"
"github.com/coder/coder/v2/coderd/notifications"
"github.com/coder/coder/v2/coderd/notifications/notificationstest"
"github.com/coder/coder/v2/codersdk"
Expand DownExpand Up@@ -407,6 +408,45 @@ func TestCustomNotification(t *testing.T) {
require.ErrorAsf(t, err, &sdkError, "error should be of type *codersdk.Error")
require.Equal(t, http.StatusBadRequest, sdkError.StatusCode())
require.Equal(t, "Invalid request body", sdkError.Message)

sent := notifyEnq.Sent(notificationstest.WithTemplateID(notifications.TemplateTestNotification))
require.Len(t, sent, 0)
})

t.Run("SystemUserNotAllowed", func(t *testing.T) {
t.Parallel()

ctx := testutil.Context(t, testutil.WaitShort)

notifyEnq := &notificationstest.FakeEnqueuer{}
ownerClient, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
DeploymentValues: coderdtest.DeploymentValues(t),
NotificationsEnqueuer: notifyEnq,
})

// Given: A system user (prebuilds system user)
_, token := dbgen.APIKey(t, db, database.APIKey{
UserID: database.PrebuildsSystemUserID,
LoginType: database.LoginTypeNone,
})
systemUserClient := codersdk.New(ownerClient.URL)
systemUserClient.SetSessionToken(token)

// When: The system user attempts to send a custom notification
err := systemUserClient.PostCustomNotification(ctx, codersdk.CustomNotification{
Title: "Custom Title",
Message: "Custom Message",
})

// Then: a forbidden error is expected with no notifications sent
var sdkError *codersdk.Error
require.Error(t, err)
require.ErrorAsf(t, err, &sdkError, "error should be of type *codersdk.Error")
require.Equal(t, http.StatusForbidden, sdkError.StatusCode())
require.Equal(t, "Forbidden", sdkError.Message)

sent := notifyEnq.Sent(notificationstest.WithTemplateID(notifications.TemplateTestNotification))
require.Len(t, sent, 0)
})

t.Run("Success", func(t *testing.T) {
Expand Down
11 changes: 6 additions & 5 deletionsdocs/reference/api/notifications.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.

Loading
[8]ページ先頭
©2009-2025 Movatter.jp