@crivetimihai I confirm that my changes are intact in this PR

Deep-Dive Review — @KKNithin @jonpspri

Rebased cleanly onto main (6 linear commits, no conflicts). All tests pass (118 auth, 76 token-scoping, 79 email-auth-router). 100% differential test coverage on new lines. E2E verified against running server on localhost:8080.

Finding 1 (Critical): PR does NOT implement issue #3003's user story

Issue #3003 asks:

"When teams are provided in the claims of a session token, use them instead of resolving teams from DB"
User story: "I want to use a session token with teams=[team1] so that I can query resources scoped to team1 only"

What the PR actually does: resolve_session_teams() always resolves from DB and completely ignores the JWT's teams claim. The payload parameter is explicitly unused.

Commit evolution:

• @KKNithin's commits 1–4: Implemented the feature as requested (trust JWT embedded teams for session tokens, restricted to single-team only in v2)
• @jonpspri's commit 6: Removed that feature entirely, always DB-resolving. Rationale: "trusting embedded JWT claims could allow stale team scoping until token expiry"

Jonathan's security concern is valid — but the result is that Closes #3003 doesn't deliver the requested feature. A user in team1+team2 cannot narrow their session to team1 only.

Suggested resolution — intersection approach:

Resolve from DB first (ensuring revoked memberships are enforced), then intersect with JWT-embedded teams if present. This gives both the security property and the scope-narrowing user story:

async def resolve_session_teams(payload, email, user_info):
    db_teams = await _resolve_teams_from_db(email, user_info)
    if db_teams is None:  # admin bypass
        return None
    jwt_teams = payload.get("teams")
    if isinstance(jwt_teams, list) and jwt_teams:
        # Narrow to intersection — only teams the user still has in DB
        narrowed = [t for t in db_teams if t in jwt_teams]
        return narrowed if narrowed else db_teams
    return db_teams

This way:

• Revoked teams are never returned (DB is the authority)
• Users can narrow their session to a subset of their actual teams
• If the intersection is empty (all JWT teams revoked), falls back to full DB teams rather than locking the user out

Finding 2 (Medium): "Single policy point" claim has gaps

The docstring claims resolve_session_teams is "the single policy point for session-token team resolution." In practice, 3 of 6 session-token resolution paths bypass it:

The batched queries path is the default production path for first-time requests (auth_cache_batch_queries=True by default). If policy logic (e.g. the intersection above) is added to resolve_session_teams() later, the batched path would silently bypass it.

Suggested fix: Update main.py:1762 to call resolve_session_teams(). For the batched queries path, either route through resolve_session_teams() after the batch, or add a code comment marking it as a known exception. For streamablehttp_transport.py, a sync companion resolve_session_teams_sync() would be ideal, or at minimum a comment linking to the async version.

Finding 3 (Low): _check_team_membership reads JWT claims, not resolved teams

In token_scoping.py:1194, after resolving teams from DB into token_teams, the middleware calls _check_team_membership(payload) which reads payload.get("teams") — the original JWT claims, not the DB-resolved token_teams.

For normal session tokens (no teams in JWT), this is fine — _check_team_membership gets [] and returns True. But it means there are two different "team" views in the same function: DB-resolved (for scoping) and JWT-based (for membership validation). Not a bug in practice, but a confusing code pattern that could surprise future maintainers.

Finding 4 (Informational): email_auth_router test changes are correct

The added mock attributes (auth_provider, get_teams, team_memberships, password_change_enforcement_enabled) are test maintenance fixes needed because production code on main was updated to access these attributes. Correctly aligned.

Summary

The code quality, test coverage, and security posture are solid. The core question is whether the PR should actually deliver the feature #3003 requested (scope-narrowing via intersection) or accept the current DB-only approach and file a follow-up.

Addendum — cross-verified with Codex

Ran an independent automated review (Codex, read-only) against git diff main. Results:

• Finding 1 (Critical): Confirmed. resolve_session_teams ignores payload at line 321. Commit history traces the rollback (f4e1e92 added JWT trust → c6ae9fb removed it). Issue [FEATURE][AUTH]: Use team scope from session token claims instead of DB resolution #3003 user story is not delivered.

• Finding 2 (Medium): Confirmed. Batched queries path (auth.py:1138-1145) is the default production path (auth_cache_batch_queries=True per config.py:1340) and bypasses resolve_session_teams(). main.py:1762 and streamablehttp_transport.py:1219,2544 also bypass it. "Single policy point" claim in the docstring and docs (multitenancy.md:322, rbac.md:141) is not accurate.

• Finding 3 (Low): Confirmed. _check_team_membership reads payload.get("teams") (JWT) at token_scoping.py:714, not the DB-resolved token_teams. With a crafted JWT teams: ["fake"], the 403 comes from _check_team_membership, not from the DB resolution. Practical impact is low since create_access_token never embeds teams in session tokens.

• Finding 4 (Informational): Partially confirmed. The auth_provider and password_change_enforcement_enabled / detect_default_password_on_login / require_password_change_for_default_password fixes are correct and necessary. However, get_teams and team_memberships added to the mock user fixture at test_email_auth_router.py:44-45 appear unnecessary — the tested code paths (login returning 403 for password change) return before reaching create_access_token or any code that accesses these attributes. They're harmless but could be removed.

• Additional — test mock gap: Confirmed. New tests mock _resolve_teams_from_db directly (e.g., test_auth.py:330, test_token_scoping.py:429). These tests would pass even if the code called _resolve_teams_from_db() directly instead of going through resolve_session_teams(). No test asserts that resolve_session_teams itself is called.

Overall assessment (concurred by both reviewers): Requires rework if this PR is meant to close #3003 as written. The security refactoring is solid, but the feature isn't delivered and the docs overstate centralization.

@crivetimihai @jonpspri I am ok with is new architecture of narrowing teams, its actually better than what was implemented and make it more secure.

Response to deep-dive review findings

Thanks @crivetimihai for the thorough review and Codex cross-validation. All findings have been addressed across the commit history. Here's the current disposition:

Finding 1 (Critical): PR didn't implement #3003 user story — RESOLVED

The intersection approach you suggested is now implemented in _narrow_by_jwt_teams() (shared helper) called by both resolve_session_teams() and resolve_session_teams_sync(). The policy is:

1. Always resolve teams from DB first (revoked memberships enforced immediately)
2. If the JWT carries a non-empty teams claim, intersect with DB teams
3. If the intersection is empty (all JWT teams revoked), return [] — fail-closed (public-only scope, not a fallback to full DB teams)
4. If no JWT teams claim (or null/[]), return full DB membership

This delivers the #3003 user story (a user in team1+team2 can narrow their session to team1 only) while maintaining the security property that revoked memberships take effect immediately.

Semantic note on empty intersection: The original suggested code fell back to full DB teams on empty intersection. We chose fail-closed ([]) instead — if a user's JWT claims teams they've been removed from, they get demoted to public-only scope rather than silently broadened to their full membership. This is the more conservative security posture.

Finding 2 (Medium): "Single policy point" had gaps — RESOLVED

All 6 session-token resolution paths now go through the public API:

The batched path passes preresolved_db_teams to skip a redundant DB call while still routing through the intersection policy. The cache stores raw DB teams (not the narrowed intersection) so multiple sessions for the same user narrow independently.

Finding 3 (Low): _check_team_membership reads JWT not resolved teams — RESOLVED

Session tokens now skip _check_team_membership entirely in the token scoping middleware (both token_teams and public-only branches have token_use != "session" guards). Since resolve_session_teams() already resolved membership from the DB, re-checking the raw JWT claim would be both redundant and incorrect (it would conflict with the intersection semantics). Membership staleness is bounded by the auth_cache TTL.

Finding 4 (Informational): email_auth_router test changes — acknowledged

No action taken; these are compatibility fixes for main changes as noted.

Additional: test mock gap (no test asserted resolve_session_teams is called) — RESOLVED

test_session_token_calls_resolve_session_teams in test_token_scoping.py directly mocks and asserts that resolve_session_teams is the function called by the middleware, not _resolve_teams_from_db directly. Additional direct unit tests for _narrow_by_jwt_teams cover edge cases (admin bypass, empty intersection, malformed entries, teams: null, teams: []).

Documentation updates

• multitenancy.md: Separate tables for API/legacy vs session token scoping, intersection semantics, cache isolation, staleness admonition
• rbac.md: Matching session token resolution table, pipeline step updated, staleness admonition
• securing.md: Updated "Session Tokens vs API Tokens" note to mention JWT intersection narrowing

Review Changes

Rebased onto main (no conflicts) and made the following changes:

Commits added

1. 7875328ec — Removed AI co-author reference from Jonathan Springer's commit (amend only — removed Co-Authored-By: Claude Opus 4.6 line, no code change).

2. 4cae8a55d — docs(rbac): clarify admin bypass requirements per token type
   Split the admin bypass warning in rbac.md into API/legacy vs session token sections. The previous wording said admin bypass requires teams: null without qualifying that this only applies to API/legacy tokens — contradicting the session token table directly above it.

3. ef2360341 — docs(AGENTS.md): document session-token scoping contract
   Added session token table and key behaviors to the project guidance (AGENTS.md). Updated the security invariant to reference both normalize_token_teams() and resolve_session_teams(). Added explicit note that session-token narrowing is Layer 1 (visibility) only.

4. 4c3c910e8 — docs: qualify all admin bypass statements by token type
   Fixed remaining stale references in rbac.md ("Explicit Admin Bypass" section, scoping strategy table) that described admin bypass without the API/legacy vs session token distinction.

5. 9069a0a74 — docs(multitenancy): qualify teams-key behavior by token type
   Fixed multitenancy.md enforcement summary: "Missing teams key always returns []" and "Empty teams [] results in public-only" are only true for API/legacy tokens; session tokens return full DB membership for missing/null/empty teams. Also fixed one more instance in rbac.md Security Design Principles section.

No code changes

All original code by @KKNithin and @jonpspri is unchanged. The changes are documentation-only.

Follow-up issue

Filed #3799 to track narrowing RBAC Layer 2 permission checks to match session-token team scope (pre-existing behavior, out of scope for this PR).

Test results

All 1407 tests pass across all affected test files.