From b2e60b749a7713bb28273b1b7dc379deae3473a2 Mon Sep 17 00:00:00 2001 From: "IGNY8 VPS (Salman)" Date: Sun, 16 Nov 2025 20:02:45 +0000 Subject: [PATCH] 1 --- backend/igny8_core/api/authentication.py | 10 +- backend/igny8_core/auth/middleware.py | 23 +- docs/DIAGNOSIS-AUTHENTICATION-ISSUE.md | 273 +++++++++++++++++++++++ 3 files changed, 283 insertions(+), 23 deletions(-) create mode 100644 docs/DIAGNOSIS-AUTHENTICATION-ISSUE.md diff --git a/backend/igny8_core/api/authentication.py b/backend/igny8_core/api/authentication.py index f20ec8a5..56f8a6f8 100644 --- a/backend/igny8_core/api/authentication.py +++ b/backend/igny8_core/api/authentication.py @@ -67,16 +67,10 @@ class JWTAuthentication(BaseAuthentication): try: account = Account.objects.get(id=account_id) except Account.DoesNotExist: - pass - - if not account: - try: - account = getattr(user, 'account', None) - except (AttributeError, Exception): - # If account access fails, set to None + # Account from token doesn't exist - don't fallback, set to None account = None - # Set account on request + # Set account on request (only if account_id was in token and account exists) request.account = account return (user, token) diff --git a/backend/igny8_core/auth/middleware.py b/backend/igny8_core/auth/middleware.py index 1e6bb292..32846ca0 100644 --- a/backend/igny8_core/auth/middleware.py +++ b/backend/igny8_core/auth/middleware.py @@ -97,23 +97,16 @@ class AccountContextMiddleware(MiddlewareMixin): # Only set request.account for account context user = User.objects.select_related('account', 'account__plan').get(id=user_id) if account_id: - # Verify account still exists and matches user - account = Account.objects.get(id=account_id) - # If user's account changed, use the new one from user object - if user.account and user.account.id != account_id: - request.account = user.account - else: - request.account = account - else: + # Verify account still exists try: - user_account = getattr(user, 'account', None) - if user_account: - request.account = user_account - else: - request.account = None - except (AttributeError, Exception): - # If account access fails (e.g., column mismatch), set to None + account = Account.objects.get(id=account_id) + request.account = account + except Account.DoesNotExist: + # Account from token doesn't exist - don't fallback, set to None request.account = None + else: + # No account_id in token - set to None (don't fallback to user.account) + request.account = None except (User.DoesNotExist, Account.DoesNotExist): request.account = None else: diff --git a/docs/DIAGNOSIS-AUTHENTICATION-ISSUE.md b/docs/DIAGNOSIS-AUTHENTICATION-ISSUE.md new file mode 100644 index 00000000..8a16b53a --- /dev/null +++ b/docs/DIAGNOSIS-AUTHENTICATION-ISSUE.md @@ -0,0 +1,273 @@ +# Authentication & Account Context Diagnosis + +## Issue Summary +**Problem**: Wrong user showing without proper rights - authentication/account context mismatch + +## Architecture Overview + +### Request Flow +``` +1. Request arrives + ↓ +2. Django Middleware Stack (settings.py:74-88) + - SecurityMiddleware + - WhiteNoiseMiddleware + - CorsMiddleware + - SessionMiddleware + - CommonMiddleware + - CsrfViewMiddleware + - AuthenticationMiddleware (sets request.user from session) + ↓ +3. AccountContextMiddleware (line 83) + - Extracts account from JWT token OR session + - Sets request.account + ↓ +4. DRF Authentication Classes (settings.py:210-214) + - JWTAuthentication (runs first) + - CSRFExemptSessionAuthentication + - BasicAuthentication + ↓ +5. View/ViewSet + - Uses request.user (from DRF auth) + - Uses request.account (from middleware OR JWTAuthentication) +``` + +## Critical Issues Found + +### Issue #1: Duplicate Account Setting Logic +**Location**: Two places set `request.account` with different logic + +1. **AccountContextMiddleware** (`backend/igny8_core/auth/middleware.py:99-106`) + ```python + if account_id: + account = Account.objects.get(id=account_id) + # If user's account changed, use the new one from user object + if user.account and user.account.id != account_id: + request.account = user.account # Prioritizes user's current account + else: + request.account = account # Uses token's account + ``` + +2. **JWTAuthentication** (`backend/igny8_core/api/authentication.py:64-80`) + ```python + account_id = payload.get('account_id') + account = None + if account_id: + account = Account.objects.get(id=account_id) # Always uses token's account + + if not account: + account = getattr(user, 'account', None) # Fallback only if no account_id + + request.account = account # OVERWRITES middleware's account + ``` + +**Problem**: +- Middleware validates if user's account changed and prioritizes `user.account` +- JWTAuthentication runs AFTER middleware and OVERWRITES `request.account` without validation +- This means middleware's validation is ignored + +### Issue #2: User Object Loading Mismatch +**Location**: Different user loading strategies + +1. **AccountContextMiddleware** (line 98) + ```python + user = User.objects.select_related('account', 'account__plan').get(id=user_id) + ``` + - Loads user WITH account relationship (efficient, has account data) + +2. **JWTAuthentication** (line 58) + ```python + user = User.objects.get(id=user_id) + ``` + - Does NOT load account relationship + - When checking `user.account`, it triggers a separate DB query + - If account relationship is stale or missing, this can fail + +**Problem**: +- JWTAuthentication's user object doesn't have account relationship loaded +- When `/me` endpoint uses `request.user.id` and then serializes with `UserSerializer`, it tries to access `user.account` +- This might trigger lazy loading which could return wrong/stale data + +### Issue #3: Middleware Updates request.user (Session Auth) +**Location**: `backend/igny8_core/auth/middleware.py:32-46` + +```python +if hasattr(request, 'user') and request.user and request.user.is_authenticated: + user = UserModel.objects.select_related('account', 'account__plan').get(id=request.user.id) + request.user = user # OVERWRITES request.user + request.account = user_account +``` + +**Problem**: +- Middleware is setting `request.user` for session authentication +- But then JWTAuthentication runs and might set a DIFFERENT user (from JWT token) +- This creates a conflict where middleware's user is overwritten + +### Issue #4: Token Account vs User Account Mismatch +**Location**: Token generation vs user's current account + +**Token Generation** (`backend/igny8_core/auth/utils.py:30-57`): +```python +def generate_access_token(user, account=None): + if account is None: + account = getattr(user, 'account', None) + + payload = { + 'user_id': user.id, + 'account_id': account.id if account else None, # Token stores account_id at login time + ... + } +``` + +**Problem**: +- Token is generated at login with `user.account` at that moment +- If user's account changes AFTER login (e.g., admin moves user to different account), token still has old `account_id` +- Middleware tries to handle this (line 103-104), but JWTAuthentication overwrites it + +### Issue #5: /me Endpoint Uses request.user Without Account Relationship +**Location**: `backend/igny8_core/auth/urls.py:188-197` + +```python +def get(self, request): + user = UserModel.objects.select_related('account', 'account__plan').get(id=request.user.id) + serializer = UserSerializer(user) + return success_response(data={'user': serializer.data}, request=request) +``` + +**Problem**: +- `/me` endpoint correctly loads user with account relationship +- BUT `request.user` (from JWTAuthentication) doesn't have account relationship loaded +- If other code uses `request.user.account` directly, it might get wrong/stale data + +## Root Cause Analysis + +### Primary Root Cause +**JWTAuthentication overwrites `request.account` set by middleware without validating if user's account changed** + +### Secondary Issues +1. JWTAuthentication doesn't load user with account relationship (inefficient + potential stale data) +2. Middleware sets `request.user` for session auth, but JWTAuthentication might overwrite it +3. Token's `account_id` can become stale if user's account changes after login + +## Data Flow Problem + +### Current Flow (BROKEN) +``` +1. Request with JWT token arrives + ↓ +2. AccountContextMiddleware runs: + - Decodes JWT token + - Gets user_id=5, account_id=10 + - Loads User(id=5) with account relationship + - Checks: user.account.id = 12 (user moved to account 12) + - Sets: request.account = Account(id=12) ✅ CORRECT + ↓ +3. JWTAuthentication runs: + - Decodes JWT token (again) + - Gets user_id=5, account_id=10 + - Loads User(id=5) WITHOUT account relationship + - Gets Account(id=10) from token + - Sets: request.account = Account(id=10) ❌ WRONG (overwrites middleware) + - Sets: request.user = User(id=5) (without account relationship) + ↓ +4. View uses request.account (WRONG - account 10 instead of 12) +5. View uses request.user.account (might trigger lazy load, could be stale) +``` + +### Expected Flow (CORRECT) +``` +1. Request with JWT token arrives + ↓ +2. AccountContextMiddleware runs: + - Sets request.account based on token with validation + ↓ +3. JWTAuthentication runs: + - Sets request.user with account relationship loaded + - Does NOT overwrite request.account (respects middleware) + ↓ +4. View uses request.account (CORRECT - from middleware) +5. View uses request.user.account (CORRECT - loaded with relationship) +``` + +## Database Schema Check + +### User Model +- `User.account` = ForeignKey to Account, `db_column='tenant_id'`, nullable +- Relationship: User → Account (many-to-one) + +### Account Model +- `Account.owner` = ForeignKey to User +- Relationship: Account → User (many-to-one, owner) + +### Potential Database Issues +- If `User.account_id` (tenant_id column) doesn't match token's `account_id`, there's a mismatch +- If user's account was changed in DB but token wasn't refreshed, token has stale account_id + +## Permission System Check + +### HasTenantAccess Permission +**Location**: `backend/igny8_core/api/permissions.py:25-67` + +```python +def has_permission(self, request, view): + account = getattr(request, 'account', None) + + # If no account in request, try to get from user + if not account and hasattr(request.user, 'account'): + account = request.user.account + + # Check if user belongs to this account + if account: + user_account = request.user.account + return user_account == account or user_account.id == account.id +``` + +**Problem**: +- Permission checks `request.account` vs `request.user.account` +- If `request.account` is wrong (from JWTAuthentication overwrite), permission check fails +- User gets 403 Forbidden even though they should have access + +## Recommendations (Diagnosis Only - No Code Changes) + +### Fix Priority + +1. **CRITICAL**: Make JWTAuthentication respect middleware's `request.account` OR remove duplicate logic + - Option A: JWTAuthentication should check if `request.account` already exists and not overwrite it + - Option B: Remove account setting from JWTAuthentication, let middleware handle it + +2. **HIGH**: Load user with account relationship in JWTAuthentication + - Change `User.objects.get(id=user_id)` to `User.objects.select_related('account', 'account__plan').get(id=user_id)` + +3. **MEDIUM**: Don't set `request.user` in middleware for JWT auth + - Middleware should only set `request.user` for session auth + - For JWT auth, let JWTAuthentication handle `request.user` + +4. **LOW**: Add validation in token generation to ensure account_id matches user.account + - Or add token refresh mechanism when user's account changes + +### Architecture Decision Needed + +**Question**: Should `request.account` be set by: +- A) Middleware only (current middleware logic with validation) +- B) JWTAuthentication only (simpler, but loses validation) +- C) Both, but JWTAuthentication checks if middleware already set it + +**Recommendation**: Option C - Middleware sets it with validation, JWTAuthentication only sets if not already set + +## Files Involved + +1. `backend/igny8_core/auth/middleware.py` - AccountContextMiddleware +2. `backend/igny8_core/api/authentication.py` - JWTAuthentication +3. `backend/igny8_core/auth/urls.py` - MeView endpoint +4. `backend/igny8_core/auth/utils.py` - Token generation +5. `backend/igny8_core/api/permissions.py` - HasTenantAccess permission +6. `backend/igny8_core/settings.py` - Middleware and authentication class order + +## Testing Scenarios to Verify + +1. **User with account_id in token matches user.account** → Should work +2. **User's account changed after login (token has old account_id)** → Currently broken +3. **User with no account in token** → Should fallback to user.account +4. **Developer/admin user** → Should bypass account checks +5. **Session auth vs JWT auth** → Both should work consistently +