[LFXV2-2645] feat(users): scrub username from committee data on user deletion

[andrest50]

Summary

• Extends handleMergedUserDelete() to publish a lfx.v1-sync-helper.user.deleted NATS event carrying the deleted user's username and primary email
• The committee service subscribes to this event and performs the username scrub from committee members and settings writers/auditors (see lfx-v2-committee-service#161)
• Removes the query-service HTTP client, QUERY_SERVICE_URL config, and all direct committee scrub logic from the sync-helper — that responsibility now lives entirely in the committee service
• Handler is best-effort: publish failures are logged and do not block the delete ACK

Ticket

LFXV2-2645

Changes

Modified files

• cmd/lfx-v1-sync-helper/handlers_users.go — extends handleMergedUserDelete to publish a user-deleted NATS event; adds publishUserDeletedEvent, getPrimaryEmailForUserFn, and publishUserDeletedEventFn injectable vars
• cmd/lfx-v1-sync-helper/handlers_users_test.go — TestHandleMergedUserDeleteScrub covering event publish, no-username, email-lookup-failure, and hard-KV-delete cases
• go.mod / go.sum — bumps golang.org/x/text → v0.40.0 and golang.org/x/crypto → v0.54.0 to resolve CVE GO-2026-5970 and GO-2026-5932

Deleted files

• cmd/lfx-v1-sync-helper/client_query.go — query-service HTTP client (no longer needed)

Notes

• The username scrub is best-effort: if the committee service is temporarily unavailable when the event is published, the event is lost (core NATS, not JetStream). A stale username in committee display data is recoverable manually and does not affect auth or data integrity. JetStream durability can be added in a follow-up if needed.
• Hard KV deletes (v1Data == nil) cannot be scrubbed — the payload is gone so the username is unrecoverable. This path logs a warning. Hard deletes are abnormal and should not occur in normal operation.

[Copilot]

Pull request overview

Adds committee username cleanup when deleting merged users.

Changes:

• Adds query-service configuration and client.
• Scrubs committee members and settings with reuse guards.
• Adds unit tests for cleanup behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
client_query.go	Adds query-service resource lookup.
config.go	Adds query-service URL configuration.
client_committees.go	Adds committee fetch/update helpers.
handlers_users.go	Implements username scrubbing.
handlers_users_test.go	Tests deletion and scrub flows.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

cmd/lfx-v1-sync-helper/handlers_users.go:153

• The scrub is skipped in the normal deletion order documented immediately above: handleAlternateEmailDelete removes each email SFID from this user's mapping (handlers_users.go:301-303), while getPrimaryEmailForUser requires that mapping and returns an error when it is absent or empty. If alternate-email rows are deleted before the merged-user row, no event is published and the committee username remains. Source the guard email from data that survives this ordering, or emit/capture the scrub event before the email mapping is removed.

	if username != "" {
		email, emailErr := getPrimaryEmailForUser(ctx, userSfid)
		if emailErr != nil {
			logger.With(errKey, emailErr, "key", key, "user_sfid", userSfid).
				WarnContext(ctx, "failed to look up primary email for deleted user; committee username scrub skipped")

cmd/lfx-v1-sync-helper/handlers_users.go:184

• A core NATS Publish is not a durable handoff: success only means the message was queued on this client. If the committee-service subscriber is unavailable, or this process exits before the socket flushes, the event is lost; the delete handler still returns false, so the source JetStream message is ACKed and never retried. Use a persisted/acknowledged delivery mechanism and propagate publication failure so the source deletion can be retried.

	if err := natsConn.Publish(v1SyncHelperUserDeletedSubject, payload); err != nil {
		logger.With(errKey, err, "key", key).
			ErrorContext(ctx, "failed to publish user-deleted event; committee username scrub skipped")
		return

[Copilot]

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

cmd/lfx-v1-sync-helper/handlers_users.go:153

• This lookup depends on the alternate-email mapping and live alternate-email records, but handleAlternateEmailDelete removes each SFID from that mapping before returning (lines 301-315), and getV1ObjectData treats soft-deleted rows as absent (lfx_v1_client.go:1229-1236). The preceding comment notes those rows are normally deleted before or alongside the user, so the normal ordering leaves no primary email, skips this event, and never scrubs committee data. Preserve the primary email independently until the merged-user delete is handled (or resolve it from another durable source) so the event can always be emitted.

	if username != "" {
		email, emailErr := getPrimaryEmailForUser(ctx, userSfid)
		if emailErr != nil {
			logger.With(errKey, emailErr, "key", key, "user_sfid", userSfid).
				WarnContext(ctx, "failed to look up primary email for deleted user; committee username scrub skipped")

[Copilot]

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

cmd/lfx-v1-sync-helper/handlers_users.go:165

• The event identifies the deleted account only by a reusable username. In linked committee-service PR #161, the consumer looks up that username and clears records whenever current.Username == event.Username; after an LFID is reassigned, that check cannot distinguish the new owner, so a delayed event can scrub the new user's records. Include a stable identity that committee records can verify (for example, the primary email promised in this PR description) and require the consumer to match it before clearing.

type userDeletedEvent struct {
	Username string `json:"username"`

cmd/lfx-v1-sync-helper/handlers_users.go:155

• The linked committee-service PR handles this event through a newly added username secondary index, but it only writes that index on member create/update and adds no backfill for existing members (the existing members-by-committee-index command writes a different index). Consequently, users deleted after rollout will still retain usernames on all pre-existing committee member records. Add and run a username-index backfill before enabling this producer, or make the consumer fall back to a full scan until the index is populated.

This issue also appears on line 164 of the same file.

	if username != "" {
		publishUserDeletedEventFn(ctx, key, username)

[dealako]

Hey @andrest50 👋 — thanks for iterating on this one. The pivot from the query-service HTTP coupling to a fire-and-forget NATS publish is a clean simplification, and the final {username}-only payload is nicely minimal. The change is small, well-scoped, the injectable publishUserDeletedEventFn matches the existing Fn-var pattern, and go build ./... + TestHandleMergedUserDeleteScrub pass locally. Most of the earlier Copilot comments were against removed code (the primary-email cache / query-service client) and are now stale.

I have no blocking findings — just a couple of contract questions worth confirming and two nits.

Structured issue count

• 🔴 Blocking: 0
• 🟡 Minor: 0
• ⚪ Nit: 2 — publishUserDeletedEvent wrapper untested; commit rationale for the x/crypto bump is inaccurate.
• ❔ Question: 2 — raw-vs-normalized username contract; best-effort durability + username-reuse.

❔ Questions

1. Normalized-vs-raw username contract (handlers_users.go:154) — the username-index deletion gates on the normalized value (usernameToKVKey(username) != "") but the publish gates on and sends the raw username__c. Publishing raw is likely correct (the index only normalizes because KV keys require it), but it's worth confirming the committee-service consumer (#161) matches against the raw v1 username__c and normalizes on its side. If it doesn't, decomposed-Unicode / case / whitespace variants could silently miss the scrub.
2. Best-effort durability + username reuse (handlers_users.go:179) — I agree with Copilot's suppressed low-confidence note here. Core NATS publish is lost if no consumer is connected (or on shutdown before flush), and the JetStream delete is still ACKed, so a dropped event means the username persists — which is the exact thing this privacy fix removes. You've documented this as an accepted tradeoff in the PR description, so I'm not blocking on it; can you confirm there's a backfill/reconcile path (or a follow-up ticket) so a dropped event doesn't leave PII stranded indefinitely? Relatedly, since the event carries only a reusable username, a delayed event after LFID reassignment could scrub a new owner — worth a note on whether that's in scope.

AI bot reconciliation

• Copilot (durability + username-reuse, suppressed low-confidence): I agree — folded into Question #2. Not blocking given the documented tradeoff.
• Copilot (missing username-index backfill in committee service): valid, but that's consumer-side (#161), out of scope for this repo.
• Earlier Copilot comments referencing getPrimaryEmailForUser / v1-user.primary-email.* cache / client_query.go: stale — that code was removed in the final refactor.

✅ Final decision: Approved with comments

Nothing blocking; please just weigh in on the two questions before merge.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

go.mod:59

• The PR description says this bump resolves GO-2026-5932, but this line correctly notes that the advisory has no fixed upstream version: all x/crypto/openpgp versions are affected, so v0.54.0 cannot remediate it. Since this repository does not directly import openpgp, update the PR/security claim to describe non-reachability (and verify it with govulncheck), or migrate any reachable transitive OpenPGP use rather than claiming the version bump fixes the advisory.

	golang.org/x/crypto v0.54.0 // indirect; go mod tidy — GO-2026-5932 has no upstream fix yet

[Copilot]

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

cmd/lfx-v1-sync-helper/handlers_users.go:170

• The event identifies the deleted account only by username, so the linked consumer cannot safely implement its claimed username-reuse guard. If this event is handled after that username has been assigned to another account, the new owner's records still match the same username and will be scrubbed. Include a stable identity such as the deleted user's SFID and/or primary email, and require the subscriber to verify that identity before clearing data.

type userDeletedEvent struct {
	Username string `json:"username"`
}

cmd/lfx-v1-sync-helper/handlers_users.go:69

• This comment now documents normalizeKVSegment, but says it also performs RawURLEncoding even though that happens in toKVKey. Split the documentation between the two functions so callers are not misled about whether the returned value is encoded.

// toKVKey normalizes a user-provided string and encodes it as a URL-safe base64
// key segment safe for NATS KV. Order: TrimSpace → ToLower → NFC → RawURLEncoding.
// NFC unifies decomposed/precomposed Unicode (e.g. n\u0303 ≡ ñ) without semantic
// transposition. RawURLEncoding (no padding) keeps keys opaque and short.
func normalizeKVSegment(s string) string {

go.mod:59

• The inline note is accurate, but it contradicts the PR summary's claim that this bump resolves GO-2026-5932. That advisory affects all x/crypto/openpgp versions and currently has no fixed release, so v0.54.0 cannot resolve it. Update the PR's security claim; if the affected package is actually reachable, remediation requires replacing openpgp, not a version bump.

	golang.org/x/crypto v0.54.0 // indirect; go mod tidy — GO-2026-5932 has no upstream fix yet

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

cmd/lfx-v1-sync-helper/handlers_users.go:163

• getPrimaryEmailForUser reads the alternate-email mapping, but handleAlternateEmailDelete normally removes those entries before the merged-user deletion. This therefore commonly returns an error; the error is discarded and an event without email is still published. The linked committee consumer only applies its LFID-reuse guard when email is non-empty, so a delayed event can clear a reassigned user's committee username. Persist the primary email before alternate-email cleanup (as the earlier cache implementation did) and publish only with that guard value; the added test should also exercise lookup failure instead of always returning success.

		email, _ := getPrimaryEmailForUserFn(ctx, userSfid)
		publishUserDeletedEventFn(ctx, key, normalizedUsername, email)

cmd/lfx-v1-sync-helper/handlers_users.go:61

• The PR description still claims go.mod/go.sum upgrade x/text to v0.40.0 and x/crypto to v0.54.0, but the current PR changes only these two handler files and the repository remains on v0.37.0/v0.52.0. Thus the advertised dependency/CVE remediation is not part of this change. Update the PR description to note that the bumps were reverted, or restore a compliant remediation in a separate dependency change.

	publishUserDeletedEventFn = publishUserDeletedEvent
	getPrimaryEmailForUserFn  = getPrimaryEmailForUser

[Copilot]

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

cmd/lfx-v1-sync-helper/handlers_users.go:163

• Do not publish a username-only deletion when this lookup fails. getPrimaryEmailForUser reads the alternate-email mapping and live rows, but handleAlternateEmailDelete normally removes that mapping before the merged-user delete arrives. The linked committee consumer only applies its LFID-reuse guard when email is non-empty, so this fallback can scrub a newly reassigned user's committee identity/access. Persist the primary email before alternate-email cleanup (as the prior fix described), use that durable value here, and cover lookup failure in the test.

		email, _ := getPrimaryEmailForUserFn(ctx, userSfid)
		publishUserDeletedEventFn(ctx, key, normalizedUsername, email)

go.mod:25

• The dependency changes do not match the PR's stated security update: this sets x/text to v0.39.0 rather than v0.40.0, while x/crypto remains at v0.52.0 instead of the described v0.54.0. Regenerate go.mod/go.sum with the intended versions, or correct the PR description and vulnerability claims if these versions are deliberate.

	golang.org/x/text v0.39.0

[dealako]

Hey @andrest50 👋 — thanks for the quick turnaround on my last pass. This is a follow-up review after the three new commits (e92d7d1, 688db4a, 02e5766) that landed and dismissed my prior "Approved with comments."

👏 Nice work

• Question #1 (raw-vs-normalized username contract) — resolved. e92d7d1 now normalizes the published username via normalizeKVSegment (TrimSpace → ToLower → NFC), so decomposed-Unicode / case / whitespace variants can't silently miss the downstream scrub. Good call routing the shared pipeline through one helper.
• Prior Nit (untested publish wrapper) — resolved. TestPublishUserDeletedEvent + the injectable natsPublishBytesFn seam now cover the publish path, including the error-swallowed case. Clean use of the existing Fn-var pattern.
• Question #2 (username reuse) — partially mitigated by carrying the primary email in the payload so downstream scrubbers can distinguish LFID reuse. Good instinct — see the caveat below on whether it actually populates.
• All CI green (MegaLinter, CodeQL, License, DCO), targeted tests pass locally.

Revision tracking

Prior item	Status
Q1 — raw vs normalized username contract	✅ Resolved (e92d7d1)
Nit — publish wrapper untested	✅ Resolved (TestPublishUserDeletedEvent)
Nit — inaccurate x/crypto bump rationale	✅ Moot — net change vs main is a clean forward bump (x/text 0.37.0 → 0.39.0; x/crypto/x/sys unchanged). No CVE regression.
Q2 — best-effort durability + username reuse	⚠️ Partially addressed — email added for reuse detection, but see 🔴 #1 (it's usually empty); durability tradeoff still documented-as-accepted

New findings this round

🔴 Blocking

1. Primary-email enrichment is usually empty in the common delete path — undermining the reuse-detection it was added for (handlers_users.go:161-163)
getPrimaryEmailForUserFn(ctx, userSfid) reconstructs the primary email by reading the v1-merged-user.alternate-emails.<sfid> mapping and the per-email rows. But your own TODO at lines 155-159 states those alternate-email rows are "deleted before or alongside the user row." So at user-delete time this lookup will frequently hit ErrKeyNotFound → email == "". Since 688db4a added email specifically so downstream can distinguish LFID reuse (the exact risk from my Q2), an empty email in the common case means a delayed event after LFID reassignment could still scrub a new owner. Please either capture the primary email earlier (e.g. from the user payload / a value written on update, which is what 0caa31c attempted before it was reverted), or drop the email field if it can't be reliably populated — shipping a field that's empty in the common path is worse than not having it. A design confirmation would close this out.

🟡 Minor

2. Doc comment detached from toKVKey, now misdescribes normalizeKVSegment (handlers_users.go:64-70)
e92d7d1 inserted normalizeKVSegment directly beneath the existing toKVKey godoc block, so that comment ("...encodes it as a URL-safe base64 key segment ... RawURLEncoding keeps keys opaque and short") is now the godoc for normalizeKVSegment, which does none of the base64 encoding. Meanwhile toKVKey is left undocumented. Give normalizeKVSegment its own one-liner (TrimSpace→ToLower→NFC) and move the base64/RawURLEncoding sentence back down onto toKVKey.

3. getPrimaryEmailForUser error silently discarded (handlers_users.go:162)
email, _ := getPrimaryEmailForUserFn(...) drops the error, so an empty email is indistinguishable from a lookup failure in production logs. Combined with #1, a one-line DebugContext on the error would make the (expected) empty-email case observable.

4. Test gap: empty/failed-email path through handleMergedUserDelete not covered (handlers_users_test.go)
TestHandleMergedUserDeleteScrub always stubs getPrimaryEmailForUserFn → "deleted@example.com", nil. Given #1 makes empty-email the expected production path, add a case where the lookup returns "", err and assert wantPublished: true, wantEmail: "". (TestPublishUserDeletedEvent exercises "" but bypasses the handler.)

5. Primary email logged at DEBUG, now reachable from the delete path (lfx_v1_client.go:295)
getPrimaryEmailForUser logs the full fallbackEmail. Pre-existing and DEBUG-gated, but the new delete flow widens its exposure — with DEBUG=true every deleted user's primary email now hits logs. Consider masking. (Good that publishUserDeletedEvent itself keeps email out of its INFO line.)

⚪ Nit

6. Redundant double-normalization (handlers_users.go:145 & 161) — usernameToKVKey(username) and normalizeKVSegment(username) both run the NFC/lower/trim pipeline on the same input. Compute norm := normalizeKVSegment(username) once and derive the index key from it. Not worth churn on its own.

AI reconciliation

Copilot has 0 active inline comments — its earlier notes were against removed code (primary-email cache / query-service client) and are stale. No new bot findings to reconcile.

Issue count (carried + new)

• 🔴 Blocking: 1 — email enrichment empty in common delete path
• 🟡 Minor: 4 — detached godoc; swallowed error; empty-email test gap; DEBUG email log
• ⚪ Nit: 1 — redundant double-normalization

🔴 Final decision: Needs changes before approval

No mechanical bugs — tests are green and the username scrub itself is sound. The blocker is a design/correctness question: the email field added this round is likely empty in the common case, so it doesn't yet deliver the reuse-detection it was meant to. Confirm the ordering assumption and either populate email reliably or drop it, then knock out the doc/test/logging minors. Happy to re-approve quickly once #1 is settled.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (5)

cmd/lfx-v1-sync-helper/handlers_users.go:325

• This cache is populated only when a primary-email row is processed after this deployment. The existing durable KV consumer will not replay already-ACKed rows, and --rebuild-user-secondary-indexes currently writes only v1-user.email.*, so existing users generally have no primary-email cache. With the documented deletion ordering, the live fallback is then gone and the event omits email; the linked committee consumer disables its reuse guard when email is empty and scrubs solely by username, which can clear a reassigned LFID. Backfill these keys and sequence that backfill before enabling the publisher, or make email-less events safe in the consumer.

	// Primary emails are not linked as Auth0 identities (they are the Auth0 user's own email).
	// Cache the address in mappings so handleMergedUserDelete can supply it to downstream
	// scrubbers even after the alternate-email rows have been cleaned up ahead of the
	// merged-user row.
	if isPrimary, _ := v1Data["primary_email__c"].(bool); isPrimary {

cmd/lfx-v1-sync-helper/handlers_users.go:331

• A transient failure here is logged and then the handler returns false, ACKing the only row event that would populate this cache. Because the primary row is normally removed before the merged-user deletion, the later fallback cannot recover the address, permanently dropping the email reuse guard. Request redelivery when this cache write fails, as the preceding mapping write already does.

			if _, err := mappingsKV.Put(ctx, cacheKey, []byte(emailAddr)); err != nil {
				logger.With(errKey, err, "key", key, "user_sfid", leadorcontactid).
					WarnContext(ctx, "failed to cache primary email for user deletion scrub")
			}

cmd/lfx-v1-sync-helper/handlers_users.go:176

• The producer applies NFC here, but the linked committee-service consumer builds its username index with only TrimSpace + ToLower and compares settings with EqualFold. For historical decomposed Unicode usernames (which this repository explicitly supports), the event contains the composed form, so the consumer hashes a different value and canonical-equivalent settings also fail to match; those usernames are never scrubbed. Define one normalization contract and apply NFC in both producer and consumer index/comparison paths.

	if normalizedUsername := normalizeKVSegment(username); normalizedUsername != "" {

go.mod:25

• The PR description claims x/text is bumped to v0.40.0 and x/crypto to v0.54.0 to resolve GO-2026-5970/GO-2026-5932, but this diff sets x/text v0.39.0 and leaves indirect x/crypto at v0.52.0. The official advisory says v0.39.0 does fix GO-2026-5970, while GO-2026-5932 has no fixed x/crypto/openpgp version, so the stated security remediation cannot match these changes. Reconcile the PR’s security claim and dependency plan.

	golang.org/x/text v0.39.0

cmd/lfx-v1-sync-helper/handlers_users.go:190

• The primary-email cache is deleted only inside the non-empty-username branch. A soft-deleted user with a missing/blank username (explicitly supported by the new tests), and every hard delete that returns earlier, leaves the newly persisted email in v1-mappings indefinitely even though userSfid is sufficient to address the cache key. Move cache cleanup so every deletion path attempts it, independently of whether an event can be published.

		// Best-effort cleanup of the cached primary-email key.
		if err := deleteIndexKeyFn(ctx, kvKeyPrimaryEmailByUserSfid+userSfid); err != nil {
			logger.With(errKey, err, "key", key, "user_sfid", userSfid).
				WarnContext(ctx, "failed to delete cached primary email after user-deleted event")
		}