Skip to content

perf: the users page renders one edit modal, not one per user #74

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
FMSMITH91 merged 2 commits into from
Aug 9, 2026
Merged

perf: the users page renders one edit modal, not one per user #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b88f36c
perf: the users page renders one edit modal, not one per user
FMSMITH91 Aug 9, 2026
f62f0a5
fix: coerce the user id before it reaches the form action (js/xss-thr…
FMSMITH91 Aug 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions static/js/manage_users.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// Populate the one shared edit modal from the #users-data JSON island.
//
// This page used to render a full 2KB edit modal per user — ~210KB of duplicated markup at 100
// accounts, all of it for one dialog you can only have open once. The rows now carry an id and the
// data comes from a single JSON blob, which is ~120 bytes per user.
function _euUsers() {
var el = document.getElementById('users-data');
if (!el) return [];
try {
return JSON.parse(el.textContent) || [];
} catch (e) {
return []; // a malformed island must not take the page down with it
}
}

window.openEditUser = function (id) {
var u = null, all = _euUsers();
for (var i = 0; i < all.length; i++) {
if (all[i].id === id) { u = all[i]; break; }
}
if (!u) return;

// Coerce the id to a number before it reaches the form action. It is always an integer from our
// own database, but it arrives here as text read out of the DOM, and a form action is a URL sink
// — CodeQL flags that flow (js/xss-through-dom) and is right to. parseInt both proves the value
// cannot carry meta-characters and rejects a tampered island outright.
var uid = parseInt(u.id, 10);
if (!(uid > 0)) return;

var form = document.getElementById('edit-user-form');
form.setAttribute('action', (window.MOUNT || '') + '/users/' + uid + '/edit');

document.getElementById('eu-name').textContent = u.username; // textContent: never HTML
document.getElementById('eu-display').value = u.display_name || '';
document.getElementById('eu-email').value = u.email || '';
document.getElementById('eu-password').value = ''; // never prefill a password

var groups = u.groups || [];
document.querySelectorAll('.eu-group').forEach(function (cb) {
cb.checked = groups.indexOf(parseInt(cb.value, 10)) !== -1;
});

document.getElementById('eu-superadmin').checked = !!u.is_superadmin;
document.getElementById('eu-active').checked = !!u.is_active;

// The 2FA reset only makes sense for someone who has it on; it starts unchecked every time so a
// previous user's toggle can never carry over into the next one you open.
var tfa = document.getElementById('eu-2fa-block');
document.getElementById('eu-reset2fa').checked = false;
tfa.style.display = u.totp_enabled ? '' : 'none';

new bootstrap.Modal(document.getElementById('editUserModal')).show();
};
72 changes: 41 additions & 31 deletions templates/manage_users.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ <h1 class="page-title mb-0"><i class="bi bi-people-fill"></i> User Management</h
</td>
<td class="text-secondary small">{{ user.last_login|datetime if user.last_login else 'Never' }}</td>
<td class="text-end">
<button class="btn btn-outline-secondary btn-sm" data-bs-toggle="modal"
data-bs-target="#editUserModal-{{ user.id }}"
<button class="btn btn-outline-secondary btn-sm"
data-action="openEditUser" data-args='[{{ user.id }}]'
aria-label="Edit {{ user.username }}" title="Edit {{ user.username }}">
<i class="bi bi-pencil"></i>
</button>
Expand Down Expand Up @@ -148,29 +148,44 @@ <h5 class="modal-title"><i class="bi bi-person-plus-fill"></i> Add User</h5>
</div>
</div>

<!-- Edit User Modals -->
{% for user in users %}
<div class="modal fade" id="editUserModal-{{ user.id }}" tabindex="-1">
{# One row of JSON per user instead of one 2KB edit modal per user: at 100 accounts that was
~210KB of duplicated markup in the page. The single modal below is populated from this. It lives
INSIDE #users-list so an ajax refresh after a save keeps it current. #}
<script type="application/json" id="users-data" nonce="{{ csp_nonce }}">
[{% for user in users %}{"id": {{ user.id }},
"username": {{ user.username|tojson }},
"display_name": {{ (user.display_name or '')|tojson }},
"email": {{ (user.email_display or '')|tojson }},
"groups": {{ (user.groups|map(attribute='id')|list)|tojson }},
"is_superadmin": {{ 'true' if user.is_superadmin else 'false' }},
"is_active": {{ 'true' if user.is_active else 'false' }},
"totp_enabled": {{ 'true' if user.totp_enabled else 'false' }}}{{ "," if not loop.last }}{% endfor %}]
</script>
</div><!-- /#users-list -->

{# The single edit modal. Outside #users-list on purpose: an ajax refresh replaces that region, and
replacing a modal while it is open leaves Bootstrap's backdrop behind. #}
<div class="modal fade" id="editUserModal" tabindex="-1">
<div class="modal-dialog">
<div class="modal-content">
<form method="POST" action="{{ url_for('edit_user', user_id=user.id) }}"
<form method="POST" action="" id="edit-user-form"
class="ajax-form" data-ajax-refresh="#users-list" data-ajax-reset="off">
<div class="modal-header">
<h5 class="modal-title"><i class="bi bi-pencil"></i> Edit User: {{ user.username }}</h5>
<h5 class="modal-title"><i class="bi bi-pencil"></i> Edit User: <span id="eu-name"></span></h5>
<button type="button" class="btn-close btn-close-white" data-bs-dismiss="modal"></button>
</div>
<div class="modal-body">
<div class="mb-3">
<label class="form-label">Display Name</label>
<input type="text" name="display_name" class="form-control" value="{{ user.display_name }}">
<label class="form-label" for="eu-display">Display Name</label>
<input type="text" name="display_name" id="eu-display" class="form-control">
</div>
<div class="mb-3">
<label class="form-label">Email</label>
<input type="email" name="email" class="form-control" value="{{ user.email_display }}">
<label class="form-label" for="eu-email">Email</label>
<input type="email" name="email" id="eu-email" class="form-control">
</div>
<div class="mb-3">
<label class="form-label">New Password (leave blank to keep current)</label>
<input type="password" name="password" class="form-control" minlength="10">
<label class="form-label" for="eu-password">New Password (leave blank to keep current)</label>
<input type="password" name="password" id="eu-password" class="form-control" minlength="10">
<div class="form-text">At least 10 characters with upper &amp; lowercase, a number, and a symbol.</div>
</div>
<hr class="border-secondary">
Expand All @@ -182,20 +197,18 @@ <h5 class="modal-title"><i class="bi bi-pencil"></i> Edit User: {{ user.username
</div>
{% for g in groups %}
<div class="form-check">
<input type="checkbox" name="groups" value="{{ g.id }}" class="form-check-input"
id="edit-group-{{ user.id }}-{{ g.id }}"
{% if g in user.groups %}checked{% endif %}>
<label class="form-check-label" for="edit-group-{{ user.id }}-{{ g.id }}">{{ g.name }}</label>
<input type="checkbox" name="groups" value="{{ g.id }}" class="form-check-input eu-group"
id="eu-group-{{ g.id }}">
<label class="form-check-label" for="eu-group-{{ g.id }}">{{ g.name }}</label>
</div>
{% endfor %}
</div>
<hr class="border-secondary">
<div class="mb-3">
<label class="form-label mb-1">Account type</label>
<div class="form-check form-switch">
<input type="checkbox" name="is_superadmin" class="form-check-input" role="switch"
id="edit-superadmin-{{ user.id }}" {% if user.is_superadmin %}checked{% endif %}>
<label class="form-check-label" for="edit-superadmin-{{ user.id }}">Super Administrator</label>
<input type="checkbox" name="is_superadmin" class="form-check-input" role="switch" id="eu-superadmin">
<label class="form-check-label" for="eu-superadmin">Super Administrator</label>
</div>
<div class="form-text mt-0">
Ignores groups entirely — full control of everything. Leave off for a standard user.
Expand All @@ -205,27 +218,24 @@ <h5 class="modal-title"><i class="bi bi-pencil"></i> Edit User: {{ user.username
<div>
<label class="form-label mb-1">Status</label>
<div class="form-check form-switch">
<input type="checkbox" name="is_active" class="form-check-input" role="switch"
id="edit-active-{{ user.id }}" {% if user.is_active %}checked{% endif %}>
<label class="form-check-label" for="edit-active-{{ user.id }}">Active</label>
<input type="checkbox" name="is_active" class="form-check-input" role="switch" id="eu-active">
<label class="form-check-label" for="eu-active">Active</label>
</div>
<div class="form-text mt-0">
Uncheck to block this user from logging in, without deleting the account.
</div>
</div>
{% if user.totp_enabled %}
<hr class="border-secondary">
<div>
<div id="eu-2fa-block" style="display:none;">
<hr class="border-secondary">
<label class="form-label mb-1"><i class="bi bi-shield-lock"></i> Two-Factor Auth</label>
<div class="form-check form-switch">
<input type="checkbox" name="reset_2fa" class="form-check-input" role="switch" id="edit-reset2fa-{{ user.id }}">
<label class="form-check-label" for="edit-reset2fa-{{ user.id }}">Reset (disable) this user's 2FA</label>
<input type="checkbox" name="reset_2fa" class="form-check-input" role="switch" id="eu-reset2fa">
<label class="form-check-label" for="eu-reset2fa">Reset (disable) this user's 2FA</label>
</div>
<div class="form-text mt-0">
Use this if they've lost their authenticator. They can re-enable it from their Account page.
</div>
</div>
{% endif %}
</div>
<div class="modal-footer">
<button type="button" class="btn btn-outline-secondary" data-bs-dismiss="modal">Cancel</button>
Expand All @@ -235,7 +245,7 @@ <h5 class="modal-title"><i class="bi bi-pencil"></i> Edit User: {{ user.username
</div>
</div>
</div>
{% endfor %}
</div><!-- /#users-list -->

<script nonce="{{ csp_nonce }}" src="{{ asset_url('js/manage_users.js') }}"></script>

{% endblock %}
38 changes: 38 additions & 0 deletions tests/smoke_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1919,6 +1919,38 @@ def _banner_tag(html):
finally:
_am._cron_restart_pending.pop(gs_id, None)

# ── The users page renders ONE edit modal, not one per user ───────────────────────────────────
# It used to emit a full 2KB modal per row — 670KB of HTML at 100 accounts, all of it for a
# dialog you can only have open once. The rows now carry an id and the data comes from a single
# JSON island. These assert the shape holds AND that the data is actually right, because a
# smaller page that opens the wrong user's details would be a much worse bug than a big one.
import json as _json_u
_uh = c.get("/users").get_data(as_text=True)
check("users page: exactly one edit modal, however many accounts exist",
_uh.count('id="editUserModal"') == 1 and 'id="editUserModal-' not in _uh,
"found %d" % _uh.count('id="editUserModal'))
_isl = _re_ab.search(r'<script type="application/json" id="users-data"[^>]*>(.*?)</script>',
_uh, _re_ab.S)
check("users page: the JSON island is present", _isl is not None)
if _isl:
_rows = _json_u.loads(_isl.group(1)) # must be VALID json, not just present
with app.app_context():
# Materialise inside the context: .groups is a lazy relationship and reading it after
# the context closes raises DetachedInstanceError.
_want = {u.username: (bool(u.is_superadmin), bool(u.is_active),
sorted(g.id for g in u.groups)) for u in User.query.all()}
check("users page: one island entry per account", len(_rows) == len(_want),
"%d rows vs %d users" % (len(_rows), len(_want)))
_bad = [r["username"] for r in _rows
if (r["is_superadmin"], r["is_active"], sorted(r["groups"])) != _want[r["username"]]]
check("users page: each entry matches that account's real flags and groups", not _bad,
"wrong: %s" % _bad[:3])
check("users page: every row's Edit button opens the shared modal by id",
_uh.count('data-action="openEditUser"') == len(_rows),
"%d buttons for %d users" % (_uh.count('data-action="openEditUser"'), len(_rows)))
check("users page: no password is ever put in the island",
not any("password" in r for r in _rows))

# ── Bearer API tokens: the other way into every route ─────────────────────────────────────────
# A token authenticates AS its owner and inherits exactly that user's RBAC, and app.py exempts
# Bearer requests from CSRF — so this is a full authentication path that had no test at all.
Expand Down Expand Up @@ -2020,6 +2052,12 @@ def _bearer(tok):
check("custom command: a missing command is refused, not an exception",
_crcc(_adm, None, _gs) is False)

except Exception:
# A crash part-way through otherwise just prints fewer checks and still reads as green-ish.
# That has hidden three separate mistakes while writing these; a crash is a FAILURE.
import traceback as _tb
_tb.print_exc()
results.append((False, "suite crashed before finishing — see the traceback above", ""))
finally:
passed = sum(1 for ok, _, _ in results if ok)
for ok, name, detail in results:
Expand Down
7 changes: 6 additions & 1 deletion tests/template_actions_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,12 @@ def check(cond, name, detail=""):
# fine and looks harmless. Static analysers read the TEMPLATE, though, and to a JS parser "{#" is
# a private-field sigil: CodeQL raised two js/syntax-error alerts on exactly this. A whole file
# that fails to parse is a file nothing is checking, which is the real cost.
_INLINE_SCRIPT = re.compile(r"<script\b(?![^>]*\bsrc=)[^>]*>(.*?)</script>", re.S)
# Only script elements a JS parser actually reads: no src=, and either no type or a JS one.
# A <script type="application/json"> data island is not JavaScript — nothing parses it as JS,
# so Jinja inside it cannot produce the syntax error this check exists to prevent.
_INLINE_SCRIPT = re.compile(
r"<script\b(?![^>]*\bsrc=)(?![^>]*\btype=\"(?:application|text)/(?!javascript)[\w.+-]+\")"
r"[^>]*>(.*?)</script>", re.S)
_jinja_in_js = []
for _name, _src in srcs.items():
if not _name.endswith(".html"):
Expand Down