Skip to content

Commit cb783cf

Browse files
rakduttarakdutta
committed
fix
Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
1 parent e245d5d commit cb783cf

5 files changed

Lines changed: 787 additions & 2 deletions

File tree

‎ISSUE-4482-FIX-SUMMARY.md‎

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
# Fix for Issue #4482: RBAC Race Condition
2+
3+
## Summary
4+
5+
This fix addresses the potential race condition in RBAC role and user_role seeding when multiple replicas/workers bootstrap the database concurrently. The fix implements defense-in-depth with both database-level and application-level protection.
6+
7+
## Changes Made
8+
9+
### 1. Database Migration (`d21698ae4a19_add_rbac_unique_constraints_race_fix.py`)
10+
11+
**Purpose**: Add database-level unique constraints to prevent duplicate active roles and role assignments.
12+
13+
**Key Features**:
14+
- ✅ **Supports both PostgreSQL and SQLite**: Uses dialect-specific SQL for boolean values and partial indexes
15+
- ✅ **Idempotent**: Checks for existing indexes before creating them
16+
- ✅ **Safe for existing data**: Deduplicates any existing duplicate active rows before adding constraints
17+
- ✅ **Audit-friendly**: Soft-deletes duplicates (sets `is_active=false`) instead of hard-deleting them
18+
19+
**What it does**:
20+
21+
1. **Deduplication (STEP 1 & 2)**:
22+
- Finds duplicate active `roles` with same `(name, scope)` - keeps oldest by `created_at`
23+
- Finds duplicate active `user_roles` with same `(user_email, role_id, scope, scope_id)` - keeps oldest by `granted_at`
24+
- Soft-deletes newer duplicates by setting `is_active=false`
25+
- Preserves audit history - duplicates remain in DB for forensics
26+
27+
2. **Unique Constraints (STEP 3)**:
28+
- Creates partial unique index: `uq_roles_name_scope_active` on `roles(name, scope) WHERE is_active = true`
29+
- Creates partial unique index: `uq_user_roles_email_role_scope_null_active` on `user_roles(user_email, role_id, scope) WHERE scope_id IS NULL AND is_active = true`
30+
- Creates partial unique index: `uq_user_roles_email_role_scope_id_active` on `user_roles(user_email, role_id, scope, scope_id) WHERE scope_id IS NOT NULL AND is_active = true`
31+
32+
**Why partial indexes?**:
33+
- Only active rows (`is_active = true`) need uniqueness constraint
34+
- Allows multiple inactive/historical rows with same values (for audit purposes)
35+
- Split `user_roles` indexes handle nullable `scope_id` (PostgreSQL/SQLite treat NULL as distinct in unique indexes)
36+
37+
**Downgrade**:
38+
- Drops the three unique indexes
39+
- Does NOT reactivate soft-deleted duplicates (preserves audit trail)
40+
41+
### 2. Application-Level Changes (`mcpgateway/services/role_service.py`)
42+
43+
**Purpose**: Handle IntegrityError gracefully when database constraints prevent duplicates.
44+
45+
**Changes**:
46+
47+
1. **Import IntegrityError**:
48+
```python
49+
from sqlalchemy.exc import IntegrityError
50+
```
51+
52+
2. **Updated `create_role()` method**:
53+
- Wraps insert in `db.begin_nested()` (savepoint)
54+
- On `IntegrityError`: rolls back savepoint, refetches existing role, returns it
55+
- No error to caller - seamlessly returns the winner's row
56+
- Logs info-level message about concurrent creation
57+
58+
3. **Updated `assign_role_to_user()` method**:
59+
- Same savepoint + refetch pattern
60+
- On `IntegrityError`: rolls back, refetches existing assignment, returns it
61+
- Transparent to callers
62+
63+
**Benefits**:
64+
- No breaking changes - methods still return the role/assignment
65+
- Prevents `MultipleResultsFound` errors that would cause 500 responses
66+
- Logs provide visibility into concurrent operations
67+
- If refetch fails (shouldn't happen), raises descriptive error
68+
69+
## Security Considerations
70+
71+
### ✅ No Security Issues Introduced
72+
73+
1. **No authentication bypass**: Changes are pure data integrity - don't affect auth flows
74+
2. **No privilege escalation**: Doesn't modify role permissions or RBAC logic
75+
3. **No data exposure**: Deduplication only affects active rows, preserves audit history
76+
4. **No SQL injection**: Uses parameterized queries and SQLAlchemy ORM
77+
5. **Defense in depth**: Database constraints are ultimate authority, application handles gracefully
78+
79+
### ✅ Backwards Compatible
80+
81+
1. **Existing functionality preserved**:
82+
- All existing role/assignment operations work identically
83+
- Bootstrap flows unchanged (just more robust)
84+
- No API changes, no schema-breaking changes
85+
86+
2. **Safe migration**:
87+
- Idempotent - can run multiple times safely
88+
- Works on fresh DBs (skips if tables don't exist)
89+
- Works on populated DBs (deduplicates first)
90+
- Downgrade available (though not recommended in production)
91+
92+
## Testing Recommendations
93+
94+
Before committing, run:
95+
96+
```bash
97+
# 1. Check code quality
98+
make ruff
99+
make pylint
100+
make mypy
101+
102+
# 2. Run unit tests
103+
make test
104+
105+
# 3. Test migration on SQLite (default .env)
106+
.venv/bin/alembic -c mcpgateway/alembic.ini upgrade head
107+
.venv/bin/alembic -c mcpgateway/alembic.ini downgrade -1
108+
.venv/bin/alembic -c mcpgateway/alembic.ini upgrade head
109+
110+
# 4. Test migration on PostgreSQL (if available)
111+
# Set DATABASE_URL=postgresql://...
112+
# Repeat alembic commands above
113+
114+
# 5. Integration tests (if using --with-integration)
115+
make test-integration
116+
```
117+
118+
## Files Modified
119+
120+
1. **New file**: `mcpgateway/alembic/versions/d21698ae4a19_add_rbac_unique_constraints_race_fix.py`
121+
- Migration script (320 lines)
122+
123+
2. **Modified**: `mcpgateway/services/role_service.py`
124+
- Added IntegrityError import
125+
- Updated `create_role()` with savepoint + refetch pattern (+19 lines)
126+
- Updated `assign_role_to_user()` with savepoint + refetch pattern (+19 lines)
127+
128+
## Commit Message Template
129+
130+
```
131+
fix(rbac): add unique constraints to prevent role/user_role seeding race (#4482)
132+
133+
Fixes issue #4482 - RBAC role/user_role seeder race when fast-path skips
134+
advisory lock.
135+
136+
This fix implements defense-in-depth to prevent duplicate active roles
137+
and user role assignments when multiple replicas/workers bootstrap the
138+
database concurrently.
139+
140+
Changes:
141+
- Add database-level partial unique indexes on roles and user_roles tables
142+
- Update RoleService to handle IntegrityError gracefully with savepoint pattern
143+
- Deduplicate any existing duplicate active rows in migration (soft-delete)
144+
- Support both PostgreSQL and SQLite databases
145+
146+
The database constraints are the ultimate authority on uniqueness, while
147+
the application-level handling ensures no errors propagate to callers.
148+
149+
Migration: d21698ae4a19 (idempotent, safe for existing data)
150+
151+
Signed-off-by: [Your Name] <[your-email]>
152+
```
153+
154+
## Related Issues
155+
156+
- Issue #4482: RBAC role/user_role seeder race when fast-path skips advisory lock
157+
- PR #4444: fix(bootstrap): improve startup reliability for multi-replica deploys (not yet merged)
158+
- PR #4480: Draft fix mentioned in issue #4482 (this is an independent implementation)
159+
160+
## Notes for Reviewers
161+
162+
1. **This fix is safe to merge NOW** - it works with current code (advisory locks in place)
163+
2. **Makes PR #4444 safe** - when the fast-path lands, these constraints prevent the race
164+
3. **Two-layer defense**: DB constraints (authority) + app handling (graceful recovery)
165+
4. **No performance impact**: Partial indexes only on active rows, minimal overhead
166+
5. **Audit-friendly**: Soft-deletes preserve history, no data loss

0 commit comments

Comments
 (0)