Skip to content

refactor(api): unify namespace membership intake and carry a typed invite payload#6646

Open
otavio wants to merge 1 commit into
masterfrom
refactor/unify-membership-intake
Open

refactor(api): unify namespace membership intake and carry a typed invite payload#6646
otavio wants to merge 1 commit into
masterfrom
refactor/unify-membership-intake

Conversation

@otavio

@otavio otavio commented Jul 13, 2026

Copy link
Copy Markdown
Member

What

Collapses the two namespace-membership-intake code paths — "Add member" and "Generate invite link" — onto one shared, unexported intakeMembership helper, and replaces the hand-rolled positional worker payload with a single typed notification carried end to end.

Why

The invite flow was written twice and had drifted in user-visible ways: the link path skipped lowercasing the invited email (producing two placeholder accounts for Jane@x.com and jane@x.com), ran without a store transaction (half-created invitations on partial failure), and validated the tenant differently. Separately, the code firing the "member invited" hook held the fully-populated invitation but threw it away, forwarding only tenant:user:proto:host as a colon-delimited string — forcing the worker to re-read the database three times to rebuild what the caller already had, over a contract asserted only in twin prose comments across the shellhub↔cloud boundary.

Closes #6645

Changes

  • api/services: both AddNamespaceMember and GenerateInvitationLink become thin adapters over intakeMembership, which owns resolve-or-upsert, the duplicate-member check, the direct-membership short-circuit, and create-or-resend. The email is always lowercased and the whole write always runs inside store.WithTransaction, so the two paths behave identically.
  • api/services: GenerateInvitationLink's inlined signature/expiry copy is deleted in favor of the shared createMembershipInvitation/resendMembershipInvitation helpers (single source for the 7-day expiry and pairingcode signature).
  • pkg/models: new MembershipInvitationNotification — the typed, email-relevant snapshot (signature, expiry, recipient email + name, forwarded proto + host). Deliberately no role or namespace name; the template uses neither.
  • hook + internal client: OnMembershipInvited and InviteMember widen to carry the notification; the internal client JSON-encodes it over the worker's existing []byte transport (task pattern unchanged).
  • requests: FowardedHost renamed to ForwardedHost (bound X-Forwarded-Host header unchanged, wire-compatible); GenerateInvitationLink.TenantID now uuid-validated to match NamespaceAddMember.

Testing

Exercised through the two public methods with mockery: new cases pin email-lowercasing on both paths, WithTransaction usage, expired-invitation resend, the direct-membership short-circuit firing no hook, and a fully-populated post-commit notification. The cloud consumer side (worker + hook) moves in lockstep — see the paired PR on shellhub-io/cloud. Deploy both together: the cloud-api:invites payload changes from a positional string to JSON, and the shallow invite queue drains rather than carrying a dual-format reader.

@otavio otavio requested review from a team as code owners July 13, 2026 21:33
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 2m 12s —— View job


Code Review Complete

Reviewed 11 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

A few things I verified while reviewing, for the record:

  • Contract compatibility (shellhub ↔ cloud): the paired cloud changes are consistent — internal/cloud/hooks/member.go, the cloud-api:invites worker in internal/cloud/services/task.go, both mock clients, and the JSON round-trip test all move to the typed MembershipInvitationNotification. As the PR body notes, the payload shifts from a positional colon string to JSON, so the two repos must deploy together and the cloud-api:invites queue must drain across the switch.
  • *invitation.ExpiresAt deref (api/services/member.go:158) is safe — both createMembershipInvitation and resendMembershipInvitation always assign a non-nil ExpiresAt before the notification is built.
  • FowardedHostForwardedHost rename is complete in both repos; the X-Forwarded-Host struct tag is unchanged, so header binding stays wire-compatible.
  • Unified intakeMembership preserves the prior create / resend-if-expired / reject-duplicate branch logic on both entry points, and the new tests pin email-lowercasing, WithTransaction usage, expired-invitation resend, the direct-membership no-hook short-circuit, and the fully-populated post-commit notification.

To request another review round, comment /review.

Both "Add member" and "Generate invite link" now delegate to one unexported
intakeMembership helper, so the invite rules live in a single place: the
invited email is always lowercased before lookup/upsert and the whole write
runs inside store.WithTransaction on both paths. This removes the earlier
drift where the link path skipped lowercasing (producing duplicate placeholder
accounts) and ran without a transaction. GenerateInvitationLink's inlined
signature/expiry copy is deleted in favor of the shared create/resend helpers,
and its tenant is now uuid-validated to match AddNamespaceMember. The
misspelled FowardedHost request field is renamed to ForwardedHost; the bound
X-Forwarded-Host header is unchanged.

The "member invited" event now carries one typed
MembershipInvitationNotification (signature, expiry, recipient email + name,
forwarded proto + host) end to end: assembled once at intake, passed through
the widened OnMembershipInvited hook and the internal client (JSON-encoded over
the worker's []byte transport), replacing the hand-rolled positional
colon-delimited string. The core<->cloud handoff is now a single shared type
instead of twin prose comments.

Fixes: #6645
@otavio otavio force-pushed the refactor/unify-membership-intake branch from d938a39 to da0a63a Compare July 13, 2026 21:37
@otavio

otavio commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 3m 51s —— View job


Code Review Complete

Reviewed 11 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

A few things verified during this round, for the record:

  • *invitation.ExpiresAt deref (api/services/member.go:158) is safe — it's only reached when invitation came from createMembershipInvitation or resendMembershipInvitation, both of which always assign a non-nil ExpiresAt; the still-pending/not-expired branch returns an error and leaves invitation nil, so the notification block never runs for it.
  • Authorization preserved on both entry pointsAddNamespaceMember and GenerateInvitationLink each still perform their FindMember + Role.HasAuthority(req.MemberRole) gate before delegating; intakeMembership makes no privilege decisions and writes against the resolved namespace.TenantID, so tenant scoping is sound. Adding required,uuid to GenerateInvitationLink.TenantID is a tightening that matches NamespaceAddMember.
  • Email lowercasing is applied once at the top of intakeMembership and used for UserResolve, UserInvitationsUpsert, and RecipientEmail alike — closing the old link-path bug where the upsert received the raw-case address. Pinned by new tests on both paths.
  • Cross-repo contract (shellhub ↔ cloud) is complete: the cloud-api:invites worker (cloud/internal/cloud/services/task.go) now json.Unmarshals into the shared MembershipInvitationNotification — no lingering colon-string parser — and cloud consumes the same Go type via the replace directive, so JSON tags round-trip trivially (covered by TestMembershipInvitationNotification_RoundTrip). The deliberately-dropped role/namespace-name fields are confirmed unused by the worker and email template. As the PR body notes, deploy both repos together and drain the queue across the switch.
  • Tests compile against the new signatures (mock InviteMember and the hook closures both take *MembershipInvitationNotification) and assert real values — lowercased email, WithTransaction usage, expired-invitation resend, the direct-membership no-hook short-circuit, and the fully-populated post-commit notification.

To request another review round, comment /review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unify namespace membership intake and carry a typed invite payload across the worker seam

1 participant