From 884d40646c16d87cf1f4bdd5b5fc52769aafc6e1 Mon Sep 17 00:00:00 2001 From: Andres Tobon Date: Mon, 27 Jul 2026 12:43:19 -0700 Subject: [PATCH 01/13] feat(users): scrub deleted user's username from project settings (LFXV2-2645) Subscribe to lfx.v1-sync-helper.user.deleted and clear matching LFIDs from project settings role lists, reindexing updated records without emitting role-change notifications. Co-authored-by: Cursor Signed-off-by: Andres Tobon --- .../references/nats-messaging.md | 10 + cmd/project-api/main.go | 1 + internal/service/project_subscriber.go | 158 +++++++++++++++ internal/service/project_subscriber_test.go | 180 ++++++++++++++++++ pkg/constants/nats.go | 9 + 5 files changed, 358 insertions(+) diff --git a/.claude/skills/project-service-dev/references/nats-messaging.md b/.claude/skills/project-service-dev/references/nats-messaging.md index 69ad6a7e..a68a608e 100644 --- a/.claude/skills/project-service-dev/references/nats-messaging.md +++ b/.claude/skills/project-service-dev/references/nats-messaging.md @@ -36,6 +36,16 @@ All six live as `Project*Subject` constants in `pkg/constants/nats.go`. `lfx.index.*` envelopes are owned by `lfx-v2-indexer-service`. `lfx.fga-sync.*` envelopes are owned by `lfx-v2-fga-sync`. For per-resource fields, see `docs/indexer-contract.md` and `docs/fga-contract.md`. The `lfx.projects-api.*.created` payloads are `events.ProjectDocumentCreatedMessage` and `events.ProjectLinkCreatedMessage` in `pkg/events/`; this service also subscribes to them itself (`internal/service/document_subscriber.go`) to send upload-notification emails. +## Inbound event subjects (handled by this service) + +```go +"lfx.projects-api.project_settings.updated" // handled by HandleProjectSettingsUpdated (role-change notifications) +"lfx.invite-service.invite_accepted" // handled by HandleInviteAccepted (promote email-only settings entries to LFID) +"lfx.v1-sync-helper.user.deleted" // handled by HandleUserDeleted (scrub deleted user's username from project settings) +"lfx.projects-api.project_document.created" // handled by HandleProjectDocumentCreated (upload notification emails) +"lfx.projects-api.project_link.created" // handled by HandleProjectLinkCreated +``` + ## Owned KV buckets | Bucket | Purpose | History | diff --git a/cmd/project-api/main.go b/cmd/project-api/main.go index ff19d68b..6d2b5e40 100644 --- a/cmd/project-api/main.go +++ b/cmd/project-api/main.go @@ -497,6 +497,7 @@ func createNatsSubcriptions(ctx context.Context, svc *ProjectsAPI, natsConn *nat for _, eh := range []eventHandler{ {constants.ProjectSettingsUpdatedSubject, svc.service.HandleProjectSettingsUpdated}, {inviteapi.InviteServiceAcceptedSubject, svc.service.HandleInviteAccepted}, + {constants.V1SyncHelperUserDeletedSubject, svc.service.HandleUserDeleted}, {constants.ProjectDocumentCreatedSubject, svc.service.HandleProjectDocumentCreated}, {constants.ProjectLinkCreatedSubject, svc.service.HandleProjectLinkCreated}, } { diff --git a/internal/service/project_subscriber.go b/internal/service/project_subscriber.go index 47484007..cf2e3c41 100644 --- a/internal/service/project_subscriber.go +++ b/internal/service/project_subscriber.go @@ -520,6 +520,164 @@ func (s *ProjectsService) promoteInvitedUserInProjectSettings(ctx context.Contex } } +// V1UserDeletedEvent is the payload published by v1-sync-helper on "lfx.v1-sync-helper.user.deleted" +// when a merged user record is soft-deleted. Username is the user's LFID. +type V1UserDeletedEvent struct { + Username string `json:"username"` +} + +// HandleUserDeleted scrubs the deleted user's username from project settings. +// Best-effort: partial failures are logged but do not block the overall scrub. +// Unlike committee data, project settings have no separate member records — only +// the settings writers/auditors/meeting coordinators and named role fields are updated. +func (s *ProjectsService) HandleUserDeleted(ctx context.Context, msg domain.Message) error { + var event V1UserDeletedEvent + if err := json.Unmarshal(msg.Data(), &event); err != nil { + slog.WarnContext(ctx, "project_subscriber: failed to unmarshal user.deleted event", constants.ErrKey, err) + return nil + } + if event.Username == "" { + slog.WarnContext(ctx, "project_subscriber: user.deleted event missing username — nothing to scrub") + return nil + } + + slog.InfoContext(ctx, "project_subscriber: scrubbing deleted user's username from project settings", + "username", event.Username) + + listCtx, listCancel := context.WithTimeout(ctx, notificationTimeout) + allSettings, listErr := s.ProjectRepository.ListAllProjectsSettings(listCtx) + listCancel() + if listErr != nil { + slog.WarnContext(ctx, "project_subscriber: failed to list project settings for username scrub", + constants.ErrKey, listErr, "username", event.Username) + return nil + } + + for _, candidate := range allSettings { + if !projectSettingsHasUsername(candidate, event.Username) { + continue + } + scrubCtx, scrubCancel := context.WithTimeout(ctx, notificationTimeout) + s.scrubUsernameFromProjectSettings(scrubCtx, candidate.UID, event.Username) + scrubCancel() + } + + return nil +} + +// projectSettingsHasUsername reports whether any role-bearing field in settings carries username. +func projectSettingsHasUsername(s *models.ProjectSettings, username string) bool { + if s == nil { + return false + } + for _, u := range s.Writers { + if u.Username == username { + return true + } + } + for _, u := range s.Auditors { + if u.Username == username { + return true + } + } + for _, u := range s.MeetingCoordinators { + if u.Username == username { + return true + } + } + if s.ExecutiveDirector != nil && s.ExecutiveDirector.Username == username { + return true + } + if s.ProgramManager != nil && s.ProgramManager.Username == username { + return true + } + if s.OpportunityOwner != nil && s.OpportunityOwner.Username == username { + return true + } + return false +} + +// scrubUsernameInProjectSettings clears username on every matching entry in settings. +// Returns true when at least one field was changed. +func scrubUsernameInProjectSettings(settings *models.ProjectSettings, username string) bool { + changed := false + for i := range settings.Writers { + if settings.Writers[i].Username == username { + settings.Writers[i].Username = "" + changed = true + } + } + for i := range settings.Auditors { + if settings.Auditors[i].Username == username { + settings.Auditors[i].Username = "" + changed = true + } + } + for i := range settings.MeetingCoordinators { + if settings.MeetingCoordinators[i].Username == username { + settings.MeetingCoordinators[i].Username = "" + changed = true + } + } + if settings.ExecutiveDirector != nil && settings.ExecutiveDirector.Username == username { + settings.ExecutiveDirector.Username = "" + changed = true + } + if settings.ProgramManager != nil && settings.ProgramManager.Username == username { + settings.ProgramManager.Username = "" + changed = true + } + if settings.OpportunityOwner != nil && settings.OpportunityOwner.Username == username { + settings.OpportunityOwner.Username = "" + changed = true + } + return changed +} + +// scrubUsernameFromProjectSettings fetches settings for a single project, clears the +// username on any matching entry, persists, and reindexes. Retries on revision conflicts. +func (s *ProjectsService) scrubUsernameFromProjectSettings(ctx context.Context, projectUID, username string) { + const maxRetries = 4 + for attempt := 0; attempt < maxRetries; attempt++ { + settings, revision, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID) + if err != nil { + slog.DebugContext(ctx, "project_subscriber: failed to get settings for username scrub — skipping", + constants.ErrKey, err, "project_uid", projectUID) + return + } + if settings == nil { + return + } + + if !scrubUsernameInProjectSettings(settings, username) { + return + } + + updateErr := s.ProjectRepository.UpdateProjectSettings(ctx, settings, revision) + if updateErr == nil { + slog.InfoContext(ctx, "project_subscriber: cleared username from project settings", + "project_uid", projectUID, "username", username) + indexMsg := indexerTypes.IndexerMessageEnvelope{ + Action: indexerConstants.ActionUpdated, + Data: *settings, + IndexingConfig: settings.IndexingConfig(projectUID), + } + if indexErr := s.MessageBuilder.SendIndexerMessage(ctx, constants.IndexProjectSettingsSubject, indexMsg, false); indexErr != nil { + slog.WarnContext(ctx, "project_subscriber: failed to reindex project settings after username scrub", + constants.ErrKey, indexErr, "project_uid", projectUID) + } + return + } + if !errors.Is(updateErr, domain.ErrRevisionMismatch) || attempt == maxRetries-1 { + slog.WarnContext(ctx, "project_subscriber: failed to clear username from project settings", + constants.ErrKey, updateErr, "project_uid", projectUID) + return + } + slog.DebugContext(ctx, "project_subscriber: revision mismatch scrubbing username — retrying", + "attempt", attempt+1, "project_uid", projectUID) + } +} + // buildProjectURL constructs the deep-link URL for a project's overview page. func buildProjectURL(baseURL, slug string) string { base := strings.TrimRight(baseURL, "/") + "/project/overview" diff --git a/internal/service/project_subscriber_test.go b/internal/service/project_subscriber_test.go index 6ff2f37e..a4c72c64 100644 --- a/internal/service/project_subscriber_test.go +++ b/internal/service/project_subscriber_test.go @@ -20,6 +20,7 @@ import ( "github.com/linuxfoundation/lfx-v2-project-service/internal/domain" "github.com/linuxfoundation/lfx-v2-project-service/internal/domain/models" + "github.com/linuxfoundation/lfx-v2-project-service/pkg/constants" "github.com/linuxfoundation/lfx-v2-project-service/pkg/events" ) @@ -1148,6 +1149,185 @@ func TestHandleInviteAccepted(t *testing.T) { } } +func TestHandleUserDeleted(t *testing.T) { + const deletedUsername = "deleted.user" + const projectUID = "proj-1" + const project2UID = "proj-2" + + makeEvent := func(username string) V1UserDeletedEvent { + return V1UserDeletedEvent{Username: username} + } + + tests := []struct { + name string + payload any + setupRepo func(*domain.MockProjectRepository) + setupMsg func(*domain.MockMessageBuilder) + }{ + { + name: "malformed payload — returns nil without crashing", + payload: []byte("not json"), + }, + { + name: "empty username — discarded", + payload: makeEvent(""), + }, + { + name: "settings match — writer username cleared and reindexed", + payload: makeEvent(deletedUsername), + setupRepo: func(r *domain.MockProjectRepository) { + settings := &models.ProjectSettings{ + UID: projectUID, + Writers: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, + } + r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil) + r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { + return len(s.Writers) == 1 && s.Writers[0].Username == "" && s.Writers[0].Email == "deleted@example.com" + }), uint64(1)).Return(nil) + }, + setupMsg: func(m *domain.MockMessageBuilder) { + m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil) + }, + }, + { + name: "settings no match — no update", + payload: makeEvent(deletedUsername), + setupRepo: func(r *domain.MockProjectRepository) { + settings := &models.ProjectSettings{ + UID: projectUID, + Writers: []models.UserInfo{{Username: "other", Email: "other@example.com"}}, + } + r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) + }, + }, + { + name: "conflict retry — succeeds on second attempt", + payload: makeEvent(deletedUsername), + setupRepo: func(r *domain.MockProjectRepository) { + firstRead := &models.ProjectSettings{ + UID: projectUID, + Auditors: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, + } + secondRead := &models.ProjectSettings{ + UID: projectUID, + Auditors: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, + } + r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{firstRead}, nil) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(firstRead, uint64(1), nil).Once() + r.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)). + Return(domain.ErrRevisionMismatch).Once() + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(secondRead, uint64(2), nil).Once() + r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { + return len(s.Auditors) == 1 && s.Auditors[0].Username == "" + }), uint64(2)).Return(nil).Once() + }, + setupMsg: func(m *domain.MockMessageBuilder) { + m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil) + }, + }, + { + name: "ListAllProjectsSettings error — early return", + payload: makeEvent(deletedUsername), + setupRepo: func(r *domain.MockProjectRepository) { + r.On("ListAllProjectsSettings", mock.Anything).Return(nil, errors.New("kv unavailable")) + }, + }, + { + name: "meeting coordinator and named roles cleared", + payload: makeEvent(deletedUsername), + setupRepo: func(r *domain.MockProjectRepository) { + settings := &models.ProjectSettings{ + UID: projectUID, + MeetingCoordinators: []models.UserInfo{{Username: deletedUsername, Email: "mc@example.com"}}, + ExecutiveDirector: &models.UserInfo{Username: deletedUsername, Email: "ed@example.com"}, + } + r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil) + r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { + return len(s.MeetingCoordinators) == 1 && s.MeetingCoordinators[0].Username == "" && + s.ExecutiveDirector != nil && s.ExecutiveDirector.Username == "" + }), uint64(1)).Return(nil) + }, + setupMsg: func(m *domain.MockMessageBuilder) { + m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil) + }, + }, + { + name: "multiple projects — only matching project updated", + payload: makeEvent(deletedUsername), + setupRepo: func(r *domain.MockProjectRepository) { + match := &models.ProjectSettings{ + UID: projectUID, + Writers: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, + } + other := &models.ProjectSettings{ + UID: project2UID, + Writers: []models.UserInfo{{Username: "other", Email: "other@example.com"}}, + } + r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{match, other}, nil) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(match, uint64(1), nil) + r.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)).Return(nil) + }, + setupMsg: func(m *domain.MockMessageBuilder) { + m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockRepo := &domain.MockProjectRepository{} + mockMsg := &domain.MockMessageBuilder{} + if tt.setupRepo != nil { + tt.setupRepo(mockRepo) + } + if tt.setupMsg != nil { + tt.setupMsg(mockMsg) + } + + svc := &ProjectsService{ + ProjectRepository: mockRepo, + MessageBuilder: mockMsg, + } + + var data []byte + if raw, ok := tt.payload.([]byte); ok { + data = raw + } else { + data = marshalEvent(t, tt.payload) + } + msg := domain.NewMockMessage(data, constants.V1SyncHelperUserDeletedSubject) + err := svc.HandleUserDeleted(context.Background(), msg) + assert.NoError(t, err) + + mockRepo.AssertExpectations(t) + mockMsg.AssertExpectations(t) + }) + } +} + +func TestProjectSettingsHasUsername(t *testing.T) { + const username = "alice" + tests := []struct { + name string + settings *models.ProjectSettings + want bool + }{ + {name: "nil settings", settings: nil, want: false}, + {name: "writer match", settings: &models.ProjectSettings{Writers: []models.UserInfo{{Username: username}}}, want: true}, + {name: "auditor match", settings: &models.ProjectSettings{Auditors: []models.UserInfo{{Username: username}}}, want: true}, + {name: "meeting coordinator match", settings: &models.ProjectSettings{MeetingCoordinators: []models.UserInfo{{Username: username}}}, want: true}, + {name: "executive director match", settings: &models.ProjectSettings{ExecutiveDirector: &models.UserInfo{Username: username}}, want: true}, + {name: "no match", settings: &models.ProjectSettings{Writers: []models.UserInfo{{Username: "other"}}}, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, projectSettingsHasUsername(tt.settings, username)) + }) + } +} + // Compile-time checks for imported API types. var ( _ emailapi.SendEmailRequest diff --git a/pkg/constants/nats.go b/pkg/constants/nats.go index 46e91e24..2e48c9bd 100644 --- a/pkg/constants/nats.go +++ b/pkg/constants/nats.go @@ -109,3 +109,12 @@ const ( // Request: plain-text email. Reply: plain-text username on success, JSON error envelope on miss. AuthEmailToUsernameSubject = "lfx.auth-service.email_to_username" ) + +// NATS subjects consumed from other services. +const ( + // V1SyncHelperUserDeletedSubject is emitted by v1-sync-helper when a merged user record is + // soft-deleted. The project service subscribes to scrub the deleted user's username from + // project settings writers/auditors/meeting coordinators and named role fields. + // Payload: JSON-encoded {"username":""} (see internal/service/project_subscriber.go). + V1SyncHelperUserDeletedSubject = "lfx.v1-sync-helper.user.deleted" +) From ce9a78156f4e0e84ece05b773f787e15c6cef878 Mon Sep 17 00:00:00 2001 From: Andres Tobon Date: Mon, 27 Jul 2026 12:49:48 -0700 Subject: [PATCH 02/13] fix(users): publish FGA update_access after username scrub (LFXV2-2645) Refresh OpenFGA tuples from scrubbed settings via buildFGAUpdateAccessMessage, matching normal settings writes while still omitting role-change notification emails. Co-authored-by: Cursor Signed-off-by: Andres Tobon --- internal/service/project_subscriber.go | 38 ++++++++++++++++----- internal/service/project_subscriber_test.go | 9 +++++ 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/internal/service/project_subscriber.go b/internal/service/project_subscriber.go index cf2e3c41..b209b9b6 100644 --- a/internal/service/project_subscriber.go +++ b/internal/service/project_subscriber.go @@ -13,6 +13,7 @@ import ( "time" emailapi "github.com/linuxfoundation/lfx-v2-email-service/pkg/api" + fgaconstants "github.com/linuxfoundation/lfx-v2-fga-sync/pkg/constants" indexerConstants "github.com/linuxfoundation/lfx-v2-indexer-service/pkg/constants" indexerTypes "github.com/linuxfoundation/lfx-v2-indexer-service/pkg/types" inviteapi "github.com/linuxfoundation/lfx-v2-invite-service/pkg/api" @@ -657,15 +658,7 @@ func (s *ProjectsService) scrubUsernameFromProjectSettings(ctx context.Context, if updateErr == nil { slog.InfoContext(ctx, "project_subscriber: cleared username from project settings", "project_uid", projectUID, "username", username) - indexMsg := indexerTypes.IndexerMessageEnvelope{ - Action: indexerConstants.ActionUpdated, - Data: *settings, - IndexingConfig: settings.IndexingConfig(projectUID), - } - if indexErr := s.MessageBuilder.SendIndexerMessage(ctx, constants.IndexProjectSettingsSubject, indexMsg, false); indexErr != nil { - slog.WarnContext(ctx, "project_subscriber: failed to reindex project settings after username scrub", - constants.ErrKey, indexErr, "project_uid", projectUID) - } + s.publishProjectSettingsScrubSideEffects(ctx, projectUID, settings) return } if !errors.Is(updateErr, domain.ErrRevisionMismatch) || attempt == maxRetries-1 { @@ -678,6 +671,33 @@ func (s *ProjectsService) scrubUsernameFromProjectSettings(ctx context.Context, } } +// publishProjectSettingsScrubSideEffects reindexes scrubbed settings and refreshes OpenFGA +// access tuples. Best-effort: failures are logged but do not fail the scrub handler. +// ProjectSettingsUpdatedSubject is intentionally omitted to avoid role-change emails. +func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Context, projectUID string, settings *models.ProjectSettings) { + indexMsg := indexerTypes.IndexerMessageEnvelope{ + Action: indexerConstants.ActionUpdated, + Data: *settings, + IndexingConfig: settings.IndexingConfig(projectUID), + } + if indexErr := s.MessageBuilder.SendIndexerMessage(ctx, constants.IndexProjectSettingsSubject, indexMsg, false); indexErr != nil { + slog.WarnContext(ctx, "project_subscriber: failed to reindex project settings after username scrub", + constants.ErrKey, indexErr, "project_uid", projectUID) + } + + projectBase, err := s.ProjectRepository.GetProjectBase(ctx, projectUID) + if err != nil { + slog.WarnContext(ctx, "project_subscriber: failed to load project for FGA refresh after username scrub", + constants.ErrKey, err, "project_uid", projectUID) + return + } + fgaMsg := buildFGAUpdateAccessMessage(projectBase, settings) + if accessErr := s.MessageBuilder.SendAccessMessage(ctx, fgaconstants.GenericUpdateAccessSubject, fgaMsg, false); accessErr != nil { + slog.WarnContext(ctx, "project_subscriber: failed to publish FGA update after username scrub", + constants.ErrKey, accessErr, "project_uid", projectUID) + } +} + // buildProjectURL constructs the deep-link URL for a project's overview page. func buildProjectURL(baseURL, slug string) string { base := strings.TrimRight(baseURL, "/") + "/project/overview" diff --git a/internal/service/project_subscriber_test.go b/internal/service/project_subscriber_test.go index a4c72c64..8b55af69 100644 --- a/internal/service/project_subscriber_test.go +++ b/internal/service/project_subscriber_test.go @@ -12,6 +12,7 @@ import ( "time" emailapi "github.com/linuxfoundation/lfx-v2-email-service/pkg/api" + fgaconstants "github.com/linuxfoundation/lfx-v2-fga-sync/pkg/constants" indexerTypes "github.com/linuxfoundation/lfx-v2-indexer-service/pkg/types" inviteapi "github.com/linuxfoundation/lfx-v2-invite-service/pkg/api" "github.com/stretchr/testify/assert" @@ -1185,9 +1186,11 @@ func TestHandleUserDeleted(t *testing.T) { r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { return len(s.Writers) == 1 && s.Writers[0].Username == "" && s.Writers[0].Email == "deleted@example.com" }), uint64(1)).Return(nil) + r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) }, setupMsg: func(m *domain.MockMessageBuilder) { m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil) + m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false).Return(nil) }, }, { @@ -1221,9 +1224,11 @@ func TestHandleUserDeleted(t *testing.T) { r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { return len(s.Auditors) == 1 && s.Auditors[0].Username == "" }), uint64(2)).Return(nil).Once() + r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) }, setupMsg: func(m *domain.MockMessageBuilder) { m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil) + m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false).Return(nil) }, }, { @@ -1248,9 +1253,11 @@ func TestHandleUserDeleted(t *testing.T) { return len(s.MeetingCoordinators) == 1 && s.MeetingCoordinators[0].Username == "" && s.ExecutiveDirector != nil && s.ExecutiveDirector.Username == "" }), uint64(1)).Return(nil) + r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) }, setupMsg: func(m *domain.MockMessageBuilder) { m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil) + m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false).Return(nil) }, }, { @@ -1268,9 +1275,11 @@ func TestHandleUserDeleted(t *testing.T) { r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{match, other}, nil) r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(match, uint64(1), nil) r.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)).Return(nil) + r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) }, setupMsg: func(m *domain.MockMessageBuilder) { m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil) + m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false).Return(nil) }, }, } From b590038eb5631f0f05df1d80c5a372669dbdeb16 Mon Sep 17 00:00:00 2001 From: Andres Tobon Date: Mon, 27 Jul 2026 12:55:01 -0700 Subject: [PATCH 03/13] fix(review): move V1UserDeletedEvent to pkg/events and expand named-role tests Aligns with the NATS payload convention and covers Program Manager and Opportunity Owner in both the scrub integration test and has-username table. Signed-off-by: Andres Tobon Co-authored-by: Cursor --- internal/service/project_subscriber.go | 8 +------- internal/service/project_subscriber_test.go | 12 +++++++++--- pkg/constants/nats.go | 2 +- pkg/events/project.go | 7 +++++++ 4 files changed, 18 insertions(+), 11 deletions(-) diff --git a/internal/service/project_subscriber.go b/internal/service/project_subscriber.go index b209b9b6..72a57846 100644 --- a/internal/service/project_subscriber.go +++ b/internal/service/project_subscriber.go @@ -521,18 +521,12 @@ func (s *ProjectsService) promoteInvitedUserInProjectSettings(ctx context.Contex } } -// V1UserDeletedEvent is the payload published by v1-sync-helper on "lfx.v1-sync-helper.user.deleted" -// when a merged user record is soft-deleted. Username is the user's LFID. -type V1UserDeletedEvent struct { - Username string `json:"username"` -} - // HandleUserDeleted scrubs the deleted user's username from project settings. // Best-effort: partial failures are logged but do not block the overall scrub. // Unlike committee data, project settings have no separate member records — only // the settings writers/auditors/meeting coordinators and named role fields are updated. func (s *ProjectsService) HandleUserDeleted(ctx context.Context, msg domain.Message) error { - var event V1UserDeletedEvent + var event events.V1UserDeletedEvent if err := json.Unmarshal(msg.Data(), &event); err != nil { slog.WarnContext(ctx, "project_subscriber: failed to unmarshal user.deleted event", constants.ErrKey, err) return nil diff --git a/internal/service/project_subscriber_test.go b/internal/service/project_subscriber_test.go index 8b55af69..9edc6683 100644 --- a/internal/service/project_subscriber_test.go +++ b/internal/service/project_subscriber_test.go @@ -1155,8 +1155,8 @@ func TestHandleUserDeleted(t *testing.T) { const projectUID = "proj-1" const project2UID = "proj-2" - makeEvent := func(username string) V1UserDeletedEvent { - return V1UserDeletedEvent{Username: username} + makeEvent := func(username string) events.V1UserDeletedEvent { + return events.V1UserDeletedEvent{Username: username} } tests := []struct { @@ -1246,12 +1246,16 @@ func TestHandleUserDeleted(t *testing.T) { UID: projectUID, MeetingCoordinators: []models.UserInfo{{Username: deletedUsername, Email: "mc@example.com"}}, ExecutiveDirector: &models.UserInfo{Username: deletedUsername, Email: "ed@example.com"}, + ProgramManager: &models.UserInfo{Username: deletedUsername, Email: "pm@example.com"}, + OpportunityOwner: &models.UserInfo{Username: deletedUsername, Email: "oo@example.com"}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil) r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { return len(s.MeetingCoordinators) == 1 && s.MeetingCoordinators[0].Username == "" && - s.ExecutiveDirector != nil && s.ExecutiveDirector.Username == "" + s.ExecutiveDirector != nil && s.ExecutiveDirector.Username == "" && + s.ProgramManager != nil && s.ProgramManager.Username == "" && + s.OpportunityOwner != nil && s.OpportunityOwner.Username == "" }), uint64(1)).Return(nil) r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) }, @@ -1328,6 +1332,8 @@ func TestProjectSettingsHasUsername(t *testing.T) { {name: "auditor match", settings: &models.ProjectSettings{Auditors: []models.UserInfo{{Username: username}}}, want: true}, {name: "meeting coordinator match", settings: &models.ProjectSettings{MeetingCoordinators: []models.UserInfo{{Username: username}}}, want: true}, {name: "executive director match", settings: &models.ProjectSettings{ExecutiveDirector: &models.UserInfo{Username: username}}, want: true}, + {name: "program manager match", settings: &models.ProjectSettings{ProgramManager: &models.UserInfo{Username: username}}, want: true}, + {name: "opportunity owner match", settings: &models.ProjectSettings{OpportunityOwner: &models.UserInfo{Username: username}}, want: true}, {name: "no match", settings: &models.ProjectSettings{Writers: []models.UserInfo{{Username: "other"}}}, want: false}, } for _, tt := range tests { diff --git a/pkg/constants/nats.go b/pkg/constants/nats.go index 2e48c9bd..8d53c39d 100644 --- a/pkg/constants/nats.go +++ b/pkg/constants/nats.go @@ -115,6 +115,6 @@ const ( // V1SyncHelperUserDeletedSubject is emitted by v1-sync-helper when a merged user record is // soft-deleted. The project service subscribes to scrub the deleted user's username from // project settings writers/auditors/meeting coordinators and named role fields. - // Payload: JSON-encoded {"username":""} (see internal/service/project_subscriber.go). + // Payload: JSON-encoded events.V1UserDeletedEvent. V1SyncHelperUserDeletedSubject = "lfx.v1-sync-helper.user.deleted" ) diff --git a/pkg/events/project.go b/pkg/events/project.go index 068a1021..e9bda77c 100644 --- a/pkg/events/project.go +++ b/pkg/events/project.go @@ -90,3 +90,10 @@ type InviteAccepted struct { InviteUID string `json:"invite_uid"` Username string `json:"username"` } + +// V1UserDeletedEvent is the payload published by v1-sync-helper on +// lfx.v1-sync-helper.user.deleted when a merged user record is soft-deleted. +// Username is the user's LFID. +type V1UserDeletedEvent struct { + Username string `json:"username"` +} From aa480f4472496698ddc59430cf7ae09f57cf41fb Mon Sep 17 00:00:00 2001 From: Andres Tobon Date: Mon, 27 Jul 2026 13:05:46 -0700 Subject: [PATCH 04/13] fix(review): harden username scrub timeouts and side-effect retries (LFXV2-2645) Use settingsScanTimeout for full project-settings scans, retry indexer/FGA publishes after KV commit, and document core NATS delivery parity with committee-service rather than implying durable replay. Signed-off-by: Andres Tobon Co-authored-by: Cursor --- .../references/nats-messaging.md | 6 +++ cmd/project-api/main.go | 3 ++ internal/service/project_subscriber.go | 51 +++++++++++++++---- internal/service/project_subscriber_test.go | 21 ++++++++ 4 files changed, 71 insertions(+), 10 deletions(-) diff --git a/.claude/skills/project-service-dev/references/nats-messaging.md b/.claude/skills/project-service-dev/references/nats-messaging.md index a68a608e..d5c78f52 100644 --- a/.claude/skills/project-service-dev/references/nats-messaging.md +++ b/.claude/skills/project-service-dev/references/nats-messaging.md @@ -46,6 +46,12 @@ All six live as `Project*Subject` constants in `pkg/constants/nats.go`. "lfx.projects-api.project_link.created" // handled by HandleProjectLinkCreated ``` +### Inbound event delivery (core NATS) + +Lifecycle handlers (`invite_accepted`, `user.deleted`, document/link created) subscribe via core NATS queue groups (`QueueSubscribe`), the same transport used by `lfx-v2-committee-service` for `user.deleted` (LFXV2-2645). v1-sync-helper publishes deletions with core NATS `Publish`. Messages emitted while this service is disconnected are not automatically replayed; durable JetStream delivery or a periodic reconciliation job would be a cross-service follow-up. + +Full project-settings scans inside those handlers use `settingsScanTimeout` (2 minutes), not `notificationTimeout` (5 seconds). + ## Owned KV buckets | Bucket | Purpose | History | diff --git a/cmd/project-api/main.go b/cmd/project-api/main.go index 6d2b5e40..a5957faa 100644 --- a/cmd/project-api/main.go +++ b/cmd/project-api/main.go @@ -501,6 +501,9 @@ func createNatsSubcriptions(ctx context.Context, svc *ProjectsAPI, natsConn *nat {constants.ProjectDocumentCreatedSubject, svc.service.HandleProjectDocumentCreated}, {constants.ProjectLinkCreatedSubject, svc.service.HandleProjectLinkCreated}, } { + // Inbound lifecycle events use core NATS queue subscriptions, matching + // lfx-v2-committee-service (LFXV2-2645). v1-sync-helper publishes with core NATS + // Publish; missed messages during disconnect are not replayed automatically. slog.With("subject", eh.subject, "queue", queueName).Debug("subscribing to NATS subject") _, err := natsConn.QueueSubscribe(eh.subject, queueName, func(msg *nats.Msg) { msgCtx, end := internalnats.ExtractMsgContext(ctx, msg, eh.subject) diff --git a/internal/service/project_subscriber.go b/internal/service/project_subscriber.go index 72a57846..3c925d14 100644 --- a/internal/service/project_subscriber.go +++ b/internal/service/project_subscriber.go @@ -30,6 +30,16 @@ import ( // auth-service actor name lookup all run under this deadline. const notificationTimeout = 5 * time.Second +// settingsScanTimeout caps ListAllProjectsSettings and per-project reconciliation work. +// Unlike notificationTimeout (single RPC), a full project-settings bucket scan can require +// many sequential KV reads under load. +const settingsScanTimeout = 2 * time.Minute + +// scrubSideEffectMaxRetries is the number of attempts for indexer/FGA publishes after a +// successful KV write. Retries are independent of the username match so a transient NATS +// failure does not leave access tuples stale after the settings record was already scrubbed. +const scrubSideEffectMaxRetries = 4 + const ( roleWriter = "Writer" roleAuditor = "Auditor" @@ -403,7 +413,7 @@ func (s *ProjectsService) HandleInviteAccepted(ctx context.Context, msg domain.M } // Scan all project settings for email-only entries that match the recipient. - listCtx, listCancel := context.WithTimeout(ctx, notificationTimeout) + listCtx, listCancel := context.WithTimeout(ctx, settingsScanTimeout) allSettings, listErr := s.ProjectRepository.ListAllProjectsSettings(listCtx) listCancel() if listErr != nil { @@ -417,7 +427,7 @@ func (s *ProjectsService) HandleInviteAccepted(ctx context.Context, msg domain.M continue } projectUID := candidate.UID - promoteCtx, promoteCancel := context.WithTimeout(ctx, notificationTimeout) + promoteCtx, promoteCancel := context.WithTimeout(ctx, settingsScanTimeout) s.promoteInvitedUserInProjectSettings(promoteCtx, projectUID, normalizedEmail, event.AcceptedBy, event.UID, event.Role) promoteCancel() } @@ -539,7 +549,7 @@ func (s *ProjectsService) HandleUserDeleted(ctx context.Context, msg domain.Mess slog.InfoContext(ctx, "project_subscriber: scrubbing deleted user's username from project settings", "username", event.Username) - listCtx, listCancel := context.WithTimeout(ctx, notificationTimeout) + listCtx, listCancel := context.WithTimeout(ctx, settingsScanTimeout) allSettings, listErr := s.ProjectRepository.ListAllProjectsSettings(listCtx) listCancel() if listErr != nil { @@ -552,7 +562,7 @@ func (s *ProjectsService) HandleUserDeleted(ctx context.Context, msg domain.Mess if !projectSettingsHasUsername(candidate, event.Username) { continue } - scrubCtx, scrubCancel := context.WithTimeout(ctx, notificationTimeout) + scrubCtx, scrubCancel := context.WithTimeout(ctx, settingsScanTimeout) s.scrubUsernameFromProjectSettings(scrubCtx, candidate.UID, event.Username) scrubCancel() } @@ -666,7 +676,8 @@ func (s *ProjectsService) scrubUsernameFromProjectSettings(ctx context.Context, } // publishProjectSettingsScrubSideEffects reindexes scrubbed settings and refreshes OpenFGA -// access tuples. Best-effort: failures are logged but do not fail the scrub handler. +// access tuples. Indexer and FGA publishes retry independently of the username match so a +// transient NATS failure after the KV write does not leave stale access tuples. // ProjectSettingsUpdatedSubject is intentionally omitted to avoid role-change emails. func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Context, projectUID string, settings *models.ProjectSettings) { indexMsg := indexerTypes.IndexerMessageEnvelope{ @@ -674,9 +685,20 @@ func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Con Data: *settings, IndexingConfig: settings.IndexingConfig(projectUID), } - if indexErr := s.MessageBuilder.SendIndexerMessage(ctx, constants.IndexProjectSettingsSubject, indexMsg, false); indexErr != nil { + var indexErr error + for attempt := 0; attempt < scrubSideEffectMaxRetries; attempt++ { + indexErr = s.MessageBuilder.SendIndexerMessage(ctx, constants.IndexProjectSettingsSubject, indexMsg, false) + if indexErr == nil { + break + } + if attempt < scrubSideEffectMaxRetries-1 { + slog.DebugContext(ctx, "project_subscriber: retrying reindex after username scrub", + "attempt", attempt+1, "project_uid", projectUID) + } + } + if indexErr != nil { slog.WarnContext(ctx, "project_subscriber: failed to reindex project settings after username scrub", - constants.ErrKey, indexErr, "project_uid", projectUID) + constants.ErrKey, indexErr, "project_uid", projectUID, "attempts", scrubSideEffectMaxRetries) } projectBase, err := s.ProjectRepository.GetProjectBase(ctx, projectUID) @@ -686,10 +708,19 @@ func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Con return } fgaMsg := buildFGAUpdateAccessMessage(projectBase, settings) - if accessErr := s.MessageBuilder.SendAccessMessage(ctx, fgaconstants.GenericUpdateAccessSubject, fgaMsg, false); accessErr != nil { - slog.WarnContext(ctx, "project_subscriber: failed to publish FGA update after username scrub", - constants.ErrKey, accessErr, "project_uid", projectUID) + var accessErr error + for attempt := 0; attempt < scrubSideEffectMaxRetries; attempt++ { + accessErr = s.MessageBuilder.SendAccessMessage(ctx, fgaconstants.GenericUpdateAccessSubject, fgaMsg, false) + if accessErr == nil { + return + } + if attempt < scrubSideEffectMaxRetries-1 { + slog.DebugContext(ctx, "project_subscriber: retrying FGA publish after username scrub", + "attempt", attempt+1, "project_uid", projectUID) + } } + slog.WarnContext(ctx, "project_subscriber: failed to publish FGA update after username scrub", + constants.ErrKey, accessErr, "project_uid", projectUID, "attempts", scrubSideEffectMaxRetries) } // buildProjectURL constructs the deep-link URL for a project's overview page. diff --git a/internal/service/project_subscriber_test.go b/internal/service/project_subscriber_test.go index 9edc6683..39f4bbd3 100644 --- a/internal/service/project_subscriber_test.go +++ b/internal/service/project_subscriber_test.go @@ -1264,6 +1264,27 @@ func TestHandleUserDeleted(t *testing.T) { m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false).Return(nil) }, }, + { + name: "FGA publish retry — succeeds on second attempt", + payload: makeEvent(deletedUsername), + setupRepo: func(r *domain.MockProjectRepository) { + settings := &models.ProjectSettings{ + UID: projectUID, + Writers: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, + } + r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil) + r.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)).Return(nil) + r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) + }, + setupMsg: func(m *domain.MockMessageBuilder) { + m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil) + m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false). + Return(errors.New("transient nats failure")).Once() + m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false). + Return(nil).Once() + }, + }, { name: "multiple projects — only matching project updated", payload: makeEvent(deletedUsername), From b5444b063fab1d0f29f2707247c28bf1f8cd14f7 Mon Sep 17 00:00:00 2001 From: Andres Tobon Date: Mon, 27 Jul 2026 13:12:22 -0700 Subject: [PATCH 05/13] fix(review): reload settings from KV before scrub side-effect publishes Re-read project settings on each indexer/FGA attempt so retries cannot publish a stale snapshot after a concurrent settings update. Signed-off-by: Andres Tobon Co-authored-by: Cursor --- internal/service/project_subscriber.go | 80 +++++++++++---------- internal/service/project_subscriber_test.go | 13 ++-- 2 files changed, 50 insertions(+), 43 deletions(-) diff --git a/internal/service/project_subscriber.go b/internal/service/project_subscriber.go index 3c925d14..38d43803 100644 --- a/internal/service/project_subscriber.go +++ b/internal/service/project_subscriber.go @@ -662,7 +662,7 @@ func (s *ProjectsService) scrubUsernameFromProjectSettings(ctx context.Context, if updateErr == nil { slog.InfoContext(ctx, "project_subscriber: cleared username from project settings", "project_uid", projectUID, "username", username) - s.publishProjectSettingsScrubSideEffects(ctx, projectUID, settings) + s.publishProjectSettingsScrubSideEffects(ctx, projectUID) return } if !errors.Is(updateErr, domain.ErrRevisionMismatch) || attempt == maxRetries-1 { @@ -676,51 +676,57 @@ func (s *ProjectsService) scrubUsernameFromProjectSettings(ctx context.Context, } // publishProjectSettingsScrubSideEffects reindexes scrubbed settings and refreshes OpenFGA -// access tuples. Indexer and FGA publishes retry independently of the username match so a -// transient NATS failure after the KV write does not leave stale access tuples. +// access tuples. Each attempt reloads the current KV record so retries cannot publish a +// stale snapshot if another writer updated settings concurrently after the scrub commit. // ProjectSettingsUpdatedSubject is intentionally omitted to avoid role-change emails. -func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Context, projectUID string, settings *models.ProjectSettings) { - indexMsg := indexerTypes.IndexerMessageEnvelope{ - Action: indexerConstants.ActionUpdated, - Data: *settings, - IndexingConfig: settings.IndexingConfig(projectUID), - } - var indexErr error +func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Context, projectUID string) { for attempt := 0; attempt < scrubSideEffectMaxRetries; attempt++ { - indexErr = s.MessageBuilder.SendIndexerMessage(ctx, constants.IndexProjectSettingsSubject, indexMsg, false) - if indexErr == nil { - break + settings, _, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID) + if err != nil { + slog.WarnContext(ctx, "project_subscriber: failed to reload settings for scrub side effects", + constants.ErrKey, err, "project_uid", projectUID) + return } - if attempt < scrubSideEffectMaxRetries-1 { - slog.DebugContext(ctx, "project_subscriber: retrying reindex after username scrub", - "attempt", attempt+1, "project_uid", projectUID) + if settings == nil { + return } - } - if indexErr != nil { - slog.WarnContext(ctx, "project_subscriber: failed to reindex project settings after username scrub", - constants.ErrKey, indexErr, "project_uid", projectUID, "attempts", scrubSideEffectMaxRetries) - } - projectBase, err := s.ProjectRepository.GetProjectBase(ctx, projectUID) - if err != nil { - slog.WarnContext(ctx, "project_subscriber: failed to load project for FGA refresh after username scrub", - constants.ErrKey, err, "project_uid", projectUID) - return - } - fgaMsg := buildFGAUpdateAccessMessage(projectBase, settings) - var accessErr error - for attempt := 0; attempt < scrubSideEffectMaxRetries; attempt++ { - accessErr = s.MessageBuilder.SendAccessMessage(ctx, fgaconstants.GenericUpdateAccessSubject, fgaMsg, false) - if accessErr == nil { + indexMsg := indexerTypes.IndexerMessageEnvelope{ + Action: indexerConstants.ActionUpdated, + Data: *settings, + IndexingConfig: settings.IndexingConfig(projectUID), + } + indexErr := s.MessageBuilder.SendIndexerMessage(ctx, constants.IndexProjectSettingsSubject, indexMsg, false) + + var accessErr error + projectBase, baseErr := s.ProjectRepository.GetProjectBase(ctx, projectUID) + if baseErr != nil { + slog.WarnContext(ctx, "project_subscriber: failed to load project for FGA refresh after username scrub", + constants.ErrKey, baseErr, "project_uid", projectUID) + } else { + fgaMsg := buildFGAUpdateAccessMessage(projectBase, settings) + accessErr = s.MessageBuilder.SendAccessMessage(ctx, fgaconstants.GenericUpdateAccessSubject, fgaMsg, false) + } + + if indexErr == nil && accessErr == nil { return } - if attempt < scrubSideEffectMaxRetries-1 { - slog.DebugContext(ctx, "project_subscriber: retrying FGA publish after username scrub", - "attempt", attempt+1, "project_uid", projectUID) + + if attempt == scrubSideEffectMaxRetries-1 { + if indexErr != nil { + slog.WarnContext(ctx, "project_subscriber: failed to reindex project settings after username scrub", + constants.ErrKey, indexErr, "project_uid", projectUID, "attempts", scrubSideEffectMaxRetries) + } + if accessErr != nil { + slog.WarnContext(ctx, "project_subscriber: failed to publish FGA update after username scrub", + constants.ErrKey, accessErr, "project_uid", projectUID, "attempts", scrubSideEffectMaxRetries) + } + return } + + slog.DebugContext(ctx, "project_subscriber: retrying scrub side effects after reload", + "attempt", attempt+1, "project_uid", projectUID) } - slog.WarnContext(ctx, "project_subscriber: failed to publish FGA update after username scrub", - constants.ErrKey, accessErr, "project_uid", projectUID, "attempts", scrubSideEffectMaxRetries) } // buildProjectURL constructs the deep-link URL for a project's overview page. diff --git a/internal/service/project_subscriber_test.go b/internal/service/project_subscriber_test.go index 39f4bbd3..90d31429 100644 --- a/internal/service/project_subscriber_test.go +++ b/internal/service/project_subscriber_test.go @@ -1182,7 +1182,7 @@ func TestHandleUserDeleted(t *testing.T) { Writers: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(2) r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { return len(s.Writers) == 1 && s.Writers[0].Username == "" && s.Writers[0].Email == "deleted@example.com" }), uint64(1)).Return(nil) @@ -1224,6 +1224,7 @@ func TestHandleUserDeleted(t *testing.T) { r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { return len(s.Auditors) == 1 && s.Auditors[0].Username == "" }), uint64(2)).Return(nil).Once() + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(secondRead, uint64(2), nil).Once() r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) }, setupMsg: func(m *domain.MockMessageBuilder) { @@ -1250,7 +1251,7 @@ func TestHandleUserDeleted(t *testing.T) { OpportunityOwner: &models.UserInfo{Username: deletedUsername, Email: "oo@example.com"}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(2) r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { return len(s.MeetingCoordinators) == 1 && s.MeetingCoordinators[0].Username == "" && s.ExecutiveDirector != nil && s.ExecutiveDirector.Username == "" && @@ -1273,12 +1274,12 @@ func TestHandleUserDeleted(t *testing.T) { Writers: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(3) r.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)).Return(nil) - r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) + r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil).Times(2) }, setupMsg: func(m *domain.MockMessageBuilder) { - m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil) + m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil).Times(2) m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false). Return(errors.New("transient nats failure")).Once() m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false). @@ -1298,7 +1299,7 @@ func TestHandleUserDeleted(t *testing.T) { Writers: []models.UserInfo{{Username: "other", Email: "other@example.com"}}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{match, other}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(match, uint64(1), nil) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(match, uint64(1), nil).Times(2) r.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)).Return(nil) r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) }, From c9c2f86218b371ab8f170a2c0b3f61ba0d8d5f3c Mon Sep 17 00:00:00 2001 From: Andres Tobon Date: Mon, 27 Jul 2026 13:23:09 -0700 Subject: [PATCH 06/13] fix: retry FGA refresh when GetProjectBase fails after username scrub Propagate GetProjectBase errors into the side-effect retry loop and retry settings reload failures instead of exiting early so FGA tuples stay in sync. Signed-off-by: Andres Tobon Co-authored-by: Cursor --- internal/service/project_subscriber.go | 12 +++++++++--- internal/service/project_subscriber_test.go | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/internal/service/project_subscriber.go b/internal/service/project_subscriber.go index 38d43803..7eb020fb 100644 --- a/internal/service/project_subscriber.go +++ b/internal/service/project_subscriber.go @@ -683,9 +683,14 @@ func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Con for attempt := 0; attempt < scrubSideEffectMaxRetries; attempt++ { settings, _, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID) if err != nil { - slog.WarnContext(ctx, "project_subscriber: failed to reload settings for scrub side effects", - constants.ErrKey, err, "project_uid", projectUID) - return + if attempt == scrubSideEffectMaxRetries-1 { + slog.WarnContext(ctx, "project_subscriber: failed to reload settings for scrub side effects", + constants.ErrKey, err, "project_uid", projectUID, "attempts", scrubSideEffectMaxRetries) + return + } + slog.DebugContext(ctx, "project_subscriber: retrying scrub side effects after settings reload failure", + "attempt", attempt+1, "project_uid", projectUID) + continue } if settings == nil { return @@ -703,6 +708,7 @@ func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Con if baseErr != nil { slog.WarnContext(ctx, "project_subscriber: failed to load project for FGA refresh after username scrub", constants.ErrKey, baseErr, "project_uid", projectUID) + accessErr = baseErr } else { fgaMsg := buildFGAUpdateAccessMessage(projectBase, settings) accessErr = s.MessageBuilder.SendAccessMessage(ctx, fgaconstants.GenericUpdateAccessSubject, fgaMsg, false) diff --git a/internal/service/project_subscriber_test.go b/internal/service/project_subscriber_test.go index 90d31429..3facfb1b 100644 --- a/internal/service/project_subscriber_test.go +++ b/internal/service/project_subscriber_test.go @@ -1265,6 +1265,27 @@ func TestHandleUserDeleted(t *testing.T) { m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false).Return(nil) }, }, + { + name: "GetProjectBase retry — succeeds on second attempt", + payload: makeEvent(deletedUsername), + setupRepo: func(r *domain.MockProjectRepository) { + settings := &models.ProjectSettings{ + UID: projectUID, + Writers: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, + } + r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(3) + r.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)).Return(nil) + r.On("GetProjectBase", mock.Anything, projectUID). + Return((*models.ProjectBase)(nil), errors.New("transient read failure")).Once() + r.On("GetProjectBase", mock.Anything, projectUID). + Return(&models.ProjectBase{UID: projectUID}, nil).Once() + }, + setupMsg: func(m *domain.MockMessageBuilder) { + m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil).Times(2) + m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false).Return(nil).Once() + }, + }, { name: "FGA publish retry — succeeds on second attempt", payload: makeEvent(deletedUsername), From d9a8ebafa0c7d19a536bd0d8626165fff833e1c2 Mon Sep 17 00:00:00 2001 From: Andres Tobon Date: Mon, 27 Jul 2026 14:37:40 -0700 Subject: [PATCH 07/13] fix: inject service auth for indexer publishes from NATS handlers NATS queue subscribers have no inbound JWT, so username-scrub and invite promotion indexer messages were missing the authorization header required by indexer V2 transactions. Signed-off-by: Andres Tobon Co-authored-by: Cursor --- internal/service/project_subscriber.go | 18 +++++++++++++++++- internal/service/project_subscriber_test.go | 18 ++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/internal/service/project_subscriber.go b/internal/service/project_subscriber.go index 7eb020fb..2d923cbb 100644 --- a/internal/service/project_subscriber.go +++ b/internal/service/project_subscriber.go @@ -40,6 +40,11 @@ const settingsScanTimeout = 2 * time.Minute // failure does not leave access tuples stale after the settings record was already scrubbed. const scrubSideEffectMaxRetries = 4 +// serviceAuthBearer is the static JWT audience token used for background NATS handler +// side effects (indexer/FGA) that have no originating HTTP request context. +// Pattern matches lfx-v2-committee-service message_handler.go. +const serviceAuthBearer = "Bearer lfx-v2-project-service" + const ( roleWriter = "Writer" roleAuditor = "Auditor" @@ -515,7 +520,7 @@ func (s *ProjectsService) promoteInvitedUserInProjectSettings(ctx context.Contex Data: *settings, IndexingConfig: settings.IndexingConfig(projectUID), } - if indexErr := s.MessageBuilder.SendIndexerMessage(ctx, constants.IndexProjectSettingsSubject, indexMsg, false); indexErr != nil { + if indexErr := s.MessageBuilder.SendIndexerMessage(ctxWithServiceAuth(ctx), constants.IndexProjectSettingsSubject, indexMsg, false); indexErr != nil { slog.WarnContext(ctx, "project_subscriber: failed to reindex project settings after invite acceptance", constants.ErrKey, indexErr, "project_uid", projectUID) } @@ -680,6 +685,7 @@ func (s *ProjectsService) scrubUsernameFromProjectSettings(ctx context.Context, // stale snapshot if another writer updated settings concurrently after the scrub commit. // ProjectSettingsUpdatedSubject is intentionally omitted to avoid role-change emails. func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Context, projectUID string) { + ctx = ctxWithServiceAuth(ctx) for attempt := 0; attempt < scrubSideEffectMaxRetries; attempt++ { settings, _, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID) if err != nil { @@ -735,6 +741,16 @@ func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Con } } +// ctxWithServiceAuth returns ctx with a service-identity bearer when no inbound JWT is present. +// NATS queue subscribers have no HTTP middleware, but indexer V2 transactions require an +// authorization header in the message envelope. +func ctxWithServiceAuth(ctx context.Context) context.Context { + if _, ok := ctx.Value(constants.AuthorizationContextID).(string); ok { + return ctx + } + return context.WithValue(ctx, constants.AuthorizationContextID, serviceAuthBearer) +} + // buildProjectURL constructs the deep-link URL for a project's overview page. func buildProjectURL(baseURL, slug string) string { base := strings.TrimRight(baseURL, "/") + "/project/overview" diff --git a/internal/service/project_subscriber_test.go b/internal/service/project_subscriber_test.go index 3facfb1b..87bba85c 100644 --- a/internal/service/project_subscriber_test.go +++ b/internal/service/project_subscriber_test.go @@ -1150,6 +1150,24 @@ func TestHandleInviteAccepted(t *testing.T) { } } +func TestCtxWithServiceAuth(t *testing.T) { + t.Run("injects service bearer when absent", func(t *testing.T) { + ctx := ctxWithServiceAuth(context.Background()) + auth, ok := ctx.Value(constants.AuthorizationContextID).(string) + require.True(t, ok) + assert.Equal(t, serviceAuthBearer, auth) + }) + + t.Run("preserves existing bearer", func(t *testing.T) { + existing := "Bearer caller-jwt" + ctx := context.WithValue(context.Background(), constants.AuthorizationContextID, existing) + ctx = ctxWithServiceAuth(ctx) + auth, ok := ctx.Value(constants.AuthorizationContextID).(string) + require.True(t, ok) + assert.Equal(t, existing, auth) + }) +} + func TestHandleUserDeleted(t *testing.T) { const deletedUsername = "deleted.user" const projectUID = "proj-1" From 488416894d69c254b2a8c5d969c3c1492e52a8c2 Mon Sep 17 00:00:00 2001 From: Andres Tobon Date: Mon, 27 Jul 2026 14:46:20 -0700 Subject: [PATCH 08/13] fix(review): guard username scrub reuse and stale side-effect publishes Skip scrub when auth still maps an entry's email to the deleted LFID, confirm KV revision before indexer/FGA publishes, and relax flaky async mock expectations. Signed-off-by: Andres Tobon Co-authored-by: Cursor --- internal/service/project_subscriber.go | 123 +++++++++++++------- internal/service/project_subscriber_test.go | 49 ++++++-- 2 files changed, 120 insertions(+), 52 deletions(-) diff --git a/internal/service/project_subscriber.go b/internal/service/project_subscriber.go index 2d923cbb..20729048 100644 --- a/internal/service/project_subscriber.go +++ b/internal/service/project_subscriber.go @@ -607,41 +607,63 @@ func projectSettingsHasUsername(s *models.ProjectSettings, username string) bool return false } -// scrubUsernameInProjectSettings clears username on every matching entry in settings. -// Returns true when at least one field was changed. -func scrubUsernameInProjectSettings(settings *models.ProjectSettings, username string) bool { +// scrubUsernameInProjectSettings clears username on every matching entry in settings +// that still represents the deleted account. Returns true when at least one field was changed. +func (s *ProjectsService) scrubUsernameInProjectSettings(ctx context.Context, settings *models.ProjectSettings, username string) bool { changed := false - for i := range settings.Writers { - if settings.Writers[i].Username == username { - settings.Writers[i].Username = "" - changed = true + clearIfMatch := func(u *models.UserInfo) { + if u == nil || u.Username != username { + return + } + if !s.shouldScrubSettingsUsername(ctx, *u, username) { + slog.DebugContext(ctx, "project_subscriber: skipping username scrub — email still maps to active LFID", + "username", username, "email", u.Email) + return } + u.Username = "" + changed = true + } + + for i := range settings.Writers { + clearIfMatch(&settings.Writers[i]) } for i := range settings.Auditors { - if settings.Auditors[i].Username == username { - settings.Auditors[i].Username = "" - changed = true - } + clearIfMatch(&settings.Auditors[i]) } for i := range settings.MeetingCoordinators { - if settings.MeetingCoordinators[i].Username == username { - settings.MeetingCoordinators[i].Username = "" - changed = true - } + clearIfMatch(&settings.MeetingCoordinators[i]) } - if settings.ExecutiveDirector != nil && settings.ExecutiveDirector.Username == username { - settings.ExecutiveDirector.Username = "" - changed = true + clearIfMatch(settings.ExecutiveDirector) + clearIfMatch(settings.ProgramManager) + clearIfMatch(settings.OpportunityOwner) + return changed +} + +// shouldScrubSettingsUsername reports whether a settings entry carrying deletedUsername should +// be cleared. When the entry has an email, auth is consulted so a reassigned LFID reused by a +// new account (same username string, different lifecycle) is not scrubbed. +func (s *ProjectsService) shouldScrubSettingsUsername(ctx context.Context, u models.UserInfo, deletedUsername string) bool { + email := strings.ToLower(strings.TrimSpace(u.Email)) + if email == "" { + return true } - if settings.ProgramManager != nil && settings.ProgramManager.Username == username { - settings.ProgramManager.Username = "" - changed = true + if s.UserReader == nil { + return true } - if settings.OpportunityOwner != nil && settings.OpportunityOwner.Username == username { - settings.OpportunityOwner.Username = "" - changed = true + + lookupCtx, cancel := context.WithTimeout(ctx, notificationTimeout) + defer cancel() + + resolved, err := s.UserReader.UsernameByEmail(lookupCtx, email) + if err != nil { + if errors.Is(err, domain.ErrUserNotFound) { + return true + } + slog.WarnContext(ctx, "project_subscriber: auth lookup failed during username scrub — skipping entry", + constants.ErrKey, err, "email", u.Email) + return false } - return changed + return resolved != deletedUsername } // scrubUsernameFromProjectSettings fetches settings for a single project, clears the @@ -659,7 +681,7 @@ func (s *ProjectsService) scrubUsernameFromProjectSettings(ctx context.Context, return } - if !scrubUsernameInProjectSettings(settings, username) { + if !s.scrubUsernameInProjectSettings(ctx, settings, username) { return } @@ -681,13 +703,13 @@ func (s *ProjectsService) scrubUsernameFromProjectSettings(ctx context.Context, } // publishProjectSettingsScrubSideEffects reindexes scrubbed settings and refreshes OpenFGA -// access tuples. Each attempt reloads the current KV record so retries cannot publish a -// stale snapshot if another writer updated settings concurrently after the scrub commit. -// ProjectSettingsUpdatedSubject is intentionally omitted to avoid role-change emails. +// access tuples. Each attempt reloads the current KV record and confirms the revision has +// not advanced before publishing, so a concurrent settings write cannot be overwritten by a +// stale snapshot. ProjectSettingsUpdatedSubject is intentionally omitted to avoid role-change emails. func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Context, projectUID string) { ctx = ctxWithServiceAuth(ctx) for attempt := 0; attempt < scrubSideEffectMaxRetries; attempt++ { - settings, _, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID) + settings, revision, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID) if err != nil { if attempt == scrubSideEffectMaxRetries-1 { slog.WarnContext(ctx, "project_subscriber: failed to reload settings for scrub side effects", @@ -702,6 +724,35 @@ func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Con return } + projectBase, baseErr := s.ProjectRepository.GetProjectBase(ctx, projectUID) + if baseErr != nil { + slog.WarnContext(ctx, "project_subscriber: failed to load project for FGA refresh after username scrub", + constants.ErrKey, baseErr, "project_uid", projectUID) + if attempt == scrubSideEffectMaxRetries-1 { + return + } + slog.DebugContext(ctx, "project_subscriber: retrying scrub side effects after project load failure", + "attempt", attempt+1, "project_uid", projectUID) + continue + } + + _, confirmRevision, confirmErr := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID) + if confirmErr != nil { + if attempt == scrubSideEffectMaxRetries-1 { + slog.WarnContext(ctx, "project_subscriber: failed to confirm settings revision for scrub side effects", + constants.ErrKey, confirmErr, "project_uid", projectUID, "attempts", scrubSideEffectMaxRetries) + return + } + slog.DebugContext(ctx, "project_subscriber: retrying scrub side effects after revision confirm failure", + "attempt", attempt+1, "project_uid", projectUID) + continue + } + if confirmRevision != revision { + slog.DebugContext(ctx, "project_subscriber: settings revision advanced before side-effect publish — retrying", + "expected_revision", revision, "current_revision", confirmRevision, "project_uid", projectUID) + continue + } + indexMsg := indexerTypes.IndexerMessageEnvelope{ Action: indexerConstants.ActionUpdated, Data: *settings, @@ -709,16 +760,8 @@ func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Con } indexErr := s.MessageBuilder.SendIndexerMessage(ctx, constants.IndexProjectSettingsSubject, indexMsg, false) - var accessErr error - projectBase, baseErr := s.ProjectRepository.GetProjectBase(ctx, projectUID) - if baseErr != nil { - slog.WarnContext(ctx, "project_subscriber: failed to load project for FGA refresh after username scrub", - constants.ErrKey, baseErr, "project_uid", projectUID) - accessErr = baseErr - } else { - fgaMsg := buildFGAUpdateAccessMessage(projectBase, settings) - accessErr = s.MessageBuilder.SendAccessMessage(ctx, fgaconstants.GenericUpdateAccessSubject, fgaMsg, false) - } + fgaMsg := buildFGAUpdateAccessMessage(projectBase, settings) + accessErr := s.MessageBuilder.SendAccessMessage(ctx, fgaconstants.GenericUpdateAccessSubject, fgaMsg, false) if indexErr == nil && accessErr == nil { return diff --git a/internal/service/project_subscriber_test.go b/internal/service/project_subscriber_test.go index 87bba85c..2a76409f 100644 --- a/internal/service/project_subscriber_test.go +++ b/internal/service/project_subscriber_test.go @@ -1178,10 +1178,11 @@ func TestHandleUserDeleted(t *testing.T) { } tests := []struct { - name string - payload any - setupRepo func(*domain.MockProjectRepository) - setupMsg func(*domain.MockMessageBuilder) + name string + payload any + setupRepo func(*domain.MockProjectRepository) + setupMsg func(*domain.MockMessageBuilder) + setupUserReader func(*domain.MockUserReader) }{ { name: "malformed payload — returns nil without crashing", @@ -1200,7 +1201,7 @@ func TestHandleUserDeleted(t *testing.T) { Writers: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(2) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(3) r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { return len(s.Writers) == 1 && s.Writers[0].Username == "" && s.Writers[0].Email == "deleted@example.com" }), uint64(1)).Return(nil) @@ -1242,7 +1243,7 @@ func TestHandleUserDeleted(t *testing.T) { r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { return len(s.Auditors) == 1 && s.Auditors[0].Username == "" }), uint64(2)).Return(nil).Once() - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(secondRead, uint64(2), nil).Once() + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(secondRead, uint64(2), nil).Times(2) r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) }, setupMsg: func(m *domain.MockMessageBuilder) { @@ -1250,6 +1251,21 @@ func TestHandleUserDeleted(t *testing.T) { m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false).Return(nil) }, }, + { + name: "reuse guard — skips scrub when email still maps to username", + payload: makeEvent(deletedUsername), + setupRepo: func(r *domain.MockProjectRepository) { + settings := &models.ProjectSettings{ + UID: projectUID, + Writers: []models.UserInfo{{Username: deletedUsername, Email: "reassigned@example.com"}}, + } + r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Once() + }, + setupUserReader: func(u *domain.MockUserReader) { + u.On("UsernameByEmail", mock.Anything, "reassigned@example.com").Return(deletedUsername, nil) + }, + }, { name: "ListAllProjectsSettings error — early return", payload: makeEvent(deletedUsername), @@ -1269,7 +1285,7 @@ func TestHandleUserDeleted(t *testing.T) { OpportunityOwner: &models.UserInfo{Username: deletedUsername, Email: "oo@example.com"}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(2) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(3) r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { return len(s.MeetingCoordinators) == 1 && s.MeetingCoordinators[0].Username == "" && s.ExecutiveDirector != nil && s.ExecutiveDirector.Username == "" && @@ -1292,7 +1308,7 @@ func TestHandleUserDeleted(t *testing.T) { Writers: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(3) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(4) r.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)).Return(nil) r.On("GetProjectBase", mock.Anything, projectUID). Return((*models.ProjectBase)(nil), errors.New("transient read failure")).Once() @@ -1300,7 +1316,7 @@ func TestHandleUserDeleted(t *testing.T) { Return(&models.ProjectBase{UID: projectUID}, nil).Once() }, setupMsg: func(m *domain.MockMessageBuilder) { - m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil).Times(2) + m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil).Maybe() m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false).Return(nil).Once() }, }, @@ -1313,12 +1329,12 @@ func TestHandleUserDeleted(t *testing.T) { Writers: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(3) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(5) r.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)).Return(nil) r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil).Times(2) }, setupMsg: func(m *domain.MockMessageBuilder) { - m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil).Times(2) + m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil).Maybe() m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false). Return(errors.New("transient nats failure")).Once() m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false). @@ -1338,7 +1354,7 @@ func TestHandleUserDeleted(t *testing.T) { Writers: []models.UserInfo{{Username: "other", Email: "other@example.com"}}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{match, other}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(match, uint64(1), nil).Times(2) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(match, uint64(1), nil).Times(3) r.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)).Return(nil) r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) }, @@ -1353,6 +1369,7 @@ func TestHandleUserDeleted(t *testing.T) { t.Run(tt.name, func(t *testing.T) { mockRepo := &domain.MockProjectRepository{} mockMsg := &domain.MockMessageBuilder{} + var mockUserReader *domain.MockUserReader if tt.setupRepo != nil { tt.setupRepo(mockRepo) } @@ -1364,6 +1381,11 @@ func TestHandleUserDeleted(t *testing.T) { ProjectRepository: mockRepo, MessageBuilder: mockMsg, } + if tt.setupUserReader != nil { + mockUserReader = &domain.MockUserReader{} + svc.UserReader = mockUserReader + tt.setupUserReader(mockUserReader) + } var data []byte if raw, ok := tt.payload.([]byte); ok { @@ -1377,6 +1399,9 @@ func TestHandleUserDeleted(t *testing.T) { mockRepo.AssertExpectations(t) mockMsg.AssertExpectations(t) + if mockUserReader != nil { + mockUserReader.AssertExpectations(t) + } }) } } From dfe9d6ad987cca2bfe1ed7b5941fdf26432d74a0 Mon Sep 17 00:00:00 2001 From: Andres Tobon Date: Mon, 27 Jul 2026 16:20:11 -0700 Subject: [PATCH 09/13] refactor(users): address username scrub review feedback Drop redundant revision confirm before side effects, unify scrubMaxRetries, rename scrub helpers, document no-email tradeoff, remove email from WARN logs, and expand shouldScrubSettingsUsername test coverage. Co-authored-by: Cursor Signed-off-by: Andres Tobon Co-authored-by: Cursor --- internal/service/project_subscriber.go | 71 +++++-------- internal/service/project_subscriber_test.go | 111 ++++++++++++++++++-- 2 files changed, 134 insertions(+), 48 deletions(-) diff --git a/internal/service/project_subscriber.go b/internal/service/project_subscriber.go index 20729048..32912162 100644 --- a/internal/service/project_subscriber.go +++ b/internal/service/project_subscriber.go @@ -35,10 +35,10 @@ const notificationTimeout = 5 * time.Second // many sequential KV reads under load. const settingsScanTimeout = 2 * time.Minute -// scrubSideEffectMaxRetries is the number of attempts for indexer/FGA publishes after a -// successful KV write. Retries are independent of the username match so a transient NATS -// failure does not leave access tuples stale after the settings record was already scrubbed. -const scrubSideEffectMaxRetries = 4 +// scrubMaxRetries is the number of attempts for settings KV writes and indexer/FGA publishes +// after a successful scrub. Retries are independent of the username match so a transient +// conflict or NATS failure does not leave access tuples stale after settings were scrubbed. +const scrubMaxRetries = 4 // serviceAuthBearer is the static JWT audience token used for background NATS handler // side effects (indexer/FGA) that have no originating HTTP request context. @@ -568,7 +568,7 @@ func (s *ProjectsService) HandleUserDeleted(ctx context.Context, msg domain.Mess continue } scrubCtx, scrubCancel := context.WithTimeout(ctx, settingsScanTimeout) - s.scrubUsernameFromProjectSettings(scrubCtx, candidate.UID, event.Username) + s.scrubProjectSettingsUsername(scrubCtx, candidate.UID, event.Username) scrubCancel() } @@ -607,9 +607,9 @@ func projectSettingsHasUsername(s *models.ProjectSettings, username string) bool return false } -// scrubUsernameInProjectSettings clears username on every matching entry in settings +// clearUsernameInSettings clears username on every matching entry in settings // that still represents the deleted account. Returns true when at least one field was changed. -func (s *ProjectsService) scrubUsernameInProjectSettings(ctx context.Context, settings *models.ProjectSettings, username string) bool { +func (s *ProjectsService) clearUsernameInSettings(ctx context.Context, settings *models.ProjectSettings, username string) bool { changed := false clearIfMatch := func(u *models.UserInfo) { if u == nil || u.Username != username { @@ -642,6 +642,11 @@ func (s *ProjectsService) scrubUsernameInProjectSettings(ctx context.Context, se // shouldScrubSettingsUsername reports whether a settings entry carrying deletedUsername should // be cleared. When the entry has an email, auth is consulted so a reassigned LFID reused by a // new account (same username string, different lifecycle) is not scrubbed. +// +// Entries without email always scrub when the username matches. That is intentional: M2M and +// legacy email-less settings cannot be disambiguated via auth lookup. Downstream scrub is gated +// by the v1-sync-helper user.deleted publish ACL (project-api subscribes only from trusted +// service accounts); carrying email in the event would be a follow-up hardening step. func (s *ProjectsService) shouldScrubSettingsUsername(ctx context.Context, u models.UserInfo, deletedUsername string) bool { email := strings.ToLower(strings.TrimSpace(u.Email)) if email == "" { @@ -660,17 +665,16 @@ func (s *ProjectsService) shouldScrubSettingsUsername(ctx context.Context, u mod return true } slog.WarnContext(ctx, "project_subscriber: auth lookup failed during username scrub — skipping entry", - constants.ErrKey, err, "email", u.Email) + constants.ErrKey, err) return false } return resolved != deletedUsername } -// scrubUsernameFromProjectSettings fetches settings for a single project, clears the +// scrubProjectSettingsUsername fetches settings for a single project, clears the // username on any matching entry, persists, and reindexes. Retries on revision conflicts. -func (s *ProjectsService) scrubUsernameFromProjectSettings(ctx context.Context, projectUID, username string) { - const maxRetries = 4 - for attempt := 0; attempt < maxRetries; attempt++ { +func (s *ProjectsService) scrubProjectSettingsUsername(ctx context.Context, projectUID, username string) { + for attempt := 0; attempt < scrubMaxRetries; attempt++ { settings, revision, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID) if err != nil { slog.DebugContext(ctx, "project_subscriber: failed to get settings for username scrub — skipping", @@ -681,7 +685,7 @@ func (s *ProjectsService) scrubUsernameFromProjectSettings(ctx context.Context, return } - if !s.scrubUsernameInProjectSettings(ctx, settings, username) { + if !s.clearUsernameInSettings(ctx, settings, username) { return } @@ -692,7 +696,7 @@ func (s *ProjectsService) scrubUsernameFromProjectSettings(ctx context.Context, s.publishProjectSettingsScrubSideEffects(ctx, projectUID) return } - if !errors.Is(updateErr, domain.ErrRevisionMismatch) || attempt == maxRetries-1 { + if !errors.Is(updateErr, domain.ErrRevisionMismatch) || attempt == scrubMaxRetries-1 { slog.WarnContext(ctx, "project_subscriber: failed to clear username from project settings", constants.ErrKey, updateErr, "project_uid", projectUID) return @@ -703,17 +707,17 @@ func (s *ProjectsService) scrubUsernameFromProjectSettings(ctx context.Context, } // publishProjectSettingsScrubSideEffects reindexes scrubbed settings and refreshes OpenFGA -// access tuples. Each attempt reloads the current KV record and confirms the revision has -// not advanced before publishing, so a concurrent settings write cannot be overwritten by a -// stale snapshot. ProjectSettingsUpdatedSubject is intentionally omitted to avoid role-change emails. +// access tuples. Each attempt reloads the current KV record before publishing; indexer and FGA +// projections are full-state and idempotent. ProjectSettingsUpdatedSubject is intentionally +// omitted to avoid role-change emails. func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Context, projectUID string) { ctx = ctxWithServiceAuth(ctx) - for attempt := 0; attempt < scrubSideEffectMaxRetries; attempt++ { - settings, revision, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID) + for attempt := 0; attempt < scrubMaxRetries; attempt++ { + settings, _, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID) if err != nil { - if attempt == scrubSideEffectMaxRetries-1 { + if attempt == scrubMaxRetries-1 { slog.WarnContext(ctx, "project_subscriber: failed to reload settings for scrub side effects", - constants.ErrKey, err, "project_uid", projectUID, "attempts", scrubSideEffectMaxRetries) + constants.ErrKey, err, "project_uid", projectUID, "attempts", scrubMaxRetries) return } slog.DebugContext(ctx, "project_subscriber: retrying scrub side effects after settings reload failure", @@ -728,7 +732,7 @@ func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Con if baseErr != nil { slog.WarnContext(ctx, "project_subscriber: failed to load project for FGA refresh after username scrub", constants.ErrKey, baseErr, "project_uid", projectUID) - if attempt == scrubSideEffectMaxRetries-1 { + if attempt == scrubMaxRetries-1 { return } slog.DebugContext(ctx, "project_subscriber: retrying scrub side effects after project load failure", @@ -736,23 +740,6 @@ func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Con continue } - _, confirmRevision, confirmErr := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID) - if confirmErr != nil { - if attempt == scrubSideEffectMaxRetries-1 { - slog.WarnContext(ctx, "project_subscriber: failed to confirm settings revision for scrub side effects", - constants.ErrKey, confirmErr, "project_uid", projectUID, "attempts", scrubSideEffectMaxRetries) - return - } - slog.DebugContext(ctx, "project_subscriber: retrying scrub side effects after revision confirm failure", - "attempt", attempt+1, "project_uid", projectUID) - continue - } - if confirmRevision != revision { - slog.DebugContext(ctx, "project_subscriber: settings revision advanced before side-effect publish — retrying", - "expected_revision", revision, "current_revision", confirmRevision, "project_uid", projectUID) - continue - } - indexMsg := indexerTypes.IndexerMessageEnvelope{ Action: indexerConstants.ActionUpdated, Data: *settings, @@ -767,14 +754,14 @@ func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Con return } - if attempt == scrubSideEffectMaxRetries-1 { + if attempt == scrubMaxRetries-1 { if indexErr != nil { slog.WarnContext(ctx, "project_subscriber: failed to reindex project settings after username scrub", - constants.ErrKey, indexErr, "project_uid", projectUID, "attempts", scrubSideEffectMaxRetries) + constants.ErrKey, indexErr, "project_uid", projectUID, "attempts", scrubMaxRetries) } if accessErr != nil { slog.WarnContext(ctx, "project_subscriber: failed to publish FGA update after username scrub", - constants.ErrKey, accessErr, "project_uid", projectUID, "attempts", scrubSideEffectMaxRetries) + constants.ErrKey, accessErr, "project_uid", projectUID, "attempts", scrubMaxRetries) } return } diff --git a/internal/service/project_subscriber_test.go b/internal/service/project_subscriber_test.go index 2a76409f..062f8088 100644 --- a/internal/service/project_subscriber_test.go +++ b/internal/service/project_subscriber_test.go @@ -1201,7 +1201,7 @@ func TestHandleUserDeleted(t *testing.T) { Writers: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(3) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(2) r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { return len(s.Writers) == 1 && s.Writers[0].Username == "" && s.Writers[0].Email == "deleted@example.com" }), uint64(1)).Return(nil) @@ -1243,7 +1243,7 @@ func TestHandleUserDeleted(t *testing.T) { r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { return len(s.Auditors) == 1 && s.Auditors[0].Username == "" }), uint64(2)).Return(nil).Once() - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(secondRead, uint64(2), nil).Times(2) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(secondRead, uint64(2), nil).Once() r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) }, setupMsg: func(m *domain.MockMessageBuilder) { @@ -1285,7 +1285,7 @@ func TestHandleUserDeleted(t *testing.T) { OpportunityOwner: &models.UserInfo{Username: deletedUsername, Email: "oo@example.com"}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(3) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(2) r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { return len(s.MeetingCoordinators) == 1 && s.MeetingCoordinators[0].Username == "" && s.ExecutiveDirector != nil && s.ExecutiveDirector.Username == "" && @@ -1308,7 +1308,7 @@ func TestHandleUserDeleted(t *testing.T) { Writers: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(4) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(3) r.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)).Return(nil) r.On("GetProjectBase", mock.Anything, projectUID). Return((*models.ProjectBase)(nil), errors.New("transient read failure")).Once() @@ -1329,7 +1329,7 @@ func TestHandleUserDeleted(t *testing.T) { Writers: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(5) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(3) r.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)).Return(nil) r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil).Times(2) }, @@ -1354,7 +1354,7 @@ func TestHandleUserDeleted(t *testing.T) { Writers: []models.UserInfo{{Username: "other", Email: "other@example.com"}}, } r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{match, other}, nil) - r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(match, uint64(1), nil).Times(3) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(match, uint64(1), nil).Times(2) r.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)).Return(nil) r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) }, @@ -1406,6 +1406,105 @@ func TestHandleUserDeleted(t *testing.T) { } } +func TestShouldScrubSettingsUsername(t *testing.T) { + const deletedUsername = "deleted.user" + ctx := context.Background() + + tests := []struct { + name string + entry models.UserInfo + setupUser func(*domain.MockUserReader) + userReader bool + want bool + }{ + { + name: "no email — always scrub", + entry: models.UserInfo{Username: deletedUsername}, + want: true, + }, + { + name: "nil user reader with email — scrub", + entry: models.UserInfo{Username: deletedUsername, Email: "a@example.com"}, + want: true, + }, + { + name: "email maps to same username — do not scrub", + entry: models.UserInfo{Username: deletedUsername, Email: "active@example.com"}, + setupUser: func(u *domain.MockUserReader) { + u.On("UsernameByEmail", mock.Anything, "active@example.com").Return(deletedUsername, nil) + }, + userReader: true, + want: false, + }, + { + name: "email maps to different username — scrub", + entry: models.UserInfo{Username: deletedUsername, Email: "reassigned@example.com"}, + setupUser: func(u *domain.MockUserReader) { + u.On("UsernameByEmail", mock.Anything, "reassigned@example.com").Return("new.user", nil) + }, + userReader: true, + want: true, + }, + { + name: "email not found in auth — scrub", + entry: models.UserInfo{Username: deletedUsername, Email: "gone@example.com"}, + setupUser: func(u *domain.MockUserReader) { + u.On("UsernameByEmail", mock.Anything, "gone@example.com").Return("", domain.ErrUserNotFound) + }, + userReader: true, + want: true, + }, + { + name: "auth lookup error — skip entry", + entry: models.UserInfo{Username: deletedUsername, Email: "err@example.com"}, + setupUser: func(u *domain.MockUserReader) { + u.On("UsernameByEmail", mock.Anything, "err@example.com").Return("", errors.New("auth unavailable")) + }, + userReader: true, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc := &ProjectsService{} + if tt.userReader { + mockUser := &domain.MockUserReader{} + if tt.setupUser != nil { + tt.setupUser(mockUser) + } + svc.UserReader = mockUser + t.Cleanup(func() { mockUser.AssertExpectations(t) }) + } + got := svc.shouldScrubSettingsUsername(ctx, tt.entry, deletedUsername) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestScrubProjectSettingsUsernameRetryExhaustion(t *testing.T) { + const ( + deletedUsername = "deleted.user" + projectUID = "proj-1" + ) + mockRepo := &domain.MockProjectRepository{} + for range scrubMaxRetries { + mockRepo.On("GetProjectSettingsWithRevision", mock.Anything, projectUID). + Return(&models.ProjectSettings{ + UID: projectUID, + Writers: []models.UserInfo{{Username: deletedUsername, Email: "deleted@example.com"}}, + }, uint64(1), nil).Once() + } + mockRepo.On("UpdateProjectSettings", mock.Anything, mock.Anything, uint64(1)). + Return(domain.ErrRevisionMismatch) + + svc := &ProjectsService{ProjectRepository: mockRepo} + svc.scrubProjectSettingsUsername(context.Background(), projectUID, deletedUsername) + + mockRepo.AssertNumberOfCalls(t, "UpdateProjectSettings", scrubMaxRetries) + mockRepo.AssertExpectations(t) +} + func TestProjectSettingsHasUsername(t *testing.T) { const username = "alice" tests := []struct { From 888442e182a9d88f3ea9962a7e9d23b70f854cd2 Mon Sep 17 00:00:00 2001 From: Andres Tobon Date: Mon, 27 Jul 2026 16:35:29 -0700 Subject: [PATCH 10/13] fix(users): normalize username matching and honor event email Use case-insensitive username matching in settings prefilter/scrub paths and skip entries whose email differs from the deleted account email. Signed-off-by: Andres Tobon Co-authored-by: Cursor --- internal/service/project_subscriber.go | 58 +++++++++++++-------- internal/service/project_subscriber_test.go | 42 ++++++++++++--- pkg/events/project.go | 4 +- 3 files changed, 74 insertions(+), 30 deletions(-) diff --git a/internal/service/project_subscriber.go b/internal/service/project_subscriber.go index 32912162..3309f991 100644 --- a/internal/service/project_subscriber.go +++ b/internal/service/project_subscriber.go @@ -546,7 +546,7 @@ func (s *ProjectsService) HandleUserDeleted(ctx context.Context, msg domain.Mess slog.WarnContext(ctx, "project_subscriber: failed to unmarshal user.deleted event", constants.ErrKey, err) return nil } - if event.Username == "" { + if strings.TrimSpace(event.Username) == "" { slog.WarnContext(ctx, "project_subscriber: user.deleted event missing username — nothing to scrub") return nil } @@ -568,40 +568,50 @@ func (s *ProjectsService) HandleUserDeleted(ctx context.Context, msg domain.Mess continue } scrubCtx, scrubCancel := context.WithTimeout(ctx, settingsScanTimeout) - s.scrubProjectSettingsUsername(scrubCtx, candidate.UID, event.Username) + s.scrubProjectSettingsUsername(scrubCtx, candidate.UID, event.Username, event.Email) scrubCancel() } return nil } +// usernameMatches reports whether storedUsername represents the same LFID as deletedUsername. +func usernameMatches(deletedUsername, storedUsername string) bool { + deleted := strings.TrimSpace(deletedUsername) + stored := strings.TrimSpace(storedUsername) + if deleted == "" || stored == "" { + return false + } + return strings.EqualFold(deleted, stored) +} + // projectSettingsHasUsername reports whether any role-bearing field in settings carries username. func projectSettingsHasUsername(s *models.ProjectSettings, username string) bool { if s == nil { return false } for _, u := range s.Writers { - if u.Username == username { + if usernameMatches(username, u.Username) { return true } } for _, u := range s.Auditors { - if u.Username == username { + if usernameMatches(username, u.Username) { return true } } for _, u := range s.MeetingCoordinators { - if u.Username == username { + if usernameMatches(username, u.Username) { return true } } - if s.ExecutiveDirector != nil && s.ExecutiveDirector.Username == username { + if s.ExecutiveDirector != nil && usernameMatches(username, s.ExecutiveDirector.Username) { return true } - if s.ProgramManager != nil && s.ProgramManager.Username == username { + if s.ProgramManager != nil && usernameMatches(username, s.ProgramManager.Username) { return true } - if s.OpportunityOwner != nil && s.OpportunityOwner.Username == username { + if s.OpportunityOwner != nil && usernameMatches(username, s.OpportunityOwner.Username) { return true } return false @@ -609,13 +619,13 @@ func projectSettingsHasUsername(s *models.ProjectSettings, username string) bool // clearUsernameInSettings clears username on every matching entry in settings // that still represents the deleted account. Returns true when at least one field was changed. -func (s *ProjectsService) clearUsernameInSettings(ctx context.Context, settings *models.ProjectSettings, username string) bool { +func (s *ProjectsService) clearUsernameInSettings(ctx context.Context, settings *models.ProjectSettings, username, deletedEmail string) bool { changed := false clearIfMatch := func(u *models.UserInfo) { - if u == nil || u.Username != username { + if u == nil || !usernameMatches(username, u.Username) { return } - if !s.shouldScrubSettingsUsername(ctx, *u, username) { + if !s.shouldScrubSettingsUsername(ctx, *u, username, deletedEmail) { slog.DebugContext(ctx, "project_subscriber: skipping username scrub — email still maps to active LFID", "username", username, "email", u.Email) return @@ -641,15 +651,19 @@ func (s *ProjectsService) clearUsernameInSettings(ctx context.Context, settings // shouldScrubSettingsUsername reports whether a settings entry carrying deletedUsername should // be cleared. When the entry has an email, auth is consulted so a reassigned LFID reused by a -// new account (same username string, different lifecycle) is not scrubbed. +// new account (same username string, different lifecycle) is not scrubbed. When the deletion +// event carries an email, entries with a different non-empty email are treated as reuse and +// skipped even before auth lookup. // // Entries without email always scrub when the username matches. That is intentional: M2M and -// legacy email-less settings cannot be disambiguated via auth lookup. Downstream scrub is gated -// by the v1-sync-helper user.deleted publish ACL (project-api subscribes only from trusted -// service accounts); carrying email in the event would be a follow-up hardening step. -func (s *ProjectsService) shouldScrubSettingsUsername(ctx context.Context, u models.UserInfo, deletedUsername string) bool { - email := strings.ToLower(strings.TrimSpace(u.Email)) - if email == "" { +// legacy email-less settings cannot be disambiguated via auth lookup. +func (s *ProjectsService) shouldScrubSettingsUsername(ctx context.Context, u models.UserInfo, deletedUsername, deletedEmail string) bool { + entryEmail := strings.ToLower(strings.TrimSpace(u.Email)) + deletedEmailNorm := strings.ToLower(strings.TrimSpace(deletedEmail)) + if deletedEmailNorm != "" && entryEmail != "" && entryEmail != deletedEmailNorm { + return false + } + if entryEmail == "" { return true } if s.UserReader == nil { @@ -659,7 +673,7 @@ func (s *ProjectsService) shouldScrubSettingsUsername(ctx context.Context, u mod lookupCtx, cancel := context.WithTimeout(ctx, notificationTimeout) defer cancel() - resolved, err := s.UserReader.UsernameByEmail(lookupCtx, email) + resolved, err := s.UserReader.UsernameByEmail(lookupCtx, entryEmail) if err != nil { if errors.Is(err, domain.ErrUserNotFound) { return true @@ -668,12 +682,12 @@ func (s *ProjectsService) shouldScrubSettingsUsername(ctx context.Context, u mod constants.ErrKey, err) return false } - return resolved != deletedUsername + return !usernameMatches(resolved, deletedUsername) } // scrubProjectSettingsUsername fetches settings for a single project, clears the // username on any matching entry, persists, and reindexes. Retries on revision conflicts. -func (s *ProjectsService) scrubProjectSettingsUsername(ctx context.Context, projectUID, username string) { +func (s *ProjectsService) scrubProjectSettingsUsername(ctx context.Context, projectUID, username, deletedEmail string) { for attempt := 0; attempt < scrubMaxRetries; attempt++ { settings, revision, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID) if err != nil { @@ -685,7 +699,7 @@ func (s *ProjectsService) scrubProjectSettingsUsername(ctx context.Context, proj return } - if !s.clearUsernameInSettings(ctx, settings, username) { + if !s.clearUsernameInSettings(ctx, settings, username, deletedEmail) { return } diff --git a/internal/service/project_subscriber_test.go b/internal/service/project_subscriber_test.go index 062f8088..526bb4ff 100644 --- a/internal/service/project_subscriber_test.go +++ b/internal/service/project_subscriber_test.go @@ -1192,6 +1192,26 @@ func TestHandleUserDeleted(t *testing.T) { name: "empty username — discarded", payload: makeEvent(""), }, + { + name: "case-insensitive prefilter — stored Alice scrubbed for event alice", + payload: makeEvent("alice"), + setupRepo: func(r *domain.MockProjectRepository) { + settings := &models.ProjectSettings{ + UID: projectUID, + Writers: []models.UserInfo{{Username: "Alice", Email: "deleted@example.com"}}, + } + r.On("ListAllProjectsSettings", mock.Anything).Return([]*models.ProjectSettings{settings}, nil) + r.On("GetProjectSettingsWithRevision", mock.Anything, projectUID).Return(settings, uint64(1), nil).Times(2) + r.On("UpdateProjectSettings", mock.Anything, mock.MatchedBy(func(s *models.ProjectSettings) bool { + return len(s.Writers) == 1 && s.Writers[0].Username == "" + }), uint64(1)).Return(nil) + r.On("GetProjectBase", mock.Anything, projectUID).Return(&models.ProjectBase{UID: projectUID}, nil) + }, + setupMsg: func(m *domain.MockMessageBuilder) { + m.On("SendIndexerMessage", mock.Anything, constants.IndexProjectSettingsSubject, mock.Anything, false).Return(nil) + m.On("SendAccessMessage", mock.Anything, fgaconstants.GenericUpdateAccessSubject, mock.Anything, false).Return(nil) + }, + }, { name: "settings match — writer username cleared and reindexed", payload: makeEvent(deletedUsername), @@ -1411,11 +1431,12 @@ func TestShouldScrubSettingsUsername(t *testing.T) { ctx := context.Background() tests := []struct { - name string - entry models.UserInfo - setupUser func(*domain.MockUserReader) - userReader bool - want bool + name string + entry models.UserInfo + deletedEmail string + setupUser func(*domain.MockUserReader) + userReader bool + want bool }{ { name: "no email — always scrub", @@ -1463,6 +1484,12 @@ func TestShouldScrubSettingsUsername(t *testing.T) { userReader: true, want: false, }, + { + name: "event email differs from entry email — do not scrub", + entry: models.UserInfo{Username: deletedUsername, Email: "new@example.com"}, + deletedEmail: "old@example.com", + want: false, + }, } for _, tt := range tests { @@ -1476,7 +1503,7 @@ func TestShouldScrubSettingsUsername(t *testing.T) { svc.UserReader = mockUser t.Cleanup(func() { mockUser.AssertExpectations(t) }) } - got := svc.shouldScrubSettingsUsername(ctx, tt.entry, deletedUsername) + got := svc.shouldScrubSettingsUsername(ctx, tt.entry, deletedUsername, tt.deletedEmail) assert.Equal(t, tt.want, got) }) } @@ -1499,7 +1526,7 @@ func TestScrubProjectSettingsUsernameRetryExhaustion(t *testing.T) { Return(domain.ErrRevisionMismatch) svc := &ProjectsService{ProjectRepository: mockRepo} - svc.scrubProjectSettingsUsername(context.Background(), projectUID, deletedUsername) + svc.scrubProjectSettingsUsername(context.Background(), projectUID, deletedUsername, "") mockRepo.AssertNumberOfCalls(t, "UpdateProjectSettings", scrubMaxRetries) mockRepo.AssertExpectations(t) @@ -1514,6 +1541,7 @@ func TestProjectSettingsHasUsername(t *testing.T) { }{ {name: "nil settings", settings: nil, want: false}, {name: "writer match", settings: &models.ProjectSettings{Writers: []models.UserInfo{{Username: username}}}, want: true}, + {name: "writer case-insensitive match", settings: &models.ProjectSettings{Writers: []models.UserInfo{{Username: "Alice"}}}, want: true}, {name: "auditor match", settings: &models.ProjectSettings{Auditors: []models.UserInfo{{Username: username}}}, want: true}, {name: "meeting coordinator match", settings: &models.ProjectSettings{MeetingCoordinators: []models.UserInfo{{Username: username}}}, want: true}, {name: "executive director match", settings: &models.ProjectSettings{ExecutiveDirector: &models.UserInfo{Username: username}}, want: true}, diff --git a/pkg/events/project.go b/pkg/events/project.go index e9bda77c..28fc9437 100644 --- a/pkg/events/project.go +++ b/pkg/events/project.go @@ -93,7 +93,9 @@ type InviteAccepted struct { // V1UserDeletedEvent is the payload published by v1-sync-helper on // lfx.v1-sync-helper.user.deleted when a merged user record is soft-deleted. -// Username is the user's LFID. +// Username is the normalized LFID; Email is the deleted account's primary email when +// available so scrubbers can distinguish LFID reuse. type V1UserDeletedEvent struct { Username string `json:"username"` + Email string `json:"email,omitempty"` } From 9ed39e7d4c5b948401b7987e4db2ed958d92ea33 Mon Sep 17 00:00:00 2001 From: Andres Tobon Date: Mon, 27 Jul 2026 16:45:23 -0700 Subject: [PATCH 11/13] fix(users): skip username-only settings when event carries email Do not scrub settings entries that have a username but no email when the deletion event includes an email, since they cannot be verified. Signed-off-by: Andres Tobon Co-authored-by: Cursor --- internal/service/project_subscriber.go | 14 ++++++++++---- internal/service/project_subscriber_test.go | 6 ++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/internal/service/project_subscriber.go b/internal/service/project_subscriber.go index 3309f991..9dc308a6 100644 --- a/internal/service/project_subscriber.go +++ b/internal/service/project_subscriber.go @@ -655,13 +655,19 @@ func (s *ProjectsService) clearUsernameInSettings(ctx context.Context, settings // event carries an email, entries with a different non-empty email are treated as reuse and // skipped even before auth lookup. // -// Entries without email always scrub when the username matches. That is intentional: M2M and -// legacy email-less settings cannot be disambiguated via auth lookup. +// Entries without email scrub when the username matches only when the deletion event +// also omits email (M2M/legacy). When the event carries email, username-only entries are +// skipped because they cannot be verified as the deleted account. func (s *ProjectsService) shouldScrubSettingsUsername(ctx context.Context, u models.UserInfo, deletedUsername, deletedEmail string) bool { entryEmail := strings.ToLower(strings.TrimSpace(u.Email)) deletedEmailNorm := strings.ToLower(strings.TrimSpace(deletedEmail)) - if deletedEmailNorm != "" && entryEmail != "" && entryEmail != deletedEmailNorm { - return false + if deletedEmailNorm != "" { + if entryEmail == "" { + return false + } + if entryEmail != deletedEmailNorm { + return false + } } if entryEmail == "" { return true diff --git a/internal/service/project_subscriber_test.go b/internal/service/project_subscriber_test.go index 526bb4ff..9f0bff05 100644 --- a/internal/service/project_subscriber_test.go +++ b/internal/service/project_subscriber_test.go @@ -1490,6 +1490,12 @@ func TestShouldScrubSettingsUsername(t *testing.T) { deletedEmail: "old@example.com", want: false, }, + { + name: "event email with username-only entry — do not scrub", + entry: models.UserInfo{Username: deletedUsername}, + deletedEmail: "deleted@example.com", + want: false, + }, } for _, tt := range tests { From 80414892e96203c3d0ef251a0ba5ef20abd8ebdd Mon Sep 17 00:00:00 2001 From: Andres Tobon Date: Tue, 28 Jul 2026 08:49:08 -0700 Subject: [PATCH 12/13] fix(subscriber): scrub on definitive event-email match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the user.deleted event email matches a settings entry email, return true immediately instead of consulting auth — auth may still resolve the deleted LFID and incorrectly skip the scrub. Signed-off-by: Andres Tobon Co-authored-by: Cursor --- internal/service/project_subscriber.go | 9 +++--- internal/service/project_subscriber_test.go | 33 ++++++++++++++++----- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/internal/service/project_subscriber.go b/internal/service/project_subscriber.go index 9dc308a6..6c29aeb1 100644 --- a/internal/service/project_subscriber.go +++ b/internal/service/project_subscriber.go @@ -650,10 +650,10 @@ func (s *ProjectsService) clearUsernameInSettings(ctx context.Context, settings } // shouldScrubSettingsUsername reports whether a settings entry carrying deletedUsername should -// be cleared. When the entry has an email, auth is consulted so a reassigned LFID reused by a -// new account (same username string, different lifecycle) is not scrubbed. When the deletion -// event carries an email, entries with a different non-empty email are treated as reuse and -// skipped even before auth lookup. +// be cleared. When the deletion event carries an email, entries with a different non-empty email +// are treated as reuse and skipped; a matching entry email is a definitive identification and +// scrubs without auth lookup. When the event omits email, auth is consulted for entries that +// have an email so a reassigned LFID reused by a new account is not scrubbed. // // Entries without email scrub when the username matches only when the deletion event // also omits email (M2M/legacy). When the event carries email, username-only entries are @@ -668,6 +668,7 @@ func (s *ProjectsService) shouldScrubSettingsUsername(ctx context.Context, u mod if entryEmail != deletedEmailNorm { return false } + return true // definitive match — scrub without auth lookup } if entryEmail == "" { return true diff --git a/internal/service/project_subscriber_test.go b/internal/service/project_subscriber_test.go index 9f0bff05..4de7929a 100644 --- a/internal/service/project_subscriber_test.go +++ b/internal/service/project_subscriber_test.go @@ -1431,12 +1431,13 @@ func TestShouldScrubSettingsUsername(t *testing.T) { ctx := context.Background() tests := []struct { - name string - entry models.UserInfo - deletedEmail string - setupUser func(*domain.MockUserReader) - userReader bool - want bool + name string + entry models.UserInfo + deletedEmail string + setupUser func(*domain.MockUserReader) + userReader bool + skipAuthLookup bool + want bool }{ { name: "no email — always scrub", @@ -1496,6 +1497,17 @@ func TestShouldScrubSettingsUsername(t *testing.T) { deletedEmail: "deleted@example.com", want: false, }, + { + name: "event email matches entry email — scrub without auth lookup", + entry: models.UserInfo{Username: deletedUsername, Email: "deleted@example.com"}, + deletedEmail: "deleted@example.com", + setupUser: func(u *domain.MockUserReader) { + u.On("UsernameByEmail", mock.Anything, "deleted@example.com").Return(deletedUsername, nil) + }, + userReader: true, + skipAuthLookup: true, + want: true, + }, } for _, tt := range tests { @@ -1507,7 +1519,14 @@ func TestShouldScrubSettingsUsername(t *testing.T) { tt.setupUser(mockUser) } svc.UserReader = mockUser - t.Cleanup(func() { mockUser.AssertExpectations(t) }) + got := svc.shouldScrubSettingsUsername(ctx, tt.entry, deletedUsername, tt.deletedEmail) + assert.Equal(t, tt.want, got) + if tt.skipAuthLookup { + mockUser.AssertNumberOfCalls(t, "UsernameByEmail", 0) + } else { + mockUser.AssertExpectations(t) + } + return } got := svc.shouldScrubSettingsUsername(ctx, tt.entry, deletedUsername, tt.deletedEmail) assert.Equal(t, tt.want, got) From e03e45833f1029db3d5e6a4feda6ee9c6249511d Mon Sep 17 00:00:00 2001 From: Andres Tobon Date: Tue, 28 Jul 2026 10:54:54 -0700 Subject: [PATCH 13/13] fix(subscriber): drop raw LFID from INFO/WARN scrub logs Remove username field from application-level logs in the user-deleted scrub handler per logging-and-pii policy; keep project_uid as the correlate. Signed-off-by: Andres Tobon Co-authored-by: Cursor --- internal/service/project_subscriber.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/service/project_subscriber.go b/internal/service/project_subscriber.go index 47bda733..9f91e0cf 100644 --- a/internal/service/project_subscriber.go +++ b/internal/service/project_subscriber.go @@ -551,15 +551,14 @@ func (s *ProjectsService) HandleUserDeleted(ctx context.Context, msg domain.Mess return nil } - slog.InfoContext(ctx, "project_subscriber: scrubbing deleted user's username from project settings", - "username", event.Username) + slog.InfoContext(ctx, "project_subscriber: scrubbing deleted user's username from project settings") listCtx, listCancel := context.WithTimeout(ctx, settingsScanTimeout) allSettings, listErr := s.ProjectRepository.ListAllProjectsSettings(listCtx) listCancel() if listErr != nil { slog.WarnContext(ctx, "project_subscriber: failed to list project settings for username scrub", - constants.ErrKey, listErr, "username", event.Username) + constants.ErrKey, listErr) return nil } @@ -713,7 +712,7 @@ func (s *ProjectsService) scrubProjectSettingsUsername(ctx context.Context, proj updateErr := s.ProjectRepository.UpdateProjectSettings(ctx, settings, revision) if updateErr == nil { slog.InfoContext(ctx, "project_subscriber: cleared username from project settings", - "project_uid", projectUID, "username", username) + "project_uid", projectUID) s.publishProjectSettingsScrubSideEffects(ctx, projectUID) return }