feat(models): add fail-closed TeamScopedManager for tenant isolation#52408
Merged
feat(models): add fail-closed TeamScopedManager for tenant isolation#52408
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Base automatically changed from
fix/visual-review-team-id-denormalize
to
master
March 27, 2026 14:15
Add ContextVar-based team scoping infrastructure that will enable automatic team filtering on Django models. This is the foundation for preventing IDOR vulnerabilities across the codebase. Refs #47065
Add Django manager that auto-filters queries by the current team_id from context. The manager applies the same parent team logic as the existing RootTeamQuerySet for consistency. Refs #47065
Add manager that supports both automatic context-based scoping and explicit filter(team_id=X) calls. This enables gradual migration from RootTeamManager without breaking existing code. Refs #47065
Add Django middleware that sets the team context from the authenticated user's current_team_id. This enables automatic team scoping for all database queries within a request. Refs #47065
Add TeamScopingMiddleware to MIDDLEWARE after AuthenticationMiddleware. This sets the team context from the authenticated user for all requests. Refs #47065
Add decorator that wraps function execution in a team_scope() context. Extracts team_id from function parameters, supporting both positional and keyword arguments. Refs #47065
Add semgrep rule that flags Celery tasks accessing model managers without explicit team scoping (@with_team_scope or .unscoped()). This helps ensure background tasks properly handle team context. Refs #47065
Add README with migration guide, usage examples, and API reference for the team scoping infrastructure. Refs #47065
- Add type validation in @with_team_scope decorator (raises TypeError for non-int) - Cache inspect.signature() at decoration time instead of per-call - Add TeamContext dataclass with parent_team_id caching to avoid N+1 queries - Extract _apply_team_filter into TeamFilterMixin to reduce code duplication - Middleware now caches parent_team_id from user.current_team - Add comprehensive integration tests with real Django ORM
Subclasses Phil Haack's TeamScopedManager to provide team scoping for product models on separate databases (no FK to Team). - ProductTeamQuerySet overrides _apply_team_filter to use plain team_id filtering instead of JOINing to Team across databases - ProductTeamManager inherits the same ContextVar flow and middleware - ProductTeamModel abstract base provides team_id field + both managers - Shares infrastructure with TeamScopedManager so they can't drift This is the multi-DB counterpart of RootTeamMixin — same security guarantees, different filter strategy.
All four visual_review models (Repo, Artifact, Run, RunSnapshot) now inherit from ProductTeamModel instead of plain models.Model. This gives them auto-scoped queries via the ContextVar middleware — queries are automatically filtered by the current team from request context. Also trims the scoping module README to essentials. 170 tests pass (52 scoping + 6 product mixin + 112 visual_review).
Remove BackwardsCompatibleTeamScopedManager — rollout happens model by model, not through an intermediary layer. TeamScopedManager now raises TeamScopeError when no team context is set instead of silently returning all rows. Escape hatches: .unscoped() for intentional cross-team access, .for_team(id) for explicit scoping outside request context. Swap declaration order on ProductTeamModel so objects (scoped) is _default_manager. Update visual_review: Celery task takes team_id, management command wraps in team_scope(), tests set context via autouse fixture.
Tighten docs: AGENTS.md now explains why team_id is required on every tenant model (not just "always filter by team_id"). products/README.md adds a "Team scoping (required)" section pointing to ProductTeamModel. CI: check-idor-model-coverage.py now detects ProductTeamModel models (BigIntegerField team_id, not just FK) and splits unscoped models into three tiers: LEGITIMATELY_UNSCOPED (silent pass), NEEDS_TEAM_ID (warning annotation in GH Actions — 39 baseline violations tracked as debt), and unacknowledged (error, blocks CI). New models without team_id fail CI unless explicitly categorized.
- Add missing Django/third-party models to LEGITIMATELY_UNSCOPED (axes, social_auth, contenttypes, sessions, otp) - Fix autouse _set_team_scope fixture: only activate for tests with django_db marker to avoid pulling DB access into pure unit tests (signing tests) and double-team creation in APIBaseTest classes (presentation tests) - Generate migration 0005 for manager declaration order change - Fix stale UNSCOPED_MODELS reference in error message
webjunkie
force-pushed
the
feat/team-scoped-manager
branch
from
fa69970 to
594a022
Compare
_get_parent_team_id accessed user.current_team (FK traversal), adding one DB query to every request and breaking all N+1 assertion tests. Now only reads current_team_id (integer column, no query). parent_team_id resolves lazily in the manager only when a PERSONS_DB_MODELS query actually happens.
Remove TeamFilterMixin (inline _apply_team_filter into QuerySet), add type annotations to test variables, add type: ignore for intentional wrong-type test cases.
AlterModelManagers with managers=[] is a no-op — Django doesn't need to track anything when objects (the default name) is _default_manager.
Contributor
Author
|
@Piccirello just if you have a few minutes before the weekend you could take a look here 👀 |
Fix _db attr-defined errors, remove unused type: ignore comments, add FlatPersonOverride (unused, has BigIntegerField team_id) to EXCLUDED_MODELS.
Piccirello
reviewed
Mar 31, 2026
Member
Piccirello
left a comment
Piccirello
left a comment
There was a problem hiding this comment.
I love everything about this. I'm not sure how we confidently roll this out without fear of breaking some existing calls. Perhaps this should roll out in some sort of audit mode at first? I'm not sure how that would work though.
Comment on lines
+255
to
+258
| # Baseline violations — these models SHOULD have team_id but don't yet. | ||
| # Emits a warning (not error) in CI. Shrink this list over time. | ||
| # When adding team_id to a model, remove it from here. | ||
| NEEDS_TEAM_ID: set[str] = { |
Member
There was a problem hiding this comment.
This is a useful list to have. Do we have a way of ensuring new models get a team_id?
Contributor
Author
There was a problem hiding this comment.
This should run in CI, so if a new model does not have team_id, it will error until you tell it here "it legitimately does not need one".
Comment on lines
+3
to
+5
| # Provides automatic team scoping for Django models using Python's ContextVar. | ||
| # When a request comes in, middleware sets the current team_id in a ContextVar. | ||
| # Models using TeamScopedManager will automatically filter by this team_id. |
Comment on lines
+33
to
+40
| # Set team context if user is authenticated and has a current team. | ||
| # Only reads current_team_id (the integer FK column) — no extra query. | ||
| # parent_team_id is resolved lazily by the manager only when a | ||
| # PERSONS_DB_MODELS query actually happens (rare). | ||
| if hasattr(request, "user") and request.user.is_authenticated: | ||
| team_id = getattr(request.user, "current_team_id", None) | ||
| if team_id is not None: | ||
| token = set_current_team_id(team_id) |
Member
There was a problem hiding this comment.
One challenge I know we've run into in the past is that the team specified in the API request can be different than the "current" team, which resulted in us checking permissions incorrectly. I fixed this in #50899 and added a semgrep rule. Do you think this check could present a similar challenge?
Contributor
Author
There was a problem hiding this comment.
Thanks for the pointer... I need to take a look at these mechanics to get full context 👀
| Repo.objects.all() # filtered to current team | ||
|
|
||
| # no context — raises TeamScopeError | ||
| Repo.objects.all() # ← boom |
Contributor
Author
|
@Piccirello Thanks for taking a look! Just fyi I'm off for Easter so this won't get attention until after that unfortunately.
So first, included in the PR, is adding it for visual review. Then we can do product by product. I actually think it shouldn't be risky at all? If a model has team_id and we add the manager, we "just" have to make sure all call sites can get hold of team_id (via the new context/middleware or explicitly). But maybe I'm being too naive 🤔 I'll think about it more in any case. |
…nager # Conflicts: # posthog/settings/web.py # products/visual_review/backend/tasks/tasks.py
Same class of bug as #50899: user.current_team_id can differ from the team in the URL, so middleware-set context would silently mismatch on nested API requests. - TeamAndOrgViewSetMixin now sets the scope in initial() from URL-derived self.team_id, overriding whatever middleware set - Middleware demoted to fallback for non-DRF code paths (admin, mgmt views) - Manager skips its parent_team_id subquery+join when context already has parent_team_id resolved (initial() passes it through from cached self.team)
Contributor
Author
|
This should be reviewed @Piccirello and possibly @haacked so it doesn't get stale again 😊 |
Piccirello
approved these changes
May 4, 2026
Contributor
|
I saw @Piccirello approved this, but I'm still reviewing and found some issues you may want to address. I'll try and get it submitted by end of day. |
haacked
reviewed
May 4, 2026
Contributor
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Nice work! See my suggestions and nits inline.
- semgrep celery rule: add mutating verbs (create/update/delete/bulk_*), values/aggregate, async variants (aget/aupdate/...), and raw(). The existing list only caught read-shaped methods, missing the highest- blast-radius operations like top-level Model.objects.update(...). - with_team_scope: bool slips past isinstance(_, int) because bool is an int subclass. Tighten the runtime check. - Extract resolve_effective_team_id as one helper used by both TeamScopedQuerySet and ProductTeamQuerySet — was duplicated, would drift as 35+ models adopt this. Raise TeamScopeError on unknown team to match the rest of the fail-closed posture (silent fallback let team_scope(typo) quietly scope queries to a non-existent team). - _apply_team_filter cold path: replace Q(Subquery) | Q(JOIN) with resolve once + plain integer filter. Same semantics, much simpler query plan. - for_team(parent_team_id=...): optional param so loop-style callers can skip the per-call Team lookup. - Routing.py: comment the __dict__["team"] read so the cached_property dependency doesn't get silently broken. - Tests: add behavioral coverage for the cached-parent / cold-path / root-team branches of _apply_team_filter against real rows.
Remove the request middleware that defaulted team scoping context to `request.user.current_team_id`. The original POC (#46874) and stage 1 plan (#47073) introduced it as a sensible "automatic" default for HTTP requests so callers wouldn't have to remember to set context. That justification no longer holds: 1. `current_team_id` is a UI preference field on User, mutated by every tab switch / "set current team" call. It is not security-grade — it represents "the team the user last focused on in the UI", not the team being acted on. The equivalent issue at the org level was the subject of #50899. 2. For DRF nested views (the primary path for everything in this PR), the `TeamAndOrgViewSetMixin.initial()` override added in this PR overwrites whatever the middleware set with the URL-derived `self.team_id`. The middleware's value is stale within microseconds for the only flow that actually queries scoped models today. 3. For non-DRF paths (admin, management views), the middleware's `current_team_id` default is wrong in practice anyway: admin pages want all teams (so they should `unscoped()`), management views are typically scoped to a team via URL/argument (so they should set context explicitly). There is no concrete consumer in this codebase that benefits from the silent fallback. 4. For Celery tasks and management commands, callers already opt in via `team_scope()` / `@with_team_scope()`. Middleware never fired. 5. We already had to revert priming of `parent_team_id` from the middleware in commit e2d517e because it added one DB query per request and broke N+1 assertion tests. The middleware that remained after that revert was a partial signal at best. Net effect after this commit: - DRF nested views: unchanged, mixin's initial() is the source of truth - Celery / mgmt commands: unchanged, opt in via team_scope() helpers - Non-DRF code paths that read scoped models: now raise TeamScopeError instead of silently filtering to user's UI preference. This is the correct fail-closed behavior — the developer either wraps the call in team_scope(team_id) (if there's a real team to act on) or unscoped() (if the path genuinely needs cross-team data). Removes: - posthog/models/scoping/middleware.py - posthog/models/scoping/test_middleware.py - TeamScopingMiddleware entry in posthog/settings/web.py MIDDLEWARE Updates: - README: replace the "automatic via middleware" framing with explicit enumeration of the two ways context gets set (DRF mixin, explicit team_scope() helpers). - routing.py: comment in initial() no longer references middleware as a fallback. If self.team_id resolution fails, context stays unset and any downstream scoped read raises — fail-closed. - manager.py: TeamScopeError docstring updates the "to fix" hint to point at the mixin and team_scope() helpers, no longer middleware.
…tion at read time Aligns ProductTeamModel with RootTeamMixin's write convention and simplifies the read path. The framework now has a single, stated contract: the team_id stored in scope context is the canonical id — the parent team's id when the team is a child environment, or the team's own id when it's a root team. Both reads and writes operate on that id; no resolution happens behind the scenes. What changed: 1. ProductTeamModel.save() rewrites self.team_id to canonical via resolve_effective_team_id before super().save(). Mirror of the long-standing RootTeamMixin.save() pattern for main-DB models. Writes from a child-team context land at parent_team_id, so reads filtering by canonical find them. 2. TeamScopedQuerySet._apply_team_filter is now one line: self.filter(team_id=team_id). No persons-DB carve-out, no fast/cold-path branches, no Q(Subquery) | Q(JOIN). The PERSONS_DB special case existed because we couldn't JOIN to Team across databases — without the JOIN, the special case is moot. 3. ProductTeamQuerySet inherits the same one-liner; its only override was the unscoped() return type. 4. TeamAndOrgViewSetMixin.initial() computes the canonical team_id from self.team (already loaded by permission checks) and sets that into context. The mixin is the resolver for DRF. 5. TeamContext dataclass dropped. The ContextVar now holds an int directly. parent_team_id and effective_team_id are gone — they only existed to support the read-time resolution that's now done upstream. 6. set_current_team_id / team_scope dropped the parent_team_id parameter. Callers (mixin, decorator, manual `with team_scope(...)`) pass the canonical id directly. resolve_effective_team_id remains as a public utility for callers who have a raw id and need to convert before calling team_scope. 7. for_team(team_id) drops the parent_team_id parameter for the same reason — pass canonical, get a filter. Why this is a net win: - Symmetric. Reads and writes agree on where data lives, no asymmetry for Codex/reviewers to flag (this was the substance of the Codex P1 on product_mixin.py:73). Adoption story is "always use canonical", full stop. - One DB lookup per write (in save()) instead of one per read. For any write-once-read-many access pattern this is a win; for write-once-read-zero it's a wash; we don't have read-zero patterns through the scoped manager today. - Manager is one line. No special cases. Future readers don't have to reason about fast paths, cold paths, or persons-DB carveouts. - The resolve_effective_team_id helper raises TeamScopeError on unknown teams (was a haacked suggestion, retained), so a typo'd team_id fails loudly rather than silently scoping queries to a non-existent team. Tests: - test_scoping.py: drop the TeamContext / parent_team_id tests (concept removed). Existing scope/decorator tests unchanged. - test_manager.py: replace the cached-vs-cold path tests (no longer meaningful) with a single test that the manager filters by whatever canonical id the caller put in context, plus a stray-row exclusion test (using FeatureFlag.objects.update() to bypass RootTeamMixin.save()'s rewrite to verify the filter excludes a child-team row). - test_product_mixin.py: drop parent_team_id args from team_scope calls in SimpleTestCase tests; manager filters by ctx.team_id directly so synthetic ids work without a DB lookup.
…ixin and ProductTeamModel Address two of the remaining review items from PR #52408: 1. TeamAndOrgViewSetMixin.dispatch() guards against ContextVar token leaks across requests. Background: ContextVars in sync Django are thread-local, and the same worker thread is reused across requests. The team-scope token set in initial() and reset in finalize_response() works in the normal flow, but a subclass that overrides finalize_response() without calling super() would never reset the token, leaking the scope into the next request handled on that thread. We could not block subclass overrides via __init_subclass__ without breaking legitimate DRF use cases (sharing.py, insight_variable.py both override initial() / finalize_response() with super() chains for token-auth and authentication tweaks). Instead: an outermost dispatch() override wraps super().dispatch() in a try/finally that resets the token unconditionally. Subclasses that customize the inner lifecycle methods can still misuse them without leaking. The set-side remains in initial() because that's where authentication has completed and self.team is loaded by the permission checks for free; if a subclass skips super().initial(), context just never gets set and downstream scoped queries raise TeamScopeError — fail-closed, not silent. 2. ProductTeamModel renames the second Manager from `unscoped` to `all_teams` to remove the autocomplete footgun. Background: ProductTeamModel declares two managers — `objects` (ProductTeamManager, fail-closed, auto-scopes) and a second plain Manager that admin / migrations / `_default_manager` can reach through to bypass scoping. Having both named `unscoped` (the queryset method `Model.objects.unscoped()` and the second Manager `Model.unscoped`) was a real ergonomics trap — a future contributor autocompleting `Repo.unscoped.filter(...)` thinking "I'm being explicit about scoping" would silently get every team's rows. The two surfaces have different intents: the queryset method is the normal cross-team escape hatch ("I want to query across teams in this one place"), while the bypass Manager is for code that runs outside the request lifecycle (admin, migrations) where setting context is awkward. The new name `all_teams` makes that intent explicit and removes the spelling collision. README guidance updated: admin classes should use `Model.all_teams` for cross-team access, with a note explaining why it isn't called `unscoped`.
Contributor
Author
|
Heads up on a meaningful design change in the latest commits: the TeamScopingMiddleware is gone, and along with it the parent_team_id cache in TeamContext. The thinking: The middleware was originally introduced (#46874, #47073) as a sensible "automatic" default for HTTP requests — set the team context from For DRF nested views (the primary path for everything in this PR), the For non-DRF paths (admin, management views, OAuth callbacks), Net effect of the removal: any code path that hits a scoped model without explicit context now raises This also unlocked the broader simplification in 89ba419 — the contract becomes "ContextVar holds the canonical team_id (parent if child env, root otherwise), full stop". Diff stats for the design change: -217/+20 for middleware removal, -260/+132 for the canonical refactor. The framework is smaller and the contract is tighter. |
Address the CI failures introduced by previous commits in this PR. Each fix in turn: 1. **Ruff import sort** in `products/visual_review/backend/tasks/tasks.py`: The block had `posthog.models.scoping` and a sibling-package relative import `..logic` in the same group, which ruff/oxfmt's isort configuration treats as separate sections. Auto-fixed (added the blank line between first-party and local). 2. **`SourceBatch` missing from semgrep team-scoped rules**: New model added on master while this branch was open. Has a direct `team_id = models.BigIntegerField()`, so it qualifies as team-scoped and needs to appear in `.semgrep/rules/idor-team-scoped-models.yaml`. Added to both regex blocks (sources and sanitizers). 3. **`SourceBatchStatus` is a new unscoped model**: FK chain to `SourceBatch` (which has team_id), so it's team-scoped *indirectly*. Added to `NEEDS_TEAM_ID` baseline debt in the IDOR coverage script — same treatment as the other ~30 chain-scoped models. 4. **`celery-task-team-scope-audit` semgrep rule producing 137 findings across the codebase**: The rule's earlier fix (working `pattern-either` for bare `@shared_task` + the `unscoped()` parse-error fix) made it actually match. It then started flagging every Celery task that touches `Model.objects.X` without `@with_team_scope`, regardless of whether the targeted model is on `TeamScopedManager` / `ProductTeamModel` or still on the older `RootTeamManager`/default Manager. That over-matches: most existing tasks query main-DB models that don't read the ContextVar at all (FeatureFlag etc. use RootTeamMixin which auto-rewrites team→parent on save, no scoping context needed). Adding `@with_team_scope` to all of them would be pointless churn. Scoped the rule to `products/visual_review/**` for now — the only product that has adopted the new framework. The `paths.include` list is the migration knob: as more products adopt `TeamScopedManager` / `ProductTeamModel`, add their paths here so the rule starts checking their tasks too.
haacked
reviewed
May 5, 2026
Contributor
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Most of the previous round landed cleanly. The refactor introduced two issues worth flagging: ProductTeamModel.save() does a Team lookup on every save despite a comment claiming otherwise, and the public scoping API silently returns zero rows on raw child team_ids.
|
|
||
|
|
||
| @contextmanager | ||
| def team_scope(team_id: int) -> Generator[None, None, None]: |
Contributor
There was a problem hiding this comment.
suggestion: team_scope, for_team, and with_team_scope trust the caller to pass canonical team_id. A caller who writes with team_scope(child_team_id): gets zero rows from any read. Data lives at the parent, the filter is on the child. No error, no log.
Have these three call resolve_effective_team_id themselves. One Team lookup at scope entry; the framework guarantees correctness regardless of caller hygiene. Matches the cost the old read-time resolution paid, amortized over the scope.
If the per-scope lookup is too much, add an explicit canonical: bool = False kwarg so misuse is an argument error instead of zero rows.
|
|
||
| def dispatch(self, request, *args, **kwargs): | ||
| """ | ||
| Outermost wrapper for the request lifecycle. We use this to *guarantee* |
Contributor
There was a problem hiding this comment.
nit: A subclass overriding dispatch() itself without super() skips the cleanup. dispatch is uncommonly overridden so this is fine in practice, but *guarantee* reads stronger than the code delivers.
Suggested wording: "ensure cleanup across the standard initial/finalize_response override surface."
| """Return an unscoped queryset that bypasses automatic team filtering.""" | ||
| return self._queryset_class(self.model, using=self._db) | ||
|
|
||
| def for_team(self, team_id: int) -> TeamScopedQuerySet[T]: |
Contributor
There was a problem hiding this comment.
suggestion: The docstring tells callers to pre-resolve, but for_team doesn't accept the resolved id. Loop callers still pay N+1 on Team:
for team_id in team_ids:
canonical = resolve_effective_team_id(team_id) # 1 Team lookup per iteration
Repo.objects.for_team(canonical).count()Optional kwarg lets bulk-resolvers skip the per-call lookup:
Suggested change
| def for_team(self, team_id: int) -> TeamScopedQuerySet[T]: | |
| def for_team(self, team_id: int, parent_team_id: int | None = None) -> TeamScopedQuerySet[T]: | |
| """Explicitly scope to a team. Useful outside request context. | |
| Pass `parent_team_id` if you've already resolved (e.g. bulk-resolved | |
| for a loop) to skip the per-call lookup. Otherwise call | |
| `resolve_effective_team_id` first. | |
| """ | |
| canonical = parent_team_id if parent_team_id is not None else team_id | |
| return self._queryset_class(self.model, using=self._db)._apply_team_filter(canonical) |
| # against models that haven't adopted the new manager (FeatureFlag, etc. | ||
| # still use RootTeamManager which auto-rewrites on save and doesn't | ||
| # care about ContextVar). | ||
| include: |
Contributor
There was a problem hiding this comment.
suggestion: This include list is now a load-bearing baseline. When the next product adopts ProductTeamModel, the rule silently doesn't run on its tasks until somebody adds the path here.
Track it in the README's "Which manager to use" table (or a footnote there) so a contributor migrating a new product sees both at once.
…conditional save() lookup Address the second-round haacked review (PR #52408 review 4229959857). Each fix in turn: 1. **`team_scope` / `with_team_scope` / `for_team` auto-resolve to canonical by default, with explicit `canonical: bool = False` opt-out.** The previous "trust the caller to pass canonical team_id" contract had a sharp edge: `with team_scope(child_team_id):` silently scoped reads to the child id while data lives at parent. No error, no log — just zero rows from any query. For a security framework that's exactly the class of bug it's meant to prevent. Now the helpers default to one Team lookup at scope entry, which guarantees the manager's filter finds rows regardless of whether the caller passed a parent or a child id. The cost shape is much better than read-time resolution: one lookup per scope instead of per query, amortized over however many reads happen inside. The DRF mixin's hot path stays untouched — it pre-resolves from `self.team` (cached by permission checks for free) and calls `set_current_team_id` directly, never going through `team_scope()`, so it pays nothing extra. Tests with synthetic team_ids pass `canonical=True` to skip resolution and avoid hitting the DB for ids that don't exist. 2. **`ProductTeamModel.save()` only resolves on insert / when team_id changes.** The previous version's docstring claimed "Only resolve on insert / when team_id changes" but the code unconditionally resolved on every save. Every Run state transition / Snapshot field update paid a Team roundtrip even when team_id wasn't moving. Now gated on `self._state.adding` (insert) or `update_fields` containing `team_id` (explicit team_id update); other saves skip the lookup. 3. **Re-add `parent_team_id` knob to `for_team` (via the `canonical` kwarg).** Bulk callers iterating over teams can pre-resolve once outside the loop and pass `canonical=True` per call to skip the per-iteration Team lookup. The in-loop equivalent of the mixin's pre-resolution. 4. **Soften `dispatch()` docstring wording.** Previous version claimed it "*guarantees*" cleanup — but a subclass overriding `dispatch()` itself without super() bypasses the cleanup, and we deliberately chose not to add `dispatch` to `__init_subclass__`'s protected list (`query_coalescer.py` legitimately overrides it, calling super()). New wording acknowledges the override surface and the existing in-codebase consumer.
…e_audit decorator Switch the rule from "default-off, hand-picked allowlist" (paths.include on visual_review only) to "default-on, per-task explicit opt-out". The previous shape was opt-in: a new product had to remember to add its path to `paths.include` for the rule to fire. A forgotten path = silent no-coverage. The rule's value depended on documentation discipline, which is exactly what the framework is meant to replace. The new shape is default-on. A new Celery task in any product that queries `Model.objects.X` either uses `@with_team_scope` (scoped), `.unscoped()` (intentional cross-team), or `@skip_team_scope_audit` (model still uses RootTeamManager / default Manager). Forgetting all three = CI fails. No path-list to maintain, no silent miss. The cost: existing tasks that legitimately don't need team scope (because they query models still on the older RootTeamManager which auto-rewrites team→parent on save and doesn't read the ContextVar) need a one-time tag. 32 files, ~84 task functions, all decorated mechanically using a generated finding list from semgrep itself. Why a marker decorator instead of `# nosemgrep` comments: - One marker per task instead of one comment per `.objects` call. Tasks with multiple queries don't need N comments. - Semantic content. `@skip_team_scope_audit` says "this task's models don't read the ContextVar, audit isn't applicable" — a future contributor reading the decorator knows what it asserts and when to remove it (when the underlying model migrates to TeamScopedManager). `# nosemgrep` is just "shut up rule" with no explanation. - Per-function granularity. A new task added to an existing exempt file fires the rule normally — the marker doesn't transitively cover unrelated tasks. Resists the "lazy dev adds new task to file with exemptions" failure mode. - No-op at runtime: the decorator returns the function unchanged. Pure marker, zero behavior cost. The rule's `paths.include` is gone. `paths.exclude` keeps test files out of the audit (test fixtures don't need decoration). The 32-file mechanical update was generated by running semgrep, walking each finding back to the enclosing task `def`, and inserting `@skip_team_scope_audit` between `@shared_task(...)` and the def. Imports added or merged into existing `from posthog.models.scoping` lines, then ruff sorted the import blocks. One closure-vs-task ambiguity in `posthog/tasks/tasks.py::refresh_activity_log_fields_cache` fixed by hand (the script had targeted a nested closure instead of the outer task). Verified: semgrep produces zero findings across the full scan paths (common/, ee/, posthog/, products/, services/llm-gateway/, services/stripe-mock/) after this commit.
… into TeamScopedManager After the canonical-team-id refactor in 89ba419, ProductTeamManager and ProductTeamQuerySet had nothing left distinguishing them from their parent classes — both filter via plain `team_id=team_id`, no JOIN. Their only purpose at that point was a typing nicety (return ProductTeamQuerySet from .unscoped() instead of TeamScopedQuerySet), which doesn't justify keeping the duplicated class hierarchy. Why they originally existed: - The pre-refactor `TeamScopedManager._apply_team_filter` did a Q(Subquery) | Q(JOIN) against `posthog_team` to resolve parent vs child. That worked for main-DB models with FK to Team. It couldn't work for separate-DB product models — Postgres can't JOIN across databases — so `ProductTeamQuerySet._apply_team_filter` was a separate plain `team_id=...` filter. - After the refactor, the manager doesn't resolve at read time at all. The shared filter is just `self.filter(team_id=team_id)`. Works identically for FK-style and BigInt-style team_id columns. The product/non-product distinction in the filter step evaporated. What this commit does: - Delete `ProductTeamManager` (was: `_queryset_class = ProductTeamQuerySet`). - Delete `ProductTeamQuerySet` (was: typing-only override of unscoped()'s return type). - `ProductTeamModel` declares `objects = TeamScopedManager()` directly. Same manager class everyone else uses. One name to learn. - Remove the `from posthog.models.scoping.product_mixin import ProductTeamManager, ProductTeamQuerySet` lines from tests. test_product_mixin.py becomes a smoke test asserting that ProductTeamModel + TeamScopedManager wires up correctly for a separate-DB model (visual_review's Repo). The exhaustive manager mechanics are already covered in test_manager.py. - Update routing.py docstring reference from "TeamScopedManager / ProductTeamManager" to single name. What stays: - `ProductTeamModel` itself — that's the column-shape concern (BigIntegerField team_id, no FK across DBs) and stays a separate abstract base. - `RootTeamMixin` (in posthog/models/utils.py) — the corresponding abstract base for main-DB models with FK team_id. Both mixins handle field declaration + save() rewrite; both can use the same `TeamScopedManager` for read enforcement. Net diff: ProductTeamManager (~3 lines class), ProductTeamQuerySet (~10 lines class), and ~6 import sites all collapse into nothing. The framework is genuinely manager-agnostic now: separate-DB vs main-DB is purely a column-shape decision, not a manager-class decision.
…-level)
Module-import-safety fix. Celery task modules import @skip_team_scope_audit
at module load (it's a decorator), and importing from `posthog.models.scoping`
at that point triggers `posthog.models.__init__` to load. That __init__
in turn pulls in heavy model modules (batch_exports, helpers,
dashboard_templates, insight, file_system_mixin) which declare model
classes at module level — and Django's app registry isn't ready yet
during early `posthog.apps.py` initialization for modules pulled in by
the Celery app config.
Concrete failure chain on the affected path:
posthog/apps.py:12
→ posthog/tasks/__init__.py
→ posthog/tasks/activity_log.py
→ posthog/cdp/internal_events.py
→ posthog/kafka_client/{routing,client}.py
→ posthog/clickhouse/client/__init__.py
→ posthog/clickhouse/client/execute_async.py
→ posthog/tasks/tasks.py
→ from posthog.models.scoping import skip_team_scope_audit ← here
→ posthog/models/__init__.py
→ posthog/batch_exports/models.py
→ posthog/helpers/__init__.py
→ posthog/helpers/dashboard_templates.py
→ posthog/models/insight.py
→ class FileSystemSyncMixin(Model) ← AppRegistryNotReady
Other model imports in tasks files don't hit this because they're
defined inside function bodies (deferred), and `with_team_scope` /
`team_scope` in the framework live in modules whose loading happens
later (test files, view dispatch). The decorator is unique in being
both top-level (decorators must be) and on the early-app-load path.
The decorator is purely a no-op marker, not actually tied to any Django
model. Moving it to a top-level `posthog.scoping_audit` module keeps the
import path safe — `posthog/__init__.py` is small and only imports the
celery app, no model loading triggered.
Update all 32 task files: replace
`from posthog.models.scoping import skip_team_scope_audit`
with
`from posthog.scoping_audit import skip_team_scope_audit`
The framework's other scoping helpers (TeamScopedManager,
ProductTeamModel, team_scope, etc.) stay in `posthog.models.scoping`
because they're inherently model-tied and only get imported once Django
apps are loaded — safe under the existing import contract.
Contributor
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
…e with abstract-base smoke tests
tach interface check failed on test_product_mixin.py:
Interfaces
[FAIL] posthog/models/scoping/test_product_mixin.py:19:
The path 'products.visual_review.backend.models.Repo'
is not part of the public interface for 'products.visual_review'.
[FAIL] posthog/models/scoping/test_product_mixin.py:27:
same.
visual_review is product-isolated. External code may only import from
`backend/facade/api.py`. The previous version of the test reached into
`backend.models` directly to grab `Repo` as a concrete ProductTeamModel
subclass for asserting manager behavior — which crossed the boundary.
The pre-isolation version of the test pre-dated stricter tach
enforcement and slipped through. After the recent
ProductTeamManager → TeamScopedManager consolidation it's also largely
redundant: every assertion the file made (manager raises without
context, filter applied, unscoped/for_team behavior) is already covered
in test_manager.py against FeatureFlag, and post-consolidation those
tests apply unchanged to ProductTeamModel-based models because they
share the same TeamScopedManager class.
What's left worth testing at this layer is just that the abstract base
is wired correctly: `objects` is `TeamScopedManager`, `all_teams` is
plain `Manager`, `team_id` is `BigIntegerField(db_index=True)`. Three
smoke tests, no concrete-model imports, no boundary crossings.
Concrete-model integration tests (save() rewriting team_id, end-to-end
ContextVar propagation through real CRUD) belong inside the consuming
product (e.g. `products/visual_review/backend/tests/`) where the model
import is internal. Those already exist for visual_review via the
autouse `team_scope` conftest fixture and the existing test_api.py
flow tests.
…t IDOR scope on ToleratedHash Two CI failures from the previous push: 1. **Visual review tests using `APIBaseTest` raised `TeamScopeError`** (16 failures across `test_baselines.py`, `test_thumbnails.py`, etc.). The autouse `_set_team_scope` fixture in conftest.py only fired when the test had `@pytest.mark.django_db` set explicitly. Test classes that inherit Django's `TestCase` (including `APIBaseTest`) get DB access automatically via pytest-django *without* needing the marker, so the fixture skipped them and `Repo.objects.X` raised `TeamScopeError` mid-test. Extend the trigger condition: fire when the marker is set OR when the enclosing class inherits `TestCase`. Pure unit tests (neither marker nor TestCase) still skip the fixture and don't pull in DB access. 2. **`idor-lookup-without-team` semgrep rule fired on two `ToleratedHash.objects.get_or_create` calls** in `diffing.py:189` and `logic.py:2259`. Both had `team_id` in the `defaults` dict rather than in the lookup args, so the rule's regex (which looks for `team_id=` directly in the call) didn't see scope coverage. `ToleratedHash` is on `ProductTeamModel` so the manager auto-filters by canonical team_id at read time — the lookups *were* scoped, just not in a way the rule could detect. Promoting `team_id` from `defaults` to a top-level kwarg satisfies the rule and adds defense-in-depth: the explicit filter agrees with the manager's auto-filter, and any future change that bypasses the manager (raw SQL, `_base_manager`) still keeps the scope. The duplicated team_id between the explicit kwarg and what would have been in `defaults` collapses naturally — `get_or_create` matches on the lookup args first, then writes them as part of the row if creating, so removing `team_id` from `defaults` is correct.
…icating team in autouse fixture
Two CI failures from previous push, both in the autouse-fixture path
for TestCase / APIBaseTest tests:
1. **31 `duplicate key value violates unique constraint
posthog_team_api_token_a9a1df8a_uniq` failures.** My previous fix
widened the autouse fixture to fire for any class inheriting
`TestCase` (not just `@pytest.mark.django_db` tests). Inside that
path I called `request.getfixturevalue("team")` which spins up
`PostHogTestCase.setUpTestData()` and creates a Team with
`api_token="token123"`. APIBaseTest.setUp() then ALSO creates a
Team with the same fixed token — duplicate-key collision on every
test where both fired.
Reverted the autouse fixture to only handle raw pytest tests with
the `django_db` marker, with an extra guard that skips even when
the marker is set if the test is a TestCase subclass. APIBaseTest
classes manage their own team (and now their own scope, see #2).
2. **TestCase / APIBaseTest classes still need scope set.** Added a
`VisualReviewTeamScopedTestMixin` in conftest.py that wraps
setUp/tearDown with `team_scope(self.team.id)` using the test's
own self.team (created by APIBaseTest.setUp). Place the mixin
before APIBaseTest in the MRO so its setUp runs after
APIBaseTest's, with self.team available:
class TestFoo(VisualReviewTeamScopedTestMixin, APIBaseTest):
def setUp(self):
super().setUp() # mixin enters scope, APIBaseTest sets up team
self.repo = Repo.objects.create(...) # auto-scoped
Applied the mixin to test_baselines.py (TestBaselinesOverview),
test_thumbnails.py (TestThumbnailEndpoint), and test_presentation.py
(TestRepoViewSet, TestRunViewSet — were inlining the same pattern
before; collapsed into the shared mixin).
3. **mypy error in test_product_mixin.py.** `field.db_index` access
on an `_meta.get_field()` return value triggers
`[attr-defined]` because django-stubs doesn't expose db_index on
the narrowed BigIntegerField type. Dropped the assertion — the
fact that `_meta.get_field("team_id")` returns a BigIntegerField
is the meaningful wiring check; `db_index=True` is a Django-tested
detail of the abstract base.
Local test run: 326 passed across `posthog/models/scoping/` and
`products/visual_review/backend/tests/`. Two remaining errors
(`django_content_type` / `auth_permission` constraint violations on
test_gating, test_logic) are unrelated transient DB-state issues —
content_type for `llm_analytics.clusteringconfig` and permission
for `add_link`, neither of which this PR touches.
…om prior tests
The single remaining test failure from the previous CI run was:
FAILED posthog/api/test/test_routing.py::test_team_scope_context_set_from_url_team_not_user_current_team
AssertionError: 175 is not None
Test passed locally in isolation; only failed when run alongside other
tests in the same worker. Root cause: ContextVar is per-thread/process,
not per-test, and pytest workers reuse threads across tests. If an
earlier test on the same thread left scope set (e.g. via a partial-
init failure in a setUp/tearDown pair), `assertIsNone(get_current_team_id())`
sees that leftover value, not the None we expected.
Two fixes:
1. **Loosen the assertion in test_routing.py.** The dispatch wrapper
resets via `ContextVar.reset(token)`, which restores whatever was
in scope before `set()` was called — not unconditionally None.
Capture `pre_request_scope` before the request and assert the
wrapper restored that, which is the actual invariant.
2. **Harden VisualReviewTeamScopedTestMixin against partial-init.**
Previous version assigned `self._team_scope_cm` after
`__enter__()` succeeded. If `team_scope()` raised during construction
(e.g. `resolve_effective_team_id` couldn't find the team — possible
under cross-DB transaction visibility issues) or `__enter__()`
raised, the attribute was never set, tearDown would AttributeError,
and any context entered along the way leaked.
Now: class-level `_team_scope_cm = None`, assigned only after
__enter__() succeeds, tearDown checks for None before __exit__ and
nulls out the attribute in a try/finally. Worst case is a missed
teardown of an unentered CM (no leaked scope), not a permanent
ContextVar pollution across tests.
…r mypy Class-level None default needs an explicit type annotation under mypy's var-annotated check. Add 'AbstractContextManager[None] | None'.
The README was written against the early framework shape (separate ProductTeamManager, manual canonical resolution, paths-based semgrep opt-in) and didn't reflect the design changes from later commits in this PR. Bring it up to date so contributors land on a description of what's actually in the code. What changed: - **Lead with the canonical-team-id contract.** Single ContextVar holds the canonical id; reads filter by it; ProductTeamModel.save() rewrites writes to canonical. This is the central invariant. - **Document `canonical: bool = False` opt-out** on team_scope, with_team_scope, for_team. Explain that the default auto-resolves to remove the silent-zero-rows footgun haacked flagged. - **Drop the "which manager to use" table** — after the ProductTeamManager → TeamScopedManager consolidation in 20526af, both worlds use the same manager class. Replaced with separate "New product on separate database" and "Existing main-DB model" subsections under Adoption. - **Document `@skip_team_scope_audit`** — where it lives (`posthog.scoping_audit`, intentionally not under `posthog.models.scoping` for module-import-safety reasons), when to apply it, when to remove it. - **Clarify `Model.objects.unscoped()` vs `Model.all_teams`** — the queryset-method-vs-bypass-Manager distinction, why the bypass Manager is deliberately not named `unscoped` (autocomplete footgun). - **Note that there's intentionally no middleware fallback.** Earlier README mentioned middleware in the framing; the middleware was removed in e71383f. Document that "non-DRF paths must opt in" is the deliberate choice, not an oversight. - **Link #50899** as the org-level analog of the URL-vs-current-team bug class this framework's URL-derived scope sourcing avoids.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PostHog has ~263 Django models, ~35 of which get team scope only indirectly through FK chains. There's no automated enforcement that queries are team-scoped — it's code review only. Continues haacked's work from #47073.
Changes
Adds opt-in fail-closed team scoping for Django models. Querying a scoped model without team context raises
TeamScopeErrorinstead of silently returning all rows.Framework (
posthog/models/scoping/)team_scope()/with_team_scope()— context manager + decorator. Caller passes the canonical team_id (parent if the team is a child env, the team's own id otherwise).resolve_effective_team_id()— public helper for callers that have a raw team_id.TeamScopedManager/TeamScopedQuerySet— fail-closed manager for main-DB models. Auto-filters byctx.team_id. Raises if no context. Escape hatches:.unscoped(),.for_team(id).ProductTeamModel— abstract base for separate-DB products. Plainteam_idBigIntegerField (no FK across DBs).save()rewrites to canonical, mirror ofRootTeamMixin. Two managers:objects(fail-closed, scoped) andall_teams(bypass, for admin / migrations / mgmt commands — deliberately not namedunscopedto avoid collision with the queryset method).DRF integration
TeamAndOrgViewSetMixin.initial()sets canonical team scope from URLself.team_idfor nested DRF views.dispatch()override resets the ContextVar token in a finally block — safety net against subclass overrides that skip super(), preventing scope leaking across requests on a reused worker thread.user.current_team_id) avoids the same class of bug as fix: ensure permission checks are scoped to correct org #50899.Visual review wired up as first consumer
ProductTeamModel(Repo,Run,RunSnapshot,Artifact,ToleratedHash,QuarantinedIdentifier)process_run_diffstakesteam_id+@with_team_scope()team_scopefixtureCI guardrails
check-idor-model-coverage.pyscript + GitHub Action: any new model withoutteam_id(FK or BigIntegerField) fails CI unless explicitly added toLEGITIMATELY_UNSCOPEDorNEEDS_TEAM_IDbaseline.celery-team-scope.yaml) flags Celery tasks accessing scoped models without@with_team_scopeor.unscoped(). Currently scoped toproducts/visual_review/**to avoid noise from older tasks; expandpaths.includeas more products adopt.Notable design decisions (see PR comments for the discussion threads)
TeamScopingMiddleware: original design had middleware default scope touser.current_team_id. Removed because it's a UI preference field that drifts from the team being acted on — silent fallback is exactly the bug class the framework prevents. Non-DRF code paths must opt in viateam_scope()/unscoped(). Fail-closed by default.filter(team_id=team_id). No parent resolution at read time. Writes resolve to canonical viasave(). Reads and writes agree on storage. Same conventionRootTeamMixinhas used for main-DB models.Limitations (documented in
posthog/models/scoping/README.md)This is defense-in-depth, not a complete security boundary. Django's
_base_managerbypasses custom managers for related-object access (run.snapshots.all()), and raw SQL bypasses the ORM entirely. FK traversal is safe because the FK constrains results, but noteam_idfilter is applied. Full enforcement would need Postgres RLS.How did you test this code?
I'm Claude Code (agent) — only automated tests, no manual UI testing.
test_scoping.py,test_manager.py,test_product_mixin.py) — 47 pass locallytest_routing.py::TestTeamAndOrgViewSetMixin) including the URL-vs-current-team-id assertion — 6 pass locallyteam_scopefixture works for django_db teststest_tasks.py@shared_taskand@shared_task(...)declarations, ignores tasks with@with_team_scopeor.unscoped()Publish to changelog?
No — infrastructure only, no user-facing changes.
Docs update
No — internal framework, the README in
posthog/models/scoping/is the contributor docs.🤖 Agent context
parent_team_idresolution out of the manager and into the caller (mixin /save()) for symmetric reads/writes — addresses the Codex P1 read/write asymmetry flagunscopedtoall_teamsto remove autocomplete footgun (haacked review)products/visual_review/**instead of converting 137 pre-existing tasksprocess_run_diffssignature change. Visual review's queue is small/young; we'll accept stale "processing" rows on deploy and re-trigger via the GitHub action if needed.