From 9c1848fc61658eee2d2ae10f9df065e48cd324f2 Mon Sep 17 00:00:00 2001 From: cproudlock Date: Mon, 31 Aug 2026 07:02:00 -0400 Subject: [PATCH] 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 --- CHANGELOG.md | 7 +++ shopdb/__init__.py | 17 ++++++- tests/test_core/test_deactivated_user.py | 61 ++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 tests/test_core/test_deactivated_user.py diff --git a/CHANGELOG.md b/CHANGELOG.md index f9f83525..64cd8cba 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/shopdb/__init__.py b/shopdb/__init__.py index 01a3bac5..55ada8fc 100644 --- a/shopdb/__init__.py +++ b/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): diff --git a/tests/test_core/test_deactivated_user.py b/tests/test_core/test_deactivated_user.py new file mode 100644 index 00000000..01b10432 --- /dev/null +++ b/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