Skip to content

Commit fd5bd59

Browse files
committed
fix(db): chain users migration off c7d8e9f0a1b2; persist dev user on NO_LOGIN (#586)
Address review on #980: - Drop obsolete merge revision f0e1d2c3b4a5; main already merges the two embedding heads via c7d8e9f0a1b2 (#979). Second merge left two Alembic heads. - Point users migration at c7d8e9f0a1b2 and update the Revises docstring. Verified: flask db heads reports a single head; upgrade/downgrade/upgrade clean on Postgres. - NO_LOGIN dev login now upserts the User and sets session['user_id'] when CRE_ENABLE_LOGIN is on, so local dev sessions have a row for user-scoped endpoints. Covered by tests for flag-on and flag-off. - Narrow login persistence except from Exception to SQLAlchemyError.
1 parent 137f17e commit fd5bd59

4 files changed

Lines changed: 64 additions & 32 deletions

File tree

application/tests/web_main_test.py

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1219,6 +1219,47 @@ def test_callback_does_not_persist_when_login_flag_off(
12191219

12201220
self.assertEqual(sqla.session.query(db.User).count(), 0)
12211221

1222+
def test_no_login_dev_path_persists_user_and_sets_session(self) -> None:
1223+
# NO_LOGIN=1 bypasses OAuth for local dev. It must still upsert a User
1224+
# and set session['user_id'], otherwise the dev session looks logged in
1225+
# but user-scoped endpoints have no row to hang a selection on.
1226+
self.app.secret_key = "test-secret"
1227+
with patch.dict(
1228+
os.environ,
1229+
{
1230+
"NO_LOGIN": "1",
1231+
"CRE_ENABLE_LOGIN": "1",
1232+
"NO_LOAD_GRAPH_DB": "1",
1233+
"INSECURE_REQUESTS": "1",
1234+
},
1235+
):
1236+
with self.app.test_client() as client:
1237+
client.get("/rest/v1/login")
1238+
with client.session_transaction() as sess:
1239+
self.assertIn("user_id", sess)
1240+
session_user_id = sess["user_id"]
1241+
1242+
users = sqla.session.query(db.User).all()
1243+
self.assertEqual(len(users), 1)
1244+
self.assertEqual(users[0].google_sub, "some dev id")
1245+
self.assertEqual(session_user_id, users[0].id)
1246+
1247+
def test_no_login_dev_path_does_not_persist_when_flag_off(self) -> None:
1248+
self.app.secret_key = "test-secret"
1249+
with patch.dict(
1250+
os.environ,
1251+
{
1252+
"NO_LOGIN": "1",
1253+
"NO_LOAD_GRAPH_DB": "1",
1254+
"INSECURE_REQUESTS": "1",
1255+
},
1256+
):
1257+
os.environ.pop("CRE_ENABLE_LOGIN", None)
1258+
with self.app.test_client() as client:
1259+
client.get("/rest/v1/login")
1260+
1261+
self.assertEqual(sqla.session.query(db.User).count(), 0)
1262+
12221263
@patch("application.web.web_main.id_token")
12231264
@patch("application.web.web_main.CREFlow")
12241265
def test_callback_skips_persistence_when_sub_missing(

application/web/web_main.py

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
)
4545
from google.oauth2 import id_token
4646
from google_auth_oauthlib.flow import Flow
47+
from sqlalchemy.exc import SQLAlchemyError
4748
from application.utils.spreadsheet import write_csv
4849
import oauthlib
4950
import google.auth.transport.requests
@@ -1165,6 +1166,21 @@ def login():
11651166
session["state"] = {"state": True}
11661167
session["google_id"] = "some dev id"
11671168
session["name"] = "dev user"
1169+
# Persist the dev account as well, so local/dev sessions carry a real
1170+
# user_id for user-scoped endpoints to hang data on. Without this the
1171+
# dev session looks logged in but has no User row. Mirrors the callback.
1172+
if is_login_enabled():
1173+
try:
1174+
user = db.Node_collection().upsert_user(
1175+
google_sub=session["google_id"],
1176+
email="",
1177+
display_name=session["name"],
1178+
)
1179+
session["user_id"] = user.id
1180+
except SQLAlchemyError as e:
1181+
logger.error(
1182+
"failed to persist dev user on login: %s", type(e).__name__
1183+
)
11681184
return redirect("/chatbot")
11691185
flow_instance = CREFlow.instance()
11701186
authorization_url, state = flow_instance.flow.authorization_url()
@@ -1236,9 +1252,11 @@ def callback():
12361252
display_name=id_info.get("name"),
12371253
)
12381254
session["user_id"] = user.id
1239-
except Exception as e:
1240-
# Avoid logging the raw exception: it can carry SQL parameters
1241-
# such as the user's email or OIDC subject. Log only the class.
1255+
except SQLAlchemyError as e:
1256+
# Keep DB failures soft so persistence can never block login, but
1257+
# let unexpected (non-DB) bugs surface instead of being swallowed.
1258+
# Log only the exception class: the message can carry SQL
1259+
# parameters such as the user's email or OIDC subject.
12421260
logger.error("failed to persist user on login: %s", type(e).__name__)
12431261
return redirect("/chatbot")
12441262

migrations/versions/a1b2c3d4e5f6_add_users_and_resource_selection.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
"""add users and user_resource_selection tables (issue #586)
22
33
Revision ID: a1b2c3d4e5f6
4-
Revises: f0e1d2c3b4a5
4+
Revises: c7d8e9f0a1b2
55
Create Date: 2026-07-08
66
77
"""
@@ -11,7 +11,7 @@
1111

1212

1313
revision = "a1b2c3d4e5f6"
14-
down_revision = "f0e1d2c3b4a5"
14+
down_revision = "c7d8e9f0a1b2"
1515
branch_labels = None
1616
depends_on = None
1717

migrations/versions/f0e1d2c3b4a5_merge_heads_before_users.py

Lines changed: 0 additions & 27 deletions
This file was deleted.

0 commit comments

Comments
 (0)