Show account names and roles on cards from scheduled scans

Cards from scheduled scans of editor-saved profiles showed the account's
GUID as its name and no Elev/Ansat badge: the profile editor stored bare
id strings, the scheduler passes them straight to run_scan, and run_scan
normalised them to displayName=<id>, userRole="other". Before 1.7.10 the
scan-start profile overwrite kept repairing profiles to named objects,
which is why this surfaced now.

- run_scan: _enrich_bare_users() looks bare accounts up in the directory
  (list_users, falling back to lookup_user) for name and role.
- M365Connector.build_role_sku_map(): the SKU map /api/users built inline,
  now shared so cards and the account list classify roles identically.
  lookup_user() also returns assignedLicenses.
- Profile editor saves {id, displayName, userRole} (manual accounts keep
  email/manual), like the 💾 save.
- _accountPill: an account_name equal to account_id counts as missing;
  name and role are filled from the loaded user list, so existing cards
  display correctly without a rescan.
- Excel export: the scan's account-name map no longer breaks on bare id
  strings (.get() on a str raised and dropped the whole map).

Tests: tests/test_account_names.py (10); the run_scan test fails on the
old engine with "Collecting emails — guid-henrik…".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
StyxX65andClaude Opus 5.5 committed 2026-09-28 14:40:14 +02:00
1 parent c55db48894
commit 20156ffc93
8 files changed
+283 -37

No files matched your search

+5 -1
View File
@@ -744,9 +744,13 @@ def _build_article30_docx() -> tuple[bytes, str]:
# user_ids are stored in the options JSON column
opts_json = json.loads(row["options"] or "{}") if row else {}
for u in opts_json.get("user_ids", []):
# Scheduled scans of editor-saved profiles stored bare id
# strings; .get() on those raised and dropped the whole map.
if not isinstance(u, dict):
continue
uid = u.get("id", "")
name = u.get("displayName", "")
if uid and name:
if uid and name and name != uid:
_acct_map[uid] = name
except Exception:
pass
+25 -1
View File
@@ -226,7 +226,8 @@ class M365Connector:
def lookup_user(self, upn: str) -> dict:
"""Look up one directory user by UPN or email (requires User.Read.All)."""
return self._get(f"/users/{upn}", {"$select": "id,displayName,mail,userPrincipalName"})
return self._get(f"/users/{upn}",
{"$select": "id,displayName,mail,userPrincipalName,assignedLicenses"})
def list_users(self, top: int = 999) -> list:
"""List licensed domain users in the tenant (requires User.Read.All).
@@ -1156,6 +1157,29 @@ class M365Connector:
break
return result
def build_role_sku_map(self, users: list) -> dict:
"""The {skuId: skuPartNumber} map classify_user_role() needs.
Tenant SKUs (get_subscribed_skus) merged with a sample of per-user
licenseDetails — the tenant call may only return the admin's own SKUs.
Shared by GET /api/users and the scan engine so both classify roles
the same way. Never raises; an empty map means ID-only classification.
"""
try:
sku_map = self.get_subscribed_skus()
except Exception:
sku_map = {}
try:
per_user = self.build_sku_map_from_users(users)
if per_user:
added = len(set(per_user) - set(sku_map))
sku_map.update(per_user)
if added:
logger.info("[skus] merged %d additional SKU(s) from per-user licenseDetails", added)
except Exception:
pass
return sku_map
def classify_user_role(self, assigned_licenses: list,
sku_map: dict) -> str:
"""Return 'student', 'staff', or 'other' based on assigned O365 licenses.
+3 -19
View File
@@ -24,25 +24,9 @@ def get_users():
out = []
seen = set()
# Build SKU map for role classification.
# get_subscribed_skus() tries /subscribedSkus → /me/licenseDetails.
# Then always merge per-user licenseDetails on top — this ensures we
# have skuPartNumbers for every distinct SKU in the tenant, not just
# the admin's own license (which is all /me/licenseDetails returns).
try:
sku_map = state.connector.get_subscribed_skus()
except Exception:
sku_map = {}
try:
per_user = state.connector.build_sku_map_from_users(users)
if per_user:
added = len(set(per_user) - set(sku_map))
sku_map.update(per_user)
if added:
logger.info("[skus] merged %d additional SKU(s) from per-user licenseDetails", added)
except Exception:
pass
# SKU map for role classification — shared with the scan engine so the
# account list and scanned cards classify Elev/Ansat identically.
sku_map = state.connector.build_role_sku_map(users)
# Load any manual role overrides set by the admin
_role_overrides = _load_role_overrides()
+44
View File
@@ -87,6 +87,49 @@ def _resolve_scan_users(conn, user_ids: list[dict]) -> list[dict]:
return resolved
def _enrich_bare_users(conn, user_ids: list[dict]) -> list[dict]:
"""Fill in displayName and userRole for accounts given only by id.
Profiles saved by the profile editor (and scheduled scans of them) pass
plain id strings, which run_scan normalises to displayName=<id>,
userRole="other" — so their cards showed the GUID as the name and no
Elev/Ansat badge. Look those accounts up in the directory once, using the
same role classification as GET /api/users. Accounts that can't be looked
up keep what they had.
"""
bare = {id(u) for u in user_ids
if not u.get("displayName") or u.get("displayName") == u.get("id")}
if not bare:
return user_ids
directory: dict[str, dict] = {}
sku_map: dict = {}
try:
users = conn.list_users()
directory = {u["id"]: u for u in users if u.get("id")}
sku_map = conn.build_role_sku_map(users)
except Exception as e: # e.g. delegated mode without User.Read.All
logger.info("[run_scan] directory list unavailable for name lookup: %s", e)
enriched = []
for u in user_ids:
if id(u) in bare:
uid = u.get("id", "")
d = directory.get(uid)
if d is None:
try:
d = conn.lookup_user(uid) # not in the licensed-member list
except Exception:
d = None
if d:
_email = d.get("mail") or d.get("userPrincipalName", "")
u = {**u, "displayName": _resolve_display_name(d.get("displayName", ""), _email, uid)}
if d.get("assignedLicenses") is not None and u.get("userRole", "other") == "other":
u["userRole"] = conn.classify_user_role(d.get("assignedLicenses", []), sku_map)
enriched.append(u)
return enriched
def _get_scan_abort():
return _state._scan_abort
@@ -686,6 +729,7 @@ def run_scan(options: dict):
me_info.get("mail") or me_info.get("userPrincipalName", "me"))}]
else:
user_ids = _resolve_scan_users(conn, user_ids)
user_ids = _enrich_bare_users(conn, user_ids)
sample = user_ids[0] if user_ids else None
logger.info("[run_scan] user_ids: %d entries, type=%s, sample=%s",
len(user_ids), type(user_ids).__name__, sample)
+2 -2
View File
@@ -57,7 +57,7 @@ Never revert to `!!window._googleConnected` / `_fileSources.length > 0` — thos
## Card user/group badge — results.js
- **`_accountPill(f)`** builds the account/role pill for both card layouts (list + grid). The **group badge is driven by `f.user_role`** (`student`/`staff`) alone, so it renders even with no display name — items from scans saved before `account_name` was persisted (DB migration 11) have only `user_role` + `account_id`. The user label resolves best-effort: `f.account_name` → `S._allUsers` match (by `id` or `email`) → email-style `account_id` → omit. Do not re-nest the role badge inside an `account_name` check (the old bug) — that hides the group badge for legacy items. Both layouts call `_accountPill(f)`; keep them sharing the one helper.
- **`_accountPill(f)`** builds the account/role pill for both card layouts (list + grid). The **group badge is driven by `f.user_role`** (`student`/`staff`) alone, so it renders even with no display name — items from scans saved before `account_name` was persisted (DB migration 11) have only `user_role` + `account_id`. The user label resolves best-effort: `f.account_name` → `S._allUsers` match (by `id` or `email`) → email-style `account_id` → omit. An `account_name` equal to `account_id` (a GUID stored by older scheduled scans of editor-saved profiles) counts as missing, and a role other than `student`/`staff` is filled from the same `S._allUsers` match. Do not re-nest the role badge inside an `account_name` check (the old bug) — that hides the group badge for legacy items. Both layouts call `_accountPill(f)`; keep them sharing the one helper.
## CPR cross-referencing — results.js
@@ -85,7 +85,7 @@ Never revert to `!!window._googleConnected` / `_fileSources.length > 0` — thos
- **`scheduler.js` strings must use `t()`** — frequency labels, "Next", "Running...", "Disabled", empty-job text, and empty-history text all have translation keys. Do not hard-code English strings in `schedLoad()` or `schedRenderJobs()`.
- **Scheduler UI — `schedToggleReportOnly()`** — dims the Profile row, shows/hides `#schedReportOnlyHint`, and forces `#schedAutoEmail` checked. Called from the checkbox `onchange` handler and at the start of `schedAddJob()` / `schedEditJob()`.
- **Profile editor accounts** — default to unchecked. Only explicitly saved `user_ids` are checked. Saved IDs not in `S._allUsers` (`manual:<email>`, users not loaded yet, users gone from the directory) are rendered by `_unresolvedAccountRows()` as checked rows so the next save keeps them — do not go back to building rows from `S._allUsers` alone (that silently dropped them on save). `user_ids: "all"` is kept as `"all"` when saved before users have loaded.
- **Profile editor accounts** — default to unchecked. Only explicitly saved `user_ids` are checked. Saved IDs not in `S._allUsers` (`manual:<email>`, users not loaded yet, users gone from the directory) are rendered by `_unresolvedAccountRows()` as checked rows so the next save keeps them — do not go back to building rows from `S._allUsers` alone (that silently dropped them on save). `user_ids: "all"` is kept as `"all"` when saved before users have loaded. The editor saves each account as `{id, displayName, userRole}` (plus `email`/`manual` for manual accounts), the same shape as 💾 — not a bare id string: scheduled scans pass profile accounts straight to `run_scan`, and bare ids produced GUID-named cards with no role badge. Server side, `scan_engine._enrich_bare_users()` names any bare ids that remain (older profiles) using `conn.build_role_sku_map()`, the same role classification as `GET /api/users`.
- **Manual accounts must be scannable** — they aren't in the `/api/users` directory list, so:
- `getSelectedUsers()` treats a missing `platform` as `'m365'`. Sidebar `addUserManually()` users historically had no platform and were silently excluded from every scan. Do not go back to a strict `u.platform === 'm365'` check.
- The profile editor's "+ Tilføj konto" looks the email up via `/api/users/lookup` and stores `{id: <directory id>, displayName, email, manual: true}`. It falls back to `"manual:<email>"` only when the lookup fails. Checkboxes with `data-manual` save as objects; everything else saves as plain id strings.
+18 -7
View File
@@ -437,8 +437,9 @@ function _openEditorForProfile(profile) {
const accountRows = S._allUsers.map(u => {
// Only check if the user was explicitly saved — default to unchecked like the main window
const checked = isSaved(u) ? 'checked' : '';
const manualAttrs = u.manual
? ` data-manual="1" data-name="${_esc(u.displayName || '')}" data-email="${_esc(u.email || '')}"` : '';
// data-name lets the save keep the display name with the id (see _pmgmtSaveFullEdit)
const manualAttrs = ` data-name="${_esc(u.displayName || '')}"` + (u.manual
? ` data-manual="1" data-email="${_esc(u.email || '')}"` : '');
const platBadge = u.platform === 'both' ? '<span style="font-size:9px;padding:1px 5px;border-radius:10px;background:linear-gradient(90deg,#E6F1FB 50%,#EAF3DE 50%);color:#1a4a1a;font-weight:500;border:0.5px solid #b5d4b5">M365+GWS</span>'
: (u.platform || 'm365') === 'google' ? '<span style="font-size:9px;padding:1px 5px;border-radius:10px;background:#EAF3DE;color:#3B6D11;font-weight:500">GWS</span>'
: '<span style="font-size:9px;padding:1px 5px;border-radius:10px;background:#E6F1FB;color:#185FA5;font-weight:500">M365</span>';
@@ -550,8 +551,9 @@ function _unresolvedAccountRows(savedList) {
const label = (typeof u === 'object' && u.displayName) || id.replace(/^(manual|google):/, '');
const badge = isManual ? t('m365_profile_acct_manual', 'Manual')
: t('m365_profile_acct_unresolved', 'Not loaded');
const manualAttrs = _isManualEntry(u) && typeof u === 'object'
? ` data-manual="1" data-name="${_esc(u.displayName || '')}" data-email="${_esc(u.email || '')}"` : '';
const savedName = typeof u === 'object' && u.displayName && u.displayName !== id ? u.displayName : '';
const manualAttrs = (savedName ? ` data-name="${_esc(savedName)}"` : '') + (isManual && typeof u === 'object'
? ` data-manual="1" data-email="${_esc(u.email || '')}"` : '');
return `<label class="pmgmt-acct-row" data-uid="${_esc(id)}" data-role="${_esc((typeof u === 'object' && u.userRole) || 'other')}"><input type="checkbox" checked data-uid="${_esc(id)}"${manualAttrs}><span style="flex:1;color:var(--text);overflow:hidden;text-overflow:ellipsis;white-space:nowrap" title="${_esc(id)}">${_esc(label)}</span><span style="font-size:9px;padding:1px 5px;border-radius:10px;background:#D3D1C7;color:#444441">${_esc(badge)}</span></label>`;
}).join('');
}
@@ -737,9 +739,18 @@ async function _pmgmtSaveFullEdit() {
const allSources = m365Sources.concat(effectiveGoogleSources).concat(effectiveFileSources);
let user_ids = Array.from(document.querySelectorAll('#pmgmtAcctList input[type=checkbox]:checked'))
.filter(cb => cb.dataset.uid)
.map(cb => cb.dataset.manual
? {id: cb.dataset.uid, displayName: cb.dataset.name || '', email: cb.dataset.email || '', manual: true}
: cb.dataset.uid);
.map(cb => {
// Save name + role with the id (same shape as 💾 / getSelectedUsers()):
// scheduled scans pass these straight to the scan, and a bare id became
// the card's "name" with no Elev/Ansat badge.
const role = cb.closest('label')?.dataset.role || 'other';
if (cb.dataset.manual) {
return {id: cb.dataset.uid, displayName: cb.dataset.name || '', email: cb.dataset.email || '',
userRole: role, manual: true};
}
return cb.dataset.name ? {id: cb.dataset.uid, displayName: cb.dataset.name, userRole: role}
: cb.dataset.uid;
});
// "all" profile opened before users loaded: there were no rows to tick, so keep "all"
if (window._pmgmtSavedAll && !S._allUsers.length && !user_ids.length) user_ids = 'all';
const updated = {
+17 -7
View File
@@ -32,21 +32,31 @@ const SOURCE_BADGES = {
// email), falling back to an email-style account_id. Returns '' when there is
// neither a label nor a role to show.
function _accountPill(f) {
const roleBadge =
f.user_role === 'student' ? '<span class="role-badge">' + t('role_student', 'Elev') + '</span>' :
f.user_role === 'staff' ? '<span class="role-badge">' + t('role_staff', 'Ansat') + '</span>' : '';
let role = f.user_role;
let label = f.account_name || '';
if (!label && f.account_id) {
// Scheduled scans of editor-saved profiles stored the account id (a GUID) as
// the name, with role "other" — treat that like a missing name.
if (label && f.account_id && label === String(f.account_id)) label = '';
const needName = !label && f.account_id;
const needRole = role !== 'student' && role !== 'staff';
if (f.account_id && (needName || needRole)) {
const aid = String(f.account_id);
const u = (S._allUsers || []).find(function(u) {
return u.id === f.account_id ||
(u.email && u.email.toLowerCase() === aid.toLowerCase());
});
if (u) label = u.displayName || '';
else if (aid.includes('@')) label = aid; // an email is already human-readable
if (u) {
if (needName) label = u.displayName || '';
if (needRole) role = u.userRole;
} else if (needName && aid.includes('@')) {
label = aid; // an email is already human-readable
}
}
const roleBadge =
role === 'student' ? '<span class="role-badge">' + t('role_student', 'Elev') + '</span>' :
role === 'staff' ? '<span class="role-badge">' + t('role_staff', 'Ansat') + '</span>' : '';
if (!label && !roleBadge) return '';
const title = label || f.user_role || '';
const title = label || role || '';
return '<span class="account-pill" title="' + esc(title) + '">' + roleBadge + (label ? esc(label) : '') + '</span>';
}
+169
View File
@@ -0,0 +1,169 @@
"""
Result cards must show the account's name and Elev/Ansat role.
Regression: profiles saved by the profile editor stored bare id strings, and
scheduled scans pass those straight to run_scan, which normalised them to
displayName=<id>, userRole="other" — so cards showed the GUID as the name and
no role badge. run_scan now looks bare accounts up in the directory.
"""
from __future__ import annotations
from unittest.mock import MagicMock
import pytest
import scan_engine
class _Conn:
"""Directory stub: list_users + role classification + single lookups."""
def __init__(self, directory=None, extra=None, list_fails=False):
self.directory = directory or []
self.extra = extra or {} # users only reachable via lookup_user
self.list_fails = list_fails
self.list_calls = 0
self.lookups: list[str] = []
def list_users(self):
self.list_calls += 1
if self.list_fails:
raise RuntimeError("Authorization_RequestDenied")
return list(self.directory)
def build_role_sku_map(self, users):
return {"sku-staff": "STANDARDWOFFPACK_FACULTY", "sku-stud": "STANDARDWOFFPACK_STUDENT"}
def classify_user_role(self, assigned_licenses, sku_map):
names = {sku_map.get(l.get("skuId", ""), "") for l in assigned_licenses}
if any("FACULTY" in n for n in names):
return "staff"
if any("STUDENT" in n for n in names):
return "student"
return "other"
def lookup_user(self, uid):
self.lookups.append(uid)
if uid in self.extra:
return self.extra[uid]
raise RuntimeError("Request_ResourceNotFound")
DIRECTORY = [
{"id": "guid-henrik", "displayName": "Henrik Højmark", "mail": "henrik@school.dk",
"assignedLicenses": [{"skuId": "sku-staff"}]},
{"id": "guid-elna", "displayName": "Elna Sofie", "mail": "elna@school.dk",
"assignedLicenses": [{"skuId": "sku-stud"}]},
]
def _bare(uid): # what run_scan makes of a bare id string
return {"id": uid, "displayName": uid, "userRole": "other"}
class TestEnrichBareUsers:
def test_bare_ids_get_name_and_role_from_directory(self):
out = scan_engine._enrich_bare_users(_Conn(DIRECTORY), [_bare("guid-henrik"), _bare("guid-elna")])
assert out == [
{"id": "guid-henrik", "displayName": "Henrik Højmark", "userRole": "staff"},
{"id": "guid-elna", "displayName": "Elna Sofie", "userRole": "student"},
]
def test_named_accounts_untouched_and_directory_not_listed(self):
conn = _Conn(DIRECTORY)
users = [{"id": "guid-henrik", "displayName": "Henrik Højmark", "userRole": "staff"}]
assert scan_engine._enrich_bare_users(conn, users) == users
assert conn.list_calls == 0 # no directory call when nothing is bare
def test_existing_role_is_kept(self):
# e.g. a role the UI already knew — only a missing ("other") role is filled
out = scan_engine._enrich_bare_users(
_Conn(DIRECTORY), [{"id": "guid-elna", "displayName": "guid-elna", "userRole": "staff"}])
assert out[0]["displayName"] == "Elna Sofie" and out[0]["userRole"] == "staff"
def test_account_outside_member_list_is_looked_up(self):
extra = {"guid-guest": {"id": "guid-guest", "displayName": "Gæst Lærer", "mail": "g@school.dk",
"assignedLicenses": [{"skuId": "sku-staff"}]}}
conn = _Conn(DIRECTORY, extra=extra)
out = scan_engine._enrich_bare_users(conn, [_bare("guid-guest")])
assert out == [{"id": "guid-guest", "displayName": "Gæst Lærer", "userRole": "staff"}]
assert conn.lookups == ["guid-guest"]
def test_directory_list_unavailable_falls_back_to_lookup(self):
extra = {"guid-henrik": DIRECTORY[0]}
out = scan_engine._enrich_bare_users(_Conn(list_fails=True, extra=extra), [_bare("guid-henrik")])
assert out[0]["displayName"] == "Henrik Højmark"
def test_unknown_account_keeps_what_it_had(self):
out = scan_engine._enrich_bare_users(_Conn(DIRECTORY), [_bare("guid-gone")])
assert out == [_bare("guid-gone")]
class TestBuildRoleSkuMap:
def _conn(self):
from m365_connector import M365Connector
return M365Connector.__new__(M365Connector) # no auth needed for these calls
def test_merges_tenant_and_per_user_skus(self, monkeypatch):
conn = self._conn()
monkeypatch.setattr(conn, "get_subscribed_skus", lambda: {"a": "ADMIN_SKU"})
monkeypatch.setattr(conn, "build_sku_map_from_users", lambda users: {"b": "FACULTY_SKU"})
assert conn.build_role_sku_map([{"id": "x"}]) == {"a": "ADMIN_SKU", "b": "FACULTY_SKU"}
def test_never_raises(self, monkeypatch):
conn = self._conn()
def _boom(*a, **kw): raise RuntimeError("403")
monkeypatch.setattr(conn, "get_subscribed_skus", _boom)
monkeypatch.setattr(conn, "build_sku_map_from_users", _boom)
assert conn.build_role_sku_map([]) == {}
class TestUsersRouteUsesSharedMap:
def test_api_users_classifies_with_build_role_sku_map(self, monkeypatch):
import gdpr_scanner
from routes import state
import routes.users as users_mod
monkeypatch.setattr(users_mod, "_load_role_overrides", lambda: {})
conn = MagicMock()
conn.is_app_mode = True
conn.list_users.return_value = DIRECTORY
conn.build_role_sku_map.return_value = {"sku-staff": "X_FACULTY"}
conn.classify_user_role.side_effect = lambda lic, m: "staff" if m else "other"
monkeypatch.setattr(state, "connector", conn)
gdpr_scanner.app.config["TESTING"] = True
with gdpr_scanner.app.test_client() as c:
data = c.get("/api/users").get_json()
conn.build_role_sku_map.assert_called_once_with(DIRECTORY)
assert {u["userRole"] for u in data["users"]} == {"staff"}
class TestRunScanNamesBareIds:
def test_scheduled_style_bare_ids_are_named_in_the_scan(self, monkeypatch):
"""End to end: bare id strings (as a scheduled scan passes them) reach the
per-user collection with their real name."""
from routes import state
import sse
sent = []
monkeypatch.setattr(scan_engine, "broadcast", lambda ev, data: sent.append((ev, data)))
monkeypatch.setattr(scan_engine, "DB_OK", False)
monkeypatch.setattr(scan_engine, "_load_checkpoint", lambda key, **kw: None)
monkeypatch.setattr(scan_engine, "_save_checkpoint", lambda *a, **kw: None)
monkeypatch.setattr(scan_engine, "_clear_checkpoint", lambda **kw: None)
monkeypatch.setattr(scan_engine, "_load_role_overrides", lambda: {})
monkeypatch.setattr(state, "flagged_items", [])
stub = _Conn(DIRECTORY)
conn = MagicMock()
conn.is_app_mode = True
conn.list_users.side_effect = stub.list_users
conn.build_role_sku_map.side_effect = stub.build_role_sku_map
conn.classify_user_role.side_effect = stub.classify_user_role
conn.list_all_mail_folders_for.return_value = []
monkeypatch.setattr(state, "connector", conn)
saved = sse._current_scan_id
try:
scan_engine.run_scan({"sources": ["email"], "options": {},
"user_ids": ["guid-henrik", "guid-elna"]})
finally:
sse._current_scan_id = saved
phases = " | ".join(d.get("phase", "") for e, d in sent if e == "scan_phase")
assert "Henrik Højmark" in phases and "Elna Sofie" in phases
assert "guid-henrik" not in phases