refactor(events): typed payloads for PII-bearing emit sites (F-05 subset)#511
Conversation
Convert every SCIM, idp, totp, and identity-link event emit site from
map[string]any literals to the shared internal/eventtypes/payloads
structs, so the emit site and the projector decoder share one wire
shape (compile-time drift detection) and spec 19's pii:"true" tag
mechanism has typed structs to reflect over.
- New payload structs: SCIMGroupMapped/Unmapped/MappingUpdated,
TOTPVerified/Disabled/BackupCodeUsed, UserDeleted/Disabled/Enabled,
IdentityProviderDeleted/SCIMDisabled. Empty structs marshal to {},
byte-identical to the legacy map emits (pinned by test).
- IdentityLinkLoginUpdated gains user_id — the DEK-owner spec 19's
crypto-shred layer resolves; both emitters (SSO linker, SCIM sync)
populate it. Pre-#507 events decode with UserID == "".
- IdentityUnlinked emitters now populate user_id/provider_id instead
of {} (audit value; the projector keys off stream_id regardless).
- payloads.UserGroupUpdated.Description becomes *string omitempty,
aligned with the projector's update-vs-preserve contract; the SCIM
rename path relies on nil-preserve, the API handler sets the pointer.
Decoders for UserGroupUpdated + SCIMGroup* now decode via the shared
structs (projector-local duplicates removed).
- New archtest TestEventPayloadsAreTyped: exact-count per-file ratchet
over Data: map[string]any emit literals — new untyped sites fail the
build, and converting a site forces the allowance down in the same
commit. Remaining 14 allowlisted sites are the non-PII broader-F-05
backlog.
Wire compatibility: emitted keys are unchanged on every path (empty
structs -> {}, always-set pointers -> always-present keys); the only
additive changes are user_id fields and UserGroupCreated's is_dynamic/
dynamic_query defaults, all ignored-or-tolerated by the decoders.
Local CR finding on the typed-payload conversion: the profile-sync gate skipped the UserProfileUpdated emit whenever every computed name value was empty, so a SCIM PUT/sync that explicitly cleared the name could never propagate — contradicting the pointer semantics (present-but-empty = overwrite). Gate on the name object being asserted instead: omitted name preserves the profile, explicit empty name clears it. Regression test pins both directions.
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR converts event ChangesTyped Payload Conversion
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/scim/groups_helpers.go (1)
178-188: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheck the orphan unmap append before creating the replacement mapping.
Line 178 uses
h.appendEventin a function that can return errors. If this unmap append fails, Line 192 can still create the replacement mapping, leaving the stale mapping active alongside the new one.Suggested fix
- h.appendEvent(ctx, store.Event{ + if err := h.store.AppendEvent(ctx, store.Event{ StreamType: "scim_group_mapping", StreamID: m.ID, EventType: string(eventtypes.SCIMGroupUnmapped), Data: payloads.SCIMGroupUnmapped{ ProviderID: providerID, SCIMGroupID: m.SCIMGroupID, }, ActorType: "scim", ActorID: providerID, - }) + }); err != nil { + return m, fmt.Errorf("unmap old SCIM group mapping: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scim/groups_helpers.go` around lines 178 - 188, The orphan-unmap path in the SCIM group mapping flow can fail silently and still proceed to create a replacement mapping, leaving the old mapping active. In the function that calls h.appendEvent for scim_group_mapping, check and handle the error from the SCIMGroupUnmapped append before continuing. Make the replacement-mapping creation path run only after a successful unmap event so the stale mapping cannot coexist with the new one.internal/scim/users_mutate.go (1)
121-137: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn an error when the profile event append fails.
This branch now handles explicit SCIM name clears, but a failed
AppendEventis only logged and the handler continues with a 200. That can silently drop a source-of-truth profile update while email/status failures correctly abort.Proposed fix
if err := h.store.AppendEvent(ctx, store.Event{ StreamType: "user", StreamID: userID, EventType: string(eventtypes.UserProfileUpdated), @@ ActorType: "scim", ActorID: provider.ID, }); err != nil { - h.logger.Warn("failed to update user profile via SCIM", "error", err) + h.logger.Error("failed to update user profile via SCIM", "error", err) + writeError(w, http.StatusInternalServerError, "failed to update user profile") + return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scim/users_mutate.go` around lines 121 - 137, The SCIM profile update path in users_mutate.go logs AppendEvent failures but still continues, which can return success after a missed source-of-truth update. Update the user profile update handler around h.store.AppendEvent so that on error it returns an error to the caller instead of only calling h.logger.Warn, matching the failure handling used for the other SCIM mutations and ensuring the request does not complete with a 200 when the event write fails.
🧹 Nitpick comments (1)
internal/eventtypes/payloads/payloads_test.go (1)
774-781: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit-empty-string case for
UserGroupUpdated.Description.
SCIMGroupMappingUpdatedhas a dedicated test for the "explicit empty string stays on the wire" case, butUserGroupUpdatedonly tests the non-nil-value and nil-omit cases. Given the second commit in this PR fixed exactly this "explicit empty vs. omitted" class of bug for SCIM name fields, adding the analogousDescription: ptr("")roundtrip test here would guard the same regression class for group descriptions.✅ Suggested additional test
func TestUserGroupUpdated_NilDescriptionOmitsKey(t *testing.T) { raw, err := json.Marshal(payloads.UserGroupUpdated{Name: "ops"}) require.NoError(t, err) assert.JSONEq(t, `{"name":"ops"}`, string(raw)) } + +func TestUserGroupUpdated_ExplicitEmptyDescriptionStaysOnWire(t *testing.T) { + raw, err := json.Marshal(payloads.UserGroupUpdated{Name: "ops", Description: ptr("")}) + require.NoError(t, err) + assert.JSONEq(t, `{"name":"ops","description":""}`, string(raw)) +}Also applies to: 965-974
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/eventtypes/payloads/payloads_test.go` around lines 774 - 781, Add an explicit empty-string roundtrip test for UserGroupUpdated.Description in the payloads tests: alongside TestRoundtrip_UserGroupUpdated, verify that Description: ptr("") survives json.Marshal/json.Unmarshal unchanged, similar to the existing SCIMGroupMappingUpdated empty-string case. Keep the current non-nil and nil-omit coverage, and use the UserGroupUpdated type plus its roundtrip test to locate and extend the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/scim/groups_helpers.go`:
- Around line 178-188: The orphan-unmap path in the SCIM group mapping flow can
fail silently and still proceed to create a replacement mapping, leaving the old
mapping active. In the function that calls h.appendEvent for scim_group_mapping,
check and handle the error from the SCIMGroupUnmapped append before continuing.
Make the replacement-mapping creation path run only after a successful unmap
event so the stale mapping cannot coexist with the new one.
In `@internal/scim/users_mutate.go`:
- Around line 121-137: The SCIM profile update path in users_mutate.go logs
AppendEvent failures but still continues, which can return success after a
missed source-of-truth update. Update the user profile update handler around
h.store.AppendEvent so that on error it returns an error to the caller instead
of only calling h.logger.Warn, matching the failure handling used for the other
SCIM mutations and ensuring the request does not complete with a 200 when the
event write fails.
---
Nitpick comments:
In `@internal/eventtypes/payloads/payloads_test.go`:
- Around line 774-781: Add an explicit empty-string roundtrip test for
UserGroupUpdated.Description in the payloads tests: alongside
TestRoundtrip_UserGroupUpdated, verify that Description: ptr("") survives
json.Marshal/json.Unmarshal unchanged, similar to the existing
SCIMGroupMappingUpdated empty-string case. Keep the current non-nil and nil-omit
coverage, and use the UserGroupUpdated type plus its roundtrip test to locate
and extend the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cafaa358-3940-4f3d-8394-f91a55713e03
📒 Files selected for processing (26)
internal/api/auth_handler.gointernal/api/idp_handler.gointernal/api/totp_handler.gointernal/api/user_group_handler.gointernal/api/user_handler.gointernal/archtest/typed_payloads_test.gointernal/eventtypes/payloads/identity_link.gointernal/eventtypes/payloads/idp.gointernal/eventtypes/payloads/payloads_test.gointernal/eventtypes/payloads/scim_group.gointernal/eventtypes/payloads/totp.gointernal/eventtypes/payloads/user.gointernal/eventtypes/payloads/user_group.gointernal/idp/linker.gointernal/idp/linker_test.gointernal/projectors/scim_group_mapping.gointernal/projectors/user_group.gointernal/scim/groups.gointernal/scim/groups_create.gointernal/scim/groups_helpers.gointernal/scim/groups_mutate.gointernal/scim/users.gointernal/scim/users_create.gointernal/scim/users_helpers.gointernal/scim/users_link_test.gointernal/scim/users_mutate.go
1 participant
Closes #507. Prerequisite for spec 19 (#502) — the
pii:"true"tag mechanism reflects over typed structs, so the PII-bearing emit sites must be typed first. Sequenced ahead of #504 (spec 20), which depends on these typed structs for row-bound AAD derivation.What
internal/eventtypes/payloadsstruct asDatainstead of amap[string]anyliteral — emit and projector decode share one wire shape, so key drift fails at compile time.SCIMGroupMapped/Unmapped/MappingUpdated,TOTPVerified/Disabled/BackupCodeUsed,UserDeleted/Disabled/Enabled,IdentityProviderDeleted/SCIMDisabled. Empty structs marshal to{}, byte-identical to the legacy emits (pinned byTestEmptyPayloads_MarshalToEmptyObject).IdentityLinkLoginUpdatedgainsuser_id— the DEK owner spec 19's crypto-shred layer resolves. Both emitters (SSO linker, SCIM sync) populate it; pre-existing events decode withUserID == "". Pinned by a linker unit test and a real-Postgres SCIM test, both red-checked.IdentityUnlinkedemitters now carryuser_id/provider_idinstead of{}(audit value; projector keys off stream_id regardless).payloads.UserGroupUpdated.Description→*string, aligned with the projector's update-vs-preserve contract. The SCIM rename path relies on nil-preserve (previously it emitted onlynameas a map — a naive struct conversion would have wiped descriptions on every SCIM group rename); the API handler sets the pointer. TheUserGroupUpdated+SCIMGroup*decoders now decode via the shared structs.TestEventPayloadsAreTyped: exact-count per-file allowlist overData: map[string]anyemit literals. A new untyped site fails the build anywhere; converting a site forces the allowance down in the same commit. The remaining 14 sites across 12 files are the non-PII broader-F-05 backlog.Wire compatibility
Emitted keys are unchanged on every path: empty structs →
{}; always-set pointers → always-present keys (including explicit""). Additive-only changes:user_idfields, and SCIMUserGroupCreatednow emitsis_dynamic:false/dynamic_query:""like the API path already does (decoder-neutral).Not converted (deliberate)
IdentityProviderUpdatedinidp_handler.gobuilds a dynamic map with conditional keys (provider config, no per-user PII — spec 19 scopes IdP secrets out). It evades the syntactic ratchet (map variable) and belongs to the broader-F-05 follow-up, as do the 14 allowlisted non-PII sites.Verification
go vet,staticcheck,gofmtclean; fullgo test ./... -count=1: 33 packages ok, 0 failures.user_idemit tests red-checked by scoped edits.coderabbit review: 2 majors found → both fixed in the second commit; no other findings.Summary by CodeRabbit
New Features
Bug Fixes