Skip to content

Scope reactor lookups to the calling tenant - #42

Merged
bruth merged 2 commits into
mainfrom
feat/tenant-scoped-reactor-lookup
Jul 31, 2026
Merged

Scope reactor lookups to the calling tenant#42
bruth merged 2 commits into
mainfrom
feat/tenant-scoped-reactor-lookup

Conversation

@bruth

@bruth bruth commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

On a tenant store, GetReactor and ListReactors ignored the handle's tenant
and returned reactors across all tenants — inconsistent with the subject-level
isolation enforced everywhere else on a tenant store. This scopes both lookups
to the calling handle's tenant.

  • A tenant-scoped handle (es.Tenant("acme")) now lists only its own reactors,
    and GetReactor returns ErrReactorNotFound for another tenant's durable.
  • An unscoped handle keeps the full cross-tenant view.
  • Membership is determined by the tenant subject prefix already present in each
    durable's FilterSubjects (exactly how Create/Update build them). A reactor
    with no filters, or filters spanning more than one tenant (e.g. a consumer
    created outside Rita), is claimed by no tenant and appears only in the
    unscoped listing.
  • Durable names remain a single stream-wide namespace, so tenants must still
    name reactors distinctly. Mutations and the untenanted path are unchanged.

Addresses @bruth's review comment on #41:
#41 (comment)

Tests

Adds TestTenantReactorLookupScoped: two tenants each create a reactor, then
asserts each scoped handle lists only its own, the unscoped handle lists both,
and a cross-tenant GetReactor returns ErrReactorNotFound.

Follow-up

PR #41's docs/tenancy.md and the reactor.go godoc currently describe the
prior "lookups are not tenant-filtered" behavior; #41's docs will be updated to
match once this merges.

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 30, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context
  • Read changed files
  • Analyze for correctness, security, performance, NATS/event-sourcing issues
  • Post inline comments on specific issues
  • Post overall review summary

Summary

The core change is sound. reactorInTenantScope is a clean helper that uses the same subjectPrefix("") anchor that filtersToSubjects uses when building filter subjects, so the ownership check is consistent with how reactors are created. The unscoped-sees-all / scoped-sees-mine split is well-modeled and the test coverage is thorough.

One issue worth deciding before merge:

Cross-tenant mutation gap (medium)DeleteReactor and UpdateReactor call requireTenant() (presence check only) and then call JetStream unconditionally. A scoped handle for "acme" that somehow learns "beta-shipper" by name can delete or overwrite it, even though it cannot discover it through ListReactors/GetReactor. The new read-isolation contract creates an implicit expectation of write-isolation; the asymmetry is surprising and could be exploited if tenant names are guessable (e.g. "<tenant>-<common-verb>").

Fix sketch — add an ownership check before the destructive call (same pattern for UpdateReactor):

// In DeleteReactor, after requireTenant():
if _, err := s.GetReactor(ctx, name); err != nil {
    return err // ErrReactorNotFound if out-of-scope or truly absent
}

This reuses the scope logic already introduced in this PR and avoids duplicating the prefix logic.

Minor (noted inline): GetReactor always makes a full JetStream round-trip before the scope check, so a timing-sensitive adversarial tenant can distinguish "durable exists but belongs to another tenant" from "durable does not exist at all" — both surface as ErrReactorNotFound. Negligible in most deployments.

Everything else looks good:

  • reactorInTenantScope correctly handles no-filters (returns false for tenant scope) and cross-tenant filters (any non-matching prefix returns false)
  • ListReactors iteration is correct — scope filter applied before reactorInfoFromJS allocation, and lister.Err() checked after draining the channel
  • CachedInfo() is safe to call here since newReactorHandle only succeeds after a live consumer fetch
  • The new test covers all four cases: own-tenant list, other-tenant list, unscoped list, and cross-tenant GetReactorErrReactorNotFound

Comment thread reactor.go
if err != nil {
return nil, err
}
if !s.reactorInTenantScope(handle.cons.CachedInfo().Config.FilterSubjects) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetReactor always pays a JetStream round-trip before the scope check, so a cross-tenant request for a real consumer (one owned by another tenant) takes a longer code path than a request for a non-existent consumer. Both return ErrReactorNotFound to the caller, but an adversarial tenant that can measure latency gets a weak oracle for "does a durable by this name exist for some other tenant?"

For most Rita deployments this is negligible, but worth noting if tenants are considered mutually adversarial.

bruth added 2 commits July 31, 2026 07:40
A tenant-scoped handle saw every tenant's reactors through GetReactor and
ListReactors, inconsistent with the subject-level isolation enforced everywhere
else on a tenant store. Filter both by the tenant subject prefix carried in each
durable's FilterSubjects: a scoped handle now sees only its own reactors, while
an unscoped handle keeps the full cross-tenant view. Durable names remain a
single stream-wide namespace, so tenants must still name reactors distinctly;
mutations and the untenanted path are unchanged.

Signed-off-by: Byron Ruth <byron@synadia.com>
Signed-off-by: Byron Ruth <byron@synadia.com>
@bruth
bruth force-pushed the feat/tenant-scoped-reactor-lookup branch from e6b3faa to be5fd4c Compare July 31, 2026 11:42
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jul 31, 2026

Copy link
Copy Markdown

Claude finished @bruth's task in 2m 4s —— View job


Review of updated PR

  • Gather context
  • Read changed files (reactor.go, tenant_test.go)
  • Check whether prior review feedback (cross-tenant mutation gap) was addressed
  • Post review summary

Summary

This update closes the gap flagged in the previous review. requireReactorScope (reactor.go:361-373) now guards the mutating paths:

  • DeleteReactor (reactor.go:258) and UpdateReactor (reactor.go:199) both call it before touching JetStream, so a scoped handle can no longer delete/overwrite a foreign tenant's durable by guessing its name — it now surfaces as ErrReactorNotFound, consistent with GetReactor's visibility.
  • CreateOrUpdateReactor (reactor.go:220-234) applies the equivalent ownership check inline rather than reusing requireReactorScope, because it needs to surface ErrReactorExists (matching CreateReactor's conflict signal) instead of ErrReactorNotFound. That's a reasonable reason to not share the helper directly, though it does duplicate the "fetch consumer + reactorInTenantScope" pattern — worth a shared helper if a third caller ever needs the same check, not blocking here.
  • New coverage in TestTenantReactorMutationScoped (tenant_test.go:319-363) exercises exactly the scenario from the original comment: a foreign Delete/Update/CreateOrUpdate is refused, the foreign durable is left untouched, and the legitimate owner retains full mutation rights.

No new correctness or security issues found. Two pre-existing, non-blocking notes carried over from the first pass:

  • The timing side-channel on GetReactor (round-trips to JetStream before the scope check, so an existing-but-foreign durable and a truly-absent one are distinguishable by latency) is still present and wasn't in scope for this fix — negligible outside a mutually-adversarial-tenant threat model.
  • The scope-then-mutate pattern (requireReactorScope followed by a separate UpdateConsumer/DeleteConsumer call) adds a check-then-act window and an extra JetStream round trip per call. This is an acceptable, deliberate trade-off for correctness given JetStream doesn't offer a compare-and-swap primitive here.

The isolation model (unscoped-sees-all, scoped-sees-mine, ownership derived from the same subject-prefix logic used at creation) is internally consistent across list/get/update/delete/upsert, and the test suite now covers both the read and write side of that contract.

@bruth
bruth merged commit 41aa2bc into main Jul 31, 2026
10 checks passed
@bruth
bruth deleted the feat/tenant-scoped-reactor-lookup branch July 31, 2026 11:55
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