Skip to content

fix(auth): eliminate duplicate DB sessions in auth and RBAC middleware#3886

Open
MohanLaksh wants to merge 1 commit intomainfrom
fix/issue-3622-duplicate-sessions-auth-rbac
Open

fix(auth): eliminate duplicate DB sessions in auth and RBAC middleware#3886
MohanLaksh wants to merge 1 commit intomainfrom
fix/issue-3622-duplicate-sessions-auth-rbac

Conversation

@MohanLaksh
Copy link
Copy Markdown
Collaborator

Problem

After PR #3600 fixed duplicate session creation in observability middleware and PR #3813 established proper transaction control, auth and RBAC middleware remain the last components creating duplicate database sessions.

Current State:

  • ObservabilityMiddleware: Creates 1 session ✓
  • AuthContextMiddleware: Creates 3 additional sessions for security logging ✗
  • RBACMiddleware: Has deprecated get_db() that creates 1 session ✗
  • Route handlers: Reuse middleware session via get_db()

Total: Up to 4-6 sessions per authenticated request

Issues:

Solution

Apply the established session-sharing pattern from PR #3600 to auth and RBAC middleware.

Implementation

1. Auth Middleware (auth_middleware.py)

  • Added _get_or_create_session() helper function
  • Checks for existing request.state.db from ObservabilityMiddleware
  • Falls back to creating session if none exists (when observability disabled)
  • Returns (session, owned) tuple to track ownership
  • Applied pattern to 3 SessionLocal() calls at lines 134, 159, 213
  • Only closes session if it was created (owned=True)
  • Removed all db.commit() calls (transaction control delegated to get_db())

2. RBAC Middleware (rbac.py)

  • Updated deprecated get_db() signature: def get_db(request: Request = None)
  • Checks request.state.db and reuses if available
  • Falls back to creating own session for backwards compatibility
  • Added DeprecationWarning to guide migration
  • Matches pattern from main.py:get_db() (line 3089)

3. Session Lifecycle (No changes to middleware order)

Request
  ↓
ObservabilityMiddleware (runs first, creates session)
  ↓
AuthContextMiddleware (runs second, reuses session)
  ↓
Route Handler via get_db() (reuses session, controls transactions)
  ↓
get_db() commits on success / rollbacks on error
  ↓
ObservabilityMiddleware closes session in finally block

Fallback Strategy:

  • When observability enabled: ObservabilityMiddleware creates session
  • When observability disabled: AuthContextMiddleware creates session
  • Edge case: If both disabled, get_db() in route handler creates session

Test Coverage

Unit Tests (14 new tests):

  • 7 tests for auth middleware session reuse patterns
  • 7 tests for RBAC get_db() deprecation and reuse

Integration Tests (6 new tests):

  • Single session per request with all middleware enabled
  • Session creation when observability disabled (fallback)
  • Auth middleware reusing observability session
  • Session sharing under concurrent load
  • RBAC get_db() integration with session sharing

Coverage: 100% (435 statements, 0 missing lines) - Exceeds 95% CI/CD requirement

Impact

Performance:

  • Reduces session creation from 4-6 per request → 1 per request (83% reduction)
  • Prevents connection pool exhaustion
  • Reduces memory overhead

Security:

  • Transaction isolation maintained (get_db() sole authority for commit/rollback)
  • Connection invalidation for PostgreSQL/PgBouncer compatibility
  • No new security vulnerabilities introduced (bandit scan: 0 issues)

Compatibility:

  • Backwards compatible (no breaking changes)
  • Existing dependency overrides continue to work
  • Migration path documented via deprecation warnings

Quality Checks

All checks passed:

  • Tests: 15,443 passed (529 skipped, 4 xfailed)
  • Coverage: 100% (4160/4160 docstrings)
  • Pylint: 10.00/10
  • Bandit: 0 security issues (148,516 lines scanned)
  • Flake8, ruff, interrogate: All passed

Files Changed

  • mcpgateway/middleware/auth_middleware.py (session reuse implementation)
  • mcpgateway/middleware/rbac.py (deprecated get_db() update)
  • tests/unit/mcpgateway/middleware/test_auth_middleware.py (7 new tests)
  • tests/unit/mcpgateway/middleware/test_rbac.py (7 new tests)
  • tests/integration/test_middleware_session_sharing.py (6 new integration tests)

Stats: 5 files changed, 857 insertions(+), 30 deletions(-)

Closes #3622

Implements session reuse pattern from PR #3600 and PR #3813 to achieve
1 shared database session per request across all middleware layers.

**Changes:**
- Auth middleware: Added _get_or_create_session() helper to reuse
  request.state.db from ObservabilityMiddleware (lines 134, 159, 213)
- RBAC middleware: Updated deprecated get_db() to accept optional
  request parameter and reuse middleware session when available
- Transaction control: Delegated all commit/rollback to get_db() per
  PR #3813 (removed db.commit() from auth middleware)
- Added 7 unit tests for auth session reuse patterns
- Added 7 unit tests for RBAC get_db() deprecation
- Added 6 integration tests for end-to-end session sharing validation

**Impact:**
- Reduces session creation from 4-6 per request to 1 per request
- Prevents connection pool exhaustion under load
- Achieves 100% test coverage (435 statements, 0 missing)

**Security:**
- Transaction isolation maintained (get_db() controls all commits)
- Connection invalidation for PgBouncer compatibility
- Backwards compatible (existing dependency overrides work)

Closes #3622

Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com>
@MohanLaksh
Copy link
Copy Markdown
Collaborator Author

@ja8zyjits , Can you please help me review this PR?

@MohanLaksh MohanLaksh requested a review from ja8zyjits March 27, 2026 09:11
@crivetimihai crivetimihai changed the title fix: eliminate duplicate DB sessions in auth and RBAC middleware fix(auth): eliminate duplicate DB sessions in auth and RBAC middleware Mar 29, 2026
@crivetimihai crivetimihai added bug Something isn't working MUST P1: Non-negotiable, critical requirements without which the product is non-functional or unsafe performance Performance related items labels Mar 29, 2026
@crivetimihai crivetimihai added this to the Release 1.0.0 milestone Mar 29, 2026
@crivetimihai
Copy link
Copy Markdown
Member

Thanks @MohanLaksh. The session reuse pattern via _get_or_create_session() is the right approach — it aligns with the pattern from PR #3600. Two items to verify:

  1. You removed db.commit() calls with a note that transactions are managed by get_db() — please confirm that auth logging events are still persisted correctly when the session comes from request.state.db.
  2. The DCO Signed-off-by line is required on all commits. Please update with git commit --amend -s.

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

Labels

bug Something isn't working MUST P1: Non-negotiable, critical requirements without which the product is non-functional or unsafe performance Performance related items

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG][PERFORMANCE]: Auth and RBAC middleware create duplicate DB sessions alongside observability middleware

2 participants