From 6a5459141f6144904d30fc74686b1bd6963b92d8 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 8 Jan 2025 14:37:23 +0000 Subject: [PATCH 1/5] fix: send workspace create/update notifications to template admins only --- coderd/workspacebuilds.go | 22 +++++++++++++-- coderd/workspacebuilds_test.go | 51 +++++++++++++++++++++++----------- coderd/workspaces.go | 25 ++++++++++++++--- coderd/workspaces_test.go | 37 +++++++++++++++++------- 4 files changed, 103 insertions(+), 32 deletions(-) diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 7ee6398d1d857..802ecd1ec0954 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -446,7 +446,24 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { // If this workspace build has a different template version ID to the previous build // we can assume it has just been updated. if createBuild.TemplateVersionID != uuid.Nil && createBuild.TemplateVersionID != previousWorkspaceBuild.TemplateVersionID { - api.notifyWorkspaceUpdated(ctx, apiKey.UserID, workspace, createBuild.RichParameterValues) + // nolint:gocritic // Need system context to fetch admins + admins, err := findTemplateAdmins(dbauthz.AsSystemRestricted(ctx), api.Database) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching template admins.", + Detail: err.Error(), + }) + return + } + + for _, admin := range admins { + // Don't send notifications to user which initiated the event. + if admin.ID == apiKey.UserID { + continue + } + + api.notifyWorkspaceUpdated(ctx, apiKey.UserID, admin.ID, workspace, createBuild.RichParameterValues) + } } api.publishWorkspaceUpdate(ctx, workspace.OwnerID, wspubsub.WorkspaceEvent{ @@ -460,6 +477,7 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { func (api *API) notifyWorkspaceUpdated( ctx context.Context, initiatorID uuid.UUID, + receiverID uuid.UUID, workspace database.Workspace, parameters []codersdk.WorkspaceBuildParameter, ) { @@ -500,7 +518,7 @@ func (api *API) notifyWorkspaceUpdated( if _, err := api.NotificationsEnqueuer.EnqueueWithData( // nolint:gocritic // Need notifier actor to enqueue notifications dbauthz.AsNotifier(ctx), - workspace.OwnerID, + receiverID, notifications.TemplateWorkspaceManuallyUpdated, map[string]string{ "organization": template.OrganizationName, diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 43674b308583c..f2d4c4b741002 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -565,19 +565,20 @@ func TestWorkspaceBuildResources(t *testing.T) { func TestWorkspaceBuildWithUpdatedTemplateVersionSendsNotification(t *testing.T) { t.Parallel() - t.Run("OnlyOneNotification", func(t *testing.T) { + t.Run("NoRepeatedNotifications", func(t *testing.T) { t.Parallel() notify := ¬ificationstest.FakeEnqueuer{} client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true, NotificationsEnqueuer: notify}) first := coderdtest.CreateFirstUser(t, client) + templateAdminClient, templateAdmin := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleTemplateAdmin()) userClient, user := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) // Create a template with an initial version - version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID) + version := coderdtest.CreateTemplateVersion(t, templateAdminClient, first.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdminClient, version.ID) + template := coderdtest.CreateTemplate(t, templateAdminClient, first.OrganizationID, version.ID) // Create a workspace using this template workspace := coderdtest.CreateWorkspace(t, userClient, template.ID) @@ -585,10 +586,10 @@ func TestWorkspaceBuildWithUpdatedTemplateVersionSendsNotification(t *testing.T) coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) // Create a new version of the template - newVersion := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil, func(ctvr *codersdk.CreateTemplateVersionRequest) { + newVersion := coderdtest.CreateTemplateVersion(t, templateAdminClient, first.OrganizationID, nil, func(ctvr *codersdk.CreateTemplateVersionRequest) { ctvr.TemplateID = template.ID }) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, newVersion.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdminClient, newVersion.ID) // Create a workspace build using this new template version build := coderdtest.CreateWorkspaceBuild(t, userClient, workspace, database.WorkspaceTransitionStart, func(cwbr *codersdk.CreateWorkspaceBuildRequest) { @@ -604,14 +605,31 @@ func TestWorkspaceBuildWithUpdatedTemplateVersionSendsNotification(t *testing.T) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, build.ID) coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) - // Ensure we receive only 1 workspace manually updated notification + // We're going to have two notifications (one for owner and one for the template admin) + // By ensuring we only have these two, we are sure the second build didn't trigger more + // notifications. sent := notify.Sent(notificationstest.WithTemplateID(notifications.TemplateWorkspaceManuallyUpdated)) - require.Len(t, sent, 1) - require.Equal(t, user.ID, sent[0].UserID) + require.Len(t, sent, 2) + + receivers := make([]uuid.UUID, len(sent)) + for idx, notif := range sent { + receivers[idx] = notif.UserID + } + + // Check the notification was sent to the owner and template admin + require.Contains(t, receivers, templateAdmin.ID) + require.Contains(t, receivers, first.UserID) + require.NotContains(t, receivers, user.ID) + require.Contains(t, sent[0].Targets, template.ID) require.Contains(t, sent[0].Targets, workspace.ID) require.Contains(t, sent[0].Targets, workspace.OrganizationID) require.Contains(t, sent[0].Targets, workspace.OwnerID) + + require.Contains(t, sent[1].Targets, template.ID) + require.Contains(t, sent[1].Targets, workspace.ID) + require.Contains(t, sent[1].Targets, workspace.OrganizationID) + require.Contains(t, sent[1].Targets, workspace.OwnerID) }) t.Run("ToCorrectUser", func(t *testing.T) { @@ -621,12 +639,13 @@ func TestWorkspaceBuildWithUpdatedTemplateVersionSendsNotification(t *testing.T) client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true, NotificationsEnqueuer: notify}) first := coderdtest.CreateFirstUser(t, client) - userClient, user := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) + templateAdminClient, templateAdmin := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleTemplateAdmin()) + userClient, _ := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) // Create a template with an initial version - version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID) + version := coderdtest.CreateTemplateVersion(t, templateAdminClient, first.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdminClient, version.ID) + template := coderdtest.CreateTemplate(t, templateAdminClient, first.OrganizationID, version.ID) // Create a workspace using this template workspace := coderdtest.CreateWorkspace(t, userClient, template.ID) @@ -634,10 +653,10 @@ func TestWorkspaceBuildWithUpdatedTemplateVersionSendsNotification(t *testing.T) coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) // Create a new version of the template - newVersion := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil, func(ctvr *codersdk.CreateTemplateVersionRequest) { + newVersion := coderdtest.CreateTemplateVersion(t, templateAdminClient, first.OrganizationID, nil, func(ctvr *codersdk.CreateTemplateVersionRequest) { ctvr.TemplateID = template.ID }) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, newVersion.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdminClient, newVersion.ID) // Create a workspace build using this new template version from a different user ctx := testutil.Context(t, testutil.WaitShort) @@ -652,7 +671,7 @@ func TestWorkspaceBuildWithUpdatedTemplateVersionSendsNotification(t *testing.T) // Ensure we receive only 1 workspace manually updated notification and to the right user sent := notify.Sent(notificationstest.WithTemplateID(notifications.TemplateWorkspaceManuallyUpdated)) require.Len(t, sent, 1) - require.Equal(t, user.ID, sent[0].UserID) + require.Equal(t, templateAdmin.ID, sent[0].UserID) require.Contains(t, sent[0].Targets, template.ID) require.Contains(t, sent[0].Targets, workspace.ID) require.Contains(t, sent[0].Targets, workspace.OrganizationID) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 19fb1ec1ce810..9de7050777305 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -665,9 +665,6 @@ func createWorkspace( ) return err }, nil) - - api.notifyWorkspaceCreated(ctx, workspace, req.RichParameterValues) - var bldErr wsbuilder.BuildError if xerrors.As(err, &bldErr) { httpapi.Write(ctx, rw, bldErr.Status, codersdk.Response{ @@ -689,6 +686,25 @@ func createWorkspace( api.Logger.Error(ctx, "failed to post provisioner job to pubsub", slog.Error(err)) } + // nolint:gocritic // Need system context to fetch admins + admins, err := findTemplateAdmins(dbauthz.AsSystemRestricted(ctx), api.Database) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching template admins.", + Detail: err.Error(), + }) + return + } + + for _, admin := range admins { + // Don't send notifications to user which initiated the event. + if admin.ID == initiatorID { + continue + } + + api.notifyWorkspaceCreated(ctx, admin.ID, workspace, req.RichParameterValues) + } + auditReq.New = workspace.WorkspaceTable() api.Telemetry.Report(&telemetry.Snapshot{ @@ -739,6 +755,7 @@ func createWorkspace( func (api *API) notifyWorkspaceCreated( ctx context.Context, + receiverID uuid.UUID, workspace database.Workspace, parameters []codersdk.WorkspaceBuildParameter, ) { @@ -773,7 +790,7 @@ func (api *API) notifyWorkspaceCreated( if _, err := api.NotificationsEnqueuer.EnqueueWithData( // nolint:gocritic // Need notifier actor to enqueue notifications dbauthz.AsNotifier(ctx), - workspace.OwnerID, + receiverID, notifications.TemplateWorkspaceCreated, map[string]string{ "workspace": workspace.Name, diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index d6e365011b929..6a4222fe5e500 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -577,22 +577,38 @@ func TestPostWorkspacesByOrganization(t *testing.T) { enqueuer := notificationstest.FakeEnqueuer{} client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true, NotificationsEnqueuer: &enqueuer}) user := coderdtest.CreateFirstUser(t, client) + templateAdminClient, templateAdmin := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleTemplateAdmin()) memberClient, memberUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) - version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + version := coderdtest.CreateTemplateVersion(t, templateAdminClient, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, templateAdminClient, user.OrganizationID, version.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdminClient, version.ID) workspace := coderdtest.CreateWorkspace(t, memberClient, template.ID) coderdtest.AwaitWorkspaceBuildJobCompleted(t, memberClient, workspace.LatestBuild.ID) sent := enqueuer.Sent(notificationstest.WithTemplateID(notifications.TemplateWorkspaceCreated)) - require.Len(t, sent, 1) - require.Equal(t, memberUser.ID, sent[0].UserID) + require.Len(t, sent, 2) + + receivers := make([]uuid.UUID, len(sent)) + for idx, notif := range sent { + receivers[idx] = notif.UserID + } + + // Check the notification was sent to the owner and template admin + require.Contains(t, receivers, templateAdmin.ID) + require.Contains(t, receivers, user.UserID) + require.NotContains(t, receivers, memberUser.ID) + require.Contains(t, sent[0].Targets, template.ID) require.Contains(t, sent[0].Targets, workspace.ID) require.Contains(t, sent[0].Targets, workspace.OrganizationID) require.Contains(t, sent[0].Targets, workspace.OwnerID) + + require.Contains(t, sent[1].Targets, template.ID) + require.Contains(t, sent[1].Targets, workspace.ID) + require.Contains(t, sent[1].Targets, workspace.OrganizationID) + require.Contains(t, sent[1].Targets, workspace.OwnerID) }) t.Run("CreateSendsNotificationToCorrectUser", func(t *testing.T) { @@ -601,14 +617,15 @@ func TestPostWorkspacesByOrganization(t *testing.T) { enqueuer := notificationstest.FakeEnqueuer{} client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true, NotificationsEnqueuer: &enqueuer}) user := coderdtest.CreateFirstUser(t, client) + templateAdminClient, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleTemplateAdmin(), rbac.RoleOwner()) _, memberUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) - version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + version := coderdtest.CreateTemplateVersion(t, templateAdminClient, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, templateAdminClient, user.OrganizationID, version.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdminClient, version.ID) ctx := testutil.Context(t, testutil.WaitShort) - workspace, err := client.CreateUserWorkspace(ctx, memberUser.Username, codersdk.CreateWorkspaceRequest{ + workspace, err := templateAdminClient.CreateUserWorkspace(ctx, memberUser.Username, codersdk.CreateWorkspaceRequest{ TemplateID: template.ID, Name: coderdtest.RandomUsername(t), }) @@ -617,7 +634,7 @@ func TestPostWorkspacesByOrganization(t *testing.T) { sent := enqueuer.Sent(notificationstest.WithTemplateID(notifications.TemplateWorkspaceCreated)) require.Len(t, sent, 1) - require.Equal(t, memberUser.ID, sent[0].UserID) + require.Equal(t, user.UserID, sent[0].UserID) require.Contains(t, sent[0].Targets, template.ID) require.Contains(t, sent[0].Targets, workspace.ID) require.Contains(t, sent[0].Targets, workspace.OrganizationID) From ba68c67d81bfd930506bd3279676bd68fed772e3 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 9 Jan 2025 12:41:29 +0000 Subject: [PATCH 2/5] chore: improve comments --- coderd/workspacebuilds_test.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index f2d4c4b741002..7d1042f761ac4 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -598,14 +598,19 @@ func TestWorkspaceBuildWithUpdatedTemplateVersionSendsNotification(t *testing.T) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, build.ID) coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) - // Create the workspace build _again_. We are doing this to ensure we only create 1 notification. + // Create the workspace build _again_. We are doing this to + // ensure we do not create _another_ notification. This is + // separate to the notifications subsystem dedupe mechanism + // as this build shouldn't create a notification. It shouldn't + // create another notification as this new build isn't changing + // the template version. build = coderdtest.CreateWorkspaceBuild(t, userClient, workspace, database.WorkspaceTransitionStart, func(cwbr *codersdk.CreateWorkspaceBuildRequest) { cwbr.TemplateVersionID = newVersion.ID }) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, build.ID) coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) - // We're going to have two notifications (one for owner and one for the template admin) + // We're going to have two notifications (one for the first user and one for the template admin) // By ensuring we only have these two, we are sure the second build didn't trigger more // notifications. sent := notify.Sent(notificationstest.WithTemplateID(notifications.TemplateWorkspaceManuallyUpdated)) @@ -616,7 +621,9 @@ func TestWorkspaceBuildWithUpdatedTemplateVersionSendsNotification(t *testing.T) receivers[idx] = notif.UserID } - // Check the notification was sent to the owner and template admin + // Check the notification was sent to the first user and template admin + // (both of whom have the "template admin" role), and explicitly not the + // workspace owner (since they initiated the workspace build). require.Contains(t, receivers, templateAdmin.ID) require.Contains(t, receivers, first.UserID) require.NotContains(t, receivers, user.ID) From 7a38242e5b51b0de651fe1dda40b1b563c3cbc22 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 15 Jan 2025 10:46:16 +0000 Subject: [PATCH 3/5] chore: 'owner' -> 'first user' --- coderd/workspaces_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 8826cf8a3dde9..c03fe73175c70 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -595,7 +595,7 @@ func TestPostWorkspacesByOrganization(t *testing.T) { receivers[idx] = notif.UserID } - // Check the notification was sent to the owner and template admin + // Check the notification was sent to the first user and template admin require.Contains(t, receivers, templateAdmin.ID) require.Contains(t, receivers, user.UserID) require.NotContains(t, receivers, memberUser.ID) From 3811133792a5af75abf5f6f70f762fc95bbd8fa7 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 15 Jan 2025 11:39:53 +0000 Subject: [PATCH 4/5] chore: log instead of returning --- coderd/workspacebuilds.go | 23 ++++++++++------------- coderd/workspaces.go | 20 ++++++++------------ 2 files changed, 18 insertions(+), 25 deletions(-) diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 802ecd1ec0954..a901b6602f7e5 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -449,21 +449,18 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { // nolint:gocritic // Need system context to fetch admins admins, err := findTemplateAdmins(dbauthz.AsSystemRestricted(ctx), api.Database) if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching template admins.", - Detail: err.Error(), - }) - return - } - - for _, admin := range admins { - // Don't send notifications to user which initiated the event. - if admin.ID == apiKey.UserID { - continue + api.Logger.Error(ctx, "find template admins", slog.Error(err)) + } else { + for _, admin := range admins { + // Don't send notifications to user which initiated the event. + if admin.ID == apiKey.UserID { + continue + } + + api.notifyWorkspaceUpdated(ctx, apiKey.UserID, admin.ID, workspace, createBuild.RichParameterValues) } - - api.notifyWorkspaceUpdated(ctx, apiKey.UserID, admin.ID, workspace, createBuild.RichParameterValues) } + } api.publishWorkspaceUpdate(ctx, workspace.OwnerID, wspubsub.WorkspaceEvent{ diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 44774ecdd09c5..a1d50233a8198 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -689,20 +689,16 @@ func createWorkspace( // nolint:gocritic // Need system context to fetch admins admins, err := findTemplateAdmins(dbauthz.AsSystemRestricted(ctx), api.Database) if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching template admins.", - Detail: err.Error(), - }) - return - } + api.Logger.Error(ctx, "find template admins", slog.Error(err)) + } else { + for _, admin := range admins { + // Don't send notifications to user which initiated the event. + if admin.ID == initiatorID { + continue + } - for _, admin := range admins { - // Don't send notifications to user which initiated the event. - if admin.ID == initiatorID { - continue + api.notifyWorkspaceCreated(ctx, admin.ID, workspace, req.RichParameterValues) } - - api.notifyWorkspaceCreated(ctx, admin.ID, workspace, req.RichParameterValues) } auditReq.New = workspace.WorkspaceTable() From 66bf5b906b0588fc6d97888654ae08f9cc7dec02 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 15 Jan 2025 14:04:59 +0000 Subject: [PATCH 5/5] chore: appease linter --- coderd/workspacebuilds.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index a901b6602f7e5..375eaab5cd33b 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -460,7 +460,6 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { api.notifyWorkspaceUpdated(ctx, apiKey.UserID, admin.ID, workspace, createBuild.RichParameterValues) } } - } api.publishWorkspaceUpdate(ctx, workspace.OwnerID, wspubsub.WorkspaceEvent{