Skip to content

refactor(api): extract acting-member and admit-member seams#6648

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

refactor(api): extract acting-member and admit-member seams#6648
otavio wants to merge 1 commit into
refactor/unify-membership-intakefrom
refactor/membership-intake-seams

Conversation

@otavio

@otavio otavio commented Jul 13, 2026

Copy link
Copy Markdown
Member

What

Introduces two deep, unexported seams in the api/services package and routes
the existing membership-intake code through them, with no change to the public
service interfaces or HTTP contracts. Stacked on top of #6646.

Closes #6647

Why

The membership-intake code had grown two shallow spreads:

  • The acting-member prologue — resolve the namespace, find the acting member,
    check their authority — was hand-copied across 8 service methods. A change to
    how the acting member is resolved (or which error a missing member yields) had
    to be made in 8 places and re-asserted in 8 tests.
  • "Admit a user into a namespace" (write the membership, consume the invitation,
    atomically) was hand-rolled in 3 intake paths, with the atomicity rule restated
    as a comment in two of them.

The goal is locality (one place to change each concept) and testability, aligned
with the in-flight refactor/unify-membership-intake work.

Changes

  • resolveActingMember: hides the resolve-and-authorize dance behind one
    call returning the namespace and acting member, or the correct typed error.
    Asserts authority only when a non-RoleInvalid role is passed — the same
    sentinel convention UpdateNamespaceMember already used. Migrated 8 call sites
    (add/update/remove/leave member, generate/list/cancel invitation, update API
    key), passing the role known at each site and keeping the delicate call-site
    guards (BFLA, owner, invitation-role) verbatim.
  • admitMember: owns the single add-and-consume write (create membership,
    then delete the invitation when one is supplied). Opens no transaction of its
    own — the store's WithTransaction is not reentrant, so it joins the caller's
    ambient transaction via the context. Wired into all 3 intake paths
    (AcceptInvite, createInvitedUser, intakeMembership direct-add).
  • Dropped the redundant acting-user UserResolve the three member.go methods did:
    the actor's ID comes from the authenticated token, so a resolved membership
    already proves the account exists.

Behavior change

A non-member acting user now yields a uniform NamespaceMemberNotFound across all
membership endpoints, where the three member.go methods previously returned
UserNotFound and LeaveNamespace returned AuthForbidden. This is stricter and
more consistent, and is guarded upstream by RequiresPermission.

Testing

go test ./services/... and golangci-lint run ./services/... pass clean. Tests
exercise both seams through the public service methods only. Added two
UpdateAPIKey cases covering the empty-role path (an observer renaming a key; a
non-member being rejected) to lock in the now-unconditional membership resolution.

Route the repeated resolve-and-authorize prologue through a single
resolveActingMember seam (8 call sites) and the add-and-consume
membership write through admitMember (3 intake paths), so each
contract lives in one place. Behavior-preserving except that a
non-member acting user now yields a uniform NamespaceMemberNotFound,
guarded upstream by RequiresPermission.

Fixes: #6647
@otavio otavio requested a review from a team as a code owner July 13, 2026 23:26
@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 1m 52s —— View job


Code Review Complete

Reviewed 6 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 rather than assumed:

  • RoleInvalid sentinel equivalenceRoleInvalid Role = "" (pkg/api/authorizer/role.go:11), so UpdateAPIKey's previous req.Role != "" guard maps exactly onto the seam's requireAuthorityOver != RoleInvalid check. The empty-role path stays semantically identical, and the two new test cases lock it in.
  • admitMember transaction safety — both ambient-transaction callers (AcceptInvite, and createInvitedUser at register.go:153) wrap the call in WithTransaction, and the intakeMembership direct-add passes nil (single write). admitMember correctly opens no transaction of its own, honoring the non-reentrant store.
  • Removed test mocks — only the actor's UserIDResolver expectations were dropped (matching the removed redundant UserResolve); the passive invitee's UserEmailResolver mocks correctly remain.
  • Documented behavior changes — the uniform NamespaceMemberNotFound for non-member actors and the now-unconditional membership resolution in UpdateAPIKey are intentional, guarded upstream by RequiresPermission, and covered by updated/added tests.

Locality and testability both improve here, and the call-site guards (BFLA, owner, invitation-role) were preserved verbatim.
· refactor/membership-intake-seams

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.

1 participant