274 lines
9.9 KiB
Markdown
274 lines
9.9 KiB
Markdown
# 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
|
|
|