Skip to content

Commit 0615571

Browse files
rakduttarakdutta
committed
fix(rbac): exclude expired assignments from unique constraints and use deterministic deduplication
Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
1 parent a0499ac commit 0615571

2 files changed

Lines changed: 61 additions & 74 deletions

File tree

‎mcpgateway/alembic/versions/d21698ae4a19_add_rbac_unique_constraints_race_fix.py‎

Lines changed: 40 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -50,43 +50,36 @@ def upgrade() -> None:
5050
# Find duplicate active roles (same name+scope combination)
5151
# Strategy: soft-delete (set is_active=false) for duplicates, keep oldest by created_at
5252

53-
# PostgreSQL uses DELETE...RETURNING, SQLite needs subquery approach
53+
# Use row_number() for deterministic deduplication (handles timestamp ties via id ordering)
54+
# Keep the row with row_number = 1 (oldest created_at, lowest id on ties)
5455
if dialect == 'postgresql':
5556
dedupe_roles_sql = text("""
5657
UPDATE roles
5758
SET is_active = false
5859
WHERE id IN (
59-
SELECT r.id
60-
FROM roles r
61-
INNER JOIN (
62-
SELECT name, scope, MIN(created_at) as oldest_created_at
60+
SELECT id
61+
FROM (
62+
SELECT id,
63+
ROW_NUMBER() OVER (PARTITION BY name, scope ORDER BY created_at, id) as rn
6364
FROM roles
6465
WHERE is_active = true
65-
GROUP BY name, scope
66-
HAVING COUNT(*) > 1
67-
) dupes ON r.name = dupes.name
68-
AND r.scope = dupes.scope
69-
AND r.created_at > dupes.oldest_created_at
70-
WHERE r.is_active = true
66+
) ranked
67+
WHERE rn > 1
7168
)
7269
""")
7370
else: # SQLite
7471
dedupe_roles_sql = text("""
7572
UPDATE roles
7673
SET is_active = 0
7774
WHERE id IN (
78-
SELECT r.id
79-
FROM roles r
80-
INNER JOIN (
81-
SELECT name, scope, MIN(created_at) as oldest_created_at
75+
SELECT id
76+
FROM (
77+
SELECT id,
78+
ROW_NUMBER() OVER (PARTITION BY name, scope ORDER BY created_at, id) as rn
8279
FROM roles
8380
WHERE is_active = 1
84-
GROUP BY name, scope
85-
HAVING COUNT(*) > 1
86-
) dupes ON r.name = dupes.name
87-
AND r.scope = dupes.scope
88-
AND r.created_at > dupes.oldest_created_at
89-
WHERE r.is_active = 1
81+
) ranked
82+
WHERE rn > 1
9083
)
9184
""")
9285

@@ -102,94 +95,70 @@ def upgrade() -> None:
10295
# For user_roles, we need to handle scope_id IS NULL and IS NOT NULL separately
10396
# Strategy: soft-delete duplicates, keep oldest by granted_at
10497

105-
# Handle scope_id IS NULL case
98+
# Handle scope_id IS NULL case - use row_number() for deterministic tie-breaking
10699
if dialect == 'postgresql':
107100
dedupe_user_roles_null_scope_sql = text("""
108101
UPDATE user_roles
109102
SET is_active = false
110103
WHERE id IN (
111-
SELECT ur.id
112-
FROM user_roles ur
113-
INNER JOIN (
114-
SELECT user_email, role_id, scope, MIN(granted_at) as oldest_granted_at
104+
SELECT id
105+
FROM (
106+
SELECT id,
107+
ROW_NUMBER() OVER (PARTITION BY user_email, role_id, scope ORDER BY granted_at, id) as rn
115108
FROM user_roles
116109
WHERE is_active = true AND scope_id IS NULL
117-
GROUP BY user_email, role_id, scope
118-
HAVING COUNT(*) > 1
119-
) dupes ON ur.user_email = dupes.user_email
120-
AND ur.role_id = dupes.role_id
121-
AND ur.scope = dupes.scope
122-
AND ur.scope_id IS NULL
123-
AND ur.granted_at > dupes.oldest_granted_at
124-
WHERE ur.is_active = true
110+
) ranked
111+
WHERE rn > 1
125112
)
126113
""")
127114
else: # SQLite
128115
dedupe_user_roles_null_scope_sql = text("""
129116
UPDATE user_roles
130117
SET is_active = 0
131118
WHERE id IN (
132-
SELECT ur.id
133-
FROM user_roles ur
134-
INNER JOIN (
135-
SELECT user_email, role_id, scope, MIN(granted_at) as oldest_granted_at
119+
SELECT id
120+
FROM (
121+
SELECT id,
122+
ROW_NUMBER() OVER (PARTITION BY user_email, role_id, scope ORDER BY granted_at, id) as rn
136123
FROM user_roles
137124
WHERE is_active = 1 AND scope_id IS NULL
138-
GROUP BY user_email, role_id, scope
139-
HAVING COUNT(*) > 1
140-
) dupes ON ur.user_email = dupes.user_email
141-
AND ur.role_id = dupes.role_id
142-
AND ur.scope = dupes.scope
143-
AND ur.scope_id IS NULL
144-
AND ur.granted_at > dupes.oldest_granted_at
145-
WHERE ur.is_active = 1
125+
) ranked
126+
WHERE rn > 1
146127
)
147128
""")
148129

149130
result = bind.execute(dedupe_user_roles_null_scope_sql)
150131
deduped_user_roles_null = result.rowcount
151132

152-
# Handle scope_id IS NOT NULL case
133+
# Handle scope_id IS NOT NULL case - use row_number() for deterministic tie-breaking
153134
if dialect == 'postgresql':
154135
dedupe_user_roles_with_scope_sql = text("""
155136
UPDATE user_roles
156137
SET is_active = false
157138
WHERE id IN (
158-
SELECT ur.id
159-
FROM user_roles ur
160-
INNER JOIN (
161-
SELECT user_email, role_id, scope, scope_id, MIN(granted_at) as oldest_granted_at
139+
SELECT id
140+
FROM (
141+
SELECT id,
142+
ROW_NUMBER() OVER (PARTITION BY user_email, role_id, scope, scope_id ORDER BY granted_at, id) as rn
162143
FROM user_roles
163144
WHERE is_active = true AND scope_id IS NOT NULL
164-
GROUP BY user_email, role_id, scope, scope_id
165-
HAVING COUNT(*) > 1
166-
) dupes ON ur.user_email = dupes.user_email
167-
AND ur.role_id = dupes.role_id
168-
AND ur.scope = dupes.scope
169-
AND ur.scope_id = dupes.scope_id
170-
AND ur.granted_at > dupes.oldest_granted_at
171-
WHERE ur.is_active = true
145+
) ranked
146+
WHERE rn > 1
172147
)
173148
""")
174149
else: # SQLite
175150
dedupe_user_roles_with_scope_sql = text("""
176151
UPDATE user_roles
177152
SET is_active = 0
178153
WHERE id IN (
179-
SELECT ur.id
180-
FROM user_roles ur
181-
INNER JOIN (
182-
SELECT user_email, role_id, scope, scope_id, MIN(granted_at) as oldest_granted_at
154+
SELECT id
155+
FROM (
156+
SELECT id,
157+
ROW_NUMBER() OVER (PARTITION BY user_email, role_id, scope, scope_id ORDER BY granted_at, id) as rn
183158
FROM user_roles
184159
WHERE is_active = 1 AND scope_id IS NOT NULL
185-
GROUP BY user_email, role_id, scope, scope_id
186-
HAVING COUNT(*) > 1
187-
) dupes ON ur.user_email = dupes.user_email
188-
AND ur.role_id = dupes.role_id
189-
AND ur.scope = dupes.scope
190-
AND ur.scope_id = dupes.scope_id
191-
AND ur.granted_at > dupes.oldest_granted_at
192-
WHERE ur.is_active = 1
160+
) ranked
161+
WHERE rn > 1
193162
)
194163
""")
195164

‎mcpgateway/db.py‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1193,10 +1193,28 @@ class UserRole(Base):
11931193

11941194
__tablename__ = "user_roles"
11951195
__table_args__ = (
1196-
# Partial unique indexes: only one active assignment per (user, role, scope, scope_id) combination
1196+
# Partial unique indexes: only one active NON-EXPIRED assignment per (user, role, scope, scope_id) combination
11971197
# Need two separate indexes to handle NULL vs non-NULL scope_id cases (SQL NULL != NULL semantics)
1198-
Index("uq_user_roles_email_role_scope_null_active", "user_email", "role_id", "scope", unique=True, postgresql_where=text("scope_id IS NULL AND is_active = true"), sqlite_where=text("scope_id IS NULL AND is_active = 1")),
1199-
Index("uq_user_roles_email_role_scope_id_active", "user_email", "role_id", "scope", "scope_id", unique=True, postgresql_where=text("scope_id IS NOT NULL AND is_active = true"), sqlite_where=text("scope_id IS NOT NULL AND is_active = 1")),
1198+
# Excludes expired assignments: is_active = true AND (expires_at IS NULL OR expires_at > CURRENT_TIMESTAMP)
1199+
Index(
1200+
"uq_user_roles_email_role_scope_null_active",
1201+
"user_email",
1202+
"role_id",
1203+
"scope",
1204+
unique=True,
1205+
postgresql_where=text("scope_id IS NULL AND is_active = true AND (expires_at IS NULL OR expires_at > CURRENT_TIMESTAMP)"),
1206+
sqlite_where=text("scope_id IS NULL AND is_active = 1 AND (expires_at IS NULL OR expires_at > datetime('now'))"),
1207+
),
1208+
Index(
1209+
"uq_user_roles_email_role_scope_id_active",
1210+
"user_email",
1211+
"role_id",
1212+
"scope",
1213+
"scope_id",
1214+
unique=True,
1215+
postgresql_where=text("scope_id IS NOT NULL AND is_active = true AND (expires_at IS NULL OR expires_at > CURRENT_TIMESTAMP)"),
1216+
sqlite_where=text("scope_id IS NOT NULL AND is_active = 1 AND (expires_at IS NULL OR expires_at > datetime('now'))"),
1217+
),
12001218
)
12011219

12021220
# Primary key

0 commit comments

Comments
 (0)