From 20156ffc9388d67bbd93d667cd092c04df5a9140 Mon Sep 17 00:00:00 2001 From: StyxX65 <150797939+StyxX65@users.noreply.github.com> Date: Mon, 28 Sep 2026 14:40:14 +0200 Subject: [PATCH] Show account names and roles on cards from scheduled scans MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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=, 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 --- gdpr_scanner.py | 6 +- m365_connector.py | 26 +++++- routes/users.py | 22 +---- scan_engine.py | 44 ++++++++++ static/js/CLAUDE.md | 4 +- static/js/profiles.js | 25 ++++-- static/js/results.js | 24 +++-- tests/test_account_names.py | 169 ++++++++++++++++++++++++++++++++++++ 8 files changed, 283 insertions(+), 37 deletions(-) create mode 100644 tests/test_account_names.py diff --git a/gdpr_scanner.py b/gdpr_scanner.py index e8f525e..cfccc72 100644 --- a/gdpr_scanner.py +++ b/gdpr_scanner.py @@ -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 diff --git a/m365_connector.py b/m365_connector.py index ce3c31a..48c00f8 100644 --- a/m365_connector.py +++ b/m365_connector.py @@ -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. diff --git a/routes/users.py b/routes/users.py index ebd6ed2..38fc261 100644 --- a/routes/users.py +++ b/routes/users.py @@ -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() diff --git a/scan_engine.py b/scan_engine.py index f1f429d..fbf64c3 100644 --- a/scan_engine.py +++ b/scan_engine.py @@ -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=, + 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) diff --git a/static/js/CLAUDE.md b/static/js/CLAUDE.md index 93ef6f6..c6fd362 100644 --- a/static/js/CLAUDE.md +++ b/static/js/CLAUDE.md @@ -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:`, 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:`, 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: , displayName, email, manual: true}`. It falls back to `"manual:"` only when the lookup fails. Checkboxes with `data-manual` save as objects; everything else saves as plain id strings. diff --git a/static/js/profiles.js b/static/js/profiles.js index f507cd5..6e37fbd 100644 --- a/static/js/profiles.js +++ b/static/js/profiles.js @@ -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' ? 'M365+GWS' : (u.platform || 'm365') === 'google' ? 'GWS' : 'M365'; @@ -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 ``; }).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 = { diff --git a/static/js/results.js b/static/js/results.js index c2eb1ff..72c6686 100644 --- a/static/js/results.js +++ b/static/js/results.js @@ -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' ? '' + t('role_student', 'Elev') + '' : - f.user_role === 'staff' ? '' + t('role_staff', 'Ansat') + '' : ''; + 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' ? '' + t('role_student', 'Elev') + '' : + role === 'staff' ? '' + t('role_staff', 'Ansat') + '' : ''; if (!label && !roleBadge) return ''; - const title = label || f.user_role || ''; + const title = label || role || ''; return ''; } diff --git a/tests/test_account_names.py b/tests/test_account_names.py new file mode 100644 index 0000000..ea50297 --- /dev/null +++ b/tests/test_account_names.py @@ -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=, 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