End a user's sessions when the account is deactivated

There is no token denylist in this product, so the JWT user loader is
the only place a deactivated account can be stopped, and it did a bare
lookup by id. Somebody deactivated at 09:00 kept a working access token
until it expired, and a refresh token that minted fresh ones for thirty
days.

/api/auth/refresh and the API-token path already checked isactive. This
makes the loader consistent with them rather than inventing policy.

Scope: product
(cherry picked from commit 9c1848fc61)

CHANGELOG.md

@@ -147,6 +147,13 @@ ADR-007 and ADR-002.
   `applications.download` permission, which is not grantable, so the catalogue
   metadata can still be opened to a site's floor without the bytes going with it.
 
+- **Deactivating an account did not end its sessions.** There is no token
+  denylist in this product, so the JWT user loader is the only place a
+  deactivated account can be stopped, and it did a bare lookup by id. Somebody
+  deactivated at 09:00 kept a working access token until it expired and a
+  refresh token that minted new ones for thirty days. `/api/auth/refresh` and
+  the API-token path already checked `isactive`; the loader now does too.
+
 - **The published site's API documentation page had no spec to show.**
   `shopdb/core/api/docs.py` reads `docs/openapi.json` at runtime to serve
   `/api/docs`, and publication excluded the whole of `docs/` because the prose

shopdb/__init__.py

@@ -98,7 +98,13 @@ from .plugins import plugin_manager
 # model internals. The endpoints plugin resolves a delivered script by name and
 # serves its body by digest to the agent. Additive name on the surface, minor
 # bump.
-__contract_version__ = '0.27.0'
+# 0.28.0: added client_ip and strip_port to shopdb.api so a plugin does not
+# hand-roll caller-IP extraction. Two plugins had, and both read the first
+# X-Forwarded-For hop, which the caller sends - so a rate limit keyed on it was
+# defeated by rotating the header. The helper resolves from remote_addr, which
+# ProxyFix has already filled from the header when TRUSTED_PROXY_COUNT says a
+# proxy is really in front. Additive names on the surface, minor bump.
+__contract_version__ = '0.28.0'
 
 # Product release version (see ADR-007). The product version and the
 # plugin-contract version above are distinct series with independent
@@ -211,7 +217,14 @@ def create_app(config_name: str = None) -> Flask:
     def user_lookup_callback(_jwt_header, jwt_data):
         from .core.models import User
         identity = jwt_data["sub"]
-        return db.session.get(User, int(identity))
+        user = db.session.get(User, int(identity))
+        # Deactivating an account has to end its sessions. There is no token
+        # denylist, so without this check the access token stays good until it
+        # expires and the refresh token keeps minting new ones for 30 days.
+        # /auth/refresh and the API-token path already check isactive.
+        if user is None or not user.isactive:
+            return None
+        return user
 
     @app.after_request
     def apply_security_headers(response):

tests/test_core/test_deactivated_user.py

@@ -0,0 +1,61 @@
+"""Deactivating an account ends its sessions.
+
+There is no token denylist in this product, so the JWT user loader is the only
+place a deactivated account can be stopped. It did a bare lookup by id, which
+meant deactivating somebody left their access token working until it expired and
+their refresh token minting new ones for thirty days. /auth/refresh and the API
+token path already checked isactive; the loader did not.
+"""
+from werkzeug.security import generate_password_hash
+
+
+def _login(client, username='deactivateme', password='testpass'):
+    response = client.post('/api/auth/login',
+                           json={'username': username, 'password': password})
+    assert response.status_code == 200, response.get_json()
+    return response.get_json()['data']
+
+
+def _make_user(db, username='deactivateme'):
+    from shopdb.core.models import User
+
+    user = User(
+        username=username,
+        email=f'{username}@test.local',
+        passwordhash=generate_password_hash('testpass'),
+    )
+    db.session.add(user)
+    db.session.commit()
+    return user
+
+
+def test_a_live_token_stops_working_once_the_account_is_deactivated(client, db):
+    user = _make_user(db)
+    tokens = _login(client)
+    headers = {'Authorization': 'Bearer {}'.format(tokens['access_token'])}
+
+    assert client.get('/api/auth/me', headers=headers).status_code == 200
+
+    user.isactive = False
+    db.session.commit()
+
+    assert client.get('/api/auth/me', headers=headers).status_code == 401
+
+
+def test_a_deactivated_account_cannot_refresh(client, db):
+    user = _make_user(db, 'refreshme')
+    tokens = _login(client, 'refreshme')
+    headers = {'Authorization': 'Bearer {}'.format(tokens['refresh_token'])}
+
+    user.isactive = False
+    db.session.commit()
+
+    assert client.post('/api/auth/refresh', headers=headers).status_code == 401
+
+
+def test_an_active_account_is_unaffected(client, db):
+    """The loader must not reject everybody; isactive defaults true."""
+    _make_user(db, 'stillhere')
+    tokens = _login(client, 'stillhere')
+    headers = {'Authorization': 'Bearer {}'.format(tokens['access_token'])}
+    assert client.get('/api/auth/me', headers=headers).status_code == 200