From d85da32ef5a41656e4dd78f5e4a1afcf585abbf0 Mon Sep 17 00:00:00 2001 From: GGThed Date: Sat, 8 Aug 2026 14:51:41 -0400 Subject: [PATCH] fix(arch): changement de role sans jeter la session, et en une transaction ARCH-008. `role` est le discriminateur polymorphe, et SQLAlchemy fixe la classe d une instance au chargement. edit_user ecrivait donc la colonne par un UPDATE de niveau instruction puis relisait la ligne -- c est correct. La suite ne l etait pas. db.session.commit() db.session.remove() # discard stale session entirely user = User.query.get(user_id_local) Deux defauts dans ces trois lignes. 1. remove() jette la session entiere. Tout ce que la requete tenait encore se retrouvait detache, current_user compris ; le moindre acces a un attribut ensuite levait DetachedInstanceError. La route ne survivait qu en ayant recopie le nom et l id de l acteur dans des variables locales avant -- un contournement, pas la correction. Un expunge de la seule instance perimee suffit. 2. Le commit intermediaire coupait l edition en deux. Le role etait acquis avant que le reste du formulaire soit applique : une erreur ensuite laissait un compte promu et le reste perdu, sans qu aucune interface ne le signale. Sans ce commit, l UPDATE reste dans la transaction, la relecture le voit, et l ensemble part en un seul commit. Les evenements d audit passent apres le commit. account.role_changed etait journalise avant l UPDATE : le journal affirmait un changement que la transaction pouvait encore annuler. tests/test_role_change.py, 6 tests. Un seul echoue sur le code d avant -- celui de l atomicite ; les cinq autres fixent le comportement qui marchait deja, pour que la suite de la vague D ne le casse pas. Co-Authored-By: Claude Opus 5 --- app/routes/users.py | 58 ++++++++++++------- tests/test_role_change.py | 119 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 21 deletions(-) create mode 100644 tests/test_role_change.py diff --git a/app/routes/users.py b/app/routes/users.py index 7218ffd..2f051d9 100644 --- a/app/routes/users.py +++ b/app/routes/users.py @@ -134,9 +134,6 @@ def edit_user(user_id): user = User.query.get_or_404(user_id) if request.method == 'POST': - # Captured up front, on purpose. A role change below calls - # db.session.remove(), which detaches current_user from the session: - # any later attribute access on it raises DetachedInstanceError. actor_name, actor_id = current_user.username, current_user.id def _rerender(): @@ -171,7 +168,10 @@ def edit_user(user_id): flash(_('Email already in use by another account.'), 'danger') return _rerender() - if user.role != role: + role_changed = user.role != role + previous_role = user.role + + if role_changed: # Two ways to lock everyone out of administration, neither of # which any interface can undo afterwards. if user.id == actor_id: @@ -190,23 +190,30 @@ def edit_user(user_id): 'another account before changing this one.'), 'danger') return _rerender() - # Change role via raw SQL to avoid polymorphic identity corruption. - # Must discard the entire session because the polymorphic discriminator - # change invalidates the identity map for this instance and anything - # that references it via relationships. - if user.role != role: - log_auth_event('account.role_changed', - actor=actor_name, actor_id=actor_id, - target=user.username, target_id=user.id, - previous_role=user.role, new_role=role) - user_id_local = user.id + if role_changed: + # The role column is the polymorphic discriminator, and SQLAlchemy + # decides an instance's class when it loads it. Assigning to it + # through the ORM leaves a Player object in the identity map for a + # row that now says 'coach', so every later isinstance() check — + # which is how this application does authorisation — answers with + # the old role. Hence the statement-level UPDATE. + # + # The instance then has to be re-read. This used to call + # db.session.remove(), which throws away the whole session: + # everything the request still held was detached, current_user + # included, and the next attribute access on any of them raised + # DetachedInstanceError. Expunging the one stale instance is + # enough, and it leaves the transaction open — so the role change + # and the rest of the edit now commit together instead of the + # role landing on its own and the remaining fields failing after + # it (ARCH-008). + user_pk = user.id db.session.execute( - db.text("UPDATE users SET role = :role WHERE id = :id"), - {"role": role, "id": user_id_local} + db.text('UPDATE users SET role = :role WHERE id = :id'), + {'role': role, 'id': user_pk}, ) - db.session.commit() - db.session.remove() # discard stale session entirely - user = User.query.get(user_id_local) # fresh session, correct class + db.session.expunge(user) + user = db.session.get(User, user_pk) user.full_name = full_name user.email = email @@ -224,11 +231,20 @@ def edit_user(user_id): password = validated.get('password') if password: user.password_hash = hash_password(password) + + db.session.commit() + + # Logged after the commit, not before: the audit trail should record + # what happened, and until this point nothing had. + if role_changed: + log_auth_event('account.role_changed', + actor=actor_name, actor_id=actor_id, + target=user.username, target_id=user.id, + previous_role=previous_role, new_role=role) + if password: log_auth_event('account.password_reset_by_admin', actor=actor_name, actor_id=actor_id, target=user.username, target_id=user.id) - - db.session.commit() log_auth_event('account.updated', actor=actor_name, actor_id=actor_id, target=user.username, target_id=user.id, active=is_active) diff --git a/tests/test_role_change.py b/tests/test_role_change.py new file mode 100644 index 0000000..e6f8443 --- /dev/null +++ b/tests/test_role_change.py @@ -0,0 +1,119 @@ +"""Changing a user's role — ARCH-008. + +`role` is the polymorphic discriminator, and SQLAlchemy fixes an instance's +class at load time. edit_user therefore writes the column with a +statement-level UPDATE and re-reads the row, which is correct. What it did +next was not: it called db.session.remove(), throwing away the whole +session, and it committed the role on its own before the rest of the edit +had been applied. + +The two consequences these tests pin down: + + - everything the request still held became detached, current_user + included. The route only survived because it had copied the actor's + name and id into locals beforehand — a workaround, in place of the fix. + - the role change was already committed when the rest of the edit ran, so + a failure afterwards left the account promoted and nothing else + applied. +""" + +import pytest + +from app.extensions import db +from app.models import Coach, Player, User + + +def _edit(client, user_id, **overrides): + payload = { + 'full_name': 'Promoted Person', + 'email': 'promoted@example.test', + 'role': 'coach', + 'is_active_account': 'on', + } + payload.update(overrides) + return client.post(f'/users/{user_id}/edit', data=payload, follow_redirects=True) + + +class TestPolymorphicIdentity: + def test_the_account_takes_its_new_class(self, app, client, as_role, make_user): + target_id = make_user('player') + as_role('admin') + + _edit(client, target_id) + + with app.app_context(): + assert isinstance(db.session.get(User, target_id), Coach) + + def test_the_new_role_grants_its_pages_right_away( + self, app, client, as_role, make_user, login + ): + """isinstance() is how this application authorises; a stale class in + the identity map is a stale permission set.""" + target_id = make_user('player') + as_role('admin') + _edit(client, target_id) + + with app.app_context(): + username = db.session.get(User, target_id).username + client.get('/auth/logout') + login(username) + + # Coach-only, and it is a coach's own page rather than a redirect. + assert client.get('/users/notes-dashboard').status_code == 200 + + def test_the_other_fields_of_the_edit_land_too( + self, app, client, as_role, make_user + ): + target_id = make_user('player') + as_role('admin') + + _edit(client, target_id, full_name='Renamed Too', phone='555-0100') + + with app.app_context(): + user = db.session.get(User, target_id) + assert user.full_name == 'Renamed Too' + assert user.phone == '555-0100' + + +class TestTheActingPresident: + """db.session.remove() detached current_user mid-request.""" + + def test_the_president_keeps_their_session(self, client, as_role, make_user): + target_id = make_user('player') + as_role('admin') + + _edit(client, target_id) + + assert client.get('/users').status_code == 200 + + def test_the_response_renders(self, client, as_role, make_user): + target_id = make_user('player') + as_role('admin') + + response = _edit(client, target_id) + + assert response.status_code == 200 + + +class TestAtomicity: + def test_a_failure_after_the_update_leaves_the_role_alone( + self, app, client, as_role, make_user, monkeypatch + ): + """The role used to be committed on its own, before the rest of the + edit ran. A failure afterwards left an account promoted, and no + interface reported it.""" + target_id = make_user('player') + as_role('admin') + + def _explode(user, selected_games): + raise RuntimeError('storage unavailable') + + monkeypatch.setattr('app.routes.users.update_user_gamertags', _explode) + + with pytest.raises(RuntimeError): + _edit(client, target_id) + + with app.app_context(): + user = db.session.get(User, target_id) + assert isinstance(user, Player) + assert user.role == 'player'