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 <[email protected]>

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)

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': '[email protected]',
+        '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'