From 42f4bd77a7bc5ab24cc174e2e55c55b33eb943c1 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 2 Aug 2024 09:37:26 +0200 Subject: [PATCH 1/7] sql --- .../000234_notifications_user_deleted.down.sql | 1 + .../migrations/000234_notifications_user_deleted.up.sql | 9 +++++++++ 2 files changed, 10 insertions(+) create mode 100644 coderd/database/migrations/000234_notifications_user_deleted.down.sql create mode 100644 coderd/database/migrations/000234_notifications_user_deleted.up.sql diff --git a/coderd/database/migrations/000234_notifications_user_deleted.down.sql b/coderd/database/migrations/000234_notifications_user_deleted.down.sql new file mode 100644 index 0000000000000..e0d3c2f7e9823 --- /dev/null +++ b/coderd/database/migrations/000234_notifications_user_deleted.down.sql @@ -0,0 +1 @@ +DELETE FROM notification_templates WHERE id = 'f44d9314-ad03-4bc8-95d0-5cad491da6b6'; diff --git a/coderd/database/migrations/000234_notifications_user_deleted.up.sql b/coderd/database/migrations/000234_notifications_user_deleted.up.sql new file mode 100644 index 0000000000000..9827640ef7775 --- /dev/null +++ b/coderd/database/migrations/000234_notifications_user_deleted.up.sql @@ -0,0 +1,9 @@ +INSERT INTO notification_templates (id, name, title_template, body_template, "group", actions) +VALUES ('f44d9314-ad03-4bc8-95d0-5cad491da6b6', 'User account deleted', E'User account "{{.Labels.deleted_account_name}}" deleted', + E'Hi {{.UserName}},\n\User account **{{.Labels.created_account_name}}** has been deleted.', + 'Workspace Events', '[ + { + "label": "View accounts", + "url": "{{ base_url }}/deployment/users?filter=status%3Aactive" + } + ]'::jsonb); From 9c476371c252d09818effc2df8aa27799ad50bc7 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 2 Aug 2024 09:59:09 +0200 Subject: [PATCH 2/7] WIP --- .../migrations/000234_notifications_user_deleted.up.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/migrations/000234_notifications_user_deleted.up.sql b/coderd/database/migrations/000234_notifications_user_deleted.up.sql index 9827640ef7775..5626218647c1c 100644 --- a/coderd/database/migrations/000234_notifications_user_deleted.up.sql +++ b/coderd/database/migrations/000234_notifications_user_deleted.up.sql @@ -1,6 +1,6 @@ INSERT INTO notification_templates (id, name, title_template, body_template, "group", actions) VALUES ('f44d9314-ad03-4bc8-95d0-5cad491da6b6', 'User account deleted', E'User account "{{.Labels.deleted_account_name}}" deleted', - E'Hi {{.UserName}},\n\User account **{{.Labels.created_account_name}}** has been deleted.', + E'Hi {{.UserName}},\n\nUser account **{{.Labels.created_account_name}}** has been deleted.', 'Workspace Events', '[ { "label": "View accounts", From 4e787ec3a7cd26e327fba773c5c214273be97293 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 2 Aug 2024 11:51:06 +0200 Subject: [PATCH 3/7] fix --- .../migrations/000234_notifications_user_deleted.up.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/migrations/000234_notifications_user_deleted.up.sql b/coderd/database/migrations/000234_notifications_user_deleted.up.sql index 5626218647c1c..8d5140466e9d5 100644 --- a/coderd/database/migrations/000234_notifications_user_deleted.up.sql +++ b/coderd/database/migrations/000234_notifications_user_deleted.up.sql @@ -1,7 +1,7 @@ INSERT INTO notification_templates (id, name, title_template, body_template, "group", actions) VALUES ('f44d9314-ad03-4bc8-95d0-5cad491da6b6', 'User account deleted', E'User account "{{.Labels.deleted_account_name}}" deleted', E'Hi {{.UserName}},\n\nUser account **{{.Labels.created_account_name}}** has been deleted.', - 'Workspace Events', '[ + 'User Events', '[ { "label": "View accounts", "url": "{{ base_url }}/deployment/users?filter=status%3Aactive" From 15fec2bfc1c0fb301a42cc9e5de5925da200ab8d Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 2 Aug 2024 12:36:20 +0200 Subject: [PATCH 4/7] logic --- .../000234_notifications_user_deleted.up.sql | 2 +- coderd/notifications/events.go | 1 + coderd/users.go | 54 ++++++++++++++----- 3 files changed, 42 insertions(+), 15 deletions(-) diff --git a/coderd/database/migrations/000234_notifications_user_deleted.up.sql b/coderd/database/migrations/000234_notifications_user_deleted.up.sql index 8d5140466e9d5..d8354ca2b4c5d 100644 --- a/coderd/database/migrations/000234_notifications_user_deleted.up.sql +++ b/coderd/database/migrations/000234_notifications_user_deleted.up.sql @@ -1,6 +1,6 @@ INSERT INTO notification_templates (id, name, title_template, body_template, "group", actions) VALUES ('f44d9314-ad03-4bc8-95d0-5cad491da6b6', 'User account deleted', E'User account "{{.Labels.deleted_account_name}}" deleted', - E'Hi {{.UserName}},\n\nUser account **{{.Labels.created_account_name}}** has been deleted.', + E'Hi {{.UserName}},\n\nUser account **{{.Labels.deleted_account_name}}** has been deleted.', 'User Events', '[ { "label": "View accounts", diff --git a/coderd/notifications/events.go b/coderd/notifications/events.go index 9908a3e06adfb..c00912d70734c 100644 --- a/coderd/notifications/events.go +++ b/coderd/notifications/events.go @@ -17,4 +17,5 @@ var ( // Account-related events. var ( TemplateUserAccountCreated = uuid.MustParse("4e19c0ac-94e1-4532-9515-d1801aa283b2") + TemplateUserAccountDeleted = uuid.MustParse("f44d9314-ad03-4bc8-95d0-5cad491da6b6") ) diff --git a/coderd/users.go b/coderd/users.go index 565aeca1cb2a8..4f4072c5ff53b 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -560,6 +560,24 @@ func (api *API) deleteUser(rw http.ResponseWriter, r *http.Request) { } user.Deleted = true aReq.New = user + + userAdmins, err := findUserAdmins(ctx, api.Database) + if err != nil { + api.Logger.Warn(ctx, "unable to fetch user admins", slog.Error(err)) + rw.WriteHeader(http.StatusNoContent) + } + + for _, u := range userAdmins { + if _, err := api.NotificationsEnqueuer.Enqueue(ctx, u.ID, notifications.TemplateUserAccountCreated, + map[string]string{ + "deleted_account_name": user.Username, + }, "api-users-delete", + user.ID, + ); err != nil { + api.Logger.Warn(ctx, "unable to notify about deleted user", slog.F("deleted_user", user.Username), slog.Error(err)) + } + } + rw.WriteHeader(http.StatusNoContent) } @@ -1280,23 +1298,12 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create return user, req.OrganizationID, err } - // Notify all users with user admin permission including owners - // Notice: we can't scrape the user information in parallel as pq - // fails with: unexpected describe rows response: 'D' - owners, err := store.GetUsers(ctx, database.GetUsersParams{ - RbacRole: []string{codersdk.RoleOwner}, - }) + userAdmins, err := findUserAdmins(ctx, store) if err != nil { - return user, req.OrganizationID, xerrors.Errorf("get owners: %w", err) - } - userAdmins, err := store.GetUsers(ctx, database.GetUsersParams{ - RbacRole: []string{codersdk.RoleUserAdmin}, - }) - if err != nil { - return user, req.OrganizationID, xerrors.Errorf("get user admins: %w", err) + return user, req.OrganizationID, xerrors.Errorf("find user admins: %w", err) } - for _, u := range append(owners, userAdmins...) { + for _, u := range userAdmins { if _, err := api.NotificationsEnqueuer.Enqueue(ctx, u.ID, notifications.TemplateUserAccountCreated, map[string]string{ "created_account_name": user.Username, @@ -1309,6 +1316,25 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create return user, req.OrganizationID, err } +// findUserAdmins fetches all users with user admin permission including owners. +func findUserAdmins(ctx context.Context, store database.Store) ([]database.GetUsersRow, error) { + // Notice: we can't scrape the user information in parallel as pq + // fails with: unexpected describe rows response: 'D' + owners, err := store.GetUsers(ctx, database.GetUsersParams{ + RbacRole: []string{codersdk.RoleOwner}, + }) + if err != nil { + return nil, xerrors.Errorf("get owners: %w", err) + } + userAdmins, err := store.GetUsers(ctx, database.GetUsersParams{ + RbacRole: []string{codersdk.RoleUserAdmin}, + }) + if err != nil { + return nil, xerrors.Errorf("get user admins: %w", err) + } + return append(owners, userAdmins...), nil +} + func convertUsers(users []database.User, organizationIDsByUserID map[uuid.UUID][]uuid.UUID) []codersdk.User { converted := make([]codersdk.User, 0, len(users)) for _, u := range users { From 37bbb4129f0dc2bc58646d96a054135f921b6a1a Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 2 Aug 2024 12:59:17 +0200 Subject: [PATCH 5/7] tests --- coderd/users.go | 2 +- coderd/users_test.go | 97 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/coderd/users.go b/coderd/users.go index 4f4072c5ff53b..a79cb6056684a 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -568,7 +568,7 @@ func (api *API) deleteUser(rw http.ResponseWriter, r *http.Request) { } for _, u := range userAdmins { - if _, err := api.NotificationsEnqueuer.Enqueue(ctx, u.ID, notifications.TemplateUserAccountCreated, + if _, err := api.NotificationsEnqueuer.Enqueue(ctx, u.ID, notifications.TemplateUserAccountDeleted, map[string]string{ "deleted_account_name": user.Username, }, "api-users-delete", diff --git a/coderd/users_test.go b/coderd/users_test.go index d84dfee820b90..eba27c897e20b 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -374,6 +374,103 @@ func TestDeleteUser(t *testing.T) { }) } +func TestNotifyDeletedUser(t *testing.T) { + t.Parallel() + + t.Run("OwnerNotified", func(t *testing.T) { + t.Parallel() + + // given + notifyEnq := &testutil.FakeNotificationsEnqueuer{} + adminClient := coderdtest.New(t, &coderdtest.Options{ + NotificationsEnqueuer: notifyEnq, + }) + firstUser := coderdtest.CreateFirstUser(t, adminClient) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + user, err := adminClient.CreateUser(ctx, codersdk.CreateUserRequest{ + OrganizationID: firstUser.OrganizationID, + Email: "another@user.org", + Username: "someone-else", + Password: "SomeSecurePassword!", + }) + require.NoError(t, err) + + // when + err = adminClient.DeleteUser(context.Background(), user.ID) + require.NoError(t, err) + + // then + require.Len(t, notifyEnq.Sent, 2) + // notifyEnq.Sent[0] is create account event + require.Equal(t, notifications.TemplateUserAccountDeleted, notifyEnq.Sent[1].TemplateID) + require.Equal(t, firstUser.UserID, notifyEnq.Sent[1].UserID) + require.Contains(t, notifyEnq.Sent[1].Targets, user.ID) + require.Equal(t, user.Username, notifyEnq.Sent[1].Labels["deleted_account_name"]) + }) + + t.Run("UserAdminNotified", func(t *testing.T) { + t.Parallel() + + // given + notifyEnq := &testutil.FakeNotificationsEnqueuer{} + adminClient := coderdtest.New(t, &coderdtest.Options{ + NotificationsEnqueuer: notifyEnq, + }) + firstUser := coderdtest.CreateFirstUser(t, adminClient) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + userAdmin, err := adminClient.CreateUser(ctx, codersdk.CreateUserRequest{ + OrganizationID: firstUser.OrganizationID, + Email: "user-admin@user.org", + Username: "mr-user-admin", + Password: "SomeSecurePassword!", + }) + require.NoError(t, err) + + _, err = adminClient.UpdateUserRoles(ctx, userAdmin.Username, codersdk.UpdateRoles{ + Roles: []string{ + rbac.RoleUserAdmin().String(), + }, + }) + require.NoError(t, err) + + member, err := adminClient.CreateUser(ctx, codersdk.CreateUserRequest{ + OrganizationID: firstUser.OrganizationID, + Email: "another@user.org", + Username: "someone-else", + Password: "SomeSecurePassword!", + }) + require.NoError(t, err) + + // when + err = adminClient.DeleteUser(context.Background(), member.ID) + require.NoError(t, err) + + // then + require.Len(t, notifyEnq.Sent, 5) + // notifyEnq.Sent[0]: "User admin" account created, "owner" notified + // notifyEnq.Sent[1]: "Member" account created, "owner" notified + // notifyEnq.Sent[2]: "Member" account created, "user admin" notified + + // "Member" account deleted, "owner" notified + require.Equal(t, notifications.TemplateUserAccountDeleted, notifyEnq.Sent[3].TemplateID) + require.Equal(t, firstUser.UserID, notifyEnq.Sent[3].UserID) + require.Contains(t, notifyEnq.Sent[3].Targets, member.ID) + require.Equal(t, member.Username, notifyEnq.Sent[3].Labels["deleted_account_name"]) + + // "Member" account deleted, "user admin" notified + require.Equal(t, notifications.TemplateUserAccountDeleted, notifyEnq.Sent[4].TemplateID) + require.Equal(t, userAdmin.ID, notifyEnq.Sent[4].UserID) + require.Contains(t, notifyEnq.Sent[4].Targets, member.ID) + require.Equal(t, member.Username, notifyEnq.Sent[4].Labels["deleted_account_name"]) + }) +} + func TestPostLogout(t *testing.T) { t.Parallel() From ac5a3e6deb1d9c18f727e9c2de1b696c7e883632 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 2 Aug 2024 13:01:38 +0200 Subject: [PATCH 6/7] Fix migration numbers --- ...eleted.down.sql => 000236_notifications_user_deleted.down.sql} | 0 ...er_deleted.up.sql => 000236_notifications_user_deleted.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename coderd/database/migrations/{000234_notifications_user_deleted.down.sql => 000236_notifications_user_deleted.down.sql} (100%) rename coderd/database/migrations/{000234_notifications_user_deleted.up.sql => 000236_notifications_user_deleted.up.sql} (100%) diff --git a/coderd/database/migrations/000234_notifications_user_deleted.down.sql b/coderd/database/migrations/000236_notifications_user_deleted.down.sql similarity index 100% rename from coderd/database/migrations/000234_notifications_user_deleted.down.sql rename to coderd/database/migrations/000236_notifications_user_deleted.down.sql diff --git a/coderd/database/migrations/000234_notifications_user_deleted.up.sql b/coderd/database/migrations/000236_notifications_user_deleted.up.sql similarity index 100% rename from coderd/database/migrations/000234_notifications_user_deleted.up.sql rename to coderd/database/migrations/000236_notifications_user_deleted.up.sql From 085c5bf19834aa19b5605dbbcb5ba7bb2408ae53 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 2 Aug 2024 13:55:56 +0200 Subject: [PATCH 7/7] Danny's review --- coderd/users.go | 7 +++++-- coderd/users_test.go | 15 +-------------- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 12b6bf88280b2..adf329ea0059d 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -570,8 +570,11 @@ func (api *API) deleteUser(rw http.ResponseWriter, r *http.Request) { userAdmins, err := findUserAdmins(ctx, api.Database) if err != nil { - api.Logger.Warn(ctx, "unable to fetch user admins", slog.Error(err)) - rw.WriteHeader(http.StatusNoContent) + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching user admins.", + Detail: err.Error(), + }) + return } for _, u := range userAdmins { diff --git a/coderd/users_test.go b/coderd/users_test.go index eba27c897e20b..4f44da42ed59b 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -424,20 +424,7 @@ func TestNotifyDeletedUser(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - userAdmin, err := adminClient.CreateUser(ctx, codersdk.CreateUserRequest{ - OrganizationID: firstUser.OrganizationID, - Email: "user-admin@user.org", - Username: "mr-user-admin", - Password: "SomeSecurePassword!", - }) - require.NoError(t, err) - - _, err = adminClient.UpdateUserRoles(ctx, userAdmin.Username, codersdk.UpdateRoles{ - Roles: []string{ - rbac.RoleUserAdmin().String(), - }, - }) - require.NoError(t, err) + _, userAdmin := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID, rbac.RoleUserAdmin()) member, err := adminClient.CreateUser(ctx, codersdk.CreateUserRequest{ OrganizationID: firstUser.OrganizationID,