Skip to content

Commit 07c971b

Browse files
committed
fix(authentication): handle concurrent first-login race condition
When multiple servers process a user's first login simultaneously, each may query, find no existing record, and attempt to INSERT. The losing request raised an unhandled IntegrityError. - Wrap the commit in a try/except IntegrityError block - On collision: rollback and re-fetch the existing user record - Add a test that simulates the race and asserts the correct user is returned with exactly one DB row
1 parent 9247dca commit 07c971b

2 files changed

Lines changed: 61 additions & 3 deletions

File tree

src/mavedb/lib/authentication.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
HTTPBearer,
1414
)
1515
from jose import jwt
16+
from sqlalchemy.exc import IntegrityError
1617
from sqlalchemy.orm import Session
1718

1819
from mavedb import deps
@@ -230,7 +231,15 @@ async def get_current_user(
230231
)
231232

232233
db.add(user)
233-
db.commit()
234+
try:
235+
db.commit()
236+
except IntegrityError:
237+
# A concurrent request created this user between our initial query and this commit.
238+
# Roll back and re-fetch the existing record.
239+
db.rollback()
240+
user = db.query(User).filter(User.username == username).one()
241+
logger.debug(msg="Concurrent first-login resolved; returning existing user.", extra=logging_context())
242+
234243
db.refresh(user)
235244
logger.info(msg="Successfully authenticated user via JWT.", extra=logging_context())
236245

tests/lib/test_authentication.py

Lines changed: 51 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
# ruff: noqa: E402
22

3-
import pytest
43
from unittest.mock import patch
54

5+
import pytest
6+
from sqlalchemy.exc import IntegrityError
7+
68
arq = pytest.importorskip("arq")
79
cdot = pytest.importorskip("cdot")
810
fastapi = pytest.importorskip("fastapi")
@@ -11,7 +13,6 @@
1113
from mavedb.models.enums.user_role import UserRole
1214
from mavedb.models.user import User
1315
from tests.helpers.constants import ADMIN_USER, ADMIN_USER_DECODED_JWT, TEST_USER, TEST_USER_DECODED_JWT
14-
1516
from tests.helpers.util.access_key import create_api_key_for_user
1617
from tests.helpers.util.user import mark_user_inactive
1718

@@ -121,3 +122,51 @@ async def test_get_current_user_user_extraneous_roles(session, setup_lib_db):
121122

122123
assert user_data.user.username == TEST_USER["username"]
123124
assert user_data.active_roles == []
125+
126+
127+
@pytest.mark.asyncio
128+
async def test_get_current_user_concurrent_first_login_integrity_error_returns_existing_user(session, setup_lib_db):
129+
"""
130+
Simulate two servers racing on first login: the commit raises IntegrityError because a
131+
concurrent request already inserted the row. The handler should roll back and return the
132+
existing user rather than surfacing the error.
133+
"""
134+
new_user_jwt = {
135+
"sub": "9999-0000-0000-9999",
136+
"given_name": "Race",
137+
"family_name": "Condition",
138+
}
139+
140+
# Insert the user as if a concurrent request already committed it.
141+
pre_existing = User(
142+
username=new_user_jwt["sub"],
143+
first_name=new_user_jwt["given_name"],
144+
last_name=new_user_jwt["family_name"],
145+
is_active=True,
146+
is_first_login=True,
147+
)
148+
session.add(pre_existing)
149+
session.commit()
150+
151+
# Wrap the real session so we can intercept the first commit call and raise IntegrityError,
152+
# letting subsequent calls (rollback, refresh, etc.) pass through to the real session.
153+
original_commit = session.commit
154+
commit_calls = []
155+
156+
def fake_commit():
157+
commit_calls.append(1)
158+
if len(commit_calls) == 1:
159+
raise IntegrityError(statement=None, params=None, orig=Exception("duplicate key"))
160+
return original_commit()
161+
162+
session.commit = fake_commit
163+
164+
with patch("mavedb.lib.authentication.fetch_orcid_user_email", return_value=None):
165+
user_data = await get_current_user(None, new_user_jwt, session, None)
166+
167+
assert user_data is not None
168+
assert user_data.user.username == new_user_jwt["sub"]
169+
170+
# Only one user record should exist in the database.
171+
users = session.query(User).filter(User.username == new_user_jwt["sub"]).all()
172+
assert len(users) == 1

0 commit comments

Comments
 (0)